Right now we are invoking the public API gst_codec_utils_h264_caps_set_level_and_profile() inside the h264parser to set the profile and level. 

There are three ways to add the proposed features:

1: Just change the gst_codec_utils_h264_caps_set_level_and_profile() to include all compatible profiles in caps.Caution: This api is using in multiple places including demuxers. This will make the caps as non-fixed too.

2: An API break in pbutils/codec-utils.h by adding a third argument "gboolean strict"  to gst_codec_utils_h264_caps_set_level_and_profile(). If strict==TRUE, it will be the default case and in case of non-strict usage add multiple profiles to caps and that will make the caps as non-fixed too.

3: duplicate the gst_codec_utils_h264_caps_set_level_and_profile() and gst_codec_utils_h264_get_profile() with necessary additions with in h264parser itself.

Suggestions?

I think we can do simpler, with approach 4:

4.a. Keep gst_codec_utils_h264_caps_set_level_and_profile() as is to maintain API and ABI compatibility ;

4.b. Add new helper functions, e.g. gst_codec_utils_h264_get_profile_name(), gst_codec_utils_h264_get_level_name() to return a string representation of the supplied profile_idc and level_idc values.

4.c. If we can derive compatible profiles for src pad caps, I think we could simply gst_structure_copy() the "native" caps, and replace "profile" to the compatible one we determined.

4.d. Then, we could simply append compatible structures and ultimately gst_caps_simplify() the result to have a clean representation with profile={multiview-high, stereo-high, high} (as an example).

We already have APIs for gst_codec_utils_h264_get_profile_name() and gst_codec_utils_h264_get_level_name(), so that shouldn't be a problem.

Initially I was thinking  that a trivial code change  http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=b1a286cd7c6b1a4bc6a8e81cac79aa3ccd5ff523 would be sufficient. But not, pad_set_caps() is only for fixed caps ;)

(In reply to comment #3)
> We already have APIs for gst_codec_utils_h264_get_profile_name() and
> gst_codec_utils_h264_get_level_name(), so that shouldn't be a problem.

They require an SPS block, but anyway, it should be hard to provide those *_name() variants and have the existing ones taking an SPS block use them instead. :)

> Initially I was thinking  that a trivial code change 
> http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=b1a286cd7c6b1a4bc6a8e81cac79aa3ccd5ff523
> would be sufficient. But not, pad_set_caps() is only for fixed caps ;)

Ah, can't we "pre" parse down to the first slice header and then expose non fixed caps for negotiation purposes?

s/should be hard/shouldn't be hard/ i.e. trivial. :)

