[FFmpeg-devel] [PATCH 15/21] avformat/matroskadec: Redo level handling
Andreas Rheinhardt
andreas.rheinhardt at googlemail.com
Sun Apr 7 20:27:00 EEST 2019
Steve Lhomme:
> On 3/27/2019 12:18 PM, Andreas Rheinhardt via ffmpeg-devel wrote:
>> This commit changes how levels are handled: If the level used for
>> ebml_parse ends directly after an element that has been consumed, then
>> ebml_parse ends the level itself (and any finite-sized levels that end
>> there as well) and informs the caller via the return value; if the
>> current level is unknown-sized, then the level is ended as soon as
>> an element that is not valid on the current level is encountered.
>>
>> This is designed for situations where one wants to parse master
>> elements
>> incrementally, i.e. not in one go via ebml_parse_nest.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at googlemail.com>
>> ---
>> libavformat/matroskadec.c | 104
>> +++++++++++++++++++++++++++++++-------
>> 1 file changed, 85 insertions(+), 19 deletions(-)
>>
>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>> index 42f1c21042..32cf57685f 100644
>> --- a/libavformat/matroskadec.c
>> +++ b/libavformat/matroskadec.c
>> @@ -69,6 +69,8 @@
>> #include "qtpalette.h"
>> #define EBML_UNKNOWN_LENGTH UINT64_MAX /* EBML unknown length,
>> in uint64_t */
>> +#define LEVEL_ENDED 2 /* return value of
>> ebml_parse when the
>> + * syntax level used for
>> parsing ended. */
>> typedef enum {
>> EBML_NONE,
>> @@ -1041,16 +1043,32 @@ static int
>> ebml_parse_id(MatroskaDemuxContext *matroska, EbmlSyntax *syntax,
>> uint32_t id, void *data)
>> {
>> int i;
>> +
>> for (i = 0; syntax[i].id; i++)
>> if (id == syntax[i].id)
>> break;
>> - if (!syntax[i].id && id == MATROSKA_ID_CLUSTER &&
>> - matroska->num_levels > 0 &&
>> - matroska->levels[matroska->num_levels - 1].length ==
>> EBML_UNKNOWN_LENGTH)
>> - return 0; // we reached the end of an unknown size cluster
>> +
>> if (!syntax[i].id && id != EBML_ID_VOID && id != EBML_ID_CRC32) {
>> - av_log(matroska->ctx, AV_LOG_DEBUG, "Unknown entry
>> 0x%"PRIX32"\n", id);
>> + if (!matroska->num_levels ||
>> matroska->levels[matroska->num_levels - 1].length
>> + !=
>> EBML_UNKNOWN_LENGTH) {
>> + av_log(matroska->ctx, AV_LOG_DEBUG, "Unknown entry
>> 0x%"PRIX32"\n", id);
>
> Would this mean a Segment with unknown length would produce this log ?
> Or Segments are not part of the levels at all ? If so, same question
> with level 1 elements which have an unknown length.
>
The part of the code you are refering to is only executed if the read
id is not part of the syntax list that is currently being used for
parsing (and if it is not one of the global elements (void or crc32)).
After the EBML head has been parsed, the segment is parsed using the
matroska_segments syntax list which of course contains an entry for
the segment. So this segment won't trigger this log no matter whether
it is unknown length or not (the segment's length hasn't been parsed
at this point btw).
But if you have an EBML Stream (a concatenation of multiple EBML
(here: Matroska) documents), then the second EBML header and the
second segment will be treated as unknown elements and it will be
acted accordingly, i.e.:
If the current level is unknown-sized and > 1, then the level will be
ended.
If the level is 1 and unknown-sized, then this will be treated as
error (we'll eventually end up in matroska_resync which will look for
level 1 elements and probably end up finding level 1 elements of the
second EBML document, but they will be treated as if the header of the
first EBML document were active. This is likely not good, but it is a
known restriction: Multiple EBML-documents are simply not supported.)
If the current level is zero or known-sized, then this level log
message will be emitted (depending on the loglevel) and it is
attempted to skip the new element. If the current-level is known-sized
and the new element unknown-sized, then according to the current
patchset, an error will be emitted; according to your proposal, the
new element will be truncated to end at the end of the current level.
The answer to the question regarding clusters is essentially the same.
And this can only happen if some error occured, namely if a cluster is
encountered at a place where no cluster is expected (e.g. if because
of a transmission error a part of a known-sized cluster is missing, so
that the next cluster is (judging by the length field of the preceding
cluster) part of the preceding cluster. This will then be treated as
"unknown entry".
(The non-incremental parsing of clusters has a similar drawback; the
current incremental parsing of clusters (which uses a syntax list
which mixes elements from different levels (which is the reason for
the error message that is the reason why I turned my attention to the
Matroska demuxer in the first place)) doesn't have this error.)
Notice that the above scenario is very unlikely: If such an error
occurs, usually the next position presumed to be the beginning of an
EBML Element is somewhere in the middle of another EBML element (or in
some form of junk, depending on the kind of error).
But I think that there is nevertheless a way to improve error recovery
a bit: The introduction of a "last known good" position. Said position
is incremented (set equal to the current position) whenever a valid
element that is known to be allowed at the current hierarchy in the
syntax is encountered. If lateron an error occurs, resyncing starts at
the last known good position, not at the position we are currently at.
>> + } else if (matroska->num_levels > 1) {
>> + // Unknown-sized master elements (except root elements)
>> end
>> + // when an id not known to be allowed in them is
>> encountered.
>> + matroska->num_levels--;
>> + return LEVEL_ENDED;
>> + } else if (matroska->num_levels == 1) {
>> + AVIOContext *pb = matroska->ctx->pb;
>> + int64_t pos = avio_tell(pb);
>> + // An unkown level 1 element inside an unknown-sized
>> segment
>> + // is considered an error.
>> + av_log(matroska->ctx, AV_LOG_ERROR, "Found unknown
>> element id 0x%"PRIX32
>> + " at pos. %"PRIu64" (0x%"PRIx64") in an
>> unknown-sized segment. "
>> + "Will be treated as error.\n", id, pos, pos);
>> + return AVERROR_INVALIDDATA;
>> + }
>> }
>> +
>> return ebml_parse_elem(matroska, &syntax[i], data);
>> }
>> @@ -1061,9 +1079,24 @@ static int ebml_parse(MatroskaDemuxContext
>> *matroska, EbmlSyntax *syntax,
>> uint64_t id;
>> int res = ebml_read_num(matroska, matroska->ctx->pb, 4,
>> &id, 0);
>> if (res < 0) {
>> - // in live mode, finish parsing if EOF is reached.
>> - return (matroska->is_live &&
>> matroska->ctx->pb->eof_reached &&
>> - res == AVERROR_EOF) ? 1 : res;
>> + if (matroska->ctx->pb->eof_reached && res ==
>> AVERROR_EOF) {
>> + if (matroska->is_live)
>> + // in live mode, finish parsing if EOF is reached.
>> + return 1;
>> + if (matroska->num_levels) {
>> + MatroskaLevel *level =
>> &matroska->levels[matroska->num_levels - 1];
>> +
>> + if (level->length == EBML_UNKNOWN_LENGTH) {
>> + // Unknown-sized elements automatically end
>> at EOF.
>> + matroska->num_levels = 0;
>> + return LEVEL_ENDED;
>> + } else if (avio_tell(matroska->ctx->pb) <
>> level->start + level->length) {
>> + av_log(matroska->ctx, AV_LOG_ERROR, "File
>> ended before "
>> + "its declared end\n");
>> + }
>> + }
>> + }
>> + return res;
>> }
>> matroska->current_id = id | 1 << 7 * res;
>> }
>> @@ -1098,10 +1131,15 @@ static int
>> ebml_parse_nest(MatroskaDemuxContext *matroska, EbmlSyntax *syntax,
>> break;
>> }
>> - while (!res && !ebml_level_end(matroska))
>> + if (!matroska->levels[matroska->num_levels - 1].length) {
>> + matroska->num_levels--;
>> + return 0;
>> + }
>> +
>> + while (!res)
>> res = ebml_parse(matroska, syntax, data);
>> - return res;
>> + return res == LEVEL_ENDED ? 0 : res;
>> }
>> static int is_ebml_id_valid(uint32_t id)
>> @@ -1169,7 +1207,7 @@ static int
>> ebml_parse_elem(MatroskaDemuxContext *matroska,
>> AVIOContext *pb = matroska->ctx->pb;
>> uint32_t id = syntax->id;
>> uint64_t length;
>> - int res;
>> + int res, level_check;
>> void *newelem;
>> MatroskaLevel1Element *level1_elem;
>> @@ -1195,7 +1233,8 @@ static int
>> ebml_parse_elem(MatroskaDemuxContext *matroska,
>> length, max_lengths[syntax->type], syntax->type);
>> return AVERROR_INVALIDDATA;
>> }
>> - if (matroska->num_levels > 0) {
>> +
>> + if (matroska->num_levels) {
>> MatroskaLevel *level =
>> &matroska->levels[matroska->num_levels - 1];
>> int64_t pos = avio_tell(pb);
>> @@ -1204,18 +1243,24 @@ static int
>> ebml_parse_elem(MatroskaDemuxContext *matroska,
>> uint64_t elem_end = pos + length,
>> level_end = level->start + level->length;
>> - if (level_end < elem_end) {
>> + if (elem_end < level_end) {
>> + level_check = 0;
>> + } else if (elem_end == level_end) {
>> + level_check = LEVEL_ENDED;
>> + } else {
>> av_log(matroska->ctx, AV_LOG_ERROR,
>> "Element at 0x%"PRIx64" ending at
>> 0x%"PRIx64" exceeds "
>> "containing master element ending at
>> 0x%"PRIx64"\n",
>> pos, elem_end, level_end);
>> return AVERROR_INVALIDDATA;
>> }
>> + } else if (length != EBML_UNKNOWN_LENGTH) {
>> + level_check = 0;
>> } else if (level->length != EBML_UNKNOWN_LENGTH) {
>> av_log(matroska->ctx, AV_LOG_ERROR, "Unknown-sized
>> element "
>> "at 0x%"PRIx64" inside parent with finite
>> size\n", pos);
>> return AVERROR_INVALIDDATA;
>> - } else if (length == EBML_UNKNOWN_LENGTH && id !=
>> MATROSKA_ID_CLUSTER) {
>> + } else if (id != MATROSKA_ID_CLUSTER) {
>> // According to the specifications only clusters
>> and segments
>> // are allowed to be unknown-sized.
>> av_log(matroska->ctx, AV_LOG_ERROR,
>> @@ -1223,7 +1268,8 @@ static int
>> ebml_parse_elem(MatroskaDemuxContext *matroska,
>> "0x%"PRIx64". Dropping the invalid
>> element.\n", pos);
>> return AVERROR_INVALIDDATA;
>> }
>> - }
>> + } else
>> + level_check = 0;
>> }
>> switch (syntax->type) {
>> @@ -1257,19 +1303,36 @@ static int
>> ebml_parse_elem(MatroskaDemuxContext *matroska,
>> av_log(matroska->ctx, AV_LOG_ERROR, "Duplicate
>> element\n");
>> level1_elem->parsed = 1;
>> }
>> - return ebml_parse_nest(matroska, syntax->def.n, data);
>> + res = ebml_parse_nest(matroska, syntax->def.n, data);
>> + break;
>> case EBML_STOP:
>> return 1;
>> default:
>> if (ffio_limit(pb, length) != length)
>> return AVERROR(EIO);
>> - return avio_skip(pb, length) < 0 ? AVERROR(EIO) : 0;
>> + res = avio_skip(pb, length) < 0 ? AVERROR(EIO) : 0;
>> }
>> if (res == AVERROR_INVALIDDATA)
>> av_log(matroska->ctx, AV_LOG_ERROR, "Invalid element\n");
>> else if (res == AVERROR(EIO))
>> av_log(matroska->ctx, AV_LOG_ERROR, "Read error\n");
>> - return res;
>> +
>> + if (res < 0 || res == 1)
>> + return res;
>> +
>> + if (level_check == LEVEL_ENDED && matroska->num_levels) {
>> + MatroskaLevel *level =
>> &matroska->levels[matroska->num_levels - 1];
>> + int64_t pos = avio_tell(pb);
>> +
>> + // Given that pos >= level->start no check for
>> + // level->length != EBML_UNKNOWN_LENGTH is necessary.
>> + while (matroska->num_levels && pos == level->start +
>> level->length) {
>> + matroska->num_levels--;
>> + level--;
>> + }
>
> It seems you're assuming that unknown length can only return to level
> 0 (Segment). But an unknown length Cluster could be followed by
> another unknown length Cluster.
>
I am absolutely not assuming this. An unknown-length cluster would not
be ended at the above place in the code: That's the place where
known-sized levels are ended. Unknown-sized levels are ended
(currently in this code; needs to be adapted to be brought into line
with what you said about when unknown-length levels end, of course) in
ebml_parse_id (after the next EBML ID has been read).
And if an unknown-sized cluster is currently
>> + }
>> +
>> + return level_check;
>> }
>> static void ebml_free(EbmlSyntax *syntax, void *data)
>> @@ -3491,7 +3554,7 @@ static int
>> matroska_parse_cluster(MatroskaDemuxContext *matroska)
>> cluster);
>> }
>> - if (!res && block->bin.size > 0) {
>> + if ((!res || res == LEVEL_ENDED) && block->bin.size > 0) {
>> int is_keyframe = block->non_simple ? block->reference
>> == INT64_MIN : -1;
>> uint8_t* additional = block->additional.size > 0 ?
>> block->additional.data : NULL;
>> @@ -3506,6 +3569,9 @@ static int
>> matroska_parse_cluster(MatroskaDemuxContext *matroska)
>> block->discard_padding);
>> }
>> + if (res == LEVEL_ENDED)
>> + cluster->pos = 0;
>
> This seems odd.
>
First, this commit's intention is to modify the EBML functions to end
levels on their own. The actual changes to matroska_parse_cluster()
are done in the next commit (or the next couple of commits). This
commit only contains the bare minimum of changes so that it still
works (in particular, so that FATE (FFmpeg's automated regression
testing) passes).
And now to the rationale for the change: As has been said multiple
times, the current incremental parsing mixes levels, i.e. its syntax
list includes both the elements inside the cluster as well as the
cluster itself. For the incremental parsing as it is in current git
head, matroska->current_cluster_pos (that's the equivalent variable
for cluster->pos) has two meanings:
1. It is used for the positions of the blocks. If a keyframe is
encountered, FFmpeg automatically adds an index entry for said block
to speed up future seeks and the cluster's position is needed for said
index entry.
2. It is used to distinguish between the first cluster and any
non-first cluster: If matroska->current_cluster_pos is not set, then
there is no need to close the preceding cluster (because it isn't
open). If it is set, then the earlier level is ended, before the new
cluster is entered.
Now that the EBML functions have been adapted, a known-sized cluster
is ended by the ebml_parse_id itself. But an unknown-sized cluster
isn't, because the used syntax list still mixes levels, so that a
cluster is not an unknown element. In order to handle this, I
(temporarily) use cluster->pos to indicate whether a cluster is
currently open (and of course it is also used for creating the index).
So the end of the parsing process for a finite sized cluster followed
by another cluster looks like this: When the end of the cluster is
reached (according to the cluster's size), ebml_parse automatically
ends the cluster's level (new num_levels will be 1) and returns
LEVEL_ENDED. If the cluster ended with a block, said block is parsed
and then the cluster's position is reset. Then the next element is
parsed (usually a cluster, but legally it can be any level 1 element
or a global element) and turns out to be a cluster (an EBML_STOP
element according to the used syntax), the special codepath for
entering clusters is executed. And given that this time no cluster is
"open", no cluster needs to be closed, i.e. ebml_level_end is not
executed.
For an infinite sized cluster followed by another cluster the
situation is like this: ebml_parse this time will not close the
current level on its own (because it doesn't know that it has ended),
so when the new cluster is encountered and the codepath for entering
clusters is executed, cluster->pos is still set and so that the open
cluster is ended via ebml_level_end. cluster->pos is then set equal to
the position of the new cluster and the new cluster is entered.
>> +
>> ebml_free(matroska_blockgroup, block);
>> memset(block, 0, sizeof(*block));
>> --
>> 2.19.2
>>
More information about the ffmpeg-devel
mailing list