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

Andreas Rheinhardt andreas.rheinhardt at googlemail.com
Sun Apr 7 12:38:00 EEST 2019


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 at 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 at ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".



More information about the ffmpeg-devel mailing list