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

[Rostislav Pehlivanov]

On 10 November 2017 at 21:09, Aurelien Jacobs  wrote:

> The encoder was reverse engineered from binary library and from
> EP0398973B1 patent (long expired).
> The decoder was simply deduced from the encoder.
> ---
>  doc/general.texi|   2 +
>  libavcodec/Makefile |   2 +
>  libavcodec/allcodecs.c  |   1 +
>  libavcodec/aptx.c   | 860 ++
> ++
>  libavcodec/avcodec.h|   1 +
>  libavcodec/codec_desc.c |   7 +
>  6 files changed, 873 insertions(+)
>  create mode 100644 libavcodec/aptx.c
>
> diff --git a/doc/general.texi b/doc/general.texi
> index e6ae277d23..de4efee913 100644
> --- a/doc/general.texi
> +++ b/doc/general.texi
> @@ -993,6 +993,8 @@ following image formats are supported:
>  @item Amazing Studio PAF Audio @tab @tab  X
>  @item Apple lossless audio   @tab  X  @tab  X
>  @tab QuickTime fourcc 'alac'
> +@item aptX   @tab  X  @tab  X
> +@tab Used in Bluetooth A2DP
>  @item ATRAC1 @tab @tab  X
>  @item ATRAC3 @tab @tab  X
>  @item ATRAC3+@tab @tab  X
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index 45f4db5939..95c843dee7 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -188,6 +188,8 @@ OBJS-$(CONFIG_AMV_ENCODER) += mjpegenc.o
> mjpegenc_common.o \
>  OBJS-$(CONFIG_ANM_DECODER) += anm.o
>  OBJS-$(CONFIG_ANSI_DECODER)+= ansi.o cga_data.o
>  OBJS-$(CONFIG_APE_DECODER) += apedec.o
> +OBJS-$(CONFIG_APTX_DECODER)+= aptx.o
> +OBJS-$(CONFIG_APTX_ENCODER)+= aptx.o
>  OBJS-$(CONFIG_APNG_DECODER)+= png.o pngdec.o pngdsp.o
>  OBJS-$(CONFIG_APNG_ENCODER)+= png.o pngenc.o
>  OBJS-$(CONFIG_SSA_DECODER) += assdec.o ass.o
> diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
> index d96e499ba7..463f7ed64e 100644
> --- a/libavcodec/allcodecs.c
> +++ b/libavcodec/allcodecs.c
> @@ -406,6 +406,7 @@ static void register_all(void)
>  REGISTER_DECODER(AMRNB, amrnb);
>  REGISTER_DECODER(AMRWB, amrwb);
>  REGISTER_DECODER(APE,   ape);
> +REGISTER_ENCDEC (APTX,  aptx);
>  REGISTER_DECODER(ATRAC1,atrac1);
>  REGISTER_DECODER(ATRAC3,atrac3);
>  REGISTER_DECODER(ATRAC3AL,  atrac3al);
> diff --git a/libavcodec/aptx.c b/libavcodec/aptx.c
> new file mode 100644
> index 00..d09ce8f838
> --- /dev/null
> +++ b/libavcodec/aptx.c
> @@ -0,0 +1,860 @@
> +/*
> + * Audio Processing Technology codec for Bluetooth (aptX)
> + *
> + * Copyright (C) 2017  Aurelien Jacobs 
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02110-1301 USA
> + */
> +
> +#include "libavutil/intreadwrite.h"
> +#include "avcodec.h"
> +#include "internal.h"
> +#include "mathops.h"
> +#include "audio_frame_queue.h"
> +
> +
> +enum channels {
> +LEFT,
> +RIGHT,
> +NB_CHANNELS
> +};
> +
> +enum subbands {
> +LF,  // Low Frequency (0-5.5 kHz)
> +MLF, // Medium-Low Frequency (5.5-11kHz)
> +MHF, // Medium-High Frequency (11-16.5kHz)
> +HF,  // High Frequency (16.5-22kHz)
> +NB_SUBBANDS
> +};
> +
> +#define NB_FILTERS 2
> +#define FILTER_TAPS 16
> +
> +typedef struct {
> +int pos;
> +int32_t buffer[2*FILTER_TAPS];
> +} FilterSignal;
> +
> +typedef struct {
> +FilterSignal outer_filter_signal[NB_FILTERS];
> +FilterSignal inner_filter_signal[NB_FILTERS][NB_FILTERS];
> +} QMFAnalysis;
> +
> +typedef struct {
> +int32_t quantized_sample;
> +int32_t quantized_sample_parity_change;
> +int32_t error;
> +} Quantize;
> +
> +typedef struct {
> +int32_t quantization_factor;
> +int32_t factor_select;
> +int32_t reconstructed_difference;
> +} InvertQuantize;
> +
> +typedef struct {
> +int32_t prev_sign[2];
> +int32_t s_weight[2];
> +int32_t d_weight[24];
> +int32_t pos;
> +int32_t reconstructed_differences[48];
> +int32_t previous_reconstructed_sample;
> +int32_t predicted_difference;
> +int32_t predicted_sample;
> +} Prediction;
> +
> +typedef struct {
> +int32_t codeword_history;
> +int32_t 

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

[Aurelien Jacobs]

On Fri, Nov 10, 2017 at 12:25:12AM +, Rostislav Pehlivanov wrote:
> On 9 November 2017 at 22:48, Aurelien Jacobs  wrote:
> 
> > On Thu, Nov 09, 2017 at 02:32:44PM +, Rostislav Pehlivanov wrote:
> > > On 9 November 2017 at 13:37, Aurelien Jacobs  wrote:
> > >
> > > > On Thu, Nov 09, 2017 at 12:52:34AM +, Rostislav Pehlivanov wrote:
> > > > > On 8 November 2017 at 22:41, 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:
> > > > > > > [...]
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +/*
> > > > > > > > + * 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 = >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@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.

I found a list of supported samplerate in an aptX commercial
documentation, so I think it makes sense to list them in the encoder.
Regarding the demuxer, I set it 48000 by default, but I added an
AVOption to allow user to 

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

[Aurelien Jacobs]

The encoder was reverse engineered from binary library and from
EP0398973B1 patent (long expired).
The decoder was simply deduced from the encoder.
---
 doc/general.texi|   2 +
 libavcodec/Makefile |   2 +
 libavcodec/allcodecs.c  |   1 +
 libavcodec/aptx.c   | 860 
 libavcodec/avcodec.h|   1 +
 libavcodec/codec_desc.c |   7 +
 6 files changed, 873 insertions(+)
 create mode 100644 libavcodec/aptx.c

diff --git a/doc/general.texi b/doc/general.texi
index e6ae277d23..de4efee913 100644
--- a/doc/general.texi
+++ b/doc/general.texi
@@ -993,6 +993,8 @@ following image formats are supported:
 @item Amazing Studio PAF Audio @tab @tab  X
 @item Apple lossless audio   @tab  X  @tab  X
 @tab QuickTime fourcc 'alac'
+@item aptX   @tab  X  @tab  X
+@tab Used in Bluetooth A2DP
 @item ATRAC1 @tab @tab  X
 @item ATRAC3 @tab @tab  X
 @item ATRAC3+@tab @tab  X
diff --git a/libavcodec/Makefile b/libavcodec/Makefile
index 45f4db5939..95c843dee7 100644
--- a/libavcodec/Makefile
+++ b/libavcodec/Makefile
@@ -188,6 +188,8 @@ OBJS-$(CONFIG_AMV_ENCODER) += mjpegenc.o 
mjpegenc_common.o \
 OBJS-$(CONFIG_ANM_DECODER) += anm.o
 OBJS-$(CONFIG_ANSI_DECODER)+= ansi.o cga_data.o
 OBJS-$(CONFIG_APE_DECODER) += apedec.o
+OBJS-$(CONFIG_APTX_DECODER)+= aptx.o
+OBJS-$(CONFIG_APTX_ENCODER)+= aptx.o
 OBJS-$(CONFIG_APNG_DECODER)+= png.o pngdec.o pngdsp.o
 OBJS-$(CONFIG_APNG_ENCODER)+= png.o pngenc.o
 OBJS-$(CONFIG_SSA_DECODER) += assdec.o ass.o
diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
index d96e499ba7..463f7ed64e 100644
--- a/libavcodec/allcodecs.c
+++ b/libavcodec/allcodecs.c
@@ -406,6 +406,7 @@ static void register_all(void)
 REGISTER_DECODER(AMRNB, amrnb);
 REGISTER_DECODER(AMRWB, amrwb);
 REGISTER_DECODER(APE,   ape);
+REGISTER_ENCDEC (APTX,  aptx);
 REGISTER_DECODER(ATRAC1,atrac1);
 REGISTER_DECODER(ATRAC3,atrac3);
 REGISTER_DECODER(ATRAC3AL,  atrac3al);
diff --git a/libavcodec/aptx.c b/libavcodec/aptx.c
new file mode 100644
index 00..d09ce8f838
--- /dev/null
+++ b/libavcodec/aptx.c
@@ -0,0 +1,860 @@
+/*
+ * Audio Processing Technology codec for Bluetooth (aptX)
+ *
+ * Copyright (C) 2017  Aurelien Jacobs 
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include "libavutil/intreadwrite.h"
+#include "avcodec.h"
+#include "internal.h"
+#include "mathops.h"
+#include "audio_frame_queue.h"
+
+
+enum channels {
+LEFT,
+RIGHT,
+NB_CHANNELS
+};
+
+enum subbands {
+LF,  // Low Frequency (0-5.5 kHz)
+MLF, // Medium-Low Frequency (5.5-11kHz)
+MHF, // Medium-High Frequency (11-16.5kHz)
+HF,  // High Frequency (16.5-22kHz)
+NB_SUBBANDS
+};
+
+#define NB_FILTERS 2
+#define FILTER_TAPS 16
+
+typedef struct {
+int pos;
+int32_t buffer[2*FILTER_TAPS];
+} FilterSignal;
+
+typedef struct {
+FilterSignal outer_filter_signal[NB_FILTERS];
+FilterSignal inner_filter_signal[NB_FILTERS][NB_FILTERS];
+} QMFAnalysis;
+
+typedef struct {
+int32_t quantized_sample;
+int32_t quantized_sample_parity_change;
+int32_t error;
+} Quantize;
+
+typedef struct {
+int32_t quantization_factor;
+int32_t factor_select;
+int32_t reconstructed_difference;
+} InvertQuantize;
+
+typedef struct {
+int32_t prev_sign[2];
+int32_t s_weight[2];
+int32_t d_weight[24];
+int32_t pos;
+int32_t reconstructed_differences[48];
+int32_t previous_reconstructed_sample;
+int32_t predicted_difference;
+int32_t predicted_sample;
+} Prediction;
+
+typedef struct {
+int32_t codeword_history;
+int32_t dither_parity;
+int32_t dither[NB_SUBBANDS];
+
+QMFAnalysis qmf;
+Quantize quantize[NB_SUBBANDS];
+InvertQuantize invert_quantize[NB_SUBBANDS];
+Prediction prediction[NB_SUBBANDS];
+} Channel;
+
+typedef struct {
+int32_t sync_idx;
+Channel channels[NB_CHANNELS];
+AudioFrameQueue afq;
+} AptXContext;
+
+
+static const int32_t 

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

[Rostislav Pehlivanov]

On 9 November 2017 at 22:48, Aurelien Jacobs  wrote:

> On Thu, Nov 09, 2017 at 02:32:44PM +, Rostislav Pehlivanov wrote:
> > On 9 November 2017 at 13:37, Aurelien Jacobs  wrote:
> >
> > > On Thu, Nov 09, 2017 at 12:52:34AM +, Rostislav Pehlivanov wrote:
> > > > On 8 November 2017 at 22:41, 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:
> > > > > > [...]
> > > > > > > +}
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * 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 = >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@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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Aurelien Jacobs]

On Thu, Nov 09, 2017 at 02:32:44PM +, Rostislav Pehlivanov wrote:
> On 9 November 2017 at 13:37, Aurelien Jacobs  wrote:
> 
> > On Thu, Nov 09, 2017 at 12:52:34AM +, Rostislav Pehlivanov wrote:
> > > On 8 November 2017 at 22:41, 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:
> > > > > [...]
> > > > > > +}
> > > > > > +
> > > > > > +/*
> > > > > > + * 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 = >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@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.

> 2.) "Frame must have a multiple of 4 samples" gets printed out when trying
> to resample a 48000 file and encode it to aptx.
> What you need to do is to remove AV_CODEC_CAP_VARIABLE_FRAME_SIZE from the
> capabilities and set some sane default for avctx->frame_size if
> avctx->frame_size is unset. Users can override it prior to encoder init and
> you can do it through the command line using -frame_size. If the user sets
> the frame_size, check if its a multiple of 4 and use it. Else, error out.
> If it isn't set (e.g. its 0), use the default (1024 is a good value).

OK, I will try this.
And I should probably document that for acheiving the lowest possible
latency for realtime bluetooth streaming, frame_size shoud be set to
4 samples.

> 3.) The packet timestamps on the encoder side are missing. Use the
> AudioFrameQueue API (libavcodec/audio_frame_queue.h) which at every frame
> counts how many samples you're given via the avframe and calculates what
> dts/pts to set on the avpacket.
> Just call ff_af_queue_init at the end of your init function (after the
> frame size is set and checked), call 

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

[Rostislav Pehlivanov]

On 9 November 2017 at 13:37, Aurelien Jacobs  wrote:

> On Thu, Nov 09, 2017 at 12:52:34AM +, Rostislav Pehlivanov wrote:
> > On 8 November 2017 at 22:41, 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:
> > > > [...]
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * 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 = >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@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?

2.) "Frame must have a multiple of 4 samples" gets printed out when trying
to resample a 48000 file and encode it to aptx.
What you need to do is to remove AV_CODEC_CAP_VARIABLE_FRAME_SIZE from the
capabilities and set some sane default for avctx->frame_size if
avctx->frame_size is unset. Users can override it prior to encoder init and
you can do it through the command line using -frame_size. If the user sets
the frame_size, check if its a multiple of 4 and use it. Else, error out.
If it isn't set (e.g. its 0), use the default (1024 is a good value).

3.) The packet timestamps on the encoder side are missing. Use the
AudioFrameQueue API (libavcodec/audio_frame_queue.h) which at every frame
counts how many samples you're given via the avframe and calculates what
dts/pts to set on the avpacket.
Just call ff_af_queue_init at the end of your init function (after the
frame size is set and checked), call ff_af_queue_add at the start of your
encode function and ff_af_queue_remove with the number of samples you
encoded and pointers to the packet pts/duration fields. And make sure to
call ff_af_queue_close on uninit, which will print out an error if you've
encoded more samples than you've received or less samples than you've
recevied.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[James Almer]

On 11/9/2017 10:37 AM, Aurelien Jacobs wrote:
> On Thu, Nov 09, 2017 at 12:52:34AM +, Rostislav Pehlivanov wrote:
>> On 8 November 2017 at 22:41, 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:
 [...]
> +}
> +
> +/*
> + * 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 = >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)

If you combine lines like

+int channel;
+int ret;

and

+int pos, channel, sample;
+int ret;

You can reduce that a bit.

Don't send a new patch just for that. Only if there's some other change
requested. Whoever commits this can change it before pushing.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Aurelien Jacobs]

On Thu, Nov 09, 2017 at 03:22:21AM +0100, Michael Niedermayer wrote:
> 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:
> > > [...]
> > > > +}
> > > > +
> > > > +/*
> > > > + * 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 = >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 ...

OK, got it.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Aurelien Jacobs]

On Thu, Nov 09, 2017 at 12:52:34AM +, Rostislav Pehlivanov wrote:
> On 8 November 2017 at 22:41, 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:
> > > [...]
> > > > +}
> > > > +
> > > > +/*
> > > > + * 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 = >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)
>From a0c78c471247847b841eff631f6d3a4a7db868d9 Mon Sep 17 00:00:00 2001
From: Aurelien Jacobs 
Date: Thu, 31 Aug 2017 20:12:54 +0200
Subject: [PATCH 1/2] aptx: implement the aptX bluetooth codec

The encoder was reverse engineered from binary library and from
EP0398973B1 patent (long expired).
The decoder was simply deduced from the encoder.
---
 doc/general.texi|   2 +
 libavcodec/Makefile |   2 +
 libavcodec/allcodecs.c  |   1 +
 libavcodec/aptx.c   | 845 
 libavcodec/avcodec.h|   1 +
 libavcodec/codec_desc.c |   7 +
 6 files changed, 858 insertions(+)
 create mode 100644 libavcodec/aptx.c

diff --git a/doc/general.texi b/doc/general.texi
index e6ae277d23..de4efee913 100644
--- a/doc/general.texi
+++ b/doc/general.texi
@@ -993,6 +993,8 @@ following image formats are supported:
 @item Amazing Studio PAF Audio @tab @tab  X
 @item Apple lossless audio   @tab  X  @tab  X
 @tab QuickTime fourcc 'alac'
+@item aptX   @tab  X  @tab  X
+@tab Used in Bluetooth A2DP
 @item ATRAC1 @tab @tab  X
 @item ATRAC3 @tab @tab  X
 @item ATRAC3+@tab @tab  X
diff --git a/libavcodec/Makefile b/libavcodec/Makefile
index 45f4db5939..95c843dee7 100644
--- a/libavcodec/Makefile
+++ b/libavcodec/Makefile
@@ -188,6 +188,8 @@ OBJS-$(CONFIG_AMV_ENCODER) += mjpegenc.o mjpegenc_common.o \
 OBJS-$(CONFIG_ANM_DECODER) += anm.o
 OBJS-$(CONFIG_ANSI_DECODER)+= ansi.o cga_data.o
 OBJS-$(CONFIG_APE_DECODER) += apedec.o
+OBJS-$(CONFIG_APTX_DECODER)+= aptx.o
+OBJS-$(CONFIG_APTX_ENCODER)+= aptx.o
 OBJS-$(CONFIG_APNG_DECODER)+= png.o pngdec.o pngdsp.o
 OBJS-$(CONFIG_APNG_ENCODER)+= png.o pngenc.o
 OBJS-$(CONFIG_SSA_DECODER) += assdec.o ass.o
diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
index d96e499ba7..463f7ed64e 100644
--- a/libavcodec/allcodecs.c
+++ b/libavcodec/allcodecs.c
@@ -406,6 +406,7 @@ static void register_all(void)
 REGISTER_DECODER(AMRNB, amrnb);
 REGISTER_DECODER(AMRWB, amrwb);
 REGISTER_DECODER(APE,   ape);
+REGISTER_ENCDEC (APTX,  aptx);
 REGISTER_DECODER(ATRAC1,atrac1);
 REGISTER_DECODER(ATRAC3,atrac3);
 REGISTER_DECODER(ATRAC3AL,  atrac3al);
diff --git a/libavcodec/aptx.c b/libavcodec/aptx.c
new file mode 100644
index 00..32e0d01dcf
--- /dev/null
+++ b/libavcodec/aptx.c
@@ -0,0 +1,845 @@
+/*
+ * Audio Processing Technology codec for Bluetooth (aptX)
+ *
+ * Copyright (C) 2017  Aurelien Jacobs 
+ *
+ * 

Re: [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 = >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


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Rostislav Pehlivanov]

On 8 November 2017 at 22:41, 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 = >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@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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Aurelien Jacobs]

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 = >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.>From d7c728bdb2e78ccfb49a17e7fa75785e771480f6 Mon Sep 17 00:00:00 2001
From: Aurelien Jacobs 
Date: Thu, 31 Aug 2017 20:12:54 +0200
Subject: [PATCH 1/2] aptx: implement the aptX bluetooth codec

The encoder was reverse engineered from binary library and from
EP0398973B1 patent (long expired).
The decoder was simply deduced from the encoder.
---
 doc/general.texi|   2 +
 libavcodec/Makefile |   2 +
 libavcodec/allcodecs.c  |   1 +
 libavcodec/aptx.c   | 826 
 libavcodec/avcodec.h|   1 +
 libavcodec/codec_desc.c |   7 +
 6 files changed, 839 insertions(+)
 create mode 100644 libavcodec/aptx.c

diff --git a/doc/general.texi b/doc/general.texi
index e6ae277d23..de4efee913 100644
--- a/doc/general.texi
+++ b/doc/general.texi
@@ -993,6 +993,8 @@ following image formats are supported:
 @item Amazing Studio PAF Audio @tab @tab  X
 @item Apple lossless audio   @tab  X  @tab  X
 @tab QuickTime fourcc 'alac'
+@item aptX   @tab  X  @tab  X
+@tab Used in Bluetooth A2DP
 @item ATRAC1 @tab @tab  X
 @item ATRAC3 @tab @tab  X
 @item ATRAC3+@tab @tab  X
diff --git a/libavcodec/Makefile b/libavcodec/Makefile
index 45f4db5939..95c843dee7 100644
--- a/libavcodec/Makefile
+++ b/libavcodec/Makefile
@@ -188,6 +188,8 @@ OBJS-$(CONFIG_AMV_ENCODER) += mjpegenc.o mjpegenc_common.o \
 OBJS-$(CONFIG_ANM_DECODER) += anm.o
 OBJS-$(CONFIG_ANSI_DECODER)+= ansi.o cga_data.o
 OBJS-$(CONFIG_APE_DECODER) += apedec.o
+OBJS-$(CONFIG_APTX_DECODER)+= aptx.o
+OBJS-$(CONFIG_APTX_ENCODER)+= aptx.o
 OBJS-$(CONFIG_APNG_DECODER)+= png.o pngdec.o pngdsp.o
 OBJS-$(CONFIG_APNG_ENCODER)+= png.o pngenc.o
 OBJS-$(CONFIG_SSA_DECODER) += assdec.o ass.o
diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
index d96e499ba7..463f7ed64e 100644
--- a/libavcodec/allcodecs.c
+++ b/libavcodec/allcodecs.c
@@ -406,6 +406,7 @@ static void register_all(void)
 REGISTER_DECODER(AMRNB, amrnb);
 REGISTER_DECODER(AMRWB, amrwb);
 REGISTER_DECODER(APE,   ape);
+REGISTER_ENCDEC (APTX,  aptx);
 REGISTER_DECODER(ATRAC1,atrac1);
 REGISTER_DECODER(ATRAC3,atrac3);
 REGISTER_DECODER(ATRAC3AL,  atrac3al);
diff --git a/libavcodec/aptx.c b/libavcodec/aptx.c
new file mode 100644
index 00..392af2eb31
--- /dev/null
+++ b/libavcodec/aptx.c
@@ -0,0 +1,826 @@

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

[Rostislav Pehlivanov]

On 8 November 2017 at 17:26, 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 *
>
>
> > +int tables_size;
> > +int32_t quantized_bits;
> > +int32_t prediction_order;
> > +} ConstTables;
> > +
> > +static ConstTables tables[NB_SUBBANDS] = {
> > +[LF]  = { quantize_intervals_LF,
> > +  invert_quantize_dither_factors_LF,
> > +  quantize_dither_factors_LF,
> > +  quantize_factor_select_offset_LF,
> > +  FF_ARRAY_ELEMS(quantize_intervals_LF),
> > +  7, 24 },
> > +[MLF] = { quantize_intervals_MLF,
> > +  invert_quantize_dither_factors_MLF,
> > +  quantize_dither_factors_MLF,
> > +  quantize_factor_select_offset_MLF,
> > +  FF_ARRAY_ELEMS(quantize_intervals_MLF),
> > +  4, 12 },
> > +[MHF] = { quantize_intervals_MHF,
> > +  invert_quantize_dither_factors_MHF,
> > +  quantize_dither_factors_MHF,
> > +  quantize_factor_select_offset_MHF,
> > +  FF_ARRAY_ELEMS(quantize_intervals_MHF),
> > +  2, 6 },
> > +[HF]  = { quantize_intervals_HF,
> > +  invert_quantize_dither_factors_HF,
> > +  quantize_dither_factors_HF,
> > +  quantize_factor_select_offset_HF,
> > +  FF_ARRAY_ELEMS(quantize_intervals_HF),
> > +  3, 12 },
> > +};
> > +
>
> > +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
>
> [...]
> > +/*
> > + * 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 &
>
>
> > +}
> > +
> > +/*
> > + * 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 = >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
>
> also a fate test for this would be a good idea
>
>
No, I completely reject this idea.

I want to use for (int loops and have always wanted to use them.
Show me a comiler which we support which doesn't accept those. Then I'll
suggest we drop support for it.

Its time we bring the codebase to the 21st century and this is my main
issue - being unable to use for (int loops.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Michael Niedermayer]

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 *


> +int tables_size;
> +int32_t quantized_bits;
> +int32_t prediction_order;
> +} ConstTables;
> +
> +static ConstTables tables[NB_SUBBANDS] = {
> +[LF]  = { quantize_intervals_LF,
> +  invert_quantize_dither_factors_LF,
> +  quantize_dither_factors_LF,
> +  quantize_factor_select_offset_LF,
> +  FF_ARRAY_ELEMS(quantize_intervals_LF),
> +  7, 24 },
> +[MLF] = { quantize_intervals_MLF,
> +  invert_quantize_dither_factors_MLF,
> +  quantize_dither_factors_MLF,
> +  quantize_factor_select_offset_MLF,
> +  FF_ARRAY_ELEMS(quantize_intervals_MLF),
> +  4, 12 },
> +[MHF] = { quantize_intervals_MHF,
> +  invert_quantize_dither_factors_MHF,
> +  quantize_dither_factors_MHF,
> +  quantize_factor_select_offset_MHF,
> +  FF_ARRAY_ELEMS(quantize_intervals_MHF),
> +  2, 6 },
> +[HF]  = { quantize_intervals_HF,
> +  invert_quantize_dither_factors_HF,
> +  quantize_dither_factors_HF,
> +  quantize_factor_select_offset_HF,
> +  FF_ARRAY_ELEMS(quantize_intervals_HF),
> +  3, 12 },
> +};
> +

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

[...]
> +/*
> + * 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 &


> +}
> +
> +/*
> + * 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 = >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

also a fate test for this would be a good idea

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The real ebay dictionary, page 1
"Used only once"- "Some unspecified defect prevented a second use"
"In good condition" - "Can be repaird by experienced expert"
"As is" - "You wouldnt want it even if you were payed for it, if you knew ..."


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Aurelien Jacobs]

On Wed, Nov 08, 2017 at 10:54:18AM +0100, Paul B Mahol wrote:
> On 11/7/17, Aurelien Jacobs  wrote:
> > The encoder was reverse engineered from binary library and from
> > EP0398973B1 patent (long expired).
> > The decoder was simply deduced from the encoder.
> > ---
> >  doc/general.texi|   2 +
> >  libavcodec/Makefile |   2 +
> >  libavcodec/allcodecs.c  |   1 +
> >  libavcodec/aptx.c   | 826
> > 
> >  libavcodec/avcodec.h|   1 +
> >  libavcodec/codec_desc.c |   7 +
> >  6 files changed, 839 insertions(+)
> >  create mode 100644 libavcodec/aptx.c
> >
> 
> [...]
> 
> > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > index c4134424f0..36a99f4162 100644
> > --- a/libavcodec/avcodec.h
> > +++ b/libavcodec/avcodec.h
> > @@ -614,6 +614,7 @@ enum AVCodecID {
> >  AV_CODEC_ID_PAF_AUDIO,
> >  AV_CODEC_ID_ON2AVC,
> >  AV_CODEC_ID_DSS_SP,
> > +AV_CODEC_ID_APTX,
> 
> Wrong place, it should be put bellow DOLBY_E.

Humm... Not sure why there are 2 separate lists for audio codecs but OK,
here is the updated patch.
>From 3d3a1afe678edfbd3b00079b59b84add3461a7b0 Mon Sep 17 00:00:00 2001
From: Aurelien Jacobs 
Date: Thu, 31 Aug 2017 20:12:54 +0200
Subject: [PATCH 1/2] aptx: implement the aptX bluetooth codec

The encoder was reverse engineered from binary library and from
EP0398973B1 patent (long expired).
The decoder was simply deduced from the encoder.
---
 doc/general.texi|   2 +
 libavcodec/Makefile |   2 +
 libavcodec/allcodecs.c  |   1 +
 libavcodec/aptx.c   | 826 
 libavcodec/avcodec.h|   1 +
 libavcodec/codec_desc.c |   7 +
 6 files changed, 839 insertions(+)
 create mode 100644 libavcodec/aptx.c

diff --git a/doc/general.texi b/doc/general.texi
index 9e6ae13435..4a89531c47 100644
--- a/doc/general.texi
+++ b/doc/general.texi
@@ -991,6 +991,8 @@ following image formats are supported:
 @item Amazing Studio PAF Audio @tab @tab  X
 @item Apple lossless audio   @tab  X  @tab  X
 @tab QuickTime fourcc 'alac'
+@item aptX   @tab  X  @tab  X
+@tab Used in Bluetooth A2DP
 @item ATRAC1 @tab @tab  X
 @item ATRAC3 @tab @tab  X
 @item ATRAC3+@tab @tab  X
diff --git a/libavcodec/Makefile b/libavcodec/Makefile
index 3a33361f33..25706a263d 100644
--- a/libavcodec/Makefile
+++ b/libavcodec/Makefile
@@ -188,6 +188,8 @@ OBJS-$(CONFIG_AMV_ENCODER) += mjpegenc.o mjpegenc_common.o \
 OBJS-$(CONFIG_ANM_DECODER) += anm.o
 OBJS-$(CONFIG_ANSI_DECODER)+= ansi.o cga_data.o
 OBJS-$(CONFIG_APE_DECODER) += apedec.o
+OBJS-$(CONFIG_APTX_DECODER)+= aptx.o
+OBJS-$(CONFIG_APTX_ENCODER)+= aptx.o
 OBJS-$(CONFIG_APNG_DECODER)+= png.o pngdec.o pngdsp.o
 OBJS-$(CONFIG_APNG_ENCODER)+= png.o pngenc.o
 OBJS-$(CONFIG_SSA_DECODER) += assdec.o ass.o
diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
index 98655ddd7c..61abe9939c 100644
--- a/libavcodec/allcodecs.c
+++ b/libavcodec/allcodecs.c
@@ -406,6 +406,7 @@ static void register_all(void)
 REGISTER_DECODER(AMRNB, amrnb);
 REGISTER_DECODER(AMRWB, amrwb);
 REGISTER_DECODER(APE,   ape);
+REGISTER_ENCDEC (APTX,  aptx);
 REGISTER_DECODER(ATRAC1,atrac1);
 REGISTER_DECODER(ATRAC3,atrac3);
 REGISTER_DECODER(ATRAC3AL,  atrac3al);
diff --git a/libavcodec/aptx.c b/libavcodec/aptx.c
new file mode 100644
index 00..b2879ea370
--- /dev/null
+++ b/libavcodec/aptx.c
@@ -0,0 +1,826 @@
+/*
+ * Audio Processing Technology codec for Bluetooth (aptX)
+ *
+ * Copyright (C) 2017  Aurelien Jacobs 
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include "libavutil/intreadwrite.h"
+#include "avcodec.h"
+#include "internal.h"
+#include "mathops.h"
+
+
+enum channels {
+LEFT,
+RIGHT,
+NB_CHANNELS
+};
+
+enum subbands {
+LF,  // Low Frequency (0-5.5 kHz)
+MLF, // Medium-Low Frequency (5.5-11kHz)
+MHF, // Medium-High Frequency (11-16.5kHz)
+HF,  // High 

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

[Paul B Mahol]

On 11/7/17, Aurelien Jacobs  wrote:
> The encoder was reverse engineered from binary library and from
> EP0398973B1 patent (long expired).
> The decoder was simply deduced from the encoder.
> ---
>  doc/general.texi|   2 +
>  libavcodec/Makefile |   2 +
>  libavcodec/allcodecs.c  |   1 +
>  libavcodec/aptx.c   | 826
> 
>  libavcodec/avcodec.h|   1 +
>  libavcodec/codec_desc.c |   7 +
>  6 files changed, 839 insertions(+)
>  create mode 100644 libavcodec/aptx.c
>

[...]

> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index c4134424f0..36a99f4162 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -614,6 +614,7 @@ enum AVCodecID {
>  AV_CODEC_ID_PAF_AUDIO,
>  AV_CODEC_ID_ON2AVC,
>  AV_CODEC_ID_DSS_SP,
> +AV_CODEC_ID_APTX,

Wrong place, it should be put bellow DOLBY_E.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Rostislav Pehlivanov]

On 7 November 2017 at 22:26, Aurelien Jacobs  wrote:

> The encoder was reverse engineered from binary library and from
> EP0398973B1 patent (long expired).
> The decoder was simply deduced from the encoder.
> ---
>  doc/general.texi|   2 +
>  libavcodec/Makefile |   2 +
>  libavcodec/allcodecs.c  |   1 +
>  libavcodec/aptx.c   | 826 ++
> ++
>  libavcodec/avcodec.h|   1 +
>  libavcodec/codec_desc.c |   7 +
>  6 files changed, 839 insertions(+)
>  create mode 100644 libavcodec/aptx.c
>
>
LGTM, will apply tomorrow unless there are any complains



interval *= -(sample_difference < 0) | 1;
>

Neat, I'll keep this trick in mind
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Aurelien Jacobs]

The encoder was reverse engineered from binary library and from
EP0398973B1 patent (long expired).
The decoder was simply deduced from the encoder.
---
 doc/general.texi|   2 +
 libavcodec/Makefile |   2 +
 libavcodec/allcodecs.c  |   1 +
 libavcodec/aptx.c   | 826 
 libavcodec/avcodec.h|   1 +
 libavcodec/codec_desc.c |   7 +
 6 files changed, 839 insertions(+)
 create mode 100644 libavcodec/aptx.c

diff --git a/doc/general.texi b/doc/general.texi
index 9e6ae13435..4a89531c47 100644
--- a/doc/general.texi
+++ b/doc/general.texi
@@ -991,6 +991,8 @@ following image formats are supported:
 @item Amazing Studio PAF Audio @tab @tab  X
 @item Apple lossless audio   @tab  X  @tab  X
 @tab QuickTime fourcc 'alac'
+@item aptX   @tab  X  @tab  X
+@tab Used in Bluetooth A2DP
 @item ATRAC1 @tab @tab  X
 @item ATRAC3 @tab @tab  X
 @item ATRAC3+@tab @tab  X
diff --git a/libavcodec/Makefile b/libavcodec/Makefile
index 3a33361f33..25706a263d 100644
--- a/libavcodec/Makefile
+++ b/libavcodec/Makefile
@@ -188,6 +188,8 @@ OBJS-$(CONFIG_AMV_ENCODER) += mjpegenc.o 
mjpegenc_common.o \
 OBJS-$(CONFIG_ANM_DECODER) += anm.o
 OBJS-$(CONFIG_ANSI_DECODER)+= ansi.o cga_data.o
 OBJS-$(CONFIG_APE_DECODER) += apedec.o
+OBJS-$(CONFIG_APTX_DECODER)+= aptx.o
+OBJS-$(CONFIG_APTX_ENCODER)+= aptx.o
 OBJS-$(CONFIG_APNG_DECODER)+= png.o pngdec.o pngdsp.o
 OBJS-$(CONFIG_APNG_ENCODER)+= png.o pngenc.o
 OBJS-$(CONFIG_SSA_DECODER) += assdec.o ass.o
diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
index 98655ddd7c..61abe9939c 100644
--- a/libavcodec/allcodecs.c
+++ b/libavcodec/allcodecs.c
@@ -406,6 +406,7 @@ static void register_all(void)
 REGISTER_DECODER(AMRNB, amrnb);
 REGISTER_DECODER(AMRWB, amrwb);
 REGISTER_DECODER(APE,   ape);
+REGISTER_ENCDEC (APTX,  aptx);
 REGISTER_DECODER(ATRAC1,atrac1);
 REGISTER_DECODER(ATRAC3,atrac3);
 REGISTER_DECODER(ATRAC3AL,  atrac3al);
diff --git a/libavcodec/aptx.c b/libavcodec/aptx.c
new file mode 100644
index 00..b2879ea370
--- /dev/null
+++ b/libavcodec/aptx.c
@@ -0,0 +1,826 @@
+/*
+ * Audio Processing Technology codec for Bluetooth (aptX)
+ *
+ * Copyright (C) 2017  Aurelien Jacobs 
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include "libavutil/intreadwrite.h"
+#include "avcodec.h"
+#include "internal.h"
+#include "mathops.h"
+
+
+enum channels {
+LEFT,
+RIGHT,
+NB_CHANNELS
+};
+
+enum subbands {
+LF,  // Low Frequency (0-5.5 kHz)
+MLF, // Medium-Low Frequency (5.5-11kHz)
+MHF, // Medium-High Frequency (11-16.5kHz)
+HF,  // High Frequency (16.5-22kHz)
+NB_SUBBANDS
+};
+
+#define NB_FILTERS 2
+#define FILTER_TAPS 16
+
+typedef struct {
+int pos;
+int32_t buffer[2*FILTER_TAPS];
+} FilterSignal;
+
+typedef struct {
+FilterSignal outer_filter_signal[NB_FILTERS];
+FilterSignal inner_filter_signal[NB_FILTERS][NB_FILTERS];
+} QMFAnalysis;
+
+typedef struct {
+int32_t quantized_sample;
+int32_t quantized_sample_parity_change;
+int32_t error;
+} Quantize;
+
+typedef struct {
+int32_t quantization_factor;
+int32_t factor_select;
+int32_t reconstructed_difference;
+} InvertQuantize;
+
+typedef struct {
+int32_t prev_sign[2];
+int32_t s_weight[2];
+int32_t d_weight[24];
+int32_t pos;
+int32_t reconstructed_differences[48];
+int32_t previous_reconstructed_sample;
+int32_t predicted_difference;
+int32_t predicted_sample;
+} Prediction;
+
+typedef struct {
+int32_t codeword_history;
+int32_t dither_parity;
+int32_t dither[NB_SUBBANDS];
+
+QMFAnalysis qmf;
+Quantize quantize[NB_SUBBANDS];
+InvertQuantize invert_quantize[NB_SUBBANDS];
+Prediction prediction[NB_SUBBANDS];
+} Channel;
+
+typedef struct {
+int32_t sync_idx;
+Channel channels[NB_CHANNELS];
+} AptXContext;
+
+
+static const int32_t quantize_intervals_LF[65] = {
+  -9948,9948,   29860,   

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

[Aurelien Jacobs]

On Mon, Nov 06, 2017 at 12:30:02AM +, Rostislav Pehlivanov wrote:
> On 5 November 2017 at 23:39, Aurelien Jacobs  wrote:
> 
> > The encoder was reverse engineered from binary library and from
> > EP0398973B1 patent (long expired).
> > The decoder was simply deduced from the encoder.
> > ---
> >  doc/general.texi|   2 +
> >  libavcodec/Makefile |   2 +
> >  libavcodec/allcodecs.c  |   1 +
> >  libavcodec/aptx.c   | 854 ++
> > ++
> >  libavcodec/avcodec.h|   1 +
> >  libavcodec/codec_desc.c |   7 +
> >  6 files changed, 867 insertions(+)
> >  create mode 100644 libavcodec/aptx.c
> >
> 
> Very nice job

Thanks !

> > +
> > +static const int32_t quantization_factors[32] = {
> > +2048 << 11,
> > +2093 << 11,
> > +2139 << 11,
> > +2186 << 11,
> > +2233 << 11,
> > +2282 << 11,
> > +2332 << 11,
> > +2383 << 11,
> > +2435 << 11,
> > +2489 << 11,
> > +2543 << 11,
> > +2599 << 11,
> > +2656 << 11,
> > +2714 << 11,
> > +2774 << 11,
> > +2834 << 11,
> > +2896 << 11,
> > +2960 << 11,
> > +3025 << 11,
> > +3091 << 11,
> > +3158 << 11,
> > +3228 << 11,
> > +3298 << 11,
> > +3371 << 11,
> > +3444 << 11,
> > +3520 << 11,
> > +3597 << 11,
> > +3676 << 11,
> > +3756 << 11,
> > +3838 << 11,
> > +3922 << 11,
> > +4008 << 11,
> > +};
> >
> 
> 
> First of all, please put all numbers on the same line.
> Second of all, its pointless to do a shift here, either change the numbers
> or better yet, since you already do a shift down:
> 
> 
> 
> > +/* update quantization factor */
> > +idx = (invert_quantize->factor_select & 0xFF) >> 3;
> > +shift -= invert_quantize->factor_select >> 8;
> > +invert_quantize->quantization_factor = quantization_factors[idx] >>
> > shift;
> > +}
> >
> 
> 
> Which would be equivalent to:
> 
>  idx = (invert_quantize->factor_select & 0xFF) >> 3;
> > shift -= invert_quantize->factor_select >> 8;
> > invert_quantize->quantization_factor = (quantization_factors[idx] << 11)
> > >> shift;
> >
> 
> The compiler ought to be smart enough to handle that as a single operation.

