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

[John Stebbins]

On Fri, 2020-05-01 at 19:55 +0200, Andreas Rheinhardt wrote:
> 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?
> 

It depends on if the extradata actually changed or not.  In my actual
test case, there are not stream parameter changes.  The same data is
being delivered when initializing the muxer and by side data.

If stream parameters actually change on the fly in a TS file, ffmpeg
currently generates a broken stream. aac_adtstoasc bsf only generates
extradata side data once.  It currently ignores any other changes.  So
if incompatible aac parameters exist in the stream after the first
packet, the output stream will be broken.