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

Rostislav Pehlivanov atomnuker at gmail.com
Mon Nov 6 02:30:02 EET 2017


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


>
>
> +
> +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--;
> +    else            parity_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               = 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).
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).




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


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


> +
>


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


>
> +
> +    *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(&frame->data[<channel>][<sample_offset>]) 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


Apart from that, doesn't look too bad.


More information about the ffmpeg-devel mailing list