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

Mohammad Izadi moh.izadi at gmail.com
Fri Jan 4 21:41:04 EET 2019


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 at gmail.com>
wrote:

> On Fri, 4 Jan 2019 at 18:12, Mohammad Izadi <moh.izadi at 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 at gmail.com
> >
> > wrote:
> >
> > > On Thu, 3 Jan 2019 at 20:13, Mohammad Izadi <moh.izadi at 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 at ffmpeg.org
> > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at 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 at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>


More information about the ffmpeg-devel mailing list