[FFmpeg-devel] [PATCH 1/2] Support HDR dynamic metdata (HDR10+) in HEVC decoder.
Mohammad Izadi
moh.izadi at gmail.com
Sat Jan 5 01:19:20 EET 2019
Thanks James.
--
Best,
Mohammad
On Fri, Jan 4, 2019 at 3:03 PM James Almer <jamrial at gmail.com> wrote:
> On 1/4/2019 7:51 PM, Rostislav Pehlivanov wrote:
> > 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.
>
> 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 at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
More information about the ffmpeg-devel
mailing list