GNOME Bugzilla – Bug 709711
videodecoders: Add new event that notifies upstream about corrupted streams
Last modified: 2018-11-03 11:26:10 UTC
Apologies for longish bug report, most of it is a description of a patch that I could write/submit. I'm new here and don't want to start writing a patch that won't be accepted. +++ What I want: I want vp8dec and avdec_h264 to send upstream force_key_unit events when they are unable to decode a frame. The force_key_unit event will instruct an upstream element that downstream needs a key frame in order for the decoder to recover (e.g. from packet loss/corrupt frames). The mechanism to detect corrupt frames is codec specific, e.g. VP8 would use vpx_codec_control(&dec->decoder, VP8D_GET_FRAME_CORRUPTED, &corrupted). When decoding problems are detected, each decoder should create a GstEvent using gst_video_event_new_upstream_force_key_unit and push it on the sink pad. +++ Proposed patch: So a basic patch would be to add detection code into handle_frame method of the two decoders and push the force_key_unit event on the sink pad. However I don't want spammy force_key_unit events, I want to be able to configure that a force_key_unit event can happen only once a second. It makes sense to put this logic in the base decoder. I can provide the following patch: 1) add min-force-key-unit-interval property to gstvideodecoder.c. This read/write property will get inherited by all videodecoders including vp8dec and avdec_h264. 2) add gst_video_decoder_corrupt_frame method to gstvideodecoder.c - this should be called by a subclass whenever it cannot decode a frame. It does not replace the need to call gst_video_decoder_finish_frame or gst_video_decoder_drop_frame. 3) the base class will keep track of the last time a force_key_unit was sent and will only send new force_key_unit if: if (corrupt_frame->pts >= (last_sent_time + decoder->min_key_unit_interval)) { //send upstream force_key_unit } 4) vp8dec and avdec_h264 are changed to call new gst_video_decoder_corrupt_frame method each time a frame cannot be decoded. I'm looking for guidance as to whether this patch will likely be acceptable before I start writing it. One possible objection people may have is that by adding the min-force-key-unit-interval property to the base class, it may look as if other video decoders support sending force_key_unit events, however this won't be the case until the necessary detection logic has been added to the decoder. Another area of contention is what the default value for min-force-key-unit-interval should be. The safest would probably -1, which means never send force-key-unit events, this would ensure existing pipelines don't start behaving differently. Whilst I'm currently testing on 1.0.7 the patch would likely be for 1.2
Why should a new keyframe be requested whenever a frame can't be decoded? The frame might not be referenced by future frames, or also the corruption of the stream could've happened because of too high bitrate and dropped data so asking for a new keyframe could actually make the situation worse.
That is true, sending additional key frames may make the situation worse where packet loss is being caused by a continually congested network. I'm expecting these upstream force_key_unit events to result in PLI messages being sent over RTCP to a remote sender. RFC 4585 has the following to say about how the sender should behave when it receives PLI The sender MAY react to a PLI by transmitting an intra-picture to achieve resynchronization (making this message effectively similar to the FIR message as defined in [6]); however, the sender MUST consider congestion control as outlined in Section 7, which MAY restrict its ability to send an intra frame. Sending the force_key_unit event should be interpreted by a sender as a polite request from downstream that things aren't going too well. In a non-real-world simulation of 20% packet loss I have the following pipeline "vp8enc > identity drop-probability=0.2 > vp8dec". Without the force_key_unit events the video is shredded, whereas with it - its just about watchable. BTW we are intending to primarily use NAKs to fix packet loss, with this PLI as a backup for when things really go amiss. I'm guessing, if this were implemented, you'd prefer it not to be switched on by default and perhaps there to be some logic that waits for a set of corrupt frames before sending first force_key_unit event.
I think another kind of event would be useful for this situation. Similar to the packet lost events sent by rtpjitterbuffer already.
Another event? I don't follow. I need an event that 1) when received by an upstream rtpdepay element, it sends out a PLI over RTCP, and 2) when received by an upstream encoder element the encoder will send a key_unit (e.g. key frame). I believe the force_key_unit event does the above. Whilst the packet loss event occurs for each packet lost, I don't expect the decoder to report a frame as corrupt for minor packet loss because many codecs have packet loss concealment. Only when the decoder is seriously struggling with the stream should it send a force_key_unit. To me I feel that the correct behaviour of a decoder that can't decode is to send a force_key_unit event. However none of the gstreamer decoders send force_key_units, so maybe I'm missing something. I forgot to include some gst-launches that could demonstrate that a future patch improves things. 1) Before patch - h264 test with 10% frame drop gst-launch-1.0 videotestsrc pattern=18 ! queue ! x264enc ! identity drop-probability=0.1 ! avdec_h264 ! autovideosink 2) Before patch - vp8 test with 10% frame drop gst-launch-1.0 videotestsrc pattern=18 ! queue ! vp8enc keyframe-mode=0 keyframe-max-dist=1000 error-resilient=0 ! identity drop-probability=0.1 ! vp8dec ! autovideosink 3) After patch - h264 test with 10% frame drop and force_key_unit interval of 1000ms gst-launch-1.0 videotestsrc pattern=18 ! queue ! x264enc ! identity drop-probability=0.1 ! avdec_h264 min-force-key-unit-interval=1000 ! autovideosink 4) After patch - h264 test with 10% frame drop and force_key_unit interval of 1000ms gst-launch-1.0 videotestsrc pattern=18 ! queue ! vp8enc keyframe-mode=0 keyframe-max-dist=1000 error-resilient=0 ! identity drop-probability=0.1 ! vp8dec min-force-key-unit-interval=1000 ! autovideosink
I am interested in this also - I think that this change is roughly what Olivier was talking about on this thread http://gstreamer-devel.966125.n4.nabble.com/How-to-send-a-RTCP-FIR-td4660687.html: "Make sure that your decoder sends an upstream GstForceKeyUnit event when it detects an image loss (note: gst-libav still doesn't do that)" This adds GstForceKeyUnit events at image loss (as opposed to packet loss) - isn't this the intention of GstForceKeyUnit.
Has there been anything else discussed regarding this issue? I believe it would be quite beneficial if the decoders would detect image or packet loss, and send a GstForceKeyUnit event. At least the patch would be quite straight forward for vp8dec since it would be done in the gst_vp8_dec_handle_frame function. And then the responsibility of whether to respond or not to this event would rely on the application (Be it the encoder, or rtp session...etc...).
Wim? We talked about this in Edinburgh shortly, you mentioned that you were thinking about this a bit already. How should we move forward here?
A quick update from me (original poster). My finding were that often the vp8 and 264 decoders (not the gstreamer wrapper) did not indicate that there were problems with the stream - I eventually ended up looking for GstRTPPacketLost events (e.g. in gstrtph264depay.c) and sending an upstream key unit there. Perhaps what would have been better would be to feed the decoder with empty data when GstRTPPacketLost events were detected so that the decoder knows that packets have been lost. Perhaps then the decoder could be queried for corrupt frames.
As discussed on IRC this should become a new event that describes the problem (data corruption), not the solution (new keyframe). This event would look very similar to the QoS event and would have also a "type" or something kind of description that describes in what way things are broken. We have to distinguish here between things like: reference frame missing, some bits not as they should be, etc Also we need to distinguish between "decoding not possible at all" and "something could be decoded but might look awful". Upstream could then decide to request a new keyframe, or lower the bitrate, or do other things.
Also it should not be handled by the base class, but the subclasses would explicitly call this function to describe the problem. Only the subclasses know about that.
If we had a new event, another really useful information is "what was the last correctly decoded frame" or "what was the last correct decoded reference frame" so we can do long-term reference frame based requests.
I apologize for the delay. I was implementing this, but due to company policy I am unable to release it. Hopefully someone else can work on this.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/91.