GNOME Bugzilla – Bug 720597
decodebin: query allocation always sent twice
Last modified: 2015-12-11 15:06:56 UTC
Created attachment 264406 [details] [review] videodecoder: make sure reconfigure flag is unset when negotiated * steps to reproduce: gst-launch-1.0 filesrc location=/home/julien/Videos/mp4/big_buck_bunny.mp4 ! decodebin ! xvimagesink * but it does not happen when setting the decode chain manually: gst-launch-1.0 filesrc location=/home/julien/Videos/mp4/big_buck_bunny.mp4 ! qtdemux ! avdec_h264 ! xvimagesink * what happen: - gst_video_decoder_negotiate is called from the decoder - gst_pad_check_reconfigure (decoder->srcpad); is called from gst_video_decoder_negotiate so NEED_RECONFIGURE flag is unset on decoder src pad - then gst_video_decoder_negotiate_default is called which itself calls gst_pad_set_caps. in the mean time gst_pad_link_full between the decodebin and the sink which create a reconfigure event. - the result is that before this "gst_pad_set_caps" call the reconfigure flag is unset and just after "gst_pad_set_caps" the reconfigure flag is marked reconfigure. So that the gst_pad_check_reconfigure call from gst_video_decoder_negotiate is crossed out (like no effect) I think gst_pad_set_caps implies the link of the decodebin src pad but I'm not sure why. So that this is 100% reproductible. - the solution is to move gst_pad_check_reconfigure (decoder->srcpad); to the end of gst_video_decoder_negotiate, so that we make sure the reconfigure flag is unset when returing from this function. I'm not sure if the attached patch is just a workaround or if something should also be fixed in decodebin. In the case the patch is ok let me know and I'll do it for videoencoder, audiodec and audioenc too.
It's correct that decodebin links the srcpad of the decoder when the CAPS event is sent. But I don't think it's correct ignore the RECONFIGURE flag that was set because of the linking. There might be a more optimal format or way of allocation now that things are linked. What's the problem you're trying to fix here?
actually I think the reconfigure event is not needed in this case. Because it's sent before to even try the first negotiation. So that even if the first negotiation succeeds it will always try to reconfigure. What happens exactly: - gst_video_decoder_negotiate from avdec_h264 - gst_video_decoder_negotiate_default - gst_pad_set_caps - link pads between decodebin and the next downstream element, and this link function sends a reconfigure event - first attempt to negotiation (query allocation/propose/decide) - negotiation succeeds between decoder and next downstream element - at this stage the src pad is still marked reconfigure. - so that the next call the decoder_output_frame re-sends the query allocation. I do not see why it could have a more optimal format or way of allocation in this case. So that I think it's unnecessary to again the negotiation. Which is what I'm trying to fix. I think if the next downstream element to decodebin is not a sink and is linked later it will implies to send a reconfigure event, so that in this case it could have a more optimal format or way of allocation. If I'm wrong, what am I missing :) ?
No you're right :) The flag should be unset somewhere in the middle of the allocation procedure before doing the allocation query
I think this should happen inside negotiate_default() though
(In reply to comment #3) > No you're right :) The flag should be unset somewhere in the middle of the > allocation procedure before doing the allocation query I was about to do the fix like the following in gst_video_decoder_negotiate_default: ret = gst_pad_set_caps (decoder->srcpad, state->caps); +if (ret) + gst_pad_check_reconfigure (decoder->srcpad); But then I noticed that when using playbin (or even 2 decodebin: gst-launch-1.0 filesrc location=/home/julien/Videos/mp4/big_buck_bunny.mp4 ! decodebin ! decodebin ! xvimagesink ) then the call gst_pad_peer_query set the reconfigure flag. So maybe clearing the flag before calling klass->decide_allocation (decoder, query) is a better place than between gst_pad_set_caps and gst_pad_peer_query. So something like that in gst_video_decoder_negotiate_pool: +gst_pad_check_reconfigure (decoder->srcpad); + g_assert (klass->decide_allocation != NULL); ret = klass->decide_allocation (decoder, query); what do you think ? Also I think in the end this patch will increase the time that the pipeline spends before reaching the PAUSED state, mostly for big graphs.
You probably want to do gst_pad_check_reconfigure () before setcaps, not after.
(In reply to comment #6) > You probably want to do gst_pad_check_reconfigure () before setcaps, not after. There is already one call to gst_pad_check_reconfigure just before to enter in ->negotiate, see http://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/gst-libs/gst/video/gstvideodecoder.c#n3277 The pb is that calling setcaps (and gst_pad_peer_query) imply that the reconfigure flag is set again, see #2 and #5 So my solution is in gst_video_decoder_negotiate_pool: +gst_pad_check_reconfigure (decoder->srcpad); + g_assert (klass->decide_allocation != NULL); ret = klass->decide_allocation (decoder, query);
Created attachment 265038 [details] [review] videodec/enc: allow to call negiotate without ouput state Move "g_return_val_if_fail" from negotiate() to negotiate_default()
Comment on attachment 264406 [details] [review] videodecoder: make sure reconfigure flag is unset when negotiated wrong bug
Comment on attachment 265038 [details] [review] videodec/enc: allow to call negiotate without ouput state wrong bug :)
Created attachment 265050 [details] [review] videodecoder: make sure reconfigure flag is unset when negotiated Tested with decodebin, uridecodebin and playbin
Comment on attachment 265050 [details] [review] videodecoder: make sure reconfigure flag is unset when negotiated It is set because of the allocation (typo btw) query? That doesn't sound correct. You mean it is set because of the gst_pad_set_caps() that happened before? If so please update the comment and push :)
Created attachment 265175 [details] [review] audio/videodecoder: make sure reconfigure flag is unset when negotiated (In reply to comment #12) > (From update of attachment 265050 [details] [review]) > It is set because of the allocation (typo btw) query? That doesn't sound > correct. > > You mean it is set because of the gst_pad_set_caps() that happened before? If > so please update the comment and push :) Yup and the right place is just after gst_pad_set_caps in gst_video_decoder_negotiate_default. But with playsink (in playbin2) it's happening a behaviour that I do not understand. Is it ok that allocation can happen before pads are unblocked in playsink ? Because this is what can happen. And then the allocation query is sent a second time just after pads are unblocked. Any idea ?
Yes that's valid. decodebin does not block on ALLOCATION queries but allows them to go through (otherwise we'll deadlock easily, check the gstdecodebin2.c commit log).
I'm confused what the actual problem now is here :) I think it's correct that another ALLOCATION query happens if one happened before and then things are relinked (e.g. because decodebin exposes the pads, or because it continues autoplugging). Doing two ALLOCATION queries just because and if nothing has changed is not ideal though, but is this happening here?
(In reply to comment #15) > I'm confused what the actual problem now is here :) I think it's correct that > another ALLOCATION query happens if one happened before and then things are > relinked (e.g. because decodebin exposes the pads, or because it continues > autoplugging). Sorry I wasn't clear, the patch 265175 fixed the fact there were 2 allocation queries in decodebin (for each stream). If you are ok with the patch I'll push it and close this bug. (gst-launch-1.0 filesrc location=videofile ! decodebin ! xvimagesink) > Doing two ALLOCATION queries (before and after) just because and if nothing has > changed is not ideal though, but is this happening here? I agree and yes it's happening. But I think I should open a different bug for that.
(In reply to comment #16) > (In reply to comment #15) > > I'm confused what the actual problem now is here :) I think it's correct that > > another ALLOCATION query happens if one happened before and then things are > > relinked (e.g. because decodebin exposes the pads, or because it continues > > autoplugging). > > Sorry I wasn't clear, the patch 265175 [details] fixed the fact there were 2 allocation > queries in decodebin (for each stream). > If you are ok with the patch I'll push it and close this bug. > > (gst-launch-1.0 filesrc location=videofile ! decodebin ! xvimagesink) Can you provide a complete analysis of the situation and why the 2 allocation queries happen here? Also in the commit message :) I'm not sure anymore if this fix isn't breaking something in theory.
Created attachment 265688 [details] [review] !videodecoder: add debug just for the demo do not review it, this is just to explain what happen. See next comment
Created attachment 265691 [details] gst log using GST_DEBUG=*videodecoder*:5,*decodebin*:6,2 Pipeline: GST_DEBUG=*videodecoder*:5,*decodebin*:6,2 gst-launch-1.0 uridecodebin uri=http://clips.vorwaerts-gmbh.de/big_buck_bunny.mp4 ! xvimagesink In the attached log you can see at line 564 that avdec_h264's src pad is linked whereas decodebin's src_0 pad is not. Then gst_video_decoder_negotiate_default::"gst_pad_set_caps (decoder->srcpad, state->caps);" is called Them avdec_h264's src pad is set to be blocked by its parent (decodebin) and when this function returns you can see in the attached log at line 652 that now the decodebin's src_0 pad is linked. At this point NO allocation query has been sent yet by the avdec_h264. But also at this point the avdec_h264's src pad has the reconfigure flag set. This is the problem but let's continue a bit and talk about it later in this comment. So then the first allocation query happens. Then at the next call to "gst_video_decoder_allocate_output_frame" the negotiation is done again because of the reconfigure flag. And here we have the 2 allocation queries. Also note that the second alloc query adds nothing because the caps has not changed and also it causes 2 warnings because the pool is already active. Ok now go back to the reconfigure flag problem. It is set again because the decodebin's src_0 was linked (a link always implies to set the RECONFIGURE flag). And as this pad is a proxy of avdec_h264's src pad it causes to set the reconfigure flag too. I do not know how this works exactly about proxy/ghost pads but it seems this is what happen. Or maybe a reconfigure event is then sent from decodebin's src pad which is transmited to avdec_h264's src pad. I'm also wondering if the problem is that at line 564 we have avdec_h264's src pad linked whereas decodebin's src pad isn't (and actually does not exist yet) If this is actually the problem then we have to fix deocdebin which would remove the first allocation query. But if this is not the problem then I think the patch 265175 is correct.
Created attachment 265734 [details] [review] videodecoder: add some debug around pool negotiation remove g_prints (see https://bugzilla.gnome.org/show_bug.cgi?id=721400)
Created attachment 265736 [details] [review] videodecoder: immediately retry to negotiate if set_caps or allocation query reset the reconfigure flag If ok I'll do it for audiodecoder too
Review of attachment 265734 [details] [review]: Generally good, just fix the comments and push. And please do the same for the other 3 base classes :) ::: gst-libs/gst/video/gstvideodecoder.c @@ +3115,3 @@ query = gst_query_new_allocation (caps, TRUE); + GST_DEBUG_OBJECT (decoder, "DO query ALLOCATION"); Maybe don't scream "DO" :) @@ +3155,2 @@ if (decoder->priv->pool) { + GST_DEBUG_OBJECT (decoder, "inactivate pool %" GST_PTR_FORMAT, deactivate? @@ +3242,2 @@ ret = TRUE; + GST_DEBUG_OBJECT (decoder, "previous caps and state caps are the same"); ...and *output* state caps...
Review of attachment 265736 [details] [review]: Why so complicated? Please simplify the code and also don't store the counter in the instance :) Should be possible to just do something like: negotiate() { if (!reconfigure) return; x = 0; while (x < NUM) { negotiate_caps(); if (reconfigure) continue; negotiate_pool(); if (reconfigure) continue; } }
Julien?
I'm currently busy on another stuff but I'll come back to it soon :) (I have not give up :) )
Created attachment 267142 [details] [review] videodecoder: add some debug around pool negotiation see comment #22
Created attachment 267143 [details] [review] videodecoder: immediately retry to negotiate if set_caps or allocation query reset the reconfigure flag see comment #23
What's the status here? I think it makes sense but we should do the same for all the other base classes then.
Review of attachment 267143 [details] [review]: ::: gst-libs/gst/video/gstvideodecoder.c @@ -3334,3 @@ - decoder->priv->output_state_changed = FALSE; - /* Negotiate pool */ - ret = gst_video_decoder_negotiate_pool (decoder, state->caps); Note that negotiate_default calls negotiate_pool in some cases at the very very top too. I think this can cause problems as you now unconditionally call negotiate_pool *after* negotiate (_default) @@ +3390,3 @@ + + state = gst_video_decoder_get_output_state (decoder); + ret = gst_video_decoder_negotiate_pool (decoder, state->caps); Maybe pass the complete state here instead of just the caps? For future extensibility
Sorry I have not got time to go further. This issue would require couple of days to refresh what happened + find a proper solution. So I cannot have a look in the next months. Also I think this bug #742924 may be related.
Comment on attachment 267143 [details] [review] videodecoder: immediately retry to negotiate if set_caps or allocation query reset the reconfigure flag I think Thiago solved it with #757653. (or was already solved by #742924)
Created attachment 317182 [details] [review] videodecoder: add some debug around pool negotiation I rebased debug patch so I can at least push that one if still ok.
Review of attachment 317182 [details] [review]: One small change, please commit after that :) ::: gst-libs/gst/video/gstvideodecoder.c @@ +3626,3 @@ gst_buffer_pool_config_set_allocator (config, allocator, ¶ms); + GST_DEBUG_OBJECT (decoder, "setting config %" GST_PTR_FORMAT, pool); Maybe make this GST_DEBUG_OBJECT (decoder, "setting config %" GST_PTR_FORMAT " in pool %" GST_PTR_FORMAT, config, pool)
Comment on attachment 317182 [details] [review] videodecoder: add some debug around pool negotiation commit 4f396ae61ca5abf979ceb0febb9096789f5deabe Author: Julien Isorce <j.isorce@samsung.com> Date: Fri Dec 11 14:42:09 2015 +0000 videodecoder: add some debug around pool negotiation It lets us know easily which pool is activated or inactivated during the negotiation. https://bugzilla.gnome.org/show_bug.cgi?id=720597
I confirm I now see only one "do query ALLOCATION" in the pipeline mentioned in comment #1 in "steps to reproduce" with current master without any local patch. I am pretty sure the problem has been fixed by patches already committed in https://bugzilla.gnome.org/show_bug.cgi?id=742924 Marking as fixed and not obsolete since there is also this patch about debug I just pushed, see comment #35 Great!