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

Co-authored-by: Mohammad Izadi <moh.izadi@gmail.com>
Signed-off-by: Raphaël Zumer <rzumer@tebako.net>
---
 doc/APIchanges                   |   5 ++
 libavutil/hdr_dynamic_metadata.c | 145 +++++++++++++++++++++++++++++++
 libavutil/hdr_dynamic_metadata.h |  13 +++
 libavutil/version.h              |   2 +-
 4 files changed, 164 insertions(+), 1 deletion(-)

On 3/13/2023 6:39 PM, Raphaël Zumer wrote:
> Co-authored-by: Mohammad Izadi <moh.izadi@gmail.com>
> Signed-off-by: Raphaël Zumer <rzumer@tebako.net>
> ---
>   doc/APIchanges                   |   5 ++
>   libavutil/hdr_dynamic_metadata.c | 145 +++++++++++++++++++++++++++++++
>   libavutil/hdr_dynamic_metadata.h |  13 +++
>   libavutil/version.h              |   2 +-
>   4 files changed, 164 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 14737223cb..3a61d61931 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -2,6 +2,11 @@ The last version increases of all libraries were on 2023-02-09
>   
>   API changes, most recent first:
>   
> +2023-03-13 - xxxxxxxxxx - lavu 58.4.100 - hdr_dynamic_metadata.h
> +  Add av_dynamic_hdr_plus_from_t35() and av_dynamic_hdr_plus_to_t35()
> +  functions to convert between raw T.35 payloads containing dynamic
> +  HDR10+ metadata and their parsed representations as AVDynamicHDRPlus.
> +
>   2023-03-02 - xxxxxxxxxx - lavc 60.6.100 - avcodec.h
>     Add FF_PROFILE_EAC3_DDP_ATMOS, FF_PROFILE_TRUEHD_ATMOS,
>     FF_PROFILE_DTS_HD_MA_X and FF_PROFILE_DTS_HD_MA_X_IMAX.
> diff --git a/libavutil/hdr_dynamic_metadata.c b/libavutil/hdr_dynamic_metadata.c
> index 98f399b032..9e35d929c7 100644
> --- a/libavutil/hdr_dynamic_metadata.c
> +++ b/libavutil/hdr_dynamic_metadata.c
> @@ -225,3 +225,148 @@ int av_dynamic_hdr_plus_from_t35(AVDynamicHDRPlus *s, const uint8_t *data,
>   
>       return 0;
>   }
> +
> +int av_dynamic_hdr_plus_to_t35(uint8_t **data, size_t *size, const AVDynamicHDRPlus *s)

I'll change the signature to (const AVDynamicHDRPlus *s, uint8_t **data, 
size_t *size) to have the struct as first argument before pushing if you 
don't mind. It's more in line with the usual order of arguments in the 
project, and also with av_dynamic_hdr_plus_from_t35().

