GNOME Bugzilla – Bug 719684
videodecoder: Allocation query is always at least sent twice
Last modified: 2013-12-30 04:03:47 UTC
Created attachment 263299 [details] [review] gstvideodecoder: clear GST_PAD_FLAG_NEED_RECONFIGURE flag if gst_video_decoder_negotiate succeeds * I noticed that GstVideoDecoder::gst_video_decoder_negotiate does not clear the GST_PAD_FLAG_NEED_RECONFIGURE flag when it succeeds. It causes the decoder to negotiate again, so query allocation is done twice. (the second time in gst_video_decoder_allocate_output_frame), even if the first negotiation succeeded. * steps to reproduce: gst-launch-1.0 v4l2src io-mode=mmap ! jpegdec ! xvimagesink Since recently it seems that a pad is initially marked to GST_PAD_FLAG_NEED_RECONFIGURE : http://cgit.freedesktop.org/gstreamer/gstreamer/commit/gst/gstpad.c?id=c279bdb663de532be58b31970b26ff515ff4f098 * other informations: the negotiation design http://cgit.freedesktop.org/gstreamer/gstreamer/tree/docs/design/part-negotiation.txt
I agree, this is something we usually fully do in the baseclass, anyone see a problem with change ? I *think* it should not react badly with decoders that do unset the flag themeself, but better check then being sorry.
Yes, see my comment in the other bug :)
Review of attachment 263299 [details] [review]: Fix the commit message please, just start with videodecoder: and add the bug URL ::: gst-libs/gst/video/gstvideodecoder.c @@ +3198,2 @@ GST_VIDEO_DECODER_STREAM_LOCK (decoder); + if (klass->negotiate && gst_pad_check_reconfigure (decoder->srcpad)) { This means that calling negotiate explicitly might not trigger another try of negotiation. This is a change in behaviour, and e.g. will cause problems if priv->output_state_changed but the reconfigure flag is not set on the pad. Always call ->negotiate(), and then mark the pad is not needing reconfiguration if negotiation succeeded
I think if the downstream element sends a reconfigure event, the call to "klass->negotiate (decoder)" from "gst_video_decoder_negotiate" will be done automatically when the decoder calls any of the following functions: gst_video_decoder_allocate_output_buffer gst_video_decoder_allocate_output_frame gst_video_decoder_finish_frame But in case this is not valid, here are what you asked: if (klass->negotiate) ret = klass->negotiate (decoder); /* clear GST_PAD_FLAG_NEED_RECONFIGURE if negotiation succeeded */ if (ret) gst_pad_check_reconfigure (decoder->srcpad); Is it what you thought ?
(In reply to comment #4) > I think if the downstream element sends a reconfigure event, the call to > "klass->negotiate (decoder)" from "gst_video_decoder_negotiate" will be done > automatically when the decoder calls any of the following functions: > > gst_video_decoder_allocate_output_buffer > gst_video_decoder_allocate_output_frame > gst_video_decoder_finish_frame Yes, it will be called in these cases. But also subclasses often call it from e.g. GstVideoDecoder::set_format() to let negotiation happen ASAP. > But in case this is not valid, here are what you asked: > > if (klass->negotiate) > ret = klass->negotiate (decoder); > > /* clear GST_PAD_FLAG_NEED_RECONFIGURE if negotiation succeeded */ > if (ret) > gst_pad_check_reconfigure (decoder->srcpad); > > Is it what you thought ? Well, it's racy :) But in principle yes. I think to make it not racy you'll have to call check_reconfigure() first to unmark it in any case... and if negotiate() fails you mark it for reconfiguration again. (or probably just mark it always for reconfiguration if it fails... if it fails things have to be retried ASAP or fail completely).
Created attachment 263387 [details] [review] gstvideodecoder: clear GST_PAD_FLAG_NEED_RECONFIGURE flag if gst_video_decoder_negotiate succeeds Add gst_video_decoder_negotiate_unlocked for internal uses. So that we can have more control of what we are doing inside GstVideoDecoder. And now gst_video_decoder_negotiate always clear the reconfigure flag. And set it again if negotiate fails.
(In reply to comment #5) > (In reply to comment #4) > > I think if the downstream element sends a reconfigure event, the call to > > "klass->negotiate (decoder)" from "gst_video_decoder_negotiate" will be done > > automatically when the decoder calls any of the following functions: > > > > gst_video_decoder_allocate_output_buffer > > gst_video_decoder_allocate_output_frame > > gst_video_decoder_finish_frame > > Yes, it will be called in these cases. But also subclasses often call it from > e.g. GstVideoDecoder::set_format() to let negotiation happen ASAP. I think need_reconfigure = check_reconfigure (pad); "if (klass->negociate && need_reconfigure) then negotiate() ..." would be ok also for the asap case GstVideoDecoder::set_format() because a pad is initially marked GST_PAD_FLAG_NEED_RECONFIGURE (see comment #1) But it depends if a pad goes to inactive when going to ready state. And I'm not sure about that.
Review of attachment 263387 [details] [review]: ::: gst-libs/gst/video/gstvideodecoder.c @@ +3182,3 @@ +static gboolean +gst_video_decoder_negotiate_unlocked (GstVideoDecoder * decoder) Why do you add this? Note that the stream lock is a recursive lock
It was mainly to avoid double check of the flag, In the previous patch, there were gst_pad_needs_reconfigure and then gst_pad_check_reconfigure. It also allows to have more control on what we choose to do inside GstVideoDecoder so that we do not depends on the gst_video_decoder_negotiate behavior that can be called from outside. Then I used this opportunity to also remove the lock even if recursive. Maybe the function should be renamed to gst_video_decoder_negotiate_unchecked or something. Let me know what do you think (and also about comment #7) and I will remake the whole patch :)
Comment on attachment 263387 [details] [review] gstvideodecoder: clear GST_PAD_FLAG_NEED_RECONFIGURE flag if gst_video_decoder_negotiate succeeds No, I'm ok with that. Please do the same change in audiodecoder, audioencoder and videodecoder
Created attachment 263495 [details] [review] videoencoder: clear GST_PAD_FLAG_NEED_RECONFIGURE flag if negotiate succeeds
Created attachment 263496 [details] [review] audiodecoder/enc: clear GST_PAD_FLAG_NEED_RECONFIGURE flag if negotiate succeeds
Review of attachment 263387 [details] [review]: ::: gst-libs/gst/video/gstvideodecoder.c @@ +3249,3 @@ if (G_UNLIKELY (!decoder->priv->output_state || decoder->priv->output_state_changed || gst_pad_check_reconfigure (decoder->srcpad))) { There's a bug here btw, and in all the other patches. You only check and unset the reconfigure flag if any of the other conditions fails. So will go here twice still if first one of the other conditions is true.
Comment on attachment 263495 [details] [review] videoencoder: clear GST_PAD_FLAG_NEED_RECONFIGURE flag if negotiate succeeds Looks good except for the same problem as in videodecoder
Comment on attachment 263496 [details] [review] audiodecoder/enc: clear GST_PAD_FLAG_NEED_RECONFIGURE flag if negotiate succeeds Looks good except for the same problem as in videodecoder
ah right, nice catch :) thx. So do you think I should go back to the first solution (using gst_pad_needs_reconfigure and using the same negotiate function) ? or move gst_pad_check_reconfigure (decoder->srcpad) to the first place in the if ?
I would put it to the first place in the if, or just call it unconditionally before the if and remember the return value.
Created attachment 263592 [details] [review] videodec/enc: clear reconfigure flag if negotiate succeeds
Created attachment 263593 [details] [review] audiodec/enc: clear reconfigure flag if negotiate succeeds
commit e68317f070caf533c582c9fe2795e21b75f4747f Author: Julien Isorce <julien.isorce@collabora.co.uk> Date: Thu Dec 5 14:39:57 2013 +0000 audiodec/enc: clear reconfigure flag if negotiate succeeds So that it avoids to send an allocation query twice. One from an early call to gst_audio_encoder_negotiate from a subclass, then one from gst_audio_encoder_allocate_output_buffer. Which means that previously gst_audio_encoder_negotiate was not clearing the GST_PAD_FLAG_NEED_RECONFIGURE even on success. Fixes bug https://bugzilla.gnome.org/show_bug.cgi?id=719684 commit 79ef75888c88c5883b8963c9e85d4260fd57f72d Author: Julien Isorce <julien.isorce@collabora.co.uk> Date: Thu Dec 5 14:31:25 2013 +0000 videodec/enc: clear reconfigure flag if negotiate succeeds So that it avoids to send an allocation query twice. One from an early call to gst_video_encoder_negotiate from a subclass, then one from gst_video_encoder_allocate_output_frame. Which means that previously gst_video_encoder_negotiate was not clearing the GST_PAD_FLAG_NEED_RECONFIGURE even on success. Fixes bug https://bugzilla.gnome.org/show_bug.cgi?id=719684
Backported to the 1.2 branch. Changing the target milestone accordingly.