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

[Rostislav Pehlivanov]

On Fri, 4 Jan 2019 at 21:08, James Almer <jamrial at 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 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.
>
> 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 at 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.