[FFmpeg-devel] [PATCH 07/27] cbs: Implement common parts of cbs-based bitstream filters separately

[Mark Thompson]

On 06/01/2021 20:01, Andreas Rheinhardt wrote:
> Mark Thompson:
>> On 01/01/2021 22:18, Andreas Rheinhardt wrote:
>>> Mark Thompson:
>>>> This allows removal of a lot of duplicated code between BSFs.
>>>> ---
>>>>    libavcodec/Makefile  |   2 +-
>>>>    libavcodec/cbs_bsf.c | 161 +++++++++++++++++++++++++++++++++++++++++++
>>>>    libavcodec/cbs_bsf.h | 100 +++++++++++++++++++++++++++
>>>>    3 files changed, 262 insertions(+), 1 deletion(-)
>>>>    create mode 100644 libavcodec/cbs_bsf.c
>>>>    create mode 100644 libavcodec/cbs_bsf.h
>>>>
>>>> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
>>>> index 6e12a8171d..8f50217ad4 100644
>>>> --- a/libavcodec/Makefile
>>>> +++ b/libavcodec/Makefile
>>>> @@ -69,7 +69,7 @@ OBJS-$(CONFIG_AUDIODSP)                += audiodsp.o
>>>>    OBJS-$(CONFIG_BLOCKDSP)                += blockdsp.o
>>>>    OBJS-$(CONFIG_BSWAPDSP)                += bswapdsp.o
>>>>    OBJS-$(CONFIG_CABAC)                   += cabac.o
>>>> -OBJS-$(CONFIG_CBS)                     += cbs.o
>>>> +OBJS-$(CONFIG_CBS)                     += cbs.o cbs_bsf.o
>>>>    OBJS-$(CONFIG_CBS_AV1)                 += cbs_av1.o
>>>>    OBJS-$(CONFIG_CBS_H264)                += cbs_h2645.o cbs_sei.o
>>>> h2645_parse.o
>>>>    OBJS-$(CONFIG_CBS_H265)                += cbs_h2645.o cbs_sei.o
>>>> h2645_parse.o
>>>> diff --git a/libavcodec/cbs_bsf.c b/libavcodec/cbs_bsf.c
>>>> new file mode 100644
>>>> index 0000000000..429f360014
>>>> --- /dev/null
>>>> +++ b/libavcodec/cbs_bsf.c
>>>> @@ -0,0 +1,161 @@
>>>> +/*
>>>> + * 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 "bsf_internal.h"
>>>> +#include "cbs_bsf.h"
>>>> +
>>>> +static int ff_cbs_bsf_update_side_data(AVBSFContext *bsf, AVPacket
>>>> *pkt)
>>>
>>> The ff_ prefix is not for static functions.
>>>
>>>> +{
>>>> +    CBSBSFContext           *ctx = bsf->priv_data;
>>>> +    CodedBitstreamFragment *frag = &ctx->fragment;
>>>> +    uint8_t *side_data;
>>>> +    int side_data_size;
>>>> +    int err;
>>>> +
>>>> +    side_data = av_packet_get_side_data(pkt, AV_PKT_DATA_NEW_EXTRADATA,
>>>> +                                        &side_data_size);
>>>> +    if (!side_data_size)
>>>> +        return 0;
>>>> +
>>>> +    err = ff_cbs_read(ctx->input, frag, side_data, side_data_size);
>>>> +    if (err < 0) {
>>>> +        av_log(bsf, AV_LOG_ERROR,
>>>> +               "Failed to read extradata from packet side data.\n");
>>>> +        return err;
>>>> +    }
>>>> +
>>>> +    err = ctx->type->update_fragment(bsf, NULL, frag);
>>>> +    if (err < 0)
>>>> +        goto fail;
>>>
>>> This is inconsistent: All the other errors paths out of this function do
>>> not reset the fragment; and that is fine, because the one and only
>>> caller already does so. (Did you intend for this function to be more
>>> standalone?)
>>
>> IIRC it originally was a standalone function, but got changed when it
>> was clear that every BSF was calling it in exactly the same way.  (None
>> of this code has changed since the previous version in August.)
>>
>> The reset is needed, because we are going to use the same fragment in
>> read_packet() immediately after the return.  So, added the gotos to all
>> other error cases except the first.
>>
> 
> The reset here is redundant, because the only caller already resets the
> fragment if ff_cbs_bsf_update_side_data returns an error.
> 
> And ff_cbs_read() returns unclean fragments on error, so treating it
> differently than an error from update_fragment is inconsistent (albeit
> not dangerous because (as said) the only caller resets the fragment).

Oops, yes, you are correct.  I was thinking about read_packet() being called after on the dirty fragment, but obviously that only happens if this didn't return an error so we only need the reset on success.

Removed all of the gotos.

>>>> +
>>>> +    err = ff_cbs_write_fragment_data(ctx->output, frag);
>>>> +    if (err < 0) {
>>>> +        av_log(bsf, AV_LOG_ERROR,
>>>> +               "Failed to write extradata into packet side data.\n");
>>>> +        return err;
>>>> +    }
>>>> +
>>>> +    side_data = av_packet_new_side_data(pkt, AV_PKT_DATA_NEW_EXTRADATA,
>>>> +                                        frag->data_size);
>>>> +    if (!side_data)
>>>> +        return AVERROR(ENOMEM);
>>>> +    memcpy(side_data, frag->data, frag->data_size);
>>>> +
>>>> +    err = 0;
>>>> +fail:
>>>> +    ff_cbs_fragment_reset(frag);
>>>> +    return err;
>>>> +}
>>>> +
>>>> ...

Thanks,

- Mark