[FFmpeg-devel] [PATCH v2 3/5] avformat/matroskaenc: Write dvcC/dvvC block additional mapping

[quietvoid]

On 23/09/2021 01.33, Andreas Rheinhardt wrote:
>> diff --git a/libavformat/dovi_isom.h b/libavformat/dovi_isom.h
>> index 9d9a05bdb0..b58ab1bea0 100644
>> --- a/libavformat/dovi_isom.h
>> +++ b/libavformat/dovi_isom.h
>> @@ -23,7 +23,12 @@
>>   #define AVFORMAT_DOVI_ISOM_H
>>   
>>   #include "avformat.h"
>> +#include "libavutil/dovi_meta.h"
>> +
>> +#define ISOM_DVCC_DVVC_SIZE 24
>>   
>>   int ff_isom_parse_dvcc_dvvc(AVFormatContext *s, AVStream *st, const uint8_t *buf_ptr, uint64_t size);
>> +int ff_isom_put_dvcc_dvvc(AVFormatContext *s, uint8_t *out, int size, uint32_t *type,
>> +                         AVDOVIDecoderConfigurationRecord *dovi);
>>   
>>   #endif /* AVFORMAT_DOVI_ISOM_H */
>> \ No newline at end of file
>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
>> index 039f20988a..a54f7b5feb 100644
>> --- a/libavformat/matroskaenc.c
>> +++ b/libavformat/matroskaenc.c
>> @@ -54,6 +54,8 @@
>>   #include "libavcodec/xiph.h"
>>   #include "libavcodec/mpeg4audio.h"
>>   
>> +#include "libavformat/dovi_isom.h"
>> +
>>   /* Level 1 elements we create a SeekHead entry for:
>>    * Info, Tracks, Chapters, Attachments, Tags (potentially twice) and Cues */
>>   #define MAX_SEEKHEAD_ENTRIES 7
>> @@ -1115,6 +1117,33 @@ static int mkv_write_stereo_mode(AVFormatContext *s, AVIOContext *pb,
>>       return 0;
>>   }
>>   
>> +static int mkv_write_dovi(AVFormatContext *s, AVIOContext *pb, AVStream *st)
>> +{
>> +    int ret;
>> +    AVDOVIDecoderConfigurationRecord *dovi = (AVDOVIDecoderConfigurationRecord *)
>> +                                             av_stream_get_side_data(st, AV_PKT_DATA_DOVI_CONF, NULL);
>> +
>> +    if (dovi) {
>> +        ebml_master mapping;
>> +        uint8_t buf[ISOM_DVCC_DVVC_SIZE];
>> +        uint32_t type;
>> +        int size;
>> +
>> +        if ((ret = ff_isom_put_dvcc_dvvc(s, buf, sizeof(buf), &type, dovi)) < 0)
>> +            return ret;
>> +
>> +        size = ret;
>> +
>> +        mapping = start_ebml_master(pb, MATROSKA_ID_TRACKBLKADDMAPPING, ISOM_DVCC_DVVC_SIZE);
> This is wrong: ISOM_DVCC_DVVC_SIZE need not be an upper bound for the
> size of the blockgroup. (2 + 1 + strlen("Dolby Vision configuration")) +
> (2 + 1 + 4) + (2 + 1 + ISOM_DVCC_DVVC_SIZE) is -- although I'd prefer if
> you used sizeof("Dolby Vision configuration") - 1 instead of strlen.
Okay, I was confused on what you meant previously.
I have changed it to use this variable:
uint64_t expected_size = (2 + 1 + (sizeof("Dolby Vision configuration") 
- 1))
                                              + (2 + 1 + 4) + (2 + 1 + 
ISOM_DVCC_DVVC_SIZE);

>> +        put_ebml_string(pb, MATROSKA_ID_BLKADDIDNAME, "Dolby Vision configuration");
>> +        put_ebml_uint(pb, MATROSKA_ID_BLKADDIDTYPE, type);
>> +        put_ebml_binary(pb, MATROSKA_ID_BLKADDIDEXTRADATA, buf, size);
>> +        end_ebml_master(pb, mapping);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static int mkv_write_track(AVFormatContext *s, MatroskaMuxContext *mkv,
>>                              AVStream *st, mkv_track *track, AVIOContext *pb,
>>                              int is_default)
>> @@ -1380,6 +1409,9 @@ static int mkv_write_track(AVFormatContext *s, MatroskaMuxContext *mkv,
>>           return AVERROR(EINVAL);
>>       }
>>   
>> +    if ((ret = mkv_write_dovi(s, pb, st)) < 0)
>> +        return ret;
> Given that this is not part of WebM, there needs to be check for
> mkv->mode != MODE_WEBM. Furthermore, this should also be moved to the
> AVMEDIA_TYPE_VIDEO switch statement directly after the video master
> element has been closed.

Corrected, thank you.


Is it OK to resend a new version or should I wait in case more
changes are necessary in the other patches?