diff mbox series

[FFmpeg-devel] Update HDR10+ metadata structure.

Message ID 20200210194408.251496-1-moh.izadi@gmail.com
State New
Headers show
Series [FFmpeg-devel] Update HDR10+ metadata structure.
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Mohammad Izadi Feb. 10, 2020, 7:44 p.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 | 165 +++++++++++++++++++++++++++++++
 libavutil/hdr_dynamic_metadata.h |  13 ++-
 libavutil/version.h              |   2 +-
 3 files changed, 178 insertions(+), 2 deletions(-)

Comments

Mohammad Izadi Feb. 18, 2020, 6 p.m. UTC | #1
Hello,

What's the status of the cl review? Is it good to submit?
--
Best,
Mohammad


On Mon, Feb 10, 2020 at 11:44 AM Mohammad Izadi <moh.izadi@gmail.com> wrote:

> 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 | 165 +++++++++++++++++++++++++++++++
>  libavutil/hdr_dynamic_metadata.h |  13 ++-
>  libavutil/version.h              |   2 +-
>  3 files changed, 178 insertions(+), 2 deletions(-)
>
> diff --git a/libavutil/hdr_dynamic_metadata.c
> b/libavutil/hdr_dynamic_metadata.c
> index 0fa1ee82de..f24bcb40f5 100644
> --- a/libavutil/hdr_dynamic_metadata.c
> +++ b/libavutil/hdr_dynamic_metadata.c
> @@ -19,8 +19,17 @@
>   */
>
>  #include "hdr_dynamic_metadata.h"
> +#include "libavcodec/get_bits.h"
>  #include "mem.h"
>
> +static const int32_t luminance_den = 1;
> +static const int32_t peak_luminance_den = 15;
> +static const int32_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 +54,159 @@ 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_INVALIDDATA;
> +    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_INVALIDDATA;
> +        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_INVALIDDATA;
> +
> +        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_INVALIDDATA;
> +        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_INVALIDDATA;
> +        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_INVALIDDATA;
> +        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_INVALIDDATA;
> +    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_INVALIDDATA;
> +        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_INVALIDDATA;
> +
> +        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_INVALIDDATA;
> +        s->params[w].tone_mapping_flag = get_bits1(&gb);
> +        if (s->params[w].tone_mapping_flag) {
> +            if (get_bits_left(&gb) < 28)
> +                return AVERROR_INVALIDDATA;
> +            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_INVALIDDATA;
> +            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_INVALIDDATA;
> +        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_INVALIDDATA;
> +            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..107df4bacf 100644
> --- a/libavutil/hdr_dynamic_metadata.h
> +++ b/libavutil/hdr_dynamic_metadata.h
> @@ -265,7 +265,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 +340,15 @@ AVDynamicHDRPlus *av_dynamic_hdr_plus_alloc(size_t
> *size);
>   */
>  AVDynamicHDRPlus *av_dynamic_hdr_plus_create_side_data(AVFrame *frame);
>
> +/**
> + * Decode the payload of SMPTE ST 2094-40 (coming from the user data
> registered
> + * ITU-T T.35) to AVDynamicHDRPlus.
> + * @param data The payload of SMPTE ST 2094-40 extracted from the user
> data registered ITU-T T.35.
> + * @param size The size of the payload of 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, \
> --
> 2.25.0.341.g760bfbb309-goog
>
>
Derek Buitenhuis Feb. 19, 2020, 3:15 p.m. UTC | #2
On 10/02/2020 19:44, Mohammad Izadi wrote:
>      /**
>       * 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;

Is this comment change related to a behavior change (thus an API
break), or was it always 1, and the comment was wrong?

- Derek
Mohammad Izadi Feb. 19, 2020, 6:25 p.m. UTC | #3
The comment was wrong.
--
Best,
Mohammad


On Wed, Feb 19, 2020 at 7:22 AM Derek Buitenhuis <derek.buitenhuis@gmail.com>
wrote:

> On 10/02/2020 19:44, Mohammad Izadi wrote:
> >      /**
> >       * 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;
>
> Is this comment change related to a behavior change (thus an API
> break), or was it always 1, and the comment was wrong?
>
> - Derek
> _______________________________________________
> 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".
Derek Buitenhuis Feb. 19, 2020, 9:09 p.m. UTC | #4
On 19/02/2020 18:25, Mohammad Izadi wrote:
> The comment was wrong.

Thanks for clarifying!

- Derek
Vittorio Giovara Feb. 20, 2020, 8:02 p.m. UTC | #5
On Mon, Feb 10, 2020 at 3:14 PM Mohammad Izadi <moh.izadi@gmail.com> wrote:

> 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 | 165 +++++++++++++++++++++++++++++++
>  libavutil/hdr_dynamic_metadata.h |  13 ++-
>  libavutil/version.h              |   2 +-
>  3 files changed, 178 insertions(+), 2 deletions(-)
>
> diff --git a/libavutil/hdr_dynamic_metadata.c
> b/libavutil/hdr_dynamic_metadata.c
> index 0fa1ee82de..f24bcb40f5 100644
> --- a/libavutil/hdr_dynamic_metadata.c
> +++ b/libavutil/hdr_dynamic_metadata.c
> @@ -19,8 +19,17 @@
>   */
>
>  #include "hdr_dynamic_metadata.h"
> +#include "libavcodec/get_bits.h"
>

wait is it ok to use libavcodec headers in libavutil? while it's fine at
compilation time since it is an inlined header, I don't think it's a good
idea to freely use such functions in this way: what I would do is rather
implement the parsing in libavcodec, store the fields in a structure
defined in libavutil and then use this new structure in here


>  #include "mem.h"
>
> +static const int32_t luminance_den = 1;
> +static const int32_t peak_luminance_den = 15;
> +static const int32_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;
>

These fields could also be stored in the structure i mentioned, rather then
having them declared as static here.
Mohammad Izadi Feb. 20, 2020, 8:15 p.m. UTC | #6
On Thu, Feb 20, 2020 at 12:10 PM Vittorio Giovara <
vittorio.giovara@gmail.com> wrote:

> On Mon, Feb 10, 2020 at 3:14 PM Mohammad Izadi <moh.izadi@gmail.com>
> wrote:
>
> > 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 | 165 +++++++++++++++++++++++++++++++
> >  libavutil/hdr_dynamic_metadata.h |  13 ++-
> >  libavutil/version.h              |   2 +-
> >  3 files changed, 178 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavutil/hdr_dynamic_metadata.c
> > b/libavutil/hdr_dynamic_metadata.c
> > index 0fa1ee82de..f24bcb40f5 100644
> > --- a/libavutil/hdr_dynamic_metadata.c
> > +++ b/libavutil/hdr_dynamic_metadata.c
> > @@ -19,8 +19,17 @@
> >   */
> >
> >  #include "hdr_dynamic_metadata.h"
> > +#include "libavcodec/get_bits.h"
> >
>
> wait is it ok to use libavcodec headers in libavutil? while it's fine at
> compilation time since it is an inlined header, I don't think it's a good
> idea to freely use such functions in this way: what I would do is rather
> implement the parsing in libavcodec, store the fields in a structure
> defined in libavutil and then use this new structure in here
>
> What if I move it all to livavcodec?

>
> >  #include "mem.h"
> >
> > +static const int32_t luminance_den = 1;
> > +static const int32_t peak_luminance_den = 15;
> > +static const int32_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;
> >
>
> These fields could also be stored in the structure i mentioned, rather then
> having them declared as static here.
>
You mean to create a new struct just for constants?

> --
> Vittorio
> _______________________________________________
> 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".
Vittorio Giovara Feb. 20, 2020, 9:11 p.m. UTC | #7
On Thu, Feb 20, 2020 at 3:16 PM Mohammad Izadi <moh.izadi@gmail.com> wrote:

> On Thu, Feb 20, 2020 at 12:10 PM Vittorio Giovara <
> vittorio.giovara@gmail.com> wrote:
>
> > On Mon, Feb 10, 2020 at 3:14 PM Mohammad Izadi <moh.izadi@gmail.com>
> > wrote:
> >
> > > 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 | 165 +++++++++++++++++++++++++++++++
> > >  libavutil/hdr_dynamic_metadata.h |  13 ++-
> > >  libavutil/version.h              |   2 +-
> > >  3 files changed, 178 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/libavutil/hdr_dynamic_metadata.c
> > > b/libavutil/hdr_dynamic_metadata.c
> > > index 0fa1ee82de..f24bcb40f5 100644
> > > --- a/libavutil/hdr_dynamic_metadata.c
> > > +++ b/libavutil/hdr_dynamic_metadata.c
> > > @@ -19,8 +19,17 @@
> > >   */
> > >
> > >  #include "hdr_dynamic_metadata.h"
> > > +#include "libavcodec/get_bits.h"
> > >
> >
> > wait is it ok to use libavcodec headers in libavutil? while it's fine at
> > compilation time since it is an inlined header, I don't think it's a good
> > idea to freely use such functions in this way: what I would do is rather
> > implement the parsing in libavcodec, store the fields in a structure
> > defined in libavutil and then use this new structure in here
> >
> > What if I move it all to livavcodec?
>
>
that works too, but then you'd need to figure out a way to export this
information to the callers I suppose
See how it is done for the various SEI handling functions, that parse
fields in h264*.c or hevc*.c, call a lavu function, and then export such
information with a frame side data.

(note that you're replying with some formatting on, mixing your reply with
the previous one)


> >
> > >  #include "mem.h"
> > >
> > > +static const int32_t luminance_den = 1;
> > > +static const int32_t peak_luminance_den = 15;
> > > +static const int32_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;
> > >
> >
> > These fields could also be stored in the structure i mentioned, rather
> then
> > having them declared as static here.
> >
> You mean to create a new struct just for constants?
>

no i guess i don't understand why those constants are needed there: if they
are just used as constant you could just use #define instead of static int,
if they are used once, you could use them where needed, and avoid declaring
them in the first place
James Almer Feb. 20, 2020, 9:33 p.m. UTC | #8
On 2/20/2020 5:02 PM, Vittorio Giovara wrote:
> On Mon, Feb 10, 2020 at 3:14 PM Mohammad Izadi <moh.izadi@gmail.com> wrote:
> 
>> 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 | 165 +++++++++++++++++++++++++++++++
>>  libavutil/hdr_dynamic_metadata.h |  13 ++-
>>  libavutil/version.h              |   2 +-
>>  3 files changed, 178 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavutil/hdr_dynamic_metadata.c
>> b/libavutil/hdr_dynamic_metadata.c
>> index 0fa1ee82de..f24bcb40f5 100644
>> --- a/libavutil/hdr_dynamic_metadata.c
>> +++ b/libavutil/hdr_dynamic_metadata.c
>> @@ -19,8 +19,17 @@
>>   */
>>
>>  #include "hdr_dynamic_metadata.h"
>> +#include "libavcodec/get_bits.h"
>>
> 
> wait is it ok to use libavcodec headers in libavutil? while it's fine at
> compilation time since it is an inlined header, I don't think it's a good
> idea to freely use such functions in this way: what I would do is rather
> implement the parsing in libavcodec, store the fields in a structure
> defined in libavutil and then use this new structure in here

This seems excessive only to use a bitstream reader to read a bunch of
fields in a buffer.

get_bits.h is included in lavf, so i don't see why it couldn't be used
in lavu. As you said it's inlined.

> 
> 
>>  #include "mem.h"
>>
>> +static const int32_t luminance_den = 1;
>> +static const int32_t peak_luminance_den = 15;
>> +static const int32_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;
>>
> 
> These fields could also be stored in the structure i mentioned, rather then
> having them declared as static here.
>
Vittorio Giovara Feb. 21, 2020, 4:59 a.m. UTC | #9
On Thu, Feb 20, 2020 at 6:38 PM James Almer <jamrial@gmail.com> wrote:

> On 2/20/2020 5:02 PM, Vittorio Giovara wrote:
> > On Mon, Feb 10, 2020 at 3:14 PM Mohammad Izadi <moh.izadi@gmail.com>
> wrote:
> >
> >> 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 | 165 +++++++++++++++++++++++++++++++
> >>  libavutil/hdr_dynamic_metadata.h |  13 ++-
> >>  libavutil/version.h              |   2 +-
> >>  3 files changed, 178 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/libavutil/hdr_dynamic_metadata.c
> >> b/libavutil/hdr_dynamic_metadata.c
> >> index 0fa1ee82de..f24bcb40f5 100644
> >> --- a/libavutil/hdr_dynamic_metadata.c
> >> +++ b/libavutil/hdr_dynamic_metadata.c
> >> @@ -19,8 +19,17 @@
> >>   */
> >>
> >>  #include "hdr_dynamic_metadata.h"
> >> +#include "libavcodec/get_bits.h"
> >>
> >
> > wait is it ok to use libavcodec headers in libavutil? while it's fine at
> > compilation time since it is an inlined header, I don't think it's a good
> > idea to freely use such functions in this way: what I would do is rather
> > implement the parsing in libavcodec, store the fields in a structure
> > defined in libavutil and then use this new structure in here
>
> This seems excessive only to use a bitstream reader to read a bunch of
> fields in a buffer.
>
> get_bits.h is included in lavf, so i don't see why it couldn't be used
> in lavu.


bacuase lavf is a library which depends on lavc, not vice versa, hierarchy
is respected

As you said it's inlined.
>

I still consider it a poor design choice: either make bitstream reader
public in a separate library (not easy, i am aware) or do the bitstream
reading where the bistream actually is, IMO

Vittorio


> >
> >
> >>  #include "mem.h"
> >>
> >> +static const int32_t luminance_den = 1;
> >> +static const int32_t peak_luminance_den = 15;
> >> +static const int32_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;
> >>
> >
> > These fields could also be stored in the structure i mentioned, rather
> then
> > having them declared as static here.
> >
>
> _______________________________________________
> 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".
Nicolas George Feb. 21, 2020, 10:30 a.m. UTC | #10
Vittorio Giovara (12020-02-20):
> bacuase lavf is a library which depends on lavc, not vice versa, hierarchy
> is respected
> 
> I still consider it a poor design choice

The poor design choice is to keep the libraries separated. We can see
all the trouble it causes us, between this discussion and all the
avpriv_ symbols. I have yet to see any actual benefit.

Regards,
Hendrik Leppkes Feb. 21, 2020, 10:47 a.m. UTC | #11
On Fri, Feb 21, 2020 at 5:59 AM Vittorio Giovara
<vittorio.giovara@gmail.com> wrote:
>
> On Thu, Feb 20, 2020 at 6:38 PM James Almer <jamrial@gmail.com> wrote:
>
> > On 2/20/2020 5:02 PM, Vittorio Giovara wrote:
> > > On Mon, Feb 10, 2020 at 3:14 PM Mohammad Izadi <moh.izadi@gmail.com>
> > wrote:
> > >
> > >> 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 | 165 +++++++++++++++++++++++++++++++
> > >>  libavutil/hdr_dynamic_metadata.h |  13 ++-
> > >>  libavutil/version.h              |   2 +-
> > >>  3 files changed, 178 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/libavutil/hdr_dynamic_metadata.c
> > >> b/libavutil/hdr_dynamic_metadata.c
> > >> index 0fa1ee82de..f24bcb40f5 100644
> > >> --- a/libavutil/hdr_dynamic_metadata.c
> > >> +++ b/libavutil/hdr_dynamic_metadata.c
> > >> @@ -19,8 +19,17 @@
> > >>   */
> > >>
> > >>  #include "hdr_dynamic_metadata.h"
> > >> +#include "libavcodec/get_bits.h"
> > >>
> > >
> > > wait is it ok to use libavcodec headers in libavutil? while it's fine at
> > > compilation time since it is an inlined header, I don't think it's a good
> > > idea to freely use such functions in this way: what I would do is rather
> > > implement the parsing in libavcodec, store the fields in a structure
> > > defined in libavutil and then use this new structure in here
> >
> > This seems excessive only to use a bitstream reader to read a bunch of
> > fields in a buffer.
> >
> > get_bits.h is included in lavf, so i don't see why it couldn't be used
> > in lavu.
>
>
> bacuase lavf is a library which depends on lavc, not vice versa, hierarchy
> is respected
>
> As you said it's inlined.
> >
>
> I still consider it a poor design choice: either make bitstream reader
> public in a separate library (not easy, i am aware) or do the bitstream
> reading where the bistream actually is, IMO
>

I agree that the parsing should just move to avcodec. We parse every
other SEI straight in the codec it comes from, why does this one have
parsing in avutil? It seems weird.

- Hendrik
Mohammad Izadi Feb. 21, 2020, 6:07 p.m. UTC | #12
If you believe lavc is at the top of the hierarchy, I can simply move the
file to lavc. Then both lavc and lavf can use it and hierarchy is
respected. Can I do that? Doesn't break API? Any objection (with solution)?
Let's make right decisions to speed up the process please :).
--
Best,
Mohammad


On Fri, Feb 21, 2020 at 2:53 AM Hendrik Leppkes <h.leppkes@gmail.com> wrote:

> On Fri, Feb 21, 2020 at 5:59 AM Vittorio Giovara
> <vittorio.giovara@gmail.com> wrote:
> >
> > On Thu, Feb 20, 2020 at 6:38 PM James Almer <jamrial@gmail.com> wrote:
> >
> > > On 2/20/2020 5:02 PM, Vittorio Giovara wrote:
> > > > On Mon, Feb 10, 2020 at 3:14 PM Mohammad Izadi <moh.izadi@gmail.com>
> > > wrote:
> > > >
> > > >> 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 | 165
> +++++++++++++++++++++++++++++++
> > > >>  libavutil/hdr_dynamic_metadata.h |  13 ++-
> > > >>  libavutil/version.h              |   2 +-
> > > >>  3 files changed, 178 insertions(+), 2 deletions(-)
> > > >>
> > > >> diff --git a/libavutil/hdr_dynamic_metadata.c
> > > >> b/libavutil/hdr_dynamic_metadata.c
> > > >> index 0fa1ee82de..f24bcb40f5 100644
> > > >> --- a/libavutil/hdr_dynamic_metadata.c
> > > >> +++ b/libavutil/hdr_dynamic_metadata.c
> > > >> @@ -19,8 +19,17 @@
> > > >>   */
> > > >>
> > > >>  #include "hdr_dynamic_metadata.h"
> > > >> +#include "libavcodec/get_bits.h"
> > > >>
> > > >
> > > > wait is it ok to use libavcodec headers in libavutil? while it's
> fine at
> > > > compilation time since it is an inlined header, I don't think it's a
> good
> > > > idea to freely use such functions in this way: what I would do is
> rather
> > > > implement the parsing in libavcodec, store the fields in a structure
> > > > defined in libavutil and then use this new structure in here
> > >
> > > This seems excessive only to use a bitstream reader to read a bunch of
> > > fields in a buffer.
> > >
> > > get_bits.h is included in lavf, so i don't see why it couldn't be used
> > > in lavu.
> >
> >
> > bacuase lavf is a library which depends on lavc, not vice versa,
> hierarchy
> > is respected
> >
> > As you said it's inlined.
> > >
> >
> > I still consider it a poor design choice: either make bitstream reader
> > public in a separate library (not easy, i am aware) or do the bitstream
> > reading where the bistream actually is, IMO
> >
>
> I agree that the parsing should just move to avcodec. We parse every
> other SEI straight in the codec it comes from, why does this one have
> parsing in avutil? It seems weird.
>
> - Hendrik
> _______________________________________________
> 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".
Hendrik Leppkes Feb. 21, 2020, 10:02 p.m. UTC | #13
On Fri, Feb 21, 2020 at 7:08 PM Mohammad Izadi <moh.izadi@gmail.com> wrote:
>
> If you believe lavc is at the top of the hierarchy, I can simply move the
> file to lavc. Then both lavc and lavf can use it and hierarchy is
> respected. Can I do that? Doesn't break API? Any objection (with solution)?
> Let's make right decisions to speed up the process please :).
> --


The struct itself belongs in lavu with everything else of AVFrame. The
parsing of the mpeg-specific SEI data belongs in avcodec. You can't
just blindly move everything.

- Hendrik
Mohammad Izadi Feb. 21, 2020, 10:10 p.m. UTC | #14
Why does the struct belong to lavu? This struct is super similar to structs
in libavcodec/hevc_sei.h. We just move it to a new file to share it between
hevc and vp9 encoder/decoder.

--
Best,
Mohammad


On Fri, Feb 21, 2020 at 2:03 PM Hendrik Leppkes <h.leppkes@gmail.com> wrote:

> On Fri, Feb 21, 2020 at 7:08 PM Mohammad Izadi <moh.izadi@gmail.com>
> wrote:
> >
> > If you believe lavc is at the top of the hierarchy, I can simply move the
> > file to lavc. Then both lavc and lavf can use it and hierarchy is
> > respected. Can I do that? Doesn't break API? Any objection (with
> solution)?
> > Let's make right decisions to speed up the process please :).
> > --
>
>
> The struct itself belongs in lavu with everything else of AVFrame. The
> parsing of the mpeg-specific SEI data belongs in avcodec. You can't
> just blindly move everything.
>
> - Hendrik
> _______________________________________________
> 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".
Vittorio Giovara Feb. 22, 2020, 2:43 a.m. UTC | #15
On Fri, Feb 21, 2020 at 5:17 PM Mohammad Izadi <moh.izadi@gmail.com> wrote:

