Re: [FFmpeg-devel] [PATCH 2/2] avutil: add HDR10+ dynamic metadata serialization function

[Leo Izen <leo.izen@gmail.com>]

On 3/2/23 14:25, Raphaël Zumer wrote:
> Signed-off-by: Raphaël Zumer <rzumer@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)


_______________________________________________
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".