Created attachment 353477 [details] [review]
Sample implementation

This patch depends on the infrastructure provided in attachment 353187 [details] [review]. It uses a "base_only" field in struct GstVaapiDecoderH264Private.

A final patch can be submitted after bug 783588 is resolved.

This task is assigned to Orestis and he is doing it as a part of Google Summer of Code 2017.

Let's discuss about the possible cases.
 

  a: vaapih264dec only advertise the Annex-A profiles if there is no MVC decode support in hardware , and h264parse should drop all NON-ANNEX-A NAL units if pipeline is ".. ! h264parse ! vaapih264dec !..."
The negotiated profile will be Annex-A profile in this case.

  b: The pipeline ".. ! demuxer ! vaapihh264dec !.." will fail if there is no hw capability to decode MVC, Because the decoder advertise the support for Annex-A but demuxer report MVC profile.Which means there is no one to drop the NON-Annex-A NALs.
There is no negotiated profle in this case, because negotiation failed.

  c: The pipeline ".. ! demuxer ! vaapihh264dec !.." will work even if there is no hw capability to decode MVC If and Only if the user explicitly enable base-only=TRUE.
The negotiated profile will be Annex-H profile in this case, because we forcefully enable the dropping. If we try to negotiate Annex-A profile, there is no guarantee about the capability of upstream element to drop NON-ANNEX-A NALs.

There are different combinations possible like the above. Any better suggestions?

Created attachment 354073 [details] [review]
Current work with property and caps negotiation

This is my current work until now.
It still depends on attachment 353187 [details] [review] since bug 783588 is still unresolved.

The patch should handle cases (b.) and (c.) from comment 2. Another patch will be submitted for (a.) in bug 732267.
With the current h264parse element a pipeline like this:
... h264parse ! vaapih264dec base-only=TRUE ! vaapisink
will make vaapih264 negotiate Annex-H profiles and drop the NON-ANNEX-A NALs itself.
With base-only=FALSE negotiation fails.

An open issue is whether should we initialize base_only to TRUE if we negotiate Annex-A profiles regardless of the property's value.

