[Bug 736655] basesink: preroll issue for some clips which audio is shorter than video

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=736655

Sebastian Dröge (slomo) <slomo at coaxion.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #295409|none                        |needs-work
             status|                            |

--- Comment #98 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
Review of attachment 295409:
 --> (https://bugzilla.gnome.org/review?bug=736655&attachment=295409)

Looks good, just some minor things that should be fast to update :) Then this
should be ready to be merged

::: gst/playback/gststreamsynchronizer.c
@@ +207,3 @@
+/* must be called with the STREAM_SYNCHRONIZER_LOCK */
+static gboolean
+gst_stream_synchronizer_wait (GstStreamSynchronizer * self, GstPad * pad)

Can this function also be used for the g_cond_wait() in the SEGMENT event
handler? I think it should be used there too for consistency (and modified to
work in both cases).

@@ +445,3 @@
       GST_STREAM_SYNCHRONIZER_LOCK (self);
       stream = gst_pad_get_element_private (pad);
+      stream->flushing = TRUE;

See the "if (stream)" some lines below :) You need to check for NULL

@@ +550,3 @@
+
+      /* send eos if haven't seed data as sink can't handle if haven't
negotiate */
+      if (!seen_data || self->eos) {

The only case when we get here and this is not true, is when we're flushing?
Document that and maybe even add an assertion :)

@@ +904,3 @@
+        if (stream->is_eos && !stream->eos_sent) {
+          self->send_gap_event = TRUE;
+          g_cond_broadcast (&stream->stream_finish_cond);

Please document in a comment here why a GAP event should be sent, also why not
another one is required when going from PAUSED to PLAYING

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.