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

[Rostislav Pehlivanov]

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

> On Thu, Nov 09, 2017 at 02:32:44PM +0000, Rostislav Pehlivanov wrote:
> > On 9 November 2017 at 13:37, Aurelien Jacobs <aurel at gnuage.org> wrote:
> >
> > > On Thu, Nov 09, 2017 at 12:52:34AM +0000, Rostislav Pehlivanov wrote:
> > > > 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:
> > > > > > [...]
> > > > > > > +}
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * 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.
> > > > >
> > > > >
> > > > 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.
> > >
> > > Here you go.
> > >
> > > Just for reference, splitting the déclaration out of the for loop added
> > > 19 lines in this file, which is a 2.3 % increase of the line count.
> > > (I'm not sure this file is representative of the rest of the code base)
> > >
> > > _______________________________________________
> > > ffmpeg-devel mailing list
> > > ffmpeg-devel at ffmpeg.org
> > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >
> > >
> > Still some issues:
> >
> > 1.) You need to set AVCodec->supported_samplerates for the encoder, since
> > in the raw aptx muxer you always set the samplerate as 44100. Does the
> > codec/container support other samplerates?
>
> The codec is actually samplerate agnostic.
> It only convert each group of 4 samples to a 16 bit integer.
> There is no other information than the samples themselves in the
> encoded stream. No header, no frame size, no samplerate.
> And there is no standard container used to store aptX along with
> the needed metadata such as the samplerate. It is meant to be streamed
> thru bluetooth using the A2DP "protocol" which includes its own metadata
> signaling with samplerate.
> The raw muxer/demuxer that I implemented is mostly for testing purpose,
> not really for practical use.
> So with this in mind, I don't think that it make any sense to set
> AVCodec->supported_samplerates.
> I don't think I set the samplerate anywhere in the encoder.
> I only set it in the demuxer to a default 44100 just to be able to
> playback test files even though there is no way to know its actual
> samplerate.
>
>
Ah, I see.
In this case set the allowed samplerate to 48000 on the encoder side and
the output samplerate in the demuxer to the same. Pretty much all systems,
be it bluetooth, I2S or some other interface run at 48khz so that's what we
should set it so its as compatible as possible. Nothing but systems which
handle redbook cds use 44100hz.