[FFmpeg-devel,10/21] avformat/matroskadec: Don't keep old blocks

Submitted by Oliver Collyer via ffmpeg-devel on March 27, 2019, 11:18 a.m.

Details

Message ID 20190327111852.3784-11-andreas.rheinhardt@googlemail.com
State New
Headers show

Commit Message

Oliver Collyer via ffmpeg-devel March 27, 2019, 11:18 a.m.
Before this commit, the Matroska muxer would read a block when required
to do so, parse the block, create and return the necessary AVPackets and
yet keep the blocks (in a dynamically allocated list), although they
aren't used at all any more. This has been changed. There is no list any
more and the block is immediately discarded after parsing.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@googlemail.com>
---
 libavformat/matroskadec.c | 87 +++++++++++++++++----------------------
 1 file changed, 38 insertions(+), 49 deletions(-)

Comments

Steve Lhomme April 7, 2019, 7:05 a.m.
On 3/27/2019 12:18 PM, Andreas Rheinhardt via ffmpeg-devel wrote:
> Before this commit, the Matroska muxer would read a block when required
> to do so, parse the block, create and return the necessary AVPackets and
> yet keep the blocks (in a dynamically allocated list), although they
> aren't used at all any more. This has been changed. There is no list any
> more and the block is immediately discarded after parsing.

My understanding of the code is that the blocks are put in a queue, 
that's whatwebm_clusters_start_with_keyframe() uses to check that the 
first frame is a keyframe (it doesn't check the type of the frame 
though...). But since there's only one read 
inmatroska_parse_cluster_incremental()there's only one kept in memory.

So LGTM.