> Why does the struct belong to lavu? This struct is super similar to structs
> in libavcodec/hevc_sei.h. We just move it to a new file to share it between
> hevc and vp9 encoder/decoder.
>
> --
>

1. Please kindly stop top posting: http://www.idallen.com/topposting.html
2. It belongs to lavu because it's where the frame code generically code
is. I'm not familiar with this API too much, but from what i gather users
may need to have a way of accessing this data without pulling in all the
dependencies of lavc or lavf.

Vittorio

On Fri, Feb 21, 2020 at 2:03 PM Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>
> > On Fri, Feb 21, 2020 at 7:08 PM Mohammad Izadi <moh.izadi@gmail.com>
> > wrote:
> > >
> > > If you believe lavc is at the top of the hierarchy, I can simply move
> the
> > > file to lavc. Then both lavc and lavf can use it and hierarchy is
> > > respected. Can I do that? Doesn't break API? Any objection (with
> > solution)?
> > > Let's make right decisions to speed up the process please :).
> > > --
> >
> >
> > The struct itself belongs in lavu with everything else of AVFrame. The
> > parsing of the mpeg-specific SEI data belongs in avcodec. You can't
> > just blindly move everything.
> >
> > - Hendrik
> > _______________________________________________
> > 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".
> _______________________________________________
> 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".
Mohammad Izadi Feb. 22, 2020, 5:44 p.m. UTC | #16
On Fri, Feb 21, 2020, 6:44 PM Vittorio Giovara <vittorio.giovara@gmail.com>
wrote:

> On Fri, Feb 21, 2020 at 5:17 PM Mohammad Izadi <moh.izadi@gmail.com>
> wrote:
>
> > Why does the struct belong to lavu? This struct is super similar to
> structs
> > in libavcodec/hevc_sei.h. We just move it to a new file to share it
> between
> > hevc and vp9 encoder/decoder.
> >
> > --
> >
>
> 1. Please kindly stop top posting: http://www.idallen.com/topposting.html
> 2. It belongs to lavu because it's where the frame code generically code
> is. I'm not familiar with this API too much, but from what i gather users
> may need to have a way of accessing this data without pulling in all the
> dependencies of lavc or lavf.
>
This struct is related to parsing and SEI, not frame. If so, why other
structs are not in lavu? Please check similar structs in hevc_sei?

>
> Vittorio
>
> On Fri, Feb 21, 2020 at 2:03 PM Hendrik Leppkes <h.leppkes@gmail.com>
> wrote:
> >
> > > On Fri, Feb 21, 2020 at 7:08 PM Mohammad Izadi <moh.izadi@gmail.com>
> > > wrote:
> > > >
> > > > If you believe lavc is at the top of the hierarchy, I can simply move
> > the
> > > > file to lavc. Then both lavc and lavf can use it and hierarchy is
> > > > respected. Can I do that? Doesn't break API? Any objection (with
> > > solution)?
> > > > Let's make right decisions to speed up the process please :).
> > > > --
> > >
> > >
> > > The struct itself belongs in lavu with everything else of AVFrame. The
> > > parsing of the mpeg-specific SEI data belongs in avcodec. You can't
> > > just blindly move everything.
> > >
> > > - Hendrik
> > > _______________________________________________
> > > 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".
> > _______________________________________________
> > 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".
>
>
>
> --
> Vittorio
> _______________________________________________
> 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".
Vittorio Giovara Feb. 24, 2020, 5:29 p.m. UTC | #17
On Sat, Feb 22, 2020 at 12:44 PM Mohammad Izadi <moh.izadi@gmail.com> wrote:

