From: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
To: ffmpeg-devel@ffmpeg.org
Subject: Re: [FFmpeg-devel] [PATCH] avformat/matroska: Support HDR10+ metadata in Matroska.
Date: Wed, 26 Oct 2022 02:16:09 +0200
Message-ID: <AS8P250MB0744A5953FBB124FCA4F9A058F309@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <20220908012214.1556959-1-izadi@google.com>
Mohammad Izadi:
> The fate test file can be found here: https://drive.google.com/file/d/1jGW3f94rglLfr5WGmMQe3SEnp1YkbMRy/view?usp=drivesdk
> The video file needs to be copied to fate-suite/mkv/
This does not describe the patch at all and should therefore not be part
of the commit message.
> ---
Instead put it here.
> libavcodec/dynamic_hdr10_plus.c | 269 +++++++++++++++++---
The changes to libavcodec and Matroska need to be in separate patches.
> libavcodec/dynamic_hdr10_plus.h | 26 +-
This is a private header and therefore must not be used for public symbols.
> libavformat/matroska.h | 5 +
> libavformat/matroskadec.c | 30 ++-
> libavformat/matroskaenc.c | 44 +++-
> tests/fate/matroska.mak | 6 +
> tests/ref/fate/matroska-hdr10-plus-metadata | 74 ++++++
> 7 files changed, 397 insertions(+), 57 deletions(-)
> create mode 100644 tests/ref/fate/matroska-hdr10-plus-metadata
>
> diff --git a/libavcodec/dynamic_hdr10_plus.c b/libavcodec/dynamic_hdr10_plus.c
> index 34a44aac65..d05f211c94 100644
> --- a/libavcodec/dynamic_hdr10_plus.c
> +++ b/libavcodec/dynamic_hdr10_plus.c
> @@ -18,6 +18,12 @@
>
> #include "dynamic_hdr10_plus.h"
> #include "get_bits.h"
> +#include "put_bits.h"
> +
> +static const uint8_t usa_country_code = 0xB5;
> +static const uint16_t smpte_provider_code = 0x003C;
> +static const uint16_t smpte2094_40_provider_oriented_code = 0x0001;
> +static const uint16_t smpte2094_40_application_identifier = 0x04;
>
We typically use defines or enums for such things; C does not have
constexpr.
> static const int64_t luminance_den = 1;
> static const int32_t peak_luminance_den = 15;
> @@ -27,8 +33,8 @@ static const int32_t knee_point_den = 4095;
> static const int32_t bezier_anchor_den = 1023;
> static const int32_t saturation_weight_den = 8;
>
> -int ff_parse_itu_t_t35_to_dynamic_hdr10_plus(AVDynamicHDRPlus *s, const uint8_t *data,
> - int size)
> +int ff_parse_itu_t_t35_to_dynamic_hdr10_plus(AVDynamicHDRPlus* s, const uint8_t* data,
> + int size)
> {
> GetBitContext gbc, *gb = &gbc;
> int ret;
> @@ -40,10 +46,12 @@ int ff_parse_itu_t_t35_to_dynamic_hdr10_plus(AVDynamicHDRPlus *s, const uint8_t
> if (ret < 0)
> return ret;
>
> - if (get_bits_left(gb) < 10)
> + if (get_bits_left(gb) < 8)
> return AVERROR_INVALIDDATA;
> + s->application_version = get_bits(gb, 8);
>
> - s->application_version = get_bits(gb, 8);
> + if (get_bits_left(gb) < 2)
> + return AVERROR_INVALIDDATA;
These changes add an unnecessary branch.
> s->num_windows = get_bits(gb, 2);
>
> if (s->num_windows < 1 || s->num_windows > 3) {
> @@ -56,15 +64,11 @@ int ff_parse_itu_t_t35_to_dynamic_hdr10_plus(AVDynamicHDRPlus *s, const uint8_t
> for (int w = 1; w < s->num_windows; w++) {
> // The corners are set to absolute coordinates here. They should be
> // converted to the relative coordinates (in [0, 1]) in the decoder.
All the following changes to this functions are cosmetic and should be
removed. Mixing cosmetic and non-cosmetic changes make patches harder to
review.
> - AVHDRPlusColorTransformParams *params = &s->params[w];
> - params->window_upper_left_corner_x =
> - (AVRational){get_bits(gb, 16), 1};
> - params->window_upper_left_corner_y =
> - (AVRational){get_bits(gb, 16), 1};
> - params->window_lower_right_corner_x =
> - (AVRational){get_bits(gb, 16), 1};
> - params->window_lower_right_corner_y =
> - (AVRational){get_bits(gb, 16), 1};
> + AVHDRPlusColorTransformParams* params = &s->params[w];
> + params->window_upper_left_corner_x = (AVRational) { get_bits(gb, 16), 1 };
> + params->window_upper_left_corner_y = (AVRational) { get_bits(gb, 16), 1 };
> + params->window_lower_right_corner_x = (AVRational) { get_bits(gb, 16), 1 };
> + params->window_lower_right_corner_y = (AVRational) { get_bits(gb, 16), 1 };
>
> params->center_of_ellipse_x = get_bits(gb, 16);
> params->center_of_ellipse_y = get_bits(gb, 16);
> @@ -78,8 +82,7 @@ int ff_parse_itu_t_t35_to_dynamic_hdr10_plus(AVDynamicHDRPlus *s, const uint8_t
> if (get_bits_left(gb) < 28)
> return AVERROR_INVALIDDATA;
>
> - s->targeted_system_display_maximum_luminance =
> - (AVRational){get_bits_long(gb, 27), luminance_den};
> + s->targeted_system_display_maximum_luminance = (AVRational) { get_bits_long(gb, 27), luminance_den };
> s->targeted_system_display_actual_peak_luminance_flag = get_bits1(gb);
>
> if (s->targeted_system_display_actual_peak_luminance_flag) {
> @@ -99,22 +102,19 @@ int ff_parse_itu_t_t35_to_dynamic_hdr10_plus(AVDynamicHDRPlus *s, const uint8_t
>
> for (int i = 0; i < rows; i++) {
> for (int j = 0; j < cols; j++) {
> - s->targeted_system_display_actual_peak_luminance[i][j] =
> - (AVRational){get_bits(gb, 4), peak_luminance_den};
> + s->targeted_system_display_actual_peak_luminance[i][j] = (AVRational) { get_bits(gb, 4), peak_luminance_den };
> }
> }
> }
> for (int w = 0; w < s->num_windows; w++) {
> - AVHDRPlusColorTransformParams *params = &s->params[w];
> + AVHDRPlusColorTransformParams* params = &s->params[w];
> if (get_bits_left(gb) < (3 * 17 + 17 + 4))
> return AVERROR_INVALIDDATA;
>
> for (int i = 0; i < 3; i++) {
> - params->maxscl[i] =
> - (AVRational){get_bits(gb, 17), rgb_den};
> + params->maxscl[i] = (AVRational) { get_bits(gb, 17), rgb_den };
> }
> - params->average_maxrgb =
> - (AVRational){get_bits(gb, 17), rgb_den};
> + params->average_maxrgb = (AVRational) { get_bits(gb, 17), rgb_den };
> params->num_distribution_maxrgb_percentiles = get_bits(gb, 4);
>
> if (get_bits_left(gb) <
> @@ -123,14 +123,13 @@ int ff_parse_itu_t_t35_to_dynamic_hdr10_plus(AVDynamicHDRPlus *s, const uint8_t
>
> for (int i = 0; i < params->num_distribution_maxrgb_percentiles; i++) {
> params->distribution_maxrgb[i].percentage = get_bits(gb, 7);
> - params->distribution_maxrgb[i].percentile =
> - (AVRational){get_bits(gb, 17), rgb_den};
> + params->distribution_maxrgb[i].percentile = (AVRational) { get_bits(gb, 17), rgb_den };
> }
>
> if (get_bits_left(gb) < 10)
> return AVERROR_INVALIDDATA;
>
> - params->fraction_bright_pixels = (AVRational){get_bits(gb, 10), fraction_pixel_den};
> + params->fraction_bright_pixels = (AVRational) { get_bits(gb, 10), fraction_pixel_den };
> }
> if (get_bits_left(gb) < 1)
> return AVERROR_INVALIDDATA;
> @@ -152,14 +151,13 @@ int ff_parse_itu_t_t35_to_dynamic_hdr10_plus(AVDynamicHDRPlus *s, const uint8_t
>
> for (int i = 0; i < rows; i++) {
> for (int j = 0; j < cols; j++) {
> - s->mastering_display_actual_peak_luminance[i][j] =
> - (AVRational){get_bits(gb, 4), peak_luminance_den};
> + s->mastering_display_actual_peak_luminance[i][j] = (AVRational) { get_bits(gb, 4), peak_luminance_den };
> }
> }
> }
>
> for (int w = 0; w < s->num_windows; w++) {
> - AVHDRPlusColorTransformParams *params = &s->params[w];
> + AVHDRPlusColorTransformParams* params = &s->params[w];
This is not the codestyle preferred by FFmpeg (rationale: The '*'
belongs to the variable, not the type. After all "T* foo, bar;" defines
foo as pointer to type T and bar as variable of type T, not as pointer
to T). And anyway, it is a cosmetic change.
> if (get_bits_left(gb) < 1)
> return AVERROR_INVALIDDATA;
>
> @@ -168,18 +166,15 @@ int ff_parse_itu_t_t35_to_dynamic_hdr10_plus(AVDynamicHDRPlus *s, const uint8_t
> if (get_bits_left(gb) < 28)
> return AVERROR_INVALIDDATA;
>
> - params->knee_point_x =
> - (AVRational){get_bits(gb, 12), knee_point_den};
> - params->knee_point_y =
> - (AVRational){get_bits(gb, 12), knee_point_den};
> + params->knee_point_x = (AVRational) { get_bits(gb, 12), knee_point_den };
> + params->knee_point_y = (AVRational) { get_bits(gb, 12), knee_point_den };
> params->num_bezier_curve_anchors = get_bits(gb, 4);
>
> if (get_bits_left(gb) < (params->num_bezier_curve_anchors * 10))
> return AVERROR_INVALIDDATA;
>
> for (int i = 0; i < params->num_bezier_curve_anchors; i++) {
> - params->bezier_curve_anchors[i] =
> - (AVRational){get_bits(gb, 10), bezier_anchor_den};
> + params->bezier_curve_anchors[i] = (AVRational) { get_bits(gb, 10), bezier_anchor_den };
> }
> }
>
> @@ -196,3 +191,209 @@ int ff_parse_itu_t_t35_to_dynamic_hdr10_plus(AVDynamicHDRPlus *s, const uint8_t
>
> return 0;
> }
> +
> +int av_parse_full_itu_t_t35_to_dynamic_hdr10_plus(AVDynamicHDRPlus* s, const uint8_t* data,
> + int size)
> +{
> + uint8_t country_code;
> + uint16_t provider_code;
> + uint16_t provider_oriented_code;
> + uint8_t application_identifier;
> + GetBitContext gbc, *gb = &gbc;
> + int ret, offset;
> +
> + if (!s)
> + return AVERROR(ENOMEM);
?
> +
> + if (size < 7)
> + return AVERROR_INVALIDDATA;
> +
> + ret = init_get_bits8(gb, data, size);
> + if (ret < 0)
> + return ret;
> +
> + country_code = get_bits(gb, 8);
> + provider_code = get_bits(gb, 16);
> +
> + if (country_code != usa_country_code ||
> + provider_code != smpte_provider_code)
> + return AVERROR_INVALIDDATA;
> +
> + // A/341 Amendment – 2094-40
> + provider_oriented_code = get_bits(gb, 16);
> + application_identifier = get_bits(gb, 8);
> + if (provider_oriented_code != smpte2094_40_provider_oriented_code ||
> + application_identifier != smpte2094_40_application_identifier)
> + return AVERROR_INVALIDDATA;
> +
> + offset = get_bits_count(gb) / 8;
This function only performs byte-aligned reads and does not need to use
the GetBit API at all.
> +
> + return ff_parse_itu_t_t35_to_dynamic_hdr10_plus(s, gb->buffer + offset, size - offset);
> +}
> +
> +static int ff_itu_t_t35_buffer_size(const AVDynamicHDRPlus* s)
ff_ is our prefix for functions with external linkage (as well as
sometimes for static inline functions declared in headers).
> +{
> + int bit_count = 0;
> + int w, size;
> +
> + if (!s)
> + return 0;
> +
> + // 7 bytes for country code, provider code, and user identifier.
> + bit_count += 56;
> +
> + if (s->num_windows < 1 || s->num_windows > 3)
> + return 0;
> + // Count bits for window params.
> + bit_count += 2 + ((19 * 8 + 1) * (s->num_windows - 1));
> +
> + bit_count += 28;
> + if (s->targeted_system_display_actual_peak_luminance_flag) {
> + int rows, cols;
> + rows = s->num_rows_targeted_system_display_actual_peak_luminance;
> + cols = s->num_cols_targeted_system_display_actual_peak_luminance;
> + if (((rows < 2) || (rows > 25)) || ((cols < 2) || (cols > 25)))
> + return 0;
> +
> + bit_count += (10 + rows * cols * 4);
> + }
> + for (w = 0; w < s->num_windows; w++) {
> + bit_count += (3 * 17 + 17 + 4 + 10) + (s->params[w].num_distribution_maxrgb_percentiles * 24);
> + }
> + bit_count++;
> +
> + if (s->mastering_display_actual_peak_luminance_flag) {
> + int rows, cols;
> + rows = s->num_rows_mastering_display_actual_peak_luminance;
> + cols = s->num_cols_mastering_display_actual_peak_luminance;
> + if (((rows < 2) || (rows > 25)) || ((cols < 2) || (cols > 25)))
> + return 0;
> +
> + bit_count += (10 + rows * cols * 4);
> + }
> +
> + for (w = 0; w < s->num_windows; w++) {
> + bit_count++;
> + if (s->params[w].tone_mapping_flag)
> + bit_count += (28 + s->params[w].num_bezier_curve_anchors * 10);
> +
> + bit_count++;
> + if (s->params[w].color_saturation_mapping_flag)
> + bit_count += 6;
> + }
> + size = bit_count / 8;
> + if (bit_count % 8 != 0)
> + size++;
> + return size;
> +}
> +
> +int av_write_dynamic_hdr10_plus_to_full_itu_t_t35(const AVDynamicHDRPlus* s, uint8_t** data, size_t* size)
> +{
> + int w, i, j;
> + PutBitContext pbc, *pb = &pbc;
> +
> + if (!s || !size)
> + return AVERROR(EINVAL);
> +
> + *size = ff_itu_t_t35_buffer_size(s);
> + if (*size <= 0)
> + return AVERROR(EINVAL);
> + *data = av_mallocz(*size);
> + init_put_bits(pb, *data, *size);
> + if (put_bits_left(pb) < *size) {
> + av_freep(data);
> + return AVERROR(EINVAL);
> + }
> + put_bits(pb, 8, usa_country_code);
> +
> + put_bits(pb, 16, smpte_provider_code);
> + put_bits(pb, 16, smpte2094_40_provider_oriented_code);
> + put_bits(pb, 8, smpte2094_40_application_identifier);
> + put_bits(pb, 8, s->application_version);
> +
> + put_bits(pb, 2, s->num_windows);
> +
> + for (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) {
> + int rows, cols;
> + rows = s->num_rows_targeted_system_display_actual_peak_luminance;
> + cols = s->num_cols_targeted_system_display_actual_peak_luminance;
> + put_bits(pb, 5, rows);
> + put_bits(pb, 5, cols);
> + for (i = 0; i < rows; i++) {
> + for (j = 0; j < cols; 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 (w = 0; w < s->num_windows; w++) {
> + for (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 (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) {
> + int rows, cols;
> + rows = s->num_rows_mastering_display_actual_peak_luminance;
> + cols = s->num_cols_mastering_display_actual_peak_luminance;
> + put_bits(pb, 5, rows);
> + put_bits(pb, 5, cols);
> + for (i = 0; i < rows; i++) {
> + for (j = 0; j < cols; 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 (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 (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 0;
> +}
> diff --git a/libavcodec/dynamic_hdr10_plus.h b/libavcodec/dynamic_hdr10_plus.h
> index cd7acf0432..d52c3e552d 100644
> --- a/libavcodec/dynamic_hdr10_plus.h
> +++ b/libavcodec/dynamic_hdr10_plus.h
> @@ -29,7 +29,29 @@
> *
> * @return 0 if succeed. Otherwise, returns the appropriate AVERROR.
> */
> -int ff_parse_itu_t_t35_to_dynamic_hdr10_plus(AVDynamicHDRPlus *s, const uint8_t *data,
> - int size);
> +int ff_parse_itu_t_t35_to_dynamic_hdr10_plus(AVDynamicHDRPlus* s, const uint8_t* data,
> + int size);
> +
> +/**
> + * Parse the user data registered ITU-T T.35 with header to AVDynamicHDRPlus. At first check
> + * the header if the provider code is SMPTE-2094-40. Then will parse the data to AVDynamicHDRPlus.
> + * @param s A pointer containing the decoded AVDynamicHDRPlus structure.
> + * @param data The byte array containing the raw ITU-T T.35 data with header.
> + * @param size Size of the data array in bytes.
> + *
> + * @return 0 if succeed. Otherwise, returns the appropriate AVERROR.
> + */
> +int av_parse_full_itu_t_t35_to_dynamic_hdr10_plus(AVDynamicHDRPlus* s, const uint8_t* data,
> + int size);
> +
> +/**
> + * Encode and write AVDynamicHDRPlus to the user data registered ITU-T T.3 with header (containing the provider code).
> + * @param s A pointer containing the AVDynamicHDRPlus structure.
> + * @param data The byte array containing the raw ITU-T T.35 data with header.
> + * @param size The size of the raw ITU-T T.35 data.
> + *
> + * @return 0 if succeed. Otherwise, returns the appropriate AVERROR.
> + */
> +int av_write_dynamic_hdr10_plus_to_full_itu_t_t35(const AVDynamicHDRPlus* s, uint8_t** data, size_t* size);
>
> #endif /* AVCODEC_DYNAMIC_HDR10_PLUS_H */
> diff --git a/libavformat/matroska.h b/libavformat/matroska.h
> index 45077ed33f..c3cf9eef53 100644
> --- a/libavformat/matroska.h
> +++ b/libavformat/matroska.h
> @@ -358,6 +358,11 @@ typedef enum {
> MATROSKA_VIDEO_PROJECTION_TYPE_MESH = 3,
> } MatroskaVideoProjectionType;
>
> +typedef enum {
> + MATROSKA_BLOCK_ADD_ID_DEFAULT = 0,
The default value of BlockAddID is actually 1. 0 is an invalid value for
BlockAddID.
> + MATROSKA_BLOCK_ADD_ID_DYNAMIC_HDR10_PLUS = 4,
> +} MatroskaBlockAddID;
> +
> /*
> * Matroska Codec IDs, strings
> */
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 16a3e93611..7fa4483ba4 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -50,6 +50,7 @@
> #include "libavutil/spherical.h"
>
> #include "libavcodec/bytestream.h"
> +#include "libavcodec/dynamic_hdr10_plus.h"
> #include "libavcodec/flac.h"
> #include "libavcodec/mpeg4audio.h"
> #include "libavcodec/packet_internal.h"
> @@ -3636,15 +3637,28 @@ static int matroska_parse_frame(MatroskaDemuxContext *matroska,
> pkt->stream_index = st->index;
>
> if (additional_size > 0) {
> - uint8_t *side_data = av_packet_new_side_data(pkt,
> - AV_PKT_DATA_MATROSKA_BLOCKADDITIONAL,
> - additional_size + 8);
> - if (!side_data) {
> - av_packet_unref(pkt);
> - return AVERROR(ENOMEM);
> + if (additional_id == MATROSKA_BLOCK_ADD_ID_DYNAMIC_HDR10_PLUS) {
> + AVDynamicHDRPlus hdr10_plus;
> + if (!av_parse_full_itu_t_t35_to_dynamic_hdr10_plus(&hdr10_plus, additional, additional_size)) {
> + uint8_t *side_data = av_packet_new_side_data(pkt, AV_PKT_DATA_DYNAMIC_HDR10_PLUS, sizeof(hdr10_plus));
> + if (!side_data) {
> + av_packet_unref(pkt);
> + av_free(pkt);
Where did you get that from?
> + return AVERROR(ENOMEM);
> + }
> + memcpy(side_data, (uint8_t*)(&hdr10_plus), sizeof(hdr10_plus));
> + }
> + } else {
> + uint8_t *side_data = av_packet_new_side_data(pkt,
> + AV_PKT_DATA_MATROSKA_BLOCKADDITIONAL,
> + additional_size + 8);
> + if (!side_data) {
> + av_packet_unref(pkt);
> + return AVERROR(ENOMEM);
> + }
> + AV_WB64(side_data, additional_id);
> + memcpy(side_data + 8, additional, additional_size);
> }
> - AV_WB64(side_data, additional_id);
> - memcpy(side_data + 8, additional, additional_size);
> }
>
> if (discard_padding) {
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index ed1ad5039d..665e3b7ec6 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -57,6 +57,7 @@
> #include "libavutil/stereo3d.h"
>
> #include "libavcodec/av1.h"
> +#include "libavcodec/dynamic_hdr10_plus.h"
> #include "libavcodec/xiph.h"
> #include "libavcodec/mpeg4audio.h"
>
> @@ -2490,7 +2491,7 @@ static int mkv_write_header(AVFormatContext *s)
>
> #if CONFIG_MATROSKA_MUXER
> static int mkv_reformat_h2645(MatroskaMuxContext *mkv, AVIOContext *pb,
> - const AVPacket *pkt, int *size)
> + const AVPacket *pkt, int *size)
Spurious change.
> {
> int ret;
> if (pb) {
> @@ -2591,8 +2592,11 @@ static int mkv_write_block(void *logctx, MatroskaMuxContext *mkv,
> {
> uint8_t *side_data;
> size_t side_data_size;
> - uint64_t additional_id;
> + uint64_t additional_id = 0;
> unsigned track_number = track->track_num;
> + int has_codec_specific_blockmore = 0;
> + uint8_t *hdr10_plus_itu_t_t35;
> + size_t hdr10_plus_itu_t_t35_size = 0;
> EBML_WRITER(9);
>
> mkv->cur_block.track = track;
> @@ -2627,24 +2631,38 @@ static int mkv_write_block(void *logctx, MatroskaMuxContext *mkv,
> ebml_writer_add_sint(&writer, MATROSKA_ID_DISCARDPADDING, discard_padding);
> }
> }
> + side_data = av_packet_get_side_data(pkt,
> + AV_PKT_DATA_DYNAMIC_HDR10_PLUS,
> + &side_data_size);
> + if (side_data && side_data_size > 0)
> + av_write_dynamic_hdr10_plus_to_full_itu_t_t35((AVDynamicHDRPlus*)side_data, &hdr10_plus_itu_t_t35, &hdr10_plus_itu_t_t35_size);
>
> side_data = av_packet_get_side_data(pkt,
> AV_PKT_DATA_MATROSKA_BLOCKADDITIONAL,
> &side_data_size);
> - if (side_data && side_data_size >= 8 &&
> - // Only the Codec-specific BlockMore (id == 1) is currently supported.
> - (additional_id = AV_RB64(side_data)) == 1) {
> + has_codec_specific_blockmore = side_data && side_data_size >= 8 &&
> + (additional_id = AV_RB64(side_data)) == 1;
> + // The Codec-specific BlockMore (id == 1) and HDR10+ (id == 4) are supported.
> + if (has_codec_specific_blockmore || hdr10_plus_itu_t_t35_size > 0) {
> ebml_writer_open_master(&writer, MATROSKA_ID_BLOCKADDITIONS);
> - ebml_writer_open_master(&writer, MATROSKA_ID_BLOCKMORE);
> - /* Until dbc50f8a our demuxer used a wrong default value
> - * of BlockAddID, so we write it unconditionally. */
> - ebml_writer_add_uint(&writer, MATROSKA_ID_BLOCKADDID, additional_id);
> - ebml_writer_add_bin (&writer, MATROSKA_ID_BLOCKADDITIONAL,
> - side_data + 8, side_data_size - 8);
> - ebml_writer_close_master(&writer);
> + if (has_codec_specific_blockmore) {
> + ebml_writer_open_master(&writer, MATROSKA_ID_BLOCKMORE);
> + /* Until dbc50f8a our demuxer used a wrong default value
> + * of BlockAddID, so we write it unconditionally. */
> + ebml_writer_add_uint(&writer, MATROSKA_ID_BLOCKADDID, additional_id);
> + ebml_writer_add_bin (&writer, MATROSKA_ID_BLOCKADDITIONAL,
> + side_data + 8, side_data_size - 8);
> + ebml_writer_close_master(&writer);
> + }
> + if (hdr10_plus_itu_t_t35_size > 0) {
> + ebml_writer_open_master(&writer, MATROSKA_ID_BLOCKMORE);
> + ebml_writer_add_uint(&writer, MATROSKA_ID_BLOCKADDID, MATROSKA_BLOCK_ADD_ID_DYNAMIC_HDR10_PLUS);
> + ebml_writer_add_bin (&writer, MATROSKA_ID_BLOCKADDITIONAL,
> + hdr10_plus_itu_t_t35, hdr10_plus_itu_t_t35_size);
> + ebml_writer_close_master(&writer);
> + }
> ebml_writer_close_master(&writer);
> }
> -
1. You are writing the WebM HDR10+ metadata in Matroska, although this
is not yet specified for Matroska (see
https://github.com/ietf-wg-cellar/matroska-specification/issues/345).
This is invalid.
2. You increased the potential maximum of elements to write, but not the
size of the ebml writer's buffer.
3. Seems like hdr10_plus_itu_t_t35 leaks here (is the allocation even
necessary? Is there an upper bound for the size of the created data?).
4. You are adding lots of checks for whether you should open the
blockadditions master; they are all unnecessary: Open it unconditionally
and close it with ebml_writer_close_or_discard_master() (I added this
function for purposes like this.)
- Andreas
> if (!force_blockgroup && writer.nb_elements == 2) {
> /* Nothing except the BlockGroup + Block. Can use a SimpleBlock. */
> writer.elements++; // Skip the BlockGroup.
> diff --git a/tests/fate/matroska.mak b/tests/fate/matroska.mak
> index 63e81f121b..068f178d9b 100644
> --- a/tests/fate/matroska.mak
> +++ b/tests/fate/matroska.mak
> @@ -148,6 +148,12 @@ FATE_MATROSKA_FFMPEG_FFPROBE-$(call TRANSCODE, PCM_S32LE MP2, MATROSKA, \
> ARESAMPLE_FILTER \
> PCM_S32BE_ENCODER) \
> += fate-matroska-h264-remux
> +
> +# The input file of the following test contains HDR10+ metadata and so this
> +# test tests correct encoding and decoding HDR10+ for VP9/MKV.
> +FATE_MATROSKA_FFPROBE-$(call ALLYES, MATROSKA_DEMUXER) += fate-matroska-hdr10-plus-metadata
> +fate-matroska-hdr10-plus-metadata: CMD = run ffprobe$(PROGSSUF)$(EXESUF) -show_frames -select_streams v -v 0 $(TARGET_SAMPLES)/mkv/hdr10_plus_vp9_sample.mkv
> +
This does not test muxing HDR10+ at all, as it just runs ffprobe on an
already existing file. You would need to use a "transcode" test to test
muxing.
> fate-matroska-h264-remux: CMD = transcode mpegts $(TARGET_SAMPLES)/h264/h264_intra_first-small.ts matroska "-map 0:0 -map 0 -c:v copy -sar:0 3:4 -bsf:v:1 h264_metadata=aud=remove:delete_filler=1 -disposition:v +hearing_impaired -af aresample -c:a:0 pcm_s32le -c:a:1 pcm_s32be -disposition:a:0 original -metadata:s:a:0 title=swedish_silence -metadata:s:a:1 title=norwegian_silence -disposition:a:1 dub" "-map 0:v" "-show_entries stream=index,codec_name:stream_tags=title,language"
>
> # Tests writing BlockAdditional and BlockGroups with ReferenceBlock elements;
> diff --git a/tests/ref/fate/matroska-hdr10-plus-metadata b/tests/ref/fate/matroska-hdr10-plus-metadata
> new file mode 100644
> index 0000000000..0f9ad331ad
> --- /dev/null
> +++ b/tests/ref/fate/matroska-hdr10-plus-metadata
> @@ -0,0 +1,74 @@
> +[FRAME]
> +media_type=video
> +stream_index=0
> +key_frame=1
> +pts=0
> +pts_time=0.000000
> +pkt_dts=0
> +pkt_dts_time=0.000000
> +best_effort_timestamp=0
> +best_effort_timestamp_time=0.000000
> +pkt_duration=40
> +pkt_duration_time=0.040000
> +duration=40
> +duration_time=0.040000
> +pkt_pos=561
> +pkt_size=13350
> +width=1280
> +height=720
> +pix_fmt=yuv420p10le
> +sample_aspect_ratio=1:1
> +pict_type=I
> +coded_picture_number=0
> +display_picture_number=0
> +interlaced_frame=0
> +top_field_first=0
> +repeat_pict=0
> +color_range=tv
> +color_space=unknown
> +color_primaries=unknown
> +color_transfer=unknown
> +chroma_location=unspecified
> +[SIDE_DATA]
> +side_data_type=HDR Dynamic Metadata SMPTE2094-40 (HDR10+)
> +application version=1
> +num_windows=1
> +targeted_system_display_maximum_luminance=400/1
> +maxscl=3340/100000
> +maxscl=2870/100000
> +maxscl=2720/100000
> +average_maxrgb=510/100000
> +num_distribution_maxrgb_percentiles=9
> +distribution_maxrgb_percentage=1
> +distribution_maxrgb_percentile=30/100000
> +distribution_maxrgb_percentage=5
> +distribution_maxrgb_percentile=2940/100000
> +distribution_maxrgb_percentage=10
> +distribution_maxrgb_percentile=255/100000
> +distribution_maxrgb_percentage=25
> +distribution_maxrgb_percentile=70/100000
> +distribution_maxrgb_percentage=50
> +distribution_maxrgb_percentile=1340/100000
> +distribution_maxrgb_percentage=75
> +distribution_maxrgb_percentile=1600/100000
> +distribution_maxrgb_percentage=90
> +distribution_maxrgb_percentile=1850/100000
> +distribution_maxrgb_percentage=95
> +distribution_maxrgb_percentile=1950/100000
> +distribution_maxrgb_percentage=99
> +distribution_maxrgb_percentile=2940/100000
> +fraction_bright_pixels=1/1000
> +knee_point_x=0/4095
> +knee_point_y=0/4095
> +num_bezier_curve_anchors=9
> +bezier_curve_anchors=102/1023
> +bezier_curve_anchors=205/1023
> +bezier_curve_anchors=307/1023
> +bezier_curve_anchors=410/1023
> +bezier_curve_anchors=512/1023
> +bezier_curve_anchors=614/1023
> +bezier_curve_anchors=717/1023
> +bezier_curve_anchors=819/1023
> +bezier_curve_anchors=922/1023
> +[/SIDE_DATA]
> +[/FRAME]
_______________________________________________
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".
next prev parent reply other threads:[~2022-10-26 0:16 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20220417172658.GN3529341@pb2>
2022-09-06 21:47 ` Mohammad Izadi
2022-09-06 22:29 ` Michael Niedermayer
2022-09-07 13:12 ` Derek Buitenhuis
2022-09-08 0:07 ` Mohammad Izadi
2022-09-08 1:22 ` Mohammad Izadi
2022-10-26 0:16 ` Andreas Rheinhardt [this message]
2022-09-08 16:31 ` Michael Niedermayer
2022-09-08 17:03 ` Mohammad Izadi
2022-10-25 23:38 ` Mohammad Izadi
2022-10-26 20:17 ` Michael Niedermayer
[not found] <20210818205538.GF4067@pb2>
2022-03-30 23:22 ` Mohammad Izadi
2022-04-17 17:15 ` Michael Niedermayer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=AS8P250MB0744A5953FBB124FCA4F9A058F309@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM \
--to=andreas.rheinhardt@outlook.com \
--cc=ffmpeg-devel@ffmpeg.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
This inbox may be cloned and mirrored by anyone:
git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
ffmpegdev@gitmailbox.com
public-inbox-index ffmpegdev
Example config snippet for mirrors.
AGPL code for this site: git clone https://public-inbox.org/public-inbox.git