[FFmpeg-devel] [PATCH 1/2] aptx: implement the aptX bluetooth codec
Aurelien Jacobs
aurel at gnuage.org
Wed Nov 8 00:16:48 EET 2017
On Mon, Nov 06, 2017 at 12:30:02AM +0000, Rostislav Pehlivanov wrote:
> On 5 November 2017 at 23:39, Aurelien Jacobs <aurel at gnuage.org> 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;
I prefer:
interval *= -(sample_difference < 0) | 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--;
> > + else parity_change--;
> >
> +
> >
>
> Coding style issues, seen this in much of the code, must be
> if (something)
> stuff;
> else
> other_stuff;
OK.
> > + inv = -(sample_difference < 0);
> > + quantize->quantized_sample = quantized_sample ^ inv;
> > + quantize->quantized_sample_parity_change = parity_change ^ inv;
>
>
>
> That's nasty. You invert the sign and at the same time you increment by one
> and you decrement (before that).
That's indeed nasty.
> I'm not quite sure but I think this might be an error. This is in the
> encoder section too, so I'm wondering, is this what the spec says to do
> (most codecs only specify the decoder except for old audio/speech ones like
> this one).
It would be awsome to have the spec, but unfortunatly the best we have
is reverse engineering...
But I double checked what the binary library does and I can assure you
that this is not an error.
My encoder produce a bit exact identical output as the binary library.
> +}
> > +
> > +static int aptx_decode_frame(AVCodecContext *avctx, void *data,
> > + int *got_frame_ptr, AVPacket *avpkt)
> > +{
> > + AptXContext *s = avctx->priv_data;
> > + AVFrame *frame = data;
> > + const uint8_t *buf = avpkt->data;
> > + int len = avpkt->size;
> > + int16_t *ptr;
> > + int ret;
> > +
> > + if (len < 4) {
> > + av_log(avctx, AV_LOG_ERROR, "Packet is too small\n");
> > + return AVERROR_INVALIDDATA;
> > + }
> > +
> > + /* get output buffer */
> > + frame->nb_samples = len & ~3;
> > + if ((ret = ff_get_buffer(avctx, frame, 0)) < 0)
> > + return ret;
> > + ptr = (int16_t *)frame->data[0];
> >
>
> No need to cast, nor to define the ptr as int16_t
OK.
> > +
> > + while (len >= 4) {
> > + int32_t samples[NB_CHANNELS][4];
> > +
> > + if (aptx_decode_samples(s, buf, samples))
> > + av_log(avctx, AV_LOG_ERROR, "Synchronization error\n");
> >
>
> Return AVERROR_INVALIDDDATA?
That's probably better indeed, and that will allow a bluetooth manager
to detect stream corruption and to re-initialize the radio link.
> > +
> >
>
>
> > + for (int i = 0; i < 4; i++) {
> > + /* convert 24 bits planar samples to 16 bits interleaved
> > output */
> > + *ptr++ = samples[LEFT ][i] >> 8;
> > + *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[<channel>][<sample>], samples[<channel>][<sample>] >>
> 8);
>
> To write the data. No point to convert to interleaved when the data's
> planar.
The data has to be shifted anyway, so the interleaving is done for free
at the same time. But I get it that it is not very elegant.
Changed to use native planar format.
> >
> > +
> > + *got_frame_ptr = 1;
> > + return avpkt->size - len;
> >
>
> ?
> Decoders should return the number of bytes read from the packet
That's exactly what it does. len represent the size of the input data
that has not yet been consumed by the decoder.
Anyway, I've simplified this while swtiching to native planar format.
> > +}
> > +
> > +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(&frame->data[<channel>][<sample_offset>]) to read them.
Done.
> > +
> > + 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
Actually changed to AV_SAMPLE_FMT_S32P as suggested.
> > +
> > 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
Same here.
> Apart from that, doesn't look too bad.
Thanks a lot for the review.
I will submit updated patch set.
More information about the ffmpeg-devel
mailing list