GNOME Bugzilla – Bug 760666
vp8enc: Do not mix up Booleans with FlowReturn
Last modified: 2016-01-20 08:19:41 UTC
This should be the correct behaviour and avoids segfault as that frame does not have user data anymore.
Created attachment 319091 [details] [review] vp8enc: Drop frames when failing encoding them Otherwise it will segfault on next working iteration while processing buffer as the user_data is set to NULL.
Created attachment 319092 [details] [review] vp8enc: Return FLOW_ERROR when an error accures FALSE would mean FLOW_OK
Even if you return FLOW_ERROR you may still be called again with another buffer (and shouldn't crash then). Not sure if that'd be a problem or not, just thought I'd mention it in case it would be.
Created attachment 319105 [details] [review] videoencoder: Release video frame when ->handle return ERROR or DROPPED
Review of attachment 319105 [details] [review]: ::: gst-libs/gst/video/gstvideoencoder.c @@ +215,3 @@ +static void +gst_video_encoder_release_frame (GstVideoEncoder * enc, + GstVideoCodecFrame * frame); Why? @@ +1484,3 @@ + if (ret == GST_VIDEO_ENCODER_FLOW_DROPPED || ret == GST_FLOW_ERROR) { + GST_WARNING_OBJECT (encoder, "Dropping frame %p", frame); + gst_video_encoder_release_frame (encoder, frame); I would be interested in why this never caused any problems so far, and what the problems are now. I see this potentially causing new problems in old code that assumes that the frames themselves still stay around. @@ +1861,1 @@ gst_video_encoder_release_frame (GstVideoEncoder * enc, Why? Looks like leftover from some experiments
(In reply to Sebastian Dröge (slomo) from comment #5) > Review of attachment 319105 [details] [review] [review]: > > ::: gst-libs/gst/video/gstvideoencoder.c > @@ +215,3 @@ > +static void > +gst_video_encoder_release_frame (GstVideoEncoder * enc, > + GstVideoCodecFrame * frame); > > Why? So I can do it later. > @@ +1484,3 @@ > + if (ret == GST_VIDEO_ENCODER_FLOW_DROPPED || ret == GST_FLOW_ERROR) { > + GST_WARNING_OBJECT (encoder, "Dropping frame %p", frame); > + gst_video_encoder_release_frame (encoder, frame); > > I would be interested in why this never caused any problems so far, and what > the problems are now. I see this potentially causing new problems in old > code that assumes that the frames themselves still stay around. This is a bug, you should drop frame on GST_VIDEO_ENCODER_FLOW_DROPPED which is what the documentation say... this is just doing it here now. > @@ +1861,1 @@ > gst_video_encoder_release_frame (GstVideoEncoder * enc, > > Why? Looks like leftover from some experiments Fixed.
Created attachment 319114 [details] [review] videoencoder: Release video frame when ->handle return ERROR or DROPPED
Review of attachment 319114 [details] [review]: ::: gst-libs/gst/video/gstvideoencoder.c @@ +1483,3 @@ + if (ret == GST_VIDEO_ENCODER_FLOW_DROPPED || ret == GST_FLOW_ERROR) { + GST_WARNING_OBJECT (encoder, "Dropping frame %p", frame); I think this shouldn't be a warning but less, otherwise seems good to go
Comment on attachment 319114 [details] [review] videoencoder: Release video frame when ->handle return ERROR or DROPPED Attachment 319114 [details] pushed as 63517d0 - videoencoder: Release video frame when ->handle return ERROR or DROPPED
Attachment 319092 [details] pushed as 1783882 - vp8enc: Return FLOW_ERROR when an error accures
t show
Not sure I should backport the GstVideoEncoder part, should I?
> Not sure I should backport the GstVideoEncoder part, should I? Just did (before seeing the question here). Any reason not to?
(In reply to Tim-Philipp Müller from comment #13) > > Not sure I should backport the GstVideoEncoder part, should I? > > Just did (before seeing the question here). Any reason not to? I personally think it is safe but Sebastien said "I would be interested in why this never caused any problems so far, and what the problems are now. I see this potentially causing new problems in old code that assumes that the frames themselves still stay around." which is why I prefered to ask before cherry picking.
Right, on second thought I wonder if subclasses may have released the frame themselves, after all the flow return is called 'DROPPED' and not 'DROP'. Guess we should investigate. What made you change this? Did you notice a leak or memory building up in the encoder?
I changed this because it is what the doc says": > #define GST_VIDEO_ENCODER_FLOW_DROPPED GST_FLOW_CUSTOM_SUCCESS_1 > > Returned when the event/buffer should be dropped. But the AFAICT nobody uses that right now anyway. And when an ERROR occured we should also make sure the frame won't be used later on (see the other patch).
Ok, if it doesn't fix anything specifically we should probably not ship it in 1.6. Also, in GstVideoDecoder we have: GstFlowReturn gst_video_decoder_drop_frame (GstVideoDecoder *dec, GstVideoCodecFrame *frame); perhaps we should have the same in the encoder?
(In reply to Tim-Philipp Müller from comment #17) > Ok, if it doesn't fix anything specifically we should probably not ship it > in 1.6. The handling of GST_FLOW_ERROR fix a potential race with the patch I pushed on the vp8enc. > Also, in GstVideoDecoder we have: > > GstFlowReturn gst_video_decoder_drop_frame (GstVideoDecoder *dec, > GstVideoCodecFrame *frame); > > perhaps we should have the same in the encoder? The doc says: GstFlowReturn gst_video_encoder_finish_frame (GstVideoEncoder *encoder, GstVideoCodecFrame *frame); @frame must have a valid encoded data buffer, whose metadata fields are then appropriately set according to frame data or no buffer at all if the frame should be dropped. It is subsequently pushed downstream or provided to pre_push . In any case, the frame is considered finished and released. So I am not sure it is necessary to add another method?
Ok, let's leave it as is then :)
It also breaks something. Run > gst-launch-1.0 videotestsrc ! x264enc ! avdec_h264 ! xvimagesink and close the window. ERROR: from element /GstPipeline:pipeline0/GstXvImageSink:xvimagesink0: Output window was closed Additional debug info: xvimagesink.c(555): gst_xv_image_sink_handle_xevents (): /GstPipeline:pipeline0/GstXvImageSink:xvimagesink0 Execution ended after 0:00:01.722223530 Setting pipeline to PAUSED ... Setting pipeline to READY ... ** (gst-launch-1.0:11323): CRITICAL **: gst_video_codec_frame_unref: assertion 'frame->ref_count > 0' failed
Created attachment 319386 [details] [review] vp8enc: Drop frame on ERROR http://bugzilla.gnome.org/show_bug.cgi?id=760666
Created attachment 319387 [details] [review] videoencoder: Do not drop frame on ERROR flow That was a change of behaviour introduce in commit 63517d0ed348784cce4ab4b295c2c0f1b78baa81 http://bugzilla.gnome.org/show_bug.cgi?id=760666
Created attachment 319388 [details] [review] videoencoder: Do not drop frame on ERROR flow That was a change of behaviour introduced in commit 63517d0ed348784cce4ab4b295c2c0f1b78baa81 http://bugzilla.gnome.org/show_bug.cgi?id=760666
Comment on attachment 319388 [details] [review] videoencoder: Do not drop frame on ERROR flow Why? Where is it released in case of errors? The change seems to make sense though, release only if no error and explicitly requested to be dropped.
Comment on attachment 319388 [details] [review] videoencoder: Do not drop frame on ERROR flow Actually this is wrong with regard to refcounting. handle_frame() takes ownership of the frame, you must not unref it here. I think for the DROPPED case, release_frame(ref(frame)) should be called. Which removes the frame from the list but does not destroy it. handle_frame() should do that if needed.
Actually also wrong :) You would need to get a reference before handle_frame(). It might be gone already otherwise.
Created attachment 319389 [details] [review] Revert "videoencoder: Release video frame when ->handle return ERROR or DROPPED" This reverts commit 63517d0ed348784cce4ab4b295c2c0f1b78baa81. It was wrong ref counting wise and we decided to deprecated DROPPED return value
Created attachment 319390 [details] [review] videoencoder: Deprecate GST_VIDEO_ENCODER_FLOW_DROPPED It was never actually supported or used
Comment on attachment 319386 [details] [review] vp8enc: Drop frame on ERROR Just unref should be enough here
Attachment 319389 [details] pushed as 7d35a07 - Revert "videoencoder: Release video frame when ->handle return ERROR or DROPPED" Attachment 319390 [details] pushed as 1bf18f6 - videoencoder: Deprecate GST_VIDEO_ENCODER_FLOW_DROPPED
commit 7eee775d5f3ee705abd7805838cd48cab1f6870a Author: Sebastian Dröge <sebastian@centricular.com> Date: Wed Jan 20 10:02:48 2016 +0200 vpxdec: Unref frame in all code paths of handle_frame() https://bugzilla.gnome.org/show_bug.cgi?id=760666 commit 01f995b8ddc4f86e12ff32788ecd902c930129b5 Author: Thibault Saunier <tsaunier@gnome.org> Date: Tue Jan 19 22:49:20 2016 +0100 vpxenc: Unref frame on ERROR All code paths for handle_frame() must somehow take ownership of the frame, be it by actually unreffing, forwarding the frame elsewhere or storing it for later. http://bugzilla.gnome.org/show_bug.cgi?id=760666
Also backported all to 1.6 for 1.6.3, and applied the changes to the VP9 elements too there.