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

[Andreas Rheinhardt]

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).

>>> +
>>> +    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
> _______________________________________________
> 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".