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

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

>      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);
>