On Mon, Apr 09, 2012 at 05:27:52PM -0400, Justin Ruggles wrote:
> --- a/configure
> +++ b/configure
> @@ -3490,3 +3494,4 @@ pkgconfig_generate libavformat "Libav container format
> library" "$LIBAVFORMAT_VE
> pkgconfig_generate libavdevice "Libav device handling library"
> "$LIBAVDEVICE_VERSION" "$extralibs" "libavformat = $LIBAVFORMAT_VERSION"
> pkgconfig_generate libavfilter "Libav video filtering library"
> "$LIBAVFILTER_VERSION" "$extralibs"
> pkgconfig_generate libswscale "Libav image rescaling library"
> "$LIBSWSCALE_VERSION" "$LIBM" "libavutil = $LIBAVUTIL_VERSION"
> +pkgconfig_generate libavresample "Libav audio resampling library"
> "$LIBAVRESAMPLE_VERSION" "$extralibs"
alphabetical order
> --- /dev/null
> +++ b/libavresample/Makefile
> @@ -0,0 +1,11 @@
> +OBJS = audio_convert.o audio_data.o audio_mix.o audio_mix_matrix.o options.o
> \
> + resample.o utils.o
Place one object per line please, we now do this everywhere.
> +DIRS = x86
Drop DIRS, I recently made this unnecessary.
> --- /dev/null
> +++ b/libavresample/audio_convert.c
> @@ -0,0 +1,344 @@
> + case CONV_FUNC_TYPE_DEINTERLEAVE:
> + if (ac->in_fmt == in_fmt && ac->out_fmt == out_fmt &&
> + (!channels || ac->channels == channels)) {
> + ac->conv_deinterleave = conv;
> + ac->func_descr = descr;
> + ac->ptr_align = ptr_align;
> + ac->samples_align = samples_align;
> + if (ptr_align == 1 && samples_align == 1) {
> + ac->conv_deinterleave_generic = conv;
> + ac->func_descr_generic = descr;
align
> + if (found) {
> + av_log(ac->avr, AV_LOG_DEBUG, "audio_convert: found function: %-4s
> to %-4s (%s)\n",
> + av_get_sample_fmt_name(ac->in_fmt),
> av_get_sample_fmt_name(ac->out_fmt),
> + descr);
long lines
> +CONV_FUNC_GROUP(AV_SAMPLE_FMT_U8 , uint8_t, AV_SAMPLE_FMT_U8 , uint8_t,
> *(const uint8_t*)pi)
> +CONV_FUNC_GROUP(AV_SAMPLE_FMT_S16, int16_t, AV_SAMPLE_FMT_U8 , uint8_t,
> (*(const uint8_t*)pi - 0x80)<<8)
Add a space before * for better alignment.
> +CONV_FUNC_GROUP(AV_SAMPLE_FMT_S32, int32_t, AV_SAMPLE_FMT_U8 , uint8_t,
> (*(const uint8_t*)pi - 0x80)<<24)
> +CONV_FUNC_GROUP(AV_SAMPLE_FMT_FLT, float , AV_SAMPLE_FMT_U8 , uint8_t,
> (*(const uint8_t*)pi - 0x80)*(1.0 / (1<<7)))
> +CONV_FUNC_GROUP(AV_SAMPLE_FMT_DBL, double , AV_SAMPLE_FMT_U8 , uint8_t,
> (*(const uint8_t*)pi - 0x80)*(1.0 / (1<<7)))
> +CONV_FUNC_GROUP(AV_SAMPLE_FMT_U8 , uint8_t, AV_SAMPLE_FMT_S16, int16_t,
> (*(const int16_t*)pi>>8) + 0x80)
> +CONV_FUNC_GROUP(AV_SAMPLE_FMT_S16, int16_t, AV_SAMPLE_FMT_S16, int16_t,
> *(const int16_t*)pi)
> +CONV_FUNC_GROUP(AV_SAMPLE_FMT_S32, int32_t, AV_SAMPLE_FMT_S16, int16_t,
> *(const int16_t*)pi<<16)
> +CONV_FUNC_GROUP(AV_SAMPLE_FMT_FLT, float , AV_SAMPLE_FMT_S16, int16_t,
> *(const int16_t*)pi*(1.0 / (1<<15)))
> +CONV_FUNC_GROUP(AV_SAMPLE_FMT_DBL, double , AV_SAMPLE_FMT_S16, int16_t,
> *(const int16_t*)pi*(1.0 / (1<<15)))
> +CONV_FUNC_GROUP(AV_SAMPLE_FMT_U8 , uint8_t, AV_SAMPLE_FMT_S32, int32_t,
> (*(const int32_t*)pi>>24) + 0x80)
> +CONV_FUNC_GROUP(AV_SAMPLE_FMT_S16, int16_t, AV_SAMPLE_FMT_S32, int32_t,
> *(const int32_t*)pi>>16)
> +CONV_FUNC_GROUP(AV_SAMPLE_FMT_S32, int32_t, AV_SAMPLE_FMT_S32, int32_t,
> *(const int32_t*)pi)
> +CONV_FUNC_GROUP(AV_SAMPLE_FMT_FLT, float , AV_SAMPLE_FMT_S32, int32_t,
> *(const int32_t*)pi*(1.0 / (1U<<31)))
> +CONV_FUNC_GROUP(AV_SAMPLE_FMT_DBL, double , AV_SAMPLE_FMT_S32, int32_t,
> *(const int32_t*)pi*(1.0 / (1U<<31)))
> +CONV_FUNC_GROUP(AV_SAMPLE_FMT_U8 , uint8_t, AV_SAMPLE_FMT_FLT, float ,
> av_clip_uint8( lrintf(*(const float*)pi * (1<<7)) + 0x80))
> +CONV_FUNC_GROUP(AV_SAMPLE_FMT_S16, int16_t, AV_SAMPLE_FMT_FLT, float ,
> av_clip_int16( lrintf(*(const float*)pi * (1<<15))))
> +CONV_FUNC_GROUP(AV_SAMPLE_FMT_S32, int32_t, AV_SAMPLE_FMT_FLT, float ,
> av_clipl_int32(llrintf(*(const float*)pi * (1U<<31))))
> +CONV_FUNC_GROUP(AV_SAMPLE_FMT_FLT, float , AV_SAMPLE_FMT_FLT, float ,
> *(const float*)pi)
> +CONV_FUNC_GROUP(AV_SAMPLE_FMT_DBL, double , AV_SAMPLE_FMT_FLT, float ,
> *(const float*)pi)
> +CONV_FUNC_GROUP(AV_SAMPLE_FMT_U8 , uint8_t, AV_SAMPLE_FMT_DBL, double ,
> av_clip_uint8( lrint(*(const double*)pi * (1<<7)) + 0x80))
> +CONV_FUNC_GROUP(AV_SAMPLE_FMT_S16, int16_t, AV_SAMPLE_FMT_DBL, double ,
> av_clip_int16( lrint(*(const double*)pi * (1<<15))))
> +CONV_FUNC_GROUP(AV_SAMPLE_FMT_S32, int32_t, AV_SAMPLE_FMT_DBL, double ,
> av_clipl_int32(llrint(*(const double*)pi * (1U<<31))))
> +CONV_FUNC_GROUP(AV_SAMPLE_FMT_FLT, float , AV_SAMPLE_FMT_DBL, double ,
> *(const double*)pi)
> +CONV_FUNC_GROUP(AV_SAMPLE_FMT_DBL, double , AV_SAMPLE_FMT_DBL, double ,
> *(const double*)pi)
spaces around * and <<
Please drop the spaces before commas.
> +AudioConvert *ff_audio_convert_alloc(AVAudioResampleContext *avr,
> + enum AVSampleFormat out_fmt,
> + enum AVSampleFormat in_fmt,
> + int channels)
> +{
> +
> + if (HAVE_MMX)
> + ff_audio_convert_init_x86(ac);
I wonder why the x86 init is not called from a global init function.
There is no dependency on MMX there and these are implementation details.
Please check for ARCH_X86 instead.
> +int ff_audio_convert(AudioConvert *ac, AudioData *out, AudioData *in, int
> len)
> +{
> + if (!(ptr_align % ac->ptr_align ) &&
> + (samples_align >= aligned_len)) {
weird spacing
> +++ b/libavresample/audio_data.c
> @@ -0,0 +1,342 @@
> +
> +/*
> + * Calculate alignment for data pointers and number of samples
.
> +int ff_audio_data_realloc(AudioData *a, int nb_samples)
> +{
> +
> + /* if there is already data in the buffer and the sample format is
> planar,
> + allocate a new buffer and copy the data, otherwise just realloc the
> + internal buffer and set new data pointers */
nit: Add * to the beginning of lines.
> +int ff_audio_data_combine(AudioData *dst, int dst_offset, AudioData *src,
> + int src_offset, int nb_samples)
> +{
> + /* validate offsets are within the buffer bounds */
> + if (dst_offset < 0 || dst_offset > dst->nb_samples ||
> + src_offset < 0 || src_offset > src->nb_samples) {
> + av_log(NULL, AV_LOG_ERROR, "offset out-of-bounds: src=%d dst=%d\n",
> src_offset, dst_offset);
long line
> + dst_offset2 = dst_offset + nb_samples;
> + dst_move_size = dst->nb_samples - dst_offset;
nit: align
> --- /dev/null
> +++ b/libavresample/audio_data.h
> @@ -0,0 +1,167 @@
> +
> +/**
> + * Audio buffer used for intermediate storage between conversion phases.
> + */
> +typedef struct AudioData {
> + uint8_t *data[AVRESAMPLE_MAX_CHANNELS]; /**< data plane pointers
> */
> + uint8_t *buffer; /**< data buffer
> */
> + unsigned int buffer_size; /**< allocated buffer size
> */
> + int allocated_samples; /**< number of samples the buffer
> can hold */
> + int nb_samples; /**< current number of samples
> */
> + enum AVSampleFormat sample_fmt; /**< sample format
> */
> + int channels; /**< channel count
> */
> + int allocated_channels; /**< allocated channel count
> */
> + int is_planar; /**< sample format is planar
> */
> + int planes; /**< number of data planes
> */
> + int sample_size; /**< bytes per sample
> */
> + int stride; /**< sample byte offset within a
> plane */
> + int plane_size; /**< size of each plane, in bytes
> */
> + int read_only; /**< data is read-only
> */
> + int allow_realloc; /**< realloc is allowed
> */
> + int ptr_align; /**< minimum data pointer alignment
> */
> + int samples_align; /**< allocated samples alignment
> */
> + const char *name; /**< name for debug logging
> */
> +} AudioData;
nit: I'd skip aligning the closing comments.
> +/**
> + * Initialize AudioData using a given source.
> + *
> + * This does not allocate an internal buffer. It only sets the data pointers
> + * and audio parameters.
> + *
> + * @param a AudioData struct
> + * @param src source data pointers
> + * @param channels channel count
> + * @param nb_samples number of samples in the source data
> + * @param sample_fmt sample format
> + * @param read_only indicates if buffer is read only or read/write
> + * @param name name for debug logging (can be NULL)
> + * @return 0 on success, negative AVERROR value on error
> + */
> +int ff_audio_data_init(AudioData *a, void **src, int plane_size, int
> channels,
> + int nb_samples, enum AVSampleFormat sample_fmt,
> + int read_only, const char *name);
plane_size is not documented.
> +/**
> + * Reallocate AudioData.
> + *
> + * The AudioData must have been previously allocated with
> ff_audio_data_alloc()
.
> +/**
> + * Free AudioData.
> + *
> + * The AudioData must have been previously allocated with
> ff_audio_data_alloc()
.
> +/**
> + * Append data from one AudioData to the end of another.
> + *
> + * @param dst destination AudioData
> + * @param prepend add to the beginning of dst instead of the end
> + * @param src source AudioData
> + * @param src_offset offset, in samples, to start copying, relative to the
> + start of the src
> + * @param nb_samples number of samples to copy
> + * @return 0 on success, negative AVERROR value on error
> + */
> +int ff_audio_data_combine(AudioData *dst, int dst_offset, AudioData *src,
> + int src_offset, int nb_samples);
Something went wrong with prepend/dst_offset here.
> --- /dev/null
> +++ b/libavresample/audio_mix.c
> @@ -0,0 +1,354 @@
> +
> +void ff_audio_mix_set_func(AudioMix *am, enum AVSampleFormat fmt,
> + enum AVMixCoeffType coeff_type, int in_channels,
> + int out_channels, int ptr_align, int
> samples_align,
> + const char *descr, void *mix_func)
> +{
> + av_log(am->avr, AV_LOG_DEBUG, "audio_mix: found function: [fmt=%s]
> [c=%s] %s(%s)\n",
> + av_get_sample_fmt_name(fmt), coeff_type_names[coeff_type],
> + (in_channels || out_channels) ? chan_str : "", descr);
long line, av_dlog?
> +#define MIX_FUNC_GENERIC(fmt, cfmt, stype, ctype, sumtype, expr) \
> +static void MIX_FUNC_NAME(fmt, cfmt)(stype **samples, ctype **matrix, \
> + int len, int out_ch, int in_ch) \
> +{ \
> + int i, in, out; \
> + for (i = 0; i < len; i++) { \
> + for (out = 0; out < out_ch; out++) { \
> + sumtype sum = 0; \
> + for (in = 0; in < in_ch; in++) \
> + sum += samples[in][i] * matrix[out][in]; \
> + samples[out][i] = expr; \
> + } \
> + } \
> +}
nit: please align the \ on column 72
> +static int mix_function_init(AudioMix *am)
> +{
> +
> + if (HAVE_MMX)
> + ff_audio_mix_init_x86(am);
ARCH_X86
> + if (!am->mix) {
> + av_log(am->avr, AV_LOG_ERROR, "audio_mix: NO FUNCTION FOUND:
> [fmt=%s] [c=%s] [%d to %d]\n",
> + av_get_sample_fmt_name(am->fmt),
> coeff_type_names[am->coeff_type],
> + am->in_channels, am->out_channels);
long line
> +void ff_audio_mix_close(AudioMix *am)
> +{
> + if(!am)
> + return;
if (
> --- /dev/null
> +++ b/libavresample/audio_mix_matrix.c
> @@ -0,0 +1,353 @@
> +
> +/* channel positions */
> +#define FRONT_LEFT 0
> +#define FRONT_RIGHT 1
> +#define FRONT_CENTER 2
> +#define LOW_FREQUENCY 3
> +#define BACK_LEFT 4
> +#define BACK_RIGHT 5
> +#define FRONT_LEFT_OF_CENTER 6
> +#define FRONT_RIGHT_OF_CENTER 7
> +#define BACK_CENTER 8
> +#define SIDE_LEFT 9
> +#define SIDE_RIGHT 10
> +#define TOP_CENTER 11
> +#define TOP_FRONT_LEFT 12
> +#define TOP_FRONT_CENTER 13
> +#define TOP_FRONT_RIGHT 14
> +#define TOP_BACK_LEFT 15
> +#define TOP_BACK_CENTER 16
> +#define TOP_BACK_RIGHT 17
> +#define STEREO_LEFT 29
> +#define STEREO_RIGHT 30
> +#define WIDE_LEFT 31
> +#define WIDE_RIGHT 32
> +#define SURROUND_DIRECT_LEFT 33
> +#define SURROUND_DIRECT_RIGHT 34
nit: right-align
> +static int sane_layout(uint64_t layout)
> +{
> + /* check that there is at least 1 front speaker */
> + if (!(layout & AV_CH_LAYOUT_SURROUND))
> + return 0;
> + /* check for left/right symmetry */
> + if (!even(layout & (AV_CH_FRONT_LEFT | AV_CH_FRONT_RIGHT)))
> + return 0;
> + if (!even(layout & (AV_CH_SIDE_LEFT | AV_CH_SIDE_RIGHT)))
> + return 0;
> + if (!even(layout & (AV_CH_BACK_LEFT | AV_CH_BACK_RIGHT)))
> + return 0;
> + if (!even(layout & (AV_CH_FRONT_LEFT_OF_CENTER |
> AV_CH_FRONT_RIGHT_OF_CENTER)))
> + return 0;
> + if (!even(layout & (AV_CH_TOP_FRONT_LEFT | AV_CH_TOP_FRONT_RIGHT)))
> + return 0;
> + if (!even(layout & (AV_CH_TOP_BACK_LEFT | AV_CH_TOP_BACK_RIGHT)))
> + return 0;
> + if (!even(layout & (AV_CH_STEREO_LEFT | AV_CH_STEREO_RIGHT)))
> + return 0;
> + if (!even(layout & (AV_CH_WIDE_LEFT | AV_CH_WIDE_RIGHT)))
> + return 0;
> + if (!even(layout & (AV_CH_SURROUND_DIRECT_LEFT |
> AV_CH_SURROUND_DIRECT_RIGHT)))
> + return 0;
Maybe connect these with || instead of making them different conditions?
> +int avresample_get_matrix(AVAudioResampleContext *avr, double *matrix,
> + int stride)
> +{
> + switch (avr->mix_coeff_type) {
> + case AV_MIX_COEFF_TYPE_Q6:
Indent switch and case at the same depth.
> + switch (avr->mix_coeff_type) {
> + case AV_MIX_COEFF_TYPE_Q6:
ditto
> --- /dev/null
> +++ b/libavresample/avresample-test.c
> @@ -0,0 +1,338 @@
> +
> +#define PUT_FUNC(name, fmt, type, expr) \
> +static void put_sample_ ## name(void **data, enum AVSampleFormat sample_fmt,\
> + int channels, int sample, int ch, \
> + double v_dbl) \
> +{ \
> + type v = expr; \
> + type **out = (type **)data; \
> + if (av_sample_fmt_is_planar(sample_fmt)) \
> + out[ch][sample] = v; \
> + else \
> + out[0][sample * channels + ch] = v; \
> +}
nit: Break the long line and move the \ to column 72.
> +static void audiogen(AVLFG *rnd, void **data, enum AVSampleFormat sample_fmt,
> + int channels, int sample_rate, int nb_samples)
> +{
> + int i, ch, k;
> + double v, f, a, ampa;
> + double tabf1[AVRESAMPLE_MAX_CHANNELS];
> + double tabf2[AVRESAMPLE_MAX_CHANNELS];
> + double taba[AVRESAMPLE_MAX_CHANNELS];
> +
> +#define PUT_SAMPLE put_sample(data, sample_fmt, channels, k, ch, v);
> +
> + k = 0;
> +
> + /* 1 second of single freq sinus at 1000 Hz */
> + a = 0;
nit: declaration and initialization could be merged.
> +/* formats, rates, and layouts are ordered for priority in testing.
> + e.g. 'avresample-test 4 2 2' will test all input/output combinations of
> + S16/FLTP/S16P/FLT, 48000/44100, and stereo/mono */
nit: add * to all lines
> +int main(int argc, char **argv)
> +{
> + for (i = 0; i < num_formats; i++) {
> + in_fmt = formats[i];
> + for (k = 0; k < num_layouts; k++) {
> + in_ch_layout = layouts[k];
> + in_channels = av_get_channel_layout_nb_channels(in_ch_layout);
> + for (m = 0; m < num_rates; m++) {
> + in_rate = rates[m];
> +
> + ret = av_samples_fill_arrays(in_data, &in_linesize, in_buf,
> + in_channels, in_rate * 6,
> + in_fmt, 0);
> + if (ret < 0) {
> + av_log(s, AV_LOG_ERROR, "failed in_data fill arrays\n");
> + goto end;
> + }
> + audiogen(&rnd, (void **)in_data, in_fmt, in_channels,
> in_rate, in_rate * 6);
> +
> + for (j = 0; j < num_formats; j++) {
> + out_fmt = formats[j];
> + for (l = 0; l < num_layouts; l++) {
> + out_ch_layout = layouts[l];
> + out_channels =
> av_get_channel_layout_nb_channels(out_ch_layout);
> + for (n = 0; n < num_rates; n++) {
> + out_rate = rates[n];
> +
> + av_log(NULL, AV_LOG_INFO, "%s to %s, %d to %d
> channels, %d Hz to %d Hz\n",
> + av_get_sample_fmt_name(in_fmt),
> av_get_sample_fmt_name(out_fmt),
> + in_channels, out_channels, in_rate,
> out_rate);
> +
> + ret = av_samples_fill_arrays(out_data,
> &out_linesize,
> + out_buf,
> out_channels,
> + out_rate * 6,
> out_fmt, 0);
> + if (ret < 0) {
> + av_log(s, AV_LOG_ERROR, "failed out_data
> fill arrays\n");
> + goto end;
> + }
> +
> + av_opt_set_int(s, "in_channel_layout",
> in_ch_layout, 0);
> + av_opt_set_int(s, "in_sample_fmt", in_fmt,
> 0);
> + av_opt_set_int(s, "in_sample_rate", in_rate,
> 0);
> + av_opt_set_int(s, "out_channel_layout",
> out_ch_layout, 0);
> + av_opt_set_int(s, "out_sample_fmt", out_fmt,
> 0);
> + av_opt_set_int(s, "out_sample_rate",
> out_rate, 0);
> +
> + av_opt_set_int(s, "internal_sample_fmt",
> AV_SAMPLE_FMT_FLTP, 0);
> +
> + ret = avresample_open(s);
> + if (ret < 0) {
> + av_log(s, AV_LOG_ERROR, "Error opening
> context\n");
> + goto end;
> + }
> +
> + ret = avresample_convert(s, (void **)out_data,
> out_linesize, out_rate * 6,
> + (void **) in_data,
> in_linesize, in_rate * 6);
> + if (ret < 0) {
> + char errbuf[256];
> + av_strerror(ret, errbuf, sizeof(errbuf));
> + av_log(NULL, AV_LOG_ERROR, "%s\n", errbuf);
> + goto end;
> + }
> + av_log(NULL, AV_LOG_INFO, "Converted %d samples
> to %d samples\n", in_rate * 6, ret);
> + if (avresample_get_delay(s) > 0)
> + av_log(NULL, AV_LOG_INFO, "%d delay samples
> not converted\n",
> + avresample_get_delay(s));
> + if (avresample_available(s) > 0)
> + av_log(NULL, AV_LOG_INFO, "%d samples
> available for output\n",
> + avresample_available(s));
> + av_log(NULL, AV_LOG_INFO, "\n");
> +
> + avresample_close(s);
nit: long lines you may or may not care about
> --- /dev/null
> +++ b/libavresample/avresample.h
> @@ -0,0 +1,235 @@
> +/** Mixing Coefficient Types */
> +enum AVMixCoeffType {
> + AV_MIX_COEFF_TYPE_Q6, /** 16-bit 10.6 fixed-point
> */
> + AV_MIX_COEFF_TYPE_Q15, /** 32-bit 17.15 fixed-point
> */
> + AV_MIX_COEFF_TYPE_FLT, /** floating-point
> */
> + AV_MIX_COEFF_TYPE_NB, /** Number of coeff types. Not part of ABI
> */
> +};
> +
> +/**
> + * Return the LIBAVRESAMPLE_VERSION_INT constant.
> + */
> +unsigned avresample_version(void);
> +
> +/**
> + * Get the AVClass for AVAudioResampleContext.
> + *
> + * It can be used in combination with AV_OPT_SEARCH_FAKE_OBJ for examining
> + * options without allocating a context.
I'd say s/It //.
> + * @see av_opt_find().
> + *
> + * @return AVClass for AVAudioResampleContext
> + */
> +const AVClass *avresample_get_class(void);
> +
> +/**
> + * Return the libavresample build-time configuration.
> + * @return configure string
> + */
> +const char *avresample_configuration(void);
I'd move this below avresample_version; the functions are quite similar.
> +/**
> + * Generate a channel mixing matrix.
> + *
> + * @param in_layout input channel layout
> + * @param out_layout output channel layout
> + * @param center_mix_level mix level for the center channel
> + * @param surround_mix_level mix level for the surround channel(s)
> + * @param lfe_mix_level mix level for the low-frequency effects channel
> + * @param normalize indicates whether to normalize coefficients to
> + prevent overflow
How does it indicate either?
#
> + * @param matrix mixing coefficients; matrix[i + stride * o] is
> + the weight of input channel i in output
> channel o.
> + * @param stride distance between adjacent input channels in the
> + matrix array
> + */
> +int avresample_build_matrix(uint64_t in_layout, uint64_t out_layout,
> + double center_mix_level, double
> surround_mix_level,
> + double lfe_mix_level, int normalize, double
> *matrix,
> + int stride);
@return is missing.
> +/**
> + * Set channel mixing matrix.
> + *
> + * This allows for setting a custom mixing matrix, overriding the default
I'd say s/for//
> +/**
> + * Convert input samples and write them to the output FIFO.
> + *
> + * The output data can be NULL or have fewer allocated samples than required.
> + * In this case, any remaining samples not written to the output will be
> added
> + * to an internal FIFO, to be returned in the next call to this function or
> to
> + * avresample_read().
I'd say "at the next call".
> + * @param output output data pointers
> + * @param input input data pointers
pointer or pointers?
> +/**
> + * Return the libavresample license.
> + */
> +const char *avresample_license(void);
This I'd also move next to avresample_version().
> --- /dev/null
> +++ b/libavresample/internal.h
> @@ -0,0 +1,75 @@
> +
> +struct AVAudioResampleContext {
> + const AVClass *av_class; /**< AVClass for logging and AVOptions
> */
> +
> + uint64_t in_channel_layout; /**< input channel layout
> */
> + enum AVSampleFormat in_sample_fmt; /**< input sample format
> */
> + int in_sample_rate; /**< input sample rate
> */
> + uint64_t out_channel_layout; /**< output channel layout
> */
> + enum AVSampleFormat out_sample_fmt; /**< output sample format
> */
> + int out_sample_rate; /**< output sample rate
> */
> + enum AVSampleFormat internal_sample_fmt; /**< internal sample format
> */
> + enum AVMixCoeffType mix_coeff_type; /**< mixing coefficient type
> */
> + double center_mix_level; /**< center mix level
> */
> + double surround_mix_level; /**< surround mix level
> */
> + double lfe_mix_level; /**< lfe mix level
> */
> + int force_resampling; /**< force resampling
> */
> + int filter_size; /**< length of each FIR
> filter in the resampling filterbank relative to the cutoff frequency */
> + int phase_shift; /**< log2 of the number of
> entries in the resampling polyphase filterbank */
> + int linear; /**< if 1 then the
> resampling FIR filter will be linearly interpolated */
> + double cutoff; /**< resampling cutoff
> frequency. 1.0 corresponds to half the output sample rate */
> +
> + int in_channels; /**< number of input channels
> */
> + int out_channels; /**< number of output channels
> */
> + int resample_channels; /**< number of channels used for resampling
> */
> + int downmix_needed; /**< downmixing is needed
> */
> + int upmix_needed; /**< upmixing is needed
> */
> + int mixing_needed; /**< either upmixing or downmixing is needed
> */
> + int resample_needed; /**< resampling is needed
> */
> + int in_convert_needed; /**< input sample format conversion is needed
> */
> + int out_convert_needed; /**< output sample format conversion is needed
> */
> +
> + AudioData *in_buffer; /**< buffer for converted input
> */
> + AudioData *resample_out_buffer; /**< buffer for output from resampler
> */
> + AudioData *out_buffer; /**< buffer for converted output
> */
> + AVAudioFifo *out_fifo; /**< FIFO for output samples
> */
> +
> + AudioConvert *ac_in; /**< input sample format conversion context
> */
> + AudioConvert *ac_out; /**< output sample format conversion context
> */
> + ResampleContext *resample; /**< resampling context
> */
> + AudioMix *am; /**< channel mixing context
> */
> +};
see above about aligning closing comments
> --- /dev/null
> +++ b/libavresample/options.c
> @@ -0,0 +1,64 @@
> +
> +#include "options_table.c"
ugh ...
Why did you put this in a separate file and not right here?
> +AVAudioResampleContext *avresample_alloc_context(void)
> +{
> + AVAudioResampleContext *avr;
> +
> + avr = av_mallocz(sizeof(*avr));
> + if (!avr)
> + return avr;
> +
> + avr->am = av_mallocz(sizeof(*avr->am));
> + if (!avr->am) {
> + av_free(avr);
> + return NULL;
> + }
I think returning NULL in both cases would be a tad more consistent.
> --- /dev/null
> +++ b/libavresample/resample.c
> @@ -0,0 +1,466 @@
> +
> +#ifdef CONFIG_RESAMPLE_FLT
> +/* flt template */
Can I haz the "oa" back please?
> +/**
> + * builds a polyphase filterbank.
Build
> + * @param factor resampling factor
> + * @param scale wanted sum of coefficients for each filter
> + * @param type 0->cubic, 1->blackman nuttall windowed sinc, 2..16->kaiser
> windowed sinc beta=2..16
> + * @return 0 on success, negative on error
> + */
> +static int build_filter(FELEM *filter, double factor, int tap_count,
> + int phase_count, int scale, int type)
Not all parameters are documented, this will add Doxygen warnings.
> + switch (type) {
> + case 0: {
> + const float d = -0.5; //first order derivative = -0.5
> + x = fabs(((double)(i - center) - (double)ph / phase_count) *
> factor);
> + if (x < 1.0) y = 1 - 3*x*x + 2*x*x*x + d*( -x*x +
> x*x*x);
> + else y = d*(-4 + 8*x - 5*x*x +
> x*x*x);
> + break;
No spaces around '*' here?
> + /* TODO: add support for s32 and float internal formats */
> + if (avr->internal_sample_fmt != AV_SAMPLE_FMT_S16P) {
> + av_log(avr, AV_LOG_ERROR, "Unsupported internal format for
> resampling: %s\n",
> + av_get_sample_fmt_name(avr->internal_sample_fmt));
nit: long line
> + av_log(avr, AV_LOG_DEBUG, "resample: %s from %d Hz to %d Hz\n",
> + av_get_sample_fmt_name(avr->internal_sample_fmt),
> + avr->in_sample_rate, avr->out_sample_rate);
av_dlog?
> + return c;
> +error:
> + ff_audio_data_free(&c->buffer);
> + av_free(c->filter_bank);
> + av_free(c);
> + return NULL;
> +}
Please add an empty line before goto labels.
> +int avresample_set_compensation(AVAudioResampleContext *avr, int
> sample_delta,
> + int compensation_distance)
> +{
> + return 0;
> +reinit_fail:
> + ff_audio_data_free(&fifo_buf);
> + return ret;
> +}
ditto
> +static int resample(ResampleContext *c, int16_t *dst, const int16_t *src,
> + int *consumed, int src_size, int dst_size, int
> update_ctx)
> +{
> + int dst_index, i;
> + int index = c->index;
> + int frac = c->frac;
> + int dst_incr_frac = c->dst_incr % c->src_incr;
> + int dst_incr = c->dst_incr / c->src_incr;
> + int compensation_distance = c->compensation_distance;
nit: You could align one more here.
> + if (compensation_distance == 0 && c->filter_length == 1 &&
> c->phase_shift == 0) {
> + int64_t index2 = ((int64_t)index) << 32;
> + int64_t incr = (1LL << 32) * c->dst_incr / c->src_incr;
> + dst_size = FFMIN(dst_size, (src_size-1-index) *
> (int64_t)c->src_incr / c->dst_incr);
> + } else {
> + for (dst_index = 0; dst_index < dst_size; dst_index++) {
> +
> + if (!dst && (sample_index + c->filter_length > src_size ||
> -sample_index >= src_size))
> + break;
nit: long lines
> + c->frac = frac;
> + c->index = index;
> + c->dst_incr = dst_incr_frac + c->src_incr*dst_incr;
> + c->compensation_distance = compensation_distance;
nit: You could align one more here.
> --- /dev/null
> +++ b/libavresample/utils.c
> @@ -0,0 +1,400 @@
> +
> +int avresample_open(AVAudioResampleContext *avr)
> +{
> + /* allocate buffers */
> + if (avr->mixing_needed || avr->in_convert_needed) {
> + avr->in_buffer = ff_audio_data_alloc(FFMAX(avr->in_channels,
> avr->out_channels),
> + 0, avr->internal_sample_fmt,
> + "in_buffer");
> + if (avr->resample_needed) {
> + avr->resample_out_buffer = ff_audio_data_alloc(avr->out_channels,
> + 0,
> avr->internal_sample_fmt,
> +
> "resample_out_buffer");
> + if (avr->out_convert_needed) {
> + avr->out_buffer = ff_audio_data_alloc(avr->out_channels, 0,
> + avr->out_sample_fmt,
> "out_buffer");
> + /* setup contexts */
> + if (avr->in_convert_needed) {
> + avr->ac_in = ff_audio_convert_alloc(avr, avr->internal_sample_fmt,
> + avr->in_sample_fmt,
> avr->in_channels);
nit: long lines
> + return 0;
> +error:
> + avresample_close(avr);
> + return ret;
> +}
see above about goto labels
> --- /dev/null
> +++ b/libavresample/version.h
> @@ -0,0 +1,42 @@
> +/**
> + * These FF_API_* defines are not part of public API.
> + * They may change, break or disappear at any time.
> + */
> +
> +
> +#endif /* AVRESAMPLE_VERSION_H */
nit: double empty line
> --- /dev/null
> +++ b/libavresample/x86/audio_convert_mmx.c
> @@ -0,0 +1,43 @@
> +
> +extern void ff_conv_fltp_to_flt_6ch_mmx(float *dst, float *const *src, int
> len);
> +extern void ff_conv_fltp_to_flt_6ch_sse(float *dst, float *const *src, int
> len);
> +
> +av_cold void ff_audio_convert_init_x86(AudioConvert *ac)
> +{
> +#if HAVE_YASM
> + int mm_flags = av_get_cpu_flags();
> +
> + if (mm_flags & AV_CPU_FLAG_MMX) {
> + ff_audio_convert_set_func(ac, AV_SAMPLE_FMT_FLT, AV_SAMPLE_FMT_FLTP,
> + 6, 1, 4, "MMX",
> ff_conv_fltp_to_flt_6ch_mmx);
> +
> + if (mm_flags & AV_CPU_FLAG_SSE && HAVE_SSE) {
> + ff_audio_convert_set_func(ac, AV_SAMPLE_FMT_FLT,
> AV_SAMPLE_FMT_FLTP,
> + 6, 16, 4, "SSE",
> ff_conv_fltp_to_flt_6ch_sse);
> + }
pointless {}
Maybe not nesting the second check inside the first is cleaner.
> + }
> +#endif
> +}
> --- /dev/null
> +++ b/libavresample/x86/audio_mix_mmx.c
> @@ -0,0 +1,46 @@
> +
> +void ff_mix_2_to_1_fltp_flt_sse(float **src, float **matrix, int len,
> + int out_ch, int in_ch);
> +void ff_mix_2_to_1_fltp_flt_avx(float **src, float **matrix, int len,
> + int out_ch, int in_ch);
> +
> +av_cold void ff_audio_mix_init_x86(AudioMix *am)
> +{
> +#if HAVE_YASM
> + int mm_flags = av_get_cpu_flags();
> +
> + if (mm_flags & AV_CPU_FLAG_MMX) {
> + if (mm_flags & AV_CPU_FLAG_SSE && HAVE_SSE) {
> + ff_audio_mix_set_func(am, AV_SAMPLE_FMT_FLTP,
> AV_MIX_COEFF_TYPE_FLT,
> + 2, 1, 16, 8, "SSE",
> ff_mix_2_to_1_fltp_flt_sse);
> + }
> + if (mm_flags & AV_CPU_FLAG_AVX && HAVE_AVX) {
> + ff_audio_mix_set_func(am, AV_SAMPLE_FMT_FLTP,
> AV_MIX_COEFF_TYPE_FLT,
> + 2, 1, 32, 16, "AVX",
> ff_mix_2_to_1_fltp_flt_avx);
> + }
> + }
more pointless {}
I think there is no point in checking for AV_CPU_FLAG_MMX first.
> +#endif
> +}
Why do these two files have "_mmx" in the name? Only the first has some
MMX code in it, the other it seems to be about SSE and AVX.
audio_foo_init.c sounds like a better name to me.
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
