[Bug 735663] dashdemux: synchronize with the download loop thread before signalling it

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=735663
  GStreamer | gst-plugins-bad | git

Matthieu Bouron <matthieu.bouron> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |matthieu.bouron at collabora.c
                   |                            |om

--- Comment #5 from Matthieu Bouron <matthieu.bouron at collabora.com> 2014-09-09 11:08:46 UTC ---
(In reply to comment #4)
> Review of attachment 285496 [details]:
> 
> Very good catch!
> 
> I suggested some minor corrections below. See if you agree.
> 
> ::: ext/dash/gstdashdemux.c
> @@ +432,3 @@
> 
> +          g_mutex_lock (&stream->fragment_download_signaled_lock);
> +          stream->fragment_download_signaled = TRUE;
> 
> I think the signaling has to be done after this variable is set, inside the
> same lock.

Updated in the new version of the patch.

> 
> @@ +1089,3 @@
>      gst_element_set_state (stream->src, GST_STATE_READY);
>      g_cond_signal (&stream->fragment_download_cond);
> +    g_mutex_lock (&stream->fragment_download_signaled_lock);
> 
> Same as above.
> 
> @@ +2014,3 @@
>      g_cond_signal (&stream->fragment_download_cond);
> +    g_mutex_lock (&stream->fragment_download_signaled_lock);
> +    stream->fragment_download_signaled = TRUE;
> 
> Same here.
> 
> @@ +2034,3 @@
> +
> +      g_mutex_lock (&stream->fragment_download_signaled_lock);
> +      stream->fragment_download_signaled = TRUE;
> 
> Same.
> 
> @@ +2217,3 @@
>            "Waiting for fragment download to finish: %s", uri);
> +
> +      end_time = g_get_monotonic_time () + 5 * G_TIME_SPAN_MILLISECOND;
> 
> If at all, this should be configurable via a property.
> 
> I'd prefer to have this as a separate patch. Let's focus here on just adding
> the boolean flag and making sure it properly works.
> 
> ::: ext/dash/gstdashdemux.h
> @@ +87,3 @@
>    GMutex fragment_download_lock;
>    GCond fragment_download_cond;
> +  GMutex fragment_download_signaled_lock;
> 
> Any reason to add a new lock? Can't use the fragment_download_lock for the
> signaling and the variable? I think it makes sense to use the same.

Thanks for the review.

We need to introduce a new lock as gst_dash_demux_download_uri is called when
fragment_download_lock is already held. gst_dash_demux_download_uri calls
gst_element_sync_state_with_parent/gst_element_set_state which is likely to
call gst_dash_demux_handle_message. If the same lock is used > deadlock.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.