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 668471 - [h264parse] Negotiation regression for autoplugging
[h264parse] Negotiation regression for autoplugging
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
0.10.x
Other Linux
: Normal blocker
: 0.10.23
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2012-01-23 07:39 UTC by Sebastian Dröge (slomo)
Modified: 2012-02-03 10:33 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Sebastian Dröge (slomo) 2012-01-23 07:39:28 UTC
Hi,

9d0c7d054eafc6b92402d283a2e6cdd8cfa5cb1f breaks autoplugging negotiation, e.g. in decodebin2. The problem is that decodebin2 will always add a capsfilter after h264parse immediately, which has all the possible decoder caps as first caps structures and then the parser srcpad caps afterwards. Together with this commit this will cause h264parse to always negotiate to passthrough mode and never to something else.

The capsfilter is used to provide all possible decoder caps to the parser before it has actually negotiated or even looked at the stream. h264parse can then look at the stream, select one of the possible downstream caps (first possible, preferred structure) and afterwards decodebin2 adds the decoder after the capsfilter. The parser srcpad caps are necessary in the capsfilter to cause useful error messages if no decoder for the format is available (instead of just not-negotiated).
Comment 1 Mark Nauwelaerts 2012-01-23 10:31:22 UTC
That sounds well, but what real meaning has h264parse's selection in such case ?

It will then happen to pick whatever caps happened to end up first in those filter caps, which may then very well come down to non-passthrough, although passthrough would be possible, and that happening is what prompted the above commit.

Also, if the filter caps are set to all *possible* caps and h264parse finds that passthrough is among these and selects that, then it should not go wrong afterwards since they were included in possible caps (so there should have been a decoder capable of handling that selected passthrough case).

Problem might be due to decodebin2 (afaics) always adding the (non-fixed) parser src pad caps to the filter_caps which renders mute the rest of the *possible* caps as well as any sort of nifty negotiation a parser might be trying to do (like avoiding useless conversion if possible).
Comment 2 Sebastian Dröge (slomo) 2012-01-23 10:43:34 UTC
(In reply to comment #1)
> That sounds well, but what real meaning has h264parse's selection in such case
> ?
> 
> It will then happen to pick whatever caps happened to end up first in those
> filter caps, which may then very well come down to non-passthrough, although
> passthrough would be possible, and that happening is what prompted the above
> commit.

The caps are ordered by the ranks of the decoders. The preferred decoder on the system should have the highest rank (possibly the hardware decoder, only supporting bytestream) and other decoders lower ranks (e.g. software decoders that can decode all profiles/levels and actually support avc too).

In this case passthrough might be possible for avc but bytestream conversion would be the better decision by h264parse. And it would do conversion if it looked at the caps instead of preferring passthrough.

> Also, if the filter caps are set to all *possible* caps and h264parse finds
> that passthrough is among these and selects that, then it should not go wrong
> afterwards since they were included in possible caps (so there should have been
> a decoder capable of handling that selected passthrough case).
> 
> Problem might be due to decodebin2 (afaics) always adding the (non-fixed)
> parser src pad caps to the filter_caps which renders mute the rest of the
> *possible* caps as well as any sort of nifty negotiation a parser might be
> trying to do (like avoiding useless conversion if possible).

That's what I've written above, yes. It's necessary to add the parser caps to the filter caps, otherwise negotiation would fail if a decoder is missing instead of properly reporting a missing-plugin error. The parser srcpad caps are *always* the last structures in the caps and would only be selected by the parser if everything before that was incompatible and only if this has to end in a missing-plugin error anyway.
Comment 3 Mark Nauwelaerts 2012-01-23 10:54:43 UTC
So far it has not been specified that a get_caps result should necessarily be treated in "first order", and as such not clear/certain it should do conversion if not doing so would also work for downstream (as indicated in caps), and so relying upon such in responding to get_caps is borderline.

Besides that it seems like the rest of the negotiation in h264parse may need some fixing since it not set to handle cases like stream-format = { byte-stream, avc }.  And in this case the set {} there makes it not clear what is 'first' anyway.
Comment 4 Sebastian Dröge (slomo) 2012-01-23 11:09:23 UTC
(In reply to comment #3)
> So far it has not been specified that a get_caps result should necessarily be
> treated in "first order", and as such not clear/certain it should do conversion
> if not doing so would also work for downstream (as indicated in caps), and so
> relying upon such in responding to get_caps is borderline.

No, that was never defined. Correct :) But that's the behaviour all parsers used in the past and that what the special handling of parsers in decodebin2 expects parsers to do.

The alternative would be to intercept gst_pad_push() and gst_pad_set_caps() calls before the parser to catch GST_FLOW_NOT_NEGOTIATED and something to catch error messages from the parser. Or some new API for element negotiation that somehow allows explicit negotiation of elements.

> Besides that it seems like the rest of the negotiation in h264parse may need
> some fixing since it not set to handle cases like stream-format = {
> byte-stream, avc }.  And in this case the set {} there makes it not clear what
> is 'first' anyway.

Agreed
Comment 5 Vincent Penquerc'h 2012-01-23 16:15:13 UTC
For info, this commit has also broken reverse playback of H.264 in MPEG4, see https://bugzilla.gnome.org/show_bug.cgi?id=667560 (two different commits broke this in turn).
Comment 6 Mark Nauwelaerts 2012-01-23 16:59:11 UTC
Will see about fixing/enhancing h264parse negotiation so as to try and prefer the "first choice" (as a courtesy beyond API/specs) while trying to avoid the original (ridiculous) conversion case that was happening.

In any case, whatever output format is decided upon at negotiation does not really affect (or break) reverse playback.  It may just happen to work with the other format, but that is basically a different matter (it used to prefer byte-stream a lot more, which then tends to bring in e.g. ffmpeg parser into the game as well having a chance to 'fix' things if needed at that time).
Comment 7 Vincent Penquerc'h 2012-01-23 17:21:17 UTC
OK. I make no claim as to which bit of code is at fault here, I am just the second level messenger of the git bisect messenger :)
Comment 8 Mark Nauwelaerts 2012-01-23 18:01:05 UTC
(In reply to comment #7)
> OK. I make no claim as to which bit of code is at fault here, I am just the
> second level messenger of the git bisect messenger :)

Don't mind, just added some thoughts about (likely?) root cause (byte-stream vs avc lala etc).
Comment 9 Sebastian Dröge (slomo) 2012-02-02 10:36:49 UTC
Is there anything left to be done here?
Comment 10 Mark Nauwelaerts 2012-02-02 10:47:21 UTC
See comment #5 (first paragraph) and some other likely fixes alluded to before (i.e. dealing properly with set-valued stream-format cases and so on).
Comment 11 Mark Nauwelaerts 2012-02-03 10:33:21 UTC
Following commit should have h264parse still trying for passthrough, but not "too much" ...

commit e80adf0936062c9581dd0a68f3576b5092229403
Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk>
Date:   Fri Feb 3 11:26:53 2012 +0100

    h264parse: decrease passthrough negotiation preference
    
    Also ensure parsing fixed caps when negotiating rather than failing to
    handle non-fixed list cases.
    
    See #668471.