After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 720597 - decodebin: query allocation always sent twice
decodebin: query allocation always sent twice
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
1.2.1
Other All
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-12-17 12:07 UTC by Julien Isorce
Modified: 2015-12-11 15:06 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
videodecoder: make sure reconfigure flag is unset when negotiated (1.25 KB, patch)
2013-12-17 12:07 UTC, Julien Isorce
needs-work Details | Review
videodec/enc: allow to call negiotate without ouput state (2.40 KB, patch)
2013-12-30 15:35 UTC, Julien Isorce
none Details | Review
videodecoder: make sure reconfigure flag is unset when negotiated (1.01 KB, patch)
2013-12-30 17:45 UTC, Julien Isorce
reviewed Details | Review
audio/videodecoder: make sure reconfigure flag is unset when negotiated (1.97 KB, patch)
2014-01-02 18:05 UTC, Julien Isorce
needs-work Details | Review
!videodecoder: add debug just for the demo (4.06 KB, patch)
2014-01-08 11:32 UTC, Julien Isorce
none Details | Review
gst log using GST_DEBUG=*videodecoder*:5,*decodebin*:6,2 (213.62 KB, text/x-log)
2014-01-08 11:54 UTC, Julien Isorce
  Details
videodecoder: add some debug around pool negotiation (2.91 KB, patch)
2014-01-08 18:32 UTC, Julien Isorce
accepted-commit_now Details | Review
videodecoder: immediately retry to negotiate if set_caps or allocation query reset the reconfigure flag (4.48 KB, patch)
2014-01-08 18:34 UTC, Julien Isorce
needs-work Details | Review
videodecoder: add some debug around pool negotiation (2.92 KB, patch)
2014-01-24 17:20 UTC, Julien Isorce
none Details | Review
videodecoder: immediately retry to negotiate if set_caps or allocation query reset the reconfigure flag (5.38 KB, patch)
2014-01-24 17:21 UTC, Julien Isorce
reviewed Details | Review
videodecoder: add some debug around pool negotiation (2.96 KB, patch)
2015-12-11 05:34 UTC, Julien Isorce
committed Details | Review

Description Julien Isorce 2013-12-17 12:07:34 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.
Comment 1 Sebastian Dröge (slomo) 2013-12-17 12:15:49 UTC
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?
Comment 2 Julien Isorce 2013-12-17 13:03:12 UTC
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 :) ?
Comment 3 Sebastian Dröge (slomo) 2013-12-17 13:47:45 UTC
No you're right :) The flag should be unset somewhere in the middle of the allocation procedure before doing the allocation query
Comment 4 Sebastian Dröge (slomo) 2013-12-17 13:49:00 UTC
I think this should happen inside negotiate_default() though
Comment 5 Julien Isorce 2013-12-18 10:32:27 UTC
(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.
Comment 6 Olivier Crête 2013-12-18 17:56:15 UTC
You probably want to do gst_pad_check_reconfigure () before setcaps, not after.
Comment 7 Julien Isorce 2013-12-19 09:21:14 UTC
(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);
Comment 8 Julien Isorce 2013-12-30 15:35:48 UTC
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 9 Julien Isorce 2013-12-30 15:36:45 UTC
Comment on attachment 264406 [details] [review]
videodecoder: make sure reconfigure flag is unset when negotiated

wrong bug
Comment 10 Julien Isorce 2013-12-30 15:37:13 UTC
Comment on attachment 265038 [details] [review]
videodec/enc: allow to call negiotate without ouput state

wrong bug :)
Comment 11 Julien Isorce 2013-12-30 17:45:07 UTC
Created attachment 265050 [details] [review]
videodecoder: make sure reconfigure flag is unset when negotiated

Tested with decodebin, uridecodebin and playbin
Comment 12 Sebastian Dröge (slomo) 2013-12-31 09:43:22 UTC
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 :)
Comment 13 Julien Isorce 2014-01-02 18:05:35 UTC
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 ?
Comment 14 Sebastian Dröge (slomo) 2014-01-03 10:20:56 UTC
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).
Comment 15 Sebastian Dröge (slomo) 2014-01-03 13:03:57 UTC
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?
Comment 16 Julien Isorce 2014-01-03 15:01:03 UTC
(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.
Comment 17 Sebastian Dröge (slomo) 2014-01-08 09:43:37 UTC
(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.
Comment 18 Julien Isorce 2014-01-08 11:32:53 UTC
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
Comment 19 Julien Isorce 2014-01-08 11:54:38 UTC
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.
Comment 20 Julien Isorce 2014-01-08 18:32:32 UTC
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)
Comment 21 Julien Isorce 2014-01-08 18:34:08 UTC
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
Comment 22 Sebastian Dröge (slomo) 2014-01-08 19:06:09 UTC
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...
Comment 23 Sebastian Dröge (slomo) 2014-01-08 19:09:09 UTC
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;
  }
}
Comment 24 Sebastian Dröge (slomo) 2014-01-21 09:08:06 UTC
Julien?
Comment 25 Julien Isorce 2014-01-21 09:11:18 UTC
I'm currently busy on another stuff but I'll come back to it soon :) (I have not give up :) )
Comment 26 Julien Isorce 2014-01-24 17:20:59 UTC
Created attachment 267142 [details] [review]
videodecoder: add some debug around pool negotiation

see comment #22
Comment 27 Julien Isorce 2014-01-24 17:21:54 UTC
Created attachment 267143 [details] [review]
videodecoder: immediately retry to negotiate if set_caps or allocation query reset the reconfigure flag

see comment #23
Comment 28 Sebastian Dröge (slomo) 2014-05-03 08:06:07 UTC
What's the status here? I think it makes sense but we should do the same for all the other base classes then.
Comment 29 Sebastian Dröge (slomo) 2014-05-03 08:11:33 UTC
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
Comment 30 Sebastian Dröge (slomo) 2014-05-26 07:30:48 UTC
Julien?
Comment 31 Julien Isorce 2015-09-07 09:51:21 UTC
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 32 Julien Isorce 2015-12-11 05:32:11 UTC
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)
Comment 33 Julien Isorce 2015-12-11 05:34:27 UTC
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.
Comment 34 Sebastian Dröge (slomo) 2015-12-11 08:02:10 UTC
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, &params);
 
+  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 35 Julien Isorce 2015-12-11 15:01:17 UTC
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
Comment 36 Julien Isorce 2015-12-11 15:06:56 UTC
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!