Raphaël Zumer:
> Co-authored-by: Mohammad Izadi <moh.izadi@gmail.com>
> Signed-off-by: Raphaël Zumer <rzumer@tebako.net>
> ---
>  doc/APIchanges                   |   5 ++
>  libavutil/hdr_dynamic_metadata.c | 145 +++++++++++++++++++++++++++++++
>  libavutil/hdr_dynamic_metadata.h |  13 +++
>  libavutil/version.h              |   2 +-
>  4 files changed, 164 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 14737223cb..3a61d61931 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -2,6 +2,11 @@ The last version increases of all libraries were on 2023-02-09
>  
>  API changes, most recent first:
>  
> +2023-03-13 - xxxxxxxxxx - lavu 58.4.100 - hdr_dynamic_metadata.h
> +  Add av_dynamic_hdr_plus_from_t35() and av_dynamic_hdr_plus_to_t35()
> +  functions to convert between raw T.35 payloads containing dynamic
> +  HDR10+ metadata and their parsed representations as AVDynamicHDRPlus.
> +
>  2023-03-02 - xxxxxxxxxx - lavc 60.6.100 - avcodec.h
>    Add FF_PROFILE_EAC3_DDP_ATMOS, FF_PROFILE_TRUEHD_ATMOS,
>    FF_PROFILE_DTS_HD_MA_X and FF_PROFILE_DTS_HD_MA_X_IMAX.
> diff --git a/libavutil/hdr_dynamic_metadata.c b/libavutil/hdr_dynamic_metadata.c
> index 98f399b032..9e35d929c7 100644
> --- a/libavutil/hdr_dynamic_metadata.c
> +++ b/libavutil/hdr_dynamic_metadata.c
> @@ -225,3 +225,148 @@ int av_dynamic_hdr_plus_from_t35(AVDynamicHDRPlus *s, const uint8_t *data,
>  
>      return 0;
>  }
> +
> +int av_dynamic_hdr_plus_to_t35(uint8_t **data, size_t *size, const AVDynamicHDRPlus *s)
> +{
> +    uint8_t *buf;
> +    size_t size_bits, size_bytes;
> +    PutBitContext pbc, *pb = &pbc;
> +
> +    if (!data || !size || !s)

size being mandatory is different from similar APIs. There is even a
usecase without size: If you simply feed this to something that expects
the data to be serialized and trust the data to be complete, you don't
need the size.

> +        return AVERROR(EINVAL);
> +
> +    /**
> +     * 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_malloc(size_bytes);

You are allocating without any padding. This implies that one could not
use this buffer with our GetBit-API or in other places where one needed
a padded buffer.

> +    if (!buf)
> +        return AVERROR(ENOMEM);
> +
> +    init_put_bits(pb, buf, 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);
> +
> +    *data = buf;
> +    *size = size_bytes;
> +    return 0;
> +}
> diff --git a/libavutil/hdr_dynamic_metadata.h b/libavutil/hdr_dynamic_metadata.h
> index 1f953ef1f5..f2d78051ab 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,16 @@ 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 data A pointer to a byte buffer to be allocated and filled
> + *             with the serialized metadata.
> + * @param size A pointer to a size to be set to the returned buffer's size.
> + * @param s A pointer containing the decoded AVDynamicHDRPlus structure.
> + *
> + * @return 0 if succeed. Otherwise, returns the appropriate AVERROR.
> + */
> +int av_dynamic_hdr_plus_to_t35(uint8_t **data, size_t *size, const 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/13/23 18:35, Andreas Rheinhardt wrote:
> size being mandatory is different from similar APIs. There is even a
> usecase without size: If you simply feed this to something that expects
> the data to be serialized and trust the data to be complete, you don't
> need the size.
OK, I'll amend that.
> You are allocating without any padding. This implies that one could not
> use this buffer with our GetBit-API or in other places where one needed
> a padded buffer.

Is there any comparable code that does that? I feel like padding a buffer should be the responsibility of the caller for a public function, otherwise the user has to be aware of the padding to avoid embedding extra payload bytes accidentally (even though it is negligible in size), it is an extra manipulation if padding is not needed, and requires including an extra file to access the padding size.

RZ

On 3/13/2023 8:19 PM, Raphaël Zumer wrote:
> On 3/13/23 18:35, Andreas Rheinhardt wrote:
>> size being mandatory is different from similar APIs. There is even a
>> usecase without size: If you simply feed this to something that expects
>> the data to be serialized and trust the data to be complete, you don't
>> need the size.
> OK, I'll amend that.
>> You are allocating without any padding. This implies that one could not
>> use this buffer with our GetBit-API or in other places where one needed
>> a padded buffer.
> 
> Is there any comparable code that does that? I feel like padding a buffer should be the responsibility of the caller for a public function, otherwise the user has to be aware of the padding to avoid embedding extra payload bytes accidentally (even though it is negligible in size), it is an extra manipulation if padding is not needed, and requires including an extra file to access the padding size.

The returned value in *size would not take the padding bytes into 
account, so no way to include them accidentally anywhere. You either 
know the size of the serialized data and trust the buffer is complete as 
Andreas mentioned, or you read the size returned by the function. In 
either case, the padding bytes are never considered.

And since av_dynamic_hdr_plus_from_t35() uses GetBitContext to parse the 
buffer you feed to it, the output of the serialization function should 
ideally work with it without issues.

> 
> RZ
> 
> _______________________________________________
> 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/13/23 19:25, James Almer wrote:
>>> You are allocating without any padding. This implies that one could not
>>> use this buffer with our GetBit-API or in other places where one needed
>>> a padded buffer.
>> Is there any comparable code that does that? I feel like padding a buffer should be the responsibility of the caller for a public function, otherwise the user has to be aware of the padding to avoid embedding extra payload bytes accidentally (even though it is negligible in size), it is an extra manipulation if padding is not needed, and requires including an extra file to access the padding size.
> The returned value in *size would not take the padding bytes into 
> account, so no way to include them accidentally anywhere. You either 
> know the size of the serialized data and trust the buffer is complete as 
> Andreas mentioned, or you read the size returned by the function. In 
> either case, the padding bytes are never considered.

Right, I did not think it through. Will do.

RZ