[FFmpeg-devel] [PATCH] avformat/matroskaenc: always reserve max aac private data

[Andreas Rheinhardt]

James Almer:
> On 5/1/2020 2:09 PM, John Stebbins wrote:
>> When extra data is updated using AV_PKT_DATA_NEW_EXTRADATA, the data is
>> written plus extra space is reserved for the max possible size extradata.
>> But when extradata is written during mkv_write_header, no extra space is
>> reserved. So the side data update overwrites whatever follows the
>> extradata in the track header and results in an invalid file.
> 
> In what scenario there's extradata during init() and then new extradata
> propagated in a packet side data for AAC? And should the side data one
> take priority over the original one?
> 
> With FLAC we know it must have priority because the encoder sends it at
> the end of the encoding process with information that was not available
> during init(), but afaik that's not the case with AAC.
> 
>> ---
>>  libavformat/matroskaenc.c | 14 ++++++++------
>>  1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
>> index 784973a951..e6917002c4 100644
>> --- a/libavformat/matroskaenc.c
>> +++ b/libavformat/matroskaenc.c
>> @@ -728,8 +728,6 @@ static int mkv_write_native_codecprivate(AVFormatContext *s, AVIOContext *pb,
>>      case AV_CODEC_ID_AAC:
>>          if (par->extradata_size)
>>              avio_write(dyn_cp, par->extradata, par->extradata_size);
>> -        else
>> -            put_ebml_void(pb, MAX_PCE_SIZE + 2 + 4);
>>          break;
>>      default:
>>          if (par->codec_id == AV_CODEC_ID_PRORES &&
>> @@ -749,6 +747,7 @@ static int mkv_write_codecprivate(AVFormatContext *s, AVIOContext *pb,
>>      AVIOContext *dyn_cp;
>>      uint8_t *codecpriv;
>>      int ret, codecpriv_size;
>> +    int64_t offset;
>>  
>>      ret = avio_open_dyn_buf(&dyn_cp);
>>      if (ret < 0)
>> @@ -802,9 +801,15 @@ static int mkv_write_codecprivate(AVFormatContext *s, AVIOContext *pb,
>>      }
>>  
>>      codecpriv_size = avio_get_dyn_buf(dyn_cp, &codecpriv);
>> +    offset = avio_tell(pb);
>>      if (codecpriv_size)
>>          put_ebml_binary(pb, MATROSKA_ID_CODECPRIVATE, codecpriv,
>>                          codecpriv_size);
>> +    if (par->codec_id == AV_CODEC_ID_AAC) {
>> +        int filler = MAX_PCE_SIZE + 2 + 4 - (avio_tell(pb) - offset);
>> +        if (filler)
>> +            put_ebml_void(pb, filler);
>> +    }
> 
> Why are you adding this here instead of adapting the code in
> mkv_write_native_codecprivate()?
> 
> Can't you just do something like
> 
> case AV_CODEC_ID_AAC:
>     if (par->extradata_size)
>         avio_write(dyn_cp, par->extradata, par->extradata_size);
>     put_ebml_void(pb, MAX_PCE_SIZE + 2 + 4 - avio_tell(dyn_cp));
>     break;
> 
Then mkv_write_codecprivate() would overwrite the beginning of the void
element when it writes the CodecPrivate, creating an invalid file.
Instead one could do something like

if (par->extradata_size)
    put_ebml_binary(pb, MATROSKA_ID_CODECPRIVATE, par->extradata,
par->extradata_size);
put_ebml_void(pb, MAX_PCE_SIZE + 2 + 4 - what was just written (not only
par->extradata);

- Andreas