| Message ID | 20200326004144.26758-3-andreas.rheinhardt@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avformat/matroskadec: Don't discard the upper 32bits of TrackNumber | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
Quoting Andreas Rheinhardt (2020-03-26 01:41:44) > Matroska is built around the principle that a reader does not need to > understand everything in a file in order to be able to make use of it; > it just needs to ignore the data it doesn't know about. > > Our demuxer typically follows this principle, but there is one important > instance where it does not: A Block belonging to a TrackEntry with no > associated stream is treated as invalid data (i.e. the demuxer will try > to resync to the next level 1 element because it takes this as a sign > that it has lost sync). Given that we do not create streams if we don't > know or don't support the type of the TrackEntry, this impairs this > demuxer's forward compability. > > Furthermore, ignoring Blocks belonging to a TrackEntry without > corresponding stream can (in future commits) also be used to ignore > TrackEntries with obviously bogus entries without affecting the other > TrackEntries (by not creating a stream for said TrackEntry). > > Finally, given that matroska_find_track_by_num() already emits its own > error message in case there is no TrackEntry with a given TrackNumber, > the error message (with level AV_LOG_INFO) for this can be removed. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- Looks reasonable
On Thu, Mar 26, 2020 at 01:41:44AM +0100, Andreas Rheinhardt wrote: > Matroska is built around the principle that a reader does not need to > understand everything in a file in order to be able to make use of it; > it just needs to ignore the data it doesn't know about. > > Our demuxer typically follows this principle, but there is one important > instance where it does not: A Block belonging to a TrackEntry with no > associated stream is treated as invalid data (i.e. the demuxer will try > to resync to the next level 1 element because it takes this as a sign > that it has lost sync). Given that we do not create streams if we don't > know or don't support the type of the TrackEntry, this impairs this > demuxer's forward compability. > > Furthermore, ignoring Blocks belonging to a TrackEntry without > corresponding stream can (in future commits) also be used to ignore > TrackEntries with obviously bogus entries without affecting the other > TrackEntries (by not creating a stream for said TrackEntry). > > Finally, given that matroska_find_track_by_num() already emits its own > error message in case there is no TrackEntry with a given TrackNumber, > the error message (with level AV_LOG_INFO) for this can be removed. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/matroskadec.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) does this affect the efficiency of the demuxer to resync in case of actual errors ? if so it may make sense to check the format version before accepting unreferenced track numbers thanks [...]
diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c index 7aea13dda0..cd85d3e281 100644 --- a/libavformat/matroskadec.c +++ b/libavformat/matroskadec.c @@ -3522,13 +3522,16 @@ static int matroska_parse_block(MatroskaDemuxContext *matroska, AVBufferRef *buf size -= n; track = matroska_find_track_by_num(matroska, num); - if (!track || !track->stream) { - av_log(matroska->ctx, AV_LOG_INFO, - "Invalid stream %"PRIu64"\n", num); + if (!track || size < 3) return AVERROR_INVALIDDATA; - } else if (size < 3) - return AVERROR_INVALIDDATA; - st = track->stream; + + if (!(st = track->stream)) { + av_log(matroska->ctx, AV_LOG_VERBOSE, + "No stream associated to TrackNumber %"PRIu64". " + "Ignoring Block with this TrackNumber.\n", num); + return 0; + } + if (st->discard >= AVDISCARD_ALL) return res; av_assert1(block_duration != AV_NOPTS_VALUE);
Matroska is built around the principle that a reader does not need to understand everything in a file in order to be able to make use of it; it just needs to ignore the data it doesn't know about. Our demuxer typically follows this principle, but there is one important instance where it does not: A Block belonging to a TrackEntry with no associated stream is treated as invalid data (i.e. the demuxer will try to resync to the next level 1 element because it takes this as a sign that it has lost sync). Given that we do not create streams if we don't know or don't support the type of the TrackEntry, this impairs this demuxer's forward compability. Furthermore, ignoring Blocks belonging to a TrackEntry without corresponding stream can (in future commits) also be used to ignore TrackEntries with obviously bogus entries without affecting the other TrackEntries (by not creating a stream for said TrackEntry). Finally, given that matroska_find_track_by_num() already emits its own error message in case there is no TrackEntry with a given TrackNumber, the error message (with level AV_LOG_INFO) for this can be removed. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavformat/matroskadec.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)