[Bug 735663] dashdemux: synchronize with the download loop thread before signalling it
GStreamer (bugzilla.gnome.org)
bugzilla at gnome.org
Tue Sep 9 04:08:50 PDT 2014
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.
More information about the gstreamer-bugs
mailing list