diff mbox series

[FFmpeg-devel] Update HDR10+ metadata structure.

Message ID 20200205024400.46883-1-moh.izadi@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel] Update HDR10+ metadata structure. | expand

Checks

Context Check Description
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Mohammad Izadi Feb. 5, 2020, 2:44 a.m. UTC 
From: Mohammad Izadi <izadi@google.com>

Trying to read HDR10+ metadata from HEVC/SEI and pass it to packet side data in the follow-up CLs.
---
 libavutil/hdr_dynamic_metadata.c | 167 ++++++++++++++++++++++++++++++-
 libavutil/hdr_dynamic_metadata.h |  14 ++-
 libavutil/version.h              |   2 +-
 3 files changed, 180 insertions(+), 3 deletions(-)

Comments

Moritz Barsnick Feb. 5, 2020, 1:48 p.m. UTC | #1 
Hi Mohammad,

On Tue, Feb 04, 2020 at 18:44:00 -0800, Mohammad Izadi wrote:
> --- a/libavutil/hdr_dynamic_metadata.c
> +++ b/libavutil/hdr_dynamic_metadata.c
> @@ -1,4 +1,4 @@
> -/**
> + /**

Please review your git diff and your submitted patches carefully. You
need to avoid this accidental change.

> +    if(!data)

ffmpeg style: "if ("

> +    if(ret < 0)
> +        return AVERROR_INVALIDDATA;

Ditto.

> +    if (get_bits_left(&gb) < 2)
> +        return AVERROR_INVALIDDATA;
> +    s->num_windows = get_bits(&gb, 2);
> +
> +    if (s->num_windows < 1 || s->num_windows > 3) {
> +        return AVERROR_INVALIDDATA;
> +    }

Above, you skip the brackets for the one-liner return. Here, you use
them. That's inconsistent.

> --- a/libavutil/hdr_dynamic_metadata.h
> +++ b/libavutil/hdr_dynamic_metadata.h
> @@ -23,6 +23,8 @@
>
>  #include "frame.h"
>  #include "rational.h"
> +#include "libavcodec/get_bits.h"
> +#include "libavcodec/put_bits.h"

As the header doesn't use functions from these, but the implementation
does, they should be in libavutil/hdr_dynamic_metadata.c instead.

>      /**
>       * The nominal maximum display luminance of the targeted system display,
> -     * in units of 0.0001 candelas per square metre. The value shall be in
> +     * in units of 1 candelas per square metre. The value shall be in
>       * the range of 0 to 10000, inclusive.
>       */

This fix is probably not strictly related to your change?

Cheers,
Moritz
Mohammad Izadi Feb. 5, 2020, 6:43 p.m. UTC | #2 
Please check my responses inline:


On Wed, Feb 5, 2020 at 5:48 AM Moritz Barsnick <barsnick@gmx.net> wrote:

> Hi Mohammad,
>
> On Tue, Feb 04, 2020 at 18:44:00 -0800, Mohammad Izadi wrote:
> > --- a/libavutil/hdr_dynamic_metadata.c
> > +++ b/libavutil/hdr_dynamic_metadata.c
> > @@ -1,4 +1,4 @@
> > -/**
> > + /**
>
> Please review your git diff and your submitted patches carefully. You
> need to avoid this accidental change.
>
reverted the change.

>
> > +    if(!data)
>
> ffmpeg style: "if ("
>
Fixed.

>
> > +    if(ret < 0)
> > +        return AVERROR_INVALIDDATA;
>
> Ditto.
>
> Fixed.

> > +    if (get_bits_left(&gb) < 2)
> > +        return AVERROR_INVALIDDATA;
> > +    s->num_windows = get_bits(&gb, 2);
> > +
> > +    if (s->num_windows < 1 || s->num_windows > 3) {
> > +        return AVERROR_INVALIDDATA;
> > +    }
>
> Above, you skip the brackets for the one-liner return. Here, you use
> them. That's inconsistent.
>
> Fixed.

> > --- a/libavutil/hdr_dynamic_metadata.h
> > +++ b/libavutil/hdr_dynamic_metadata.h
> > @@ -23,6 +23,8 @@
> >
> >  #include "frame.h"
> >  #include "rational.h"
> > +#include "libavcodec/get_bits.h"
> > +#include "libavcodec/put_bits.h"
>
> As the header doesn't use functions from these, but the implementation
> does, they should be in libavutil/hdr_dynamic_metadata.c instead.
>
Right. Changed.

>
> >      /**
> >       * The nominal maximum display luminance of the targeted system
> display,
> > -     * in units of 0.0001 candelas per square metre. The value shall be
> in
> > +     * in units of 1 candelas per square metre. The value shall be in
> >       * the range of 0 to 10000, inclusive.
> >       */
>
> This fix is probably not strictly related to your change?
>
Right. I have changed ffmpeg locally to pass through HDR10+ in ffmpeg. I
have to split my changes to smaller CLs for review. So, some changes like
this may not strictly related, but required for my next changes. Here, the
comment explanation needs correction anyway.

