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 790115 - video: add SMPTE 2086 meta API for partially supporting HDR
video: add SMPTE 2086 meta API for partially supporting HDR
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Windows
: Normal enhancement
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-11-09 11:33 UTC by Seungha Yang
Modified: 2018-11-03 12:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
video-color: Add new GstVideoMasteringDisplayMeta structure (11.98 KB, patch)
2017-11-12 08:13 UTC, Seungha Yang
none Details | Review
tests: video: Add test for mastering-display-meta (2.75 KB, patch)
2017-11-12 08:13 UTC, Seungha Yang
none Details | Review
avviddec: Parse mastering display meta from decoder (3.82 KB, patch)
2017-11-12 08:15 UTC, Seungha Yang
none Details | Review
[base, 1/3] video: Add new APIs for HDR information representation (12.82 KB, patch)
2018-06-24 14:58 UTC, Seungha Yang
none Details | Review
[base, 2/3] tests: video: Add test for video-hdr (2.95 KB, patch)
2018-06-24 14:58 UTC, Seungha Yang
none Details | Review
[base, 3/3] videodecoder: Forward upstream HDR info. to downstream (6.36 KB, patch)
2018-06-24 15:00 UTC, Seungha Yang
none Details | Review
[good, 1/2] matroskademux: Add support parsing HDR metadata (7.72 KB, patch)
2018-06-24 15:02 UTC, Seungha Yang
none Details | Review
[good, 2/2] matroskamux: Write MasteringMetadata and Max{CLL,FALL} (3.72 KB, patch)
2018-06-24 15:03 UTC, Seungha Yang
none Details | Review
avviddec: Parse HDR information from decoder (4.64 KB, patch)
2018-06-24 15:04 UTC, Seungha Yang
none Details | Review
[base, 1/3] video: Add new APIs for HDR information representation (12.83 KB, patch)
2018-10-27 10:09 UTC, Seungha Yang
needs-work Details | Review

Description Seungha Yang 2017-11-09 11:33:04 UTC
To fully support HDR by Gstreamer framework, HDR related color meta data
should be passed to rendering device. Note that, the meta data is used for post processing rather than YUV<->RGB conversion. So there are not much things to do with the HDR meta in Gstreamer pipeline.

However, there is no official APIs to pass the meta data to render device in linux, so we have to be dependent on device specific implementation now (actually, HDR is very render device dependent tech).  
Specifically, v4l2, wayland, x11, things don't provide APIs for them know yet. AFAIK, nvidia provides some APIs on windows system which allow it. See https://lwn.net/Articles/702563/

What should we do now then?
- Add new API for HDR related meta data in order to pass it to any element.
  Custom sink element might be able to utilize it.