> On Fri, Feb 21, 2020, 6:44 PM Vittorio Giovara <vittorio.giovara@gmail.com
> >
> wrote:
>
> > On Fri, Feb 21, 2020 at 5:17 PM Mohammad Izadi <moh.izadi@gmail.com>
> > wrote:
> >
> > > Why does the struct belong to lavu? This struct is super similar to
> > structs
> > > in libavcodec/hevc_sei.h. We just move it to a new file to share it
> > between
> > > hevc and vp9 encoder/decoder.
> > >
> > > --
> > >
> >
> > 1. Please kindly stop top posting:
> http://www.idallen.com/topposting.html
> > 2. It belongs to lavu because it's where the frame code generically code
> > is. I'm not familiar with this API too much, but from what i gather users
> > may need to have a way of accessing this data without pulling in all the
> > dependencies of lavc or lavf.
> >
> This struct is related to parsing and SEI, not frame. If so, why other
> structs are not in lavu? Please check similar structs in hevc_sei?
>

I don't think I understand your question, but if you need examples you can
check these patches
8f58ecc344a92e63193c38e28c173be987954bbb structure defined in lavu,
e7a6f8c972a0b5b98ef7bbf393e95c434e9e2539 structure populated in lavc
d91718107c33960ad295950d7419e6dba292d723 structure defined in lavu, used in
lavc
7e244c68600f479270e979258e389ed5240885fb same
and so on and so on, so I'd advise you do to the same, scrapping your
current code if necessary.
diff mbox series

