[FFmpeg-devel] [PATCH 10/21] avformat/matroskadec: Don't keep old blocks
Steve Lhomme
robux4 at ycbcr.xyz
Mon Apr 8 09:46:48 EEST 2019
> On April 7, 2019 at 11:38 AM Andreas Rheinhardt via ffmpeg-devel <ffmpeg-devel at 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 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".
>
> _______________________________________________
> 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