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 732265 - vaapidecode: h264: add support for decoding MVC base views only
vaapidecode: h264: add support for decoding MVC base views only
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer-vaapi
git master
Other Linux
: Normal enhancement
: 1.13.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
P1
Depends on: 732267 783588
Blocks: 758397
 
 
Reported: 2014-06-26 04:08 UTC by Gwenole Beauchesne
Modified: 2017-08-03 21:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Sample implementation (4.17 KB, patch)
2017-06-09 17:21 UTC, Orestis Floros
needs-work Details | Review
Current work with property and caps negotiation (8.59 KB, patch)
2017-06-19 23:48 UTC, Orestis Floros
none Details | Review
vaapidecode_props: h264: add base only property (2.59 KB, patch)
2017-07-25 20:15 UTC, Orestis Floros
none Details | Review
libs: decoder: h264: add setter for base only mode (1.95 KB, patch)
2017-07-25 20:15 UTC, Orestis Floros
committed Details | Review
vaapidecode: set h264 base only to decoder (905 bytes, patch)
2017-07-25 20:15 UTC, Orestis Floros
committed Details | Review
vaapidecode_props: h264: set base-only in decoder (1018 bytes, patch)
2017-07-25 20:16 UTC, Orestis Floros
committed Details | Review
vaapidecode_props: h264: add base-only getter (1.64 KB, patch)
2017-07-25 20:16 UTC, Orestis Floros
none Details | Review
vaapidecode: force add h264 MVC profiles in caps (2.62 KB, patch)
2017-07-25 20:16 UTC, Orestis Floros
none Details | Review
libs: decoder: h264: decode MVC base view only (1.17 KB, patch)
2017-07-25 20:16 UTC, Orestis Floros
committed Details | Review
vaapidecode_props: h264: add base only property (2.52 KB, patch)
2017-08-03 10:30 UTC, Orestis Floros
committed Details | Review
vaapidecode: force add h264 MVC profiles in caps (2.56 KB, patch)
2017-08-03 10:45 UTC, Orestis Floros
committed Details | Review
vaapidecode: fix gst_caps_new_simple call (882 bytes, patch)
2017-08-03 20:21 UTC, Orestis Floros
committed Details | Review

Description Gwenole Beauchesne 2014-06-26 04:08:32 UTC
A standard AVC conformant decoder should be able to decode the base view of MVC encoded streams. The purpose of this bug is twofold: (i) make sure vaapidecode can positively react to upstream caps from h264parse (a selection of { high-profile, <mvc profiles> } and (ii) probably add a "base-only" property to enable this feature from vaapidecode without explicit caps negotation. In case (ii), vaapidecode will perform the negotiation itself.
Comment 1 Orestis Floros 2017-06-09 17:21:46 UTC
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.
Comment 2 sreerenj 2017-06-19 21:36:00 UTC
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?
Comment 3 Orestis Floros 2017-06-19 23:48:38 UTC
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.
Comment 4 Orestis Floros 2017-06-19 23:50:26 UTC
(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.
Comment 5 Orestis Floros 2017-06-20 19:21:46 UTC
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
Comment 6 Orestis Floros 2017-07-25 20:15:26 UTC
Created attachment 356376 [details] [review]
vaapidecode_props: h264: add base only property
Comment 7 Orestis Floros 2017-07-25 20:15:39 UTC
Created attachment 356377 [details] [review]
libs: decoder: h264: add setter for base only mode
Comment 8 Orestis Floros 2017-07-25 20:15:53 UTC
Created attachment 356378 [details] [review]
vaapidecode: set h264 base only to decoder
Comment 9 Orestis Floros 2017-07-25 20:16:02 UTC
Created attachment 356379 [details] [review]
vaapidecode_props: h264: set base-only in decoder
Comment 10 Orestis Floros 2017-07-25 20:16:16 UTC
Created attachment 356380 [details] [review]
vaapidecode_props: h264: add base-only getter

Generic getter using a GObject.
Comment 11 Orestis Floros 2017-07-25 20:16:24 UTC
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.
Comment 12 Orestis Floros 2017-07-25 20:16:32 UTC
Created attachment 356382 [details] [review]
libs: decoder: h264: decode MVC base view only
Comment 13 Orestis Floros 2017-07-25 20:20:18 UTC
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.
Comment 14 Víctor Manuel Jáquez Leal 2017-07-26 14:58:11 UTC
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"
Comment 15 Víctor Manuel Jáquez Leal 2017-07-26 14:59:56 UTC
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
Comment 16 Víctor Manuel Jáquez Leal 2017-07-26 15:00:44 UTC
Review of attachment 356378 [details] [review]:

lgtm
Comment 17 Víctor Manuel Jáquez Leal 2017-07-26 15:01:40 UTC
Review of attachment 356379 [details] [review]:

lgtm
Comment 18 Víctor Manuel Jáquez Leal 2017-07-26 15:07:52 UTC
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)
}
Comment 19 Víctor Manuel Jáquez Leal 2017-07-26 15:13:20 UTC
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.
Comment 20 Víctor Manuel Jáquez Leal 2017-07-26 15:13:58 UTC
Review of attachment 356382 [details] [review]:

lgtm
Comment 21 Víctor Manuel Jáquez Leal 2017-07-26 15:16:02 UTC
(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 :)
Comment 22 Orestis Floros 2017-07-26 21:22:48 UTC
(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.
Comment 23 Orestis Floros 2017-07-26 23:58:58 UTC
(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.
Comment 24 Víctor Manuel Jáquez Leal 2017-07-27 08:50:23 UTC
(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.
Comment 25 Orestis Floros 2017-07-27 09:22:52 UTC
(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?
Comment 26 Orestis Floros 2017-08-03 10:30:10 UTC
Created attachment 356837 [details] [review]
vaapidecode_props: h264: add base only property
Comment 27 Orestis Floros 2017-08-03 10:45:22 UTC
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.
Comment 28 Víctor Manuel Jáquez Leal 2017-08-03 14:28:33 UTC
Review of attachment 356837 [details] [review]:

lgtm
Comment 29 Víctor Manuel Jáquez Leal 2017-08-03 14:29:32 UTC
Review of attachment 356839 [details] [review]:

lgtm
Comment 30 Víctor Manuel Jáquez Leal 2017-08-03 15:15:49 UTC
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.
Comment 31 Víctor Manuel Jáquez Leal 2017-08-03 15:18:03 UTC
Congratulation Orestis.

Now let's finish with bug 732266.
Comment 32 sreerenj 2017-08-03 18:26:48 UTC
Great!, Thanks for implementing this feature Orestis.
Comment 33 Orestis Floros 2017-08-03 20:21:39 UTC
Created attachment 356890 [details] [review]
vaapidecode: fix gst_caps_new_simple call
Comment 34 Víctor Manuel Jáquez Leal 2017-08-03 21:32:56 UTC
Attachment 356890 [details] pushed as 3bb96ef - vaapidecode: fix gst_caps_new_simple call
Comment 35 Víctor Manuel Jáquez Leal 2017-08-03 21:33:53 UTC
Thanks. I ought spot that issue.
Comment 36 Tim-Philipp Müller 2017-08-03 21:55:38 UTC
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 :)