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

[James Almer]

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.