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 771291 - Encoder: HEVC: Add 10 bit encoding support
Encoder: HEVC: Add 10 bit encoding support
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer-vaapi
git master
Other Linux
: Normal enhancement
: 1.11.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-09-12 12:10 UTC by sreerenj
Modified: 2017-04-07 10:13 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
vaapiencode: enhance logs of negotiated caps (1.28 KB, patch)
2017-04-06 20:00 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
libs: encoder: initialize chroma_type (4.02 KB, patch)
2017-04-06 20:00 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
libs: encoder: refactor init_context_info() (2.12 KB, patch)
2017-04-06 20:00 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
libs: encoder: dummy context for get_surface_formats() (2.23 KB, patch)
2017-04-06 20:00 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
libs: encoder: pass profile to get_surface_formats() (3.29 KB, patch)
2017-04-06 20:00 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
vaapiencode: add get_profile() vmethod (5.17 KB, patch)
2017-04-06 20:00 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
libs: encode: merge all possible surface formats (3.77 KB, patch)
2017-04-06 20:00 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
libs: encoder: h265: ensures profile given format (1.31 KB, patch)
2017-04-06 20:00 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
libs: encoder: admit YUV420_10BPP as valid chroma (1.11 KB, patch)
2017-04-06 20:01 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
libs: encoder: h265: chroma and luma with format (2.18 KB, patch)
2017-04-06 20:01 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
vaapiencode: h265: add main-10 in caps template (964 bytes, patch)
2017-04-06 20:01 UTC, Víctor Manuel Jáquez Leal
committed Details | Review

Comment 1 Víctor Manuel Jáquez Leal 2017-03-22 17:49:42 UTC
Since the resolution of the bug 769266 this enhancement has become trickier.

The patches for bug 769266 assume that the encoder has context already configured (a pair profile/entrypoint) which will provide all the supported color formats to upload.

Nonetheless, in the case of HEVC, there are two VAEntrypointEncSlice for HEVC:
Main and Main10, where the difference is the accepted color space to upload. The first admits NV12, I420, YV12 and IMC3; the former one NV12 and P010.

So we have an exception to the rule: two profiles for one encoder. That means that gst_vaapi_encoder_get_surface_formats() needs to be aware of this exception. It will need to try all the available profiles.
Comment 2 Víctor Manuel Jáquez Leal 2017-04-06 20:00:10 UTC
Created attachment 349403 [details] [review]
vaapiencode: enhance logs of negotiated caps
Comment 3 Víctor Manuel Jáquez Leal 2017-04-06 20:00:15 UTC
Created attachment 349404 [details] [review]
libs: encoder: initialize chroma_type

Instead of initialize the chroma_type with a undefined value, which
will be converted to GST_VAAPI_CHROMA_TYPE_YUV420 by GstVaapiContext,
this patch queries the VA config, given the received
GstVaapiContextInfo's parameters, and gets the first response.

In order to get the GstVaapiChromaType value, also it was needed to
add a new utility function: to_GstVaapiChromaType(), which, given a
VA_RT_FORMAT_* will return the associated GstVaapiChromaType.
Comment 4 Víctor Manuel Jáquez Leal 2017-04-06 20:00:21 UTC
Created attachment 349405 [details] [review]
libs: encoder: refactor init_context_info()

In order to generate vaapi contexts iterative, the function
init_context_info() is refactored to pass, as parameters the
GstVaapiContextInfo and the GstVaapiProfile.
Comment 5 Víctor Manuel Jáquez Leal 2017-04-06 20:00:27 UTC
Created attachment 349406 [details] [review]
libs: encoder: dummy context for get_surface_formats()

