GNOME Bugzilla – Bug 721666
videodecoder: push newsegment earlier for reverse playback
Last modified: 2014-01-13 11:22:21 UTC
For reverse playback, videodecoder creates a list of frames to push and then reverses it and pushes. The problem is that the frame events will get attached to the 1st buffer of the list. So, they will be pushed last. This causes an assertion as a few buffers will be pushed before a new segment event for example. My concern here is which/how to push events without breaking serialization. Should we care about multiple new segments in the list? What about caps events?
Created attachment 265488 [details] [review] videodecoder: push newsegment earlier for reverse playback As events are 'attached' to video frames in videodecoder, when doing reverse playback the new segment event will be attached to the first frame. And this frame is pushed last, so a few frames are pushed before a new segment. To prevent this, iterate over the list of pending frame events and push the new segment event before processing the buffers.
Comment on attachment 265488 [details] [review] videodecoder: push newsegment earlier for reverse playback You need to push all events <= SEGMENT here. Also I think you need to do that while pushing all the frames, and there make sure that handling of multiple events of the same type is done in order. That is, push the oldest SEGMENT event, then buffers that come before it. Then the next SEGMENT event, then buffers before that, etc.
Created attachment 265711 [details] [review] videodecoder: use new segment earlier for reverse playback For reverse playback, the segment event will only be pushed when the first buffer is actually pushed. But for decoding frames and storing those into the list to be pushed the output_segment.rate value is used to determine if it is forward or reverse playback. In case a previous segment event (or none) is in use it will mistakenly think it is doing forward playback and push the buffers immediatelly and try to clip buffers based on an old segment (or an uninitialized one, leading to an assertion) This patch fixes this by copying the segment earlier if on reverse playback
Created attachment 265712 [details] [review] tests: videodecoder: add test for reverse playback Checks that buffers are pushed backwards in reverse playback
I think this is only a workaround. You should not special case the segment event, but handle all events < segment like this and properly push them at the right time.
(In reply to comment #5) > I think this is only a workaround. You should not special case the segment > event, but handle all events < segment like this and properly push them at the > right time. AFAIK they are being pushed on the right time, the problem is on decoder_finish_frame: if (decoder->output_segment.rate < 0.0) { GST_LOG_OBJECT (decoder, "queued frame"); priv->output_queued = g_list_prepend (priv->output_queued, output_buffer); } else { ret = gst_video_decoder_clip_and_push_buf (decoder, output_buffer); } It needs the output_segment to be properly set to be able to know if it is doing reverse playback and avoiding trying to push the buffers. Note that the segment event isn't discarded or pushed at that check, it is just applied to output_segment so this piece of code works reliably.
I see, but this could cause the wrong segment to be set if there are multiple ones, right? They will all have the same playback direction but might have other differences. But I guess this is fine, it will do the right thing later on as far as I can see it
*** Bug 681634 has been marked as a duplicate of this bug. ***
Created attachment 265913 [details] [review] videodecoder: use new segment earlier for reverse playback Updated patch to make sure that the relevant events are pushed before any buffer, otherwise we can get 'buffer before segment' asserts
Review of attachment 265913 [details] [review]: Makes sense, does it also fix the duplicate here and the other bug about the segment format?
It fixes the duplicate, waiting for confirmation on the other bug before pushing.
Created attachment 265943 [details] Log time seek This is what hapen when the src element accept a seek with format = GST_FORMAT_TIME so no byte offset is calculated. It look like the gst_element_seek never exit;
(In reply to comment #12) > Created an attachment (id=265943) [details] > Log time seek > > This is what hapen when the src element accept a seek with format = > GST_FORMAT_TIME so no byte offset is calculated. > > It look like the gst_element_seek never exit; Actually, it does exit, but then I have to kill my application to stop it... I'll try to provide more details...
Looks like in my case, gst_video_decoder_flush_parse isn't even called...
Review of attachment 265913 [details] [review]: ::: gst-libs/gst/video/gstvideodecoder.c @@ +1961,3 @@ + GST_ERROR_OBJECT (dec, "Segment at frame %p %" GST_TIME_FORMAT, + frame, GST_TIME_ARGS (GST_BUFFER_PTS (frame->input_buffer))); + gst_event_copy_segment (event, &segment); Isn't using GST_ERROR_OBJECT a problem?
Review of attachment 265913 [details] [review]: ::: gst-libs/gst/video/gstvideodecoder.c @@ +1968,3 @@ + dec->priv->pending_events = + g_list_append (dec->priv->pending_events, event); + frame->events = g_list_delete_link (frame->events, cur); Trying to understand the patch. Why do we need to move the event from the frame and put it in the pending events list? Don't they get all pushed in gst_video_decoder_prepare_finish_frame? First, the pending events and then the frame events?
Pushed to master commit b1e728a8540d6aab44d4b7c489120b3acf896385 Author: Thiago Santos <ts.santos@sisa.samsung.com> Date: Wed Jan 8 11:29:29 2014 -0300 tests: videodecoder: add test for reverse playback Checks that buffers are pushed backwards in reverse playback https://bugzilla.gnome.org/show_bug.cgi?id=721666 commit 5b8e1925b566b57b516c76f12610d42e6ad2b791 Author: Thiago Santos <ts.santos@sisa.samsung.com> Date: Mon Jan 6 20:53:15 2014 -0300 videodecoder: use new segment earlier for reverse playback For reverse playback, the segment event will only be pushed when the first buffer is actually pushed. But for decoding frames and storing those into the list to be pushed the output_segment.rate value is used to determine if it is forward or reverse playback. In case a previous segment event (or none) is in use it will mistakenly think it is doing forward playback and push the buffers immediatelly and try to clip buffers based on an old segment (or an uninitialized one, leading to an assertion) This patch fixes this by copying the segment earlier if on reverse playback https://bugzilla.gnome.org/show_bug.cgi?id=721666
Pushed to 1.2 commit bcc99d6d7a069473a93ecf8b72e5bdfe78f78096 Author: Thiago Santos <ts.santos@sisa.samsung.com> Date: Wed Jan 8 11:29:29 2014 -0300 tests: videodecoder: add test for reverse playback Checks that buffers are pushed backwards in reverse playback https://bugzilla.gnome.org/show_bug.cgi?id=721666 commit 6216366d25eb327dc18803a5bded793b434f6aa5 Author: Thiago Santos <ts.santos@sisa.samsung.com> Date: Mon Jan 6 20:53:15 2014 -0300 videodecoder: use new segment earlier for reverse playback For reverse playback, the segment event will only be pushed when the first buffer is actually pushed. But for decoding frames and storing those into the list to be pushed the output_segment.rate value is used to determine if it is forward or reverse playback. In case a previous segment event (or none) is in use it will mistakenly think it is doing forward playback and push the buffers immediatelly and try to clip buffers based on an old segment (or an uninitialized one, leading to an assertion) This patch fixes this by copying the segment earlier if on reverse playback https://bugzilla.gnome.org/show_bug.cgi?id=721666
(In reply to comment #15) > Review of attachment 265913 [details] [review]: > > ::: gst-libs/gst/video/gstvideodecoder.c > @@ +1961,3 @@ > + GST_ERROR_OBJECT (dec, "Segment at frame %p %" GST_TIME_FORMAT, > + frame, GST_TIME_ARGS (GST_BUFFER_PTS (frame->input_buffer))); > + gst_event_copy_segment (event, &segment); > > Isn't using GST_ERROR_OBJECT a problem? This was fixed on the final version of the patch, thanks for pointing it out
(In reply to comment #16) > Review of attachment 265913 [details] [review]: > > ::: gst-libs/gst/video/gstvideodecoder.c > @@ +1968,3 @@ > + dec->priv->pending_events = > + g_list_append (dec->priv->pending_events, event); > + frame->events = g_list_delete_link (frame->events, cur); > > Trying to understand the patch. > > Why do we need to move the event from the frame and put it in the pending > events list? > > Don't they get all pushed in gst_video_decoder_prepare_finish_frame? First, the > pending events and then the frame events? For reverse playback, the buffers will be pushed in reverse order, but some events need to be pushed before any data and they are usually attached to the first buffer. To be sure that those events are going to be pushed early enough, move them to the pending events list, so they will be pushed before any of the buffers.