GNOME Bugzilla – Bug 755226
dashdemux: download_finish variable is not reset safely
Last modified: 2015-10-29 11:10:35 UTC
download_finished is set to FALSE when the streams are exposed. It is set to TRUE when the download is finished or when the task must stop. After the task has waited on download_finished, it needs to reset it to FALSE before it releases the fragment_download_lock. If it releases the lock and does the reset later, it might overwrite a stop request. The current implementation resets it before waiting on it and worse, does it without grabbing the lock, so it is very probable that it will override any stop requests.
Created attachment 311641 [details] [review] proposed patch
Created attachment 311743 [details] [review] proposed patch download_finished needs to be reset to false before streams are started, not when they are exposed. That's because it is possible to stop and start them without calling expose again (when handling seek events).
Seems OK, except for the last hunk, which I'm not sure why it needs resetting there.
(In reply to Vincent Penquerc'h from comment #3) > Seems OK, except for the last hunk, which I'm not sure why it needs > resetting there. I don't understand your question. As I described in the comment and in the bug report, if the download_finished is not reset while the lock is taken, it cannot be safely done later. The following scenario can occur: Thread A: gst_adaptive_demux_stream_download_uri g_cond_wait (&stream->fragment_download_cond, &stream->fragment_download_lock); g_mutex_unlock (&stream->fragment_download_lock); ... Thread B tries to stop the stream: g_mutex_lock (&stream->fragment_download_lock); stream->download_finished = TRUE; g_mutex_unlock (&stream->fragment_download_lock); ... Thread A, sometime later, tries to reset download_finished and will overwrite the stop request: g_mutex_lock (&stream->fragment_download_lock); stream->download_finished = FALSE; g_mutex_unlock (&stream->fragment_download_lock);
What I meant is, why does it need to be reset to FALSE in this function, and not why does it need to be reset here and not above. It will be reset when _start_tasks is called again. But I think I see why this needs to be so now, because another segment can be downloaded without _start_tasks being called again. In which case that explains what I was wondering.
(In reply to Vincent Penquerc'h from comment #5) > What I meant is, why does it need to be reset to FALSE in this function, and > not why does it need to be reset here and not above. It will be reset when > _start_tasks is called again. But I think I see why this needs to be so now, > because another segment can be downloaded without _start_tasks being called > again. In which case that explains what I was wondering. that's correct, there can be up to 3 downloads (initialization segment, index segment, media segment) for a single start_task call
This got subsumed in the fix for https://bugzilla.gnome.org/show_bug.cgi?id=755169