I don't think the compiler will optimze as much, but that's a trivial
operation, and there is no mesurable difference, so indeed, I moved
the shift out of the table and cleaned up the formatting.

> > +static int##size##_t rshift##size(int##size##_t value, int shift)
> >  \
> >
> > +static int##size##_t rshift##size##_clip24(int##size##_t value, int
> > shift)\
> >
> > +static void aptx_update_codeword_history(Channel *channel)
> >
> > +static void aptx_generate_dither(Channel *channel)
> >
> >
> +static void aptx_qmf_filter_signal_push(FilterSignal *signal, int32_t
> > sample)
> >
> > +static int32_t aptx_qmf_convolution(FilterSignal *signal,
> > +const int32_t coeffs[FILTER_TAPS],
> > +int shift)
> >
> > +static void aptx_qmf_polyphase_analysis(FilterSignal signal[NB_FILTERS],
> > +const int32_t
> > coeffs[NB_FILTERS][FILTER_TAPS],
> > +int shift,
> > +int32_t samples[NB_FILTERS],
> > +int32_t *low_subband_output,
> > +int32_t *high_subband_output)
> > +
> >
> 
> 
> Add an inline flag to small functions like these. Won't make a difference
> but eh.

Done.

> > +
> > +static void aptx_quantise_difference(Quantize *quantize,
> > + int32_t sample_difference,
> > + int32_t dither,
> > + int32_t quantization_factor,
> > + ConstTables *tables)
> >
> 
> English spelling of quantize? I prefer quantize since that's how its
> spelled throughout the entire codebase.