(In reply to sreerenj from comment #2)
>   b: The pipeline ".. ! demuxer ! vaapihh264dec !.." will fail if there is
> no hw capability to decode MVC, Because the decoder advertise the support
> for Annex-A but demuxer report MVC profile.Which means there is no one to
> drop the NON-Annex-A NALs.
> There is no negotiated profle in this case, because negotiation failed.
>
>   c: The pipeline ".. ! demuxer ! vaapihh264dec !.." will work even if there
> is no hw capability to decode MVC If and Only if the user explicitly enable
> base-only=TRUE.
> The negotiated profile will be Annex-H profile in this case, because we
> forcefully enable the dropping. If we try to negotiate Annex-A profile,
> there is no guarantee about the capability of upstream element to drop
> NON-ANNEX-A NALs.
>
> There are different combinations possible like the above. Any better
> suggestions?

An other option (don't know if it is doable) is to dynamically switch between (a.) and (c.). If the upstream element can't negotiate Annex-A profiles, negotiate Annex-H profile and drop the NON-ANNEX-A NALs. If the upstream element fails to drop the NON-ANNEX-A NALs we can always drop them from vaapih264dec.

Review of attachment 354073 [details] [review]:

::: gst-libs/gst/vaapi/gstvaapidecoder_h264.c
@@ +4668,3 @@
+      case GST_H264_NAL_SUBSET_SPS:
+      case GST_H264_NAL_SLICE_EXT:
+        flags |= GST_VAAPI_DECODER_UNIT_FLAG_SKIP;

This actually causes multiple (harmless) errors with a stream-format=byte-stream and alignment=nal pipeline. The sps != NULL assertion in decode_picture() fails.

Removing this part avoids that while the result doesn't change. Don't know if I should correct that or not set the GST_VAAPI_DECODER_UNIT_FLAG_SKIP flag at all.

Tested with pipeline:
filesrc location=a.ts ! tsdemux ! h264parse ! vaapih264dec base-only=TRUE ! fakesink

Created attachment 356376 [details] [review]
vaapidecode_props: h264: add base only property

Created attachment 356377 [details] [review]
libs: decoder: h264: add setter for base only mode

Created attachment 356378 [details] [review]
vaapidecode: set h264 base only to decoder

Created attachment 356379 [details] [review]
vaapidecode_props: h264: set base-only in decoder

Created attachment 356380 [details] [review]
vaapidecode_props: h264: add base-only getter

Generic getter using a GObject.

Created attachment 356381 [details] [review]
vaapidecode: force add h264 MVC profiles in caps

When vaapih264dec's base-only profile is set to TRUE, fake MVC profile
support in caps.

Created attachment 356382 [details] [review]
libs: decoder: h264: decode MVC base view only

I tried to follow submit patches in a way similar to bug 783588. I can also squash them if this is too much.

Changes here are not enough to work for SVC streams. Related changes will go to bug 732266.

Review of attachment 356376 [details] [review]:

::: gst/vaapi/gstvaapidecode_props.c
@@ +99,3 @@
+  g_object_class_install_property (klass, GST_VAAPI_DECODER_H264_PROP_BASE_ONLY,
+      g_param_spec_boolean ("base-only", "Decode base view only",
+          "Only decode the base view of the stream: Any NAL units that do not pertain to the Annex.A set will be dropped",

I would maker shorter this description.

Perhaps just to: "Drop any NAL unit not defined in Annex.A"

Review of attachment 356377 [details] [review]:

::: gst-libs/gst/vaapi/gstvaapidecoder_h264.c
@@ +4805,3 @@
+ *
+ * if @base_only is %TRUE only the base view of MVC encoded streams
+ * is decoded.

is this property going to be only for MVC streams? not for SVC ones?

AFAIU this property will be drop both NAL

Review of attachment 356378 [details] [review]:

lgtm

Review of attachment 356379 [details] [review]:

lgtm

Review of attachment 356380 [details] [review]:

::: gst/vaapi/gstvaapidecode_props.c
@@ +115,3 @@
+
+gboolean
+gst_vaapi_decode_get_base_only (GObject * object)

from my point of view this function makes little sense. You can add in gstvaapidecode.c something like this:

if (g_object_class_find_property(G_OBJECT_GET_CLASS(decode),"base-only")) {
  g_object_get (decode, "base-only", &base_only, NULL)
}

Review of attachment 356381 [details] [review]:

::: gst/vaapi/gstvaapidecode.c
@@ +1169,3 @@
+  caps_new = gst_caps_from_string ("video/x-h264");
+  structure = gst_caps_get_structure (caps_new, 0);
+  gst_structure_set (structure, "profile", G_TYPE_STRING, profile_name, NULL);

Use this 

caps_new = gst_caps_new_simple ("video/x-h264", "profile", profile_name, NULL);

@@ +1222,3 @@
+  }
+
+  if (base_only && !have_mvc && have_high) {

If va backend doesn't profile mvc profiles but has high profile, I would set base_only as TRUE mandatory.

Review of attachment 356382 [details] [review]:

lgtm

(In reply to Orestis Floros from comment #13)
> I tried to follow submit patches in a way similar to bug 783588. I can also
> squash them if this is too much.

Squash them, please :)

(In reply to Víctor Manuel Jáquez Leal from comment #21)
> (In reply to Orestis Floros from comment #13)
> > I tried to follow submit patches in a way similar to bug 783588. I can also
> > squash them if this is too much.
>
> Squash them, please :)

Ok, I'll leave them like this until everything is good and then squash them.

(In reply to Víctor Manuel Jáquez Leal from comment #14)

> Review of attachment 356376 [details] [review] [review]:
>
> ::: gst/vaapi/gstvaapidecode_props.c
> @@ +99,3 @@
> +  g_object_class_install_property (klass,
> GST_VAAPI_DECODER_H264_PROP_BASE_ONLY,
> +      g_param_spec_boolean ("base-only", "Decode base view only",
> +          "Only decode the base view of the stream: Any NAL units that do
> not pertain to the Annex.A set will be dropped",
>
> I would maker shorter this description.
>
> Perhaps just to: "Drop any NAL unit not defined in Annex.A"

Ok.

(In reply to Víctor Manuel Jáquez Leal from comment #15)
> Review of attachment 356377 [details] [review] [review]:
>
> ::: gst-libs/gst/vaapi/gstvaapidecoder_h264.c
> @@ +4805,3 @@
> + *
> + * if @base_only is %TRUE only the base view of MVC encoded streams
> + * is decoded.
>
> is this property going to be only for MVC streams? not for SVC ones?
>
> AFAIU this property will be drop both NAL

I thought that I should not mention SVC until I am ready to submit patches in bug 732266. Right now it doesn't affect SVC streams.

(In reply to Víctor Manuel Jáquez Leal from comment #18)
> Review of attachment 356380 [details] [review] [review]:
>
> ::: gst/vaapi/gstvaapidecode_props.c
> @@ +115,3 @@
> +
> +gboolean
> +gst_vaapi_decode_get_base_only (GObject * object)
>
> from my point of view this function makes little sense. You can add in
> gstvaapidecode.c something like this:
>
> if (g_object_class_find_property(G_OBJECT_GET_CLASS(decode),"base-only")) {
>   g_object_get (decode, "base-only", &base_only, NULL)
> }

Ok. But:

(In reply to Víctor Manuel Jáquez Leal from comment #19)
> Review of attachment 356381 [details] [review] [review]:
>
> ::: gst/vaapi/gstvaapidecode.c
> @@ +1169,3 @@
> +  caps_new = gst_caps_from_string ("video/x-h264");
> +  structure = gst_caps_get_structure (caps_new, 0);
> +  gst_structure_set (structure, "profile", G_TYPE_STRING, profile_name,
> NULL);
>
> Use this
>
> caps_new = gst_caps_new_simple ("video/x-h264", "profile", profile_name,
> NULL);
>
> @@ +1222,3 @@
> +  }
> +
> +  if (base_only && !have_mvc && have_high) {
>
> If va backend doesn't profile mvc profiles but has high profile, I would set
> base_only as TRUE mandatory.

Ok. This means that gstvaapidecode doesn't need to get the value of base_only since it won't use it anywhere. It need to set base_only accordingly though.

(In reply to Orestis Floros from comment #22)
> (In reply to Víctor Manuel Jáquez Leal from comment #19)
> > Review of attachment 356381 [details] [review] [review] [review]:
> > @@ +1222,3 @@
> > +  }
> > +
> > +  if (base_only && !have_mvc && have_high) {
> >
> > If va backend doesn't profile mvc profiles but has high profile, I would set
> > base_only as TRUE mandatory.
> 
> Ok. This means that gstvaapidecode doesn't need to get the value of
> base_only since it won't use it anywhere. It need to set base_only
> accordingly though.

It would also mean that h264parse won't be able to use the changes in bug 732267 because vaapih264dec would fake MVC/SVC support.

(In reply to Orestis Floros from comment #22)
> > @@ +1222,3 @@
> > +  }
> > +
> > +  if (base_only && !have_mvc && have_high) {
> >
> > If va backend doesn't profile mvc profiles but has high profile, I would set
> > base_only as TRUE mandatory.
> 
> Ok. This means that gstvaapidecode doesn't need to get the value of
> base_only since it won't use it anywhere. It need to set base_only
> accordingly though.

I would use base_only property if the use doesn't want MVC/SVC support on purpose.

But it will be definitely useful for user that their hardware doesn't have support and still they will be able to playback the media.

(In reply to Orestis Floros from comment #23)
> 
> It would also mean that h264parse won't be able to use the changes in bug
> 732267 because vaapih264dec would fake MVC/SVC support.

h264parse change is useful for avdec_h264 users or any other decoder that might not handle MVC/SVC streams.

(In reply to Víctor Manuel Jáquez Leal from comment #24)
> (In reply to Orestis Floros from comment #22)
> > > @@ +1222,3 @@
> > > +  }
> > > +
> > > +  if (base_only && !have_mvc && have_high) {
> > >
> > > If va backend doesn't profile mvc profiles but has high profile, I would set
> > > base_only as TRUE mandatory.
> > 
> > Ok. This means that gstvaapidecode doesn't need to get the value of
> > base_only since it won't use it anywhere. It need to set base_only
> > accordingly though.
> 
> I would use base_only property if the use doesn't want MVC/SVC support on
> purpose.

Yes. I am saying that what I am doing in attachment 356380 [details] [review] won't be needed at all since vaapidecode won't need to read the property, only vaapih264dec.

> (In reply to Orestis Floros from comment #23)
> > 
> > It would also mean that h264parse won't be able to use the changes in bug
> > 732267 because vaapih264dec would fake MVC/SVC support.
> 
> h264parse change is useful for avdec_h264 users or any other decoder that
> might not handle MVC/SVC streams.

Ok. The problem here might be that we will always lie about SVC support without the user asking for it and with no way to disable it. Could this be problematic behavior for some cases?

Created attachment 356837 [details] [review]
vaapidecode_props: h264: add base only property

Created attachment 356839 [details] [review]
vaapidecode: force add h264 MVC profiles in caps

When vaapih264dec's base-only profile is set to TRUE, fake MVC profile
support in caps.

Review of attachment 356837 [details] [review]:

lgtm

Review of attachment 356839 [details] [review]:

lgtm

Attachment 356379 [details] pushed as bd040ad - vaapidecode_props: h264: add base-only property
Attachment 356377 [details] pushed as 1dd03ac - libs: decoder: h264: add setter for base-only mode
Attachment 356379 [details] and attachment 356378 [details] [review] where squashed and pushed as 66703a7 - vaapidecode: set h264 base-only to decoder
Attachment 356382 [details] pushed as ac9ddc5 - libs: decoder: h264: decode MVC base view only
Attachment 356839 [details] pushed as d4b6459 - vaapidecode: force add h264 MVC profiles in caps

Some commit logs where updated and in commit ac9ddc5 the log message where updated too.

Congratulation Orestis.

Now let's finish with bug 732266.

Great!, Thanks for implementing this feature Orestis.

Created attachment 356890 [details] [review]
vaapidecode: fix gst_caps_new_simple call

Attachment 356890 [details] pushed as 3bb96ef - vaapidecode: fix gst_caps_new_simple call

Thanks. I ought spot that issue.

Would be great if you could run git config --global user.name "Orestis Floros" on your dev machine, so that future patches have a proper name in them :)