[Bug 750794] Lipsync problem using mp4mux with late v4l2src

[GStreamer (GNOME Bugzilla)]

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

Nicolas Dufresne (stormer) <nicolas.dufresne at collabora.co.uk> changed:

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

--- Comment #9 from Nicolas Dufresne (stormer) <nicolas.dufresne at collabora.co.uk> ---
Review of attachment 305560:
 --> (https://bugzilla.gnome.org/review?bug=750794&attachment=305560)

This is just a drop toward what comes to my mind. The sycnhronization in that
code is base on PTS instead of running-time PTS. This means that unless you
have sources that starts at PTS 0, you won't have synchronized output. It is
interesting that this implementation is non-blocking. While this creates
simplicity, it reduce the ability to make the right decision upon which buffer
should be kept (and clipped) or pushed downstream. A very complete solution in
my opinion would actually queue up to two buffers so we can ensure to always
keep the best buffer.

* Each pad would queue up to two buffers (will all in-between serialized
events) and block.
* When a buffer is received, look for a start "running-time" on all pads.
* When that is found, for each pad, lookup for buffers that don't fit (the
start running time isn't between buffer start and stop (start + duration) and
drop.
* In case you have dropped something, unblock that pad, wait for the next
buffer.
* In case you didn't drop anything, update segments event for each pads (they
can be all different), drain all queued, unclock all pads (basically
passthrough afterward).

::: gst/multisync/gstmultisync.c
@@ +43,3 @@
+
+/**
+ * SECTION:element-multi_sync

Sections need to be added docs/plugins/gst-plugins-bad-plugins-sections.txt

@@ +98,3 @@
+#define GST_MULTI_SYNC_MUTEX_UNLOCK(m) G_STMT_START {                        \
+  g_mutex_unlock (&m->lock);                                            \
+} G_STMT_END

I think you can use the GST_OBJECT_LOCK() instead of creating a new one.  For
the future, add extra parenthesis when using macro member. The member could be
an expression which may lead to an ambiguity.

@@ +108,3 @@
+    GST_PAD_SRC,
+    GST_PAD_SOMETIMES,
+    GST_STATIC_CAPS_ANY);

I don't thinks this is media agnostic as you don't deal with deltas, neither
you add decode only flags. You should only mark formats that you are use won't
break.

@@ +309,3 @@
+    gst_buffer_unref (sstream->last_buffer);
+  }
+  g_free(sstream);

I think you need to do something more. If all pads, except the this one, had
data, the streaming will be stalled.

@@ +376,3 @@
+  GST_DEBUG_OBJECT (pad, "Current buffer TS is: %" GST_TIME_FORMAT,
GST_TIME_ARGS (GST_BUFFER_PTS (buffer)));
+
+  if (GST_BUFFER_PTS (buffer) < msync->start_timestamp) {

This is just an example where this cannot work for let's say H264 with
reordering. The delta chain will be broken + the PTS are not monotonic in that
case.

One of the issue with this, is that you blindly drop base on the TS. You don't
consider the duration of the buffer. This mostly reduce accuracy, but you don't
seem to break a/v sync. This will have side effects if you have buffer with
super long duration in one stream, or as I already seen, a single buffer in a
video stream and then audio.

@@ +382,3 @@
+  }
+
+  GST_BUFFER_PTS (buffer) -= msync->start_timestamp;

Don't re-timestamp. Send a segment event instead (to bring back the running
time of the PTS).

@@ +413,3 @@
+
+    GST_DEBUG_OBJECT (pad, "Trying to determine start TS");
+    msync->start_timestamp = gst_multi_sync_find_start_timestamp (msync);

This is fundamentally broken because timestamp alone don't allow
synchronization. As what you want is to synchosize, you need to convert the TS
base on the received segment to a common time base. In this case, you want to
convert the PTS to running time.

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