Instead of creating (if it doesn't exist, yet) the encoder's context
the method gst_vaapi_encoder_get_surface_formats() now it creates
dummy contexts, unless the encoder has it already created.

The purpose of this is to avoid setting a encoder's context with a
wrong profile.
Comment 6 Víctor Manuel Jáquez Leal 2017-04-06 20:00:32 UTC
Created attachment 349407 [details] [review]
libs: encoder: pass profile to get_surface_formats()

In order to get the supported surface formats within a specific
profile this patch adds the GstVaapiProfile as property to
gst_vaapi_encoder_get_surface_formats().

Currently the extracted formats are only those related with the
default profile of the element's codec.
Comment 7 Víctor Manuel Jáquez Leal 2017-04-06 20:00:37 UTC
Created attachment 349408 [details] [review]
vaapiencode: add get_profile() vmethod

This new virtual method, get_profile(), if implemented by specific
encoders, will return the VA profile potentially determined by the
source caps.

Also it is implemented by h264 and h265 encoders, which are the main
users of this vmethod.
Comment 8 Víctor Manuel Jáquez Leal 2017-04-06 20:00:43 UTC
Created attachment 349409 [details] [review]
libs: encode: merge all possible surface formats

When the function gst_vaapi_encoder_get_surface_formats() was added
it was under the assumption that any VA profile of the specific codec
supported the same format colors. But it is not, for example the
profiles that support 10bit formats.

In other words, different VA profiles of a same codec may support
different color formats in their upload surfaces.

In order to expose all the possible color formats, if no profile is
specified via source caps, or if the encoder doesn't have yet a
context, all the possible VA profiles for the specific codec are
iterated and their color formats are merged.
Comment 9 Víctor Manuel Jáquez Leal 2017-04-06 20:00:54 UTC
Created attachment 349410 [details] [review]
libs: encoder: h265: ensures profile given format

Set the VA profile as GST_VAAPI_PROFILE_H265_MAIN10 if the
configured color format is P010_10LE.

Otherwise, keep GST_VAAPI_PROFILE_H265_MAIN
Comment 10 Víctor Manuel Jáquez Leal 2017-04-06 20:01:00 UTC
Created attachment 349411 [details] [review]
libs: encoder: admit YUV420_10BPP as valid chroma

Accepts as supported the GST_VAAPI_CHROMA_TYPE_YUV420_10BPP chroma
type.
Comment 11 Víctor Manuel Jáquez Leal 2017-04-06 20:01:06 UTC
Created attachment 349412 [details] [review]
libs: encoder: h265: chroma and luma with format

If the profile is main-10 the bit_depth_luma_minus8, in the sequence
parameter buffer, shall be the color format bit depth minus 8, 10-8
which is 2. Also for bit_depth_chroma_minus8.

This patch gets the negotiated sink caps format and queries its
luma's depth and uses that value to fill the mentioned parameters.
Comment 12 Víctor Manuel Jáquez Leal 2017-04-06 20:01:12 UTC
Created attachment 349413 [details] [review]
vaapiencode: h265: add main-10 in caps template

This patch adds h265's main-10 profile in encoder src caps template.
Comment 13 Víctor Manuel Jáquez Leal 2017-04-06 20:10:18 UTC
These are the patches required to enable main-10 in HEVC encoder.

I had to extend what it was done in bug 769266, by merging all the caps extracted from all the available codec profiles. Also added an optimization to only return the color formats of the specified profile in dowstream caps.

I know these are complicated patches, but if you can look through them would be great.

Since tomorrow afternoon (EEST) is the code freeze, I'll push them by the morning. Sorry.
Comment 14 Scott D Phillips 2017-04-06 21:07:31 UTC
Review of attachment 349404 [details] [review]:

::: gst-libs/gst/vaapi/gstvaapiencoder.c
@@ +598,3 @@
+          gst_vaapi_profile_get_va_profile (cip->profile),
+          gst_vaapi_entrypoint_get_va_entrypoint (cip->entrypoint),
+  guint value;

a query for VAConfigAttribRTFormat returns a bit field with supported RT_FORMATS or'd together, it won't give a single preferred default format.
Comment 15 Scott D Phillips 2017-04-06 21:18:26 UTC
Review of attachment 349412 [details] [review]:

::: gst-libs/gst/vaapi/gstvaapiencoder_h265.c
@@ +1439,3 @@
+  const GstVideoFormat format =
+      GST_VIDEO_INFO_FORMAT (GST_VAAPI_ENCODER_VIDEO_INFO (encoder));
+  guint bits_depth_luma =

small nit: may be a good idea to add the minus8 to the variable name here so it's obvious that -8 doesn't need to be applied below.
Comment 16 Scott D Phillips 2017-04-06 21:21:04 UTC
(In reply to Scott D Phillips from comment #14)
> a query for VAConfigAttribRTFormat returns a bit field with supported
> RT_FORMATS or'd together, it won't give a single preferred default format.

Maybe a different approach here would be to make a map from profile to rt_format? for h.264 and h.265 each profile supports a single subsampling type.
Comment 17 Scott D Phillips 2017-04-06 21:22:44 UTC
All other patches lgtm
Comment 18 Víctor Manuel Jáquez Leal 2017-04-07 09:36:11 UTC
(In reply to Scott D Phillips from comment #14)
> Review of attachment 349404 [details] [review] [review]:
> 
> ::: gst-libs/gst/vaapi/gstvaapiencoder.c
> @@ +598,3 @@
> +          gst_vaapi_profile_get_va_profile (cip->profile),
> +          gst_vaapi_entrypoint_get_va_entrypoint (cip->entrypoint),
> +  guint value;
> 
> a query for VAConfigAttribRTFormat returns a bit field with supported
> RT_FORMATS or'd together, it won't give a single preferred default format.

True. But still I prefer to rely on this "less hard" assumption rather than hard-coded map of preferred rt_format per VA profile, because it heavily relies on the specific driver.
Comment 19 Víctor Manuel Jáquez Leal 2017-04-07 09:46:59 UTC
(In reply to Scott D Phillips from comment #15)
> Review of attachment 349412 [details] [review] [review]:
> 
> ::: gst-libs/gst/vaapi/gstvaapiencoder_h265.c
> @@ +1439,3 @@
> +  const GstVideoFormat format =
> +      GST_VIDEO_INFO_FORMAT (GST_VAAPI_ENCODER_VIDEO_INFO (encoder));
> +  guint bits_depth_luma =
> 
> small nit: may be a good idea to add the minus8 to the variable name here so
> it's obvious that -8 doesn't need to be applied below.

Agree.
Comment 20 Víctor Manuel Jáquez Leal 2017-04-07 10:11:40 UTC
Attachment 349403 [details] pushed as 914a471 - vaapiencode: enhance logs of negotiated caps
Attachment 349404 [details] pushed as db72681 - libs: encoder: initialize chroma_type
Attachment 349405 [details] pushed as 31d326c - libs: encoder: refactor init_context_info()
Attachment 349406 [details] pushed as 9aa63e4 - libs: encoder: dummy context for get_surface_formats()
Attachment 349407 [details] pushed as 7153b45 - libs: encoder: pass profile to get_surface_formats()
Attachment 349408 [details] pushed as 5ccadd6 - vaapiencode: add get_profile() vmethod
Attachment 349409 [details] pushed as e534ff5 - libs: encode: merge all possible surface formats
Attachment 349410 [details] pushed as 48e21d6 - libs: encoder: h265: ensures profile given format
Attachment 349411 [details] pushed as d744aeb - libs: encoder: admit YUV420_10BPP as valid chroma
Attachment 349412 [details] pushed as 3532dca - libs: encoder: h265: chroma and luma with format
Attachment 349413 [details] pushed as b2815a3 - vaapiencode: h265: add main-10 in caps template