[FFmpeg-devel] [PATCH 2/2] avutil: add HDR10+ dynamic metadata serialization function
Leo Izen
leo.izen at gmail.com
Thu Mar 2 22:24:01 EET 2023
On 3/2/23 14:25, Raphaël Zumer wrote:
> Signed-off-by: Raphaël Zumer <rzumer at tebako.net>
> ---
> libavutil/hdr_dynamic_metadata.c | 146 +++++++++++++++++++++++++++++++
> libavutil/hdr_dynamic_metadata.h | 11 +++
> libavutil/version.h | 2 +-
> 3 files changed, 158 insertions(+), 1 deletion(-)
>
Why not put this in avcodec/dynamic_hdr10_plus.c? You reference
put_bits.h which is in avcodec, so that can possibly be an issue, even
though it is inlined (i.e. it sends the wrong message since avutil is
supposed to not depend on avcodec).
> diff --git a/libavutil/hdr_dynamic_metadata.c b/libavutil/hdr_dynamic_metadata.c
> index 98f399b032..39a7886a2e 100644
> --- a/libavutil/hdr_dynamic_metadata.c
> +++ b/libavutil/hdr_dynamic_metadata.c
> @@ -225,3 +225,149 @@ int av_dynamic_hdr_plus_from_t35(AVDynamicHDRPlus *s, const uint8_t *data,
>
> return 0;
> }
> +
> +AVBufferRef *av_dynamic_hdr_plus_to_t35(AVDynamicHDRPlus *s)
> +{
av_dynamic_hdr_plus_from_t35 returns an int status code and takes a
pointer as an argument, is there any particular reason you didn't mirror
user interface here?
> + 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:
> + size_bits =
> + // 56 bits for the header, minus 8-bit excluded country code
> + 48 +
> + // 2 bits for num_windows
> + 2 +
> + // 937 bits for window geometry for each window above 1
> + FFMAX((s->num_windows - 1), 0) * 937 +
> + // 27 bits for targeted_system_display_maximum_luminance
> + 27 +
> + // 1-3855 bits for targeted system display peak luminance information
> + 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) +
> + // 0-442 bits for intra-window pixel distribution information
> + s->num_windows * 82;
This sequence above is difficult to read due to the inline // comments.
It should be more readable to just have the entire expression be
contiguous with a /* */ multiline block comment above it explaining each
item.
> + for (int w = 0; w < s->num_windows; w++) {
> + size_bits += s->params[w].num_distribution_maxrgb_percentiles * 24;
> + }
Likewise, another code style issue, don't use {} to enclose a single
line unless it's unavoidable. This occurs in several places in this patch.
> + // 1-3855 bits for mastering display peak luminance information
> + 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) +
> + // 0-537 bits for per-window tonemapping information
> + 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;
> + }
> + }
> + // 0-21 bits for per-window color saturation mapping information
> + 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;
> +
If you update this to match the status code, this becomes AVERROR(ENOMEM);
> +
> + 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, \
- Leo Izen (thebombzen)
More information about the ffmpeg-devel
mailing list