>
> Cheers,
> Moritz
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

diff --git a/libavutil/hdr_dynamic_metadata.c b/libavutil/hdr_dynamic_metadata.c
index 0fa1ee82de..fbab192c62 100644
--- a/libavutil/hdr_dynamic_metadata.c
+++ b/libavutil/hdr_dynamic_metadata.c
@@ -1,4 +1,4 @@ 
-/**
+ /**
  * Copyright (c) 2018 Mohammad Izadi <moh.izadi at gmail.com>
  *
  * This file is part of FFmpeg.
@@ -21,6 +21,14 @@ 
 #include "hdr_dynamic_metadata.h"
 #include "mem.h"
 
+static const int64_t luminance_den = 1;
+static const int32_t peak_luminance_den = 15;
+static const int64_t rgb_den = 100000;
+static const int32_t fraction_pixel_den = 1000;
+static const int32_t knee_point_den = 4095;
+static const int32_t bezier_anchor_den = 1023;
+static const int32_t saturation_weight_den = 8;
+
 AVDynamicHDRPlus *av_dynamic_hdr_plus_alloc(size_t *size)
 {
     AVDynamicHDRPlus *hdr_plus = av_mallocz(sizeof(AVDynamicHDRPlus));
@@ -45,3 +53,160 @@  AVDynamicHDRPlus *av_dynamic_hdr_plus_create_side_data(AVFrame *frame)
 
     return (AVDynamicHDRPlus *)side_data->data;
 }
+
+int av_dynamic_hdr_plus_decode(const uint8_t *data, size_t size,
+			       AVDynamicHDRPlus *s)
+{
+    int w, i, j;
+    GetBitContext gb;
+    if(!data)
+        return AVERROR_INVALIDDATA;
+    
+    int ret = init_get_bits8(&gb, data, size);
+    if(ret < 0)
+        return AVERROR_INVALIDDATA;
+
+    if (get_bits_left(&gb) < 2)
+        return AVERROR_INVALIDDATA;
+    s->num_windows = get_bits(&gb, 2);
+
+    if (s->num_windows < 1 || s->num_windows > 3) {
+        return AVERROR_INVALIDDATA;
+    }
+
+    if (get_bits_left(&gb) < ((19 * 8 + 1) * (s->num_windows - 1)))
+        return AVERROR_INVALIDDATA;
+    for (w = 1; w < s->num_windows; w++) {
+        s->params[w].window_upper_left_corner_x.num = get_bits(&gb, 16);
+        s->params[w].window_upper_left_corner_y.num = get_bits(&gb, 16);
+        s->params[w].window_lower_right_corner_x.num = get_bits(&gb, 16);
+        s->params[w].window_lower_right_corner_y.num = get_bits(&gb, 16);
+        // The corners are set to absolute coordinates here. They should be
+        // converted to the relative coordinates (in [0, 1]) in the decoder.
+        s->params[w].window_upper_left_corner_x.den = 1;
+        s->params[w].window_upper_left_corner_y.den = 1;
+        s->params[w].window_lower_right_corner_x.den = 1;
+        s->params[w].window_lower_right_corner_y.den = 1;
+
+        s->params[w].center_of_ellipse_x = get_bits(&gb, 16);
+        s->params[w].center_of_ellipse_y = get_bits(&gb, 16);
+        s->params[w].rotation_angle = get_bits(&gb, 8);
+        s->params[w].semimajor_axis_internal_ellipse = get_bits(&gb, 16);
+        s->params[w].semimajor_axis_external_ellipse = get_bits(&gb, 16);
+        s->params[w].semiminor_axis_external_ellipse = get_bits(&gb, 16);
+        s->params[w].overlap_process_option = get_bits1(&gb);
+    }
+
+    if (get_bits_left(&gb) < 28)
+        return AVERROR(EINVAL);
+    s->targeted_system_display_maximum_luminance.num = get_bits(&gb, 27);
+    s->targeted_system_display_maximum_luminance.den = luminance_den;
+    s->targeted_system_display_actual_peak_luminance_flag = get_bits1(&gb);
+
+    if (s->targeted_system_display_actual_peak_luminance_flag) {
+        int rows, cols;
+        if (get_bits_left(&gb) < 10)
+            return AVERROR(EINVAL);
+        rows = get_bits(&gb, 5);
+        cols = get_bits(&gb, 5);
+        if (((rows < 2) || (rows > 25)) || ((cols < 2) || (cols > 25))) {
+            return AVERROR_INVALIDDATA;
+        }
+        s->num_rows_targeted_system_display_actual_peak_luminance = rows;
+        s->num_cols_targeted_system_display_actual_peak_luminance = cols;
+
+        if (get_bits_left(&gb) < (rows * cols * 4))
+            return AVERROR(EINVAL);
+
+        for (i = 0; i < rows; i++) {
+            for (j = 0; j < cols; j++) {
+                s->targeted_system_display_actual_peak_luminance[i][j].num = get_bits(&gb, 4);
+                s->targeted_system_display_actual_peak_luminance[i][j].den = peak_luminance_den;
+            }
+        }
+    }
+    for (w = 0; w < s->num_windows; w++) {
+        if (get_bits_left(&gb) < (3 * 17 + 17 + 4))
+            return AVERROR(EINVAL);
+        for (i = 0; i < 3; i++) {
+            s->params[w].maxscl[i].num = get_bits(&gb, 17);
+            s->params[w].maxscl[i].den = rgb_den;
+        }
+        s->params[w].average_maxrgb.num = get_bits(&gb, 17);
+        s->params[w].average_maxrgb.den = rgb_den;
+        s->params[w].num_distribution_maxrgb_percentiles = get_bits(&gb, 4);
+
+        if (get_bits_left(&gb) <
+            (s->params[w].num_distribution_maxrgb_percentiles * 24))
+            return AVERROR(EINVAL);
+        for (i = 0; i < s->params[w].num_distribution_maxrgb_percentiles; i++) {
+            s->params[w].distribution_maxrgb[i].percentage = get_bits(&gb, 7);
+            s->params[w].distribution_maxrgb[i].percentile.num = get_bits(&gb, 17);
+            s->params[w].distribution_maxrgb[i].percentile.den = rgb_den;
+        }
+
+        if (get_bits_left(&gb) < 10)
+            return AVERROR(EINVAL);
+        s->params[w].fraction_bright_pixels.num = get_bits(&gb, 10);
+        s->params[w].fraction_bright_pixels.den = fraction_pixel_den;
+    }
+    if (get_bits_left(&gb) < 1)
+        return AVERROR(EINVAL);
+    s->mastering_display_actual_peak_luminance_flag = get_bits1(&gb);
+    if (s->mastering_display_actual_peak_luminance_flag) {
+        int rows, cols;
+        if (get_bits_left(&gb) < 10)
+            return AVERROR(EINVAL);
+        rows = get_bits(&gb, 5);
+        cols = get_bits(&gb, 5);
+        if (((rows < 2) || (rows > 25)) || ((cols < 2) || (cols > 25))) {
+            return AVERROR_INVALIDDATA;
+        }
+        s->num_rows_mastering_display_actual_peak_luminance = rows;
+        s->num_cols_mastering_display_actual_peak_luminance = cols;
+
+        if (get_bits_left(&gb) < (rows * cols * 4))
+            return AVERROR(EINVAL);
+
+        for (i = 0; i < rows; i++) {
+            for (j = 0; j < cols; j++) {
+                s->mastering_display_actual_peak_luminance[i][j].num = get_bits(&gb, 4);
+                s->mastering_display_actual_peak_luminance[i][j].den = peak_luminance_den;
+            }
+        }
+    }
+
+    for (w = 0; w < s->num_windows; w++) {
+        if (get_bits_left(&gb) < 1)
+            return AVERROR(EINVAL);
+        s->params[w].tone_mapping_flag = get_bits1(&gb);
+        if (s->params[w].tone_mapping_flag) {
+            if (get_bits_left(&gb) < 28)
+                return AVERROR(EINVAL);
+            s->params[w].knee_point_x.num = get_bits(&gb, 12);
+            s->params[w].knee_point_x.den = knee_point_den;
+            s->params[w].knee_point_y.num = get_bits(&gb, 12);
+            s->params[w].knee_point_y.den = knee_point_den;
+            s->params[w].num_bezier_curve_anchors = get_bits(&gb, 4);
+
+            if (get_bits_left(&gb) < (s->params[w].num_bezier_curve_anchors * 10))
+                return AVERROR(EINVAL);
+            for (i = 0; i < s->params[w].num_bezier_curve_anchors; i++) {
+                s->params[w].bezier_curve_anchors[i].num = get_bits(&gb, 10);
+                s->params[w].bezier_curve_anchors[i].den = bezier_anchor_den;
+            }
+        }
+
+        if (get_bits_left(&gb) < 1)
+            return AVERROR(EINVAL);
+        s->params[w].color_saturation_mapping_flag = get_bits1(&gb);
+        if (s->params[w].color_saturation_mapping_flag) {
+            if (get_bits_left(&gb) < 6)
+                return AVERROR(EINVAL);
+            s->params[w].color_saturation_weight.num = get_bits(&gb, 6);
+            s->params[w].color_saturation_weight.den = saturation_weight_den;
+        }
+    }
+
+    return 0;
+}
diff --git a/libavutil/hdr_dynamic_metadata.h b/libavutil/hdr_dynamic_metadata.h
index 2d72de56ae..0fc070b078 100644
--- a/libavutil/hdr_dynamic_metadata.h
+++ b/libavutil/hdr_dynamic_metadata.h
@@ -23,6 +23,8 @@ 
 
 #include "frame.h"
 #include "rational.h"
+#include "libavcodec/get_bits.h"
+#include "libavcodec/put_bits.h"
 
 /**
  * Option for overlapping elliptical pixel selectors in an image.
@@ -265,7 +267,7 @@  typedef struct AVDynamicHDRPlus {
 
     /**
      * The nominal maximum display luminance of the targeted system display,
-     * in units of 0.0001 candelas per square metre. The value shall be in
+     * in units of 1 candelas per square metre. The value shall be in
      * the range of 0 to 10000, inclusive.
      */
     AVRational targeted_system_display_maximum_luminance;
