GNOME Bugzilla – Bug 635020
Flushing everything on a discont produces worse result than just ignoring the discont flag
Last modified: 2012-04-20 18:47:26 UTC
Currently, when a gst-ffmpeg decoder gets a buffer with DISCONT set, it flushes all of its state. This is wrong for the RTP case where we want to just continue decoding as-is until the next keyframe. The flushing after a discont should probably be delayed into a keyframe is received (and I guess the decoders will full their state internally on a keyframe anyway), so I suggest we just drop it from there. --- a/ext/ffmpeg/gstffmpegdec.c +++ b/ext/ffmpeg/gstffmpegdec.c @@ -2439,10 +2439,7 @@ gst_ffmpegdec_chain (GstPad * pad, GstBuffer * inbuf) * case of a network error, better show the errors than to drop all data.. */ if (G_UNLIKELY (discont)) { GST_DEBUG_OBJECT (ffmpegdec, "received DISCONT"); - /* drain what we have queued */ - gst_ffmpegdec_drain (ffmpegdec); gst_ffmpegdec_flush_pcache (ffmpegdec); - avcodec_flush_buffers (ffmpegdec->context); ffmpegdec->discont = TRUE; gst_ffmpegdec_reset_ts (ffmpegdec); }
I don't think this would be correct. After a DISCONT you can't really do anything with the still queued and not yet processed buffers, because if they're still queued something is missing for them to be processed and future buffers will be completely unrelated to them.
(In reply to comment #1) > I don't think this would be correct. After a DISCONT you can't really do > anything with the still queued and not yet processed buffers, because if > they're still queued something is missing for them to be processed and future > buffers will be completely unrelated to them. Even if that is the case, sometimes having motion and corruption on the screen is better than a still image where nothing happens at all. I'm guessing that this bug claims that letting ffmpeg decode as much as possible gives a better experience.
It depends whether the DISCONT means "some missing data skipped" or "completely unrelated data". This is a difference that needs to be addressed in 0.11, preferably by a corrupt or missing data marker on buffers. In particular, passing this information downstream allows better opportunities for reconstruction.
I guess for now we could have a property on gstffmpegdec to decide how to deal with disconts (in my case, there will never be unrelated data). That said, if its valid new data, shouldn't it start with a keyframe anyway ?
Actually, I'm not sure anymore if it's not better to have a frozen image than a corrupted image. It probably depends if we can request a new keyframe in a timely manner or not.
.. Do we have a solution for this in 0.11 ? Do we want to split the DISCONT flag into two or something ?
Here's a summary of an IRC discussion about the DISCONT flag and how it should be handled in decoder and encoders: * decoders should handle the DISCONT flag on the input as a hint that something might be missing and that they might need to do some concealment for missing data. They must not reset themselves and require a new keyframe or whatever. * decoders must not forward DISCONT flag downstream * encoders should handle the DISCONT flag as a hint that timestamp resync should happen (RESYNC buffer flag in 0.11, so this only applies to 0.10) but that's it * encoders must not forward DISCONT flag downstream
Small correction/clarification: timestamp resync is only interesting for audio, not video. And because of the timestamp resync, audio decoders must forward the DISCONT flag downstream. But only in 0.10, not in 0.11.
(In reply to comment #3) > It depends whether the DISCONT means "some missing data skipped" or "completely > unrelated data". This is a difference that needs to be addressed in 0.11, DISCONT on a buffer means that one or more earlier buffers are missing. It does not matter if it is 1 buffer or unrelated data (= many,many buffers missing). For new streams we have the STREAM_START event. For discont after a seek (unrelated data in the same file) it usually should fall on a keyframe. > preferably by a corrupt or missing data marker on buffers. In particular, There is a CORRUPT flag for buffers in 0.11. It is meant to be used when you know that the data in the buffer could have errors, like when a checksum failed or so. In 0.11 there is also a RESYNC flag on buffers to tell elements that this buffer is a good place to resynchronize against the clock. This is most useful for raw audio where an audiosink ignores jitter in the timestamps to align the samples. > passing this information downstream allows better opportunities for > reconstruction.
(In reply to comment #8) > Small correction/clarification: timestamp resync is only interesting for audio, > not video. And because of the timestamp resync, audio decoders must forward the > DISCONT flag downstream. But only in 0.10, not in 0.11. In 0.11 it makes sense for a decoder to turn the input DISCONT flag into a RESYNC flag on the output, this for both audio and video decoders. For video decoders this currently is not needed but it could be interesting when dealing with a sink that flips buffers on vsync.
(In reply to comment #7) > * decoders must not forward DISCONT flag downstream If no concealment happened, it should forward DISCONT, after all there is data missing in the output as well. If concealment data is made, the DISCONT should not be forwarded. In 1.0, the conceiled output should have the CORRUPT flag. Not sure if maybe we should add another new flag CONCEILED or something because it's not really corrupted.
Another point to decide against a new CONCEILED flag: If you consider the dv depayloader, it a packet is lost it will use the data from a previous image to conceil the error. Is the output image now CORRUPTED or not? I would say that when the output doesn't look like it was intended, it is corrupted.
I commited this: commit 630790f50d04a4998a37a0e01304e9196f776476 Author: Wim Taymans <wim.taymans@collabora.co.uk> Date: Fri Apr 20 16:16:25 2012 +0200 ffdec: don't flush buffers on DISCONT Don't flush the buffers that ffmpeg has on DISCONT but instead let it recover. This gives a much better image in the case of packet loss. Fixes https://bugzilla.gnome.org/show_bug.cgi?id=635020
I tested with a simple v4l2src and x264enc and identity drop-probability=0.05. Not flushing the ffmpeg buffers gives _much_ better results. Note that the current code did not wait for a keyframe after DISCONT, it just cleared a bit too much state.
For 0.11, would you apply the CORRUPTED flag for every buffer until the next keyframe?
Setting it on every buffer until the next keyframe doesn't make sense if you have long term reference frames as you may recover without keyframes. So maybe only one concealed frames. What's the usecase for the concealed flag on recovered frames anyway ?
(In reply to comment #15) > For 0.11, would you apply the CORRUPTED flag for every buffer until the next > keyframe? I would do that, things like face tracking etc might just not work so well on CORRUPTED frames. Dunno, it depends on how damaged the frame is but we don't get that from libav..
(In reply to comment #16) > Setting it on every buffer until the next keyframe doesn't make sense if you > have long term reference frames as you may recover without keyframes. So maybe We don't know when libav managed to recover without a keyframe.. > only one concealed frames. What's the usecase for the concealed flag on > recovered frames anyway ? I can't see a good use case, the CORRUPTED flag seems enough for now. It's just that intuitively there is a difference between corrupted and conceiled raw data (mostly for audio, you would think corrupted data sounds like noise and conceiled data is sortof close to the intended output)..