On 1/4/2019 11:45 PM, Rostislav Pehlivanov wrote: > On Fri, 4 Jan 2019 at 23:19, Mohammad Izadi <[email protected]> wrote: > >> Thanks James. >> -- >> Best, >> Mohammad >> >> >> On Fri, Jan 4, 2019 at 3:03 PM James Almer <[email protected]> wrote: >> >>> On 1/4/2019 7:51 PM, Rostislav Pehlivanov wrote: >>>> On Fri, 4 Jan 2019 at 21:08, James Almer <[email protected]> wrote: >>>> >>>>> On 1/4/2019 3:53 PM, Rostislav Pehlivanov wrote: >>>>>> On Fri, 4 Jan 2019 at 18:12, Mohammad Izadi <[email protected]> >>> wrote: >>>>>> >>>>>>> You mean a pointer in HEVCSEIDynamicHDRPlus, not in >>> HEVCSEIContentLight? >>>>>>> -- >>>>>>> Best, >>>>>>> Mohammad >>>>>>> >>>>>>> >>>>>>> On Thu, Jan 3, 2019 at 5:08 PM Rostislav Pehlivanov < >>>>> [email protected]> >>>>>>> wrote: >>>>>>> >>>>>>>> On Thu, 3 Jan 2019 at 20:13, Mohammad Izadi <[email protected]> >>>>> 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 >>>>>>>> [email protected] >>>>>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >>>>>>>> >>>>>>> _______________________________________________ >>>>>>> ffmpeg-devel mailing list >>>>>>> [email protected] >>>>>>> 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 >>>>> [email protected] >>>>> 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 >>> [email protected] >>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >>> >> _______________________________________________ >> ffmpeg-devel mailing list >> [email protected] >> 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 [email protected] http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
