| Message ID | 3bd88490-9f9f-39e0-756e-9d15d2e78d88@vimeo.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
Hi, While I omitted adding v2/v3 here, I believe all comments on this set of patches have been addressed so far, unless anyone strongly disagrees with the rationale for moving dynamic HDR parsing and serialization to libavutil or with the function signature. Please let me know if I missed anything. Thanks, Raphaël Zumer On 3/2/23 16:43, Raphaël Zumer wrote: > Fixed brace style and moved inline buffer size calculation comments to a single block at the top. > > > Signed-off-by: Raphaël Zumer <rzumer@tebako.net> > --- > libavutil/hdr_dynamic_metadata.c | 142 +++++++++++++++++++++++++++++++ > libavutil/hdr_dynamic_metadata.h | 11 +++ > libavutil/version.h | 2 +- > 3 files changed, 154 insertions(+), 1 deletion(-) > > diff --git a/libavutil/hdr_dynamic_metadata.c b/libavutil/hdr_dynamic_metadata.c > index 98f399b032..24dd3dab2d 100644 > --- a/libavutil/hdr_dynamic_metadata.c > +++ b/libavutil/hdr_dynamic_metadata.c > @@ -225,3 +225,145 @@ int av_dynamic_hdr_plus_from_t35(AVDynamicHDRPlus *s, const uint8_t *data, > > return 0; > } > + > +AVBufferRef *av_dynamic_hdr_plus_to_t35(AVDynamicHDRPlus *s) > +{ > + AVBufferRef *buf; > + size_t size_bits, size_bytes; > + PutBitContext pbc, *pb = &pbc; > + > + if (!s) > + return NULL; > + > + /** > + * Buffer size per CTA-861-H p.253-254: > + * 48 bits for the header (56 minus excluded 8-bit country code) > + * 2 bits for num_windows > + * 937 bits for window geometry, for each window above 1 > + * 27 bits for targeted_system_display_maximum_luminance > + * 1-3855 bits for targeted system display peak luminance information > + * 0-442 bits for intra-window pixel distribution information > + * 1-3855 bits for mastering display peak luminance information > + * 0-537 bits for per-window tonemapping information > + * 0-21 bits for per-window color saturation mapping information > + */ > + size_bits = 48 + > + 2 + > + FFMAX((s->num_windows - 1), 0) * 937 + > + 27 + > + 1 + (s->targeted_system_display_actual_peak_luminance_flag ? 10 + > + s->num_rows_targeted_system_display_actual_peak_luminance * > + s->num_cols_targeted_system_display_actual_peak_luminance * 4 : 0) + > + s->num_windows * 82; > + > + for (int w = 0; w < s->num_windows; w++) > + size_bits += s->params[w].num_distribution_maxrgb_percentiles * 24; > + > + size_bits += 1 + (s->mastering_display_actual_peak_luminance_flag ? 10 + > + s->num_rows_mastering_display_actual_peak_luminance * > + s->num_cols_mastering_display_actual_peak_luminance * 4 : 0) + > + s->num_windows * 1; > + > + for (int w = 0; w < s->num_windows; w++) { > + if (s->params[w].tone_mapping_flag) > + size_bits += 28 + s->params[w].num_bezier_curve_anchors * 10; > + } > + > + size_bits += s->num_windows * 1; > + for (int w = 0; w < s->num_windows; w++) { > + if (s->params[w].color_saturation_mapping_flag) > + size_bits += 6; > + } > + > + size_bytes = (size_bits + 7) / 8; > + > + buf = av_buffer_alloc(size_bytes); > + if (!buf) > + return NULL; > + > + init_put_bits(pb, buf->data, size_bytes); > + > + // itu_t_t35_country_code shall be 0xB5 (USA) (excluded from the payload) > + // itu_t_t35_terminal_provider_code shall be 0x003C > + put_bits(pb, 16, 0x003C); > + // itu_t_t35_terminal_provider_oriented_code is set to ST 2094-40 > + put_bits(pb, 16, 0x0001); > + // application_identifier shall be set to 4 > + put_bits(pb, 8, 4); > + // application_mode is set to Application Version 1 > + put_bits(pb, 8, 1); > + > + // Payload as per CTA-861-H p.253-254 > + put_bits(pb, 2, s->num_windows); > + > + for (int w = 1; w < s->num_windows; w++) { > + put_bits(pb, 16, s->params[w].window_upper_left_corner_x.num / s->params[w].window_upper_left_corner_x.den); > + put_bits(pb, 16, s->params[w].window_upper_left_corner_y.num / s->params[w].window_upper_left_corner_y.den); > + put_bits(pb, 16, s->params[w].window_lower_right_corner_x.num / s->params[w].window_lower_right_corner_x.den); > + put_bits(pb, 16, s->params[w].window_lower_right_corner_y.num / s->params[w].window_lower_right_corner_y.den); > + put_bits(pb, 16, s->params[w].center_of_ellipse_x); > + put_bits(pb, 16, s->params[w].center_of_ellipse_y); > + put_bits(pb, 8, s->params[w].rotation_angle); > + put_bits(pb, 16, s->params[w].semimajor_axis_internal_ellipse); > + put_bits(pb, 16, s->params[w].semimajor_axis_external_ellipse); > + put_bits(pb, 16, s->params[w].semiminor_axis_external_ellipse); > + put_bits(pb, 1, s->params[w].overlap_process_option); > + } > + > + put_bits(pb, 27, s->targeted_system_display_maximum_luminance.num * luminance_den / > + s->targeted_system_display_maximum_luminance.den); > + put_bits(pb, 1, s->targeted_system_display_actual_peak_luminance_flag); > + if (s->targeted_system_display_actual_peak_luminance_flag) { > + put_bits(pb, 5, s->num_rows_targeted_system_display_actual_peak_luminance); > + put_bits(pb, 5, s->num_cols_targeted_system_display_actual_peak_luminance); > + for (int i = 0; i < s->num_rows_targeted_system_display_actual_peak_luminance; i++) { > + for (int j = 0; j < s->num_cols_targeted_system_display_actual_peak_luminance; j++) > + put_bits(pb, 4, s->targeted_system_display_actual_peak_luminance[i][j].num * peak_luminance_den / > + s->targeted_system_display_actual_peak_luminance[i][j].den); > + } > + } > + > + for (int w = 0; w < s->num_windows; w++) { > + for (int i = 0; i < 3; i++) > + put_bits(pb, 17, s->params[w].maxscl[i].num * rgb_den / s->params[w].maxscl[i].den); > + put_bits(pb, 17, s->params[w].average_maxrgb.num * rgb_den / s->params[w].average_maxrgb.den); > + put_bits(pb, 4, s->params[w].num_distribution_maxrgb_percentiles); > + for (int i = 0; i < s->params[w].num_distribution_maxrgb_percentiles; i++) { > + put_bits(pb, 7, s->params[w].distribution_maxrgb[i].percentage); > + put_bits(pb, 17, s->params[w].distribution_maxrgb[i].percentile.num * rgb_den / > + s->params[w].distribution_maxrgb[i].percentile.den); > + } > + put_bits(pb, 10, s->params[w].fraction_bright_pixels.num * fraction_pixel_den / > + s->params[w].fraction_bright_pixels.den); > + } > + > + put_bits(pb, 1, s->mastering_display_actual_peak_luminance_flag); > + if (s->mastering_display_actual_peak_luminance_flag) { > + put_bits(pb, 5, s->num_rows_mastering_display_actual_peak_luminance); > + put_bits(pb, 5, s->num_cols_mastering_display_actual_peak_luminance); > + for (int i = 0; i < s->num_rows_mastering_display_actual_peak_luminance; i++) { > + for (int j = 0; j < s->num_cols_mastering_display_actual_peak_luminance; j++) > + put_bits(pb, 4, s->mastering_display_actual_peak_luminance[i][j].num * peak_luminance_den / > + s->mastering_display_actual_peak_luminance[i][j].den); > + } > + } > + > + for (int w = 0; w < s->num_windows; w++) { > + put_bits(pb, 1, s->params[w].tone_mapping_flag); > + if (s->params[w].tone_mapping_flag) { > + put_bits(pb, 12, s->params[w].knee_point_x.num * knee_point_den / s->params[w].knee_point_x.den); > + put_bits(pb, 12, s->params[w].knee_point_y.num * knee_point_den / s->params[w].knee_point_y.den); > + put_bits(pb, 4, s->params[w].num_bezier_curve_anchors); > + for (int i = 0; i < s->params[w].num_bezier_curve_anchors; i++) > + put_bits(pb, 10, s->params[w].bezier_curve_anchors[i].num * bezier_anchor_den / > + s->params[w].bezier_curve_anchors[i].den); > + put_bits(pb, 1, s->params[w].color_saturation_mapping_flag); > + if (s->params[w].color_saturation_mapping_flag) > + put_bits(pb, 6, s->params[w].color_saturation_weight.num * saturation_weight_den / > + s->params[w].color_saturation_weight.den); > + } > + } > + > + flush_put_bits(pb); > + return buf; > +} > diff --git a/libavutil/hdr_dynamic_metadata.h b/libavutil/hdr_dynamic_metadata.h > index 1f953ef1f5..797a5c64ae 100644 > --- a/libavutil/hdr_dynamic_metadata.h > +++ b/libavutil/hdr_dynamic_metadata.h > @@ -21,6 +21,7 @@ > #ifndef AVUTIL_HDR_DYNAMIC_METADATA_H > #define AVUTIL_HDR_DYNAMIC_METADATA_H > > +#include "buffer.h" > #include "frame.h" > #include "rational.h" > > @@ -351,4 +352,14 @@ AVDynamicHDRPlus *av_dynamic_hdr_plus_create_side_data(AVFrame *frame); > int av_dynamic_hdr_plus_from_t35(AVDynamicHDRPlus *s, const uint8_t *data, > int size); > > +/** > + * Serialize dynamic HDR10+ metadata to a user data registered ITU-T T.35 buffer, > + * excluding the country code and beginning with the terminal provider code. > + * @param s A pointer containing the decoded AVDynamicHDRPlus structure. > + * > + * @return Pointer to an AVBufferRef containing the raw ITU-T T.35 representation > + * of the HDR10+ metadata if succeed, or NULL if buffer allocation fails. > + */ > +AVBufferRef *av_dynamic_hdr_plus_to_t35(AVDynamicHDRPlus *s); > + > #endif /* AVUTIL_HDR_DYNAMIC_METADATA_H */ > diff --git a/libavutil/version.h b/libavutil/version.h > index 900b798971..7635672985 100644 > --- a/libavutil/version.h > +++ b/libavutil/version.h > @@ -79,7 +79,7 @@ > */ > > #define LIBAVUTIL_VERSION_MAJOR 58 > -#define LIBAVUTIL_VERSION_MINOR 3 > +#define LIBAVUTIL_VERSION_MINOR 4 > #define LIBAVUTIL_VERSION_MICRO 100 > > #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
On 3/9/2023 11:18 AM, Raphaël Zumer wrote: > Hi, > > While I omitted adding v2/v3 here, I believe all comments on this set of patches have been addressed so far, unless anyone strongly disagrees with the rationale for moving dynamic HDR parsing and serialization to libavutil or with the function signature. > > Please let me know if I missed anything. > > Thanks, > Raphaël Zumer I'll apply this (and patch 1/1) in a few days if nobody comments. The code needs to be in lavu if we want muxers and demuxers to use this functionality, so moving the existing lavc functions is fine unless we add more avpriv_ functions that people tend to dislike.
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Raphaël Zumer > Sent: 2023年3月3日 5:43 > To: ffmpeg-devel@ffmpeg.org > Subject: [FFmpeg-devel] [PATCH 2/2] avutil: add HDR10+ dynamic metadata serialization function > > Fixed brace style and moved inline buffer size calculation comments to a single block at the top. > > > Signed-off-by: Raphaël Zumer <rzumer@tebako.net> > --- > libavutil/hdr_dynamic_metadata.c | 142 +++++++++++++++++++++++++++++++ > libavutil/hdr_dynamic_metadata.h | 11 +++ > libavutil/version.h | 2 +- > 3 files changed, 154 insertions(+), 1 deletion(-) > > diff --git a/libavutil/hdr_dynamic_metadata.c b/libavutil/hdr_dynamic_metadata.c > index 98f399b032..24dd3dab2d 100644 > --- a/libavutil/hdr_dynamic_metadata.c > +++ b/libavutil/hdr_dynamic_metadata.c > @@ -225,3 +225,145 @@ int av_dynamic_hdr_plus_from_t35(AVDynamicHDRPlus *s, const uint8_t *data, > > return 0; > } > + > +AVBufferRef *av_dynamic_hdr_plus_to_t35(AVDynamicHDRPlus *s) > +{ How about add 'const' modifier to AVDynamicHDRPlus *s ? > + AVBufferRef *buf; > + size_t size_bits, size_bytes; > + PutBitContext pbc, *pb = &pbc; > + > + if (!s) > + return NULL; > + > + /** > + * Buffer size per CTA-861-H p.253-254: > + * 48 bits for the header (56 minus excluded 8-bit country code) > + * 2 bits for num_windows > + * 937 bits for window geometry, for each window above 1 > + * 27 bits for targeted_system_display_maximum_luminance > + * 1-3855 bits for targeted system display peak luminance information > + * 0-442 bits for intra-window pixel distribution information > + * 1-3855 bits for mastering display peak luminance information > + * 0-537 bits for per-window tonemapping information > + * 0-21 bits for per-window color saturation mapping information > + */ > + size_bits = 48 + > + 2 + > + FFMAX((s->num_windows - 1), 0) * 937 + > + 27 + > + 1 + (s->targeted_system_display_actual_peak_luminance_flag ? 10 + > + s->num_rows_targeted_system_display_actual_peak_luminance * > + s->num_cols_targeted_system_display_actual_peak_luminance * 4 : 0) + > + s->num_windows * 82; > + > + for (int w = 0; w < s->num_windows; w++) > + size_bits += s->params[w].num_distribution_maxrgb_percentiles * 24; > + > + size_bits += 1 + (s->mastering_display_actual_peak_luminance_flag ? 10 + > + s->num_rows_mastering_display_actual_peak_luminance * > + s->num_cols_mastering_display_actual_peak_luminance * 4 : 0) + > + s->num_windows * 1; > + > + for (int w = 0; w < s->num_windows; w++) { > + if (s->params[w].tone_mapping_flag) > + size_bits += 28 + s->params[w].num_bezier_curve_anchors * 10; > + } > + > + size_bits += s->num_windows * 1; > + for (int w = 0; w < s->num_windows; w++) { > + if (s->params[w].color_saturation_mapping_flag) > + size_bits += 6; > + } > + > + size_bytes = (size_bits + 7) / 8; > + > + buf = av_buffer_alloc(size_bytes); > + if (!buf) > + return NULL; > + > + init_put_bits(pb, buf->data, size_bytes); > + > + // itu_t_t35_country_code shall be 0xB5 (USA) (excluded from the payload) > + // itu_t_t35_terminal_provider_code shall be 0x003C > + put_bits(pb, 16, 0x003C); > + // itu_t_t35_terminal_provider_oriented_code is set to ST 2094-40 > + put_bits(pb, 16, 0x0001); > + // application_identifier shall be set to 4 > + put_bits(pb, 8, 4); > + // application_mode is set to Application Version 1 > + put_bits(pb, 8, 1); > + > + // Payload as per CTA-861-H p.253-254 > + put_bits(pb, 2, s->num_windows); > + > + for (int w = 1; w < s->num_windows; w++) { > + put_bits(pb, 16, s->params[w].window_upper_left_corner_x.num / s->params[w].window_upper_left_corner_x.den); > + put_bits(pb, 16, s->params[w].window_upper_left_corner_y.num / s->params[w].window_upper_left_corner_y.den); > + put_bits(pb, 16, s->params[w].window_lower_right_corner_x.num / s->params[w].window_lower_right_corner_x.den); > + put_bits(pb, 16, s->params[w].window_lower_right_corner_y.num / s->params[w].window_lower_right_corner_y.den); > + put_bits(pb, 16, s->params[w].center_of_ellipse_x); > + put_bits(pb, 16, s->params[w].center_of_ellipse_y); > + put_bits(pb, 8, s->params[w].rotation_angle); > + put_bits(pb, 16, s->params[w].semimajor_axis_internal_ellipse); > + put_bits(pb, 16, s->params[w].semimajor_axis_external_ellipse); > + put_bits(pb, 16, s->params[w].semiminor_axis_external_ellipse); > + put_bits(pb, 1, s->params[w].overlap_process_option); > + } > + > + put_bits(pb, 27, s->targeted_system_display_maximum_luminance.num * luminance_den / > + s->targeted_system_display_maximum_luminance.den); > + put_bits(pb, 1, s->targeted_system_display_actual_peak_luminance_flag); > + if (s->targeted_system_display_actual_peak_luminance_flag) { > + put_bits(pb, 5, s->num_rows_targeted_system_display_actual_peak_luminance); > + put_bits(pb, 5, s->num_cols_targeted_system_display_actual_peak_luminance); > + for (int i = 0; i < s->num_rows_targeted_system_display_actual_peak_luminance; i++) { > + for (int j = 0; j < s->num_cols_targeted_system_display_actual_peak_luminance; j++) > + put_bits(pb, 4, s->targeted_system_display_actual_peak_luminance[i][j].num * peak_luminance_den / > + s->targeted_system_display_actual_peak_luminance[i][j].den); > + } > + } > + > + for (int w = 0; w < s->num_windows; w++) { > + for (int i = 0; i < 3; i++) > + put_bits(pb, 17, s->params[w].maxscl[i].num * rgb_den / s->params[w].maxscl[i].den); > + put_bits(pb, 17, s->params[w].average_maxrgb.num * rgb_den / s->params[w].average_maxrgb.den); > + put_bits(pb, 4, s->params[w].num_distribution_maxrgb_percentiles); > + for (int i = 0; i < s->params[w].num_distribution_maxrgb_percentiles; i++) { > + put_bits(pb, 7, s->params[w].distribution_maxrgb[i].percentage); > + put_bits(pb, 17, s->params[w].distribution_maxrgb[i].percentile.num * rgb_den / > + s->params[w].distribution_maxrgb[i].percentile.den); > + } > + put_bits(pb, 10, s->params[w].fraction_bright_pixels.num * fraction_pixel_den / > + s->params[w].fraction_bright_pixels.den); > + } > + > + put_bits(pb, 1, s->mastering_display_actual_peak_luminance_flag); > + if (s->mastering_display_actual_peak_luminance_flag) { > + put_bits(pb, 5, s->num_rows_mastering_display_actual_peak_luminance); > + put_bits(pb, 5, s->num_cols_mastering_display_actual_peak_luminance); > + for (int i = 0; i < s->num_rows_mastering_display_actual_peak_luminance; i++) { > + for (int j = 0; j < s->num_cols_mastering_display_actual_peak_luminance; j++) > + put_bits(pb, 4, s->mastering_display_actual_peak_luminance[i][j].num * peak_luminance_den / > + s->mastering_display_actual_peak_luminance[i][j].den); > + } > + } > + > + for (int w = 0; w < s->num_windows; w++) { > + put_bits(pb, 1, s->params[w].tone_mapping_flag); > + if (s->params[w].tone_mapping_flag) { > + put_bits(pb, 12, s->params[w].knee_point_x.num * knee_point_den / s->params[w].knee_point_x.den); > + put_bits(pb, 12, s->params[w].knee_point_y.num * knee_point_den / s->params[w].knee_point_y.den); > + put_bits(pb, 4, s->params[w].num_bezier_curve_anchors); > + for (int i = 0; i < s->params[w].num_bezier_curve_anchors; i++) > + put_bits(pb, 10, s->params[w].bezier_curve_anchors[i].num * bezier_anchor_den / > + s->params[w].bezier_curve_anchors[i].den); > + put_bits(pb, 1, s->params[w].color_saturation_mapping_flag); > + if (s->params[w].color_saturation_mapping_flag) > + put_bits(pb, 6, s->params[w].color_saturation_weight.num * saturation_weight_den / > + s->params[w].color_saturation_weight.den); > + } > + } > + > + flush_put_bits(pb); > + return buf; > +} > diff --git a/libavutil/hdr_dynamic_metadata.h b/libavutil/hdr_dynamic_metadata.h > index 1f953ef1f5..797a5c64ae 100644 > --- a/libavutil/hdr_dynamic_metadata.h > +++ b/libavutil/hdr_dynamic_metadata.h > @@ -21,6 +21,7 @@ > #ifndef AVUTIL_HDR_DYNAMIC_METADATA_H > #define AVUTIL_HDR_DYNAMIC_METADATA_H > > +#include "buffer.h" > #include "frame.h" > #include "rational.h" > > @@ -351,4 +352,14 @@ AVDynamicHDRPlus *av_dynamic_hdr_plus_create_side_data(AVFrame *frame); > int av_dynamic_hdr_plus_from_t35(AVDynamicHDRPlus *s, const uint8_t *data, > int size); > > +/** > + * Serialize dynamic HDR10+ metadata to a user data registered ITU-T T.35 buffer, > + * excluding the country code and beginning with the terminal provider code. > + * @param s A pointer containing the decoded AVDynamicHDRPlus structure. > + * > + * @return Pointer to an AVBufferRef containing the raw ITU-T T.35 representation > + * of the HDR10+ metadata if succeed, or NULL if buffer allocation fails. > + */ > +AVBufferRef *av_dynamic_hdr_plus_to_t35(AVDynamicHDRPlus *s); > + > #endif /* AVUTIL_HDR_DYNAMIC_METADATA_H */ > diff --git a/libavutil/version.h b/libavutil/version.h > index 900b798971..7635672985 100644 > --- a/libavutil/version.h > +++ b/libavutil/version.h > @@ -79,7 +79,7 @@ > */ > > #define LIBAVUTIL_VERSION_MAJOR 58 > -#define LIBAVUTIL_VERSION_MINOR 3 > +#define LIBAVUTIL_VERSION_MINOR 4 > #define LIBAVUTIL_VERSION_MICRO 100 > > #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \ > -- > 2.39.1 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Quoting Raphaël Zumer (2023-03-02 22:43:29) > +/** > + * Serialize dynamic HDR10+ metadata to a user data registered ITU-T T.35 buffer, > + * excluding the country code and beginning with the terminal provider code. > + * @param s A pointer containing the decoded AVDynamicHDRPlus structure. > + * > + * @return Pointer to an AVBufferRef containing the raw ITU-T T.35 representation > + * of the HDR10+ metadata if succeed, or NULL if buffer allocation fails. > + */ > +AVBufferRef *av_dynamic_hdr_plus_to_t35(AVDynamicHDRPlus *s); Why is this an AVBufferRef rather than a plain av_malloced() uint8_t array? You can very easily turn the latter into the former, but the reverse is a lot more annoying.
I expanded on this in another email in the chain, but the buffer size needs to be communicated to the user, as it is not embedded in the payload. It seems needlessly convoluted to me to create a separate function solely to calculate the size of the buffer so that it can be allocated by the user and passed to the serialization function, and I cannot think of another solution that would not be even more convoluted and awkward for the user. I don't understand how going from AVBufferRef to uint8_t* is more complicated than the reverse. The buffer in the AVBufferRef is allocated via av_malloc() and is directly accessible through the data field. Am I missing some detail? Raphaël Zumer On 3/12/23 15:48, Anton Khirnov wrote: > Quoting Raphaël Zumer (2023-03-02 22:43:29) >> +/** >> + * Serialize dynamic HDR10+ metadata to a user data registered ITU-T T.35 buffer, >> + * excluding the country code and beginning with the terminal provider code. >> + * @param s A pointer containing the decoded AVDynamicHDRPlus structure. >> + * >> + * @return Pointer to an AVBufferRef containing the raw ITU-T T.35 representation >> + * of the HDR10+ metadata if succeed, or NULL if buffer allocation fails. >> + */ >> +AVBufferRef *av_dynamic_hdr_plus_to_t35(AVDynamicHDRPlus *s); > Why is this an AVBufferRef rather than a plain av_malloced() uint8_t > array? You can very easily turn the latter into the former, but the > reverse is a lot more annoying. >
On 3/12/2023 6:50 PM, Raphaël Zumer wrote: > I expanded on this in another email in the chain, but the buffer size needs to be communicated to the user, as it is not embedded in the payload. It seems needlessly convoluted to me to create a separate function solely to calculate the size of the buffer so that it can be allocated by the user and passed to the serialization function, and I cannot think of another solution that would not be even more convoluted and awkward for the user. > > I don't understand how going from AVBufferRef to uint8_t* is more complicated than the reverse. The buffer in the AVBufferRef is allocated via av_malloc() and is directly accessible through the data field. Am I missing some detail? > > Raphaël Zumer You can do it like how the AVDynamicHDRPlus struct is allocated and its size returned to the user in av_dynamic_hdr_plus_alloc(). That is uint8_t *av_dynamic_hdr_plus_to_t35(AVDynamicHDRPlus *s, size_t *size); The function would then store the calculated size of the array on *size. > > On 3/12/23 15:48, Anton Khirnov wrote: >> Quoting Raphaël Zumer (2023-03-02 22:43:29) >>> +/** >>> + * Serialize dynamic HDR10+ metadata to a user data registered ITU-T T.35 buffer, >>> + * excluding the country code and beginning with the terminal provider code. >>> + * @param s A pointer containing the decoded AVDynamicHDRPlus structure. >>> + * >>> + * @return Pointer to an AVBufferRef containing the raw ITU-T T.35 representation >>> + * of the HDR10+ metadata if succeed, or NULL if buffer allocation fails. >>> + */ >>> +AVBufferRef *av_dynamic_hdr_plus_to_t35(AVDynamicHDRPlus *s); >> Why is this an AVBufferRef rather than a plain av_malloced() uint8_t >> array? You can very easily turn the latter into the former, but the >> reverse is a lot more annoying. >> > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On 3/12/23 17:52, James Almer wrote: > On 3/12/2023 6:50 PM, Raphaël Zumer wrote: >> I expanded on this in another email in the chain, but the buffer size needs to be communicated to the user, as it is not embedded in the payload. It seems needlessly convoluted to me to create a separate function solely to calculate the size of the buffer so that it can be allocated by the user and passed to the serialization function, and I cannot think of another solution that would not be even more convoluted and awkward for the user. >> >> I don't understand how going from AVBufferRef to uint8_t* is more complicated than the reverse. The buffer in the AVBufferRef is allocated via av_malloc() and is directly accessible through the data field. Am I missing some detail? >> >> Raphaël Zumer > You can do it like how the AVDynamicHDRPlus struct is allocated and its > size returned to the user in av_dynamic_hdr_plus_alloc(). That is > > uint8_t *av_dynamic_hdr_plus_to_t35(AVDynamicHDRPlus *s, size_t *size); > > The function would then store the calculated size of the array on *size. OK great, I will do that and address the remaining comments tomorrow. Thanks >> On 3/12/23 15:48, Anton Khirnov wrote: >>> Quoting Raphaël Zumer (2023-03-02 22:43:29) >>>> +/** >>>> + * Serialize dynamic HDR10+ metadata to a user data registered ITU-T T.35 buffer, >>>> + * excluding the country code and beginning with the terminal provider code. >>>> + * @param s A pointer containing the decoded AVDynamicHDRPlus structure. >>>> + * >>>> + * @return Pointer to an AVBufferRef containing the raw ITU-T T.35 representation >>>> + * of the HDR10+ metadata if succeed, or NULL if buffer allocation fails. >>>> + */ >>>> +AVBufferRef *av_dynamic_hdr_plus_to_t35(AVDynamicHDRPlus *s); >>> Why is this an AVBufferRef rather than a plain av_malloced() uint8_t >>> array? You can very easily turn the latter into the former, but the >>> reverse is a lot more annoying. >>> >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Quoting Raphaël Zumer (2023-03-12 22:50:27) > I expanded on this in another email in the chain, but the buffer size needs to be communicated to the user, as it is not embedded in the payload. It seems needlessly convoluted to me to create a separate function solely to calculate the size of the buffer so that it can be allocated by the user and passed to the serialization function, and I cannot think of another solution that would not be even more convoluted and awkward for the user. > > I don't understand how going from AVBufferRef to uint8_t* is more complicated than the reverse. The buffer in the AVBufferRef is allocated via av_malloc() and is directly accessible through the data field. Am I missing some detail? What I mean is that once data is wrapped in an AVBufer, it is "locked in", you always have to keep an AVBufferRef around.
James Almer: > On 3/9/2023 11:18 AM, Raphaël Zumer wrote: >> Hi, >> >> While I omitted adding v2/v3 here, I believe all comments on this set >> of patches have been addressed so far, unless anyone strongly >> disagrees with the rationale for moving dynamic HDR parsing and >> serialization to libavutil or with the function signature. >> >> Please let me know if I missed anything. >> >> Thanks, >> Raphaël Zumer > > I'll apply this (and patch 1/1) in a few days if nobody comments. > The code needs to be in lavu if we want muxers and demuxers to use this > functionality, so moving the existing lavc functions is fine unless we > add more avpriv_ functions that people tend to dislike. Can we wait with this until we have the actual patches that make use of it? (And is lavfi actually supposed to make use of this? Or why is it moved to lavu at all? lavf can also use it in lavc.) - Andreas
On 3/13/2023 7:25 PM, Andreas Rheinhardt wrote: > James Almer: >> On 3/9/2023 11:18 AM, Raphaël Zumer wrote: >>> Hi, >>> >>> While I omitted adding v2/v3 here, I believe all comments on this set >>> of patches have been addressed so far, unless anyone strongly >>> disagrees with the rationale for moving dynamic HDR parsing and >>> serialization to libavutil or with the function signature. >>> >>> Please let me know if I missed anything. >>> >>> Thanks, >>> Raphaël Zumer >> >> I'll apply this (and patch 1/1) in a few days if nobody comments. >> The code needs to be in lavu if we want muxers and demuxers to use this >> functionality, so moving the existing lavc functions is fine unless we >> add more avpriv_ functions that people tend to dislike. > > Can we wait with this until we have the actual patches that make use of it? > (And is lavfi actually supposed to make use of this? Or why is it moved > to lavu at all? lavf can also use it in lavc.) I have patches ready to make use of one of the two functions in the matroska demuxer. Will send them after this is pushed. And I'll attempt to write one to make use of the serialization function in the matroska muxer too. And it makes sense for this to be in lavu even if only lavf uses these, given there's a public header for it already. Installing two headers for HDR10+ functions in two separate libraries seems overkill when they use the same struct.
diff --git a/libavutil/hdr_dynamic_metadata.c b/libavutil/hdr_dynamic_metadata.c index 98f399b032..24dd3dab2d 100644 --- a/libavutil/hdr_dynamic_metadata.c +++ b/libavutil/hdr_dynamic_metadata.c @@ -225,3 +225,145 @@ int av_dynamic_hdr_plus_from_t35(AVDynamicHDRPlus *s, const uint8_t *data, return 0; } + +AVBufferRef *av_dynamic_hdr_plus_to_t35(AVDynamicHDRPlus *s) +{ + AVBufferRef *buf; + size_t size_bits, size_bytes; + PutBitContext pbc, *pb = &pbc; + + if (!s) + return NULL; + + /** + * Buffer size per CTA-861-H p.253-254: + * 48 bits for the header (56 minus excluded 8-bit country code) + * 2 bits for num_windows + * 937 bits for window geometry, for each window above 1 + * 27 bits for targeted_system_display_maximum_luminance + * 1-3855 bits for targeted system display peak luminance information + * 0-442 bits for intra-window pixel distribution information + * 1-3855 bits for mastering display peak luminance information + * 0-537 bits for per-window tonemapping information + * 0-21 bits for per-window color saturation mapping information + */ + size_bits = 48 + + 2 + + FFMAX((s->num_windows - 1), 0) * 937 + + 27 + + 1 + (s->targeted_system_display_actual_peak_luminance_flag ? 10 + + s->num_rows_targeted_system_display_actual_peak_luminance * + s->num_cols_targeted_system_display_actual_peak_luminance * 4 : 0) + + s->num_windows * 82; + + for (int w = 0; w < s->num_windows; w++) + size_bits += s->params[w].num_distribution_maxrgb_percentiles * 24; + + size_bits += 1 + (s->mastering_display_actual_peak_luminance_flag ? 10 + + s->num_rows_mastering_display_actual_peak_luminance * + s->num_cols_mastering_display_actual_peak_luminance * 4 : 0) + + s->num_windows * 1; + + for (int w = 0; w < s->num_windows; w++) { + if (s->params[w].tone_mapping_flag) + size_bits += 28 + s->params[w].num_bezier_curve_anchors * 10; + } + + size_bits += s->num_windows * 1; + for (int w = 0; w < s->num_windows; w++) { + if (s->params[w].color_saturation_mapping_flag) + size_bits += 6; + } + + size_bytes = (size_bits + 7) / 8; + + buf = av_buffer_alloc(size_bytes); + if (!buf) + return NULL; + + init_put_bits(pb, buf->data, size_bytes); + + // itu_t_t35_country_code shall be 0xB5 (USA) (excluded from the payload) + // itu_t_t35_terminal_provider_code shall be 0x003C + put_bits(pb, 16, 0x003C); + // itu_t_t35_terminal_provider_oriented_code is set to ST 2094-40 + put_bits(pb, 16, 0x0001); + // application_identifier shall be set to 4 + put_bits(pb, 8, 4); + // application_mode is set to Application Version 1 + put_bits(pb, 8, 1); + + // Payload as per CTA-861-H p.253-254 + put_bits(pb, 2, s->num_windows); + + for (int w = 1; w < s->num_windows; w++) { + put_bits(pb, 16, s->params[w].window_upper_left_corner_x.num / s->params[w].window_upper_left_corner_x.den); + put_bits(pb, 16, s->params[w].window_upper_left_corner_y.num / s->params[w].window_upper_left_corner_y.den); + put_bits(pb, 16, s->params[w].window_lower_right_corner_x.num / s->params[w].window_lower_right_corner_x.den); + put_bits(pb, 16, s->params[w].window_lower_right_corner_y.num / s->params[w].window_lower_right_corner_y.den); + put_bits(pb, 16, s->params[w].center_of_ellipse_x); + put_bits(pb, 16, s->params[w].center_of_ellipse_y); + put_bits(pb, 8, s->params[w].rotation_angle); + put_bits(pb, 16, s->params[w].semimajor_axis_internal_ellipse); + put_bits(pb, 16, s->params[w].semimajor_axis_external_ellipse); + put_bits(pb, 16, s->params[w].semiminor_axis_external_ellipse); + put_bits(pb, 1, s->params[w].overlap_process_option); + } + + put_bits(pb, 27, s->targeted_system_display_maximum_luminance.num * luminance_den / + s->targeted_system_display_maximum_luminance.den); + put_bits(pb, 1, s->targeted_system_display_actual_peak_luminance_flag); + if (s->targeted_system_display_actual_peak_luminance_flag) { + put_bits(pb, 5, s->num_rows_targeted_system_display_actual_peak_luminance); + put_bits(pb, 5, s->num_cols_targeted_system_display_actual_peak_luminance); + for (int i = 0; i < s->num_rows_targeted_system_display_actual_peak_luminance; i++) { + for (int j = 0; j < s->num_cols_targeted_system_display_actual_peak_luminance; j++) + put_bits(pb, 4, s->targeted_system_display_actual_peak_luminance[i][j].num * peak_luminance_den / + s->targeted_system_display_actual_peak_luminance[i][j].den); + } + } + + for (int w = 0; w < s->num_windows; w++) { + for (int i = 0; i < 3; i++) + put_bits(pb, 17, s->params[w].maxscl[i].num * rgb_den / s->params[w].maxscl[i].den); + put_bits(pb, 17, s->params[w].average_maxrgb.num * rgb_den / s->params[w].average_maxrgb.den); + put_bits(pb, 4, s->params[w].num_distribution_maxrgb_percentiles); + for (int i = 0; i < s->params[w].num_distribution_maxrgb_percentiles; i++) { + put_bits(pb, 7, s->params[w].distribution_maxrgb[i].percentage); + put_bits(pb, 17, s->params[w].distribution_maxrgb[i].percentile.num * rgb_den / + s->params[w].distribution_maxrgb[i].percentile.den); + } + put_bits(pb, 10, s->params[w].fraction_bright_pixels.num * fraction_pixel_den / + s->params[w].fraction_bright_pixels.den); + } + + put_bits(pb, 1, s->mastering_display_actual_peak_luminance_flag); + if (s->mastering_display_actual_peak_luminance_flag) { + put_bits(pb, 5, s->num_rows_mastering_display_actual_peak_luminance); + put_bits(pb, 5, s->num_cols_mastering_display_actual_peak_luminance); + for (int i = 0; i < s->num_rows_mastering_display_actual_peak_luminance; i++) { + for (int j = 0; j < s->num_cols_mastering_display_actual_peak_luminance; j++) + put_bits(pb, 4, s->mastering_display_actual_peak_luminance[i][j].num * peak_luminance_den / + s->mastering_display_actual_peak_luminance[i][j].den); + } + } + + for (int w = 0; w < s->num_windows; w++) { + put_bits(pb, 1, s->params[w].tone_mapping_flag); + if (s->params[w].tone_mapping_flag) { + put_bits(pb, 12, s->params[w].knee_point_x.num * knee_point_den / s->params[w].knee_point_x.den); + put_bits(pb, 12, s->params[w].knee_point_y.num * knee_point_den / s->params[w].knee_point_y.den); + put_bits(pb, 4, s->params[w].num_bezier_curve_anchors); + for (int i = 0; i < s->params[w].num_bezier_curve_anchors; i++) + put_bits(pb, 10, s->params[w].bezier_curve_anchors[i].num * bezier_anchor_den / + s->params[w].bezier_curve_anchors[i].den); + put_bits(pb, 1, s->params[w].color_saturation_mapping_flag); + if (s->params[w].color_saturation_mapping_flag) + put_bits(pb, 6, s->params[w].color_saturation_weight.num * saturation_weight_den / + s->params[w].color_saturation_weight.den); + } + } + + flush_put_bits(pb); + return buf; +} diff --git a/libavutil/hdr_dynamic_metadata.h b/libavutil/hdr_dynamic_metadata.h index 1f953ef1f5..797a5c64ae 100644 --- a/libavutil/hdr_dynamic_metadata.h +++ b/libavutil/hdr_dynamic_metadata.h @@ -21,6 +21,7 @@ #ifndef AVUTIL_HDR_DYNAMIC_METADATA_H #define AVUTIL_HDR_DYNAMIC_METADATA_H +#include "buffer.h" #include "frame.h" #include "rational.h" @@ -351,4 +352,14 @@ AVDynamicHDRPlus *av_dynamic_hdr_plus_create_side_data(AVFrame *frame); int av_dynamic_hdr_plus_from_t35(AVDynamicHDRPlus *s, const uint8_t *data, int size); +/** + * Serialize dynamic HDR10+ metadata to a user data registered ITU-T T.35 buffer, + * excluding the country code and beginning with the terminal provider code. + * @param s A pointer containing the decoded AVDynamicHDRPlus structure. + * + * @return Pointer to an AVBufferRef containing the raw ITU-T T.35 representation + * of the HDR10+ metadata if succeed, or NULL if buffer allocation fails. + */ +AVBufferRef *av_dynamic_hdr_plus_to_t35(AVDynamicHDRPlus *s); + #endif /* AVUTIL_HDR_DYNAMIC_METADATA_H */ diff --git a/libavutil/version.h b/libavutil/version.h index 900b798971..7635672985 100644 --- a/libavutil/version.h +++ b/libavutil/version.h @@ -79,7 +79,7 @@ */ #define LIBAVUTIL_VERSION_MAJOR 58 -#define LIBAVUTIL_VERSION_MINOR 3 +#define LIBAVUTIL_VERSION_MINOR 4 #define LIBAVUTIL_VERSION_MICRO 100 #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
Fixed brace style and moved inline buffer size calculation comments to a single block at the top. Signed-off-by: Raphaël Zumer <rzumer@tebako.net> --- libavutil/hdr_dynamic_metadata.c | 142 +++++++++++++++++++++++++++++++ libavutil/hdr_dynamic_metadata.h | 11 +++ libavutil/version.h | 2 +- 3 files changed, 154 insertions(+), 1 deletion(-)