[FFmpeg-devel] [PATCH] Add mixer registration and mixer header file.

Stefano Sabatini stefano.sabatini-lala
Sat Aug 14 19:28:02 CEST 2010


On date Saturday 2010-08-14 13:29:31 +0200, Sebastian Vater encoded:
> Sebastian Vater a ?crit :
> > Mixers do output a 32 bit PCM stream of n input channels
> > varying in frequency, volume, panning and loop points.
> >
> >   
> 
> Sorry, I forgot adding mixer.h to the Makefile, fixed!
> 
> -- 
> 
> Best regards,
>                    :-) Basty/CDGS (-:
> 

> diff --git a/configure b/configure
> index ed1fae6..59f69ba 100755
> --- a/configure
> +++ b/configure
> @@ -125,6 +125,9 @@ Configuration options:
>    --disable-hwaccel=NAME   disable hwaccel NAME
>    --enable-hwaccel=NAME    enable hwaccel NAME
>    --disable-hwaccels       disable all hwaccels
> +  --enable-mixer=NAME      enable mixer NAME
> +  --disable-mixer=NAME     disable mixer NAME
> +  --disable-mixers         disable all mixers
>    --disable-muxer=NAME     disable muxer NAME
>    --enable-muxer=NAME      enable muxer NAME
>    --disable-muxers         disable all muxers
> @@ -159,6 +162,7 @@ Configuration options:
>    --list-indevs            show all available input devices
>    --list-outdevs           show all available output devices
>    --list-filters           show all available filters
> +  --list-mixers            show all available mixers
>  
>  External library support:
>    --enable-avisynth        enable reading of AVISynth script files [no]
> @@ -838,6 +842,7 @@ COMPONENT_LIST="
>      filters
>      hwaccels
>      indevs
> +    mixers
>      muxers
>      outdevs
>      parsers
> @@ -1624,6 +1629,7 @@ OUTDEV_LIST=$(find_things   outdev   OUTDEV   libavdevice/alldevices.c)
>  INDEV_LIST=$(find_things    indev    _IN      libavdevice/alldevices.c)
>  PROTOCOL_LIST=$(find_things protocol PROTOCOL libavformat/allformats.c)
>  FILTER_LIST=$(find_things   filter   FILTER   libavfilter/allfilters.c)
> +MIXER_LIST=$(find_things    mixer    MIXER    libavsequencer/allmixers.c)
>  
>  find_tests(){
>      map "echo ${2}\${v}_test" $(ls "$source_path"/tests/ref/$1 | grep -v '[^-a-z0-9_]')
> @@ -2896,6 +2902,7 @@ check_deps $CONFIG_LIST       \
>             $DEMUXER_LIST      \
>             $MUXER_LIST        \
>             $FILTER_LIST       \
> +           $MIXER_LIST        \
>             $INDEV_LIST        \
>             $OUTDEV_LIST       \
>             $PROTOCOL_LIST     \
> @@ -2987,7 +2994,7 @@ echo "zlib enabled              ${zlib-no}"
>  echo "bzlib enabled             ${bzlib-no}"
>  echo
>  
> -for type in decoder encoder hwaccel parser demuxer muxer protocol filter bsf indev outdev; do
> +for type in decoder encoder hwaccel parser demuxer mixer muxer protocol filter bsf indev outdev; do
>      echo "Enabled ${type}s:"
>      eval list=\$$(toupper $type)_LIST
>      print_enabled '_*' $list | sort | pr -3 -t
> @@ -3178,6 +3185,7 @@ print_config CONFIG_ $TMPH config.mak $CONFIG_LIST       \
>                                        $DEMUXER_LIST      \
>                                        $MUXER_LIST        \
>                                        $FILTER_LIST       \
> +                                      $MIXER_LIST        \
>                                        $PROTOCOL_LIST     \
>                                        $INDEV_LIST        \
>                                        $OUTDEV_LIST       \
> diff --git a/libavsequencer/Makefile b/libavsequencer/Makefile
> index e8088bd..3608dd7 100644
> --- a/libavsequencer/Makefile
> +++ b/libavsequencer/Makefile
> @@ -3,7 +3,9 @@ include $(SUBDIR)../config.mak
>  NAME = avsequencer
>  
>  HEADERS = avsequencer.h \
> +          mixer.h       \
>  
> -OBJS = avsequencer.o    \
> +OBJS = allmixers.o      \
> +       avsequencer.o    \
>  
>  include $(SUBDIR)../subdir.mak
> diff --git a/libavsequencer/allmixers.c b/libavsequencer/allmixers.c
> new file mode 100644
> index 0000000..7579ee7
> --- /dev/null
> +++ b/libavsequencer/allmixers.c
> @@ -0,0 +1,40 @@
> +/*
> + * Provide registration of all mixers for the sequencer
> + * Copyright (c) 2010 Sebastian Vater <cdgs.basty at googlemail.com>
> + *
> + * 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
> + * Provide registration of all mixers for the sequencer.
> + */
> +
> +#include "libavsequencer/avsequencer.h"
> +
> +#define REGISTER_MIXER(X,x) { \
> +          extern AVSequencerMixerContext x##_mixer; \
> +          if(CONFIG_##X##_MIXER)  avseq_mixer_register(&x##_mixer); }
> +
> +void avsequencer_register_all(void)
> +{
> +    static int initialized;
> +
> +    if (initialized)
> +        return;
> +    initialized = 1;
> +}
> diff --git a/libavsequencer/avsequencer.c b/libavsequencer/avsequencer.c
> index d43284f..efb8316 100644
> --- a/libavsequencer/avsequencer.c
> +++ b/libavsequencer/avsequencer.c
> @@ -41,3 +41,36 @@ const char *avsequencer_license(void) {
>  #define LICENSE_PREFIX "libavsequencer license: "
>      return LICENSE_PREFIX FFMPEG_LICENSE + sizeof(LICENSE_PREFIX) - 1;
>  }
> +
> +#define AVSEQUENCER_MAX_REGISTERED_MIXERS_NB 64
> +
> +static AVSequencerMixerContext *registered_mixers[AVSEQUENCER_MAX_REGISTERED_MIXERS_NB + 1];
> +static int next_registered_mixer_idx = 0;
> +
> +AVSequencerMixerContext *avseq_mixer_get_by_name(const char *name) {
> +    int i;

Nits: 
AVSequencerMixerContext *avseq_mixer_get_by_name(const char *name)
{

For what regards the namespace...

I'm not sure what should we use between:
av[seq[uencer]]_mixer_get_by_name()
or
av[seq[uencer]]_get_mixer_by_name()

I tend to prefer:
av_get_mixer_by_name()

which is simpler and sounds less awkward to speech.
Also this should prevent renames in case we move the mixer to another
lib (as it looks possible for the mixer code).

Also I'm not so happy about the avseq_/avsequencer_ inconsistency, I
need to ponder more about it and hear other opinions.

> +
> +    for (i = 0; i < next_registered_mixer_idx; i++) {
> +        if (!strcmp(registered_mixers[i]->name, name))
> +            return registered_mixers[i];
> +    }
> +
> +    return NULL;
> +}
> +
> +int avseq_mixer_register(AVSequencerMixerContext *mixctx) {

same considerations as before for what regards the name.

Now here there is a strange thing, but that may depend on my
misunderstanding of the API.

I'd rather expect here to register an AVSequencerMixer const struct, rather
than a context.

The context usually represents a configured instance of an element.

> +    if (next_registered_mixer_idx == AVSEQUENCER_MAX_REGISTERED_MIXERS_NB)
> +        return -1;
> +
> +    registered_mixers[next_registered_mixer_idx++] = mixctx;
> +    return 0;
> +}
> +
> +AVSequencerMixerContext **avseq_mixer_next(AVSequencerMixerContext **mixctx) {
> +    return mixctx ? ++mixctx : &registered_mixers[0];
> +}
> +
> +void avsequencer_uninit(void) {
> +    memset(registered_mixers, 0, sizeof(registered_mixers));
> +    next_registered_mixer_idx = 0;
> +}
> diff --git a/libavsequencer/avsequencer.h b/libavsequencer/avsequencer.h
> index 92c8d83..60262b0 100644
> --- a/libavsequencer/avsequencer.h
> +++ b/libavsequencer/avsequencer.h
> @@ -51,4 +51,55 @@ const char *avsequencer_configuration(void);
>   */
>  const char *avsequencer_license(void);
>  
> +#include "libavsequencer/mixer.h"
> +

> +/** Registers all mixers to the AVSequencer.
> + *
> + * @note This is part of the new sequencer API which is still under construction.
> + *       Thus do not use this yet. It may change at any time, do not expect
> + *       ABI compatibility yet!
> + */
> +void avsequencer_register_all(void);

Nits:
/**
 * Register all mixers to the AVSequencer.
 */

That's consistent with the FFmpeg doxy style.
As for the @note, I don't believe it is really necessary, and I don't
want to clutter the headers with such comments, as that consideration
applies to the whole new API, so it can be safely be skipped. Or you
can put such a note just in the @file doxy.

> +
> +/** Registers a mixer to the AVSequencer.
> + *
> + * @param mixctx the AVSequencerMixerContext to register
> + * @return >= 0 on success, a negative error code otherwise
> + *
> + * @note This is part of the new sequencer API which is still under construction.
> + *       Thus do not use this yet. It may change at any time, do not expect
> + *       ABI compatibility yet!
> + */
> +int avseq_mixer_register(AVSequencerMixerContext *mixctx);
> +

> +/** Gets a mixer by it's title metadata (name).

Nit: its title

> + *
> + * @param name the title of the mixer to get
> + * @return pointer to mixer context on success, NULL otherwise

This is a little strange, usually each element has a name, what is
this metadata thing related to?

> + *
> + * @note This is part of the new sequencer API which is still under construction.
> + *       Thus do not use this yet. It may change at any time, do not expect
> + *       ABI compatibility yet!
> + */
> +AVSequencerMixerContext *avseq_mixer_get_by_name(const char *name);
> +
> +/** Gets the pointer to the next mixer context array.
> + *
> + * @param mixctx the AVSequencerMixerContext array of the next mixer to get
> + * @return pointer to next mixer context array on success, NULL otherwise
> + *
> + * @note This is part of the new sequencer API which is still under construction.
> + *       Thus do not use this yet. It may change at any time, do not expect
> + *       ABI compatibility yet!
> + */
> +AVSequencerMixerContext **avseq_mixer_next(AVSequencerMixerContext **mixctx);
> +
> +/** Uninitializes all the mixers registered to the AVSequencer.

I'd prefer something more generic, as this function is not necessarily
supposed to only do that:

/** Uninitialize the avsequencer system. In particular unregister all mixers. */

> + *
> + * @note This is part of the new sequencer API which is still under construction.
> + *       Thus do not use this yet. It may change at any time, do not expect
> + *       ABI compatibility yet!
> + */
> +void avsequencer_uninit(void);
> +
>  #endif /* AVSEQUENCER_AVSEQUENCER_H */
> diff --git a/libavsequencer/mixer.h b/libavsequencer/mixer.h
> new file mode 100644
> index 0000000..7e325b0
> --- /dev/null
> +++ b/libavsequencer/mixer.h
> @@ -0,0 +1,285 @@
> +/*
> + * AVSequencer mixer header file for various mixing engines
> + * Copyright (c) 2010 Sebastian Vater <cdgs.basty at googlemail.com>
> + *
> + * 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
> + */
> +
> +#ifndef AVSEQUENCER_MIXER_H
> +#define AVSEQUENCER_MIXER_H
> +
> +#include "libavutil/avutil.h"
> +
> +/** AVSequencerMixerChannel->flags bitfield.  */
> +enum AVSequencerMixerChannelFlags {
> +    AVSEQ_MIXER_CHANNEL_FLAG_MUTED      = 0x01, ///< Channel is muted (i.e. processed but not outputted)
> +    AVSEQ_MIXER_CHANNEL_FLAG_SYNTH      = 0x02, ///< Start new synthetic waveform when old has finished playback
> +    AVSEQ_MIXER_CHANNEL_FLAG_LOOP       = 0x04, ///< Loop points are being used
> +    AVSEQ_MIXER_CHANNEL_FLAG_PINGPONG   = 0x08, ///< Channel loop is in ping-pong style
> +    AVSEQ_MIXER_CHANNEL_FLAG_BACKWARDS  = 0x10, ///< Channel is currently playing in backward direction
> +    AVSEQ_MIXER_CHANNEL_FLAG_BACK_LOOP  = 0x20, ///< Backward loop instead of forward
> +    AVSEQ_MIXER_CHANNEL_FLAG_SURROUND   = 0x40, ///< Use surround sound output for this channel
> +    AVSEQ_MIXER_CHANNEL_FLAG_PLAY       = 0x80, ///< This channel is currently playing a sample (i.e. enabled)
> +};
> +
> +/**
> + * Mixer channel structure which is used by the mixer to determine how
> + * to mix the samples of each channel into the target output buffer.
> + * This structure is actually for one mixing channel and therefore
> + * actually pointed as an array with size of number of total amount of
> + * allocated channels.

> + * New fields can be added to the end with minor version bumps.
> + * Removal, reordering and changes to existing fields require a major
> + * version bump.

This comment maybe can be skipped, again this is true for all public
structs so it looks redundant.

> + */
> +typedef struct AVSequencerMixerChannel {
> +    /** Current position in samples of this channel to be mixed to
> +       output data.  */
> +    uint32_t pos;
> +
> +    /** Current length in samples for this channel.  */
> +    uint32_t len;
> +
> +    /** Current sample data for this channel. Actual layout depends
> +       on number of bits per sample.  */
> +    int16_t *data;
> +
> +    /** Current sample rate in Hz for this channel.  */
> +    uint32_t rate;
> +
> +    /** Current repeat start in samples for this channel. If the loop
> +       flag is also set, the sample will be looped between
> +       repeat_start and repeat_start + repeat_length.  */
> +    uint32_t repeat_start;
> +
> +    /** Current repeat length in samples for this channel. If the loop
> +       flag is also set, the sample will be looped between repeat_start
> +       and repeat_start + repeat_length.  */
> +    uint32_t repeat_length;
> +
> +    /** Current repeat count in loop end point touches for this channel.
> +       If the loop flag is also set, the sample will be looped exactly
> +       the number of times this value between repeat_start and
> +       repeat_start + repeat_length, unless the repeat count is zero
> +       which means an unlimited repeat count.  */
> +    uint32_t repeat_count;
> +
> +    /** Current number of loop end point touches for this channel.
> +       If the loop flag is also set, the sample will stop looping when
> +       this number reaches repeat_count between repeat_start and
> +       repeat_start + repeat_length, unless the repeat count is zero
> +       which means an unlimited repeat count.  */
> +    uint32_t repeat_counted;
> +

> +    /** Number of bits per sample between 1 and 32. Mixers usually use
> +       special accelated code for 8, 16, 24 or 32 bits per sample.  */

accelated?

> +    uint8_t bits_per_sample;
> +
> +    /** Special flags which indicate things like a muted channel,
> +       start new synthetic waveform, if to use loop points, type of
> +       loop (normal forward, ping-pong or normal backward), if normal
> +       stereo or surround panning and if this channel is active.  */
> +    uint8_t flags;

Here you may mention AVSEQ_MIXER_CHANNEL_FLAG_*.

> +
> +    /** Current volume for this channel which ranges from 0 (muted)
> +       to 255 (full volume).  */
> +    uint8_t volume;
> +
> +    /** Current stereo panning level for this channel (where 0-127
> +       indicate left stereo channel panning, -128 is central stereo
> +       panning and -127 to -1 indicate right stereo panning).  */
> +    int8_t panning;
> +} AVSequencerMixerChannel;
> +
> +/** AVSequencerMixerData->flags bitfield.  */
> +enum AVSequencerMixerDataFlags {
> +    AVSEQ_MIXER_DATA_FLAG_ALLOCATED = 0x01, ///< The mixer is currently allocated and ready to use
> +    AVSEQ_MIXER_DATA_FLAG_MIXING    = 0x02, ///< The mixer is currently in actual mixing to output
> +    AVSEQ_MIXER_DATA_FLAG_STEREO    = 0x04, ///< The mixer is currently mixing in stereo mode instead of mono
> +    AVSEQ_MIXER_DATA_FLAG_FROZEN    = 0x08, ///< The mixer has been delayed by some external process like disk I/O writing
> +};
> +
> +/**

> + * Mixer data allocation structure which is used to allocate a mixer
> + * for be used by the playback handler. This structure is also used

for being used ...

> + * for setting global parameters like the output mixing rate, the
> + * size of the mixing buffer, volume boost and the tempo which
> + * decides when to call the actual playback handler.

> + * New fields can be added to the end with minor version bumps.
> + * Removal, reordering and changes to existing fields require a major
> + * version bump.

Redundant?

> + */
> +typedef struct AVSequencerMixerData {
> +    /** Pointer to basic mixer context structure which describes
> +       the mixer features.  */
> +    struct AVSequencerMixerContext *mixctx;
> +
> +    /** Pointer to be used by the playback handler of the mixing
> +       engine which is called every tempo tick.  */
> +    void *opaque;
> +
> +    /** Current mixing rate in Hz which is used to output the
> +       calculated sample data from the channels.  */
> +    uint32_t rate;
> +
> +    /** Pointer to the mixing output buffer for the calculated sample
> +       data from the channels. This is always SAMPLE_FMT_S32 in native
> +       endianess.  */
> +    int32_t *mix_buf;
> +
> +    /** The current actual size of the output buffer for the
> +       calculated sample data from the channels.  */
> +    uint32_t mix_buf_size;
> +
> +    /** The current volume boost level. 65536 equals to 100% which
> +       means no boost.  */
> +    uint32_t volume_boost;
> +
> +    /** Left channel volume level. 65536 is full volume.  */
> +    uint32_t volume_left;
> +
> +    /** Right channel volume level. 65536 is full volume.  */
> +    uint32_t volume_right;
> +
> +    /** Speed of playback handler in AV_TIME_BASE fractional
> +       seconds.  */
> +    uint32_t tempo;
> +
> +    /** Current maximum number of allocated channels. The more
> +       channels are used the more CPU power is required to
> +       calculate the output audio buffer.  */
> +    uint16_t channels_max;
> +
> +    /** Current status flags for this mixer which contain information
> +       like if the mixer has been allocated, is currently mixing,
> +       output mode (stereo or mono) or if it frozen because of some
> +       delaying (like caused by disk I/O when using disk writers.  */
> +    uint8_t flags;
> +
> +    /** Executes one tick of the playback handler when enough mixing
> +       data has been processed.  */
> +    int (*handler)( struct AVSequencerMixerData *mixer_data );
> +} AVSequencerMixerData;
> +
> +/** AVSequencerMixerContext->flags bitfield.  */
> +enum AVSequencerMixerContextFlags {
> +    AVSEQ_MIXER_CONTEXT_FLAG_STEREO     = 0x08, ///< This mixer supports stereo mixing in addition to mono
> +    AVSEQ_MIXER_CONTEXT_FLAG_SURROUND   = 0x10, ///< This mixer supports surround panning in addition to stereo panning
> +    AVSEQ_MIXER_CONTEXT_FLAG_AVFILTER   = 0x20, ///< This mixer supports additional audio filters if FFmpeg is compiled with AVFilter enabled
> +};
> +
> +/**
> + * Mixer context structure which is used to describe certain features
> + * of registered mixers to the sequencer context.

> + * New fields can be added to the end with minor version bumps.
> + * Removal, reordering and changes to existing fields require a major
> + * version bump.

Redundant?

> + */
> +typedef struct AVSequencerMixerContext {

Now I wonder, shouldn't this be called AVSequencerMixer (I mean
without Context)?

> +    /**
> +     * information on struct for av_log
> +     * - set by avseq_alloc_context
> +     */
> +    const AVClass *av_class;
> +
> +    /** Mixer name.  */
> +    const char *name;
> +
> +    /**
> +     * A description for the filter. You should use the
> +     * NULL_IF_CONFIG_SMALL() macro to define it.
> +     */
> +    const char *description;
> +
> +    /** Default mixing rate in Hz used by this mixer. This will
> +       usually set to the value which this mixer can handle the best
> +       way.  */
> +    uint32_t frequency;
> +
> +    /** Minimum mixing rate in Hz supported by this mixer.  */
> +    uint32_t frequency_min;
> +
> +    /** Maximum mixing rate in Hz supported by this mixer.  */
> +    uint32_t frequency_max;
> +
> +    /** Default mixing buffer size preferred. This will usually set
> +       to the value which this mixer can handle the at best without
> +       causing jittering and too much lag.  */
> +    uint32_t buf_size;

> +
> +    /** Minimum mixing buffer size supported by this mixer.  */
> +    uint32_t buf_size_min;
> +
> +    /** Maximum mixing buffer size supported by this mixer.  */
> +    uint32_t buf_size_max;

I tend to prefer: max_buf_size/min_buf_size, more English-like and
apparently more consistent with the rest of the FFmpeg API.

> +
> +    /** Default volume boost level. 65536 equals to 100% which
> +       means no boost.  */
> +    uint32_t volume_boost;
> +

> +    /** Maximum number of channels supported by this mixer, some
> +       engines might support less channels than maximum allowed by
> +       the sequencer.  */
> +    uint16_t channels_max;

same: max_nb_channels

> +
> +    /** Special flags indicating supported features by this mixer.  */
> +    uint8_t flags;
> +
> +    /** The initialization function to call for the mixer.  */

> +    AVSequencerMixerData * (*init)( struct AVSequencerMixerContext *mixctx, const char *args, void *opaque );

Nit: (_foo_) => (foo)
same below

> +
> +    /** The destruction function to call for the mixer.  */
> +    int (*uninit)( AVSequencerMixerData *mixer_data );
> +

> +    /** Transfers the new mixing rate in Hz from the AVSequencer to
> +       the internal mixer data.  */

Nit: Transfer ... that is use impersonal form, same below.

> +    uint32_t (*set_rate)( AVSequencerMixerData *mixer_data, uint32_t new_mix_rate );

Please avoid contractions: new_mix_rate => new_mixer_rate
they tend to be confusing and generate inconsistencies.

Regards.
-- 
FFmpeg = Fundamental and Friendly Mythic Programmable Elastic Game



More information about the ffmpeg-devel mailing list