Quoting wm4 (2016-02-26 10:34:56)
> On Thu, 25 Feb 2016 16:05:42 +0100
> Anton Khirnov <[email protected]> wrote:
>
> > Deprecate the current bitstream filtering API.
> > ---
> > configure | 2 +-
> > libavcodec/Makefile | 2 +
> > libavcodec/avcodec.h | 178 +++++++++++++++++++++++++++++++--
> > libavcodec/bitstream_filters.c | 51 ++++++++++
> > libavcodec/bsf.c | 217
> > +++++++++++++++++++++++++++++++++++++++++
> > libavcodec/bsf.h | 31 ++++++
> > libavcodec/version.h | 3 +
> > 7 files changed, 475 insertions(+), 9 deletions(-)
> > create mode 100644 libavcodec/bitstream_filters.c
> > create mode 100644 libavcodec/bsf.c
> > create mode 100644 libavcodec/bsf.h
> >
> > diff --git a/configure b/configure
> > index d8b8c07..0f59433 100755
> > --- a/configure
> > +++ b/configure
> > @@ -2517,7 +2517,6 @@ ENCODER_LIST=$(find_things encoder ENC
> > libavcodec/allcodecs.c)
> > DECODER_LIST=$(find_things decoder DEC libavcodec/allcodecs.c)
> > HWACCEL_LIST=$(find_things hwaccel HWACCEL libavcodec/allcodecs.c)
> > PARSER_LIST=$(find_things parser PARSER libavcodec/allcodecs.c)
> > -BSF_LIST=$(find_things bsf BSF libavcodec/allcodecs.c)
> > MUXER_LIST=$(find_things muxer _MUX libavformat/allformats.c)
> > DEMUXER_LIST=$(find_things demuxer DEMUX libavformat/allformats.c)
> > OUTDEV_LIST=$(find_things outdev OUTDEV libavdevice/alldevices.c)
> > @@ -2531,6 +2530,7 @@ find_things_extern(){
> > sed -n "s/^[^#]*extern.*$pattern *ff_\([^ ]*\)_$thing;/\1_$thing/p"
> > "$file"
> > }
> >
> > +BSF_LIST=$(find_things_extern bsf AVBitStreamFilter
> > libavcodec/bitstream_filters.c)
> > PROTOCOL_LIST=$(find_things_extern protocol URLProtocol
> > libavformat/protocols.c)
> >
> > ALL_COMPONENTS="
> > diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> > index 256dee3..ba1661d 100644
> > --- a/libavcodec/Makefile
> > +++ b/libavcodec/Makefile
> > @@ -19,6 +19,8 @@ OBJS = allcodecs.o
> > \
> > avpicture.o \
> > bitstream.o \
> > bitstream_filter.o \
> > + bitstream_filters.o \
> > + bsf.o \
> > codec_desc.o \
> > d3d11va.o \
> > dirac.o \
> > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > index 33de8ec..f6d474f 100644
> > --- a/libavcodec/avcodec.h
> > +++ b/libavcodec/avcodec.h
> > @@ -4692,35 +4692,197 @@ int av_get_audio_frame_duration(AVCodecContext
> > *avctx, int frame_bytes);
> > */
> > int av_get_audio_frame_duration2(AVCodecParameters *par, int frame_bytes);
> >
> > -
> > +#if FF_API_OLD_BSF
> > typedef struct AVBitStreamFilterContext {
> > void *priv_data;
> > struct AVBitStreamFilter *filter;
> > AVCodecParserContext *parser;
> > struct AVBitStreamFilterContext *next;
> > } AVBitStreamFilterContext;
> > +#endif
> > +
> > +typedef struct AVBSFInternal AVBSFInternal;
> > +
> > +typedef struct AVBSFContext {
> > + /**
> > + * A class for logging and AVOptions
> > + */
> > + const AVClass *av_class;
> > +
> > + /**
> > + * The bitstream filter this context is an instance of.
> > + */
> > + const struct AVBitStreamFilter *filter;
> > +
> > + /**
> > + * Opaque libavcodec internal data. Must not be touched by the caller
> > in any
> > + * way.
> > + */
> > + AVBSFInternal *internal;
> > +
> > + /**
> > + * Opaque filter-specific private data. If filter->priv_class is
> > non-NULL,
> > + * this is an AVOptions-enabled struct.
> > + */
> > + void *priv_data;
>
> Why is this not in internal?
Because private AVOptions live there.
>
> > +
> > + /**
> > + * Codec id this bitstream filter was initialized for.
> > + * Set by libavcodec in av_bsf_alloc().
> > + */
> > + enum AVCodecID codec_id;
> > +
> > + /**
> > + * Parameters of the input stream. Set by the caller before
> > av_bsf_init().
> > + */
> > + AVCodecParameters *par_in;
>
> Why is this not an argument to av_bsf_init() then? What about
> mid-stream changes? Are they maybe handled by side-data? If so, how does
> it interact with seeking (imagine seeking across format changes)?
Because there are multiple things you set between alloc and init, par_in
is just one of them. And we might want to add more.
>
> > +
> > + /**
> > + * Parameters of the output stream. Set by the filter in av_bsf_init().
> > + */
> > + AVCodecParameters *par_out;
>
> This and codec_id also makes me nervous... why have read-only fields
> in a public struct? Can this change during filtering? If it's supposed
> to be immutable, then you know some BSFs will start writing to it
> anyway.
Well, you need to export the information somehow. There are currently no
filters where any of those fields change during filtering. But if we
want to add them, the change could either be sent through side data (as
is done for demuxing), or signalled in some other way.
The good thing is that a BSF writing here randomly won't have any
effect, since (unlike with AVCodecContext in the old API), this struct
is not shared with anything else, so nobody would see the changes.
>
> >
> > + /**
> > + * The timebase used for the timestamps of the input packets. Set by
> > the
> > + * caller before av_bsf_init().
> > + */
> > + AVRational time_base_in;
> > +
> > + /**
> > + * The timebase used for the timestamps of the output packets. Set by
> > the
> > + * filter in av_bsf_init().
> > + */
> > + AVRational time_base_out;
> > +} AVBSFContext;
> >
> > typedef struct AVBitStreamFilter {
> > const char *name;
> > +
> > + /**
> > + * A list of codec ids supported by the filter, terminated by
> > + * AV_CODEC_ID_NONE.
> > + * May be NULL, in that case the bitstream filter works with any codec
> > id.
> > + */
> > + const enum AVCodecID *codec_ids;
> > +
> > + /**
> > + * A class for the private data, used to declare bitstream filter
> > private
> > + * AVOptions. This field is NULL for bitstream filters that do not
> > declare
> > + * any options.
> > + *
> > + * If this field is non-NULL, the first member of the filter private
> > data
> > + * must be a pointer to AVClass, which will be set by libavcodec
> > generic
> > + * code to this class.
> > + */
> > + const AVClass *priv_class;
> > +
> > + /*****************************************************************
> > + * No fields below this line are part of the public API. They
> > + * may not be used outside of libavcodec and can be changed and
> > + * removed at will.
> > + * New public fields should be added right above.
> > + *****************************************************************
> > + */
> > +
> > int priv_data_size;
>
> So priv_data_size and those callbacks were public before?
I wouldn't say so. In the old times when this API was created, people
just didn't think too long about whether stuff was private or public or
whatever. So in my view, its API status was undefined. And since there
is no good reason for any caller to access those fields, we can make
them "officially" private.
>
> > - int (*filter)(AVBitStreamFilterContext *bsfc,
> > - AVCodecContext *avctx, const char *args,
> > - uint8_t **poutbuf, int *poutbuf_size,
> > - const uint8_t *buf, int buf_size, int keyframe);
> > - void (*close)(AVBitStreamFilterContext *bsfc);
> > - struct AVBitStreamFilter *next;
> > + int (*init)(AVBSFContext *ctx);
> > + int (*filter)(AVBSFContext *ctx, AVPacket *pkt);
> > + void (*close)(AVBSFContext *ctx);
> > } AVBitStreamFilter;
> >
> > +#if FF_API_OLD_BSF
> > +/**
> > + * @deprecated the old bitstream filtering API (using
> > AVBitStreamFilterContext)
> > + * is deprecated. Use the new bitstream filtering API (using AVBSFContext).
> > + */
> > +attribute_deprecated
> > void av_register_bitstream_filter(AVBitStreamFilter *bsf);
> > +attribute_deprecated
> > AVBitStreamFilterContext *av_bitstream_filter_init(const char *name);
> > +attribute_deprecated
> > int av_bitstream_filter_filter(AVBitStreamFilterContext *bsfc,
> > AVCodecContext *avctx, const char *args,
> > uint8_t **poutbuf, int *poutbuf_size,
> > const uint8_t *buf, int buf_size, int
> > keyframe);
> > +attribute_deprecated
> > void av_bitstream_filter_close(AVBitStreamFilterContext *bsf);
> > -
> > +attribute_deprecated
> > AVBitStreamFilter *av_bitstream_filter_next(const AVBitStreamFilter *f);
> > +#endif
>
> I would appreciate it if API weren't "hard deprecated" immediately, but
> only after say 1 release with the new API has been made.
Why not? Especially in this case, you want to switch away from it as
fast as possible. And in any case, the fact is that it's deprecated
right now. We might not remove it immediately at the next bump, but it's
still deprecated, in the "you should not use it" sense.
>
> > +
> > +/**
> > + * @return a bitstream filter with the specified name or NULL if no such
> > + * bitstream filter exists.
> > + */
> > +const AVBitStreamFilter *av_bsf_get_by_name(const char *name);
> > +
> > +/**
> > + * Iterate over all registered bitstream filters.
> > + *
> > + * @param opaque a pointer where libavcodec will store the iteration
> > state. Must
> > + * point to NULL to start the iteration.
> > + *
> > + * @return the next registered bitstream filter or NULL when the iteration
> > is
> > + * finished
> > + */
> > +const AVBitStreamFilter *av_bsf_next(void **opaque);
>
> Not sure why that needs a new function. That opaque thing is weird.
See my reply to Hendrik.
>
> > +
> > +/**
> > + * Allocate a context for a given bitstream filter. The caller must fill
> > in the
> > + * context parameters as described in the documentation and then call
> > + * av_bsf_init() before sending any data to the filter.
> > + *
> > + * @param filter the filter for which to allocate an instance.
> > + * @param ctx a pointer into which the pointer to the newly-allocated
> > context
> > + * will be written. It must be freed with av_bsf_free() after
> > the
> > + * filtering is done.
> > + * @param codec_id a codec id identifying the kind of data that will be
> > sent to
> > + * the filter.
>
> Why not just make it take a codec par?
Dropped the codec id locally.
>
> > + *
>
> > + * @return newly-allocated bitstream filter context or NULL on failure
>
> Wrong doxygen.
Right, fixed.
>
> > + */
> > +int av_bsf_alloc(const AVBitStreamFilter *filter, AVBSFContext **ctx,
> > + enum AVCodecID codec_id);
> > +
> > +/**
> > + * Prepare the filter for use, after all the parameters and options have
> > been
> > + * set.
> > + */
> > +int av_bsf_init(AVBSFContext *ctx);
> > +
> > +/**
> > + * Submit a packet for filtering.
> > + *
> > + * @param pkt the packet to filter. The bitstream filter will take
> > ownership of
> > + * the packet and reset the contents of pkt. pkt is not touched if an
> > error occurs.
> > + *
> > + * @return 0 on success, a negative AVERROR on error.
> > + */
> > +int av_bsf_send_packet(AVBSFContext *ctx, AVPacket *pkt);
>
> What happens if the user writes too many packets? Should it return
> EAGAIN?
It will buffer as many packets as you have the memory for. The doxy does
tell you explicitly that you should get all the output after each input,
so it will be the caller's problem. I could restrict the buffering, but
don't see much point in it.
>
> How do you signal EOF? How can the receive API signal EOF if you can't
> signal it on input?
By a NULL packet, seems I forgot to document it. Fixed locally.
>
> Why is there no way to reset EOF?
Why should there be? Just make a new filter.
>
> > +
> > +/**
> > + * Retrieve a filtered packet.
> > + *
> > + * @param[out] pkt this struct will be filled with the contents of the
> > filtered
> > + * packet. It is owned by the caller and must be freed
> > using
> > + * av_packet_unref() when it is no longer needed.
>
> What happens on error? Does *pkt need to be initialized in any way?
> (Such clarifications are missing from many Libav APIs.)
>
> > + *
> > + * @return 0 on success. AVERROR(EAGAIN) if more packets need to be sent
> > to the
> > + * filter (using av_bsf_send_packet()) to get more output. AVERROR_EOF if
> > there
> > + * will be no further output from the filter. Another negative AVERROR
> > value if
> > + * an error occurs.
> > + *
> > + * @note one input packet may result in several output packets, so after
> > adding
> > + * a packet with av_bsf_send_packet(), this function needs to be called
> > + * repeatedly until it stops returning 0.
>
> You might also add that the reverse can happen, merging packets.
Ok, docs extended.
>
> > + */
> > +int av_bsf_receive_packet(AVBSFContext *ctx, AVPacket *pkt);
> > +
> > +/**
> > + * Free a bitstream filter context and everything associated with it;
> > write NULL
> > + * into the supplied pointer.
> > + */
> > +void av_bsf_free(AVBSFContext **ctx);
> >
> > /* memory */
> >
> > diff --git a/libavcodec/bitstream_filters.c b/libavcodec/bitstream_filters.c
> > new file mode 100644
> > index 0000000..5e406d7
> > --- /dev/null
> > +++ b/libavcodec/bitstream_filters.c
> > @@ -0,0 +1,51 @@
> > +/*
> > + * This file is part of Libav.
> > + *
> > + * Libav 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.
> > + *
> > + * Libav 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 Libav; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> > 02110-1301 USA
> > + */
> > +
> > +#include "config.h"
> > +
> > +#include "libavutil/common.h"
> > +
> > +#include "libavcodec/avcodec.h"
> > +
> > +static const AVBitStreamFilter *bitstream_filters[] = {
> > + NULL,
> > +};
> > +
> > +const AVBitStreamFilter *av_bsf_next(void **opaque)
> > +{
> > + uintptr_t i = (uintptr_t)*opaque;
> > + const AVBitStreamFilter *f = bitstream_filters[i];
> > +
> > + if (f)
> > + *opaque = (void*)(i + 1);
> > +
> > + return f;
> > +}
> > +
> > +const AVBitStreamFilter *av_bsf_get_by_name(const char *name)
> > +{
> > + int i;
> > +
> > + for (i = 0; bitstream_filters[i]; i++) {
> > + const AVBitStreamFilter *f = bitstream_filters[i];
> > + if (!strcmp(f->name, name))
> > + return f;
> > + }
> > +
> > + return NULL;
> > +}
> > diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
> > new file mode 100644
> > index 0000000..ed4aef8
> > --- /dev/null
> > +++ b/libavcodec/bsf.c
> > @@ -0,0 +1,217 @@
> > +/*
> > + * This file is part of Libav.
> > + *
> > + * Libav 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.
> > + *
> > + * Libav 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 Libav; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> > 02110-1301 USA
> > + */
> > +
> > +#include <string.h>
> > +
> > +#include "libavutil/fifo.h"
> > +#include "libavutil/log.h"
> > +#include "libavutil/mem.h"
> > +#include "libavutil/opt.h"
> > +
> > +#include "avcodec.h"
> > +#include "bsf.h"
> > +
> > +struct AVBSFInternal {
> > + AVFifoBuffer *fifo;
> > + int eof;
> > +};
> > +
> > +void av_bsf_free(AVBSFContext **pctx)
> > +{
> > + AVBSFContext *ctx;
> > +
> > + if (!pctx || !*pctx)
> > + return;
> > + ctx = *pctx;
> > +
> > + if (ctx->filter->close)
> > + ctx->filter->close(ctx);
> > + if (ctx->filter->priv_class && ctx->priv_data)
> > + av_opt_free(ctx->priv_data);
> > +
> > + av_opt_free(ctx);
> > +
> > + if (ctx->internal) {
> > + while (ctx->internal->fifo && av_fifo_size(ctx->internal->fifo)) {
> > + AVPacket *pkt;
> > + av_fifo_generic_read(ctx->internal->fifo, &pkt, sizeof(pkt),
> > NULL);
> > + av_packet_free(&pkt);
> > + }
> > + av_fifo_free(ctx->internal->fifo);
> > + }
> > + av_freep(&ctx->internal);
> > + av_freep(&ctx->priv_data);
> > +
> > + avcodec_parameters_free(&ctx->par_in);
> > + avcodec_parameters_free(&ctx->par_out);
> > +
> > + av_freep(pctx);
> > +}
> > +
> > +static const AVClass bsf_class = {
> > + .class_name = "AVBSFContext",
> > + .item_name = av_default_item_name,
> > + .version = LIBAVUTIL_VERSION_INT,
> > +};
> > +
> > +int av_bsf_alloc(const AVBitStreamFilter *filter, AVBSFContext **pctx,
> > + enum AVCodecID codec_id)
> > +{
> > + AVBSFContext *ctx;
> > + int i, ret;
> > +
> > + ctx = av_mallocz(sizeof(*ctx));
> > + if (!ctx)
> > + return AVERROR(ENOMEM);
> > +
> > + ctx->av_class = &bsf_class;
>
> I would have thought it depends on the filter...
>
The global class doesn't. The priv data class is filter-specific. That's
the way it's done for muxers/demuxers and codecs too.
--
Anton Khirnov
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
