[FFmpeg-devel] [PATCH] Add 'asetframesize' audiofilter
Stefano Sabatini
stefasab at gmail.com
Sat Feb 25 01:00:03 CET 2012
On date Thursday 2012-02-23 17:49:10 +0200, Andrey Utkin encoded:
> Filter that changes number of samples on single output operation
> ---
> libavfilter/Makefile | 1 +
> libavfilter/af_asetframesize.c | 191 ++++++++++++++++++++++++++++++++++++++++
> libavfilter/allfilters.c | 1 +
> 3 files changed, 193 insertions(+), 0 deletions(-)
> create mode 100644 libavfilter/af_asetframesize.c
>
> diff --git a/libavfilter/Makefile b/libavfilter/Makefile
> index 9f5fdcb..db7397b 100644
> --- a/libavfilter/Makefile
> +++ b/libavfilter/Makefile
> @@ -38,6 +38,7 @@ OBJS-$(CONFIG_EARWAX_FILTER) += af_earwax.o
> OBJS-$(CONFIG_PAN_FILTER) += af_pan.o
> OBJS-$(CONFIG_SILENCEDETECT_FILTER) += af_silencedetect.o
> OBJS-$(CONFIG_VOLUME_FILTER) += af_volume.o
> +OBJS-$(CONFIG_ASETFRAMESIZE_FILTER) += af_asetframesize.o
Nit: alphabetical order
> OBJS-$(CONFIG_ABUFFER_FILTER) += asrc_abuffer.o
> OBJS-$(CONFIG_AEVALSRC_FILTER) += asrc_aevalsrc.o
> diff --git a/libavfilter/af_asetframesize.c b/libavfilter/af_asetframesize.c
> new file mode 100644
> index 0000000..dd391fc
> --- /dev/null
> +++ b/libavfilter/af_asetframesize.c
> @@ -0,0 +1,191 @@
> +/*
> + * Copyright (c) 2012 Andrey Utkin
> + *
> + * 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
> + */
> +
> +/**
> + * @file
> + * Filter that changes number of samples on single output operation
> + */
> +
> +#include "avfilter.h"
> +#include "libavutil/fifo.h"
> +
> +typedef struct {
> + unsigned int framesize; // how many samples to output
uhm uhm I'm boring but framesize is usually used for specifying the
size of a buffer in bytes, so this should be changed to something less
ambiguous such as "nb_out_samples".
Also I wonder if it would make to be able to specify either the number
of samples *or* the frame size, note that this could be done by using
options.
> + AVFifoBuffer *fifo; // samples are queued here
> + int64_t next_out_pts;
nit++: ///< syntax is more usual for fields doxy
> +} ASFSContext;
> +
> +static av_cold int init(AVFilterContext *ctx, const char *args, void *opaque)
> +{
> + ASFSContext *asfs = ctx->priv;
> + if (!args) {
> + av_log(ctx, AV_LOG_ERROR, "parameter is required\n");
> + return AVERROR(EINVAL);
> + }
Is there any reason for which you can't set a default value?
> + asfs->framesize = atoi(args);
sscanf is more robust than atoi.
> + if (!asfs->framesize) {
> + av_log(ctx, AV_LOG_ERROR, "invalind framesize %d (from '%s')\n", asfs->framesize, args);
"Invalid specified framesize '%s'\n", args
also missing check for negative values
> + return AVERROR(EINVAL);
> + }
> +
> + asfs->fifo = av_fifo_alloc(2 * AVCODEC_MAX_AUDIO_FRAME_SIZE);
AVCODEC_MAX_AUDIO_FRAME_SIZE is defined in libavcodec/avcodec.h so it
is not necessarily available in avfilter.h, I wonder how this can even
compile.
Also I'd avoid this check at all and leave that to the application
level or to the lavc/lavfi glue.
> + if (!asfs->fifo)
> + return AVERROR(ENOMEM);
> +
> + asfs->next_out_pts = AV_NOPTS_VALUE;
> + return 0;
> +}
> +
> +static av_cold void uninit(AVFilterContext *ctx)
> +{
> + ASFSContext *asfs = ctx->priv;
> + av_fifo_free(asfs->fifo);
> +}
> +
> +static int query_formats(AVFilterContext *ctx)
> +{
> + /*
> + * State that we support:
> + * - every sample format
> + * - every channels layout
> + * - only PACKED packing format
> + */
> + AVFilterFormats *formats = NULL;
> + int packing_fmts[] = { AVFILTER_PACKED, -1 };
> +
> + formats = avfilter_make_all_channel_layouts();
> + if (!formats)
> + return AVERROR(ENOMEM);
> + avfilter_set_common_channel_layouts(ctx, formats);
> +
> + formats = avfilter_make_all_formats(AVMEDIA_TYPE_AUDIO);
> + if (!formats)
> + return AVERROR(ENOMEM);
> + avfilter_set_common_sample_formats(ctx, formats);
> +
> + formats = avfilter_make_format_list(packing_fmts);
> + if (!formats)
> + return AVERROR(ENOMEM);
> + avfilter_set_common_packing_formats(ctx, formats);
Note: maybe we should make a macro or have some convenience function
for this template code.
> + return 0;
> +}
> +
> +static void filter_samples(AVFilterLink *inlink, AVFilterBufferRef *insamples)
> +{
> + int r;
Nit++: ret is more customary
> + ASFSContext *asfs = inlink->dst->priv;
> + int nb_samples = insamples->audio->nb_samples;
> + int nb_channels =
> + av_get_channel_layout_nb_channels(insamples->audio->channel_layout);
> + int bytes_per_sample_1ch = av_get_bytes_per_sample(insamples->format);
> + int nb_bytes_in = nb_channels * bytes_per_sample_1ch * nb_samples;
> +
> + if (av_fifo_space(asfs->fifo) < nb_bytes_in) {
> + av_log(inlink->dst, AV_LOG_DEBUG, "no space for %d bytes, stretching fifo\n", nb_bytes_in);
Nit: here and below, use a Capitalized first word in log messages.
> + r = av_fifo_realloc2(asfs->fifo, av_fifo_size(asfs->fifo) + nb_bytes_in);
Possible int overflow.
> + if (r < 0) {
> + av_log(inlink->dst, AV_LOG_ERROR,
> + "stretching fifo fail, discarded %d samples\n", nb_samples);
> + return;
> + }
> + }
> + av_fifo_generic_write(asfs->fifo, insamples->data[0], nb_bytes_in, NULL);
> +
> + if (asfs->next_out_pts == AV_NOPTS_VALUE)
> + asfs->next_out_pts = insamples->pts;
> + avfilter_unref_buffer(insamples);
> +}
> +
> +static int poll_frame(AVFilterLink *outlink)
> +{
> + ASFSContext *asfs = outlink->src->priv;
> + int r;
> + int nb_channels =
> + av_get_channel_layout_nb_channels(outlink->channel_layout);
> + int bytes_per_sample_1ch = av_get_bytes_per_sample(outlink->format);
> + int bytes_per_sample = nb_channels * bytes_per_sample_1ch;
> + int bytes_per_outframe = asfs->framesize * bytes_per_sample;
> + if (av_fifo_size(asfs->fifo) / bytes_per_outframe >= 1)
> + return 1;
is there a reason for which you don't check on
av_fifo_size(asfs->fifo) >= bytes_per_outframe?
> + if (!avfilter_poll_frame(outlink->src->inputs[0]))
> + return 0;
> + r = avfilter_request_frame(outlink->src->inputs[0]);
> + if (r < 0) {
> + av_log(outlink->src, AV_LOG_ERROR, "requesting frame from source fail, error %d\n", r);
Nit: no need to tell the error code, I'd say:
"Error occurred when requesting frame from source\n"
> + return r;
> + }
> + if (av_fifo_size(asfs->fifo) / bytes_per_outframe >= 1)
> + return 1;
> + else
> + return 0;
> +}
> +
> +static int request_frame(AVFilterLink *outlink)
> +{
> + ASFSContext *asfs = outlink->src->priv;
> + int nb_channels =
> + av_get_channel_layout_nb_channels(outlink->channel_layout);
> + int bytes_per_sample_1ch = av_get_bytes_per_sample(outlink->format);
> + int bytes_per_sample = nb_channels * bytes_per_sample_1ch;
> + int bytes_per_outframe = asfs->framesize * bytes_per_sample;
> + AVFilterBufferRef *outsamples;
> +
> + if (av_fifo_size(asfs->fifo) < bytes_per_outframe) {
> + av_log(outlink->src, AV_LOG_ERROR, "requested frame, but having none\n");
> + return AVERROR(EINVAL);
> + }
> +
> + outsamples = avfilter_get_audio_buffer(outlink, AV_PERM_WRITE, asfs->framesize);
> + assert(outsamples);
Is there a specific reason for which you preferred an assert over
error message and return err?
> + av_fifo_generic_read(asfs->fifo, outsamples->data[0], bytes_per_outframe, NULL);
> +
> + outsamples->audio->channel_layout = outlink->channel_layout;
> + outsamples->audio->nb_samples = asfs->framesize;
> + outsamples->audio->sample_rate = outlink->sample_rate;
> + outsamples->pts = asfs->next_out_pts;
> + if (asfs->next_out_pts != AV_NOPTS_VALUE)
> + asfs->next_out_pts += asfs->framesize;
> +
> + avfilter_filter_samples(outlink, outsamples);
> + return 0;
> +}
> +
> +AVFilter avfilter_af_asetframesize = {
> + .name = "asetframesize",
> + .description = NULL_IF_CONFIG_SMALL(
> + "Set number of samples to be output from filtergraph at once"),
possibly simpler/more clear:
"Set the number of samples for the output audio frames."
> + .query_formats = query_formats,
> + .priv_size = sizeof(ASFSContext),
> + .init = init,
> + .uninit = uninit,
> +
> + .inputs = (const AVFilterPad[]) {{ .name = "default",
> + .type = AVMEDIA_TYPE_AUDIO,
> + .filter_samples = filter_samples,
> + .min_perms = AV_PERM_READ|AV_PERM_WRITE},
> + { .name = NULL}},
nit: weird indent due to copy&paste
> +
> + .outputs = (const AVFilterPad[]) {{ .name = "default",
> + .type = AVMEDIA_TYPE_AUDIO,
> + .request_frame = request_frame,
> + .poll_frame = poll_frame, },
> + { .name = NULL}},
> +};
> diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
> index 487738a..339ef8d 100644
> --- a/libavfilter/allfilters.c
> +++ b/libavfilter/allfilters.c
> @@ -46,6 +46,7 @@ void avfilter_register_all(void)
> REGISTER_FILTER (PAN, pan, af);
> REGISTER_FILTER (SILENCEDETECT, silencedetect, af);
> REGISTER_FILTER (VOLUME, volume, af);
> + REGISTER_FILTER (ASETFRAMESIZE, asetframesize, af);
Nit: alphabetical order.
>
> REGISTER_FILTER (ABUFFER, abuffer, asrc);
> REGISTER_FILTER (AEVALSRC, aevalsrc, asrc);
--
FFmpeg = Faithless Fancy Minimal Pacific Evangelical Genius
More information about the ffmpeg-devel
mailing list