[FFmpeg-devel] [PATCH 1/2] Support HDR dynamic metdata (HDR10+) in HEVC decoder.
James Almer
jamrial at gmail.com
Sat Jan 5 05:05:09 EET 2019
On 1/4/2019 11:45 PM, Rostislav Pehlivanov wrote:
> 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.
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.
More information about the ffmpeg-devel
mailing list