(In reply to comment #4)
> (In reply to comment #3)
> > Initially I was thinking  that a trivial code change 
> > http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=b1a286cd7c6b1a4bc6a8e81cac79aa3ccd5ff523
> > would be sufficient. But not, pad_set_caps() is only for fixed caps ;)
> 
> Ah, can't we "pre" parse down to the first slice header and then expose non
> fixed caps for negotiation purposes?

Not sure whether I get it correctly!. I think we need a fixed caps anyways. Basically negotiation happened based on query caps (eg: for a manual pipeline, not playbin) and h24parse will return a "video/x-h264" as supported caps. But after that it requires a fixed caps during GST_EVENT_CAPS. right??

We have to set a fixed caps using gst_pad_set_caps() (==sending caps event) from h264parse and which will initiate a query_caps too. So if the downstream elements want to support the profile that h264parse is trying to set in the src pad, it(downstream element) should provide that profile in the query_caps implementation.

Something else that we can do is, (eg: suppose the stream profile id indicate extended profile but constrained_set1_flag set to 1):
-- h264parse needs to query the downstream before setting extended profile to srcpad
-- if returned query caps didn't accept extended profile then check whether it can accept main profile
-- if the main profile is acceptive , set the profile as main profile.

But I am not that supportive to this.

Does anyone have objection to the proposal in comment 7? Or some other ways to do this? If not I will try to implement it..

(In reply to comment #8)
> Does anyone have objection to the proposal in comment 7? Or some other ways to
> do this? If not I will try to implement it..

I don't think you'd need double queries. e.g. for "constrained-baseline", we don't really query downstream for "baseline" either, we just check constraint_set0_flag and decide to set profile to "constrained-baseline". i.e. we could do the same for "extended" -> "main".

(In reply to comment #9)
> (In reply to comment #8)
> > Does anyone have objection to the proposal in comment 7? Or some other ways to
> > do this? If not I will try to implement it..
> 
> I don't think you'd need double queries. e.g. for "constrained-baseline", we
> don't really query downstream for "baseline" either, we just check
> constraint_set0_flag and decide to set profile to "constrained-baseline". i.e.
> we could do the same for "extended" -> "main".

For constrained-baseline it is the must condition(profile idc= 66 *and* constraint_set1_flag= 1) since there is no specific profile_idc for it.

Profile_idc=88 means extended profile itself. But if there is constraint_set1_flag=1, then stream only needs tools belongs to main_profile for decoding. right? Theoretically the stream is extend profile itself. 
Similarly for extended profile , we need to check constraint_set0_flag first, if that is true then we can set baseline profile instead of constrained-baseline.

I am afraid that there might be many combinations possible like this...

BTW single query is enough..The returned query should contains all downstream supported profiles. But we need a much more elaborated condition checking in h264parser to handle constrained_set_flags.

Created attachment 291485 [details] [review]
h264parse: expose compatible profiles to downstream

Review of attachment 291485 [details] [review]:

I think we should return a list and intersect the profiles, because we could also have "main" profile -> "high" profile et al. even though I can hardly see "high" profile only HW decoders :)

Review of attachment 291485 [details] [review]:

::: gst/videoparsers/gsth264parse.c
@@ +1212,3 @@
+      else if (sps->constraint_set0_flag && !sps->constraint_set1_flag)
+        compat_profile = "baseline";
+      break;

I believe the following to be simpler:

if (csf0) /* A.2.1 */
  compat_profile = csf1 ? "constrained-baseline" : "baseline";
else if (csf1) /* A.2.2 */
  compat_profile = "main";

(In reply to comment #13)
> Review of attachment 291485 [details] [review]:
> 
> I think we should return a list and intersect the profiles, because we could
> also have "main" profile -> "high" profile et al. even though I can hardly see
> "high" profile only HW decoders :)

We must set a single profile from parser! so whatever profile list returning by the downstream elements are comparing against compatible profiles inside parser.

(In reply to comment #14)
> Review of attachment 291485 [details] [review]:
> 
> ::: gst/videoparsers/gsth264parse.c
> @@ +1212,3 @@
> +      else if (sps->constraint_set0_flag && !sps->constraint_set1_flag)
> +        compat_profile = "baseline";
> +      break;
> 
> I believe the following to be simpler:
> 
> if (csf0) /* A.2.1 */
>   compat_profile = csf1 ? "constrained-baseline" : "baseline";
> else if (csf1) /* A.2.2 */
>   compat_profile = "main";

make sense! will change that.
BTW, i haven't added the S3D profile as a compatible profile of MVC since the upstream parser is still missing the mvc support. Once this patch get accepted, i will add the Stereo3D profile thing in vaapiparse_h264 ;)

Created attachment 291569 [details] [review]
h264parse: expose compatible profiles to downstream

(In reply to comment #13)
> Review of attachment 291485 [details] [review]:
> 
> I think we should return a list and intersect the profiles, because we could
> also have "main" profile -> "high" profile et al. even though I can hardly see
> "high" profile only HW decoders :)

Hm, then we should add all unnecessary cases like "extended only but no constrained-base-line etc" too. IMHO it is better to limit with useful stuffs only.

Does anyone in upstream have objection to commit this?

Review of attachment 291569 [details] [review]:

::: gst/videoparsers/gsth264parse.c
@@ +1275,3 @@
+      gst_value_init_and_copy (&profiles, value);
+
+    compat_profile = get_compatible_profile (&profiles, sps);

Are there not multiple compatible profiles?
E.g. a constrained-baseline stream is also a valid baseline, main, high*, etc stream?

You could just add them all, intersect with what downstream can do, then fixate the caps.

(In reply to comment #20)
> Review of attachment 291569 [details] [review]:
> 
> ::: gst/videoparsers/gsth264parse.c
> @@ +1275,3 @@
> +      gst_value_init_and_copy (&profiles, value);
> +
> +    compat_profile = get_compatible_profile (&profiles, sps);
> 
> Are there not multiple compatible profiles?
> E.g. a constrained-baseline stream is also a valid baseline, main, high*, etc
> stream?
> 

yes :) okay, I will provide a much more relaxed version of the patch..

> You could just add them all, intersect with what downstream can do, then fixate
> the caps.
Okay, that is an easy way instead of comparing each GVAlue.. thanks !

Created attachment 291871 [details] [review]
h264parse: expose compatible profiles to downstream

Review of attachment 291871 [details] [review]:

::: gst/videoparsers/gsth264parse.c
@@ +1274,3 @@
+
+  if (compat_caps_str)
+    caps = gst_caps_from_string (compat_caps_str);

Instead of string magic I would just set a static const NULL-terminated array of string profiles in each of the cases... and then here create empty "video/x-h264" caps that are then filled by using the gst_value_array*() API.

Let's not do more string programming than necessary :)

@@ +1286,3 @@
+  GstCaps *filter_caps, *peer_caps, *pref_caps, *compat_caps;
+
+  filter_caps = gst_caps_from_string ("video/x-h264");

gst_caps_new_empty_simple()

@@ +1293,3 @@
+    GstStructure *structure;
+
+    pref_caps = gst_caps_intersect (peer_caps, filter_caps);

Maybe check for empty caps here already? Note that you could also just put the compat profiles into the filter_caps directly

Also the peer_caps must already be intersected with filter_caps here as you used the same filter to get the peer caps.

@@ +1300,3 @@
+
+      res_caps = gst_caps_intersect (pref_caps, compat_caps);
+      if (res_caps) {

res_caps can be empty here, but not NULL

@@ +1308,3 @@
+        if (profile_str) {
+          gst_caps_set_simple (caps, "profile", G_TYPE_STRING, profile_str,
+              NULL);

Why not just caps = res_caps here?

(In reply to comment #23)
> Review of attachment 291871 [details] [review]:
> 
> ::: gst/videoparsers/gsth264parse.c
> @@ +1274,3 @@
> +
> +  if (compat_caps_str)
> +    caps = gst_caps_from_string (compat_caps_str);
> 
> Instead of string magic I would just set a static const NULL-terminated array
> of string profiles in each of the cases... and then here create empty
> "video/x-h264" caps that are then filled by using the gst_value_array*() API.
> 
> Let's not do more string programming than necessary :)

k,,

> 
> @@ +1286,3 @@
> +  GstCaps *filter_caps, *peer_caps, *pref_caps, *compat_caps;
> +
> +  filter_caps = gst_caps_from_string ("video/x-h264");
> 
> gst_caps_new_empty_simple()

 
> @@ +1293,3 @@
> +    GstStructure *structure;
> +
> +    pref_caps = gst_caps_intersect (peer_caps, filter_caps);
> 
> Maybe check for empty caps here already? Note that you could also just put the
> compat profiles into the filter_caps directly
> 
> Also the peer_caps must already be intersected with filter_caps here as you
> used the same filter to get the peer caps.

peer_caps will be the subset of filter_caps..
eg: vaapidecode returns mulitple codes and profiles in a single caps, and we only need the h264 details here. so doing an intersect to get the preferred caps which will extract the caps having name video/x-h264.

> 
> @@ +1300,3 @@
> +
> +      res_caps = gst_caps_intersect (pref_caps, compat_caps);
> +      if (res_caps) {
> 
> res_caps can be empty here, but not NULL
> 
> @@ +1308,3 @@
> +        if (profile_str) {
> +          gst_caps_set_simple (caps, "profile", G_TYPE_STRING, profile_str,
> +              NULL);
> 
> Why not just caps = res_caps here?

no, we can't !
the caps might have other properties set by the update_src_caps(). we should change profile field here..

Created attachment 291970 [details] [review]
h264parse: expose compatible profiles to downstream

Review of attachment 291970 [details] [review]:

Looks good but (apart from typo in commit message: realx -> relax)

::: gst/videoparsers/gsth264parse.c
@@ +1234,3 @@
+        profiles = profile_array;
+      } else {
+        static const gchar *profile_array[] = { "extended", NULL };

Isn't unconstrained baseline compatible with main, high, and basically everything other than constrained-baseline?

@@ +1248,3 @@
+      if (sps->constraint_set1_flag) {
+        static const gchar *profile_array[] =
+            { "main", "high-10", "high-4:2:2", "high-4:4:4", NULL };

Constrained high is compatible with main?

@@ +1259,3 @@
+      if (sps->constraint_set1_flag) {
+        static const gchar *profile_array[] =
+            { "main", "high", "high-4:2:2", "high-4:4:4", NULL };

And constrained High10 with main and non-10 high?

@@ +1278,3 @@
+      if (sps->constraint_set1_flag) {
+        static const gchar *profile_array[] =
+            { "main", "high", "high-10", "high-4:4:4", NULL };

And same here

@@ +1295,3 @@
+        static const gchar *profile_array[] =
+            { "main", "high", "high-10", "high-4:2:2", NULL };
+        profiles = profile_array;

And here

(In reply to comment #26)
> Review of attachment 291970 [details] [review]:
> 
> Looks good but (apart from typo in commit message: realx -> relax)
> 
> ::: gst/videoparsers/gsth264parse.c
> @@ +1234,3 @@
> +        profiles = profile_array;
> +      } else {
> +        static const gchar *profile_array[] = { "extended", NULL };
> 
> Isn't unconstrained baseline compatible with main, high, and basically
> everything other than constrained-baseline?

NO,,constrained_baseline < baseline < extended_profile and
constrained_baseline < main < high
but main & high are not always compatible with baseline and extended :)

> @@ +1248,3 @@
> +      if (sps->constraint_set1_flag) {
> +        static const gchar *profile_array[] =
> +            { "main", "high-10", "high-4:2:2", "high-4:4:4", NULL };
> 
> Constrained high is compatible with main?

The csf1 is checked for the compatibility with main profile :)

We are not handling constrained-high and progressive-high separately...
even in gst-plugins-base/gst-libs/gst/pbutils/codec-utils.c,,thats the reason i omitted those..but yes, can add those later on...may be first a patch to codec-utils..

> 
> @@ +1259,3 @@
> +      if (sps->constraint_set1_flag) {
> +        static const gchar *profile_array[] =
> +            { "main", "high", "high-4:2:2", "high-4:4:4", NULL };
> 
> And constrained High10 with main and non-10 high?

There is no constrained-high10 profile !!
The constraint_set1_flag has been checked for main profile..which means csf1 == true, it is main profile compatible, which means compatible with high, high-4:2:2 etc....

> @@ +1278,3 @@
> +      if (sps->constraint_set1_flag) {
> +        static const gchar *profile_array[] =
> +            { "main", "high", "high-10", "high-4:4:4", NULL };
> 
> And same here
>
same as above !
> @@ +1295,3 @@
> +        static const gchar *profile_array[] =
> +            { "main", "high", "high-10", "high-4:2:2", NULL };
> +        profiles = profile_array;
> 
> And here

same as above !

Created attachment 292167 [details] [review]
h264parse: expose compatible profiles to downstream

Fixed typo in commit message of previous patch...

Thanks for explaining, let's get this in then. I assume the same would be needed for h265parse too in one way or another.

commit 3910cbe7ce1a67ed5506f87e4c5a005c168d0e6c
Author: Sreerenj Balachandran <sreerenj.balachandran@intel.com>
Date:   Tue Dec 2 10:10:39 2014 +0200

    h264parse: expose compatible profiles to downstream
    
    Some video bitstreams report a too restrictive set of profiles. If a video
    decoder was to strictly follow the indicated profile, it wouldn't support that
    stream, whereas it could in theory and in practice. So we should relax the
    profile restriction for allowing the decoder to get connected with parser.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=739992

Hey, this broke the integration tests, you can reproduce with:

gst-validate-launcher --sync -t \ validate.file.transcode.to_mp3_and_h264_in_mp4.GH1_00094_1920x1280_MTS

Result: Failed (Application returned 18 (issues: [getcaps function isn't proxying downstream fields correctly]))

The patch is reverted now (please mention that in the related bugs in the future, Mathieu :) ). Not sure how this patch can cause fields to be not proxied, maybe this is a bug in the test?

The only thing I can imagine here is that our getcaps function will add more profiles than what downstream requests, because more profiles are compatible with the downstream one. Mathieu, can you get the caps in question (getcaps return value and downstream caps)?

(In reply to comment #31)
> The patch is reverted now (please mention that in the related bugs in the
> future, Mathieu :) ). Not sure how this patch can cause fields to be not
> proxied, maybe this is a bug in the test?
> 
> The only thing I can imagine here is that our getcaps function will add more
> profiles than what downstream requests, because more profiles are compatible
> with the downstream one. Mathieu, can you get the caps in question (getcaps
> return value and downstream caps)?

Erg I didn't revert this patch on purpose, I only wanted to push my mirror patch and the patch was reverted on my branch.

Thibault printed the details yesterday, it was :

<thiblahute> 0:00:01.038289026 11638 0x7faebc015680 ERROR               validate gst-validate-reporter.c:207:gst_validate_report_valist: <avdec_h264-0:sink> 1921 (critical) : caps: getcaps function isn't proxying downstream fields correctly : Peer pad structure 'video/x-raw, format=(string)I420, width=(int)1920, height=(int)1080, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)mixed, chroma-site=(strin
<thiblahute> g)mpeg2, colorimetry=(string)bt709, framerate=(fraction)30000/1001;' has no similar version on pad's caps 'video/x-h264, alignment=(string)au, stream-format=(string){ avc, byte-stream }, parsed=(boolean)true'

I still didn't understand why this patch get reverted !!

The same kind of error can be reproducible for audio pipeline too, for eg:
2203 (critical) : caps: getcaps function isn't proxying downstream fields correctly : Peer pad structure 'audio/x-raw, rate=(int){ 8000, 11025, 12000, 16000, 22050, 24000, 32000, 44100, 48000 }, channels=(int)1, format=(string)S16LE, layout=(string)interleaved;' has no similar version on pad's caps 'audio/x-ac3, rate=(int)48000, channels=(int)2, framed=(boolean)true, alignment=(string)frame'

!!!!

Please see comment 30 about why it is reverted. It broke a test that worked fine before :) That's a regression.

If the same bug is also in ac3parse, please create another bug report about that with a simple testcase

(In reply to comment #33)
> I still didn't understand why this patch get reverted !!

Yes, what Sebastian just said :) Also, the reverting was pushed by mistake, but it kind of makes sense to me anyway, the validate integration test suite should come to be considered as important as the "in-module" tests, and commits should be refused / reverted when they make this test suite regress, at least when they trigger new critical errors such as here. This hasn't yet been discussed and "formally" agreed upon with everyone upstream, but it is clearly a goal we should seek.

That being said, I haven't investigated the issue more than that and it could prove to be a false positive, but I count on you to tell us if it is :)

(In reply to comment #36)
> (In reply to comment #33)
> > I still didn't understand why this patch get reverted !!
> 
> Yes, what Sebastian just said :) Also, the reverting was pushed by mistake, but
> it kind of makes sense to me anyway, the validate integration test suite should
> come to be considered as important as the "in-module" tests, and commits should
> be refused / reverted when they make this test suite regress, at least when
> they trigger new critical errors such as here. This hasn't yet been discussed
> and "formally" agreed upon with everyone upstream, but it is clearly a goal we
> should seek.
> 
> That being said, I haven't investigated the issue more than that and it could
> prove to be a false positive, but I count on you to tell us if it is :)

BTB, is it still reproducible ? :) Surprisingly I am getting only issue with audio pipeline (validate is failing only for ac3).

I didn't change anything in get_caps implementation with this patch and I can't reproduce this issue with the command mentioned in comment_30 too.  Is it possible for someone to have a try with this patch once more?

(In reply to comment #38)
> I didn't change anything in get_caps implementation with this patch and I can't
> reproduce this issue with the command mentioned in comment #30 too.  Is it
> possible for someone to have a try with this patch once more?

Perhaps I'm missing something, but I have ran this command twice:

$ gst-validate-transcoding-1.0  -o "video/quicktime,variant=iso;:video/x-h264:audio/mpeg,mpegversion=1,layer=3" file:///home/vjaquez/gst-validate/gst-integration-testsuites/medias/defaults/mpegts/GH1_00094_1920x1280.MTS file:///home/vjaquez/gst-validate/rendered/validate/file/to_mp3_and_h264_in_mp4/GH1_00094_1920x1280_MTS --gst-debug=*:3

One with sreerenj's patch and another with its reversion. In both cases I got the same error:

==== Got criticals, Return value set to 18 ====
     Critical error Peer pad structure 'audio/x-raw, rate=(int){ 8000, 11025, 12000, 16000, 22050, 24000, 32000, 44100, 48000 }, channels=(int)1, format=(string)S16LE, layout=(string)interleaved;' has no similar version on pad's caps 'audio/x-ac3, rate=(int)48000, channels=(int)2, framed=(boolean)true, alignment=(string)frame'


:00:00.146352550  9763 0x7f2de40368f0 ERROR               validate gst-validate-reporter.c:207:gst_validate_report_valist: <avdec_ac3-0:sink> 1802 (critical) : caps: getcaps function isn't proxying downstream fields correctly : Peer pad structure 'audio/x-raw, rate=(int){ 8000, 11025, 12000, 16000, 22050, 24000, 32000, 44100, 48000 }, channels=(int)1, format=(string)S16LE, layout=(string)interleaved;' has no similar v
ersion on pad's caps 'audio/x-ac3, rate=(int)48000, channels=(int)2, framed=(boolean)true, alignment=(string)frame'

IMO this regression is not related with the h264parse (yet), but  with ac3parse / avdec_ac3.

Someone please re-revert the patch..(comment 37, 38 & 39).!

The regression was related to h264 but that commit was reverted, if you re enable it you will see that issue:

https://ci.gstreamer.net/job/GStreamer-master-validate/121/testReport/junit/validate.file.transcode/to_mp3_and_h264_in_mp4/GH1_00094_1920x1280_MTS/

There is another commit that made ac3 regresse to, we should figure out which one.

The bug might still be in GstValidate, we just really need to investigate it.

(In reply to comment #41)
> The regression was related to h264 but that commit was reverted, if you re
> enable it you will see that issue:

Are you talking about the same patch attached to this bug report(which got committed and reverted, has nothing to do with get_caps, can't reproduce the mentioned regression with or with out this patch for both me & Victor too)? 

> https://ci.gstreamer.net/job/GStreamer-master-validate/121/testReport/junit/validate.file.transcode/to_mp3_and_h264_in_mp4/GH1_00094_1920x1280_MTS/
> 
> There is another commit that made ac3 regresse to, we should figure out which
> one.
> 
> The bug might still be in GstValidate, we just really need to investigate it.

As this patch is blocking two other bugs and, as Sreerenj and myself can see, this patch is not responsible of that regression, we should re-reverted it.

I'll dig on the ac3 regression today.

Reapplied the patch, after further investigation the issue was indeed a false positive in -validate that is being fixed right now. No need to dig into the ac3 regression Victor, hope you didn't waste time on doing that :)

commit 542c77ab3831388f162a78878ba8902cd5c4d9f9
Author: Sreerenj Balachandran <sreerenj.balachandran@intel.com>
Date:   Tue Dec 2 10:10:39 2014 +0200

    h264parse: expose compatible profiles to downstream
    
    Some video bitstreams report a too restrictive set of profiles. If a video
    decoder was to strictly follow the indicated profile, it wouldn't support that
    stream, whereas it could in theory and in practice. So we should relax the
    profile restriction for allowing the decoder to get connected with parser.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=739992

Cool! Thanks Mathieu!