GNOME Bugzilla – Bug 699187
videorate: ends up outputting buffers with incorrect duration
Last modified: 2013-05-09 14:22:59 UTC
In a GES test case, available here : https://github.com/MathieuDuponchelle/PitiviGes/blob/rendering/tests/check/ges/render.c#L243 we end up seeing, at the beginning of the transition, which is at the 5 seconds mark a buffer with an attributed duration of ~5 seconds. All the subsequent buffers get dropped until that duration is over. the attached proposed patch resets the videorate->base_ts variable when getting a new segment event.
Created attachment 242758 [details] [review] Patch trying to fix the problem.
Review of attachment 242758 [details] [review]: ::: gst/videorate/gstvideorate.c @@ +616,1 @@ videorate->base_ts + gst_util_uint64_scale (videorate->out_frame_count, I'm not sure this is correct in general. If videorate would be outputting a single segment and everything timestampped to running time it would be correct, but AFAUI videorate doesn't do that. Instead it generates timestamps inside the configured segment and closes gaps, etc. As such the segment base should not be considered for buffer timestamps but kept in the downstream segment. @@ +725,3 @@ + + /* Our new base for interpolation is now the segment base */ + videorate->base_ts = videorate->segment.base; 5 lines (or so) above base_ts is set to 0. I think the line setting it to 0 should be removed if all this is correct
Yep, that makes sense, then I think we should only not take the segment base into account for the next timestamp calculation. That new patch does that, doesn't break the test suite and yields the good results in our own test suite.
Created attachment 242806 [details] [review] New proposed patch to fix the issue.
By the way, I'm curious to know how that part was working in 0.10, cause at that time the segment base was added as well, and it didn't create that problem.
While that looks correct there are still some other uses of segment.base that look a bit suspicious to me
Mathieu, any news on this? I saw you discussed some videorate things on IRC, do you know now how this is supposed to work?
I think a unit test for -base would be in order.
(In reply to comment #7) > Mathieu, any news on this? I saw you discussed some videorate things on IRC, do > you know now how this is supposed to work? I do understand how it's supposed to work now :) We had other problems, addressed there : https://bugzilla.gnome.org/show_bug.cgi?id=699793 , which caused us to have wrong segments in the videorate as well, and made it difficult to finish this patch which is still incomplete as you mentioned in comment 6.
Ok so further debugging our problem in GES led me to think that we actually can leave references to segment.base, as the calculated timestamp is actually relative to the base: GST_BUFFER_TIMESTAMP (outbuf) = push_ts - videorate->segment.base; I started writing a test, and realized that the behaviour is correct, except when we receive a segment, some buffers, then caps get set and a new segment is received. In that case, in the _setcaps function, we do that : l 525 : gst_video_rate_swap_prev (videorate, NULL, GST_CLOCK_TIME_NONE); which leads videorate->prevbuf to be NULL. So when we get a segment event (l 676), we check if we have a prevbuf, and then and only then we reset base_ts. This is wrong, as we should reset out_frame_count and base_ts anyway. The attached patch will do that. Another, separate problem is that this previous buffer should not be thrown away, at least when caps are the same, and ideally never. I have no idea how to do that though.
Created attachment 243609 [details] [review] New proposed patch to fix the issue.
commit aec0e48aa17077daed600468212a632361af7aba Author: Mathieu Duponchelle <mathieu.duponchelle@epitech.eu> Date: Wed May 8 20:31:00 2013 +0200 videorate: Reset base timestamp and out_frame_count in any case on SEGMENT_EVENT Fixes #699187