On 1/4/2019 11:45 PM, Rostislav Pehlivanov wrote:
> On Fri, 4 Jan 2019 at 23:19, Mohammad Izadi <moh.iz...@gmail.com> wrote:
>
>> Thanks James.
>> --
>> Best,
>> Mohammad
>>
>>
>> On Fri, Jan 4, 2019 at 3:03 PM James Almer <jamr...@gmail.com> wrote:
>>
>>> On 1/4/2019 7:51 PM, Rostislav Pehlivanov wrote:
>>>> On Fri, 4 Jan 2019 at 21:08, James Almer <jamr...@gmail.com> wrote:
>>>>
>>>>> On 1/4/2019 3:53 PM, Rostislav Pehlivanov wrote:
>>>>>> On Fri, 4 Jan 2019 at 18:12, Mohammad Izadi <moh.iz...@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 <
>>>>> atomnu...@gmail.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> On Thu, 3 Jan 2019 at 20:13, Mohammad Izadi <moh.iz...@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@ffmpeg.org
>>>>>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> ffmpeg-devel mailing list
>>>>>>> ffmpeg-devel@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@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@ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
