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

[John Stebbins]

On Fri, 2020-05-01 at 14:27 -0300, James Almer wrote:
> 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?
> 

This is partially a HandBrake issue.  Before ffmpeg ever had side data,
HandBrake parsed the extradata out of the aac stream in TS files
ourselves before initializing the muxer.  We still do this, so
extradata is supplied by us when the muxer it initialized and again by
side data generated in aac_adtstoasc bsf.

So remuxing ts to mkv with ffmpeg exe does not have this problem.  But
it is a problem when using the API in valid ways.

I don't think it matters which takes priority.  If stream parameters
change, there really is no way to handle in mkv since this is global
data in mkv.  So perhaps just ignore side data when extradata has
already been set once?

> 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;
> 
> >      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);
> > 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".