On 11/12/2019 6:24 PM, Andreas Rheinhardt wrote:
> James Almer:
>> Signed-off-by: James Almer <jamr...@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 <jamr...@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.
_______________________________________________
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".
