[FFmpeg-devel] [PATCH 2/5 v4] avcodec: add an AV1 frame merge bitstream filter

[James Almer]

On 11/12/2019 6:24 PM, Andreas Rheinhardt wrote:
> James Almer:
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>> Now using an index to swap fragments.
>>
>> I noticed that the input packet containing props is not necessarily the first
>> in a TU, so adapted the code accordingly. This fixes warnings about missing
>> timestamps when trying to merge the output of av1_frame_split, where the source
>> had the visible frames at the end of a TU rather than at the beginning.
>>
>> Not going to bother with indexes for the packets. av_packet_move_ref() is far
>> from expensive, unlike creating buffer references and expanding the fragment's
>> unit buffer.
>>
>>  configure                        |   1 +
>>  libavcodec/Makefile              |   1 +
>>  libavcodec/av1_frame_merge_bsf.c | 169 +++++++++++++++++++++++++++++++
>>  libavcodec/bitstream_filters.c   |   1 +
>>  4 files changed, 172 insertions(+)
>>  create mode 100644 libavcodec/av1_frame_merge_bsf.c
>>
>> diff --git a/configure b/configure
>> index 1de90e93fd..70f60997c1 100755
>> --- a/configure
>> +++ b/configure
>> @@ -3115,6 +3115,7 @@ vc1_parser_select="vc1dsp"
>>  
>>  # bitstream_filters
>>  aac_adtstoasc_bsf_select="adts_header"
>> +av1_frame_merge_bsf_select="cbs_av1"
>>  av1_frame_split_bsf_select="cbs_av1"
>>  av1_metadata_bsf_select="cbs_av1"
>>  eac3_core_bsf_select="ac3_parser"
>> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
>> index b990c6ba87..006a472a6d 100644
>> --- a/libavcodec/Makefile
>> +++ b/libavcodec/Makefile
>> @@ -1075,6 +1075,7 @@ OBJS-$(CONFIG_XMA_PARSER)              += xma_parser.o
>>  # bitstream filters
>>  OBJS-$(CONFIG_AAC_ADTSTOASC_BSF)          += aac_adtstoasc_bsf.o mpeg4audio.o
>>  OBJS-$(CONFIG_AV1_METADATA_BSF)           += av1_metadata_bsf.o
>> +OBJS-$(CONFIG_AV1_FRAME_MERGE_BSF)        += av1_frame_merge_bsf.o
>>  OBJS-$(CONFIG_AV1_FRAME_SPLIT_BSF)        += av1_frame_split_bsf.o
>>  OBJS-$(CONFIG_CHOMP_BSF)                  += chomp_bsf.o
>>  OBJS-$(CONFIG_DUMP_EXTRADATA_BSF)         += dump_extradata_bsf.o
>> diff --git a/libavcodec/av1_frame_merge_bsf.c b/libavcodec/av1_frame_merge_bsf.c
>> new file mode 100644
>> index 0000000000..96a9e6e863
>> --- /dev/null
>> +++ b/libavcodec/av1_frame_merge_bsf.c
>> @@ -0,0 +1,169 @@
>> +/*
>> + * Copyright (c) 2019 James Almer <jamrial at gmail.com>
>> + *
>> + * This file is part of FFmpeg.
>> + *
>> + * FFmpeg is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * FFmpeg is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with FFmpeg; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>> + */
>> +
>> +#include "avcodec.h"
>> +#include "bsf.h"
>> +#include "cbs.h"
>> +#include "cbs_av1.h"
>> +
>> +typedef struct AV1FMergeContext {
>> +    CodedBitstreamContext *cbc;
>> +    CodedBitstreamFragment frag[2];
>> +    AVPacket *pkt, *in;
>> +    int idx;
>> +} AV1FMergeContext;
>> +
>> +static int av1_frame_merge_filter(AVBSFContext *bsf, AVPacket *out)
>> +{
>> +    AV1FMergeContext *ctx = bsf->priv_data;
>> +    CodedBitstreamFragment *frag = &ctx->frag[ctx->idx], *tu = &ctx->frag[!ctx->idx];
>> +    AVPacket *in = ctx->in, *buffer_pkt = ctx->pkt;
>> +    int err, i;
>> +
>> +    err = ff_bsf_get_packet_ref(bsf, in);
>> +    if (err < 0) {
>> +        if (err == AVERROR_EOF && tu->nb_units > 0)
>> +            goto eof;
>> +        return err;
>> +    }
>> +
>> +    err = ff_cbs_read_packet(ctx->cbc, frag, in);
>> +    if (err < 0) {
>> +        av_log(bsf, AV_LOG_ERROR, "Failed to read packet.\n");
>> +        goto fail;
>> +    }
>> +
>> +    if (frag->nb_units == 0) {
>> +        av_log(bsf, AV_LOG_ERROR, "No OBU in packet.\n");
>> +        err = AVERROR_INVALIDDATA;
>> +        goto fail;
>> +    }
>> +
>> +    if (tu->nb_units == 0 && frag->units[0].type != AV1_OBU_TEMPORAL_DELIMITER) {
>> +        av_log(bsf, AV_LOG_ERROR, "Missing Temporal Delimiter.\n");
>> +        err = AVERROR_INVALIDDATA;
>> +        goto fail;
>> +    }
>> +
>> +    if (tu->nb_units > 0 && frag->units[0].type == AV1_OBU_TEMPORAL_DELIMITER) {
>> +eof:
>> +        err = ff_cbs_write_packet(ctx->cbc, buffer_pkt, tu);
>> +        if (err < 0) {
>> +            av_log(bsf, AV_LOG_ERROR, "Failed to write packet.\n");
>> +            goto fail;
>> +        }
>> +        av_packet_move_ref(out, buffer_pkt);
>> +
>> +        ff_cbs_fragment_reset(ctx->cbc, tu);
>> +
>> +        for (i = 1; i < frag->nb_units; i++) {
>> +            if (frag->units[i].type == AV1_OBU_TEMPORAL_DELIMITER) {
>> +                av_log(bsf, AV_LOG_ERROR, "Temporal Delimiter in the middle of a packet.\n");
>> +                err = AVERROR_INVALIDDATA;
>> +                goto fail;
>> +            }
>> +        }
> 
> You can move the above loop to before this block. This allows to
> remove the equivalent check in the loop below as well as unreferencing
> the out packet on failure as it will always be clean.

Good idea. Changed.

> 
>> +        // Swap fragment index, to avoid copying fragment references.
>> +        ctx->idx = !ctx->idx;
>> +
>> +        // Buffer the packet if it has a timestamp.
>> +        if (in->pts != AV_NOPTS_VALUE)
>> +            av_packet_move_ref(buffer_pkt, in);
> 
> You could add an else here. You could do the same with the other unref
> below.

I know, i didn't because i thought it looked nicer this way, and
unreferencing a clean packet is basically a no-op. But sure, i'll change it.

> 
> (You could even combine the two: put the insertion of unit content
> into the else branch of this block, set err/ret to 0 in this block and
> to AVERROR(EAGAIN) in the else branch and use the below check for
> buffering.

err is set to 0 by ff_cbs_write_packet(), so no need to make it explicit.

> Resetting the fragments could also be combined with
> ff_cbs_fragment_reset(ctx->cbc, &ctx->frag[ctx->idx]).)

I fear it may be a bit too obfuscated for the casual reader, but ok.

> 
>> +        av_packet_unref(in);
>> +
>> +        return 0;
>> +    }
>> +
>> +    // Buffer packets with timestamps. There should be at most one per TU, be it split or not.
>> +    if (!buffer_pkt->data && in->pts != AV_NOPTS_VALUE)
>> +	    av_packet_move_ref(buffer_pkt, in);
>> +
>> +    for (i = 0; i < frag->nb_units; i++) {
>> +        if (i && frag->units[i].type == AV1_OBU_TEMPORAL_DELIMITER) {
>> +            av_log(bsf, AV_LOG_ERROR, "Temporal Delimiter in the middle of a packet.\n");
>> +            err = AVERROR_INVALIDDATA;
>> +            goto fail;
>> +        }
>> +        err = ff_cbs_insert_unit_content(ctx->cbc, tu, -1, frag->units[i].type,
>> +                                         frag->units[i].content, frag->units[i].content_ref);
>> +        if (err < 0)
>> +            goto fail;
>> +    }
>> +    ff_cbs_fragment_reset(ctx->cbc, frag);
>> +    av_packet_unref(in);
>> +
>> +    return AVERROR(EAGAIN);
>> +
>> +fail:
>> +    ff_cbs_fragment_reset(ctx->cbc, tu);
>> +    ff_cbs_fragment_reset(ctx->cbc, frag);
>> +    av_packet_unref(buffer_pkt);
>> +    av_packet_unref(in);
> 
> How about replacing the above with a call to av1_frame_merge_flush()?

Ok.

Applied your suggestions and pushed the set. Thanks a lot for all the
rounds of reviews.