Good catch. Fixed.

> > +{
> > +const int32_t *intervals = tables->quantize_intervals;
> > +int32_t quantized_sample, dithered_sample, parity_change;
> > +int32_t d, mean, interval, inv;
> > +int64_t error;
> > +
> > +quantized_sample = aptx_bin_search(FFABS(sample_difference) >> 4,
> > +   quantization_factor,
> > +   intervals, tables->tables_size);
> > +
> > +d = rshift32_clip24(MULH(dither, dither), 7) - (1 << 23);
> > +d = rshift64(MUL64(d, 
> > tables->quantize_dither_factors[quantized_sample]),
> > 23);
> > +
> > +intervals += quantized_sample;
> > +mean = (intervals[1] + intervals[0]) / 2;
> > +interval = intervals[1] - intervals[0];
> > +if (sample_difference < 0)
> > +interval = -interval;
> >
> 
> 
> Can be simplified to:
> interval *= 1 - 2*(sample_difference < 0);
> or
> interval *= sample_difference < 0 ? -1 : +1;

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

[Rostislav Pehlivanov]

On 6 November 2017 at 00:30, Rostislav Pehlivanov 
wrote:

>
>
> On 5 November 2017 at 23:39, Aurelien Jacobs  wrote:
>
>> +*ptr++ = samples[RIGHT][i] >> 8;
>>
>
> How horrible, don't interleave the samples, leave them as planar.
> Change the output format in AVCodec and use
>
> AV_WN16(frame->data[][], samples[][] >>
> 8);
>
> To write the data. No point to convert to interleaved when the data's
> planar.
>
>
>>
>> +
>> +*got_frame_ptr = 1;
>> +return avpkt->size - len;
>>
>
> ?
> Decoders should return the number of bytes read from the packet (if
> convenient) or the packet size, not some random digit.
>
>
>> +}
>> +
>> +static int aptx_encode_frame(AVCodecContext *avctx, AVPacket *avpkt,
>> + const AVFrame *frame, int *got_packet_ptr)
>> +{
>> +AptXContext *s = avctx->priv_data;
>> +int16_t *ptr = (int16_t *)frame->data[0];
>> +int32_t samples[NB_CHANNELS][4];
>> +int ret;
>> +
>> +/* input must contain a multiple of 4 samples */
>> +if (frame->nb_samples & 3 || frame->nb_samples == 0) {
>> +av_log(avctx, AV_LOG_ERROR, "Frame must have a multiple of 4
>> samples\n");
>> +return 0;
>> +}
>> +
>> +if ((ret = ff_alloc_packet2(avctx, avpkt, frame->nb_samples, 0)) < 0)
>> +return ret;
>> +
>> +for (int pos = 0; pos < frame->nb_samples; pos += 4) {
>> +for (int i = 0; i < 4; i++) {
>> +/* convert 16 bits interleaved input to 24 bits planar
>> samples */
>> +samples[LEFT][i]  = ptr[LEFT ] << 8;
>> +samples[RIGHT][i] = ptr[RIGHT] << 8;
>> +ptr += NB_CHANNELS;
>> +}
>>
>
> Once again use planar sample fmt and then 
> AV_RN16(>data[][])
> to read them.
>
>
>> +
>> +aptx_encode_samples(s, samples, avpkt->data + pos);
>> +}
>> +
>> +*got_packet_ptr = 1;
>> +return 0;
>> +}
>> +
>> +
>> +#if CONFIG_APTX_DECODER
>> +AVCodec ff_aptx_decoder = {
>> +.name  = "aptx",
>> +.long_name = NULL_IF_CONFIG_SMALL("aptX (Audio
>> Processing Technology for Bluetooth)"),
>> +.type  = AVMEDIA_TYPE_AUDIO,
>> +.id= AV_CODEC_ID_APTX,
>> +.priv_data_size= sizeof(AptXContext),
>> +.init  = aptx_init,
>> +.decode= aptx_decode_frame,
>> +.capabilities  = AV_CODEC_CAP_DR1,
>> +.channel_layouts   = (const uint64_t[]) { AV_CH_LAYOUT_STEREO,
>> 0},
>> +.sample_fmts   = (const enum AVSampleFormat[]) {
>> AV_SAMPLE_FMT_S16,
>>
>
> Change to AV_SAMPLE_FMT_S16P
>
>
>> +
>>  AV_SAMPLE_FMT_NONE },
>> +};
>> +#endif
>> +
>> +#if CONFIG_APTX_ENCODER
>> +AVCodec ff_aptx_encoder = {
>> +.name  = "aptx",
>> +.long_name = NULL_IF_CONFIG_SMALL("aptX (Audio
>> Processing Technology for Bluetooth)"),
>> +.type  = AVMEDIA_TYPE_AUDIO,
>> +.id= AV_CODEC_ID_APTX,
>> +.priv_data_size= sizeof(AptXContext),
>> +.init  = aptx_init,
>> +.encode2   = aptx_encode_frame,
>> +.capabilities  = AV_CODEC_CAP_VARIABLE_FRAME_SIZE,
>> +.channel_layouts   = (const uint64_t[]) { AV_CH_LAYOUT_STEREO,
>> 0},
>> +.sample_fmts   = (const enum AVSampleFormat[]) {
>> AV_SAMPLE_FMT_S16,
>>
>
> And here to AV_SAMPLE_FMT_S16P
>
>

Actually no, the sample format should be AV_SAMPLE_FMT_S32P

The code internally seems to use 24bit precision (for some retarded
reason), so NIHing format conversion to 16 bits is pointless as you lose
precision.
Just use a 32 bit sample format and shift down on the encoder side by 8
bits to get normalized 24 bit samples and shift up on the decoder side by 8
to get 32 bit samples.

It would have been better if the encoder and decoder were float tbh but oh
well, the people writing the code were paid to make it run on uselessly low
speed devices.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Rostislav Pehlivanov]

On 5 November 2017 at 23:39, Aurelien Jacobs  wrote:

> The encoder was reverse engineered from binary library and from
> EP0398973B1 patent (long expired).
> The decoder was simply deduced from the encoder.
> ---
>  doc/general.texi|   2 +
>  libavcodec/Makefile |   2 +
>  libavcodec/allcodecs.c  |   1 +
>  libavcodec/aptx.c   | 854 ++
> ++
>  libavcodec/avcodec.h|   1 +
>  libavcodec/codec_desc.c |   7 +
>  6 files changed, 867 insertions(+)
>  create mode 100644 libavcodec/aptx.c
>

Very nice job


>
>
> +
> +static const int32_t quantization_factors[32] = {
> +2048 << 11,
> +2093 << 11,
> +2139 << 11,
> +2186 << 11,
> +2233 << 11,
> +2282 << 11,
> +2332 << 11,
> +2383 << 11,
> +2435 << 11,
> +2489 << 11,
> +2543 << 11,
> +2599 << 11,
> +2656 << 11,
> +2714 << 11,
> +2774 << 11,
> +2834 << 11,
> +2896 << 11,
> +2960 << 11,
> +3025 << 11,
> +3091 << 11,
> +3158 << 11,
> +3228 << 11,
> +3298 << 11,
> +3371 << 11,
> +3444 << 11,
> +3520 << 11,
> +3597 << 11,
> +3676 << 11,
> +3756 << 11,
> +3838 << 11,
> +3922 << 11,
> +4008 << 11,
> +};
>


First of all, please put all numbers on the same line.
Second of all, its pointless to do a shift here, either change the numbers
or better yet, since you already do a shift down:



> +/* update quantization factor */
> +idx = (invert_quantize->factor_select & 0xFF) >> 3;
> +shift -= invert_quantize->factor_select >> 8;
> +invert_quantize->quantization_factor = quantization_factors[idx] >>
> shift;
> +}
>


Which would be equivalent to:

 idx = (invert_quantize->factor_select & 0xFF) >> 3;
> shift -= invert_quantize->factor_select >> 8;
> invert_quantize->quantization_factor = (quantization_factors[idx] << 11)
> >> shift;
>

The compiler ought to be smart enough to handle that as a single operation.





>
> +static int##size##_t rshift##size(int##size##_t value, int shift)
>  \
>
> +static int##size##_t rshift##size##_clip24(int##size##_t value, int
> shift)\
>
> +static void aptx_update_codeword_history(Channel *channel)
>
> +static void aptx_generate_dither(Channel *channel)
>
>
+static void aptx_qmf_filter_signal_push(FilterSignal *signal, int32_t
> sample)
>
> +static int32_t aptx_qmf_convolution(FilterSignal *signal,
> +const int32_t coeffs[FILTER_TAPS],
> +int shift)
>
> +static void aptx_qmf_polyphase_analysis(FilterSignal signal[NB_FILTERS],
> +const int32_t
> coeffs[NB_FILTERS][FILTER_TAPS],
> +int shift,
> +int32_t samples[NB_FILTERS],
> +int32_t *low_subband_output,
> +int32_t *high_subband_output)
> +
>


