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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
_______________________________________________
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".
