[Bug 790115] video: add SMPTE 2086 meta API for partially supporting HDR

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=790115

Sebastian Dröge (slomo) <slomo at coaxion.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #374054|none                        |needs-work
             status|                            |

--- Comment #17 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
Review of attachment 374054:
 --> (https://bugzilla.gnome.org/review?bug=790115&attachment=374054)

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?

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.