GNOME Bugzilla – Bug 675773
Reverse video playback does not work anymore
Last modified: 2012-06-20 01:35:19 UTC
I have no idea since when, but the other day I was playing with the playback test and didn't found any video format on my hard drive that would run in reverse. I think this used to work.
Might be related to the new video base classes (or does it happen with other decoders too?) (Assigning to gst-plugins-base as a start, needs more investigation though)
The basevideodecoder doesn't forward the newsegment to the sink after a the seek. It attaches the event to a frame that would be pushed, but this frame doesn't get pushed when it is not a keyframe, and that might happen as it needs to accumulate data before finding a keyframe. The problem is that when it gets the next discont buffer, it flushes its state including the frame that had the newsegment attached to it.
I guess the code should be adjusted to store events stored in a frame somewhere else in the instance struct whenever a frame is dropped. And push these before the in-frame events when pushing a new frame. (And of course clear these in-instance pending events during flushing, etc)
That's effectively what the patch I put in theoradec does, although it doesn't seem to be a complete fix. With that patch, it'll play backward for a little bit, and then stop - I suspect when it tries to loop backward to the next/previous segment, although I didn't debug it yet. The question is, is it appropriate to fix it like I did and require that subclasses *always* call either gst_video_decoder_finish_frame() or gst_video_decoder_drop_frame() for a given input frame, or is it better to put the call to gst_video_decoder_drop_frame() in the base class as a response to the GST_VIDEO_DECODER_FLOW_NEED_DATA return value? commit 2fbb803d85295fe1b69ccb0425b81d3dd9db0fe5 Author: Jan Schmidt <thaytan@noraisin.net> Date: Wed Jun 13 03:17:27 2012 +1000 theoradec: Always inform base class when dropping frames Partially fixes backwards playback. Informing the base class of the dropped frame lets it manage the timestamping and events better. diff --git a/ext/theora/gsttheoradec.c b/ext/theora/gsttheoradec.c index 317d05a..4ea7803 100644 --- a/ext/theora/gsttheoradec.c +++ b/ext/theora/gsttheoradec.c @@ -819,6 +819,7 @@ theora_dec_handle_frame (GstVideoDecoder * bdec, GstVideoCodecFrame * frame) res = gst_video_decoder_finish_frame (bdec, frame); break; case GST_CUSTOM_FLOW_DROP: + case GST_VIDEO_DECODER_FLOW_NEED_DATA: res = gst_video_decoder_drop_frame (bdec, frame); break; default:
NEED_DATA is only accepted by the parse vfunc. So I guess ideally the parse vfunc should get the same frame passed until it returned something else than NEED_DATA. I thought it does this already. And gst_video_decoder_drop_frame() also drops the events of that frame, so the fix I described in comment 3 is necessary in any case (also, you should not push the event as part of drop_frame() as you might not have caps, etc. yet until the first call to finish_frame()).
Not for theoradec at least. theoradec's parse function just accepts the whole packet of data and calls gst_video_decoder_have_frame(). It's the handle_frame() callback that does all the work - including returning NEED_DATA if it's waiting for a keyframe. I guess the docs for videodecoder need to clarify this situation. They don't mention the NEED_DATA custom return value at all afaics, although the doc string for NEED_DATA itself does mention it's for use when parsing.
handle_frame() returns NEED_DATA when it has a complete frame but waits for a keyframe? That sounds wrong to begin with :) AFAIU NEED_DATA is only meant for the parse vfunc in case it needs more data until it has a complete frame.
So TODO list here would be: - Clarifying/fixing NEED_DATA usage, it should only be used in the parse() vfunc to request more data. Not if a complete frame is there already but the base class is waiting for a keyframe before it proceed. (Or am I missing something). Also document this. - Make sure events that are in frames that are passed to drop_frame() are not lost but instead pushed (in order!) when the next frame is finished. Before that frame's events are pushed. Document that drop_frame() or finish_frame() must be called for every and any event.
My phrasing may be imprecise, because the definition of "VideoCodecFrame" seems imprecise in the base class. I mean to say that: * in its parse function, theoradec says 'add_to_frame()' and _have_frame() for each inbound packet, assuming that they are already packetised. * after a seek, while waiting for a keyframe to arrive, theoradec still passes all packets through parse(), but then rejects them in the handle_frame() callback by returning NEED_DATA. I think the documenting TODO list also requires clarification about how the division of labour between parse() and handle_frame(). It sounds like theoradec should be dropping data in the parse() function while waiting for a keyframe, rather than in the handle_frame() callback.
Ah I see. theoradec must not return NEED_DATA in handle_frame() but instead return OK and drop_frame() from my understanding.* The parse() vfunc should only gather data until it has a complete frame (that is, all data/packets from the end of the last frame until the start of the next frame) and then pass it to the base class for further handling in handle_frame(). * actually the waiting for a keyframe is probably already handled in the base class... or should be
Clearly not (keyframes being handled in the base class), or theoradec would never have returned NEED_DATA in the handle_frame() callback.... You're saying that theoradec *is* doing the right thing in the parse() function, but should do what I changed it to do (call _drop) but just return OK instead of NEED_DATA from the handle_frame() callback. I think that's clear enough.
Looking further into this, there's no need to store events from the dropped frame for later pushing. Whether in forward, or reverse mode, as soon as a frame is either passed to drop_frame() or finish_frame() the events it holds are pushed downstream. Also, it seems that at least some formats do work with the current 0.10 code - I'm able to play a divx/avi file in either direction OK. Theora still has trouble though, I think mostly related to timestamp tracking. In reverse mode, oggdemux no longer interpolates timestamps to ensure each outgoing packet has one, and it's up to theoradec to calculate them. One specific problem that causes is that keyframes don't necessarily have a timestamp attached, but the base class assumes that they must and assumes a timestamp of the start of the segment (usually 0) otherwise. This is the relevant code in gst_video_decoder_prepare_finish_frame(): } else { if (GST_VIDEO_CODEC_FRAME_IS_SYNC_POINT (frame)) { GST_WARNING_OBJECT (decoder, "sync point doesn't have timestamp"); if (!GST_CLOCK_TIME_IS_VALID (priv->timestamp_offset)) { GST_WARNING_OBJECT (decoder, "No base timestamp. Assuming frames start at segment start"); priv->timestamp_offset = decoder->output_segment.start; } } } I'm still trying to figure out where to begin cleaning this up. To be honest, I don't understand why this stuff was committed to 0.10 in the state it's in.
I pushed my patches for 0.10 and 0.11 to https://github.com/thaytan/gst-plugins-base/commits/ on branches 0.10 and viddec-dec-0.11 respectively. There are couple of remaining problems: * In porting my patch to 0.10, I ran into problems with the PTS/DTS detection and reordering code (which isn't present in 0.11). One problem is that I don't see how that code works correctly at the moment. Just seeking backwards in a video makes it (mis-)detect that timestamps go backward and it start rearranging them. Commit de3c99c6b4d09577b2be48ca5942fb2eb922abb4 on the 0.10 branch fixes that for the trivial seeking case, but it still gets it wrong when playing in reverse, as the frame numbers in the submitted frames are no longer linear (triggering the PTS/DTS detection again), and it requires a more extensive fix that I don't see just yet. That makes it hard to test the changes in 0.10. * I haven't had a chance to test these changes with the 0.11 branch of gst-ffmpeg that's just been ported to GstVideoDecoder.
Those commits are too big for review. And please avoid the "I don't understand why this stuff was commited". If it works fine for 99.99% of the files out there and only breaks for ogg, I wouldn't say it's a failure.
Commented on the 0.11 github branch
You're right - I'll tone down the hyperbole, things aren't that dire :) I'll see if I can split the patches up into more digestible chunks. I'd like to make a quick catalog of a few of the most important formats that attempt to support reverse playback, so we can draw a baseline. There are other containers/formats, but I think these are the most important. Feel free to add others. I've ignored the audio stream entirely here. Containers that support reverse playback are: ogg, avi, mkv (webm), MPEG-PS, flv Containers that don't/never have: MPEG-TS, asf (wmv), dv, qt/mov/mp4 Formats that did support reverse playback: theora, all ffmpeg codecs (divx/mpeg4, h264, flv, wmv), schrodinger (?), mpeg2, vp8, jpeg (in packetised mode) Formats that didn't do reverse playback before the base class and now can: jpeg (in non-packetised mode) Formats that didn't do reverse playback (but could after porting to the base class): dv With that map, I think there's only a few key formats that used to do reverse playback for testing. In particular some of the ones I remembered as working (h264 in mov and wmv in asf) never did, because the container couldn't. Testing a few others: * ogg/theora, needs my patch to work * divx in avi works with or without the patch * jpeg in avi, works with or without * mpeg2 in mpeg-ps, throws an error in the video parser attempting to play reverse. * webm - works with or without the patch. So bilboed is correct - most of the cases work fine, it's just ogg/theora that regressed.
FWIW, I think mpeg-2 in mpeg-ps reverse playback hasn't worked for a while (I have some patches pending for the demuxer to make it work better/differently though, but they need some more work; just saying that's not a regression as far as I know).
Wim pointed out that qtdemux *does* do reverse playback in pull-mode. Testing a few codecs: h264 in mov works, mpeg4 in mov is problematic (with or without patch) and svq3 in mov works. All ffmpeg codecs - so it's strange mpeg4 fails.
With these committed to 0.10, all the formats I think should work do, except mov/mp4+h264 files still seem weird: commit f1c14fc5518ac735474d8ace56f365ff2d3c2d96 Author: Jan Schmidt <thaytan@noraisin.net> Date: Wed Jun 20 03:10:01 2012 +1000 videodecoder: Calculate correct durations interpolating One field = half the FPS. commit 07a16d541d9ac319d40e796950c97640e45ee863 Author: Jan Schmidt <thaytan@noraisin.net> Date: Sun Jun 17 20:27:33 2012 +1000 videodecoder: Clear the last_out_frame_number when flushing Don't mis-detect reordered output after a seek. Instead, clear the last_out_frame_number to an invalid value and wait for at least one frame. commit db4e440d385c2e5583dfdf1ef22b5719e447c7aa Author: Jan Schmidt <thaytan@noraisin.net> Date: Wed Jun 20 00:46:05 2012 +1000 videodecoder: EOS handling for reverse mode. Handle EOS correctly in reverse mode by treating it as a final discont and flushing out whatever we can. commit 178a3b08d21383d29af4d0fe21b0643a6feaa1e4 Author: Jan Schmidt <thaytan@noraisin.net> Date: Wed Jun 20 00:42:42 2012 +1000 videodecoder: misc improvements/changes Use g_list_free_full instead of walking lists twice when freeing them. Remove pointless clause in gst_video_decoder_chain that doesn't actually have any effect. Other changes to make the code slightly more like the 0.11 version. commit c580119af283892a150a64886a1ebf966452b0e9 Author: Jan Schmidt <thaytan@noraisin.net> Date: Wed Jun 20 00:36:38 2012 +1000 videodecoder: Improve timestamp handling. Fix problems with timestamp calculations when the incoming buffers have sparse timestamps (as for theora) and reverse playback. Fixes #675773 commit 9c8f7ba6aec561c724a0363fc5d5394a28a26ce4 Author: Jan Schmidt <thaytan@noraisin.net> Date: Wed Jun 20 00:22:25 2012 +1000 videodecoder: Re-work reverse playback handling Move processing of the gather list into the flush_parse function. Add a last ditch attempt to apply timestamps to outgoing buffers when walking backwards through decoded frames. Requires that each gathered region has at least one timestamp. Make sure to remove decoded packets from the decode list when they are sent - otherwise the list just grows on each cycle, with more and more frames being decoded and then clipped away. Break out of the processing loop early on a bad flow return to make seeking more responsive. Use the gst_video_decoder_clip_and_push_buf function in reverse mode, instead of pushing all buffers arbitrarily. A couple of small efficiency gains in the list handling, by moving list elements directly and not reallocating, and by reversing and concatenating the gather list instead of moving it one node at a time. Rename the gst_video_decoder_do_finish_frame function to gst_video_decoder_release_frame. commit dfc4ca16c2fa99fd4b554de2dcdffecccbfa1e3b Author: Jan Schmidt <thaytan@noraisin.net> Date: Wed Jun 20 00:08:57 2012 +1000 videodecoder: Split gst_video_decoder_finish_frame Split the 2nd half of the gst_video_decoder_finish_frame function out to gst_video_decoder_clip_and_push_buf. commit c99f7713c5e725022fc0d381bb56e3de52025adb Author: Jan Schmidt <thaytan@noraisin.net> Date: Tue Jun 19 23:46:44 2012 +1000 videodecoder: Rename queued list to output_queued for clarity. Use g_list_free_full instead of g_list_foreach + g_list_free commit b941b1034d61244c97a4aab9653c85381e0ec6b6 Author: Jan Schmidt <thaytan@noraisin.net> Date: Tue Jun 19 23:43:27 2012 +1000 videodecoder: Small cleanups Remove extra deref using a local var, and add/change some doc comments and debug statements commit 1b0a41dcd3ebb9992419a4221189f6511c60565f Author: Jan Schmidt <thaytan@noraisin.net> Date: Tue Jun 19 23:28:08 2012 +1000 videodecoder: Rename gst_video_decoder_have_frame_2 function Rename gst_video_decoder_have_frame_2 to gst_video_decoder_decode_frame and pass the frame to process directly, rather than using the current_frame pointer as a holding pen. Move the negative rate handling out of the function to where it is needed, and remove the process flag. commit 79b772ed9b2862b5360ef123bc2c46a48c5ef543 Author: Jan Schmidt <thaytan@noraisin.net> Date: Tue Jun 19 23:16:12 2012 +1000 videodecoder: Extend docs and add comments Update the documentation block for the base class, and add a comment block about the reverse-playback logic and implementation. commit 96883b5fc575b2114c1fb4f240f0e687081c3d80 Author: Jan Schmidt <thaytan@noraisin.net> Date: Sun Jun 17 20:25:42 2012 +1000 theoradec: Clear the base class packetized flag. The data has to be passed through the parser to get the correct SYNC_POINT flag set on each packet, for reverse playback to work.