Patch

diff --git a/libavutil/hdr_dynamic_metadata.c b/libavutil/hdr_dynamic_metadata.c
index 0fa1ee82de..f24bcb40f5 100644
--- a/libavutil/hdr_dynamic_metadata.c
+++ b/libavutil/hdr_dynamic_metadata.c
@@ -19,8 +19,17 @@ 
  */
 
 #include "hdr_dynamic_metadata.h"
+#include "libavcodec/get_bits.h"
 #include "mem.h"
 
+static const int32_t luminance_den = 1;
+static const int32_t peak_luminance_den = 15;
+static const int32_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 +54,159 @@  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_INVALIDDATA;
+    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_INVALIDDATA;
+        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_INVALIDDATA;
+
+        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_INVALIDDATA;
+        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_INVALIDDATA;
+        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_INVALIDDATA;
+        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_INVALIDDATA;
+    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_INVALIDDATA;
+        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_INVALIDDATA;
+
+        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_INVALIDDATA;
+        s->params[w].tone_mapping_flag = get_bits1(&gb);
+        if (s->params[w].tone_mapping_flag) {
+            if (get_bits_left(&gb) < 28)
+                return AVERROR_INVALIDDATA;
+            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_INVALIDDATA;
+            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_INVALIDDATA;
+        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_INVALIDDATA;
+            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..107df4bacf 100644
--- a/libavutil/hdr_dynamic_metadata.h
+++ b/libavutil/hdr_dynamic_metadata.h
@@ -265,7 +265,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 +340,15 @@  AVDynamicHDRPlus *av_dynamic_hdr_plus_alloc(size_t *size);
  */
 AVDynamicHDRPlus *av_dynamic_hdr_plus_create_side_data(AVFrame *frame);
 
+/**
+ * Decode the payload of SMPTE ST 2094-40 (coming from the user data registered
+ * ITU-T T.35) to AVDynamicHDRPlus.
+ * @param data The payload of SMPTE ST 2094-40 extracted from the user data registered ITU-T T.35.
+ * @param size The size of the payload of 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, \