diff mbox

[FFmpeg-devel,1/2] Support HDR dynamic metdata (HDR10+) in HEVC decoder.

Message ID 20190103200541.214672-1-moh.izadi@gmail.com
State Superseded
Headers show

Commit Message

Mohammad Izadi Jan. 3, 2019, 8:05 p.m. UTC 
---
 libavcodec/hevc_sei.c | 219 ++++++++++++++++++++++++++++++++++++++++--
 libavcodec/hevc_sei.h |  46 +++++++++
 libavcodec/hevcdec.c  | 195 +++++++++++++++++++++++++++++++++++++
 3 files changed, 454 insertions(+), 6 deletions(-)

Comments

Rostislav Pehlivanov Jan. 4, 2019, 1:07 a.m. UTC | #1 
On Thu, 3 Jan 2019 at 20:13, Mohammad Izadi <moh.izadi@gmail.com> wrote:

>
>  /**
> @@ -94,6 +95,50 @@ typedef struct HEVCSEIMasteringDisplay {
>      uint32_t min_luminance;
>  } HEVCSEIMasteringDisplay;
>
> +typedef struct HEVCSEIDynamicHDRPlus{
> +    int present;
> +    uint8_t itu_t_t35_country_code;
> +    uint8_t application_version;
> +    uint8_t num_windows;
> +    struct {
> +        AVRational window_upper_left_corner_x;
> +        AVRational window_upper_left_corner_y;
> +        AVRational window_lower_right_corner_x;
> +        AVRational window_lower_right_corner_y;
> +        uint16_t center_of_ellipse_x;
> +        uint16_t center_of_ellipse_y;
> +        uint8_t rotation_angle;
> +        uint16_t semimajor_axis_internal_ellipse;
> +        uint16_t semimajor_axis_external_ellipse;
> +        uint16_t semiminor_axis_external_ellipse;
> +        uint8_t overlap_process_option;
> +        AVRational maxscl[3];
> +        AVRational average_maxrgb;
> +        uint8_t num_distribution_maxrgb_percentiles;
> +        struct {
> +            uint8_t percentage;
> +            AVRational percentile;
> +        } distribution_maxrgb[15];
> +        AVRational fraction_bright_pixels;
> +        uint8_t tone_mapping_flag;
> +        AVRational knee_point_x;
> +        AVRational knee_point_y;
> +        uint8_t num_bezier_curve_anchors;
> +        AVRational bezier_curve_anchors[15];
> +        uint8_t color_saturation_mapping_flag;
> +        AVRational color_saturation_weight;
> +    } params[3];
> +    AVRational targeted_system_display_maximum_luminance;
> +    uint8_t targeted_system_display_actual_peak_luminance_flag;
> +    uint8_t num_rows_targeted_system_display_actual_peak_luminance;
> +    uint8_t num_cols_targeted_system_display_actual_peak_luminance;
> +    AVRational targeted_system_display_actual_peak_luminance[25][25];
> +    uint8_t mastering_display_actual_peak_luminance_flag;
> +    uint8_t num_rows_mastering_display_actual_peak_luminance;
> +    uint8_t num_cols_mastering_display_actual_peak_luminance;
> +    AVRational mastering_display_actual_peak_luminance[25][25];
> +} HEVCSEIDynamicHDRPlus;
> +
>

There's no reason to create a new struct for this if all you're going to do
is to copy it over and over to new frames.
What you can do is this: on every SEI containing this thing just allocate a
AVDynamicHDRPlus struct via av_dynamic_hdr_plus_alloc, wrap it as an
AVBufferRef via av_buffer_create, populate it, put it as a pointer in
HEVCSEIContentLight and then on every frame just add it to the frame via
av_frame_new_side_data_from_buf. If a new SEI is received unref your
pointer in HEVCSEIDynamicHDRPlus and repeat by allocating a new struct,
wrapping it as a buffer, populating it, etc.
I figure the only reason this wasn't done with other metadata was because
they were much smaller and because av_frame_new_side_data_from_buf didn't
exist until recently.


+            av_log(s->avctx, AV_LOG_DEBUG, ")");
> +            if (metadata->params[w].color_saturation_mapping_flag) {
> +                av_log(s->avctx, AV_LOG_DEBUG,
> +                       " color_saturation_weight=%5.4f",
> +
>  av_q2d(metadata->params[w].color_saturation_weight));
> +            }
> +            av_log(s->avctx, AV_LOG_DEBUG, "}\n");
> +        }
> +        av_log(s->avctx, AV_LOG_DEBUG,
> +               "} End of HDR10+ (SMPTE 2094-40)\n");
> +    }
>

Don't spam stuff like than in the debug log, especially not on every single
frame. Tools exist to print side data so just don't.
James Almer Jan. 4, 2019, 1:42 a.m. UTC | #2 
On 1/3/2019 10:07 PM, Rostislav Pehlivanov wrote:
> On Thu, 3 Jan 2019 at 20:13, Mohammad Izadi <moh.izadi@gmail.com> wrote:
> 
>>
>>  /**
>> @@ -94,6 +95,50 @@ typedef struct HEVCSEIMasteringDisplay {
>>      uint32_t min_luminance;
>>  } HEVCSEIMasteringDisplay;
>>
>> +typedef struct HEVCSEIDynamicHDRPlus{
>> +    int present;
>> +    uint8_t itu_t_t35_country_code;
>> +    uint8_t application_version;
>> +    uint8_t num_windows;
>> +    struct {
>> +        AVRational window_upper_left_corner_x;
>> +        AVRational window_upper_left_corner_y;
>> +        AVRational window_lower_right_corner_x;
>> +        AVRational window_lower_right_corner_y;
>> +        uint16_t center_of_ellipse_x;
>> +        uint16_t center_of_ellipse_y;
>> +        uint8_t rotation_angle;
>> +        uint16_t semimajor_axis_internal_ellipse;
>> +        uint16_t semimajor_axis_external_ellipse;
>> +        uint16_t semiminor_axis_external_ellipse;
>> +        uint8_t overlap_process_option;
>> +        AVRational maxscl[3];
>> +        AVRational average_maxrgb;
>> +        uint8_t num_distribution_maxrgb_percentiles;
>> +        struct {
>> +            uint8_t percentage;
>> +            AVRational percentile;
>> +        } distribution_maxrgb[15];
>> +        AVRational fraction_bright_pixels;
>> +        uint8_t tone_mapping_flag;
>> +        AVRational knee_point_x;
>> +        AVRational knee_point_y;
>> +        uint8_t num_bezier_curve_anchors;
>> +        AVRational bezier_curve_anchors[15];
>> +        uint8_t color_saturation_mapping_flag;
>> +        AVRational color_saturation_weight;
>> +    } params[3];
>> +    AVRational targeted_system_display_maximum_luminance;
>> +    uint8_t targeted_system_display_actual_peak_luminance_flag;
>> +    uint8_t num_rows_targeted_system_display_actual_peak_luminance;
>> +    uint8_t num_cols_targeted_system_display_actual_peak_luminance;
>> +    AVRational targeted_system_display_actual_peak_luminance[25][25];
>> +    uint8_t mastering_display_actual_peak_luminance_flag;
>> +    uint8_t num_rows_mastering_display_actual_peak_luminance;
>> +    uint8_t num_cols_mastering_display_actual_peak_luminance;
>> +    AVRational mastering_display_actual_peak_luminance[25][25];
>> +} HEVCSEIDynamicHDRPlus;
>> +
>>
> 
> There's no reason to create a new struct for this if all you're going to do
> is to copy it over and over to new frames.

It was my suggestion, as that's what's done for mastering metadata.

> What you can do is this: on every SEI containing this thing just allocate a
> AVDynamicHDRPlus struct via av_dynamic_hdr_plus_alloc, wrap it as an
> AVBufferRef via av_buffer_create, populate it, put it as a pointer in
> HEVCSEIContentLight and then on every frame just add it to the frame via
> av_frame_new_side_data_from_buf. If a new SEI is received unref your
> pointer in HEVCSEIDynamicHDRPlus and repeat by allocating a new struct,
> wrapping it as a buffer, populating it, etc.

You're right, that does sound simpler. Is this SEI valid for every frame
after it shows up? If so, it's definitely a better solution.

> I figure the only reason this wasn't done with other metadata was because
> they were much smaller and because av_frame_new_side_data_from_buf didn't
> exist until recently.
> 
> 
> +            av_log(s->avctx, AV_LOG_DEBUG, ")");
>> +            if (metadata->params[w].color_saturation_mapping_flag) {
>> +                av_log(s->avctx, AV_LOG_DEBUG,
>> +                       " color_saturation_weight=%5.4f",
>> +
>>  av_q2d(metadata->params[w].color_saturation_weight));
>> +            }
>> +            av_log(s->avctx, AV_LOG_DEBUG, "}\n");
>> +        }
>> +        av_log(s->avctx, AV_LOG_DEBUG,
>> +               "} End of HDR10+ (SMPTE 2094-40)\n");
>> +    }
>>
> 
> Don't spam stuff like than in the debug log, especially not on every single
> frame. Tools exist to print side data so just don't.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Mohammad Izadi Jan. 4, 2019, 5:51 p.m. UTC | #3 
I did that before and James ask me to copy. So now, James are you OK with
that?
--
Best,
Mohammad


On Thu, Jan 3, 2019 at 5:08 PM Rostislav Pehlivanov <atomnuker@gmail.com>
wrote:

> On Thu, 3 Jan 2019 at 20:13, Mohammad Izadi <moh.izadi@gmail.com> wrote:
>
> >
> >  /**
> > @@ -94,6 +95,50 @@ typedef struct HEVCSEIMasteringDisplay {
> >      uint32_t min_luminance;
> >  } HEVCSEIMasteringDisplay;
> >
> > +typedef struct HEVCSEIDynamicHDRPlus{
> > +    int present;
> > +    uint8_t itu_t_t35_country_code;
> > +    uint8_t application_version;
> > +    uint8_t num_windows;
> > +    struct {
> > +        AVRational window_upper_left_corner_x;
> > +        AVRational window_upper_left_corner_y;
> > +        AVRational window_lower_right_corner_x;
> > +        AVRational window_lower_right_corner_y;
> > +        uint16_t center_of_ellipse_x;
> > +        uint16_t center_of_ellipse_y;
> > +        uint8_t rotation_angle;
> > +        uint16_t semimajor_axis_internal_ellipse;
> > +        uint16_t semimajor_axis_external_ellipse;
> > +        uint16_t semiminor_axis_external_ellipse;
> > +        uint8_t overlap_process_option;
> > +        AVRational maxscl[3];
> > +        AVRational average_maxrgb;
> > +        uint8_t num_distribution_maxrgb_percentiles;
> > +        struct {
> > +            uint8_t percentage;
> > +            AVRational percentile;
> > +        } distribution_maxrgb[15];
> > +        AVRational fraction_bright_pixels;
> > +        uint8_t tone_mapping_flag;
> > +        AVRational knee_point_x;
> > +        AVRational knee_point_y;
> > +        uint8_t num_bezier_curve_anchors;
> > +        AVRational bezier_curve_anchors[15];
> > +        uint8_t color_saturation_mapping_flag;
> > +        AVRational color_saturation_weight;
> > +    } params[3];
> > +    AVRational targeted_system_display_maximum_luminance;
> > +    uint8_t targeted_system_display_actual_peak_luminance_flag;
> > +    uint8_t num_rows_targeted_system_display_actual_peak_luminance;
> > +    uint8_t num_cols_targeted_system_display_actual_peak_luminance;
> > +    AVRational targeted_system_display_actual_peak_luminance[25][25];
> > +    uint8_t mastering_display_actual_peak_luminance_flag;
> > +    uint8_t num_rows_mastering_display_actual_peak_luminance;
> > +    uint8_t num_cols_mastering_display_actual_peak_luminance;
> > +    AVRational mastering_display_actual_peak_luminance[25][25];
> > +} HEVCSEIDynamicHDRPlus;
> > +
> >
>
> There's no reason to create a new struct for this if all you're going to do
> is to copy it over and over to new frames.
> What you can do is this: on every SEI containing this thing just allocate a
> AVDynamicHDRPlus struct via av_dynamic_hdr_plus_alloc, wrap it as an
> AVBufferRef via av_buffer_create, populate it, put it as a pointer in
> HEVCSEIContentLight and then on every frame just add it to the frame via
> av_frame_new_side_data_from_buf. If a new SEI is received unref your
> pointer in HEVCSEIDynamicHDRPlus and repeat by allocating a new struct,
> wrapping it as a buffer, populating it, etc.
> I figure the only reason this wasn't done with other metadata was because
> they were much smaller and because av_frame_new_side_data_from_buf didn't
> exist until recently.
>
>
> +            av_log(s->avctx, AV_LOG_DEBUG, ")");
> > +            if (metadata->params[w].color_saturation_mapping_flag) {
> > +                av_log(s->avctx, AV_LOG_DEBUG,
> > +                       " color_saturation_weight=%5.4f",
> > +
> >  av_q2d(metadata->params[w].color_saturation_weight));
> > +            }
> > +            av_log(s->avctx, AV_LOG_DEBUG, "}\n");
> > +        }
> > +        av_log(s->avctx, AV_LOG_DEBUG,
> > +               "} End of HDR10+ (SMPTE 2094-40)\n");
> > +    }
> >
>
> Don't spam stuff like than in the debug log, especially not on every single
> frame. Tools exist to print side data so just don't.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Mohammad Izadi Jan. 4, 2019, 6:12 p.m. UTC | #4 
You mean a pointer in HEVCSEIDynamicHDRPlus, not in HEVCSEIContentLight?
--
Best,
Mohammad


On Thu, Jan 3, 2019 at 5:08 PM Rostislav Pehlivanov <atomnuker@gmail.com>
wrote:

> On Thu, 3 Jan 2019 at 20:13, Mohammad Izadi <moh.izadi@gmail.com> wrote:
>
> >
> >  /**
> > @@ -94,6 +95,50 @@ typedef struct HEVCSEIMasteringDisplay {
> >      uint32_t min_luminance;
> >  } HEVCSEIMasteringDisplay;
> >
> > +typedef struct HEVCSEIDynamicHDRPlus{
> > +    int present;
> > +    uint8_t itu_t_t35_country_code;
> > +    uint8_t application_version;
> > +    uint8_t num_windows;
> > +    struct {
> > +        AVRational window_upper_left_corner_x;
> > +        AVRational window_upper_left_corner_y;
> > +        AVRational window_lower_right_corner_x;
> > +        AVRational window_lower_right_corner_y;
> > +        uint16_t center_of_ellipse_x;
> > +        uint16_t center_of_ellipse_y;
> > +        uint8_t rotation_angle;
> > +        uint16_t semimajor_axis_internal_ellipse;
> > +        uint16_t semimajor_axis_external_ellipse;
> > +        uint16_t semiminor_axis_external_ellipse;
> > +        uint8_t overlap_process_option;
> > +        AVRational maxscl[3];
> > +        AVRational average_maxrgb;
> > +        uint8_t num_distribution_maxrgb_percentiles;
> > +        struct {
> > +            uint8_t percentage;
> > +            AVRational percentile;
> > +        } distribution_maxrgb[15];
> > +        AVRational fraction_bright_pixels;
> > +        uint8_t tone_mapping_flag;
> > +        AVRational knee_point_x;
> > +        AVRational knee_point_y;
> > +        uint8_t num_bezier_curve_anchors;
> > +        AVRational bezier_curve_anchors[15];
> > +        uint8_t color_saturation_mapping_flag;
> > +        AVRational color_saturation_weight;
> > +    } params[3];
> > +    AVRational targeted_system_display_maximum_luminance;
> > +    uint8_t targeted_system_display_actual_peak_luminance_flag;
> > +    uint8_t num_rows_targeted_system_display_actual_peak_luminance;
> > +    uint8_t num_cols_targeted_system_display_actual_peak_luminance;
> > +    AVRational targeted_system_display_actual_peak_luminance[25][25];
> > +    uint8_t mastering_display_actual_peak_luminance_flag;
> > +    uint8_t num_rows_mastering_display_actual_peak_luminance;
> > +    uint8_t num_cols_mastering_display_actual_peak_luminance;
> > +    AVRational mastering_display_actual_peak_luminance[25][25];
> > +} HEVCSEIDynamicHDRPlus;
> > +
> >
>
> There's no reason to create a new struct for this if all you're going to do
> is to copy it over and over to new frames.
> What you can do is this: on every SEI containing this thing just allocate a
> AVDynamicHDRPlus struct via av_dynamic_hdr_plus_alloc, wrap it as an
> AVBufferRef via av_buffer_create, populate it, put it as a pointer in
> HEVCSEIContentLight and then on every frame just add it to the frame via
> av_frame_new_side_data_from_buf. If a new SEI is received unref your
> pointer in HEVCSEIDynamicHDRPlus and repeat by allocating a new struct,
> wrapping it as a buffer, populating it, etc.
> I figure the only reason this wasn't done with other metadata was because
> they were much smaller and because av_frame_new_side_data_from_buf didn't
> exist until recently.
>
>
> +            av_log(s->avctx, AV_LOG_DEBUG, ")");
> > +            if (metadata->params[w].color_saturation_mapping_flag) {
> > +                av_log(s->avctx, AV_LOG_DEBUG,
> > +                       " color_saturation_weight=%5.4f",
> > +
> >  av_q2d(metadata->params[w].color_saturation_weight));
> > +            }
> > +            av_log(s->avctx, AV_LOG_DEBUG, "}\n");
> > +        }
> > +        av_log(s->avctx, AV_LOG_DEBUG,
> > +               "} End of HDR10+ (SMPTE 2094-40)\n");
> > +    }
> >
>
> Don't spam stuff like than in the debug log, especially not on every single
> frame. Tools exist to print side data so just don't.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Rostislav Pehlivanov Jan. 4, 2019, 6:53 p.m. UTC | #5 
On Fri, 4 Jan 2019 at 18:12, Mohammad Izadi <moh.izadi@gmail.com> wrote:

> You mean a pointer in HEVCSEIDynamicHDRPlus, not in HEVCSEIContentLight?
> --
> Best,
> Mohammad
>
>
> On Thu, Jan 3, 2019 at 5:08 PM Rostislav Pehlivanov <atomnuker@gmail.com>
> wrote:
>
> > On Thu, 3 Jan 2019 at 20:13, Mohammad Izadi <moh.izadi@gmail.com> wrote:
> >
> > >
> > >  /**
> > > @@ -94,6 +95,50 @@ typedef struct HEVCSEIMasteringDisplay {
> > >      uint32_t min_luminance;
> > >  } HEVCSEIMasteringDisplay;
> > >
> > > +typedef struct HEVCSEIDynamicHDRPlus{
> > > +    int present;
> > > +    uint8_t itu_t_t35_country_code;
> > > +    uint8_t application_version;
> > > +    uint8_t num_windows;
> > > +    struct {
> > > +        AVRational window_upper_left_corner_x;
> > > +        AVRational window_upper_left_corner_y;
> > > +        AVRational window_lower_right_corner_x;
> > > +        AVRational window_lower_right_corner_y;
> > > +        uint16_t center_of_ellipse_x;
> > > +        uint16_t center_of_ellipse_y;
> > > +        uint8_t rotation_angle;
> > > +        uint16_t semimajor_axis_internal_ellipse;
> > > +        uint16_t semimajor_axis_external_ellipse;
> > > +        uint16_t semiminor_axis_external_ellipse;
> > > +        uint8_t overlap_process_option;
> > > +        AVRational maxscl[3];
> > > +        AVRational average_maxrgb;
> > > +        uint8_t num_distribution_maxrgb_percentiles;
> > > +        struct {
> > > +            uint8_t percentage;
> > > +            AVRational percentile;
> > > +        } distribution_maxrgb[15];
> > > +        AVRational fraction_bright_pixels;
> > > +        uint8_t tone_mapping_flag;
> > > +        AVRational knee_point_x;
> > > +        AVRational knee_point_y;
> > > +        uint8_t num_bezier_curve_anchors;
> > > +        AVRational bezier_curve_anchors[15];
> > > +        uint8_t color_saturation_mapping_flag;
> > > +        AVRational color_saturation_weight;
> > > +    } params[3];
> > > +    AVRational targeted_system_display_maximum_luminance;
> > > +    uint8_t targeted_system_display_actual_peak_luminance_flag;
> > > +    uint8_t num_rows_targeted_system_display_actual_peak_luminance;
> > > +    uint8_t num_cols_targeted_system_display_actual_peak_luminance;
> > > +    AVRational targeted_system_display_actual_peak_luminance[25][25];
> > > +    uint8_t mastering_display_actual_peak_luminance_flag;
> > > +    uint8_t num_rows_mastering_display_actual_peak_luminance;
> > > +    uint8_t num_cols_mastering_display_actual_peak_luminance;
> > > +    AVRational mastering_display_actual_peak_luminance[25][25];
> > > +} HEVCSEIDynamicHDRPlus;
> > > +
> > >
> >
> > There's no reason to create a new struct for this if all you're going to
> do
> > is to copy it over and over to new frames.
> > What you can do is this: on every SEI containing this thing just
> allocate a
> > AVDynamicHDRPlus struct via av_dynamic_hdr_plus_alloc, wrap it as an
> > AVBufferRef via av_buffer_create, populate it, put it as a pointer in
> > HEVCSEIContentLight and then on every frame just add it to the frame via
> > av_frame_new_side_data_from_buf. If a new SEI is received unref your
> > pointer in HEVCSEIDynamicHDRPlus and repeat by allocating a new struct,
> > wrapping it as a buffer, populating it, etc.
> > I figure the only reason this wasn't done with other metadata was because
> > they were much smaller and because av_frame_new_side_data_from_buf didn't
> > exist until recently.
> >
> >
> > +            av_log(s->avctx, AV_LOG_DEBUG, ")");
> > > +            if (metadata->params[w].color_saturation_mapping_flag) {
> > > +                av_log(s->avctx, AV_LOG_DEBUG,
> > > +                       " color_saturation_weight=%5.4f",
> > > +
> > >  av_q2d(metadata->params[w].color_saturation_weight));
> > > +            }
> > > +            av_log(s->avctx, AV_LOG_DEBUG, "}\n");
> > > +        }
> > > +        av_log(s->avctx, AV_LOG_DEBUG,
> > > +               "} End of HDR10+ (SMPTE 2094-40)\n");
> > > +    }
> > >
> >
> > Don't spam stuff like than in the debug log, especially not on every
> single
> > frame. Tools exist to print side data so just don't.
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


No, I mean in HEVCSEIContentLight. Like I said, HEVCSEIDynamicHDRPlus
shouldn't exist.
Mohammad Izadi Jan. 4, 2019, 7:41 p.m. UTC | #6 
I like your idea of using AVBufferRef. Thanks for that. However, I prefer
both HEVCSEIContentLight and HEVCSEIDynamicHDRPlus exist, because
ContentLight and Dynamic HDR10+ are two different things. For a frame, we
can have both or none of them or only one of them. It would be more complex
to set the field of present and we would mess up with passing through the
data. For example, let say we dont have ContentLight data  while we have
HDR10+ data for a frame (present must be set), then we pass false
ContentLight data to side data.

Why not having separate struct? What's the problem with new struct with
internal consumption? The problem would get more complicated when we add
more dynamic metadata standards (like Dolby) in the future under
ContentLight struct.

--
Best,
Mohammad


On Fri, Jan 4, 2019 at 10:54 AM Rostislav Pehlivanov <atomnuker@gmail.com>
wrote:

> On Fri, 4 Jan 2019 at 18:12, Mohammad Izadi <moh.izadi@gmail.com> wrote:
>
> > You mean a pointer in HEVCSEIDynamicHDRPlus, not in HEVCSEIContentLight?
> > --
> > Best,
> > Mohammad
> >
> >
> > On Thu, Jan 3, 2019 at 5:08 PM Rostislav Pehlivanov <atomnuker@gmail.com
> >
> > wrote:
> >
> > > On Thu, 3 Jan 2019 at 20:13, Mohammad Izadi <moh.izadi@gmail.com>
> wrote:
> > >
> > > >
> > > >  /**
> > > > @@ -94,6 +95,50 @@ typedef struct HEVCSEIMasteringDisplay {
> > > >      uint32_t min_luminance;
> > > >  } HEVCSEIMasteringDisplay;
> > > >
> > > > +typedef struct HEVCSEIDynamicHDRPlus{
> > > > +    int present;
> > > > +    uint8_t itu_t_t35_country_code;
> > > > +    uint8_t application_version;
> > > > +    uint8_t num_windows;
> > > > +    struct {
> > > > +        AVRational window_upper_left_corner_x;
> > > > +        AVRational window_upper_left_corner_y;
> > > > +        AVRational window_lower_right_corner_x;
> > > > +        AVRational window_lower_right_corner_y;
> > > > +        uint16_t center_of_ellipse_x;
> > > > +        uint16_t center_of_ellipse_y;
> > > > +        uint8_t rotation_angle;
> > > > +        uint16_t semimajor_axis_internal_ellipse;
> > > > +        uint16_t semimajor_axis_external_ellipse;
> > > > +        uint16_t semiminor_axis_external_ellipse;
> > > > +        uint8_t overlap_process_option;
> > > > +        AVRational maxscl[3];
> > > > +        AVRational average_maxrgb;
> > > > +        uint8_t num_distribution_maxrgb_percentiles;
> > > > +        struct {
> > > > +            uint8_t percentage;
> > > > +            AVRational percentile;
> > > > +        } distribution_maxrgb[15];
> > > > +        AVRational fraction_bright_pixels;
> > > > +        uint8_t tone_mapping_flag;
> > > > +        AVRational knee_point_x;
> > > > +        AVRational knee_point_y;
> > > > +        uint8_t num_bezier_curve_anchors;
> > > > +        AVRational bezier_curve_anchors[15];
> > > > +        uint8_t color_saturation_mapping_flag;
> > > > +        AVRational color_saturation_weight;
> > > > +    } params[3];
> > > > +    AVRational targeted_system_display_maximum_luminance;
> > > > +    uint8_t targeted_system_display_actual_peak_luminance_flag;
> > > > +    uint8_t num_rows_targeted_system_display_actual_peak_luminance;
> > > > +    uint8_t num_cols_targeted_system_display_actual_peak_luminance;
> > > > +    AVRational
> targeted_system_display_actual_peak_luminance[25][25];
> > > > +    uint8_t mastering_display_actual_peak_luminance_flag;
> > > > +    uint8_t num_rows_mastering_display_actual_peak_luminance;
> > > > +    uint8_t num_cols_mastering_display_actual_peak_luminance;
> > > > +    AVRational mastering_display_actual_peak_luminance[25][25];
> > > > +} HEVCSEIDynamicHDRPlus;
> > > > +
> > > >
> > >
> > > There's no reason to create a new struct for this if all you're going
> to
> > do
> > > is to copy it over and over to new frames.
> > > What you can do is this: on every SEI containing this thing just
> > allocate a
> > > AVDynamicHDRPlus struct via av_dynamic_hdr_plus_alloc, wrap it as an
> > > AVBufferRef via av_buffer_create, populate it, put it as a pointer in
> > > HEVCSEIContentLight and then on every frame just add it to the frame
> via
> > > av_frame_new_side_data_from_buf. If a new SEI is received unref your
> > > pointer in HEVCSEIDynamicHDRPlus and repeat by allocating a new struct,
> > > wrapping it as a buffer, populating it, etc.
> > > I figure the only reason this wasn't done with other metadata was
> because
> > > they were much smaller and because av_frame_new_side_data_from_buf
> didn't
> > > exist until recently.
> > >
> > >
> > > +            av_log(s->avctx, AV_LOG_DEBUG, ")");
> > > > +            if (metadata->params[w].color_saturation_mapping_flag) {
> > > > +                av_log(s->avctx, AV_LOG_DEBUG,
> > > > +                       " color_saturation_weight=%5.4f",
> > > > +
> > > >  av_q2d(metadata->params[w].color_saturation_weight));
> > > > +            }
> > > > +            av_log(s->avctx, AV_LOG_DEBUG, "}\n");
> > > > +        }
> > > > +        av_log(s->avctx, AV_LOG_DEBUG,
> > > > +               "} End of HDR10+ (SMPTE 2094-40)\n");
> > > > +    }
> > > >
> > >
> > > Don't spam stuff like than in the debug log, especially not on every
> > single
> > > frame. Tools exist to print side data so just don't.
> > > _______________________________________________
> > > ffmpeg-devel mailing list
> > > ffmpeg-devel@ffmpeg.org
> > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
> No, I mean in HEVCSEIContentLight. Like I said, HEVCSEIDynamicHDRPlus
> shouldn't exist.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
James Almer Jan. 4, 2019, 9:05 p.m. UTC | #7 
On 1/4/2019 2:51 PM, Mohammad Izadi wrote:
> I did that before and James ask me to copy. So now, James are you OK with
> that?
Your first patch allocated a second AVDynamicHDRPlus and memcpy'd
everything to the new one every frame. Rostislav suggestion is to
allocate a single AVDynamicHDRPlus, make an AVBufferRef out of it, then
attach a new reference to it as side data on every frame.

And yes, I'm ok with that if the SEI is valid for every frame after it.
It will be much faster and cleaner.

> --
> Best,
> Mohammad
> 
> 
> On Thu, Jan 3, 2019 at 5:08 PM Rostislav Pehlivanov <atomnuker@gmail.com>
> wrote:
> 
>> On Thu, 3 Jan 2019 at 20:13, Mohammad Izadi <moh.izadi@gmail.com> wrote:
>>
>>>
>>>  /**
>>> @@ -94,6 +95,50 @@ typedef struct HEVCSEIMasteringDisplay {
>>>      uint32_t min_luminance;
>>>  } HEVCSEIMasteringDisplay;
>>>
>>> +typedef struct HEVCSEIDynamicHDRPlus{
>>> +    int present;
>>> +    uint8_t itu_t_t35_country_code;
>>> +    uint8_t application_version;
>>> +    uint8_t num_windows;
>>> +    struct {
>>> +        AVRational window_upper_left_corner_x;
>>> +        AVRational window_upper_left_corner_y;
>>> +        AVRational window_lower_right_corner_x;
>>> +        AVRational window_lower_right_corner_y;
>>> +        uint16_t center_of_ellipse_x;
>>> +        uint16_t center_of_ellipse_y;
>>> +        uint8_t rotation_angle;
>>> +        uint16_t semimajor_axis_internal_ellipse;
>>> +        uint16_t semimajor_axis_external_ellipse;
>>> +        uint16_t semiminor_axis_external_ellipse;
>>> +        uint8_t overlap_process_option;
>>> +        AVRational maxscl[3];
>>> +        AVRational average_maxrgb;
>>> +        uint8_t num_distribution_maxrgb_percentiles;
>>> +        struct {
>>> +            uint8_t percentage;
>>> +            AVRational percentile;
>>> +        } distribution_maxrgb[15];
>>> +        AVRational fraction_bright_pixels;
>>> +        uint8_t tone_mapping_flag;
>>> +        AVRational knee_point_x;
>>> +        AVRational knee_point_y;
>>> +        uint8_t num_bezier_curve_anchors;
>>> +        AVRational bezier_curve_anchors[15];
>>> +        uint8_t color_saturation_mapping_flag;
>>> +        AVRational color_saturation_weight;
>>> +    } params[3];
>>> +    AVRational targeted_system_display_maximum_luminance;
>>> +    uint8_t targeted_system_display_actual_peak_luminance_flag;
>>> +    uint8_t num_rows_targeted_system_display_actual_peak_luminance;
>>> +    uint8_t num_cols_targeted_system_display_actual_peak_luminance;
>>> +    AVRational targeted_system_display_actual_peak_luminance[25][25];
>>> +    uint8_t mastering_display_actual_peak_luminance_flag;
>>> +    uint8_t num_rows_mastering_display_actual_peak_luminance;
>>> +    uint8_t num_cols_mastering_display_actual_peak_luminance;
>>> +    AVRational mastering_display_actual_peak_luminance[25][25];
>>> +} HEVCSEIDynamicHDRPlus;
>>> +
>>>
>>
>> There's no reason to create a new struct for this if all you're going to do
>> is to copy it over and over to new frames.
>> What you can do is this: on every SEI containing this thing just allocate a
>> AVDynamicHDRPlus struct via av_dynamic_hdr_plus_alloc, wrap it as an
>> AVBufferRef via av_buffer_create, populate it, put it as a pointer in
>> HEVCSEIContentLight and then on every frame just add it to the frame via
>> av_frame_new_side_data_from_buf. If a new SEI is received unref your
>> pointer in HEVCSEIDynamicHDRPlus and repeat by allocating a new struct,
>> wrapping it as a buffer, populating it, etc.
>> I figure the only reason this wasn't done with other metadata was because
>> they were much smaller and because av_frame_new_side_data_from_buf didn't
>> exist until recently.
>>
>>
>> +            av_log(s->avctx, AV_LOG_DEBUG, ")");
>>> +            if (metadata->params[w].color_saturation_mapping_flag) {
>>> +                av_log(s->avctx, AV_LOG_DEBUG,
>>> +                       " color_saturation_weight=%5.4f",
>>> +
>>>  av_q2d(metadata->params[w].color_saturation_weight));
>>> +            }
>>> +            av_log(s->avctx, AV_LOG_DEBUG, "}\n");
>>> +        }
>>> +        av_log(s->avctx, AV_LOG_DEBUG,
>>> +               "} End of HDR10+ (SMPTE 2094-40)\n");
>>> +    }
>>>
>>
>> Don't spam stuff like than in the debug log, especially not on every single
>> frame. Tools exist to print side data so just don't.
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
James Almer Jan. 4, 2019, 9:08 p.m. UTC | #8 
On 1/4/2019 3:53 PM, Rostislav Pehlivanov wrote:
> On Fri, 4 Jan 2019 at 18:12, Mohammad Izadi <moh.izadi@gmail.com> wrote:
> 
>> You mean a pointer in HEVCSEIDynamicHDRPlus, not in HEVCSEIContentLight?
>> --
>> Best,
>> Mohammad
>>
>>
>> On Thu, Jan 3, 2019 at 5:08 PM Rostislav Pehlivanov <atomnuker@gmail.com>
>> wrote:
>>
>>> On Thu, 3 Jan 2019 at 20:13, Mohammad Izadi <moh.izadi@gmail.com> wrote:
>>>
>>>>
>>>>  /**
>>>> @@ -94,6 +95,50 @@ typedef struct HEVCSEIMasteringDisplay {
>>>>      uint32_t min_luminance;
>>>>  } HEVCSEIMasteringDisplay;
>>>>
>>>> +typedef struct HEVCSEIDynamicHDRPlus{
>>>> +    int present;
>>>> +    uint8_t itu_t_t35_country_code;
>>>> +    uint8_t application_version;
>>>> +    uint8_t num_windows;
>>>> +    struct {
>>>> +        AVRational window_upper_left_corner_x;
>>>> +        AVRational window_upper_left_corner_y;
>>>> +        AVRational window_lower_right_corner_x;
>>>> +        AVRational window_lower_right_corner_y;
>>>> +        uint16_t center_of_ellipse_x;
>>>> +        uint16_t center_of_ellipse_y;
>>>> +        uint8_t rotation_angle;
>>>> +        uint16_t semimajor_axis_internal_ellipse;
>>>> +        uint16_t semimajor_axis_external_ellipse;
>>>> +        uint16_t semiminor_axis_external_ellipse;
>>>> +        uint8_t overlap_process_option;
>>>> +        AVRational maxscl[3];
>>>> +        AVRational average_maxrgb;
>>>> +        uint8_t num_distribution_maxrgb_percentiles;
>>>> +        struct {
>>>> +            uint8_t percentage;
>>>> +            AVRational percentile;
>>>> +        } distribution_maxrgb[15];
>>>> +        AVRational fraction_bright_pixels;
>>>> +        uint8_t tone_mapping_flag;
>>>> +        AVRational knee_point_x;
>>>> +        AVRational knee_point_y;
>>>> +        uint8_t num_bezier_curve_anchors;
>>>> +        AVRational bezier_curve_anchors[15];
>>>> +        uint8_t color_saturation_mapping_flag;
>>>> +        AVRational color_saturation_weight;
>>>> +    } params[3];
>>>> +    AVRational targeted_system_display_maximum_luminance;
>>>> +    uint8_t targeted_system_display_actual_peak_luminance_flag;
>>>> +    uint8_t num_rows_targeted_system_display_actual_peak_luminance;
>>>> +    uint8_t num_cols_targeted_system_display_actual_peak_luminance;
>>>> +    AVRational targeted_system_display_actual_peak_luminance[25][25];
>>>> +    uint8_t mastering_display_actual_peak_luminance_flag;
>>>> +    uint8_t num_rows_mastering_display_actual_peak_luminance;
>>>> +    uint8_t num_cols_mastering_display_actual_peak_luminance;
>>>> +    AVRational mastering_display_actual_peak_luminance[25][25];
>>>> +} HEVCSEIDynamicHDRPlus;
>>>> +
>>>>
>>>
>>> There's no reason to create a new struct for this if all you're going to
>> do
>>> is to copy it over and over to new frames.
>>> What you can do is this: on every SEI containing this thing just
>> allocate a
>>> AVDynamicHDRPlus struct via av_dynamic_hdr_plus_alloc, wrap it as an
>>> AVBufferRef via av_buffer_create, populate it, put it as a pointer in
>>> HEVCSEIContentLight and then on every frame just add it to the frame via
>>> av_frame_new_side_data_from_buf. If a new SEI is received unref your
>>> pointer in HEVCSEIDynamicHDRPlus and repeat by allocating a new struct,
>>> wrapping it as a buffer, populating it, etc.
>>> I figure the only reason this wasn't done with other metadata was because
>>> they were much smaller and because av_frame_new_side_data_from_buf didn't
>>> exist until recently.
>>>
>>>
>>> +            av_log(s->avctx, AV_LOG_DEBUG, ")");
>>>> +            if (metadata->params[w].color_saturation_mapping_flag) {
>>>> +                av_log(s->avctx, AV_LOG_DEBUG,
>>>> +                       " color_saturation_weight=%5.4f",
>>>> +
>>>>  av_q2d(metadata->params[w].color_saturation_weight));
>>>> +            }
>>>> +            av_log(s->avctx, AV_LOG_DEBUG, "}\n");
>>>> +        }
>>>> +        av_log(s->avctx, AV_LOG_DEBUG,
>>>> +               "} End of HDR10+ (SMPTE 2094-40)\n");
>>>> +    }
>>>>
>>>
>>> Don't spam stuff like than in the debug log, especially not on every
>> single
>>> frame. Tools exist to print side data so just don't.
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> 
> No, I mean in HEVCSEIContentLight. Like I said, HEVCSEIDynamicHDRPlus
> shouldn't exist.

By "HEVCSEIDynamicHDRPlus shouldn't exist" you mean it shouldn't contain
a copy of every bitstream field in the SEI?

Content Light and Dynamic HDR10+ are two different SEI types. There's no
reason to merge them into a single struct within the HEVC decoder.
Rostislav Pehlivanov Jan. 4, 2019, 10:51 p.m. UTC | #9 
On Fri, 4 Jan 2019 at 21:08, James Almer <jamrial@gmail.com> wrote:

> On 1/4/2019 3:53 PM, Rostislav Pehlivanov wrote:
> > On Fri, 4 Jan 2019 at 18:12, Mohammad Izadi <moh.izadi@gmail.com> wrote:
> >
> >> You mean a pointer in HEVCSEIDynamicHDRPlus, not in HEVCSEIContentLight?
> >> --
> >> Best,
> >> Mohammad
> >>
> >>
> >> On Thu, Jan 3, 2019 at 5:08 PM Rostislav Pehlivanov <
> atomnuker@gmail.com>
> >> wrote:
> >>
> >>> On Thu, 3 Jan 2019 at 20:13, Mohammad Izadi <moh.izadi@gmail.com>
> wrote:
> >>>
> >>>>
> >>>>  /**
> >>>> @@ -94,6 +95,50 @@ typedef struct HEVCSEIMasteringDisplay {
> >>>>      uint32_t min_luminance;
> >>>>  } HEVCSEIMasteringDisplay;
> >>>>
> >>>> +typedef struct HEVCSEIDynamicHDRPlus{
> >>>> +    int present;
> >>>> +    uint8_t itu_t_t35_country_code;
> >>>> +    uint8_t application_version;
> >>>> +    uint8_t num_windows;
> >>>> +    struct {
> >>>> +        AVRational window_upper_left_corner_x;
> >>>> +        AVRational window_upper_left_corner_y;
> >>>> +        AVRational window_lower_right_corner_x;
> >>>> +        AVRational window_lower_right_corner_y;
> >>>> +        uint16_t center_of_ellipse_x;
> >>>> +        uint16_t center_of_ellipse_y;
> >>>> +        uint8_t rotation_angle;
> >>>> +        uint16_t semimajor_axis_internal_ellipse;
> >>>> +        uint16_t semimajor_axis_external_ellipse;
> >>>> +        uint16_t semiminor_axis_external_ellipse;
> >>>> +        uint8_t overlap_process_option;
> >>>> +        AVRational maxscl[3];
> >>>> +        AVRational average_maxrgb;
> >>>> +        uint8_t num_distribution_maxrgb_percentiles;
> >>>> +        struct {
> >>>> +            uint8_t percentage;
> >>>> +            AVRational percentile;
> >>>> +        } distribution_maxrgb[15];
> >>>> +        AVRational fraction_bright_pixels;
> >>>> +        uint8_t tone_mapping_flag;
> >>>> +        AVRational knee_point_x;
> >>>> +        AVRational knee_point_y;
> >>>> +        uint8_t num_bezier_curve_anchors;
> >>>> +        AVRational bezier_curve_anchors[15];
> >>>> +        uint8_t color_saturation_mapping_flag;
> >>>> +        AVRational color_saturation_weight;
> >>>> +    } params[3];
> >>>> +    AVRational targeted_system_display_maximum_luminance;
> >>>> +    uint8_t targeted_system_display_actual_peak_luminance_flag;
> >>>> +    uint8_t num_rows_targeted_system_display_actual_peak_luminance;
> >>>> +    uint8_t num_cols_targeted_system_display_actual_peak_luminance;
> >>>> +    AVRational targeted_system_display_actual_peak_luminance[25][25];
> >>>> +    uint8_t mastering_display_actual_peak_luminance_flag;
> >>>> +    uint8_t num_rows_mastering_display_actual_peak_luminance;
> >>>> +    uint8_t num_cols_mastering_display_actual_peak_luminance;
> >>>> +    AVRational mastering_display_actual_peak_luminance[25][25];
> >>>> +} HEVCSEIDynamicHDRPlus;
> >>>> +
> >>>>
> >>>
> >>> There's no reason to create a new struct for this if all you're going
> to
> >> do
> >>> is to copy it over and over to new frames.
> >>> What you can do is this: on every SEI containing this thing just
> >> allocate a
> >>> AVDynamicHDRPlus struct via av_dynamic_hdr_plus_alloc, wrap it as an
> >>> AVBufferRef via av_buffer_create, populate it, put it as a pointer in
> >>> HEVCSEIContentLight and then on every frame just add it to the frame
> via
> >>> av_frame_new_side_data_from_buf. If a new SEI is received unref your
> >>> pointer in HEVCSEIDynamicHDRPlus and repeat by allocating a new struct,
> >>> wrapping it as a buffer, populating it, etc.
> >>> I figure the only reason this wasn't done with other metadata was
> because
> >>> they were much smaller and because av_frame_new_side_data_from_buf
> didn't
> >>> exist until recently.
> >>>
> >>>
> >>> +            av_log(s->avctx, AV_LOG_DEBUG, ")");
> >>>> +            if (metadata->params[w].color_saturation_mapping_flag) {
> >>>> +                av_log(s->avctx, AV_LOG_DEBUG,
> >>>> +                       " color_saturation_weight=%5.4f",
> >>>> +
> >>>>  av_q2d(metadata->params[w].color_saturation_weight));
> >>>> +            }
> >>>> +            av_log(s->avctx, AV_LOG_DEBUG, "}\n");
> >>>> +        }
> >>>> +        av_log(s->avctx, AV_LOG_DEBUG,
> >>>> +               "} End of HDR10+ (SMPTE 2094-40)\n");
> >>>> +    }
> >>>>
> >>>
> >>> Don't spam stuff like than in the debug log, especially not on every
> >> single
> >>> frame. Tools exist to print side data so just don't.
> >>> _______________________________________________
> >>> ffmpeg-devel mailing list
> >>> ffmpeg-devel@ffmpeg.org
> >>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>>
> >> _______________________________________________
> >> ffmpeg-devel mailing list
> >> ffmpeg-devel@ffmpeg.org
> >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> >
> > No, I mean in HEVCSEIContentLight. Like I said, HEVCSEIDynamicHDRPlus
> > shouldn't exist.
>
> By "HEVCSEIDynamicHDRPlus shouldn't exist" you mean it shouldn't contain
> a copy of every bitstream field in the SEI?
>
> Content Light and Dynamic HDR10+ are two different SEI types. There's no
> reason to merge them into a single struct within the HEVC decoder.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


I'm not asking you to merge them, its just that you kept the 10plus state
there so it would make sense to replace that state (struct) with the
avbufferref.
In reailty if you think there's a better place to put the avbufferref state
you'd attach to avframes you should put it there.
And yes, HEVCSEIDynamicHDRPlus shouldn't exist, there's already a full
struct in libavutil, so all you need to do is like I said, allocate it,
populate it, store it somewhere and ref it to new frames.
No need to create a new structure.
Mohammad Izadi Jan. 4, 2019, 11:02 p.m. UTC | #10 
Hi Rostislav,

It seems there is a bit miscommunication here. I dont want to create a full
struct. However, as James mentioned too,  Content Light and Dynamic HDR10+
are two different SEI types. There's no
reason to merge them into a single struct within the HEVC decoder. I mean
to create the following struct rather than merging it to ContentLight:

typedef struct HEVCSEIDynamicHDRPlus{
    int present;
    AVBufferRef info;
} HEVCSEIDynamicHDRPlus;

--
Best,
Mohammad


On Fri, Jan 4, 2019 at 2:51 PM Rostislav Pehlivanov <atomnuker@gmail.com>
wrote:

> On Fri, 4 Jan 2019 at 21:08, James Almer <jamrial@gmail.com> wrote:
>
> > On 1/4/2019 3:53 PM, Rostislav Pehlivanov wrote:
> > > On Fri, 4 Jan 2019 at 18:12, Mohammad Izadi <moh.izadi@gmail.com>
> wrote:
> > >
> > >> You mean a pointer in HEVCSEIDynamicHDRPlus, not in
> HEVCSEIContentLight?
> > >> --
> > >> Best,
> > >> Mohammad
> > >>
> > >>
> > >> On Thu, Jan 3, 2019 at 5:08 PM Rostislav Pehlivanov <
> > atomnuker@gmail.com>
> > >> wrote:
> > >>
> > >>> On Thu, 3 Jan 2019 at 20:13, Mohammad Izadi <moh.izadi@gmail.com>
> > wrote:
> > >>>
> > >>>>
> > >>>>  /**
> > >>>> @@ -94,6 +95,50 @@ typedef struct HEVCSEIMasteringDisplay {
> > >>>>      uint32_t min_luminance;
> > >>>>  } HEVCSEIMasteringDisplay;
> > >>>>
> > >>>> +typedef struct HEVCSEIDynamicHDRPlus{
> > >>>> +    int present;
> > >>>> +    uint8_t itu_t_t35_country_code;
> > >>>> +    uint8_t application_version;
> > >>>> +    uint8_t num_windows;
> > >>>> +    struct {
> > >>>> +        AVRational window_upper_left_corner_x;
> > >>>> +        AVRational window_upper_left_corner_y;
> > >>>> +        AVRational window_lower_right_corner_x;
> > >>>> +        AVRational window_lower_right_corner_y;
> > >>>> +        uint16_t center_of_ellipse_x;
> > >>>> +        uint16_t center_of_ellipse_y;
> > >>>> +        uint8_t rotation_angle;
> > >>>> +        uint16_t semimajor_axis_internal_ellipse;
> > >>>> +        uint16_t semimajor_axis_external_ellipse;
> > >>>> +        uint16_t semiminor_axis_external_ellipse;
> > >>>> +        uint8_t overlap_process_option;
> > >>>> +        AVRational maxscl[3];
> > >>>> +        AVRational average_maxrgb;
> > >>>> +        uint8_t num_distribution_maxrgb_percentiles;
> > >>>> +        struct {
> > >>>> +            uint8_t percentage;
> > >>>> +            AVRational percentile;
> > >>>> +        } distribution_maxrgb[15];
> > >>>> +        AVRational fraction_bright_pixels;
> > >>>> +        uint8_t tone_mapping_flag;
> > >>>> +        AVRational knee_point_x;
> > >>>> +        AVRational knee_point_y;
> > >>>> +        uint8_t num_bezier_curve_anchors;
> > >>>> +        AVRational bezier_curve_anchors[15];
> > >>>> +        uint8_t color_saturation_mapping_flag;
> > >>>> +        AVRational color_saturation_weight;
> > >>>> +    } params[3];
> > >>>> +    AVRational targeted_system_display_maximum_luminance;
> > >>>> +    uint8_t targeted_system_display_actual_peak_luminance_flag;
> > >>>> +    uint8_t num_rows_targeted_system_display_actual_peak_luminance;
> > >>>> +    uint8_t num_cols_targeted_system_display_actual_peak_luminance;
> > >>>> +    AVRational
> targeted_system_display_actual_peak_luminance[25][25];
> > >>>> +    uint8_t mastering_display_actual_peak_luminance_flag;
> > >>>> +    uint8_t num_rows_mastering_display_actual_peak_luminance;
> > >>>> +    uint8_t num_cols_mastering_display_actual_peak_luminance;
> > >>>> +    AVRational mastering_display_actual_peak_luminance[25][25];
> > >>>> +} HEVCSEIDynamicHDRPlus;
> > >>>> +
> > >>>>
> > >>>
> > >>> There's no reason to create a new struct for this if all you're going
> > to
> > >> do
> > >>> is to copy it over and over to new frames.
> > >>> What you can do is this: on every SEI containing this thing just
> > >> allocate a
> > >>> AVDynamicHDRPlus struct via av_dynamic_hdr_plus_alloc, wrap it as an
> > >>> AVBufferRef via av_buffer_create, populate it, put it as a pointer in
> > >>> HEVCSEIContentLight and then on every frame just add it to the frame
> > via
> > >>> av_frame_new_side_data_from_buf. If a new SEI is received unref your
> > >>> pointer in HEVCSEIDynamicHDRPlus and repeat by allocating a new
> struct,
> > >>> wrapping it as a buffer, populating it, etc.
> > >>> I figure the only reason this wasn't done with other metadata was
> > because
> > >>> they were much smaller and because av_frame_new_side_data_from_buf
> > didn't
> > >>> exist until recently.
> > >>>
> > >>>
> > >>> +            av_log(s->avctx, AV_LOG_DEBUG, ")");
> > >>>> +            if (metadata->params[w].color_saturation_mapping_flag)
> {
> > >>>> +                av_log(s->avctx, AV_LOG_DEBUG,
> > >>>> +                       " color_saturation_weight=%5.4f",
> > >>>> +
> > >>>>  av_q2d(metadata->params[w].color_saturation_weight));
> > >>>> +            }
> > >>>> +            av_log(s->avctx, AV_LOG_DEBUG, "}\n");
> > >>>> +        }
> > >>>> +        av_log(s->avctx, AV_LOG_DEBUG,
> > >>>> +               "} End of HDR10+ (SMPTE 2094-40)\n");
> > >>>> +    }
> > >>>>
> > >>>
> > >>> Don't spam stuff like than in the debug log, especially not on every
> > >> single
> > >>> frame. Tools exist to print side data so just don't.
> > >>> _______________________________________________
> > >>> ffmpeg-devel mailing list
> > >>> ffmpeg-devel@ffmpeg.org
> > >>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >>>
> > >> _______________________________________________
> > >> ffmpeg-devel mailing list
> > >> ffmpeg-devel@ffmpeg.org
> > >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >
> > >
> > > No, I mean in HEVCSEIContentLight. Like I said, HEVCSEIDynamicHDRPlus
> > > shouldn't exist.
> >
> > By "HEVCSEIDynamicHDRPlus shouldn't exist" you mean it shouldn't contain
> > a copy of every bitstream field in the SEI?
> >
> > Content Light and Dynamic HDR10+ are two different SEI types. There's no
> > reason to merge them into a single struct within the HEVC decoder.
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
> I'm not asking you to merge them, its just that you kept the 10plus state
> there so it would make sense to replace that state (struct) with the
> avbufferref.
> In reailty if you think there's a better place to put the avbufferref state
> you'd attach to avframes you should put it there.
> And yes, HEVCSEIDynamicHDRPlus shouldn't exist, there's already a full
> struct in libavutil, so all you need to do is like I said, allocate it,
> populate it, store it somewhere and ref it to new frames.
> No need to create a new structure.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
James Almer Jan. 4, 2019, 11:03 p.m. UTC | #11 
On 1/4/2019 7:51 PM, Rostislav Pehlivanov wrote:
> On Fri, 4 Jan 2019 at 21:08, James Almer <jamrial@gmail.com> wrote:
> 
>> On 1/4/2019 3:53 PM, Rostislav Pehlivanov wrote:
>>> On Fri, 4 Jan 2019 at 18:12, Mohammad Izadi <moh.izadi@gmail.com> wrote:
>>>
>>>> You mean a pointer in HEVCSEIDynamicHDRPlus, not in HEVCSEIContentLight?
>>>> --
>>>> Best,
>>>> Mohammad
>>>>
>>>>
>>>> On Thu, Jan 3, 2019 at 5:08 PM Rostislav Pehlivanov <
>> atomnuker@gmail.com>
>>>> wrote:
>>>>
>>>>> On Thu, 3 Jan 2019 at 20:13, Mohammad Izadi <moh.izadi@gmail.com>
>> wrote:
>>>>>
>>>>>>
>>>>>>  /**
>>>>>> @@ -94,6 +95,50 @@ typedef struct HEVCSEIMasteringDisplay {
>>>>>>      uint32_t min_luminance;
>>>>>>  } HEVCSEIMasteringDisplay;
>>>>>>
>>>>>> +typedef struct HEVCSEIDynamicHDRPlus{
>>>>>> +    int present;
>>>>>> +    uint8_t itu_t_t35_country_code;
>>>>>> +    uint8_t application_version;
>>>>>> +    uint8_t num_windows;
>>>>>> +    struct {
>>>>>> +        AVRational window_upper_left_corner_x;
>>>>>> +        AVRational window_upper_left_corner_y;
>>>>>> +        AVRational window_lower_right_corner_x;
>>>>>> +        AVRational window_lower_right_corner_y;
>>>>>> +        uint16_t center_of_ellipse_x;
>>>>>> +        uint16_t center_of_ellipse_y;
>>>>>> +        uint8_t rotation_angle;
>>>>>> +        uint16_t semimajor_axis_internal_ellipse;
>>>>>> +        uint16_t semimajor_axis_external_ellipse;
>>>>>> +        uint16_t semiminor_axis_external_ellipse;
>>>>>> +        uint8_t overlap_process_option;
>>>>>> +        AVRational maxscl[3];
>>>>>> +        AVRational average_maxrgb;
>>>>>> +        uint8_t num_distribution_maxrgb_percentiles;
>>>>>> +        struct {
>>>>>> +            uint8_t percentage;
>>>>>> +            AVRational percentile;
>>>>>> +        } distribution_maxrgb[15];
>>>>>> +        AVRational fraction_bright_pixels;
>>>>>> +        uint8_t tone_mapping_flag;
>>>>>> +        AVRational knee_point_x;
>>>>>> +        AVRational knee_point_y;
>>>>>> +        uint8_t num_bezier_curve_anchors;
>>>>>> +        AVRational bezier_curve_anchors[15];
>>>>>> +        uint8_t color_saturation_mapping_flag;
>>>>>> +        AVRational color_saturation_weight;
>>>>>> +    } params[3];
>>>>>> +    AVRational targeted_system_display_maximum_luminance;
>>>>>> +    uint8_t targeted_system_display_actual_peak_luminance_flag;
>>>>>> +    uint8_t num_rows_targeted_system_display_actual_peak_luminance;
>>>>>> +    uint8_t num_cols_targeted_system_display_actual_peak_luminance;
>>>>>> +    AVRational targeted_system_display_actual_peak_luminance[25][25];
>>>>>> +    uint8_t mastering_display_actual_peak_luminance_flag;
>>>>>> +    uint8_t num_rows_mastering_display_actual_peak_luminance;
>>>>>> +    uint8_t num_cols_mastering_display_actual_peak_luminance;
>>>>>> +    AVRational mastering_display_actual_peak_luminance[25][25];
>>>>>> +} HEVCSEIDynamicHDRPlus;
>>>>>> +
>>>>>>
>>>>>
>>>>> There's no reason to create a new struct for this if all you're going
>> to
>>>> do
>>>>> is to copy it over and over to new frames.
>>>>> What you can do is this: on every SEI containing this thing just
>>>> allocate a
>>>>> AVDynamicHDRPlus struct via av_dynamic_hdr_plus_alloc, wrap it as an
>>>>> AVBufferRef via av_buffer_create, populate it, put it as a pointer in
>>>>> HEVCSEIContentLight and then on every frame just add it to the frame
>> via
>>>>> av_frame_new_side_data_from_buf. If a new SEI is received unref your
>>>>> pointer in HEVCSEIDynamicHDRPlus and repeat by allocating a new struct,
>>>>> wrapping it as a buffer, populating it, etc.
>>>>> I figure the only reason this wasn't done with other metadata was
>> because
>>>>> they were much smaller and because av_frame_new_side_data_from_buf
>> didn't
>>>>> exist until recently.
>>>>>
>>>>>
>>>>> +            av_log(s->avctx, AV_LOG_DEBUG, ")");
>>>>>> +            if (metadata->params[w].color_saturation_mapping_flag) {
>>>>>> +                av_log(s->avctx, AV_LOG_DEBUG,
>>>>>> +                       " color_saturation_weight=%5.4f",
>>>>>> +
>>>>>>  av_q2d(metadata->params[w].color_saturation_weight));
>>>>>> +            }
>>>>>> +            av_log(s->avctx, AV_LOG_DEBUG, "}\n");
>>>>>> +        }
>>>>>> +        av_log(s->avctx, AV_LOG_DEBUG,
>>>>>> +               "} End of HDR10+ (SMPTE 2094-40)\n");
>>>>>> +    }
>>>>>>
>>>>>
>>>>> Don't spam stuff like than in the debug log, especially not on every
>>>> single
>>>>> frame. Tools exist to print side data so just don't.
>>>>> _______________________________________________
>>>>> ffmpeg-devel mailing list
>>>>> ffmpeg-devel@ffmpeg.org
>>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>>>
>>>> _______________________________________________
>>>> ffmpeg-devel mailing list
>>>> ffmpeg-devel@ffmpeg.org
>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>>
>>> No, I mean in HEVCSEIContentLight. Like I said, HEVCSEIDynamicHDRPlus
>>> shouldn't exist.
>>
>> By "HEVCSEIDynamicHDRPlus shouldn't exist" you mean it shouldn't contain
>> a copy of every bitstream field in the SEI?
>>
>> Content Light and Dynamic HDR10+ are two different SEI types. There's no
>> reason to merge them into a single struct within the HEVC decoder.
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> 
> I'm not asking you to merge them, its just that you kept the 10plus state
> there so it would make sense to replace that state (struct) with the
> avbufferref.
> In reailty if you think there's a better place to put the avbufferref state
> you'd attach to avframes you should put it there.
> And yes, HEVCSEIDynamicHDRPlus shouldn't exist, there's already a full
> struct in libavutil, so all you need to do is like I said, allocate it,
> populate it, store it somewhere and ref it to new frames.
> No need to create a new structure.

A HEVCSEIDynamicHDRPlus struct with only the AVBufferRef pointer and a
present field is ok, IMO. No need to add all the bitstream fields there
as per your suggestion.

I don't like dumping random fields directly in HEVCSEI. Lets keep things
clean looking.
Mohammad Izadi Jan. 4, 2019, 11:19 p.m. UTC | #12 
Thanks James.
--
Best,
Mohammad


On Fri, Jan 4, 2019 at 3:03 PM James Almer <jamrial@gmail.com> wrote:

> On 1/4/2019 7:51 PM, Rostislav Pehlivanov wrote:
> > On Fri, 4 Jan 2019 at 21:08, James Almer <jamrial@gmail.com> wrote:
> >
> >> On 1/4/2019 3:53 PM, Rostislav Pehlivanov wrote:
> >>> On Fri, 4 Jan 2019 at 18:12, Mohammad Izadi <moh.izadi@gmail.com>
> wrote:
> >>>
> >>>> You mean a pointer in HEVCSEIDynamicHDRPlus, not in
> HEVCSEIContentLight?
> >>>> --
> >>>> Best,
> >>>> Mohammad
> >>>>
> >>>>
> >>>> On Thu, Jan 3, 2019 at 5:08 PM Rostislav Pehlivanov <
> >> atomnuker@gmail.com>
> >>>> wrote:
> >>>>
> >>>>> On Thu, 3 Jan 2019 at 20:13, Mohammad Izadi <moh.izadi@gmail.com>
> >> wrote:
> >>>>>
> >>>>>>
> >>>>>>  /**
> >>>>>> @@ -94,6 +95,50 @@ typedef struct HEVCSEIMasteringDisplay {
> >>>>>>      uint32_t min_luminance;
> >>>>>>  } HEVCSEIMasteringDisplay;
> >>>>>>
> >>>>>> +typedef struct HEVCSEIDynamicHDRPlus{
> >>>>>> +    int present;
> >>>>>> +    uint8_t itu_t_t35_country_code;
> >>>>>> +    uint8_t application_version;
> >>>>>> +    uint8_t num_windows;
> >>>>>> +    struct {
> >>>>>> +        AVRational window_upper_left_corner_x;
> >>>>>> +        AVRational window_upper_left_corner_y;
> >>>>>> +        AVRational window_lower_right_corner_x;
> >>>>>> +        AVRational window_lower_right_corner_y;
> >>>>>> +        uint16_t center_of_ellipse_x;
> >>>>>> +        uint16_t center_of_ellipse_y;
> >>>>>> +        uint8_t rotation_angle;
> >>>>>> +        uint16_t semimajor_axis_internal_ellipse;
> >>>>>> +        uint16_t semimajor_axis_external_ellipse;
> >>>>>> +        uint16_t semiminor_axis_external_ellipse;
> >>>>>> +        uint8_t overlap_process_option;
> >>>>>> +        AVRational maxscl[3];
> >>>>>> +        AVRational average_maxrgb;
> >>>>>> +        uint8_t num_distribution_maxrgb_percentiles;
> >>>>>> +        struct {
> >>>>>> +            uint8_t percentage;
> >>>>>> +            AVRational percentile;
> >>>>>> +        } distribution_maxrgb[15];
> >>>>>> +        AVRational fraction_bright_pixels;
> >>>>>> +        uint8_t tone_mapping_flag;
> >>>>>> +        AVRational knee_point_x;
> >>>>>> +        AVRational knee_point_y;
> >>>>>> +        uint8_t num_bezier_curve_anchors;
> >>>>>> +        AVRational bezier_curve_anchors[15];
> >>>>>> +        uint8_t color_saturation_mapping_flag;
> >>>>>> +        AVRational color_saturation_weight;
> >>>>>> +    } params[3];
> >>>>>> +    AVRational targeted_system_display_maximum_luminance;
> >>>>>> +    uint8_t targeted_system_display_actual_peak_luminance_flag;
> >>>>>> +    uint8_t num_rows_targeted_system_display_actual_peak_luminance;
> >>>>>> +    uint8_t num_cols_targeted_system_display_actual_peak_luminance;
> >>>>>> +    AVRational
> targeted_system_display_actual_peak_luminance[25][25];
> >>>>>> +    uint8_t mastering_display_actual_peak_luminance_flag;
> >>>>>> +    uint8_t num_rows_mastering_display_actual_peak_luminance;
> >>>>>> +    uint8_t num_cols_mastering_display_actual_peak_luminance;
> >>>>>> +    AVRational mastering_display_actual_peak_luminance[25][25];
> >>>>>> +} HEVCSEIDynamicHDRPlus;
> >>>>>> +
> >>>>>>
> >>>>>
> >>>>> There's no reason to create a new struct for this if all you're going
> >> to
> >>>> do
> >>>>> is to copy it over and over to new frames.
> >>>>> What you can do is this: on every SEI containing this thing just
> >>>> allocate a
> >>>>> AVDynamicHDRPlus struct via av_dynamic_hdr_plus_alloc, wrap it as an
> >>>>> AVBufferRef via av_buffer_create, populate it, put it as a pointer in
> >>>>> HEVCSEIContentLight and then on every frame just add it to the frame
> >> via
> >>>>> av_frame_new_side_data_from_buf. If a new SEI is received unref your
> >>>>> pointer in HEVCSEIDynamicHDRPlus and repeat by allocating a new
> struct,
> >>>>> wrapping it as a buffer, populating it, etc.
> >>>>> I figure the only reason this wasn't done with other metadata was
> >> because
> >>>>> they were much smaller and because av_frame_new_side_data_from_buf
> >> didn't
> >>>>> exist until recently.
> >>>>>
> >>>>>
> >>>>> +            av_log(s->avctx, AV_LOG_DEBUG, ")");
> >>>>>> +            if (metadata->params[w].color_saturation_mapping_flag)
> {
> >>>>>> +                av_log(s->avctx, AV_LOG_DEBUG,
> >>>>>> +                       " color_saturation_weight=%5.4f",
> >>>>>> +
> >>>>>>  av_q2d(metadata->params[w].color_saturation_weight));
> >>>>>> +            }
> >>>>>> +            av_log(s->avctx, AV_LOG_DEBUG, "}\n");
> >>>>>> +        }
> >>>>>> +        av_log(s->avctx, AV_LOG_DEBUG,
> >>>>>> +               "} End of HDR10+ (SMPTE 2094-40)\n");
> >>>>>> +    }
> >>>>>>
> >>>>>
> >>>>> Don't spam stuff like than in the debug log, especially not on every
> >>>> single
> >>>>> frame. Tools exist to print side data so just don't.
> >>>>> _______________________________________________
> >>>>> ffmpeg-devel mailing list
> >>>>> ffmpeg-devel@ffmpeg.org
> >>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>>>>
> >>>> _______________________________________________
> >>>> ffmpeg-devel mailing list
> >>>> ffmpeg-devel@ffmpeg.org
> >>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>>
> >>>
> >>> No, I mean in HEVCSEIContentLight. Like I said, HEVCSEIDynamicHDRPlus
> >>> shouldn't exist.
> >>
> >> By "HEVCSEIDynamicHDRPlus shouldn't exist" you mean it shouldn't contain
> >> a copy of every bitstream field in the SEI?
> >>
> >> Content Light and Dynamic HDR10+ are two different SEI types. There's no
> >> reason to merge them into a single struct within the HEVC decoder.
> >> _______________________________________________
> >> ffmpeg-devel mailing list
> >> ffmpeg-devel@ffmpeg.org
> >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> >
> > I'm not asking you to merge them, its just that you kept the 10plus state
> > there so it would make sense to replace that state (struct) with the
> > avbufferref.
> > In reailty if you think there's a better place to put the avbufferref
> state
> > you'd attach to avframes you should put it there.
> > And yes, HEVCSEIDynamicHDRPlus shouldn't exist, there's already a full
> > struct in libavutil, so all you need to do is like I said, allocate it,
> > populate it, store it somewhere and ref it to new frames.
> > No need to create a new structure.
>
> A HEVCSEIDynamicHDRPlus struct with only the AVBufferRef pointer and a
> present field is ok, IMO. No need to add all the bitstream fields there
> as per your suggestion.
>
> I don't like dumping random fields directly in HEVCSEI. Lets keep things
> clean looking.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Rostislav Pehlivanov Jan. 5, 2019, 2:45 a.m. UTC | #13 
On Fri, 4 Jan 2019 at 23:19, Mohammad Izadi <moh.izadi@gmail.com> wrote:

> Thanks James.
> --
> Best,
> Mohammad
>
>
> On Fri, Jan 4, 2019 at 3:03 PM James Almer <jamrial@gmail.com> wrote:
>
> > On 1/4/2019 7:51 PM, Rostislav Pehlivanov wrote:
> > > On Fri, 4 Jan 2019 at 21:08, James Almer <jamrial@gmail.com> wrote:
> > >
> > >> On 1/4/2019 3:53 PM, Rostislav Pehlivanov wrote:
> > >>> On Fri, 4 Jan 2019 at 18:12, Mohammad Izadi <moh.izadi@gmail.com>
> > wrote:
> > >>>
> > >>>> You mean a pointer in HEVCSEIDynamicHDRPlus, not in
> > HEVCSEIContentLight?
> > >>>> --
> > >>>> Best,
> > >>>> Mohammad
> > >>>>
> > >>>>
> > >>>> On Thu, Jan 3, 2019 at 5:08 PM Rostislav Pehlivanov <
> > >> atomnuker@gmail.com>
> > >>>> wrote:
> > >>>>
> > >>>>> On Thu, 3 Jan 2019 at 20:13, Mohammad Izadi <moh.izadi@gmail.com>
> > >> wrote:
> > >>>>>
> > >>>>>>
> > >>>>>>  /**
> > >>>>>> @@ -94,6 +95,50 @@ typedef struct HEVCSEIMasteringDisplay {
> > >>>>>>      uint32_t min_luminance;
> > >>>>>>  } HEVCSEIMasteringDisplay;
> > >>>>>>
> > >>>>>> +typedef struct HEVCSEIDynamicHDRPlus{
> > >>>>>> +    int present;
> > >>>>>> +    uint8_t itu_t_t35_country_code;
> > >>>>>> +    uint8_t application_version;
> > >>>>>> +    uint8_t num_windows;
> > >>>>>> +    struct {
> > >>>>>> +        AVRational window_upper_left_corner_x;
> > >>>>>> +        AVRational window_upper_left_corner_y;
> > >>>>>> +        AVRational window_lower_right_corner_x;
> > >>>>>> +        AVRational window_lower_right_corner_y;
> > >>>>>> +        uint16_t center_of_ellipse_x;
> > >>>>>> +        uint16_t center_of_ellipse_y;
> > >>>>>> +        uint8_t rotation_angle;
> > >>>>>> +        uint16_t semimajor_axis_internal_ellipse;
> > >>>>>> +        uint16_t semimajor_axis_external_ellipse;
> > >>>>>> +        uint16_t semiminor_axis_external_ellipse;
> > >>>>>> +        uint8_t overlap_process_option;
> > >>>>>> +        AVRational maxscl[3];
> > >>>>>> +        AVRational average_maxrgb;
> > >>>>>> +        uint8_t num_distribution_maxrgb_percentiles;
> > >>>>>> +        struct {
> > >>>>>> +            uint8_t percentage;
> > >>>>>> +            AVRational percentile;
> > >>>>>> +        } distribution_maxrgb[15];
> > >>>>>> +        AVRational fraction_bright_pixels;
> > >>>>>> +        uint8_t tone_mapping_flag;
> > >>>>>> +        AVRational knee_point_x;
> > >>>>>> +        AVRational knee_point_y;
> > >>>>>> +        uint8_t num_bezier_curve_anchors;
> > >>>>>> +        AVRational bezier_curve_anchors[15];
> > >>>>>> +        uint8_t color_saturation_mapping_flag;
> > >>>>>> +        AVRational color_saturation_weight;
> > >>>>>> +    } params[3];
> > >>>>>> +    AVRational targeted_system_display_maximum_luminance;
> > >>>>>> +    uint8_t targeted_system_display_actual_peak_luminance_flag;
> > >>>>>> +    uint8_t
> num_rows_targeted_system_display_actual_peak_luminance;
> > >>>>>> +    uint8_t
> num_cols_targeted_system_display_actual_peak_luminance;
> > >>>>>> +    AVRational
> > targeted_system_display_actual_peak_luminance[25][25];
> > >>>>>> +    uint8_t mastering_display_actual_peak_luminance_flag;
> > >>>>>> +    uint8_t num_rows_mastering_display_actual_peak_luminance;
> > >>>>>> +    uint8_t num_cols_mastering_display_actual_peak_luminance;
> > >>>>>> +    AVRational mastering_display_actual_peak_luminance[25][25];
> > >>>>>> +} HEVCSEIDynamicHDRPlus;
> > >>>>>> +
> > >>>>>>
> > >>>>>
> > >>>>> There's no reason to create a new struct for this if all you're
> going
> > >> to
> > >>>> do
> > >>>>> is to copy it over and over to new frames.
> > >>>>> What you can do is this: on every SEI containing this thing just
> > >>>> allocate a
> > >>>>> AVDynamicHDRPlus struct via av_dynamic_hdr_plus_alloc, wrap it as
> an
> > >>>>> AVBufferRef via av_buffer_create, populate it, put it as a pointer
> in
> > >>>>> HEVCSEIContentLight and then on every frame just add it to the
> frame
> > >> via
> > >>>>> av_frame_new_side_data_from_buf. If a new SEI is received unref
> your
> > >>>>> pointer in HEVCSEIDynamicHDRPlus and repeat by allocating a new
> > struct,
> > >>>>> wrapping it as a buffer, populating it, etc.
> > >>>>> I figure the only reason this wasn't done with other metadata was
> > >> because
> > >>>>> they were much smaller and because av_frame_new_side_data_from_buf
> > >> didn't
> > >>>>> exist until recently.
> > >>>>>
> > >>>>>
> > >>>>> +            av_log(s->avctx, AV_LOG_DEBUG, ")");
> > >>>>>> +            if
> (metadata->params[w].color_saturation_mapping_flag)
> > {
> > >>>>>> +                av_log(s->avctx, AV_LOG_DEBUG,
> > >>>>>> +                       " color_saturation_weight=%5.4f",
> > >>>>>> +
> > >>>>>>  av_q2d(metadata->params[w].color_saturation_weight));
> > >>>>>> +            }
> > >>>>>> +            av_log(s->avctx, AV_LOG_DEBUG, "}\n");
> > >>>>>> +        }
> > >>>>>> +        av_log(s->avctx, AV_LOG_DEBUG,
> > >>>>>> +               "} End of HDR10+ (SMPTE 2094-40)\n");
> > >>>>>> +    }
> > >>>>>>
> > >>>>>
> > >>>>> Don't spam stuff like than in the debug log, especially not on
> every
> > >>>> single
> > >>>>> frame. Tools exist to print side data so just don't.
> > >>>>> _______________________________________________
> > >>>>> ffmpeg-devel mailing list
> > >>>>> ffmpeg-devel@ffmpeg.org
> > >>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >>>>>
> > >>>> _______________________________________________
> > >>>> ffmpeg-devel mailing list
> > >>>> ffmpeg-devel@ffmpeg.org
> > >>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >>>
> > >>>
> > >>> No, I mean in HEVCSEIContentLight. Like I said, HEVCSEIDynamicHDRPlus
> > >>> shouldn't exist.
> > >>
> > >> By "HEVCSEIDynamicHDRPlus shouldn't exist" you mean it shouldn't
> contain
> > >> a copy of every bitstream field in the SEI?
> > >>
> > >> Content Light and Dynamic HDR10+ are two different SEI types. There's
> no
> > >> reason to merge them into a single struct within the HEVC decoder.
> > >> _______________________________________________
> > >> ffmpeg-devel mailing list
> > >> ffmpeg-devel@ffmpeg.org
> > >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >
> > >
> > > I'm not asking you to merge them, its just that you kept the 10plus
> state
> > > there so it would make sense to replace that state (struct) with the
> > > avbufferref.
> > > In reailty if you think there's a better place to put the avbufferref
> > state
> > > you'd attach to avframes you should put it there.
> > > And yes, HEVCSEIDynamicHDRPlus shouldn't exist, there's already a full
> > > struct in libavutil, so all you need to do is like I said, allocate it,
> > > populate it, store it somewhere and ref it to new frames.
> > > No need to create a new structure.
> >
> > A HEVCSEIDynamicHDRPlus struct with only the AVBufferRef pointer and a
> > present field is ok, IMO. No need to add all the bitstream fields there
> > as per your suggestion.
> >
> > I don't like dumping random fields directly in HEVCSEI. Lets keep things
> > clean looking.
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


There's still no need for a HEVCSEIDynamicHDRPlus struct containing a
present field and the avbufferref pointer, as the present field would be
redundant.
If you give a NULL AVBufferRef to av_buffer_ref(NULL) it'll return NULL.
Once you pass that NULL to av_frame_new_side_data_from_buf(frame, NULL)
nothing will change. Hence the present field is unneeded and that would
leave the struct with a single member, so you migh as well not have it. So
on every frame you can unconditionally do
av_frame_new_side_data_from_buf(frame, av_buffer_ref(buffer)) and it'll
work.

If you still want to have a struct, you can go for it, it'll just be
optimized out, I don't care, just keep in mind a present field would be
pointless.
James Almer Jan. 5, 2019, 3:05 a.m. UTC | #14 
On 1/4/2019 11:45 PM, Rostislav Pehlivanov wrote:
> On Fri, 4 Jan 2019 at 23:19, Mohammad Izadi <moh.izadi@gmail.com> wrote:
> 
>> Thanks James.
>> --
>> Best,
>> Mohammad
>>
>>
>> On Fri, Jan 4, 2019 at 3:03 PM James Almer <jamrial@gmail.com> wrote:
>>
>>> On 1/4/2019 7:51 PM, Rostislav Pehlivanov wrote:
>>>> On Fri, 4 Jan 2019 at 21:08, James Almer <jamrial@gmail.com> wrote:
>>>>
>>>>> On 1/4/2019 3:53 PM, Rostislav Pehlivanov wrote:
>>>>>> On Fri, 4 Jan 2019 at 18:12, Mohammad Izadi <moh.izadi@gmail.com>
>>> wrote:
>>>>>>
>>>>>>> You mean a pointer in HEVCSEIDynamicHDRPlus, not in
>>> HEVCSEIContentLight?
>>>>>>> --
>>>>>>> Best,
>>>>>>> Mohammad
>>>>>>>
>>>>>>>
>>>>>>> On Thu, Jan 3, 2019 at 5:08 PM Rostislav Pehlivanov <
>>>>> atomnuker@gmail.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> On Thu, 3 Jan 2019 at 20:13, Mohammad Izadi <moh.izadi@gmail.com>
>>>>> wrote:
>>>>>>>>
>>>>>>>>>
>>>>>>>>>  /**
>>>>>>>>> @@ -94,6 +95,50 @@ typedef struct HEVCSEIMasteringDisplay {
>>>>>>>>>      uint32_t min_luminance;
>>>>>>>>>  } HEVCSEIMasteringDisplay;
>>>>>>>>>
>>>>>>>>> +typedef struct HEVCSEIDynamicHDRPlus{
>>>>>>>>> +    int present;
>>>>>>>>> +    uint8_t itu_t_t35_country_code;
>>>>>>>>> +    uint8_t application_version;
>>>>>>>>> +    uint8_t num_windows;
>>>>>>>>> +    struct {
>>>>>>>>> +        AVRational window_upper_left_corner_x;
>>>>>>>>> +        AVRational window_upper_left_corner_y;
>>>>>>>>> +        AVRational window_lower_right_corner_x;
>>>>>>>>> +        AVRational window_lower_right_corner_y;
>>>>>>>>> +        uint16_t center_of_ellipse_x;
>>>>>>>>> +        uint16_t center_of_ellipse_y;
>>>>>>>>> +        uint8_t rotation_angle;
>>>>>>>>> +        uint16_t semimajor_axis_internal_ellipse;
>>>>>>>>> +        uint16_t semimajor_axis_external_ellipse;
>>>>>>>>> +        uint16_t semiminor_axis_external_ellipse;
>>>>>>>>> +        uint8_t overlap_process_option;
>>>>>>>>> +        AVRational maxscl[3];
>>>>>>>>> +        AVRational average_maxrgb;
>>>>>>>>> +        uint8_t num_distribution_maxrgb_percentiles;
>>>>>>>>> +        struct {
>>>>>>>>> +            uint8_t percentage;
>>>>>>>>> +            AVRational percentile;
>>>>>>>>> +        } distribution_maxrgb[15];
>>>>>>>>> +        AVRational fraction_bright_pixels;
>>>>>>>>> +        uint8_t tone_mapping_flag;
>>>>>>>>> +        AVRational knee_point_x;
>>>>>>>>> +        AVRational knee_point_y;
>>>>>>>>> +        uint8_t num_bezier_curve_anchors;
>>>>>>>>> +        AVRational bezier_curve_anchors[15];
>>>>>>>>> +        uint8_t color_saturation_mapping_flag;
>>>>>>>>> +        AVRational color_saturation_weight;
>>>>>>>>> +    } params[3];
>>>>>>>>> +    AVRational targeted_system_display_maximum_luminance;
>>>>>>>>> +    uint8_t targeted_system_display_actual_peak_luminance_flag;
>>>>>>>>> +    uint8_t
>> num_rows_targeted_system_display_actual_peak_luminance;
>>>>>>>>> +    uint8_t
>> num_cols_targeted_system_display_actual_peak_luminance;
>>>>>>>>> +    AVRational
>>> targeted_system_display_actual_peak_luminance[25][25];
>>>>>>>>> +    uint8_t mastering_display_actual_peak_luminance_flag;
>>>>>>>>> +    uint8_t num_rows_mastering_display_actual_peak_luminance;
>>>>>>>>> +    uint8_t num_cols_mastering_display_actual_peak_luminance;
>>>>>>>>> +    AVRational mastering_display_actual_peak_luminance[25][25];
>>>>>>>>> +} HEVCSEIDynamicHDRPlus;
>>>>>>>>> +
>>>>>>>>>
>>>>>>>>
>>>>>>>> There's no reason to create a new struct for this if all you're
>> going
>>>>> to
>>>>>>> do
>>>>>>>> is to copy it over and over to new frames.
>>>>>>>> What you can do is this: on every SEI containing this thing just
>>>>>>> allocate a
>>>>>>>> AVDynamicHDRPlus struct via av_dynamic_hdr_plus_alloc, wrap it as
>> an
>>>>>>>> AVBufferRef via av_buffer_create, populate it, put it as a pointer
>> in
>>>>>>>> HEVCSEIContentLight and then on every frame just add it to the
>> frame
>>>>> via
>>>>>>>> av_frame_new_side_data_from_buf. If a new SEI is received unref
>> your
>>>>>>>> pointer in HEVCSEIDynamicHDRPlus and repeat by allocating a new
>>> struct,
>>>>>>>> wrapping it as a buffer, populating it, etc.
>>>>>>>> I figure the only reason this wasn't done with other metadata was
>>>>> because
>>>>>>>> they were much smaller and because av_frame_new_side_data_from_buf
>>>>> didn't
>>>>>>>> exist until recently.
>>>>>>>>
>>>>>>>>
>>>>>>>> +            av_log(s->avctx, AV_LOG_DEBUG, ")");
>>>>>>>>> +            if
>> (metadata->params[w].color_saturation_mapping_flag)
>>> {
>>>>>>>>> +                av_log(s->avctx, AV_LOG_DEBUG,
>>>>>>>>> +                       " color_saturation_weight=%5.4f",
>>>>>>>>> +
>>>>>>>>>  av_q2d(metadata->params[w].color_saturation_weight));
>>>>>>>>> +            }
>>>>>>>>> +            av_log(s->avctx, AV_LOG_DEBUG, "}\n");
>>>>>>>>> +        }
>>>>>>>>> +        av_log(s->avctx, AV_LOG_DEBUG,
>>>>>>>>> +               "} End of HDR10+ (SMPTE 2094-40)\n");
>>>>>>>>> +    }
>>>>>>>>>
>>>>>>>>
>>>>>>>> Don't spam stuff like than in the debug log, especially not on
>> every
>>>>>>> single
>>>>>>>> frame. Tools exist to print side data so just don't.
>>>>>>>> _______________________________________________
>>>>>>>> ffmpeg-devel mailing list
>>>>>>>> ffmpeg-devel@ffmpeg.org
>>>>>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> ffmpeg-devel mailing list
>>>>>>> ffmpeg-devel@ffmpeg.org
>>>>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>>>>
>>>>>>
>>>>>> No, I mean in HEVCSEIContentLight. Like I said, HEVCSEIDynamicHDRPlus
>>>>>> shouldn't exist.
>>>>>
>>>>> By "HEVCSEIDynamicHDRPlus shouldn't exist" you mean it shouldn't
>> contain
>>>>> a copy of every bitstream field in the SEI?
>>>>>
>>>>> Content Light and Dynamic HDR10+ are two different SEI types. There's
>> no
>>>>> reason to merge them into a single struct within the HEVC decoder.
>>>>> _______________________________________________
>>>>> ffmpeg-devel mailing list
>>>>> ffmpeg-devel@ffmpeg.org
>>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>>
>>>>
>>>> I'm not asking you to merge them, its just that you kept the 10plus
>> state
>>>> there so it would make sense to replace that state (struct) with the
>>>> avbufferref.
>>>> In reailty if you think there's a better place to put the avbufferref
>>> state
>>>> you'd attach to avframes you should put it there.
>>>> And yes, HEVCSEIDynamicHDRPlus shouldn't exist, there's already a full
>>>> struct in libavutil, so all you need to do is like I said, allocate it,
>>>> populate it, store it somewhere and ref it to new frames.
>>>> No need to create a new structure.
>>>
>>> A HEVCSEIDynamicHDRPlus struct with only the AVBufferRef pointer and a
>>> present field is ok, IMO. No need to add all the bitstream fields there
>>> as per your suggestion.
>>>
>>> I don't like dumping random fields directly in HEVCSEI. Lets keep things
>>> clean looking.
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> 
> There's still no need for a HEVCSEIDynamicHDRPlus struct containing a
> present field and the avbufferref pointer, as the present field would be
> redundant.
> If you give a NULL AVBufferRef to av_buffer_ref(NULL) it'll return NULL.
> Once you pass that NULL to av_frame_new_side_data_from_buf(frame, NULL)
> nothing will change. Hence the present field is unneeded and that would
> leave the struct with a single member, so you migh as well not have it. So
> on every frame you can unconditionally do
> av_frame_new_side_data_from_buf(frame, av_buffer_ref(buffer)) and it'll
> work.

Calling av_frame_new_side_data_from_buf() like that will result in a
leak in case of failure with a valid AVBufferRef, just fyi.
You should create a new ref, pass it, and then unref it in case of failure.

> 
> If you still want to have a struct, you can go for it, it'll just be
> optimized out, I don't care, just keep in mind a present field would be
> pointless.

The present field can be skipped as you suggest. But as i said, i don't
want random SEI message specific fields in HEVCSEI. The substructs were
introduced to organize precisely organize things better, and i'd like to
keep it that way.
diff mbox

Patch

diff --git a/libavcodec/hevc_sei.c b/libavcodec/hevc_sei.c
index c59bd4321e..da6f8257b8 100644
--- a/libavcodec/hevc_sei.c
+++ b/libavcodec/hevc_sei.c
@@ -206,10 +206,207 @@  static int decode_registered_user_data_closed_caption(HEVCSEIA53Caption *s, GetB
     return 0;
 }
 
-static int decode_nal_sei_user_data_registered_itu_t_t35(HEVCSEI *s, GetBitContext *gb,
+static int decode_registered_user_data_dynamic_hdr_plus(HEVCSEIDynamicHDRPlus *s, GetBitContext *gb,
+                                                        void *logctx, int size)
+{
+    const int luminance_den = 10000;
+    const int peak_luminance_den = 15;
+    const int rgb_den = 100000;
+    const int fraction_pixel_den = 1000;
+    const int knee_point_den = 4095;
+    const int bezier_anchor_den = 1023;
+    const int saturation_weight_den = 8;
+
+    int bits_left = size * 8;
+    int w, i, j;
+
+    if (bits_left < 2)
+        return AVERROR_INVALIDDATA;
+
+    s->num_windows = get_bits(gb, 2);
+    bits_left -= 2;
+    if (s->num_windows < 1 || s->num_windows > 3) {
+        av_log(logctx, AV_LOG_ERROR, "num_windows=%d, must be in [1, 3]\n",
+               s->num_windows);
+        return AVERROR_INVALIDDATA;
+    }
+
+    if (bits_left < ((19 * 8 + 1) * (s->num_windows - 1)))
+        return AVERROR(EINVAL);
+    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_bits(gb, 1);
+        bits_left -= 19 * 8 + 1;
+    }
+
+    if (bits_left < 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_bits(gb, 1);
+    bits_left -= 28;
+
+    if (s->targeted_system_display_actual_peak_luminance_flag) {
+        int rows, cols;
+        if (bits_left < 10)
+            return AVERROR(EINVAL);
+        rows = get_bits(gb, 5);
+        cols = get_bits(gb, 5);
+        if (((rows < 2) && (rows > 25)) || ((cols < 2) && (cols > 25))) {
+            av_log(logctx, AV_LOG_ERROR, "num_rows=%d, num_cols=%d, they must "
+                   "be in [2, 25] for "
+                   "targeted_system_display_actual_peak_luminance\n",
+                   rows, cols);
+            return AVERROR_INVALIDDATA;
+        }
+        s->num_rows_targeted_system_display_actual_peak_luminance = rows;
+        s->num_cols_targeted_system_display_actual_peak_luminance = cols;
+        bits_left -= 10;
+
+        if (bits_left < (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;
+            }
+        }
+        bits_left -= (rows * cols * 4);
+    }
+    for (w = 0; w < s->num_windows; w++) {
+        if (bits_left < (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);
+        bits_left -= (3 * 17 + 17 + 4);
+
+        if (bits_left <
+            (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;
+        }
+        bits_left -= (s->params[w].num_distribution_maxrgb_percentiles * 24);
+
+        if (bits_left < 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;
+        bits_left -= 10;
+    }
+    if (bits_left < 1)
+        return AVERROR(EINVAL);
+    s->mastering_display_actual_peak_luminance_flag = get_bits(gb, 1);
+    bits_left--;
+    if (s->mastering_display_actual_peak_luminance_flag) {
+        int rows, cols;
+        if (bits_left < 10)
+            return AVERROR(EINVAL);
+        rows = get_bits(gb, 5);
+        cols = get_bits(gb, 5);
+        if (((rows < 2) && (rows > 25)) || ((cols < 2) && (cols > 25))) {
+            av_log(logctx, AV_LOG_ERROR, "num_rows=%d, num_cols=%d, they must "
+                   "be in [2, 25] for "
+                   "mastering_display_actual_peak_luminance\n",
+                   rows, cols);
+            return AVERROR_INVALIDDATA;
+        }
+        s->num_rows_mastering_display_actual_peak_luminance = rows;
+        s->num_cols_mastering_display_actual_peak_luminance = cols;
+        bits_left -= 10;
+
+        if (bits_left < (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;
+            }
+        }
+        bits_left -= (rows * cols * 4);
+    }
+
+    for (w = 0; w < s->num_windows; w++) {
+        if (bits_left < 1)
+            return AVERROR(EINVAL);
+        s->params[w].tone_mapping_flag = get_bits(gb, 1);
+        bits_left--;
+        if (s->params[w].tone_mapping_flag) {
+            if (bits_left < 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);
+            bits_left -= 28;
+
+            if (bits_left < (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;
+            }
+            bits_left -= (s->params[w].num_bezier_curve_anchors * 10);
+        }
+
+        if (bits_left < 1)
+            return AVERROR(EINVAL);
+        s->params[w].color_saturation_mapping_flag = get_bits(gb, 1);
+        bits_left--;
+        if (s->params[w].color_saturation_mapping_flag) {
+            if (bits_left < 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;
+            bits_left -= 6;
+        }
+    }
+
+    s->present = 1;
+
+    skip_bits(gb, bits_left);
+
+    return 0;
+}
+
+static int decode_nal_sei_user_data_registered_itu_t_t35(HEVCSEI *s,
+                                                         GetBitContext *gb,
+                                                         void *logctx,
                                                          int size)
 {
-    uint32_t country_code;
+    uint8_t country_code;
+    uint16_t provider_code;
     uint32_t user_identifier;
 
     if (size < 7)
@@ -222,11 +419,21 @@  static int decode_nal_sei_user_data_registered_itu_t_t35(HEVCSEI *s, GetBitConte
         size--;
     }
 
-    skip_bits(gb, 8);
-    skip_bits(gb, 8);
-
+    provider_code = get_bits(gb, 16);
     user_identifier = get_bits_long(gb, 32);
 
+    // Check for dynamic metadata - HDR10+(SMPTE 2094-40).
+    if ((provider_code == 0x003C) &&
+        ((user_identifier & 0xFFFFFF00) == 0x00010400)) {
+        s->dynamic_hdr_plus.itu_t_t35_country_code =
+            country_code;
+        s->dynamic_hdr_plus.application_version =
+            (uint8_t)((user_identifier & 0x000000FF));
+
+        return decode_registered_user_data_dynamic_hdr_plus(
+            &s->dynamic_hdr_plus, gb, logctx, size);
+    }
+
     switch (user_identifier) {
         case MKBETAG('G', 'A', '9', '4'):
             return decode_registered_user_data_closed_caption(&s->a53_caption, gb, size);
@@ -292,7 +499,7 @@  static int decode_nal_sei_prefix(GetBitContext *gb, void *logctx, HEVCSEI *s,
     case HEVC_SEI_TYPE_ACTIVE_PARAMETER_SETS:
         return decode_nal_sei_active_parameter_sets(s, gb, logctx);
     case HEVC_SEI_TYPE_USER_DATA_REGISTERED_ITU_T_T35:
-        return decode_nal_sei_user_data_registered_itu_t_t35(s, gb, size);
+        return decode_nal_sei_user_data_registered_itu_t_t35(s, gb, logctx, size);
     case HEVC_SEI_TYPE_ALTERNATIVE_TRANSFER_CHARACTERISTICS:
         return decode_nal_sei_alternative_transfer(&s->alternative_transfer, gb);
     default:
diff --git a/libavcodec/hevc_sei.h b/libavcodec/hevc_sei.h
index 2fec00ace0..ef235560d1 100644
--- a/libavcodec/hevc_sei.h
+++ b/libavcodec/hevc_sei.h
@@ -23,6 +23,7 @@ 
 
 #include <stdint.h>
 
+#include "libavutil/rational.h"
 #include "get_bits.h"
 
 /**
@@ -94,6 +95,50 @@  typedef struct HEVCSEIMasteringDisplay {
     uint32_t min_luminance;
 } HEVCSEIMasteringDisplay;
 
+typedef struct HEVCSEIDynamicHDRPlus{
+    int present;
+    uint8_t itu_t_t35_country_code;
+    uint8_t application_version;
+    uint8_t num_windows;
+    struct {
+        AVRational window_upper_left_corner_x;
+        AVRational window_upper_left_corner_y;
+        AVRational window_lower_right_corner_x;
+        AVRational window_lower_right_corner_y;
+        uint16_t center_of_ellipse_x;
+        uint16_t center_of_ellipse_y;
+        uint8_t rotation_angle;
+        uint16_t semimajor_axis_internal_ellipse;
+        uint16_t semimajor_axis_external_ellipse;
+        uint16_t semiminor_axis_external_ellipse;
+        uint8_t overlap_process_option;
+        AVRational maxscl[3];
+        AVRational average_maxrgb;
+        uint8_t num_distribution_maxrgb_percentiles;
+        struct {
+            uint8_t percentage;
+            AVRational percentile;
+        } distribution_maxrgb[15];
+        AVRational fraction_bright_pixels;
+        uint8_t tone_mapping_flag;
+        AVRational knee_point_x;
+        AVRational knee_point_y;
+        uint8_t num_bezier_curve_anchors;
+        AVRational bezier_curve_anchors[15];
+        uint8_t color_saturation_mapping_flag;
+        AVRational color_saturation_weight;
+    } params[3];
+    AVRational targeted_system_display_maximum_luminance;
+    uint8_t targeted_system_display_actual_peak_luminance_flag;
+    uint8_t num_rows_targeted_system_display_actual_peak_luminance;
+    uint8_t num_cols_targeted_system_display_actual_peak_luminance;
+    AVRational targeted_system_display_actual_peak_luminance[25][25];
+    uint8_t mastering_display_actual_peak_luminance_flag;
+    uint8_t num_rows_mastering_display_actual_peak_luminance;
+    uint8_t num_cols_mastering_display_actual_peak_luminance;
+    AVRational mastering_display_actual_peak_luminance[25][25];
+} HEVCSEIDynamicHDRPlus;
+
 typedef struct HEVCSEIContentLight {
     int present;
     uint16_t max_content_light_level;
@@ -109,6 +154,7 @@  typedef struct HEVCSEI {
     HEVCSEIPictureHash picture_hash;
     HEVCSEIFramePacking frame_packing;
     HEVCSEIDisplayOrientation display_orientation;
+    HEVCSEIDynamicHDRPlus dynamic_hdr_plus;
     HEVCSEIPictureTiming picture_timing;
     HEVCSEIA53Caption a53_caption;
     HEVCSEIMasteringDisplay mastering_display;
diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
index 10bf2563c0..9bd84687e6 100644
--- a/libavcodec/hevcdec.c
+++ b/libavcodec/hevcdec.c
@@ -28,6 +28,7 @@ 
 #include "libavutil/display.h"
 #include "libavutil/internal.h"
 #include "libavutil/mastering_display_metadata.h"
+#include "libavutil/hdr_dynamic_metadata.h"
 #include "libavutil/md5.h"
 #include "libavutil/opt.h"
 #include "libavutil/pixdesc.h"
@@ -2769,6 +2770,200 @@  static int set_side_data(HEVCContext *s)
         s->avctx->color_trc = out->color_trc = s->sei.alternative_transfer.preferred_transfer_characteristics;
     }
 
+    if (s->sei.dynamic_hdr_plus.present) {
+        int w, i, j;
+        AVDynamicHDRPlus *metadata =
+            av_dynamic_hdr_plus_create_side_data(out);
+        if (!metadata) return AVERROR(ENOMEM);
+
+        metadata->itu_t_t35_country_code = s->sei.dynamic_hdr_plus.itu_t_t35_country_code;
+        metadata->application_version = s->sei.dynamic_hdr_plus.application_version;
+        metadata->num_windows = s->sei.dynamic_hdr_plus.num_windows;
+        metadata->targeted_system_display_maximum_luminance.num =
+            s->sei.dynamic_hdr_plus.targeted_system_display_maximum_luminance.num;
+        metadata->targeted_system_display_maximum_luminance.den =
+            s->sei.dynamic_hdr_plus.targeted_system_display_maximum_luminance.den;
+        metadata->targeted_system_display_actual_peak_luminance_flag =
+            s->sei.dynamic_hdr_plus.targeted_system_display_actual_peak_luminance_flag;
+        metadata->mastering_display_actual_peak_luminance_flag =
+            s->sei.dynamic_hdr_plus.mastering_display_actual_peak_luminance_flag;
+        if (metadata->targeted_system_display_actual_peak_luminance_flag){
+            metadata->num_rows_targeted_system_display_actual_peak_luminance =
+                s->sei.dynamic_hdr_plus.num_rows_targeted_system_display_actual_peak_luminance;
+            metadata->num_cols_targeted_system_display_actual_peak_luminance =
+                s->sei.dynamic_hdr_plus.num_cols_targeted_system_display_actual_peak_luminance;
+            for (i = 0;
+                 i < metadata->num_rows_targeted_system_display_actual_peak_luminance;
+                 i++){
+                for (j = 0;
+                     j < metadata->num_cols_targeted_system_display_actual_peak_luminance;
+                     j++){
+                    metadata->targeted_system_display_actual_peak_luminance[i][j].num =
+                        s->sei.dynamic_hdr_plus.targeted_system_display_actual_peak_luminance[i][j].num;
+                    metadata->targeted_system_display_actual_peak_luminance[i][j].den =
+                        s->sei.dynamic_hdr_plus.targeted_system_display_actual_peak_luminance[i][j].den;
+                }
+            }
+        }
+        if (metadata->mastering_display_actual_peak_luminance_flag){
+            metadata->num_rows_mastering_display_actual_peak_luminance =
+                s->sei.dynamic_hdr_plus.num_rows_mastering_display_actual_peak_luminance;
+            metadata->num_cols_mastering_display_actual_peak_luminance =
+                s->sei.dynamic_hdr_plus.num_cols_mastering_display_actual_peak_luminance;
+            for (i = 0;
+                 i < metadata->num_rows_mastering_display_actual_peak_luminance;
+                 i++){
+                for (j = 0;
+                     j < metadata->num_cols_mastering_display_actual_peak_luminance;
+                     j++){
+                    metadata->mastering_display_actual_peak_luminance[i][j].num =
+                        s->sei.dynamic_hdr_plus.mastering_display_actual_peak_luminance[i][j].num;
+                    metadata->mastering_display_actual_peak_luminance[i][j].den =
+                        s->sei.dynamic_hdr_plus.mastering_display_actual_peak_luminance[i][j].den;
+                }
+            }
+        }
+        // Convert coordinates to relative coordinate in [0, 1].
+        for (w = 0; w < metadata->num_windows; w++) {
+            if (w==0){
+                metadata->params[0].window_upper_left_corner_x.num  = 0;
+                metadata->params[0].window_upper_left_corner_y.num  = 0;
+                metadata->params[0].window_lower_right_corner_x.num = out->width-1;
+                metadata->params[0].window_lower_right_corner_y.num = out->height-1;
+            }else{
+                metadata->params[0].window_upper_left_corner_x.num  =
+                    s->sei.dynamic_hdr_plus.params[0].window_upper_left_corner_x.num;
+                metadata->params[0].window_upper_left_corner_y.num  =
+                    s->sei.dynamic_hdr_plus.params[0].window_upper_left_corner_y.num;
+                metadata->params[0].window_lower_right_corner_x.num =
+                    s->sei.dynamic_hdr_plus.params[0].window_lower_right_corner_x.num;
+                metadata->params[0].window_lower_right_corner_y.num =
+                    s->sei.dynamic_hdr_plus.params[0].window_lower_right_corner_y.num;
+            }
+            metadata->params[w].window_upper_left_corner_x.den = out->width-1;
+            metadata->params[w].window_upper_left_corner_y.den = out->height-1;
+            metadata->params[w].window_lower_right_corner_x.den = out->width-1;
+            metadata->params[w].window_lower_right_corner_y.den = out->height-1;
+        }
+
+        metadata->targeted_system_display_maximum_luminance =
+            s->sei.dynamic_hdr_plus.targeted_system_display_maximum_luminance;
+        metadata->targeted_system_display_actual_peak_luminance_flag =
+            s->sei.dynamic_hdr_plus.targeted_system_display_actual_peak_luminance_flag;
+        metadata->mastering_display_actual_peak_luminance_flag =
+            s->sei.dynamic_hdr_plus.mastering_display_actual_peak_luminance_flag;
+        av_log(s->avctx, AV_LOG_DEBUG, "HDR10+(SMPTE 2094-40):{\n");
+        av_log(s->avctx, AV_LOG_DEBUG,
+               "targeted_system_display_maximum_luminance=%5.4f\n"
+               "targeted_system_display_actual_peak_luminance_flag=%d\n"
+               "mastering_display_actual_peak_luminance_flag=%d\n",
+               av_q2d(metadata->targeted_system_display_maximum_luminance),
+               metadata->targeted_system_display_actual_peak_luminance_flag,
+               metadata->mastering_display_actual_peak_luminance_flag);
+
+        for (w = 0; w < metadata->num_windows; w++) {
+            metadata->params[w].center_of_ellipse_x =
+                s->sei.dynamic_hdr_plus.params[w].center_of_ellipse_x;
+            metadata->params[w].center_of_ellipse_y =
+                s->sei.dynamic_hdr_plus.params[w].center_of_ellipse_y;
+            metadata->params[w].rotation_angle =
+                s->sei.dynamic_hdr_plus.params[w].rotation_angle;
+            metadata->params[w].semimajor_axis_internal_ellipse =
+                s->sei.dynamic_hdr_plus.params[w].semimajor_axis_internal_ellipse;
+            metadata->params[w].semimajor_axis_external_ellipse =
+                s->sei.dynamic_hdr_plus.params[w].semimajor_axis_external_ellipse;
+            metadata->params[w].semiminor_axis_external_ellipse =
+                s->sei.dynamic_hdr_plus.params[w].semiminor_axis_external_ellipse;
+            metadata->params[w].overlap_process_option =
+                s->sei.dynamic_hdr_plus.params[w].overlap_process_option;
+            for (i = 0; i < 3; i++){
+                metadata->params[w].maxscl[i].num =
+                    s->sei.dynamic_hdr_plus.params[w].maxscl[i].num;
+                metadata->params[w].maxscl[i].den =
+                    s->sei.dynamic_hdr_plus.params[w].maxscl[i].den;
+            }
+            metadata->params[w].average_maxrgb.num =
+                s->sei.dynamic_hdr_plus.params[w].average_maxrgb.num;
+            metadata->params[w].average_maxrgb.den =
+                s->sei.dynamic_hdr_plus.params[w].average_maxrgb.den;
+            metadata->params[w].num_distribution_maxrgb_percentiles =
+                s->sei.dynamic_hdr_plus.params[w].num_distribution_maxrgb_percentiles;
+            metadata->params[w].fraction_bright_pixels.num =
+                s->sei.dynamic_hdr_plus.params[w].fraction_bright_pixels.num;
+            metadata->params[w].fraction_bright_pixels.den =
+                s->sei.dynamic_hdr_plus.params[w].fraction_bright_pixels.den;
+            metadata->params[w].tone_mapping_flag =
+                s->sei.dynamic_hdr_plus.params[w].tone_mapping_flag;
+            metadata->params[w].knee_point_x.num =
+                s->sei.dynamic_hdr_plus.params[w].knee_point_x.num;
+            metadata->params[w].knee_point_x.den =
+                s->sei.dynamic_hdr_plus.params[w].knee_point_x.den;
+            metadata->params[w].knee_point_y.num =
+                s->sei.dynamic_hdr_plus.params[w].knee_point_y.num;
+            metadata->params[w].knee_point_y.den =
+                s->sei.dynamic_hdr_plus.params[w].knee_point_y.den;
+            metadata->params[w].num_bezier_curve_anchors =
+                s->sei.dynamic_hdr_plus.params[w].num_bezier_curve_anchors;
+            metadata->params[w].color_saturation_mapping_flag =
+                s->sei.dynamic_hdr_plus.params[w].color_saturation_mapping_flag;
+            metadata->params[w].color_saturation_weight.num =
+                s->sei.dynamic_hdr_plus.params[w].color_saturation_weight.num;
+            metadata->params[w].color_saturation_weight.den =
+                s->sei.dynamic_hdr_plus.params[w].color_saturation_weight.den;
+            av_log(s->avctx, AV_LOG_DEBUG,
+                   "window[%d]:{\nBox(%d,%d,%d,%d) "
+                   "maxscl=RGB(%5.4f,%5.4f,%5.4f)  average_maxrgb=%5.4f "
+                   "fraction_bright_pixels=%5.4f ", w,
+                   metadata->params[w].window_upper_left_corner_x.num,
+                   metadata->params[w].window_upper_left_corner_y.num,
+                   metadata->params[w].window_lower_right_corner_x.num,
+                   metadata->params[w].window_lower_right_corner_y.num,
+                   av_q2d(metadata->params[w].maxscl[0]),
+                   av_q2d(metadata->params[w].maxscl[1]),
+                   av_q2d(metadata->params[w].maxscl[2]),
+                   av_q2d(metadata->params[w].average_maxrgb),
+                   av_q2d(metadata->params[w].fraction_bright_pixels));
+            av_log(s->avctx, AV_LOG_DEBUG, "distribution_maxrgb[");
+            for (i = 0;
+                 i < metadata->params[w].num_distribution_maxrgb_percentiles;
+                 i++) {
+                metadata->params[w].distribution_maxrgb[i].percentage =
+                    s->sei.dynamic_hdr_plus.params[w].distribution_maxrgb[i].percentage;
+                metadata->params[w].distribution_maxrgb[i].percentile.num =
+                    s->sei.dynamic_hdr_plus.params[w].distribution_maxrgb[i].percentile.num;
+                metadata->params[w].distribution_maxrgb[i].percentile.den =
+                    s->sei.dynamic_hdr_plus.params[w].distribution_maxrgb[i].percentile.den;
+                av_log(s->avctx, AV_LOG_DEBUG, "(%d,%5.4f)",
+                       metadata->params[w].distribution_maxrgb[i].percentage,
+                       av_q2d(metadata->params[w].distribution_maxrgb[i].percentile));
+            }
+            av_log(s->avctx, AV_LOG_DEBUG, "] ");
+            if (metadata->params[w].tone_mapping_flag) {
+                av_log(s->avctx, AV_LOG_DEBUG, "knee_point(%5.4f,%5.4f) ",
+                       av_q2d(metadata->params[w].knee_point_x),
+                       av_q2d(metadata->params[w].knee_point_y));
+            }
+            av_log(s->avctx, AV_LOG_DEBUG, "bezier_curve_anchors(");
+            for (i = 0; i < metadata->params[w].num_bezier_curve_anchors; i++) {
+                metadata->params[w].bezier_curve_anchors[i].num =
+                    s->sei.dynamic_hdr_plus.params[w].bezier_curve_anchors[i].num;
+                metadata->params[w].bezier_curve_anchors[i].den =
+                    s->sei.dynamic_hdr_plus.params[w].bezier_curve_anchors[i].den;
+                av_log(s->avctx, AV_LOG_DEBUG, "%5.4f ",
+                       av_q2d(metadata->params[w].bezier_curve_anchors[i]));
+            }
+            av_log(s->avctx, AV_LOG_DEBUG, ")");
+            if (metadata->params[w].color_saturation_mapping_flag) {
+                av_log(s->avctx, AV_LOG_DEBUG,
+                       " color_saturation_weight=%5.4f",
+                       av_q2d(metadata->params[w].color_saturation_weight));
+            }
+            av_log(s->avctx, AV_LOG_DEBUG, "}\n");
+        }
+        av_log(s->avctx, AV_LOG_DEBUG,
+               "} End of HDR10+ (SMPTE 2094-40)\n");
+    }
+
     return 0;
 }