[FFmpeg-devel] [PATCH 1/2] Support HDR dynamic metdata (HDR10+) in HEVC decoder.
Rostislav Pehlivanov
atomnuker at gmail.com
Sat Jan 5 04:45:31 EET 2019
On Fri, 4 Jan 2019 at 23:19, Mohammad Izadi <moh.izadi at gmail.com> wrote:
> 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
> >
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at 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.
More information about the ffmpeg-devel
mailing list