[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

Thiago Sousa Santos <thiagossantos> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #285496|none                        |reviewed
             status|                            |

--- Comment #4 from Thiago Sousa Santos <thiagossantos at gmail.com> 2014-09-09 05:34:16 UTC ---
Review of attachment 285496:
 --> (https://bugzilla.gnome.org/review?bug=735663&attachment=285496)

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.

@@ +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.

-- 
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.