GNOME Bugzilla – Bug 701385
videomixer: Incorrect timestamp calculations for non trivial segments
Last modified: 2013-06-11 19:04:57 UTC
Context: I'm implementing video mixing in GES. The implementation is as follows: we have a videomixer set as expandable in gnonlin, and we plug our gnlsource bins on it when gnl requests a pad. The branch for this is there : https://github.com/MathieuDuponchelle/PitiviGes/tree/video_mixing There is a test case in the mixers.c file, which is called audio_video_mixed_with_pipeline. Currently, videomixer has a mix->offset_ts member which is useless, as it is always reset to 0 on flush stops and seek events. The symptoms are that buffers are dropped while they should not be dropped. My first proposed patch fixes that. There is a second bug as well, which is unrelated as without that patch, the problem also occurs. This bug doesn't happen with my test case though, I experience it in pitivi when trying to move an object on a second layer : The segment offset gets set to a value != 0, and when videomixer calls fill_queues, gets a buffer of timestamp == 0 and calls to_running_time on that timestamp with the collectpads segment, it reurns -1, which triggers the assertion just below. I think we can either set the segment offset to 0 before that calculation, or offset start_time and end_time with the segment offset as we don't want it to influence our calculation here. My second proposed patch does that. I know that slomo works on proper synchronization in adder, I'd like to know if you experience this bug ? Also, it would be great if we could synchronize the synchronizations between adder and mixer :)
Created attachment 245769 [details] [review] Proposed patch to fix the first issue
Created attachment 245770 [details] [review] Proposed patch to fix the second issue
Created attachment 245838 [details] [review] Proposed patch to fix the first issue I forgot to add mix_ts in qos calculations as well.
Review of attachment 245838 [details] [review]: I don't think this is correct. ts_offset is used in gst_videomixer2_src_setcaps() to rebase timestamps if the output framerate changes during playback. As such ts_offset has to be taken into account in the segment.position as well as in the output timestamps. There might be bugs related to that though that might need to be fixed. ts_offset is supposed to be the output running time when the last time the output framerate was set (i.e. ts_offset + nframes is always the current output timestamp) ::: gst/videomixer/videomixer2.c @@ +929,3 @@ } + timestamp += mix->ts_offset; Why? @@ +1001,3 @@ + if (mix->segment.position == -1) { + output_start_time = 0 * GST_SECOND; Why? The first timestamp after a seek should be segment.start @@ +1406,3 @@ stop_type, stop, NULL); mix->segment.position = -1; + mix->ts_offset = mix->segment.start; This could indeed make sense but is conflicting with the setting of ts_offset to position - start in update_src_caps(). That would need to be changed there to just setting it to position. These two changes I think make the usage of ts_offset sane again. @@ +1744,2 @@ mix->segment.position = -1; + mix->ts_offset = mix->segment.start; Here I think all the output status resetting should be removed, i.e. segment.position, nframes and ts_offset
Review of attachment 245770 [details] [review]: ::: gst/videomixer/videomixer2.c @@ +747,3 @@ + /* We don't want the segment offset to influence our conversion to running time */ + start_time += segment->offset; + end_time += segment->offset; Why not? That's exactly the point of the offset
Ok so I fixed the first patch, and discovered that the second one was incorrect, the bug doesn't happen with qtdemux for instance, it's specific to matroskademux and I suspect it's the same as the one tim mentions here : https://bugzilla.gnome.org/show_bug.cgi?id=680306#c4 So the second patch is incorrect, but I believe the one I'll attach is.
Created attachment 246372 [details] [review] Proposed patch to fix the first issue
I think that will break when a new fps is negotiated in gst_videomixer2_src_setcaps(). Also all the calculations in videomixer should really be with timestamp inside the segment, so start at segment.start and go to segment.stop. Maybe starting with segment.start+segment.offset.
After lengthy discussions on IRC with slomo, it was agreed that the users have to be updating the base time of the incoming segments. That way, videomixer can make its calculations inside the current output segment. For us it meant we had to patch gnlcomposition and gnloperation. With this done, two bugs were uncovered in videomixer. first one, in collected: output_start_time = mix->segment.start; [....]; output_end_time = mix->ts_offset + gst_util_uint64_scale (mix->nframes + 1, GST_SECOND * GST_VIDEO_INFO_FPS_D (&mix->info), GST_VIDEO_INFO_FPS_N (&mix->info)); When the segment start is not 0, the calculation of output_end_time created a situation where it was inferior to output_start_time, and the duration of the next buffer ended up underflowing. We need to add segment.start to output_end_time, it's what the first patch does. second one, in fill_queues: end_time = start_time - GST_BUFFER_TIMESTAMP (mixcol->queued); || end_time = GST_BUFFER_DURATION (buf); [...] if (mixcol->end_time != -1 && mixcol->end_time > end_time) { GST_DEBUG_OBJECT (pad, "Buffer from the past, dropping"); [...] end_time = gst_segment_to_running_time (segment, GST_FORMAT_TIME, end_time); [...] mixcol->end_time = end_time; The comparison has to be made *after* conversion to running_time, that's what my second patch does.
Created attachment 246554 [details] [review] Proposed patch to fix the first new issue
Created attachment 246555 [details] [review] Proposed patch to fix the second new issue
commit 6e23f1fec41fa35139f29e40ec89846890905c67 Author: Mathieu Duponchelle <mathieu.duponchelle@epitech.eu> Date: Tue Jun 11 19:24:49 2013 +0200 videomixer: check last end_time after conversion to running segment The last end_time was saved after conversion, so the comparison had to be made after conversion for it to make sense. https://bugzilla.gnome.org/show_bug.cgi?id=701385 commit 4243714301e24e2f79bacb30f76e1040d1b369fa Author: Mathieu Duponchelle <mathieu.duponchelle@epitech.eu> Date: Tue Jun 11 19:22:20 2013 +0200 videomixer: add mix->segment.start to output_end_time When the segment start is not 0, this created a situation where the output_end_time is inferior to output_start_time, and the duration of the next buffer ended up underflowing. https://bugzilla.gnome.org/show_bug.cgi?id=701385