[FFmpeg-devel] [PATCH v3 6/7] avformat/matroskaenc: support writing Dynamic HDR10+ packet side data
James Almer
jamrial at gmail.com
Thu Mar 30 04:41:39 EEST 2023
On 3/29/2023 9:44 PM, Andreas Rheinhardt wrote:
> James Almer:
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>> libavformat/matroskaenc.c | 91 ++++++++++++++++---
>> tests/ref/fate/aac-autobsf-adtstoasc | 4 +-
>> tests/ref/fate/matroska-avoid-negative-ts | 4 +-
>> tests/ref/fate/matroska-dovi-write-config7 | 4 +-
>> tests/ref/fate/matroska-dovi-write-config8 | 4 +-
>> tests/ref/fate/matroska-dvbsub-remux | 4 +-
>> tests/ref/fate/matroska-encoding-delay | 14 +--
>> tests/ref/fate/matroska-flac-extradata-update | 4 +-
>> tests/ref/fate/matroska-h264-remux | 4 +-
>> .../fate/matroska-mastering-display-metadata | 4 +-
>> tests/ref/fate/matroska-move-cues-to-front | 4 +-
>> tests/ref/fate/matroska-mpegts-remux | 4 +-
>> tests/ref/fate/matroska-ms-mode | 4 +-
>> tests/ref/fate/matroska-ogg-opus-remux | 10 +-
>> tests/ref/fate/matroska-opus-remux | 10 +-
>> tests/ref/fate/matroska-pgs-remux | 4 +-
>> tests/ref/fate/matroska-pgs-remux-durations | 4 +-
>> tests/ref/fate/matroska-qt-mode | 4 +-
>> tests/ref/fate/matroska-spherical-mono-remux | 4 +-
>> tests/ref/fate/matroska-vp8-alpha-remux | 4 +-
>> tests/ref/fate/matroska-zero-length-block | 4 +-
>> tests/ref/fate/rgb24-mkv | 4 +-
>> tests/ref/fate/shortest-sub | 4 +-
>> tests/ref/lavf-fate/av1.mkv | 4 +-
>> tests/ref/lavf/mka | 4 +-
>> tests/ref/lavf/mkv | 4 +-
>> tests/ref/lavf/mkv_attachment | 4 +-
>> 27 files changed, 141 insertions(+), 76 deletions(-)
>>
>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
>> index dfc3fbbe95..3427e93619 100644
>> --- a/libavformat/matroskaenc.c
>> +++ b/libavformat/matroskaenc.c
>> @@ -44,6 +44,7 @@
>> #include "libavutil/channel_layout.h"
>> #include "libavutil/crc.h"
>> #include "libavutil/dict.h"
>> +#include "libavutil/hdr_dynamic_metadata.h"
>> #include "libavutil/intfloat.h"
>> #include "libavutil/intreadwrite.h"
>> #include "libavutil/lfg.h"
>> @@ -1612,6 +1613,10 @@ static void mkv_write_blockadditionmapping(AVFormatContext *s, MatroskaMuxContex
>> // we either write the default value here, or a void element. Either of them will
>> // be overwritten when finishing the track.
>> put_ebml_uint(mkv->track.bc, MATROSKA_ID_TRACKMAXBLKADDID, 0);
>> + // Similarly, reserve space for an eventual HDR10+ ITU T.35 metadata BlockAdditionMapping.
>> + put_ebml_void(pb, 3 /* BlockAdditionMapping */
>> + + 4 /* BlockAddIDValue */
>> + + 4 /* BlockAddIDType */);
>> }
>>
>> if (dovi && dovi->dv_profile <= 10) {
>> @@ -2618,17 +2623,34 @@ static int webm_reformat_vtt(MatroskaMuxContext *mkv, AVIOContext *pb,
>> return 0;
>> }
>>
>> +static void mkv_write_blockadditional(EbmlWriter *writer, const uint8_t *buf,
>> + size_t size, enum AVPacketSideDataType type,
>> + uint64_t additional_id)
>> +{
>> + size_t offset = 0;
>> +
>> + if (type == AV_PKT_DATA_MATROSKA_BLOCKADDITIONAL)
>> + offset = 8;
>
> The offset should not exist; the caller should provide the correct data
> and size.
Removed.
>
>> +
>> + ebml_writer_open_master(writer, MATROSKA_ID_BLOCKMORE);
>> + ebml_writer_add_uint(writer, MATROSKA_ID_BLOCKADDID, additional_id);
>> + ebml_writer_add_bin (writer, MATROSKA_ID_BLOCKADDITIONAL,
>> + buf + offset, size - offset);
>> + ebml_writer_close_master(writer);
>> +}
>> +
>> static int mkv_write_block(void *logctx, MatroskaMuxContext *mkv,
>> AVIOContext *pb, const AVCodecParameters *par,
>> mkv_track *track, const AVPacket *pkt,
>> int keyframe, int64_t ts, uint64_t duration,
>> int force_blockgroup, int64_t relative_packet_pos)
>> {
>> - uint8_t *side_data;
>> + uint8_t *side_data, *buf = NULL;
>> size_t side_data_size;
>> - uint64_t additional_id;
>> + uint64_t additional_id, max_blockaddid = 0;
>
> max_blockaddid should not exist; instead, you should unconditionally
> open MATROSKA_ID_BLOCKADDITIONS and close it with
> ebml_writer_close_or_discard_master().
Oh, that's quite nice. Changed.
>
>> unsigned track_number = track->track_num;
>> - EBML_WRITER(9);
>> + int ret;
>> + EBML_WRITER(13);
>>
>> mkv->cur_block.track = track;
>> mkv->cur_block.pkt = pkt;
>> @@ -2670,17 +2692,51 @@ static int mkv_write_block(void *logctx, MatroskaMuxContext *mkv,
>> // Only the Codec-specific BlockMore (id == 1) is currently supported.
>> (additional_id = AV_RB64(side_data)) == MATROSKA_BLOCK_ADD_ID_TYPE_OPAQUE_DATA) {
>> 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);
>> - ebml_writer_close_master(&writer);
>> - track->max_blockaddid = additional_id;
>> + mkv_write_blockadditional(&writer, side_data, side_data_size,
>> + AV_PKT_DATA_MATROSKA_BLOCKADDITIONAL,
>> + additional_id);
>> + max_blockaddid = track->max_blockaddid = FFMAX(track->max_blockaddid,
>> + additional_id);
>> + }
>> +
>> + side_data = av_packet_get_side_data(pkt,
>> + AV_PKT_DATA_DYNAMIC_HDR10_PLUS,
>> + &side_data_size);
>> + if (side_data_size) {
>> + uint8_t *payload;
>> + size_t payload_size, buf_size;
>> + int ret = av_dynamic_hdr_plus_to_t35((AVDynamicHDRPlus *)side_data, &payload,
>> + &payload_size);
>
> I have to say, I really hate these allocations here and consider
> av_dynamic_hdr_plus_to_t35() to be ill-designed now.
What would be better? Passing an existing buffer to it and having it
return an error if it's not big enough?
I would not oppose you sending a patch to change it. It's quite recent
and nothing but this patch uses it yet. But others may disagree.
More information about the ffmpeg-devel
mailing list