Return-Path: Date: Sun, 28 Oct 2012 01:19:48 +0300 From: Siarhei Siamashka To: =?UTF-8?B?RnLDqWTDqXJpYw==?= Dalleau Cc: linux-bluetooth@vger.kernel.org Subject: Re: [PATCH v3 04/10] sbc: Add msbc flag and generic C primitive Message-ID: <20121028011948.30f09efe@i7> In-Reply-To: <1351272036-4875-5-git-send-email-frederic.dalleau@linux.intel.com> References: <1351272036-4875-1-git-send-email-frederic.dalleau@linux.intel.com> <1351272036-4875-5-git-send-email-frederic.dalleau@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: On Fri, 26 Oct 2012 19:20:30 +0200 Frédéric Dalleau wrote: > --- > sbc/sbc.c | 27 +++++++++++++++------ > sbc/sbc.h | 3 +++ > sbc/sbc_primitives.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++-- > sbc/sbc_primitives.h | 2 ++ > 4 files changed, 88 insertions(+), 10 deletions(-) Maybe it would be a bit cleaner to split this a bit? I mean a separate patch for the part, which extends "sbc_encoder_process_input_s8" function to support 8 samples granularity for "nsamples" argument (down from 16 samples granularity for "nsamples" in the current code). > diff --git a/sbc/sbc.c b/sbc/sbc.c > index 9b0634d..4bc97fc 100644 > --- a/sbc/sbc.c > +++ b/sbc/sbc.c > @@ -52,6 +52,9 @@ > > #define SBC_SYNCWORD 0x9C > > +#define MSBC_SYNCWORD 0xAD > +#define MSBC_BLOCKS 15 > + > /* This structure contains an unpacked SBC frame. > Yes, there is probably quite some unused space herein */ > struct sbc_frame { > @@ -705,6 +708,10 @@ static int sbc_analyze_audio(struct sbc_encoder_state *state, > for (ch = 0; ch < frame->channels; ch++) { > x = &state->X[ch][state->position - 8 * state->increment + > frame->blocks * 8]; > + > + if (state->pending == state->position) > + x += 8; The use of both "state->pending" and "state->position" outside "sbc_encoder_process_input_s8" function looks a bit messy. It's kind of like exposing its internal implementation detail unnecessarily. > for (blk = 0; blk < frame->blocks; blk += state->increment) { > state->sbc_analyze_4b_8s( > state, x, > @@ -901,12 +908,12 @@ static ssize_t sbc_pack_frame(uint8_t *data, struct sbc_frame *frame, size_t len > } > } > > -static void sbc_encoder_init(struct sbc_encoder_state *state, > - const struct sbc_frame *frame) > +static void sbc_encoder_init(unsigned long flags, > + struct sbc_encoder_state *state, const struct sbc_frame *frame) > { > memset(&state->X, 0, sizeof(state->X)); > state->position = (SBC_X_BUFFER_SIZE - frame->subbands * 9) & ~7; > - state->increment = 4; > + state->increment = flags & SBC_MSBC ? 1 : 4; > > sbc_init_primitives(state); > } > @@ -920,6 +927,7 @@ struct sbc_priv { > > static void sbc_set_defaults(sbc_t *sbc, unsigned long flags) > { > + sbc->flags = flags; > sbc->frequency = SBC_FREQ_44100; > sbc->mode = SBC_MODE_STEREO; > sbc->subbands = SBC_SB_8; > @@ -1055,12 +1063,13 @@ SBC_EXPORT ssize_t sbc_encode(sbc_t *sbc, const void *input, size_t input_len, > priv->frame.subband_mode = sbc->subbands; > priv->frame.subbands = sbc->subbands ? 8 : 4; > priv->frame.block_mode = sbc->blocks; > - priv->frame.blocks = 4 + (sbc->blocks * 4); > + priv->frame.blocks = sbc->flags & SBC_MSBC ? > + MSBC_BLOCKS : 4 + (sbc->blocks * 4); > priv->frame.bitpool = sbc->bitpool; > priv->frame.codesize = sbc_get_codesize(sbc); > priv->frame.length = sbc_get_frame_length(sbc); > > - sbc_encoder_init(&priv->enc_state, &priv->frame); > + sbc_encoder_init(sbc->flags, &priv->enc_state, &priv->frame); > priv->init = 1; > } else if (priv->frame.bitpool != sbc->bitpool) { > priv->frame.length = sbc_get_frame_length(sbc); > @@ -1139,7 +1148,7 @@ SBC_EXPORT size_t sbc_get_frame_length(sbc_t *sbc) > return priv->frame.length; > > subbands = sbc->subbands ? 8 : 4; > - blocks = 4 + (sbc->blocks * 4); > + blocks = sbc->flags & SBC_MSBC ? MSBC_BLOCKS : 4 + (sbc->blocks * 4); > channels = sbc->mode == SBC_MODE_MONO ? 1 : 2; > joint = sbc->mode == SBC_MODE_JOINT_STEREO ? 1 : 0; > bitpool = sbc->bitpool; > @@ -1163,7 +1172,8 @@ SBC_EXPORT unsigned sbc_get_frame_duration(sbc_t *sbc) > priv = sbc->priv; > if (!priv->init) { > subbands = sbc->subbands ? 8 : 4; > - blocks = 4 + (sbc->blocks * 4); > + blocks = sbc->flags & SBC_MSBC ? > + MSBC_BLOCKS : 4 + (sbc->blocks * 4); > } else { > subbands = priv->frame.subbands; > blocks = priv->frame.blocks; > @@ -1200,7 +1210,8 @@ SBC_EXPORT size_t sbc_get_codesize(sbc_t *sbc) > priv = sbc->priv; > if (!priv->init) { > subbands = sbc->subbands ? 8 : 4; > - blocks = 4 + (sbc->blocks * 4); > + blocks = sbc->flags & SBC_MSBC ? > + MSBC_BLOCKS : 4 + (sbc->blocks * 4); > channels = sbc->mode == SBC_MODE_MONO ? 1 : 2; > } else { > subbands = priv->frame.subbands; > diff --git a/sbc/sbc.h b/sbc/sbc.h > index bbd45da..3511119 100644 > --- a/sbc/sbc.h > +++ b/sbc/sbc.h > @@ -64,6 +64,9 @@ extern "C" { > #define SBC_LE 0x00 > #define SBC_BE 0x01 > > +/* Additional features */ > +#define SBC_MSBC 0x01 > + > struct sbc_struct { > unsigned long flags; > > diff --git a/sbc/sbc_primitives.c b/sbc/sbc_primitives.c > index 7ba0589..47caf11 100644 > --- a/sbc/sbc_primitives.c > +++ b/sbc/sbc_primitives.c > @@ -209,6 +209,17 @@ static inline void sbc_analyze_4b_8s_simd(struct sbc_encoder_state *state, > sbc_analyze_eight_simd(x + 0, out, analysis_consts_fixed8_simd_even); > } > > +static inline void sbc_analyze_1b_8s_simd(struct sbc_encoder_state *state, > + int16_t *x, int32_t *out, int out_stride) > +{ > + if (state->odd) > + sbc_analyze_eight_simd(x, out, analysis_consts_fixed8_simd_odd); > + else > + sbc_analyze_eight_simd(x, out, analysis_consts_fixed8_simd_even); > + > + state->odd = !state->odd; > +} > + > static inline int16_t unaligned16_be(const uint8_t *ptr) > { > return (int16_t) ((ptr[0] << 8) | ptr[1]); > @@ -298,8 +309,25 @@ static SBC_ALWAYS_INLINE int sbc_encoder_process_input_s8_internal( > #define PCM(i) (big_endian ? \ > unaligned16_be(pcm + (i) * 2) : unaligned16_le(pcm + (i) * 2)) > > + if (state->pending >= 0) { > + state->pending = -1; > + nsamples -= 8; > + if (nchannels > 0) { > + int16_t *x = &X[0][position]; > + x[0] = PCM(0 + (15-8) * nchannels); > + x[2] = PCM(0 + (14-8) * nchannels); > + x[3] = PCM(0 + (8-8) * nchannels); > + x[4] = PCM(0 + (13-8) * nchannels); > + x[5] = PCM(0 + (9-8) * nchannels); > + x[6] = PCM(0 + (12-8) * nchannels); > + x[7] = PCM(0 + (10-8) * nchannels); > + x[8] = PCM(0 + (11-8) * nchannels); > + } Here it would be nice to have a comment in the code about why the same processing is skipped for nchannels > 1. And because the code is becoming even more complex, more detailed comments describing "sbc_encoder_process_input" would be a great addition (the usage constraints, the expected input, the expected output). > + pcm += 16 * nchannels; > + } > + > /* copy/permutate audio samples */ > - while ((nsamples -= 16) >= 0) { > + while (nsamples >= 16) { > position -= 16; > if (nchannels > 0) { > int16_t *x = &X[0][position]; > @@ -340,6 +368,33 @@ static SBC_ALWAYS_INLINE int sbc_encoder_process_input_s8_internal( > x[15] = PCM(1 + 2 * nchannels); > } > pcm += 32 * nchannels; > + nsamples -= 16; > + } > + > + if (nsamples == 8) { > + position -= 16; > + state->pending = position; If "position" was not decremented by 16 for 8 samples here, then you would not need to do if (state->pending == state->position) x += 8; elsewhere. Maybe "state->pending" variable can be removed altogether and just replaced by the checks whether "state->position" is divisible by 16 or not? The "move old samples on wraparound" code may need to be updated to ensure that this divisibility by 16 check is not messed up. > + > + if (nchannels > 0) { > + int16_t *x = &X[0][position]; > + x[0] = 0; > + x[1] = PCM(0 + 7 * nchannels); > + x[2] = 0; > + x[3] = 0; > + x[4] = 0; > + x[5] = 0; > + x[6] = 0; > + x[7] = 0; > + x[8] = 0; The initialization to 0 looks redundant to me. The samples from the future (which are not yet available to the encoder at this moment) are not supposed to have any effect on calculations. They should be quite conveniently multiplied by zero constants in the analysis filter and cancelled out. If they did affect anything, this would indicate a serious bug in the codec. > + x[9] = PCM(0 + 3 * nchannels); > + x[10] = PCM(0 + 6 * nchannels); > + x[11] = PCM(0 + 0 * nchannels); > + x[12] = PCM(0 + 5 * nchannels); > + x[13] = PCM(0 + 1 * nchannels); > + x[14] = PCM(0 + 4 * nchannels); > + x[15] = PCM(0 + 2 * nchannels); > + } > + pcm += 16 * nchannels; > } > #undef PCM One more nitpick about "sbc_encoder_process_input_s8". The old variant was taking "position" as a function argument and returning an updated position. After you changes, the position is now read from the "state" struct in the beginning of the function. It looks a bit ugly to still return the updated position value from the function and expect the caller to store it back to the "state" struct. > @@ -523,9 +578,16 @@ static int sbc_calc_scalefactors_j( > */ > void sbc_init_primitives(struct sbc_encoder_state *state) > { > + state->pending = -1; > + state->odd = 1; > + > /* Default implementation for analyze functions */ > state->sbc_analyze_4b_4s = sbc_analyze_4b_4s_simd; > - state->sbc_analyze_4b_8s = sbc_analyze_4b_8s_simd; > + > + if (state->increment == 1) > + state->sbc_analyze_4b_8s = sbc_analyze_1b_8s_simd; > + else > + state->sbc_analyze_4b_8s = sbc_analyze_4b_8s_simd; > > /* Default implementation for input reordering / deinterleaving */ > state->sbc_enc_process_input_4s_le = sbc_enc_process_input_4s_le; > diff --git a/sbc/sbc_primitives.h b/sbc/sbc_primitives.h > index eed946e..9a27d3c 100644 > --- a/sbc/sbc_primitives.h > +++ b/sbc/sbc_primitives.h > @@ -40,6 +40,8 @@ struct sbc_encoder_state { > int position; > /* Number of consecutive blocks handled by the encoder */ > int increment; > + int pending; > + int odd; > int16_t SBC_ALIGNED X[2][SBC_X_BUFFER_SIZE]; > /* Polyphase analysis filter for 4 subbands configuration, > * it handles "increment" blocks at once */ -- Best regards, Siarhei Siamashka