GNOME Bugzilla – Bug 796900
avdec: loses the last (num_cores + 1) frames
Last modified: 2018-07-31 18:55:45 UTC
How to reproduce: 1. Update to the latest master. The problem reproduces with ffmpeg 4.0. 2. Create a test file with 3 + (num_cores + 1) frames: NUM_CORES=$(getconf _NPROCESSORS_ONLN) gst-launch-1.0 videotestsrc num-buffers=$((3 + ($NUM_CORES + 1))) ! video/x-raw, framerate=1/1 ! timeoverlay font-desc="Monospace, 64" ! x264enc ! qtmux ! filesink location=/tmp/movie.mp4 3. Play it with gst-play-1.0 or any other GStreamer based player in the same machine, making sure that avdec_h264 is being used as decoder. Expected result: `3 + (num_cores + 1)` frames should be played, during `3 + (num_cores + 1)` seconds. Actual result: Only the first 3 frames are emited by avdec_h264. Then the playback ends. If a file with only `(num_cores + 1)` or fewer frames is open, playback fails completely. This can also be checked with gst-launch: $ gst-launch-1.0 -v filesrc location=/tmp/movie.mp4 ! qtdemux ! avdec_h264 ! fakesink silent=false | grep -c chain 3 It can also be reproduced in a single pipeline if desired: $ gst-launch-1.0 -v videotestsrc num-buffers=$((3 + ($NUM_CORES + 1))) \ ! video/x-raw, framerate=1/1 ! timeoverlay font-desc="Monospace, 64" \ ! x264enc ! avdec_h264 ! fakesink silent=false | grep -c chain 3
I have the impression that it is a regression from the ffmpeg 4.X port. I am also gonna add smoke tests in GstValidate for those cases fwiw.
Created attachment 373230 [details] [review] decoders: fix draining
Sorry about that, it was a pretty trivial mistake. With this patch, both the audio and video decoders are now adequately drained.
Review of attachment 373230 [details] [review]: ::: ext/libav/gstavviddec.c @@ +1723,3 @@ got_frame = gst_ffmpegviddec_frame (ffmpegdec, NULL, &ret); } while (got_frame && ret == GST_FLOW_OK); + avcodec_flush_buffers (ffmpegdec->context); Why do you flush here now? @@ +1731,3 @@ +send_packet_failed: + GST_WARNING_OBJECT (ffmpegdec, "send packet failed, could not drain decoder"); + goto done; Shouldn't it report an error here indeed?
(In reply to Thibault Saunier from comment #4) > Review of attachment 373230 [details] [review] [review]: > > ::: ext/libav/gstavviddec.c > @@ +1723,3 @@ > got_frame = gst_ffmpegviddec_frame (ffmpegdec, NULL, &ret); > } while (got_frame && ret == GST_FLOW_OK); > + avcodec_flush_buffers (ffmpegdec->context); > > Why do you flush here now? Because that was missing, the call to flush_buffers makes it so the decoder is ready to decode again. > > @@ +1731,3 @@ > +send_packet_failed: > + GST_WARNING_OBJECT (ffmpegdec, "send packet failed, could not drain > decoder"); > + goto done; > > Shouldn't it report an error here indeed? No, as you know we don't want to error out at the first problem in the decoders / encoders, it should be translated to GST_AUDIO_DECODER_ERROR along with the rest of the error handling code, but that's a separate issue.
Review of attachment 373230 [details] [review]: (In reply to Mathieu Duponchelle from comment #5) > (In reply to Thibault Saunier from comment #4) > > Review of attachment 373230 [details] [review] [review] [review]: > > > > ::: ext/libav/gstavviddec.c > > @@ +1723,3 @@ > > got_frame = gst_ffmpegviddec_frame (ffmpegdec, NULL, &ret); > > } while (got_frame && ret == GST_FLOW_OK); > > + avcodec_flush_buffers (ffmpegdec->context); > > > > Why do you flush here now? > > Because that was missing, the call to flush_buffers makes it so the decoder > is ready to decode again. Indeed, makes sense, and it was there for the audio decoder. > > @@ +1731,3 @@ > > +send_packet_failed: > > + GST_WARNING_OBJECT (ffmpegdec, "send packet failed, could not drain > > decoder"); > > + goto done; > > > > Shouldn't it report an error here indeed? > > > No, as you know we don't want to error out at the first problem in the > decoders / encoders, it should be translated to GST_AUDIO_DECODER_ERROR > along with the rest of the error handling code, but that's a separate issue. Right, got it.
Attachment 373230 [details] pushed as ff3a8f6 - decoders: fix draining
Thanks for catching that btw Alicia!
The patch fixes indeed the issue, thanks for the fix!