- Parsing the meta from ES stream (see Bug 785823) or container (webm or mp4).
- Write it to muxer/encoder
Comment 1 Sebastian Dröge (slomo) 2017-11-09 11:49:18 UTC
Can you propose an API for signalling the HDR related metadata? I assume this should be new optional caps fields, which come from a couple of new enums? What would those enums look like?
Comment 2 Seungha Yang 2017-11-09 12:05:05 UTC
(In reply to Sebastian Dröge (slomo) from comment #1)
> Can you propose an API for signalling the HDR related metadata? I assume
> this should be new optional caps fields, which come from a couple of new
> enums? What would those enums look like?

I'm writing new APIs and planing to add some features on some place.

First, the meta doesn't need a enums I think. All the values are float

HEVC spec specifies it as SEI message syntax for the meta
mastering_display_colour_volume() {
  // for CIE1931 coordinate  
  for ( c = 0; c < 3; c++) {   
    display_primaries_x[c]
    display_primaries_y[c]
  }
  white_point_x
  white_point_y
  max_display_mastering_luminance
  min_display_mastering_luminance
}

I agree with your opinion that it should be optional caps field, because the HDR meta for HDR10 and PQ data does not changed on a stream (called static meta).
Dolby Vision HDR meta, however, can be changed per frames. 
(Dolby Vision is private tech and I don't think en/decoder for Dolby Vision will be opened public, let's do no care about Dolby Vision now.)
Comment 3 Seungha Yang 2017-11-12 08:13:15 UTC
Created attachment 363422 [details] [review]
video-color: Add new GstVideoMasteringDisplayMeta structure

New structure for SMPTE ST 2086 mastering display meta
(or sometimes called HDR meta). The meta can be used by rendering
device during post processing, and for that, some specifications
such as hevc and webm provides syntax.

Use "mastering-display-meta" caps field for this structure
Comment 4 Seungha Yang 2017-11-12 08:13:48 UTC
Created attachment 363423 [details] [review]
tests: video: Add test for mastering-display-meta
Comment 5 Seungha Yang 2017-11-12 08:15:06 UTC
Created attachment 363424 [details] [review]
avviddec: Parse mastering display meta from decoder

If it wasn't provided by upstream, use decoder output meta.
Comment 6 Seungha Yang 2017-11-12 08:16:48 UTC
A HDR sample (with mastering meta) is available at
http://files.hdrsamples.com/downloads/hdr/Exodus_UHD_HDR_Exodus_draft.mp4
Comment 7 Arnaud Vrac 2017-11-13 17:07:47 UTC
I think the proposed patches look good, thanks for this !

In the meta I would avoid the RGB prefix for primaries, and instead use a generic array like ffmpeg does. This would avoid mistakes where the primaries are not R, G, B. For example in HEVC, the  Mastering display colour volume SEI spec mentions:

"For describing mastering displays that use red, green and blue colour primaries,  it is suggested that index value c equal to 0 should correspond to the green primary, c equal to 1 should correspond to the blue primary and c equal to 2 should correspond to the red colour primary."

Also, do you have any plan to also handle the Content Light Level data (AV_PKT_DATA_CONTENT_LIGHT_LEVEL side data) ? This is also very useful for HDR.
Comment 8 Seungha Yang 2017-11-14 02:03:43 UTC
(In reply to Arnaud Vrac from comment #7)
Thanks for your attention!

About RGB prefix, I tried to make them array like ffmpeg does, but I thought it may be able to cause misunderstanding. As you know, hevc SEI message defines that the ordering is GBR. But, ffmpeg changes the ordering to RGB in the array structure. So I think explicit prefix naming can prevent the mistake. Also, other spec like webm uses prefixed syntax for them. What do think about it?

I have plan to add CLL meta which was already implemented in ffmpeg v3.4
One minor reason I didn't report it now is... current ffmpeg version used by gst-libav is v3.3.5 which includes only mastering meta. CLL meta was added in v3.4.
Comment 9 Seungha Yang 2018-06-24 14:58:06 UTC
Created attachment 372800 [details] [review]
[base, 1/3] video: Add new APIs for HDR information representation

New structures GstVideoMasteringDisplayMetadata and
GstVideoContentLightLevel for SMPTE ST 2086 mastering display meta
(or sometimes called HDR meta) and Content light level information
(specified in CEA-861.3, Appendix A) signaling, respectively.
Comment 10 Seungha Yang 2018-06-24 14:58:58 UTC
Created attachment 372801 [details] [review]
[base, 2/3] tests: video: Add test for video-hdr
Comment 11 Seungha Yang 2018-06-24 15:00:02 UTC
Created attachment 372802 [details] [review]
[base, 3/3] videodecoder: Forward upstream HDR info. to downstream

The HDR information should be forwarded to downstream
Comment 12 Seungha Yang 2018-06-24 15:02:37 UTC
Created attachment 372803 [details] [review]
[good, 1/2] matroskademux: Add support parsing HDR metadata

Set SMPTE ST 2086 mastering-display-metadata and
content-light-level to caps, if any
Comment 13 Seungha Yang 2018-06-24 15:03:10 UTC
Created attachment 372804 [details] [review]
[good, 2/2] matroskamux: Write MasteringMetadata and Max{CLL,FALL}

... only if upstream forwarded valid meta data
Comment 14 Seungha Yang 2018-06-24 15:04:30 UTC
Created attachment 372805 [details] [review]
avviddec: Parse HDR information from decoder
Comment 16 Seungha Yang 2018-10-27 10:09:35 UTC
Created attachment 374054 [details] [review]
[base, 1/3] video: Add new APIs for HDR information representation

rebased on top of master
Comment 17 Sebastian Dröge (slomo) 2018-10-27 10:38:26 UTC
Review of attachment 374054 [details] [review]:

Thanks a lot for the patch, this looks really good already :)

One big problem here is that there's no space to add this to GstVideoInfo, so I think for 1.0 we need to keep it separate and people will have to use this API here explicitly. Unless something like a GstVideoInfo2 is added, but that does not seem worth it yet.

::: gst-libs/gst/video/meson.build
@@ +57,3 @@
   'video-converter.h',
   'video-dither.h',
+  'video-hdr.h',

Also has to be added to video_mkenum_headers

::: gst-libs/gst/video/video-hdr.c
@@ +56,3 @@
+  g_return_val_if_fail (minfo != NULL, NULL);
+
+  return g_strdup_printf ("%lf:%lf:%lf:%lf:%lf:%lf:%lf:%lf:%lf:%lf",

That many fields in a single string is not very readable IMHO, but I don't have a better suggestion either :)

Also this is unfortunately locale dependant: on a German locale a , would be used as decimal separator instead of a . There's some API in GLib to do it locale-independent.

@@ +82,3 @@
+  g_return_val_if_fail (mastering != NULL, FALSE);
+
+  if (sscanf (mastering, "%lf:%lf:%lf:%lf:%lf:%lf:%lf:%lf:%lf:%lf",

sscanf() is a bad idea for parsing things. E.g. in a German locale it would use , as a decimal separator, in an English locale .

Something around g_ascii_dtostr() would probably be a solution here.

@@ +152,3 @@
+  g_return_val_if_fail (other != NULL, FALSE);
+
+  return !memcmp (minfo, other, sizeof (GstVideoMasteringDisplayMetadata));

memcmp() of structs is problematic if the compiler is adding padding to the struct. Better to compare the individual fields.

::: gst-libs/gst/video/video-hdr.h
@@ +41,3 @@
+ * @min_luma: minimum display luminance
+ *
+ * Mastering display color volume information defined by SMPTE ST 2086 (HDR meta).

Since: 1.16

Also for all other newly added API. And the new symbols have to be added to docs/libs/*sections.txt

@@ +49,3 @@
+  gdouble Wx, Wy;
+  gdouble max_luma, min_luma;
+} GstVideoMasteringDisplayMetadata;

Some background about the name: this is how it's also called in ffmpeg and in the HEVC spec. API-design is closely related to how colorimetry is done

@@ +92,3 @@
+                                                           const gchar * level);
+
+struct _GstVideoHDR {

This struct does not seem to be used anywhere, I don't think we need it?
Comment 18 Seungha Yang 2018-10-27 12:54:17 UTC
(In reply to Sebastian Dröge (slomo) from comment #17)
> Review of attachment 374054 [details] [review] [review]:
> ::: gst-libs/gst/video/video-hdr.c
> @@ +56,3 @@
> +  g_return_val_if_fail (minfo != NULL, NULL);
> +
> +  return g_strdup_printf ("%lf:%lf:%lf:%lf:%lf:%lf:%lf:%lf:%lf:%lf",
> 
> That many fields in a single string is not very readable IMHO, but I don't
> have a better suggestion either :)
> 
> Also this is unfortunately locale dependant: on a German locale a , would be
> used as decimal separator instead of a . There's some API in GLib to do it
> locale-independent.

What if we use fraction instead of double? I found there might be precision error during converting between double and string using g_ascii_dtostr(). This precision error makes equality check difficult 

> @@ +92,3 @@
> +                                                           const gchar *
> level);
> +
> +struct _GstVideoHDR {
> 
> This struct does not seem to be used anywhere, I don't think we need it?

That's actually used by the patch "videodecoder: Forward upstream HDR info. to downstream" but seems to redundant. I'll check whether adding it is really required or not.
Comment 19 GStreamer system administrator 2018-11-03 12:01:40 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/400.