On 04/10/2012 05:17 PM, Diego Biurrun wrote:
> On Mon, Apr 09, 2012 at 05:27:52PM -0400, Justin Ruggles wrote:
[...]
>> +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.
This can't be called from a global init function. It depends very much
on values in AudioConvert that are set in ff_audio_convert_alloc().
> There is no dependency on MMX there and these are implementation details.
> Please check for ARCH_X86 instead.
What do you mean there is no dependency on MMX? That's why we use
OBJS-$(HAVE_MMX) in the Makefile. There really is no point in adding
optimized functions for x86 in this case if building with MMX support is
disabled. We don't have a need for non-SIMD x86 assembly optimizations
in this library.
>> --- /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?
I think normal AV_LOG_DEBUG is fine here. It's only called at init so it
won't be too noisy.
>> +static int mix_function_init(AudioMix *am)
>> +{
>> +
>> + if (HAVE_MMX)
>> + ff_audio_mix_init_x86(am);
>
> ARCH_X86
See my reasoning for audio_convert. It's the same situation here.
>> +/**
>> + * 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?
pointer to pointers...
I'll add @see enum AVSampleFormat to make it clearer.
See related patch "avutil: add better documentation for AVSampleFormat"
>> + 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?
I'll put spaces around most of them, but the x's I want to leave as-is.
Because it's really just a long form of x-squared and x-cubed, and that
is easier to see without the extra spaces I think.
>> + 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?
Again I think AV_LOG_DEBUG is fine here since it's at init.
>> --- /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.
I'm leaving the {}. But yeah I can unnest the checks.
>> + }
>> +#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 {}
same here.
Thanks,
Justin
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
