> 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.rheinha...@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-requ...@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-requ...@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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".