Created attachment 288348 [details] [review]
decodebin: Fix typo in comment

Created attachment 288349 [details] [review]
decodebin: optimize the code a bit by avoiding unnecessary string comparisons

Created attachment 288350 [details] [review]
decodebin: fix the autoplugging of parser elements

There is no caps comparison in the current patch since I didn't manage to find a practical situation where that is necessary ! If anyone can pinpoint a case where multiple parsers for same media format(with different src caps?) are necessary in a single Decodechain, then I can add some caps comparisons too.

Looking more into this, I have a feeling that itz a clear case of major bug which should be fixed, so raising the severity to major. Since it is not a normal use case, I wouldn't prefer a caps comparison overhead without having a proper reason.

As discussed last week, this seems more like a conceptual problem. There should never be multiple parsers for the same format, otherwise they all must implement exactly the same behaviour.

Instead of something like this we should get your changes into h264parse, and you deprecate/disable your own parser if a new enough GStreamer version is found. And in the worst case we'll have two parsers autoplugged then and some computational overhead, but apart from that it should also work fine.

Am I missing something here?

The only problem is the computational overhead, I agree. But that is an enough justification for pushing the patch which is avoiding this overhead IMHO.
In any circumstances decodebin shouldn't autoplug a second parser element of same type. We cannot ask anyone/customer to something like "you are not allowed to install a second plugin which is doing same thing" which seems to be weird for me. It is not only about gstreamer-vaapi. The gstreamer-vaapi is just a use case. Another example, i will install a different parser of same type for codecanalyzer where the patches are not going to integrate to upstrem anytime soon. I could install my own plugin which is doing other parsings too. In any case we shouldn't allow decodebin to autoplug a second plugin. With my patch, the only overhead is the initial checking for some substring, what we gain is the computational gain during the full life time of playback.

It makes debugging from our side more difficult though. We wouldn't have to only ask about which decoder people are using, but also about the parsers.

In any case, for this to be acceptable
- make a GST_DEBUG_OBJECT() in decodebin if there is more than one parser
- prefer Parser/Converter over plain Parser
- only plug one parser in a chain for the same format, i.e. allow multiple parsers if somewhere the caps have changed from the first to the second parser. I think an elegant way of doing that is by just not plugging multiple parsers in a row, but if the previous element is not a parser just add one again if needed

(In reply to comment #8)
> It makes debugging from our side more difficult though. We wouldn't have to
> only ask about which decoder people are using, but also about the parsers.
> 
> In any case, for this to be acceptable
> - make a GST_DEBUG_OBJECT() in decodebin if there is more than one parser

Okay, make sense.

> - prefer Parser/Converter over plain Parser

Okay, But had a feeling that this will add one more overhead with out having any advantage in 99% of the usecases (normal use cases).
 
> - only plug one parser in a chain for the same format, i.e. allow multiple
> parsers if somewhere the caps have changed from the first to the second parser.

previous comment again,,

> I think an elegant way of doing that is by just not plugging multiple parsers
> in a row, but if the previous element is not a parser just add one again if
> needed

This shouldn't work since we are not just adding extra parsers but extra capsfilters also. So the previous element won't be be parser.
Basically every second parser in the system will cause two additions "one parser+ one capsfilter".


I have caps comparions too in my local machine. Still need some more fixes. But i have removed all caps comparisons from the patch because had a feeling that it is unnecessary. Adding all those comparisons just to make a generalization. Do we really need it? :) .. 
Anyway it seems like you are too instinct on that,, I will add those too ;)

