Re: [FFmpeg-devel] [PATCH v3 6/7] avformat/matroskaenc: support writing Dynamic HDR10+ packet side data

[James Almer <jamrial@gmail.com>]

On 3/29/2023 9:44 PM, Andreas Rheinhardt wrote:
> James Almer:
>> Signed-off-by: James Almer <jamrial@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.
_______________________________________________
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".