[FFmpeg-devel] [PATCH 00/37 v3] Matroska demuxer patches

[Andreas Rheinhardt]

Andreas Rheinhardt:
> Hello,
> 
> here is the patchset for the Matroska demuxer that I already announced
> last week [1]. It is an improvement upon an earlier patchset from March [2]
> which has been kindly reviewed by Steve Lhomme. Just as the earlier 
> attempt, it is intended to get rid of a bogus error message introduced 
> in 9326117b.
> 
> 1. Here is a recap of why this bogus error message exists:
> a) The ebml parser contained in the Matroska muxer basically has two
> modes of operation: It can parse simple elements and it can parse whole
> master elements all at once. The latter is of course implemented via the
> former. Also one can designate certain element IDs as "EBML_STOP" type
> so that when such an element is encountered, parsing immediately stops
> without parsing the length and performing the checks (regarding whether
> an element is truly contained in the master element it is supposed to be
> contained in --these checks were introduced in 9326117b).
> b) Normal parsing (via matroska_parse_cluster_incremental) uses a syntax
> list for parsing that contains elements at different levels in the
> hierarchy: The elements that can be found in a cluster and (certain)
> level 1 elements (i.e. the level of the cluster); in particular,
> it contains a cluster as EBML_STOP type element. When a cluster is
> encountered and ebml_parse returns, the current level is ended (if there
> already was an open cluster) and the new cluster is entered (via a
> designated syntax list containing a cluster as master element) and
> parsed until a SimpleBlock or a BlockGroup is encountered (these two are
> of course EBML_STOP elements in the used syntax). Then the first syntax
> list (the one containing both the cluster as well as the elements
> permitted in a cluster as entries) is used again for parsing of said
> blocks and all the blocks that follow until one encounters another
> cluster or an error occurs.
> c) The problem with this approach is that clusters are not closed
> automatically when they are finished, but only when another cluster is
> encountered. This works (i.e. yields no error message) when a cluster
> follows another cluster (because no checks are performed for EBML_STOP
> type elements), but if e.g. the cues (the index) is located after a
> cluster (as happens with most Matroska files in existence), then parsing
> the cues will yield an error: After all, the preceding cluster ended
> right before the cues began, but the level hasn't been ended, so the
> length check will fail if the preceding cluster wasn't an
> unknown-length cluster. (In the latter case, the cues are considered
> part of the preceding cluster by the current code.) That's the reason
> behind this (harmless) error.
> 
> 2. a) When ebml_parse parses a Master element by itself, it also takes
> care of ending the level (i.e. the level automatically entered during
> parsing of the master element) itself; but there is no check at the end
> of ebml_parse whether a level has just ended right after the last
> element; if there were, then the above problem wouldn't exist.
> b) So my approach is to add such a check and make ebml_parse end the
> levels all by itself; the Matroska functions shouldn't have to end
> levels manually (as happens currently when a new cluster is encountered),
> but the EBML functions would do so given that the levels are an EBML
> concept after all.
> c) But all is slightly complicated by the fact that there also can be
> unknown-length master elements. These end when an element not allowed in
> them but on a higher level is encountered. So in this situation it is
> impossible to perform the check whether a level has ended after
> consuming an element; instead, said check needs to be performed directly
> after the id of the next element is read, without reading any more data
> if it turns out that said element belongs to a higher level. And that's
> the way it has been implemented.
> 
> 3. I have also added improvements to various checks and other things.
> You can find them explained in the various commit messages. Just two things:
> a) I am unsure how to check whether a read error or attempted reading
> beyond EOF should be checked: Via avio_feof(pb) or via pb->eof_reached?
> The former tries to read again (refill the buffer) when pb->eof_reached
> was already set; does this mean that if one wants to know whether a read
> was not successfull one should check pb->eof_reached, but when one is
> more interested into whether one can perform future reads, one should
> use avio_feof (after all, in case of livestreaming, new data might have
> arrived after the earlier failed read)? That's at least the interpretation
> that I had when I wrote the patchset.
> b) ffio_limit is used to check for whether there is enough data left to
> skip if an element intended to be skipped is encountered. There are
> three issues with this function:
> i) It takes an int, although it should be an int64_t for our needs.
> ii) It emits its own error message (in case it fails) which is not
> appropriate for our needs (it claims to be truncating a packet, but in
> our case, no packet is truncated; instead it is treated as an indication
> that an error happened and a resync is performed).
> iii) For some reason it allows the remaining size to be one less than
> the size given as argument.
> Not understanding iii) is what made me hesitate to factor the needed
> part of ffio_limit out of it and using the new function in the Matroska
> demuxer. Would be nice if someone could explain the rationale behind
> this. This originated (without any explanation) in commit 559ae20d and
> got carried over from there since then.
> 
> - Andreas
> 
> [1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2019-May/243769.html
> [2]: https://ffmpeg.org/pipermail/ffmpeg-devel/2019-March/241694.html
> 
> 
> Andreas Rheinhardt (37):
>   avformat/matroskadec: Remove unused variables
>   avformat/matroskadec: Correct outdated error message
>   avformat/matroskadec: Compactify structure
>   avformat/matroskadec: Don't zero unnecessarily
>   avformat/matroskadec: Get rid of cluster size field assumption
>   avformat/matroskadec: Use generic size check for signed integers
>   avformat/matroskadec: Set offset of first cluster
>   avformat/matroskadec: Don't copy attached pictures
>   avformat/matroskadec: Remove redundant initialization
>   avformat/matroskadec: Properly check return values
>   avformat/matroskadec: Improve read error/EOF checks I
>   avformat/matroskadec: Improve read error/EOF checks II
>   avformat/matroskadec: Improve error/EOF checks III
>   avformat/matroskadec: Remove non-incremental parsing of clusters
>   avformat/matroskadec: Don't keep old blocks
>   avformat/matroskadec: Treat SimpleBlock as EBML_BIN
>   avformat/matroskadec: Don't abort resyncing upon seek failure
>   avformat/matroskadec: Add function to reset status
>   avformat/matroskadec: Use proper levels after discontínuity
>   avformat/matroskadec: Refactor some functions
>   avformat/matroskadec: Introduce a "last known good" position
>   avformat/matroskadec: Link to parents in syntax tables
>   avformat/matroskadec: Redo level handling
>   avformat/matroskadec: Make cluster parsing level compatible
>   avformat/matroskadec: Don't reset cluster position
>   avformat/matroskadec: Combine arrays
>   avformat/matroskadec: Redo EOF handling
>   avformat/matroskadec: Reuse positions
>   avformat/matroskadec: Typos, nits and cosmetics
>   avformat/matroskadec: Don't skip too much when unseekable
>   avformat/matroskadec: Improve invalid length error handling
>   avformat/matroskadec: Accept more unknown-length elements
>   avformat/matroskadec: Fix probing of unknown-length headers
>   avformat/matroskadec: Accept more unknown-length elements II
>   avformat/matroskadec: Reindent after previous commit
>   avformat/matroskadec: Use file offsets for level 1 elements
>   avformat/matroskadec: Improve check for level 1 duplicates
> 
>  libavformat/matroskadec.c | 1006 +++++++++++++++++++++----------------
>  1 file changed, 582 insertions(+), 424 deletions(-)
> 
Ping for the whole patchset.

- Andreas