(In reply to comment #9)

> > - prefer Parser/Converter over plain Parser
> 
> Okay, But had a feeling that this will add one more overhead with out having
> any advantage in 99% of the usecases (normal use cases).
> 
> > - only plug one parser in a chain for the same format, i.e. allow multiple
> > parsers if somewhere the caps have changed from the first to the second parser.
> 
> previous comment again,,
> 
> > I think an elegant way of doing that is by just not plugging multiple parsers
> > in a row, but if the previous element is not a parser just add one again if
> > needed
> 
> This shouldn't work since we are not just adding extra parsers but extra
> capsfilters also. So the previous element won't be be parser.
> Basically every second parser in the system will cause two additions "one
> parser+ one capsfilter".

I meant looking at the previous GstDecodeElement inside decodebin. There you would find the parser (and the capsfilter as a separate pointer in the same struct).

Created attachment 288985 [details] [review]
decodebin: fix the autoplugging of parser elements

I have relaxed the constraints as you mentioned, means restrict the autoplugging of parsers back to back for same media format but allowing the second parser in all other cases. Also added the debug messages and some code refactoring for better optimization.

Review of attachment 288985 [details] [review]:

I think in general it's enough to test if the previous element is also a parser, checking the media type seems not needed. It's not like some parser is going to convert audio to video or otherwise change the media type in such a way.

::: gst/playback/gstdecodebin2.c
@@ +2135,3 @@
+
+        is_video_parser =
+            strstr (classification, "Video") != NULL ? TRUE : FALSE;

For video we also use "Image" in some places

@@ +2141,3 @@
+        if (!is_video_parser && !is_audio_parser)
+          is_subtitle_parser =
+              strstr (classification, "Subtitle") != NULL ? TRUE : FALSE;

And for subtitles there's also "SubPicture"

(In reply to comment #12)
> Review of attachment 288985 [details] [review]:
> 
> I think in general it's enough to test if the previous element is also a
> parser, checking the media type seems not needed. It's not like some parser is
> going to convert audio to video or otherwise change the media type in such a
> way.

Honestly I thought about that before, but just added for better clarity :)
Will rearrange that.

> 
> ::: gst/playback/gstdecodebin2.c
> @@ +2135,3 @@
> +
> +        is_video_parser =
> +            strstr (classification, "Video") != NULL ? TRUE : FALSE;
> 
> For video we also use "Image" in some places
> 
> @@ +2141,3 @@
> +        if (!is_video_parser && !is_audio_parser)
> +          is_subtitle_parser =
> +              strstr (classification, "Subtitle") != NULL ? TRUE : FALSE;
> 
> And for subtitles there's also "SubPicture"

Okay, Thanks for the review

Created attachment 289011 [details] [review]
decodebin: fix the autoplugging of parser elements

commit aa94d5dc9aa6ef381da6b60a67f218117c662958
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Tue Oct 21 13:30:27 2014 +0200

    decodebin: Fix locking
    
    The chain mutex needs to be locked when looking at chain->elements. Move code
    around a bit to require only one lock() and unlock().

commit 2b0d3927410ae24e6b0fce100bd4ebbbe805a66f
Author: Sreerenj Balachandran <sreerenj.balachandran@intel.com>
Date:   Tue Oct 21 12:58:41 2014 +0300

    decodebin: fix the autoplugging of parser elements
    
    If there are two parser elements available for the same media format,
    then decodebin is autoplugging an extra capsfilter and parser irrespective
    of caps and rank. So restrict the decodebin from autoplugging multiple parser
    elements back to back in adjacent positions with in a single DecodeChain
    for the same media format.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=738416

I don't get this at all. 

We rely on our own plugins in pipeline to process the streams after parsers and before decoders. The plugins have higher ranks than decoders to make sure that they are added to pipeline by decodebin. These patches silently break the behavior. Please reconsider this.

(In reply to comment #16)
> I don't get this at all. 
> 
> We rely on our own plugins in pipeline to process the streams after parsers and
> before decoders. The plugins have higher ranks than decoders to make sure that
> they are added to pipeline by decodebin. These patches silently break the
> behavior. Please reconsider this.

Does it mean that your pipeline requires a second parser element? ! Why?
Or do you use some other element with Klass description "Parser" just after upstream parser element?

It requires second element after parser to do additional processing (i.e. additional timestamp correction, tag extraction from stream, etc). Unless something has changed in the meanwhile the only way to have such element added to pipeline automatically before decoder is to give it "Parser" classification and your patch has disabled that. 

The second parser requires parsed output so it is always plugged in after the "real" parser. Even though it is not a packetizer, it still parses the output so I don't think the classification is wrong.

Yes that seems like a valid use case for a second parser that I didn't consider before. As the change here is only an performance optimization for custom parsers let's revert it.

(In reply to comment #18)
> It requires second element after parser to do additional processing (i.e.
> additional timestamp correction, tag extraction from stream, etc). Unless
> something has changed in the meanwhile the only way to have such element added
> to pipeline automatically before decoder is to give it "Parser" classification
> and your patch has disabled that. 
> 
> The second parser requires parsed output so it is always plugged in after the
> "real" parser. Even though it is not a packetizer, it still parses the output
> so I don't think the classification is wrong.

So you have an element which has the same src_caps and sink_caps of upstream parser element(lets say h264parser for eg). isn't it? Or is it a general Element irrespective of the codec used?

In my case the parser has concrete video caps (parsed) on src/sink pad, i.e. "video/mpeg,mpegversion = (int) [1, 2], parsed=(boolean) true, systemstream = (boolean) false"

I'm not saying that someone else's usecase can't include generic caps, although I'm not sure decodebin would instantiate that.

Interestingly, it should just work if that element was marked as a decoder, whith higher rank to existing decoders. Remains that conceptually this is a parser. Would be nice in the long term if the missing tag extraction, or timestamp correction code could be contributed upstream. If it's needed for you, it is most likely needed by others.

Some of the things that we do (compute correct PTS for MPEG2/MPEG4) might make sense for upstream, although it also increases latency (all b-frames need to be buffered) and most decoders don't really care anyway.

Other things don't really make sense for upstream, i.e. we post extra tag if there is more than one IDR frame in H.264 stream (we need different behavior for streams with IDR frames and without; we're remuxing H.264 streams for HLS and streams without periodic IDR frames need to be transcoded because iOS client can not handle that).

Not a blocker anymore because the patches are reverted.

Should this be WONTFIXed then?

Unless someone can think of a way how to make these conflicting use cases work, I guess so.