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

[Andreas Rheinhardt]

John Stebbins:
> On Fri, 2020-05-01 at 19:22 +0200, Andreas Rheinhardt wrote:
>> John Stebbins:
>>> 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.
>>> ---
>>>  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);
>>> +    }
>>>      ffio_free_dyn_buf(&dyn_cp);
>>>      return ret;
>>>  }
>>> @@ -2182,7 +2187,7 @@ static int
>>> mkv_check_new_extra_data(AVFormatContext *s, const AVPacket *pkt)
>>>      switch (par->codec_id) {
>>>      case AV_CODEC_ID_AAC:
>>>          if (side_data_size && (s->pb->seekable &
>>> AVIO_SEEKABLE_NORMAL) && !mkv->is_live) {
>>> -            int filler, output_sample_rate = 0;
>>> +            int output_sample_rate = 0;
>>>              ret = get_aac_sample_rates(s, side_data,
>>> side_data_size, &track->sample_rate,
>>>                                         &output_sample_rate);
>>>              if (ret < 0)
>>> @@ -2195,9 +2200,6 @@ static int
>>> mkv_check_new_extra_data(AVFormatContext *s, const AVPacket *pkt)
>>>              memcpy(par->extradata, side_data, side_data_size);
>>>              avio_seek(mkv->tracks_bc, track->codecpriv_offset,
>>> SEEK_SET);
>>>              mkv_write_codecprivate(s, mkv->tracks_bc, par, 1, 0);
>>> -            filler = MAX_PCE_SIZE + 2 + 4 - (avio_tell(mkv-
>>>> tracks_bc) - track->codecpriv_offset);
>>> -            if (filler)
>>> -                put_ebml_void(mkv->tracks_bc, filler);
>>>              avio_seek(mkv->tracks_bc, track->sample_rate_offset,
>>> SEEK_SET);
>>>              put_ebml_float(mkv->tracks_bc,
>>> MATROSKA_ID_AUDIOSAMPLINGFREQ, track->sample_rate);
>>>              put_ebml_float(mkv->tracks_bc,
>>> MATROSKA_ID_AUDIOOUTSAMPLINGFREQ, output_sample_rate);
>>>
>> If we already had extradata when writing the header, then what
>> guarantees us that the new extradata is better and should be
>> preferred
>> to the old extradata? (I don't know the details of AAC extradata, but
>> is
>> it possible that the extradata is simply incompatible to the old one
>> (e.g. when a user splices together actually incompatible streams)?)
>>
> 
> The test case is a TS file with aac audio.  It TS files, it is
> certainly possible for stream parameters to change on the fly. But, if
> the extradata changes, there's really no way to handle it in mkv since
> this data is global in mkv.  So perhaps a better solution is to ignore
> side data extradata if it's already been written once?
> 
In this scenario there is no way to know whether the new extradata is
better than the old one. If we ignore the extradata in such scenarios,
then is such a file even decodable?

- Andreas