| Message ID | 20200501170948.153825-1-jstebbins@jetheaddev.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avformat/matroskaenc: always reserve max aac private data | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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)?) - Andreas
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); >
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; > Then mkv_write_codecprivate() would overwrite the beginning of the void element when it writes the CodecPrivate, creating an invalid file. Instead one could do something like if (par->extradata_size) put_ebml_binary(pb, MATROSKA_ID_CODECPRIVATE, par->extradata, par->extradata_size); put_ebml_void(pb, MAX_PCE_SIZE + 2 + 4 - what was just written (not only par->extradata); - Andreas
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?
John Stebbins (12020-05-01): > 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? Would it not lead to a corrupted file, possibly unplayable? Regards,
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
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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
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. > Does this mean that HandBrake's extradata and the aac_adtstoasc bsf side-data extradata are actually one and the same? - Andreas > 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? >
On Fri, 2020-05-01 at 19:53 +0200, Nicolas George wrote: > John Stebbins (12020-05-01): > > 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? > > Would it not lead to a corrupted file, possibly unplayable? > > If the parameters change on the fly, some part of the audio stream is going to be unplayable. Either the part after the change will be unplayable if you ignore extradata changes, or everything preceeding the last extradata change will be unplayable if you rewrite codec private on every change.
John Stebbins (12020-05-01): > If the parameters change on the fly, some part of the audio stream is > going to be unplayable. Either the part after the change will be > unplayable if you ignore extradata changes, or everything preceeding > the last extradata change will be unplayable if you rewrite codec > private on every change. Then the only acceptable solution is to report an error. If we can detect that the new extradata is compatible and will not cause the file to be unplayable, we can let it pass, but we should try to NEVER allow to lose data without notifying users. Regards,
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.
On Fri, 2020-05-01 at 20:03 +0200, Andreas Rheinhardt wrote: > 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. > > > Does this mean that HandBrake's extradata and the aac_adtstoasc bsf > side-data extradata are actually one and the same? > Yes
On Fri, 2020-05-01 at 20:10 +0200, Nicolas George wrote: > John Stebbins (12020-05-01): > > If the parameters change on the fly, some part of the audio stream > > is > > going to be unplayable. Either the part after the change will be > > unplayable if you ignore extradata changes, or everything > > preceeding > > the last extradata change will be unplayable if you rewrite codec > > private on every change. > > Then the only acceptable solution is to report an error. > > If we can detect that the new extradata is compatible and will not > cause > the file to be unplayable, we can let it pass, but we should try to > NEVER allow to lose data without notifying users. > Well, current code in aac_adtstoasc silently ignores any changes. It only generates extradata from the initial packet. It's not checked in subsequent packets. So this would have to be "fixed" in aac_adtstoasc as well if you want to add error logging. This is also a bit beyond my expertise. I don't know what would constitute incompitible parameters beyond a few obvious things. I'm not well versed in aac details.
John Stebbins (12020-05-01): > Well, current code in aac_adtstoasc silently ignores any changes. It > only generates extradata from the initial packet. It's not checked in > subsequent packets. > > So this would have to be "fixed" in aac_adtstoasc as well if you want > to add error logging. Probably. I want to emphasize that bugs in our current code cannot be considered precedent to accept similar bugs in new code. > This is also a bit beyond my expertise. I don't know what would > constitute incompitible parameters beyond a few obvious things. I'm > not well versed in aac details. Then for now, I would say that we can only accept when it is bit-identical with the extradata already there. If we cannot test for compatibility more finely, then we have to assume incompatibility and reject every AV_PKT_DATA_NEW_EXTRADATA with an error. Regards,
On Fri, 2020-05-01 at 20:28 +0200, Nicolas George wrote: > John Stebbins (12020-05-01): > > Well, current code in aac_adtstoasc silently ignores any > > changes. It > > only generates extradata from the initial packet. It's not checked > > in > > subsequent packets. > > > > So this would have to be "fixed" in aac_adtstoasc as well if you > > want > > to add error logging. > > Probably. I want to emphasize that bugs in our current code cannot be > considered precedent to accept similar bugs in new code. > > > This is also a bit beyond my expertise. I don't know what would > > constitute incompitible parameters beyond a few obvious > > things. I'm > > not well versed in aac details. > > Then for now, I would say that we can only accept when it is > bit-identical with the extradata already there. If we cannot test for > compatibility more finely, then we have to assume incompatibility and > reject every AV_PKT_DATA_NEW_EXTRADATA with an error. > > Seems reasonable. To be clear, this should result in an error returned up the call stack (i.e. av_interleaved_write_frame would ultimately return an error), and an error log?
Am Fr., 1. Mai 2020 um 21:13 Uhr schrieb John Stebbins <jstebbins@jetheaddev.com>: > > On Fri, 2020-05-01 at 20:28 +0200, Nicolas George wrote: > > John Stebbins (12020-05-01): > > > Well, current code in aac_adtstoasc silently ignores any > > > changes. It > > > only generates extradata from the initial packet. It's not checked > > > in > > > subsequent packets. > > > > > > So this would have to be "fixed" in aac_adtstoasc as well if you > > > want > > > to add error logging. > > > > Probably. I want to emphasize that bugs in our current code cannot be > > considered precedent to accept similar bugs in new code. > > > > > This is also a bit beyond my expertise. I don't know what would > > > constitute incompitible parameters beyond a few obvious > > > things. I'm > > > not well versed in aac details. > > > > Then for now, I would say that we can only accept when it is > > bit-identical with the extradata already there. If we cannot test for > > compatibility more finely, then we have to assume incompatibility and > > reject every AV_PKT_DATA_NEW_EXTRADATA with an error. > > > > > > Seems reasonable. To be clear, this should result in an error returned > up the call stack (i.e. av_interleaved_write_frame would ultimately > return an error), and an error log? Only in an error log. Carl Eugen
John Stebbins: > On Fri, 2020-05-01 at 20:28 +0200, Nicolas George wrote: >> John Stebbins (12020-05-01): >>> Well, current code in aac_adtstoasc silently ignores any >>> changes. It >>> only generates extradata from the initial packet. It's not checked >>> in >>> subsequent packets. >>> >>> So this would have to be "fixed" in aac_adtstoasc as well if you >>> want >>> to add error logging. >> >> Probably. I want to emphasize that bugs in our current code cannot be >> considered precedent to accept similar bugs in new code. >> >>> This is also a bit beyond my expertise. I don't know what would >>> constitute incompitible parameters beyond a few obvious >>> things. I'm >>> not well versed in aac details. >> >> Then for now, I would say that we can only accept when it is >> bit-identical with the extradata already there. If we cannot test for >> compatibility more finely, then we have to assume incompatibility and >> reject every AV_PKT_DATA_NEW_EXTRADATA with an error. >> >> > > Seems reasonable. To be clear, this should result in an error returned > up the call stack (i.e. av_interleaved_write_frame would ultimately > return an error), and an error log? > I'd rather opt to only warn in such a case unless strict_std_compliance is >= FF_COMPLIANCE_STRICT. And maybe we should also discard all future packets from this stream until we get one with side data matching the CodecPrivate one? - Andreas
Andreas Rheinhardt (12020-05-01): > I'd rather opt to only warn in such a case unless strict_std_compliance > is >= FF_COMPLIANCE_STRICT. And maybe we should also discard all future > packets from this stream until we get one with side data matching the > CodecPrivate one? It is not a matter of standard compliance, it is a matter of lost data. If only one extradata is written, then the other is discarded, and possibly all subsequent packets are un-decodable. Without an AVERROR code up the stack, a GUI application (most of them just let av_log go to stderr) could open a dialog "Data successfully saved. Delete original? [Yes] No" and let actual data be lost. We can have an option to ignore the error, but as it is, it really must be an error condition by default. Exactly the same as a write error on the file. Regards,
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);