[FFmpeg-devel] [PATCH 1/2] aptx: implement the aptX bluetooth codec

[Rostislav Pehlivanov]

On 8 November 2017 at 22:41, Aurelien Jacobs <aurel at gnuage.org> wrote:

> On Wed, Nov 08, 2017 at 06:26:03PM +0100, Michael Niedermayer wrote:
> > On Wed, Nov 08, 2017 at 02:06:09PM +0100, Aurelien Jacobs wrote:
> > [...]
> > > +typedef const struct {
> > > +    const int32_t *quantize_intervals;
> > > +    const int32_t *invert_quantize_dither_factors;
> > > +    const int32_t *quantize_dither_factors;
> >
> > > +    const int32_t *quantize_factor_select_offset;
> >
> > this would fit in int16_t *
>
> Right.
>
> > [...]
> > > +static const int32_t quantization_factors[32] = {
> > > +    2048, 2093, 2139, 2186, 2233, 2282, 2332, 2383,
> > > +    2435, 2489, 2543, 2599, 2656, 2714, 2774, 2834,
> > > +    2896, 2960, 3025, 3091, 3158, 3228, 3298, 3371,
> > > +    3444, 3520, 3597, 3676, 3756, 3838, 3922, 4008,
> > > +};
> >
> > this too would fir in int16_t
>
> Indeed, now that I moved the shift inside the code.
>
> > [...]
> > > +/*
> > > + * Push one sample into a circular signal buffer.
> > > + */
> > > +av_always_inline
> > > +static void aptx_qmf_filter_signal_push(FilterSignal *signal,
> int32_t sample)
> > > +{
> > > +    signal->buffer[signal->pos            ] = sample;
> > > +    signal->buffer[signal->pos+FILTER_TAPS] = sample;
> > > +    signal->pos = (signal->pos + 1) % FILTER_TAPS;
> >
> > % could be replaced by &
>
> OK. And there was a second one that I changed as well.
>
> > > +}
> > > +
> > > +/*
> > > + * Compute the convolution of the signal with the coefficients, and
> reduce
> > > + * to 24 bits by applying the specified right shifting.
> > > + */
> > > +av_always_inline
> > > +static int32_t aptx_qmf_convolution(FilterSignal *signal,
> > > +                                    const int32_t coeffs[FILTER_TAPS],
> > > +                                    int shift)
> > > +{
> > > +    int32_t *sig = &signal->buffer[signal->pos];
> > > +    int64_t e = 0;
> > > +
> >
> > > +    for (int i = 0; i < FILTER_TAPS; i++)
> >
> > "for (int" is something we avoided as some comilers didnt like it,
> > iam not sure if this is still true but there are none in the codebase
>
> If you insist on this I will of course change it, but hey, we require
> a C99 compiler since a long time and we use so many features of C99
> that I really don't get why we couldn't use "for (int".
> I can't imagine that any relevant C compiler would not support this
> nowadays !
> What I propose is to get this in as is, and see if anyone encounter
> issue with any compiler. If any issue arise, I will of course send a
> patch to fix it.
>
> Here is an updated patch.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
Send another patch because some people are beyond convincing and its really
pissing me off. Really. In particular maybe those compiler writers at
Microsoft who seem to think shipping something that doesn't support such a
simple yet important feature is important.
Or maybe users who don't want to update a 6 year old version of msvc.
Or maybe us because we support compiling with msvc at all when its clearly
_not_ a C compiler and this project is written in C.