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

[Michael Niedermayer]

On Wed, Nov 08, 2017 at 11:41:16PM +0100, Aurelien Jacobs 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

me personally, not really, i used "for (int" style as long as
i can remember outside ffmpeg but i also would like to keep supportng
all platforms ...


> 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 !

Theres a seperate thread about this now


> 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.

While that can be done, i think its especially with rarer platforms
not such a good idea. Imagine everyone would do this, for example
for every E* error code that is not supported on all platforms,
FFmpeg would often randomly not build on platforms that are less
mainstream as people re-test what breaks for whom ...

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The worst form of inequality is to try to make unequal things equal.
-- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20171109/dd67b370/attachment.sig>