Add an inline flag to small functions like these. Won't make a difference
but eh.




> +
> +static void aptx_quantise_difference(Quantize *quantize,
> + int32_t sample_difference,
> + int32_t dither,
> + int32_t quantization_factor,
> + ConstTables *tables)
>

English spelling of quantize? I prefer quantize since that's how its
spelled throughout the entire codebase.


> +{
> +const int32_t *intervals = tables->quantize_intervals;
> +int32_t quantized_sample, dithered_sample, parity_change;
> +int32_t d, mean, interval, inv;
> +int64_t error;
> +
> +quantized_sample = aptx_bin_search(FFABS(sample_difference) >> 4,
> +   quantization_factor,
> +   intervals, tables->tables_size);
> +
> +d = rshift32_clip24(MULH(dither, dither), 7) - (1 << 23);
> +d = rshift64(MUL64(d, tables->quantize_dither_factors[quantized_sample]),
> 23);
> +
> +intervals += quantized_sample;
> +mean = (intervals[1] + intervals[0]) / 2;
> +interval = intervals[1] - intervals[0];
> +if (sample_difference < 0)
> +interval = -interval;
>


Can be simplified to:
interval *= 1 - 2*(sample_difference < 0);
or
interval *= sample_difference < 0 ? -1 : +1;



> +
> +dithered_sample = rshift64_clip24(MUL64(dither, interval) +
> ((int64_t)(mean + d) << 32), 32);
> +error = ((int64_t)FFABS(sample_difference) << 20) -
> MUL64(dithered_sample, quantization_factor);
> +quantize->error = FFABS(rshift64(error, 23));
> +
> +parity_change = quantized_sample;
> +if (error < 0)  quantized_sample--;
> +elseparity_change--;
>
+
>

Coding style issues, seen this in much of the code, must be
if (something)
stuff;
else
other_stuff;



> +inv = -(sample_difference < 0);
> +quantize->quantized_sample 

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

[Aurelien Jacobs]

The encoder was reverse engineered from binary library and from
EP0398973B1 patent (long expired).
The decoder was simply deduced from the encoder.
---
 doc/general.texi|   2 +
 libavcodec/Makefile |   2 +
 libavcodec/allcodecs.c  |   1 +
 libavcodec/aptx.c   | 854 
 libavcodec/avcodec.h|   1 +
 libavcodec/codec_desc.c |   7 +
 6 files changed, 867 insertions(+)
 create mode 100644 libavcodec/aptx.c

diff --git a/doc/general.texi b/doc/general.texi
index 9e6ae13435..4a89531c47 100644
--- a/doc/general.texi
+++ b/doc/general.texi
@@ -991,6 +991,8 @@ following image formats are supported:
 @item Amazing Studio PAF Audio @tab @tab  X
 @item Apple lossless audio   @tab  X  @tab  X
 @tab QuickTime fourcc 'alac'
+@item aptX   @tab  X  @tab  X
+@tab Used in Bluetooth A2DP
 @item ATRAC1 @tab @tab  X
 @item ATRAC3 @tab @tab  X
 @item ATRAC3+@tab @tab  X
diff --git a/libavcodec/Makefile b/libavcodec/Makefile
index 3a33361f33..25706a263d 100644
--- a/libavcodec/Makefile
+++ b/libavcodec/Makefile
@@ -188,6 +188,8 @@ OBJS-$(CONFIG_AMV_ENCODER) += mjpegenc.o 
mjpegenc_common.o \
 OBJS-$(CONFIG_ANM_DECODER) += anm.o
 OBJS-$(CONFIG_ANSI_DECODER)+= ansi.o cga_data.o
 OBJS-$(CONFIG_APE_DECODER) += apedec.o
+OBJS-$(CONFIG_APTX_DECODER)+= aptx.o
+OBJS-$(CONFIG_APTX_ENCODER)+= aptx.o
 OBJS-$(CONFIG_APNG_DECODER)+= png.o pngdec.o pngdsp.o
 OBJS-$(CONFIG_APNG_ENCODER)+= png.o pngenc.o
 OBJS-$(CONFIG_SSA_DECODER) += assdec.o ass.o
diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
index 98655ddd7c..61abe9939c 100644
--- a/libavcodec/allcodecs.c
+++ b/libavcodec/allcodecs.c
@@ -406,6 +406,7 @@ static void register_all(void)
 REGISTER_DECODER(AMRNB, amrnb);
 REGISTER_DECODER(AMRWB, amrwb);
 REGISTER_DECODER(APE,   ape);
+REGISTER_ENCDEC (APTX,  aptx);
 REGISTER_DECODER(ATRAC1,atrac1);
 REGISTER_DECODER(ATRAC3,atrac3);
 REGISTER_DECODER(ATRAC3AL,  atrac3al);
diff --git a/libavcodec/aptx.c b/libavcodec/aptx.c
new file mode 100644
index 00..4ecb6bb231
--- /dev/null
+++ b/libavcodec/aptx.c
@@ -0,0 +1,854 @@
+/*
+ * Audio Processing Technology codec for Bluetooth (aptX)
+ *
+ * Copyright (C) 2017  Aurelien Jacobs 
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include "libavutil/intreadwrite.h"
+#include "avcodec.h"
+#include "internal.h"
+#include "mathops.h"
+
+
+enum channels {
+LEFT,
+RIGHT,
+NB_CHANNELS
+};
+
+enum subbands {
+LF,  // Low Frequency (0-5.5 kHz)
+MLF, // Medium-Low Frequency (5.5-11kHz)
+MHF, // Medium-High Frequency (11-16.5kHz)
+HF,  // High Frequency (16.5-22kHz)
+NB_SUBBANDS
+};
+
+#define NB_FILTERS 2
+#define FILTER_TAPS 16
+
+typedef struct {
+int pos;
+int32_t buffer[2*FILTER_TAPS];
+} FilterSignal;
+
+typedef struct {
+FilterSignal outer_filter_signal[NB_FILTERS];
+FilterSignal inner_filter_signal[NB_FILTERS][NB_FILTERS];
+} QMFAnalysis;
+
+typedef struct {
+int32_t quantized_sample;
+int32_t quantized_sample_parity_change;
+int32_t error;
+} Quantize;
+
+typedef struct {
+int32_t quantization_factor;
+int32_t factor_select;
+int32_t reconstructed_difference;
+} InvertQuantize;
+
+typedef struct {
+int32_t prev_sign[2];
+int32_t s_weight[2];
+int32_t d_weight[24];
+int32_t pos;
+int32_t reconstructed_differences[48];
+int32_t previous_reconstructed_sample;
+int32_t predicted_difference;
+int32_t predicted_sample;
+} Prediction;
+
+typedef struct {
+int32_t codeword_history;
+int32_t dither_parity;
+int32_t dither[NB_SUBBANDS];
+
+QMFAnalysis qmf;
+Quantize quantize[NB_SUBBANDS];
+InvertQuantize invert_quantize[NB_SUBBANDS];
+Prediction prediction[NB_SUBBANDS];
+} Channel;
+
+typedef struct {
+int32_t sync_idx;
+Channel channels[NB_CHANNELS];
+} AptXContext;
+
+
+static const int32_t quantize_intervals_LF[65] = {
+  -9948,9948,   29860,