>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@googlemail.com>
> ---
>   libavformat/matroskadec.c | 87 +++++++++++++++++----------------------
>   1 file changed, 38 insertions(+), 49 deletions(-)
>
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 9198fa1bea..997c96d78f 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -304,9 +304,20 @@ typedef struct MatroskaLevel {
>       uint64_t length;
>   } MatroskaLevel;
>   
> +typedef struct MatroskaBlock {
> +    uint64_t duration;
> +    int64_t  reference;
> +    uint64_t non_simple;
> +    EbmlBin  bin;
> +    uint64_t additional_id;
> +    EbmlBin  additional;
> +    int64_t discard_padding;
> +} MatroskaBlock;
> +
>   typedef struct MatroskaCluster {
> +    MatroskaBlock block;
>       uint64_t timecode;
> -    EbmlList blocks;
> +    int64_t pos;
>   } MatroskaCluster;
>   
>   typedef struct MatroskaLevel1Element {
> @@ -356,8 +367,6 @@ typedef struct MatroskaDemuxContext {
>       MatroskaLevel1Element level1_elems[64];
>       int num_level1_elems;
>   
> -    int current_cluster_num_blocks;
> -    int64_t current_cluster_pos;
>       MatroskaCluster current_cluster;
>   
>       /* WebM DASH Manifest live flag */
> @@ -367,16 +376,6 @@ typedef struct MatroskaDemuxContext {
>       int bandwidth;
>   } MatroskaDemuxContext;
>   
> -typedef struct MatroskaBlock {
> -    uint64_t duration;
> -    int64_t  reference;
> -    uint64_t non_simple;
> -    EbmlBin  bin;
> -    uint64_t additional_id;
> -    EbmlBin  additional;
> -    int64_t discard_padding;
> -} MatroskaBlock;
> -
>   static const EbmlSyntax ebml_header[] = {
>       { EBML_ID_EBMLREADVERSION,    EBML_UINT, 0, offsetof(Ebml, version),         { .u = EBML_VERSION } },
>       { EBML_ID_EBMLMAXSIZELENGTH,  EBML_UINT, 0, offsetof(Ebml, max_size),        { .u = 8 } },
> @@ -705,9 +704,9 @@ static const EbmlSyntax matroska_blockgroup[] = {
>   };
>   
>   static const EbmlSyntax matroska_cluster_parsing[] = {
> -    { MATROSKA_ID_CLUSTERTIMECODE, EBML_UINT, 0,                     offsetof(MatroskaCluster, timecode) },
> -    { MATROSKA_ID_BLOCKGROUP,      EBML_NEST, sizeof(MatroskaBlock), offsetof(MatroskaCluster, blocks), { .n = matroska_blockgroup } },
> -    { MATROSKA_ID_SIMPLEBLOCK,     EBML_PASS, sizeof(MatroskaBlock), offsetof(MatroskaCluster, blocks), { .n = matroska_blockgroup } },
> +    { MATROSKA_ID_CLUSTERTIMECODE, EBML_UINT, 0, offsetof(MatroskaCluster, timecode) },
> +    { MATROSKA_ID_BLOCKGROUP,      EBML_NEST, 0, 0, { .n = matroska_blockgroup } },
> +    { MATROSKA_ID_SIMPLEBLOCK,     EBML_PASS, 0, 0, { .n = matroska_blockgroup } },
>       { MATROSKA_ID_CLUSTERPOSITION, EBML_NONE },
>       { MATROSKA_ID_CLUSTERPREVSIZE, EBML_NONE },
>       { MATROSKA_ID_INFO,            EBML_NONE },
> @@ -3443,57 +3442,48 @@ end:
>   
>   static int matroska_parse_cluster(MatroskaDemuxContext *matroska)
>   {
> -    EbmlList *blocks_list;
> -    MatroskaBlock *blocks;
> -    int i, res;
> +    MatroskaCluster *cluster = &matroska->current_cluster;
> +    MatroskaBlock     *block = &cluster->block;
> +    int res;
>       res = ebml_parse(matroska,
>                        matroska_cluster_parsing,
> -                     &matroska->current_cluster);
> +                     cluster);
>       if (res == 1) {
>           /* New Cluster */
> -        if (matroska->current_cluster_pos)
> +        if (cluster->pos)
>               ebml_level_end(matroska);
> -        ebml_free(matroska_cluster_parsing, &matroska->current_cluster);
> -        memset(&matroska->current_cluster, 0, sizeof(MatroskaCluster));
> -        matroska->current_cluster_num_blocks = 0;
> -        matroska->current_cluster_pos        = avio_tell(matroska->ctx->pb);
> +        cluster->pos = avio_tell(matroska->ctx->pb);
>           /* sizeof the ID which was already read */
>           if (matroska->current_id)
> -            matroska->current_cluster_pos -= 4;
> +            cluster->pos -= 4;
>           res = ebml_parse(matroska,
>                            matroska_clusters,
> -                         &matroska->current_cluster);
> +                         cluster);
>           /* Try parsing the block again. */
>           if (res == 1)
>               res = ebml_parse(matroska,
>                                matroska_cluster_parsing,
> -                             &matroska->current_cluster);
> +                             cluster);
>       }
>   
> -    if (!res &&
> -        matroska->current_cluster_num_blocks <
> -        matroska->current_cluster.blocks.nb_elem) {
> -        blocks_list = &matroska->current_cluster.blocks;
> -        blocks      = blocks_list->elem;
> +    if (!res && 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;
>   
> -        matroska->current_cluster_num_blocks = blocks_list->nb_elem;
> -        i                                    = blocks_list->nb_elem - 1;
> -        if (blocks[i].bin.size > 0 && blocks[i].bin.data) {
> -            int is_keyframe = blocks[i].non_simple ? blocks[i].reference == INT64_MIN : -1;
> -            uint8_t* additional = blocks[i].additional.size > 0 ?
> -                                    blocks[i].additional.data : NULL;
> -
> -            res = matroska_parse_block(matroska, blocks[i].bin.buf, blocks[i].bin.data,
> -                                       blocks[i].bin.size, blocks[i].bin.pos,
> +            res = matroska_parse_block(matroska, block->bin.buf, block->bin.data,
> +                                       block->bin.size, block->bin.pos,
>                                          matroska->current_cluster.timecode,
> -                                       blocks[i].duration, is_keyframe,
> -                                       additional, blocks[i].additional_id,
> -                                       blocks[i].additional.size,
> -                                       matroska->current_cluster_pos,
> -                                       blocks[i].discard_padding);
> -        }
> +                                       block->duration, is_keyframe,
> +                                       additional, block->additional_id,
> +                                       block->additional.size,
> +                                       cluster->pos,
> +                                       block->discard_padding);
>       }
>   
> +    ebml_free(matroska_blockgroup, block);
> +    memset(block, 0, sizeof(*block));
> +
>       return res;
>   }
>   
> @@ -3591,7 +3581,6 @@ static int matroska_read_close(AVFormatContext *s)
>       for (n = 0; n < matroska->tracks.nb_elem; n++)
>           if (tracks[n].type == MATROSKA_TRACK_TYPE_AUDIO)
>               av_freep(&tracks[n].audio.buf);
> -    ebml_free(matroska_cluster_parsing, &matroska->current_cluster);
>       ebml_free(matroska_segment, matroska);
>   
>       return 0;
> -- 
> 2.19.2
>
> _______________________________________________
> 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".
Oliver Collyer via ffmpeg-devel April 7, 2019, 9:38 a.m.
Steve Lhomme:
> On 3/27/2019 12:18 PM, Andreas Rheinhardt via ffmpeg-devel wrote:
>> Before this commit, the Matroska muxer would read a block when required
>> to do so, parse the block, create and return the necessary AVPackets
>> and
>> yet keep the blocks (in a dynamically allocated list), although they
>> aren't used at all any more. This has been changed. There is no list
>> any
>> more and the block is immediately discarded after parsing.
> 
> My understanding of the code is that the blocks are put in a queue,

Yes, the parsed blocks are put in a queue of their own; so we don't
need to keep all the raw data of all the already parsed blocks of the
current cluster around. (Refcounting means that some of this data
might be keep a little longer though, but even in this case this patch
eliminates e.g. the constant reallocation of the list of blocks.)

> that's whatwebm_clusters_start_with_keyframe() uses to check that the
> first frame is a keyframe

Yes.

> (it doesn't check the type of the frame though...).

I see a "if (!(pkt->flags & AV_PKT_FLAG_KEY))" in there.

> But since there's only one read
> inmatroska_parse_cluster_incremental()there's only one kept in memory.
> 
> So LGTM.
> 
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@googlemail.com>
>> ---
>>   libavformat/matroskadec.c | 87
>> +++++++++++++++++----------------------
>>   1 file changed, 38 insertions(+), 49 deletions(-)
>>
>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>> index 9198fa1bea..997c96d78f 100644
>> --- a/libavformat/matroskadec.c
>> +++ b/libavformat/matroskadec.c
>> @@ -304,9 +304,20 @@ typedef struct MatroskaLevel {
>>       uint64_t length;
>>   } MatroskaLevel;
>>   +typedef struct MatroskaBlock {
>> +    uint64_t duration;
>> +    int64_t  reference;
>> +    uint64_t non_simple;
>> +    EbmlBin  bin;
>> +    uint64_t additional_id;
>> +    EbmlBin  additional;
>> +    int64_t discard_padding;
>> +} MatroskaBlock;
>> +
>>   typedef struct MatroskaCluster {
>> +    MatroskaBlock block;
>>       uint64_t timecode;
>> -    EbmlList blocks;
>> +    int64_t pos;
>>   } MatroskaCluster;
>>     typedef struct MatroskaLevel1Element {
>> @@ -356,8 +367,6 @@ typedef struct MatroskaDemuxContext {
>>       MatroskaLevel1Element level1_elems[64];
>>       int num_level1_elems;
>>   -    int current_cluster_num_blocks;
>> -    int64_t current_cluster_pos;
>>       MatroskaCluster current_cluster;
>>         /* WebM DASH Manifest live flag */
>> @@ -367,16 +376,6 @@ typedef struct MatroskaDemuxContext {
>>       int bandwidth;
>>   } MatroskaDemuxContext;
>>   -typedef struct MatroskaBlock {
>> -    uint64_t duration;
>> -    int64_t  reference;
>> -    uint64_t non_simple;
>> -    EbmlBin  bin;
>> -    uint64_t additional_id;
>> -    EbmlBin  additional;
>> -    int64_t discard_padding;
>> -} MatroskaBlock;
>> -
>>   static const EbmlSyntax ebml_header[] = {
>>       { EBML_ID_EBMLREADVERSION,    EBML_UINT, 0, offsetof(Ebml,
>> version),         { .u = EBML_VERSION } },
>>       { EBML_ID_EBMLMAXSIZELENGTH,  EBML_UINT, 0, offsetof(Ebml,
>> max_size),        { .u = 8 } },
>> @@ -705,9 +704,9 @@ static const EbmlSyntax matroska_blockgroup[] = {
>>   };
>>     static const EbmlSyntax matroska_cluster_parsing[] = {
>> -    { MATROSKA_ID_CLUSTERTIMECODE, EBML_UINT,
>> 0,                     offsetof(MatroskaCluster, timecode) },
>> -    { MATROSKA_ID_BLOCKGROUP,      EBML_NEST,
>> sizeof(MatroskaBlock), offsetof(MatroskaCluster, blocks), { .n =
>> matroska_blockgroup } },
>> -    { MATROSKA_ID_SIMPLEBLOCK,     EBML_PASS,
>> sizeof(MatroskaBlock), offsetof(MatroskaCluster, blocks), { .n =
>> matroska_blockgroup } },
>> +    { MATROSKA_ID_CLUSTERTIMECODE, EBML_UINT, 0,
>> offsetof(MatroskaCluster, timecode) },
>> +    { MATROSKA_ID_BLOCKGROUP,      EBML_NEST, 0, 0, { .n =
>> matroska_blockgroup } },
>> +    { MATROSKA_ID_SIMPLEBLOCK,     EBML_PASS, 0, 0, { .n =
>> matroska_blockgroup } },
>>       { MATROSKA_ID_CLUSTERPOSITION, EBML_NONE },
>>       { MATROSKA_ID_CLUSTERPREVSIZE, EBML_NONE },
>>       { MATROSKA_ID_INFO,            EBML_NONE },
>> @@ -3443,57 +3442,48 @@ end:
>>     static int matroska_parse_cluster(MatroskaDemuxContext *matroska)
>>   {
>> -    EbmlList *blocks_list;
>> -    MatroskaBlock *blocks;
>> -    int i, res;
>> +    MatroskaCluster *cluster = &matroska->current_cluster;
>> +    MatroskaBlock     *block = &cluster->block;
>> +    int res;
>>       res = ebml_parse(matroska,
>>                        matroska_cluster_parsing,
>> -                     &matroska->current_cluster);
>> +                     cluster);
>>       if (res == 1) {
>>           /* New Cluster */
>> -        if (matroska->current_cluster_pos)
>> +        if (cluster->pos)
>>               ebml_level_end(matroska);
>> -        ebml_free(matroska_cluster_parsing,
>> &matroska->current_cluster);
>> -        memset(&matroska->current_cluster, 0,
>> sizeof(MatroskaCluster));
>> -        matroska->current_cluster_num_blocks = 0;
>> -        matroska->current_cluster_pos        =
>> avio_tell(matroska->ctx->pb);
>> +        cluster->pos = avio_tell(matroska->ctx->pb);
>>           /* sizeof the ID which was already read */
>>           if (matroska->current_id)
>> -            matroska->current_cluster_pos -= 4;
>> +            cluster->pos -= 4;
>>           res = ebml_parse(matroska,
>>                            matroska_clusters,
>> -                         &matroska->current_cluster);
>> +                         cluster);
>>           /* Try parsing the block again. */
>>           if (res == 1)
>>               res = ebml_parse(matroska,
>>                                matroska_cluster_parsing,
>> -                             &matroska->current_cluster);
>> +                             cluster);
>>       }
>>   -    if (!res &&
>> -        matroska->current_cluster_num_blocks <
>> -        matroska->current_cluster.blocks.nb_elem) {
>> -        blocks_list = &matroska->current_cluster.blocks;
>> -        blocks      = blocks_list->elem;
>> +    if (!res && 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;
>>   -        matroska->current_cluster_num_blocks = blocks_list->nb_elem;
>> -        i                                    = blocks_list->nb_elem
>> - 1;
>> -        if (blocks[i].bin.size > 0 && blocks[i].bin.data) {
>> -            int is_keyframe = blocks[i].non_simple ?
>> blocks[i].reference == INT64_MIN : -1;
>> -            uint8_t* additional = blocks[i].additional.size > 0 ?
>> -                                    blocks[i].additional.data : NULL;
>> -
>> -            res = matroska_parse_block(matroska, blocks[i].bin.buf,
>> blocks[i].bin.data,
>> -                                       blocks[i].bin.size,
>> blocks[i].bin.pos,
>> +            res = matroska_parse_block(matroska, block->bin.buf,
>> block->bin.data,
>> +                                       block->bin.size,
>> block->bin.pos,
>>                                         
>> matroska->current_cluster.timecode,
>> -                                       blocks[i].duration,
>> is_keyframe,
>> -                                       additional,
>> blocks[i].additional_id,
>> -                                       blocks[i].additional.size,
>> -                                       matroska->current_cluster_pos,
>> -                                       blocks[i].discard_padding);
>> -        }
>> +                                       block->duration, is_keyframe,
>> +                                       additional,
>> block->additional_id,
>> +                                       block->additional.size,
>> +                                       cluster->pos,
>> +                                       block->discard_padding);
>>       }
>>   +    ebml_free(matroska_blockgroup, block);
>> +    memset(block, 0, sizeof(*block));
>> +
>>       return res;
>>   }
>>   @@ -3591,7 +3581,6 @@ static int
>> matroska_read_close(AVFormatContext *s)
>>       for (n = 0; n < matroska->tracks.nb_elem; n++)
>>           if (tracks[n].type == MATROSKA_TRACK_TYPE_AUDIO)
>>               av_freep(&tracks[n].audio.buf);
>> -    ebml_free(matroska_cluster_parsing, &matroska->current_cluster);
>>       ebml_free(matroska_segment, matroska);
>>         return 0;
>> -- 
>> 2.19.2
>>
>> _______________________________________________
>> 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".
> 
> _______________________________________________
> 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".
Steve Lhomme April 8, 2019, 6:46 a.m.
> On April 7, 2019 at 11:38 AM Andreas Rheinhardt via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> wrote:
> 
> 
> Steve Lhomme:
> > On 3/27/2019 12:18 PM, Andreas Rheinhardt via ffmpeg-devel wrote:
> >> Before this commit, the Matroska muxer would read a block when required
> >> to do so, parse the block, create and return the necessary AVPackets
> >> and
> >> yet keep the blocks (in a dynamically allocated list), although they
> >> aren't used at all any more. This has been changed. There is no list
> >> any
> >> more and the block is immediately discarded after parsing.
> > 
> > My understanding of the code is that the blocks are put in a queue,
> 
> Yes, the parsed blocks are put in a queue of their own; so we don't
> need to keep all the raw data of all the already parsed blocks of the
> current cluster around. (Refcounting means that some of this data
> might be keep a little longer though, but even in this case this patch
> eliminates e.g. the constant reallocation of the list of blocks.)
> 
> > that's whatwebm_clusters_start_with_keyframe() uses to check that the
> > first frame is a keyframe
> 
> Yes.
> 
> > (it doesn't check the type of the frame though...).
> 
> I see a "if (!(pkt->flags & AV_PKT_FLAG_KEY))" in there.

By type I meant audio/video/subtitle. Although in WebM originally the audio was supposed to be put before the corresponding audio. So this code checks if the first audio packet is a keyframe. I don't think that's the intention of this code. But that's not related to your patch.

> > But since there's only one read
> > inmatroska_parse_cluster_incremental()there's only one kept in memory.
> > 
> > So LGTM.
> > 
> >>
> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@googlemail.com>
> >> ---
> >>   libavformat/matroskadec.c | 87
> >> +++++++++++++++++----------------------
> >>   1 file changed, 38 insertions(+), 49 deletions(-)
> >>
> >> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> >> index 9198fa1bea..997c96d78f 100644
> >> --- a/libavformat/matroskadec.c
> >> +++ b/libavformat/matroskadec.c
> >> @@ -304,9 +304,20 @@ typedef struct MatroskaLevel {
> >>       uint64_t length;
> >>   } MatroskaLevel;
> >>   +typedef struct MatroskaBlock {
> >> +    uint64_t duration;
> >> +    int64_t  reference;
> >> +    uint64_t non_simple;
> >> +    EbmlBin  bin;
> >> +    uint64_t additional_id;
> >> +    EbmlBin  additional;
> >> +    int64_t discard_padding;
> >> +} MatroskaBlock;
> >> +
> >>   typedef struct MatroskaCluster {
> >> +    MatroskaBlock block;
> >>       uint64_t timecode;
> >> -    EbmlList blocks;
> >> +    int64_t pos;
> >>   } MatroskaCluster;
> >>     typedef struct MatroskaLevel1Element {
> >> @@ -356,8 +367,6 @@ typedef struct MatroskaDemuxContext {
> >>       MatroskaLevel1Element level1_elems[64];
> >>       int num_level1_elems;
> >>   -    int current_cluster_num_blocks;
> >> -    int64_t current_cluster_pos;
> >>       MatroskaCluster current_cluster;
> >>         /* WebM DASH Manifest live flag */
> >> @@ -367,16 +376,6 @@ typedef struct MatroskaDemuxContext {
> >>       int bandwidth;
> >>   } MatroskaDemuxContext;
> >>   -typedef struct MatroskaBlock {
> >> -    uint64_t duration;
> >> -    int64_t  reference;
> >> -    uint64_t non_simple;
> >> -    EbmlBin  bin;
> >> -    uint64_t additional_id;
> >> -    EbmlBin  additional;
> >> -    int64_t discard_padding;
> >> -} MatroskaBlock;
> >> -
> >>   static const EbmlSyntax ebml_header[] = {
> >>       { EBML_ID_EBMLREADVERSION,    EBML_UINT, 0, offsetof(Ebml,
> >> version),         { .u = EBML_VERSION } },
> >>       { EBML_ID_EBMLMAXSIZELENGTH,  EBML_UINT, 0, offsetof(Ebml,
> >> max_size),        { .u = 8 } },
> >> @@ -705,9 +704,9 @@ static const EbmlSyntax matroska_blockgroup[] = {
> >>   };
> >>     static const EbmlSyntax matroska_cluster_parsing[] = {
> >> -    { MATROSKA_ID_CLUSTERTIMECODE, EBML_UINT,
> >> 0,                     offsetof(MatroskaCluster, timecode) },
> >> -    { MATROSKA_ID_BLOCKGROUP,      EBML_NEST,
> >> sizeof(MatroskaBlock), offsetof(MatroskaCluster, blocks), { .n =
> >> matroska_blockgroup } },
> >> -    { MATROSKA_ID_SIMPLEBLOCK,     EBML_PASS,
> >> sizeof(MatroskaBlock), offsetof(MatroskaCluster, blocks), { .n =
> >> matroska_blockgroup } },
> >> +    { MATROSKA_ID_CLUSTERTIMECODE, EBML_UINT, 0,
> >> offsetof(MatroskaCluster, timecode) },
> >> +    { MATROSKA_ID_BLOCKGROUP,      EBML_NEST, 0, 0, { .n =
> >> matroska_blockgroup } },
> >> +    { MATROSKA_ID_SIMPLEBLOCK,     EBML_PASS, 0, 0, { .n =
> >> matroska_blockgroup } },
> >>       { MATROSKA_ID_CLUSTERPOSITION, EBML_NONE },
> >>       { MATROSKA_ID_CLUSTERPREVSIZE, EBML_NONE },
> >>       { MATROSKA_ID_INFO,            EBML_NONE },
> >> @@ -3443,57 +3442,48 @@ end:
> >>     static int matroska_parse_cluster(MatroskaDemuxContext *matroska)
> >>   {
> >> -    EbmlList *blocks_list;
> >> -    MatroskaBlock *blocks;
> >> -    int i, res;
> >> +    MatroskaCluster *cluster = &matroska->current_cluster;
> >> +    MatroskaBlock     *block = &cluster->block;
> >> +    int res;
> >>       res = ebml_parse(matroska,
> >>                        matroska_cluster_parsing,
> >> -                     &matroska->current_cluster);
> >> +                     cluster);
> >>       if (res == 1) {
> >>           /* New Cluster */
> >> -        if (matroska->current_cluster_pos)
> >> +        if (cluster->pos)
> >>               ebml_level_end(matroska);
> >> -        ebml_free(matroska_cluster_parsing,
> >> &matroska->current_cluster);
> >> -        memset(&matroska->current_cluster, 0,
> >> sizeof(MatroskaCluster));
> >> -        matroska->current_cluster_num_blocks = 0;
> >> -        matroska->current_cluster_pos        =
> >> avio_tell(matroska->ctx->pb);
> >> +        cluster->pos = avio_tell(matroska->ctx->pb);
> >>           /* sizeof the ID which was already read */
> >>           if (matroska->current_id)
> >> -            matroska->current_cluster_pos -= 4;
> >> +            cluster->pos -= 4;
> >>           res = ebml_parse(matroska,
> >>                            matroska_clusters,
> >> -                         &matroska->current_cluster);
> >> +                         cluster);
> >>           /* Try parsing the block again. */
> >>           if (res == 1)
> >>               res = ebml_parse(matroska,
> >>                                matroska_cluster_parsing,
> >> -                             &matroska->current_cluster);
> >> +                             cluster);
> >>       }
> >>   -    if (!res &&
> >> -        matroska->current_cluster_num_blocks <
> >> -        matroska->current_cluster.blocks.nb_elem) {
> >> -        blocks_list = &matroska->current_cluster.blocks;
> >> -        blocks      = blocks_list->elem;
> >> +    if (!res && 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;
> >>   -        matroska->current_cluster_num_blocks = blocks_list->nb_elem;
> >> -        i                                    = blocks_list->nb_elem
> >> - 1;
> >> -        if (blocks[i].bin.size > 0 && blocks[i].bin.data) {
> >> -            int is_keyframe = blocks[i].non_simple ?
> >> blocks[i].reference == INT64_MIN : -1;
> >> -            uint8_t* additional = blocks[i].additional.size > 0 ?
> >> -                                    blocks[i].additional.data : NULL;
> >> -
> >> -            res = matroska_parse_block(matroska, blocks[i].bin.buf,
> >> blocks[i].bin.data,
> >> -                                       blocks[i].bin.size,
> >> blocks[i].bin.pos,
> >> +            res = matroska_parse_block(matroska, block->bin.buf,
> >> block->bin.data,
> >> +                                       block->bin.size,
> >> block->bin.pos,
> >>                                         
> >> matroska->current_cluster.timecode,
> >> -                                       blocks[i].duration,
> >> is_keyframe,
> >> -                                       additional,
> >> blocks[i].additional_id,
> >> -                                       blocks[i].additional.size,
> >> -                                       matroska->current_cluster_pos,
> >> -                                       blocks[i].discard_padding);
> >> -        }
> >> +                                       block->duration, is_keyframe,
> >> +                                       additional,
> >> block->additional_id,
> >> +                                       block->additional.size,
> >> +                                       cluster->pos,
> >> +                                       block->discard_padding);
> >>       }
> >>   +    ebml_free(matroska_blockgroup, block);
> >> +    memset(block, 0, sizeof(*block));
> >> +
> >>       return res;
> >>   }
> >>   @@ -3591,7 +3581,6 @@ static int
> >> matroska_read_close(AVFormatContext *s)
> >>       for (n = 0; n < matroska->tracks.nb_elem; n++)
> >>           if (tracks[n].type == MATROSKA_TRACK_TYPE_AUDIO)
> >>               av_freep(&tracks[n].audio.buf);
> >> -    ebml_free(matroska_cluster_parsing, &matroska->current_cluster);
> >>       ebml_free(matroska_segment, matroska);
> >>         return 0;
> >> -- 
> >> 2.19.2
> >>
> >> _______________________________________________
> >> 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".
> > 
> > _______________________________________________
> > 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".
> 
> _______________________________________________
> 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".

Patch hide | download patch | download mbox

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 9198fa1bea..997c96d78f 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -304,9 +304,20 @@  typedef struct MatroskaLevel {
     uint64_t length;
 } MatroskaLevel;
 
+typedef struct MatroskaBlock {
+    uint64_t duration;
+    int64_t  reference;
+    uint64_t non_simple;
+    EbmlBin  bin;
+    uint64_t additional_id;
+    EbmlBin  additional;
+    int64_t discard_padding;
+} MatroskaBlock;
+
 typedef struct MatroskaCluster {
+    MatroskaBlock block;
     uint64_t timecode;
-    EbmlList blocks;
+    int64_t pos;
 } MatroskaCluster;
 
 typedef struct MatroskaLevel1Element {
@@ -356,8 +367,6 @@  typedef struct MatroskaDemuxContext {
     MatroskaLevel1Element level1_elems[64];
     int num_level1_elems;
 
-    int current_cluster_num_blocks;
-    int64_t current_cluster_pos;
     MatroskaCluster current_cluster;
 
     /* WebM DASH Manifest live flag */
@@ -367,16 +376,6 @@  typedef struct MatroskaDemuxContext {
     int bandwidth;
 } MatroskaDemuxContext;
 
-typedef struct MatroskaBlock {
-    uint64_t duration;
-    int64_t  reference;
-    uint64_t non_simple;
-    EbmlBin  bin;
-    uint64_t additional_id;
-    EbmlBin  additional;
-    int64_t discard_padding;
-} MatroskaBlock;
-
 static const EbmlSyntax ebml_header[] = {
     { EBML_ID_EBMLREADVERSION,    EBML_UINT, 0, offsetof(Ebml, version),         { .u = EBML_VERSION } },
     { EBML_ID_EBMLMAXSIZELENGTH,  EBML_UINT, 0, offsetof(Ebml, max_size),        { .u = 8 } },
@@ -705,9 +704,9 @@  static const EbmlSyntax matroska_blockgroup[] = {
 };
 
 static const EbmlSyntax matroska_cluster_parsing[] = {
-    { MATROSKA_ID_CLUSTERTIMECODE, EBML_UINT, 0,                     offsetof(MatroskaCluster, timecode) },
-    { MATROSKA_ID_BLOCKGROUP,      EBML_NEST, sizeof(MatroskaBlock), offsetof(MatroskaCluster, blocks), { .n = matroska_blockgroup } },
-    { MATROSKA_ID_SIMPLEBLOCK,     EBML_PASS, sizeof(MatroskaBlock), offsetof(MatroskaCluster, blocks), { .n = matroska_blockgroup } },
+    { MATROSKA_ID_CLUSTERTIMECODE, EBML_UINT, 0, offsetof(MatroskaCluster, timecode) },
+    { MATROSKA_ID_BLOCKGROUP,      EBML_NEST, 0, 0, { .n = matroska_blockgroup } },
+    { MATROSKA_ID_SIMPLEBLOCK,     EBML_PASS, 0, 0, { .n = matroska_blockgroup } },
     { MATROSKA_ID_CLUSTERPOSITION, EBML_NONE },
     { MATROSKA_ID_CLUSTERPREVSIZE, EBML_NONE },
     { MATROSKA_ID_INFO,            EBML_NONE },
@@ -3443,57 +3442,48 @@  end:
 
 static int matroska_parse_cluster(MatroskaDemuxContext *matroska)
 {
-    EbmlList *blocks_list;
-    MatroskaBlock *blocks;
-    int i, res;
+    MatroskaCluster *cluster = &matroska->current_cluster;
+    MatroskaBlock     *block = &cluster->block;
+    int res;
     res = ebml_parse(matroska,
                      matroska_cluster_parsing,
-                     &matroska->current_cluster);
+                     cluster);
     if (res == 1) {
         /* New Cluster */
-        if (matroska->current_cluster_pos)
+        if (cluster->pos)
             ebml_level_end(matroska);
-        ebml_free(matroska_cluster_parsing, &matroska->current_cluster);
-        memset(&matroska->current_cluster, 0, sizeof(MatroskaCluster));
-        matroska->current_cluster_num_blocks = 0;
-        matroska->current_cluster_pos        = avio_tell(matroska->ctx->pb);
+        cluster->pos = avio_tell(matroska->ctx->pb);
         /* sizeof the ID which was already read */
         if (matroska->current_id)
-            matroska->current_cluster_pos -= 4;
+            cluster->pos -= 4;
         res = ebml_parse(matroska,
                          matroska_clusters,
-                         &matroska->current_cluster);
+                         cluster);
         /* Try parsing the block again. */
         if (res == 1)
             res = ebml_parse(matroska,
                              matroska_cluster_parsing,
-                             &matroska->current_cluster);
+                             cluster);
     }
 
-    if (!res &&
-        matroska->current_cluster_num_blocks <
-        matroska->current_cluster.blocks.nb_elem) {
-        blocks_list = &matroska->current_cluster.blocks;
-        blocks      = blocks_list->elem;
+    if (!res && 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;
 
-        matroska->current_cluster_num_blocks = blocks_list->nb_elem;
-        i                                    = blocks_list->nb_elem - 1;
-        if (blocks[i].bin.size > 0 && blocks[i].bin.data) {
-            int is_keyframe = blocks[i].non_simple ? blocks[i].reference == INT64_MIN : -1;
-            uint8_t* additional = blocks[i].additional.size > 0 ?
-                                    blocks[i].additional.data : NULL;
-
-            res = matroska_parse_block(matroska, blocks[i].bin.buf, blocks[i].bin.data,
-                                       blocks[i].bin.size, blocks[i].bin.pos,
+            res = matroska_parse_block(matroska, block->bin.buf, block->bin.data,
+                                       block->bin.size, block->bin.pos,
                                        matroska->current_cluster.timecode,
-                                       blocks[i].duration, is_keyframe,
-                                       additional, blocks[i].additional_id,
-                                       blocks[i].additional.size,
-                                       matroska->current_cluster_pos,
-                                       blocks[i].discard_padding);
-        }
+                                       block->duration, is_keyframe,
+                                       additional, block->additional_id,
+                                       block->additional.size,
+                                       cluster->pos,
+                                       block->discard_padding);
     }
 
+    ebml_free(matroska_blockgroup, block);
+    memset(block, 0, sizeof(*block));
+
     return res;
 }
 
@@ -3591,7 +3581,6 @@  static int matroska_read_close(AVFormatContext *s)
     for (n = 0; n < matroska->tracks.nb_elem; n++)
         if (tracks[n].type == MATROSKA_TRACK_TYPE_AUDIO)
             av_freep(&tracks[n].audio.buf);
-    ebml_free(matroska_cluster_parsing, &matroska->current_cluster);
     ebml_free(matroska_segment, matroska);
 
     return 0;