@@ -340,4 +342,14 @@  AVDynamicHDRPlus *av_dynamic_hdr_plus_alloc(size_t *size);
  */
 AVDynamicHDRPlus *av_dynamic_hdr_plus_create_side_data(AVFrame *frame);
 
+/**
+ * Decode SMPTE ST 2094-40 (the user data registered ITU-T T.35) to AVDynamicHDRPlus.
+ * @param data The user data registered ITU-T T.35 (SMPTE ST 2094-40).
+ * @param size The size of the user data registered ITU-T T.35 (SMPTE ST 2094-40).
+ * @param s The decoded AVDynamicHDRPlus structure.
+ *
+ * @return 0 if succeed. Otherwise, returns the appropriate AVERROR.
+ */
+int av_dynamic_hdr_plus_decode(const uint8_t *data, size_t size, AVDynamicHDRPlus *s);
+
 #endif /* AVUTIL_HDR_DYNAMIC_METADATA_H */
diff --git a/libavutil/version.h b/libavutil/version.h
index af8f614aff..2bc1b98615 100644
--- a/libavutil/version.h
+++ b/libavutil/version.h
@@ -79,7 +79,7 @@ 
  */
 
 #define LIBAVUTIL_VERSION_MAJOR  56
-#define LIBAVUTIL_VERSION_MINOR  38
+#define LIBAVUTIL_VERSION_MINOR  39
 #define LIBAVUTIL_VERSION_MICRO 100
 
 #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \