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

[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?