[PATCH v3 00/10] mSBC tests

[PATCH v3 00/10] mSBC tests

[Frédéric Dalleau]

Hi,

v3 integrate latest comments from Marcel, hope it is fine now!

How to use:
sample.au should be an .au audio file 16000hz 16bits 1 channel pcm.
$ src/sbcenc  -m -b26 -B16 -s8   sample.au > sample.au.msbc
$ src/sbcinfo sample.au.msbc
$ src/sbcdec  -m -f sample.au.msbc.au sample.au.msbc
$ mplayer sample.au.msbc.au

Regards,
Frederic


Frédéric Dalleau (10):
  sbc: Add encoder_state to process input functions
  sbc: Add encoder_state to analysis functions
  sbc: Break 4 blocks processing to variable steps
  sbc: Add msbc flag and generic C primitive
  sbc: Add support for mSBC frame header
  sbc: Add mmx primitive for 1b 8s analyse
  sbc: Update sbcdec for msbc
  sbc: Update sbcenc for msbc
  sbc: Update sbcinfo for msbc
  sbc: Update copyrights

 sbc/sbc.c                   |  275 ++++++++++++++++++++++++++-----------------
 sbc/sbc.h                   |    3 +
 sbc/sbc_primitives.c        |  105 +++++++++++++----
 sbc/sbc_primitives.h        |   23 ++--
 sbc/sbc_primitives_armv6.c  |    6 +-
 sbc/sbc_primitives_iwmmxt.c |    8 +-
 sbc/sbc_primitives_mmx.c    |   27 ++++-
 sbc/sbc_primitives_neon.c   |   40 +++----
 src/sbcdec.c                |   18 ++-
 src/sbcenc.c                |   26 +++-
 src/sbcinfo.c               |   52 +++++---
 11 files changed, 392 insertions(+), 191 deletions(-)

-- 
1.7.9.5

[PATCH v3 01/10] sbc: Add encoder_state to process input functions

[Frédéric Dalleau]

This patch is because we plan to add a new field to encoder_state structure.
This field will be used by process_input functions in order to store whether
the encoder is in the middle of processing a block.
---
 sbc/sbc.c                 |    4 ++--
 sbc/sbc_primitives.c      |   30 ++++++++++++++++--------------
 sbc/sbc_primitives.h      |    8 ++++----
 sbc/sbc_primitives_neon.c |   32 ++++++++++++++++----------------
 4 files changed, 38 insertions(+), 36 deletions(-)

diff --git a/sbc/sbc.c b/sbc/sbc.c
index f0c77c7..76acf43 100644
--- a/sbc/sbc.c
+++ b/sbc/sbc.c
@@ -1034,7 +1034,7 @@ SBC_EXPORT ssize_t sbc_encode(sbc_t *sbc, const void *input, size_t input_len,
 	struct sbc_priv *priv;
 	int samples;
 	ssize_t framelen;
-	int (*sbc_enc_process_input)(int position,
+	int (*sbc_enc_process_input)(struct sbc_encoder_state *state,
 			const uint8_t *pcm, int16_t X[2][SBC_X_BUFFER_SIZE],
 			int nsamples, int nchannels);
 
@@ -1092,7 +1092,7 @@ SBC_EXPORT ssize_t sbc_encode(sbc_t *sbc, const void *input, size_t input_len,
 	}
 
 	priv->enc_state.position = sbc_enc_process_input(
-		priv->enc_state.position, (const uint8_t *) input,
+		&priv->enc_state, (const uint8_t *) input,
 		priv->enc_state.X, priv->frame.subbands * priv->frame.blocks,
 		priv->frame.channels);
 
diff --git a/sbc/sbc_primitives.c b/sbc/sbc_primitives.c
index ad780d0..e137604 100644
--- a/sbc/sbc_primitives.c
+++ b/sbc/sbc_primitives.c
@@ -227,10 +227,11 @@ static inline int16_t unaligned16_le(const uint8_t *ptr)
  */
 
 static SBC_ALWAYS_INLINE int sbc_encoder_process_input_s4_internal(
-	int position,
+	struct sbc_encoder_state *state,
 	const uint8_t *pcm, int16_t X[2][SBC_X_BUFFER_SIZE],
 	int nsamples, int nchannels, int big_endian)
 {
+	int position = state->position;
 	/* handle X buffer wraparound */
 	if (position < nsamples) {
 		if (nchannels > 0)
@@ -278,10 +279,11 @@ static SBC_ALWAYS_INLINE int sbc_encoder_process_input_s4_internal(
 }
 
 static SBC_ALWAYS_INLINE int sbc_encoder_process_input_s8_internal(
-	int position,
+	struct sbc_encoder_state *state,
 	const uint8_t *pcm, int16_t X[2][SBC_X_BUFFER_SIZE],
 	int nsamples, int nchannels, int big_endian)
 {
+	int position = state->position;
 	/* handle X buffer wraparound */
 	if (position < nsamples) {
 		if (nchannels > 0)
@@ -356,52 +358,52 @@ static SBC_ALWAYS_INLINE int sbc_encoder_process_input_s8_internal(
  * to the top of the buffer on buffer wraparound.
  */
 
-static int sbc_enc_process_input_4s_le(int position,
+static int sbc_enc_process_input_4s_le(struct sbc_encoder_state *state,
 		const uint8_t *pcm, int16_t X[2][SBC_X_BUFFER_SIZE],
 		int nsamples, int nchannels)
 {
 	if (nchannels > 1)
 		return sbc_encoder_process_input_s4_internal(
-			position, pcm, X, nsamples, 2, 0);
+			state, pcm, X, nsamples, 2, 0);
 	else
 		return sbc_encoder_process_input_s4_internal(
-			position, pcm, X, nsamples, 1, 0);
+			state, pcm, X, nsamples, 1, 0);
 }
 
-static int sbc_enc_process_input_4s_be(int position,
+static int sbc_enc_process_input_4s_be(struct sbc_encoder_state *state,
 		const uint8_t *pcm, int16_t X[2][SBC_X_BUFFER_SIZE],
 		int nsamples, int nchannels)
 {
 	if (nchannels > 1)
 		return sbc_encoder_process_input_s4_internal(
-			position, pcm, X, nsamples, 2, 1);
+			state, pcm, X, nsamples, 2, 1);
 	else
 		return sbc_encoder_process_input_s4_internal(
-			position, pcm, X, nsamples, 1, 1);
+			state, pcm, X, nsamples, 1, 1);
 }
 
-static int sbc_enc_process_input_8s_le(int position,
+static int sbc_enc_process_input_8s_le(struct sbc_encoder_state *state,
 		const uint8_t *pcm, int16_t X[2][SBC_X_BUFFER_SIZE],
 		int nsamples, int nchannels)
 {
 	if (nchannels > 1)
 		return sbc_encoder_process_input_s8_internal(
-			position, pcm, X, nsamples, 2, 0);
+			state, pcm, X, nsamples, 2, 0);
 	else
 		return sbc_encoder_process_input_s8_internal(
-			position, pcm, X, nsamples, 1, 0);
+			state, pcm, X, nsamples, 1, 0);
 }
 
-static int sbc_enc_process_input_8s_be(int position,
+static int sbc_enc_process_input_8s_be(struct sbc_encoder_state *state,
 		const uint8_t *pcm, int16_t X[2][SBC_X_BUFFER_SIZE],
 		int nsamples, int nchannels)
 {
 	if (nchannels > 1)
 		return sbc_encoder_process_input_s8_internal(
-			position, pcm, X, nsamples, 2, 1);
+			state, pcm, X, nsamples, 2, 1);
 	else
 		return sbc_encoder_process_input_s8_internal(
-			position, pcm, X, nsamples, 1, 1);
+			state, pcm, X, nsamples, 1, 1);
 }
 
 /* Supplementary function to count the number of leading zeros */
diff --git a/sbc/sbc_primitives.h b/sbc/sbc_primitives.h
index 17ad4f7..c80337e 100644
--- a/sbc/sbc_primitives.h
+++ b/sbc/sbc_primitives.h
@@ -47,16 +47,16 @@ struct sbc_encoder_state {
 	void (*sbc_analyze_4b_8s)(int16_t *x, int32_t *out, int out_stride);
 	/* Process input data (deinterleave, endian conversion, reordering),
 	 * depending on the number of subbands and input data byte order */
-	int (*sbc_enc_process_input_4s_le)(int position,
+	int (*sbc_enc_process_input_4s_le)(struct sbc_encoder_state *state,
 			const uint8_t *pcm, int16_t X[2][SBC_X_BUFFER_SIZE],
 			int nsamples, int nchannels);
-	int (*sbc_enc_process_input_4s_be)(int position,
+	int (*sbc_enc_process_input_4s_be)(struct sbc_encoder_state *state,
 			const uint8_t *pcm, int16_t X[2][SBC_X_BUFFER_SIZE],
 			int nsamples, int nchannels);
-	int (*sbc_enc_process_input_8s_le)(int position,
+	int (*sbc_enc_process_input_8s_le)(struct sbc_encoder_state *state,
 			const uint8_t *pcm, int16_t X[2][SBC_X_BUFFER_SIZE],
 			int nsamples, int nchannels);
-	int (*sbc_enc_process_input_8s_be)(int position,
+	int (*sbc_enc_process_input_8s_be)(struct sbc_encoder_state *state,
 			const uint8_t *pcm, int16_t X[2][SBC_X_BUFFER_SIZE],
 			int nsamples, int nchannels);
 	/* Scale factors calculation */
diff --git a/sbc/sbc_primitives_neon.c b/sbc/sbc_primitives_neon.c
index 5d4d0e3..83277ae 100644
--- a/sbc/sbc_primitives_neon.c
+++ b/sbc/sbc_primitives_neon.c
@@ -845,36 +845,36 @@ static SBC_ALWAYS_INLINE int sbc_enc_process_input_8s_neon_internal(
 #undef PERM_BE
 #undef PERM_LE
 
-static int sbc_enc_process_input_4s_be_neon(int position, const uint8_t *pcm,
-					int16_t X[2][SBC_X_BUFFER_SIZE],
-					int nsamples, int nchannels)
+static int sbc_enc_process_input_4s_be_neon(struct sbc_encoder_state *state,
+		const uint8_t *pcm, int16_t X[2][SBC_X_BUFFER_SIZE],
+		int nsamples, int nchannels)
 {
 	return sbc_enc_process_input_4s_neon_internal(
-		position, pcm, X, nsamples, nchannels, 1);
+		state->position, pcm, X, nsamples, nchannels, 1);
 }
 
-static int sbc_enc_process_input_4s_le_neon(int position, const uint8_t *pcm,
-					int16_t X[2][SBC_X_BUFFER_SIZE],
-					int nsamples, int nchannels)
+static int sbc_enc_process_input_4s_le_neon(struct sbc_encoder_state *state,
+		const uint8_t *pcm, int16_t X[2][SBC_X_BUFFER_SIZE],
+		int nsamples, int nchannels)
 {
 	return sbc_enc_process_input_4s_neon_internal(
-		position, pcm, X, nsamples, nchannels, 0);
+		state->position, pcm, X, nsamples, nchannels, 0);
 }
 
-static int sbc_enc_process_input_8s_be_neon(int position, const uint8_t *pcm,
-					int16_t X[2][SBC_X_BUFFER_SIZE],
-					int nsamples, int nchannels)
+static int sbc_enc_process_input_8s_be_neon(struct sbc_encoder_state *state,
+		const uint8_t *pcm, int16_t X[2][SBC_X_BUFFER_SIZE],
+		int nsamples, int nchannels)
 {
 	return sbc_enc_process_input_8s_neon_internal(
-		position, pcm, X, nsamples, nchannels, 1);
+		state->position, pcm, X, nsamples, nchannels, 1);
 }
 
-static int sbc_enc_process_input_8s_le_neon(int position, const uint8_t *pcm,
-					int16_t X[2][SBC_X_BUFFER_SIZE],
-					int nsamples, int nchannels)
+static int sbc_enc_process_input_8s_le_neon(struct sbc_encoder_state *state,
+		const uint8_t *pcm, int16_t X[2][SBC_X_BUFFER_SIZE],
+		int nsamples, int nchannels)
 {
 	return sbc_enc_process_input_8s_neon_internal(
-		position, pcm, X, nsamples, nchannels, 0);
+		state->position, pcm, X, nsamples, nchannels, 0);
 }
 
 void sbc_init_primitives_neon(struct sbc_encoder_state *state)
-- 
1.7.9.5

[PATCH v3 02/10] sbc: Add encoder_state to analysis functions

[Frédéric Dalleau]

Until now, SIMD analysis used to process 4 blocks of 8 samples at a time. This
was implemented using two constant tables: odd and even. This mean we can only
process 4, 8, 12, or 16 blocks par SBC packets.
mSBC requires 15 blocks, so to be able to analyse 1 block, it will be necessary
to know if we are processing an odd or even block. This will be done with a
new member to encoder_state.
---
 sbc/sbc.c                   |    4 ++--
 sbc/sbc_primitives.c        |    8 ++++----
 sbc/sbc_primitives.h        |    6 ++++--
 sbc/sbc_primitives_armv6.c  |    6 ++++--
 sbc/sbc_primitives_iwmmxt.c |    8 ++++----
 sbc/sbc_primitives_mmx.c    |    8 ++++----
 sbc/sbc_primitives_neon.c   |    8 ++++----
 7 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/sbc/sbc.c b/sbc/sbc.c
index 76acf43..08b4993 100644
--- a/sbc/sbc.c
+++ b/sbc/sbc.c
@@ -692,7 +692,7 @@ static int sbc_analyze_audio(struct sbc_encoder_state *state,
 							frame->blocks * 4];
 			for (blk = 0; blk < frame->blocks; blk += 4) {
 				state->sbc_analyze_4b_4s(
-					x,
+					state, x,
 					frame->sb_sample_f[blk][ch],
 					frame->sb_sample_f[blk + 1][ch] -
 					frame->sb_sample_f[blk][ch]);
@@ -707,7 +707,7 @@ static int sbc_analyze_audio(struct sbc_encoder_state *state,
 							frame->blocks * 8];
 			for (blk = 0; blk < frame->blocks; blk += 4) {
 				state->sbc_analyze_4b_8s(
-					x,
+					state, x,
 					frame->sb_sample_f[blk][ch],
 					frame->sb_sample_f[blk + 1][ch] -
 					frame->sb_sample_f[blk][ch]);
diff --git a/sbc/sbc_primitives.c b/sbc/sbc_primitives.c
index e137604..7ba0589 100644
--- a/sbc/sbc_primitives.c
+++ b/sbc/sbc_primitives.c
@@ -183,8 +183,8 @@ static inline void sbc_analyze_eight_simd(const int16_t *in, int32_t *out,
 			(SBC_COS_TABLE_FIXED8_SCALE - SCALE_OUT_BITS);
 }
 
-static inline void sbc_analyze_4b_4s_simd(int16_t *x,
-						int32_t *out, int out_stride)
+static inline void sbc_analyze_4b_4s_simd(struct sbc_encoder_state *state,
+		int16_t *x, int32_t *out, int out_stride)
 {
 	/* Analyze blocks */
 	sbc_analyze_four_simd(x + 12, out, analysis_consts_fixed4_simd_odd);
@@ -196,8 +196,8 @@ static inline void sbc_analyze_4b_4s_simd(int16_t *x,
 	sbc_analyze_four_simd(x + 0, out, analysis_consts_fixed4_simd_even);
 }
 
-static inline void sbc_analyze_4b_8s_simd(int16_t *x,
-					  int32_t *out, int out_stride)
+static inline void sbc_analyze_4b_8s_simd(struct sbc_encoder_state *state,
+		int16_t *x, int32_t *out, int out_stride)
 {
 	/* Analyze blocks */
 	sbc_analyze_eight_simd(x + 24, out, analysis_consts_fixed8_simd_odd);
diff --git a/sbc/sbc_primitives.h b/sbc/sbc_primitives.h
index c80337e..47363db 100644
--- a/sbc/sbc_primitives.h
+++ b/sbc/sbc_primitives.h
@@ -41,10 +41,12 @@ struct sbc_encoder_state {
 	int16_t SBC_ALIGNED X[2][SBC_X_BUFFER_SIZE];
 	/* Polyphase analysis filter for 4 subbands configuration,
 	 * it handles 4 blocks at once */
-	void (*sbc_analyze_4b_4s)(int16_t *x, int32_t *out, int out_stride);
+	void (*sbc_analyze_4b_4s)(struct sbc_encoder_state *state,
+			int16_t *x, int32_t *out, int out_stride);
 	/* Polyphase analysis filter for 8 subbands configuration,
 	 * it handles 4 blocks at once */
-	void (*sbc_analyze_4b_8s)(int16_t *x, int32_t *out, int out_stride);
+	void (*sbc_analyze_4b_8s)(struct sbc_encoder_state *state,
+			int16_t *x, int32_t *out, int out_stride);
 	/* Process input data (deinterleave, endian conversion, reordering),
 	 * depending on the number of subbands and input data byte order */
 	int (*sbc_enc_process_input_4s_le)(struct sbc_encoder_state *state,
diff --git a/sbc/sbc_primitives_armv6.c b/sbc/sbc_primitives_armv6.c
index b321272..6ad94c6 100644
--- a/sbc/sbc_primitives_armv6.c
+++ b/sbc/sbc_primitives_armv6.c
@@ -265,7 +265,8 @@ static void __attribute__((naked)) sbc_analyze_eight_armv6()
 	((void (*)(int16_t *, int32_t *, const FIXED_T*)) \
 		sbc_analyze_eight_armv6)((in), (out), (consts))
 
-static void sbc_analyze_4b_4s_armv6(int16_t *x, int32_t *out, int out_stride)
+static void sbc_analyze_4b_4s_armv6(struct sbc_encoder_state *state,
+		int16_t *x, int32_t *out, int out_stride)
 {
 	/* Analyze blocks */
 	sbc_analyze_four(x + 12, out, analysis_consts_fixed4_simd_odd);
@@ -277,7 +278,8 @@ static void sbc_analyze_4b_4s_armv6(int16_t *x, int32_t *out, int out_stride)
 	sbc_analyze_four(x + 0, out, analysis_consts_fixed4_simd_even);
 }
 
-static void sbc_analyze_4b_8s_armv6(int16_t *x, int32_t *out, int out_stride)
+static void sbc_analyze_4b_8s_armv6(struct sbc_encoder_state *state,
+		int16_t *x, int32_t *out, int out_stride)
 {
 	/* Analyze blocks */
 	sbc_analyze_eight(x + 24, out, analysis_consts_fixed8_simd_odd);
diff --git a/sbc/sbc_primitives_iwmmxt.c b/sbc/sbc_primitives_iwmmxt.c
index e0bd060..39cc390 100644
--- a/sbc/sbc_primitives_iwmmxt.c
+++ b/sbc/sbc_primitives_iwmmxt.c
@@ -268,8 +268,8 @@ static inline void sbc_analyze_eight_iwmmxt(const int16_t *in, int32_t *out,
 		  "wcgr0", "memory");
 }
 
-static inline void sbc_analyze_4b_4s_iwmmxt(int16_t *x, int32_t *out,
-						int out_stride)
+static inline void sbc_analyze_4b_4s_iwmmxt(struct sbc_encoder_state *state,
+		int16_t *x, int32_t *out, int out_stride)
 {
 	/* Analyze blocks */
 	sbc_analyze_four_iwmmxt(x + 12, out, analysis_consts_fixed4_simd_odd);
@@ -281,8 +281,8 @@ static inline void sbc_analyze_4b_4s_iwmmxt(int16_t *x, int32_t *out,
 	sbc_analyze_four_iwmmxt(x + 0, out, analysis_consts_fixed4_simd_even);
 }
 
-static inline void sbc_analyze_4b_8s_iwmmxt(int16_t *x, int32_t *out,
-						int out_stride)
+static inline void sbc_analyze_4b_8s_iwmmxt(struct sbc_encoder_state *state,
+		int16_t *x, int32_t *out, int out_stride)
 {
 	/* Analyze blocks */
 	sbc_analyze_eight_iwmmxt(x + 24, out, analysis_consts_fixed8_simd_odd);
diff --git a/sbc/sbc_primitives_mmx.c b/sbc/sbc_primitives_mmx.c
index 27e9a56..cbacb4e 100644
--- a/sbc/sbc_primitives_mmx.c
+++ b/sbc/sbc_primitives_mmx.c
@@ -246,8 +246,8 @@ static inline void sbc_analyze_eight_mmx(const int16_t *in, int32_t *out,
 		: "cc", "memory");
 }
 
-static inline void sbc_analyze_4b_4s_mmx(int16_t *x, int32_t *out,
-						int out_stride)
+static inline void sbc_analyze_4b_4s_mmx(struct sbc_encoder_state *state,
+		int16_t *x, int32_t *out, int out_stride)
 {
 	/* Analyze blocks */
 	sbc_analyze_four_mmx(x + 12, out, analysis_consts_fixed4_simd_odd);
@@ -261,8 +261,8 @@ static inline void sbc_analyze_4b_4s_mmx(int16_t *x, int32_t *out,
 	__asm__ volatile ("emms\n");
 }
 
-static inline void sbc_analyze_4b_8s_mmx(int16_t *x, int32_t *out,
-						int out_stride)
+static inline void sbc_analyze_4b_8s_mmx(struct sbc_encoder_state *state,
+		int16_t *x, int32_t *out, int out_stride)
 {
 	/* Analyze blocks */
 	sbc_analyze_eight_mmx(x + 24, out, analysis_consts_fixed8_simd_odd);
diff --git a/sbc/sbc_primitives_neon.c b/sbc/sbc_primitives_neon.c
index 83277ae..5b38060 100644
--- a/sbc/sbc_primitives_neon.c
+++ b/sbc/sbc_primitives_neon.c
@@ -211,8 +211,8 @@ static inline void _sbc_analyze_eight_neon(const int16_t *in, int32_t *out,
 			"d18", "d19");
 }
 
-static inline void sbc_analyze_4b_4s_neon(int16_t *x,
-						int32_t *out, int out_stride)
+static inline void sbc_analyze_4b_4s_neon(struct sbc_encoder_state *state,
+		int16_t *x, int32_t *out, int out_stride)
 {
 	/* Analyze blocks */
 	_sbc_analyze_four_neon(x + 12, out, analysis_consts_fixed4_simd_odd);
@@ -224,8 +224,8 @@ static inline void sbc_analyze_4b_4s_neon(int16_t *x,
 	_sbc_analyze_four_neon(x + 0, out, analysis_consts_fixed4_simd_even);
 }
 
-static inline void sbc_analyze_4b_8s_neon(int16_t *x,
-						int32_t *out, int out_stride)
+static inline void sbc_analyze_4b_8s_neon(struct sbc_encoder_state *state,
+		int16_t *x, int32_t *out, int out_stride)
 {
 	/* Analyze blocks */
 	_sbc_analyze_eight_neon(x + 24, out, analysis_consts_fixed8_simd_odd);
-- 
1.7.9.5

[PATCH v3 03/10] sbc: Break 4 blocks processing to variable steps

[Frédéric Dalleau]

Until now SBC processed 4 blocks at a time. If we want to process 15 blocks,
then we need to break this processing in one block steps. 4 blocks is still
default increment.
---
 sbc/sbc.c            |   13 +++++++------
 sbc/sbc_primitives.h |    6 ++++--
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/sbc/sbc.c b/sbc/sbc.c
index 08b4993..9b0634d 100644
--- a/sbc/sbc.c
+++ b/sbc/sbc.c
@@ -688,30 +688,30 @@ static int sbc_analyze_audio(struct sbc_encoder_state *state,
 	switch (frame->subbands) {
 	case 4:
 		for (ch = 0; ch < frame->channels; ch++) {
-			x = &state->X[ch][state->position - 16 +
+			x = &state->X[ch][state->position - 4 * state->increment +
 							frame->blocks * 4];
-			for (blk = 0; blk < frame->blocks; blk += 4) {
+			for (blk = 0; blk < frame->blocks; blk += state->increment) {
 				state->sbc_analyze_4b_4s(
 					state, x,
 					frame->sb_sample_f[blk][ch],
 					frame->sb_sample_f[blk + 1][ch] -
 					frame->sb_sample_f[blk][ch]);
-				x -= 16;
+				x -= 4 * state->increment;
 			}
 		}
 		return frame->blocks * 4;
 
 	case 8:
 		for (ch = 0; ch < frame->channels; ch++) {
-			x = &state->X[ch][state->position - 32 +
+			x = &state->X[ch][state->position - 8 * state->increment +
 							frame->blocks * 8];
-			for (blk = 0; blk < frame->blocks; blk += 4) {
+			for (blk = 0; blk < frame->blocks; blk += state->increment) {
 				state->sbc_analyze_4b_8s(
 					state, x,
 					frame->sb_sample_f[blk][ch],
 					frame->sb_sample_f[blk + 1][ch] -
 					frame->sb_sample_f[blk][ch]);
-				x -= 32;
+				x -= 8 * state->increment;
 			}
 		}
 		return frame->blocks * 8;
@@ -906,6 +906,7 @@ static void sbc_encoder_init(struct sbc_encoder_state *state,
 {
 	memset(&state->X, 0, sizeof(state->X));
 	state->position = (SBC_X_BUFFER_SIZE - frame->subbands * 9) & ~7;
+	state->increment = 4;
 
 	sbc_init_primitives(state);
 }
diff --git a/sbc/sbc_primitives.h b/sbc/sbc_primitives.h
index 47363db..eed946e 100644
--- a/sbc/sbc_primitives.h
+++ b/sbc/sbc_primitives.h
@@ -38,13 +38,15 @@
 
 struct sbc_encoder_state {
 	int position;
+	/* Number of consecutive blocks handled by the encoder */
+	int increment;
 	int16_t SBC_ALIGNED X[2][SBC_X_BUFFER_SIZE];
 	/* Polyphase analysis filter for 4 subbands configuration,
-	 * it handles 4 blocks at once */
+	 * it handles "increment" blocks at once */
 	void (*sbc_analyze_4b_4s)(struct sbc_encoder_state *state,
 			int16_t *x, int32_t *out, int out_stride);
 	/* Polyphase analysis filter for 8 subbands configuration,
-	 * it handles 4 blocks at once */
+	 * it handles "increment" blocks at once */
 	void (*sbc_analyze_4b_8s)(struct sbc_encoder_state *state,
 			int16_t *x, int32_t *out, int out_stride);
 	/* Process input data (deinterleave, endian conversion, reordering),
-- 
1.7.9.5

[PATCH v3 04/10] sbc: Add msbc flag and generic C primitive

[Frédéric Dalleau]

---
 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(-)

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;
+
 			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);
+		}
+		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 (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;
+			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
 
@@ -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 */
-- 
1.7.9.5

Re: [PATCH v3 04/10] sbc: Add msbc flag and generic C primitive

[Siarhei Siamashka]

On Fri, 26 Oct 2012 19:20:30 +0200
Frédéric Dalleau  <frederic.dalleau@linux.intel.com> 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

Re: [PATCH v3 04/10] sbc: Add msbc flag and generic C primitive

[Dalleau, Frederic]

Hi Siarhei,

Thanks for review, I mostly agree but have a few remarks.

On Sun, Oct 28, 2012 at 12:19 AM, Siarhei Siamashka
<siarhei.siamashka@gmail.com> wrote:
> On Fri, 26 Oct 2012 19:20:30 +0200
> 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.

Good idea, just note that in this case, one sample  (x[1]  = PCM(0 + 7
* nchannels))
will receive a negative index x[-7]. there is no technical problème
just a matter of style.


> One more nitpick about "sbc_encoder_process_input_s8". The old
> variant was taking "position" as a function argument and returning
> an updated position.

Addtionnally, sbc_encoder_process_input_s8 would return void, and
state->position
would be used directly from process_input function. However, it requires changes
in the neon code, which I'd rather avoid for now. I can send a patch
later for this one.

Regards,
Frédéric

Re: [PATCH v3 04/10] sbc: Add msbc flag and generic C primitive

[Siarhei Siamashka]

On Mon, 29 Oct 2012 07:47:03 +0100
"Dalleau, Frederic" <frederic.dalleau@intel.com> wrote:

> Hi Siarhei,
> 
> Thanks for review,

Hi Frederic. Thanks for providing the initial working prototype of
mSBC support and for your progress polishing the remaining rough edges.

> I mostly agree but have a few remarks.

> On Sun, Oct 28, 2012 at 12:19 AM, Siarhei Siamashka
> <siarhei.siamashka@gmail.com> wrote:
> > On Fri, 26 Oct 2012 19:20:30 +0200
> > 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.
> 
> Good idea, just note that in this case, one sample  (x[1]  = PCM(0 + 7
> * nchannels))
> will receive a negative index x[-7]. there is no technical problème
> just a matter of style.
> 
> 
> > One more nitpick about "sbc_encoder_process_input_s8". The old
> > variant was taking "position" as a function argument and returning
> > an updated position.
> 
> Addtionnally, sbc_encoder_process_input_s8 would return void, and
> state->position
> would be used directly from process_input function. However, it requires changes
> in the neon code, which I'd rather avoid for now. I can send a patch
> later for this one.

Not having the need for 'state->pending' actually allows to keep both
the arguments and return value intact for "sbc_encoder_process_input_*"
functions and make the changes a bit less invasive as a side effect.

It might be even possible to make all the even/odd block checks based on
address alignment instead of "position" divisibility by 16 and get rid
of "state->odd" variable. But this would either require increasing the
effect of SBC_ALIGNED attribute to 32 bytes and make the code
non-portable and GCC specific (right now at least the C implementation
should work correctly with any compiler). Or alternatively the "X"
buffer could be allocated with manual realignment just like the "priv"
struct here:
    http://git.kernel.org/?p=bluetooth/sbc.git;a=blob;f=sbc/sbc.c;h=f0c77c7cb546#l943
Which in turn may become rather invasive and wacky, so I wouldn't go
that far.

-- 
Best regards,
Siarhei Siamashka

[PATCH v3 05/10] sbc: Add support for mSBC frame header

[Frédéric Dalleau]

---
 sbc/sbc.c |  228 +++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 138 insertions(+), 90 deletions(-)

diff --git a/sbc/sbc.c b/sbc/sbc.c
index 4bc97fc..8236122 100644
--- a/sbc/sbc.c
+++ b/sbc/sbc.c
@@ -377,8 +377,8 @@ static void sbc_calculate_bits(const struct sbc_frame *frame, int (*bits)[8])
  *  -3   CRC8 incorrect
  *  -4   Bitpool value out of bounds
  */
-static int sbc_unpack_frame(const uint8_t *data, struct sbc_frame *frame,
-								size_t len)
+static int sbc_unpack_frame_internal(const uint8_t *data,
+		struct sbc_frame *frame, size_t len)
 {
 	unsigned int consumed;
 	/* Will copy the parts of the header that are relevant to crc
@@ -393,59 +393,6 @@ static int sbc_unpack_frame(const uint8_t *data, struct sbc_frame *frame,
 	int bits[2][8];		/* bits distribution */
 	uint32_t levels[2][8];	/* levels derived from that */
 
-	if (len < 4)
-		return -1;
-
-	if (data[0] != SBC_SYNCWORD)
-		return -2;
-
-	frame->frequency = (data[1] >> 6) & 0x03;
-
-	frame->block_mode = (data[1] >> 4) & 0x03;
-	switch (frame->block_mode) {
-	case SBC_BLK_4:
-		frame->blocks = 4;
-		break;
-	case SBC_BLK_8:
-		frame->blocks = 8;
-		break;
-	case SBC_BLK_12:
-		frame->blocks = 12;
-		break;
-	case SBC_BLK_16:
-		frame->blocks = 16;
-		break;
-	}
-
-	frame->mode = (data[1] >> 2) & 0x03;
-	switch (frame->mode) {
-	case MONO:
-		frame->channels = 1;
-		break;
-	case DUAL_CHANNEL:	/* fall-through */
-	case STEREO:
-	case JOINT_STEREO:
-		frame->channels = 2;
-		break;
-	}
-
-	frame->allocation = (data[1] >> 1) & 0x01;
-
-	frame->subband_mode = (data[1] & 0x01);
-	frame->subbands = frame->subband_mode ? 8 : 4;
-
-	frame->bitpool = data[2];
-
-	if ((frame->mode == MONO || frame->mode == DUAL_CHANNEL) &&
-			frame->bitpool > 16 * frame->subbands)
-		return -4;
-
-	if ((frame->mode == STEREO || frame->mode == JOINT_STEREO) &&
-			frame->bitpool > 32 * frame->subbands)
-		return -4;
-
-	/* data[3] is crc, we're checking it later */
-
 	consumed = 32;
 
 	crc_header[0] = data[1];
@@ -546,6 +493,90 @@ static int sbc_unpack_frame(const uint8_t *data, struct sbc_frame *frame,
 	return consumed >> 3;
 }
 
+static int sbc_unpack_frame(const uint8_t *data,
+		struct sbc_frame *frame, size_t len)
+{
+	if (len < 4)
+		return -1;
+
+	if (data[0] != SBC_SYNCWORD)
+		return -2;
+
+	frame->frequency = (data[1] >> 6) & 0x03;
+	frame->block_mode = (data[1] >> 4) & 0x03;
+
+	switch (frame->block_mode) {
+	case SBC_BLK_4:
+		frame->blocks = 4;
+		break;
+	case SBC_BLK_8:
+		frame->blocks = 8;
+		break;
+	case SBC_BLK_12:
+		frame->blocks = 12;
+		break;
+	case SBC_BLK_16:
+		frame->blocks = 16;
+		break;
+	}
+
+	frame->mode = (data[1] >> 2) & 0x03;
+
+	switch (frame->mode) {
+	case MONO:
+		frame->channels = 1;
+		break;
+	case DUAL_CHANNEL:	/* fall-through */
+	case STEREO:
+	case JOINT_STEREO:
+		frame->channels = 2;
+		break;
+	}
+
+	frame->allocation = (data[1] >> 1) & 0x01;
+
+	frame->subband_mode = (data[1] & 0x01);
+	frame->subbands = frame->subband_mode ? 8 : 4;
+
+	frame->bitpool = data[2];
+
+	if ((frame->mode == MONO || frame->mode == DUAL_CHANNEL) &&
+			frame->bitpool > 16 * frame->subbands)
+		return -4;
+
+	if ((frame->mode == STEREO || frame->mode == JOINT_STEREO) &&
+			frame->bitpool > 32 * frame->subbands)
+		return -4;
+
+	return sbc_unpack_frame_internal(data, frame, len);
+}
+
+static int msbc_unpack_frame(const uint8_t *data,
+		struct sbc_frame *frame, size_t len)
+{
+	if (len < 4)
+		return -1;
+
+	if (data[0] != MSBC_SYNCWORD)
+		return -2;
+	if (data[1] != 0)
+		return -2;
+	if (data[2] != 0)
+		return -2;
+
+	frame->frequency = SBC_FREQ_16000;
+	frame->block_mode = SBC_BLK_4;
+	frame->blocks = MSBC_BLOCKS;
+	frame->allocation = LOUDNESS;
+	frame->mode = MONO;
+	frame->channels = 1;
+	frame->subband_mode = 1;
+	frame->subbands = 8;
+	frame->bitpool = 26;
+
+	return sbc_unpack_frame_internal(data, frame, len);
+}
+
 static void sbc_decoder_init(struct sbc_decoder_state *state,
 					const struct sbc_frame *frame)
 {
@@ -792,38 +823,6 @@ static SBC_ALWAYS_INLINE ssize_t sbc_pack_frame_internal(uint8_t *data,
 	uint32_t levels[2][8];	/* levels are derived from that */
 	uint32_t sb_sample_delta[2][8];
 
-	data[0] = SBC_SYNCWORD;
-
-	data[1] = (frame->frequency & 0x03) << 6;
-
-	data[1] |= (frame->block_mode & 0x03) << 4;
-
-	data[1] |= (frame->mode & 0x03) << 2;
-
-	data[1] |= (frame->allocation & 0x01) << 1;
-
-	switch (frame_subbands) {
-	case 4:
-		/* Nothing to do */
-		break;
-	case 8:
-		data[1] |= 0x01;
-		break;
-	default:
-		return -4;
-		break;
-	}
-
-	data[2] = frame->bitpool;
-
-	if ((frame->mode == MONO || frame->mode == DUAL_CHANNEL) &&
-			frame->bitpool > frame_subbands << 4)
-		return -5;
-
-	if ((frame->mode == STEREO || frame->mode == JOINT_STEREO) &&
-			frame->bitpool > frame_subbands << 5)
-		return -5;
-
 	/* Can't fill in crc yet */
 
 	crc_header[0] = data[1];
@@ -891,6 +890,28 @@ static SBC_ALWAYS_INLINE ssize_t sbc_pack_frame_internal(uint8_t *data,
 static ssize_t sbc_pack_frame(uint8_t *data, struct sbc_frame *frame, size_t len,
 								int joint)
 {
+	int frame_subbands = 4;
+
+	data[0] = SBC_SYNCWORD;
+
+	data[1] = (frame->frequency & 0x03) << 6;
+	data[1] |= (frame->block_mode & 0x03) << 4;
+	data[1] |= (frame->mode & 0x03) << 2;
+	data[1] |= (frame->allocation & 0x01) << 1;
+
+	data[2] = frame->bitpool;
+
+	if (frame->subbands != 4)
+		frame_subbands = 8;
+
+	if ((frame->mode == MONO || frame->mode == DUAL_CHANNEL) &&
+			frame->bitpool > frame_subbands << 4)
+		return -5;
+
+	if ((frame->mode == STEREO || frame->mode == JOINT_STEREO) &&
+			frame->bitpool > frame_subbands << 5)
+		return -5;
+
 	if (frame->subbands == 4) {
 		if (frame->channels == 1)
 			return sbc_pack_frame_internal(
@@ -899,6 +920,7 @@ static ssize_t sbc_pack_frame(uint8_t *data, struct sbc_frame *frame, size_t len
 			return sbc_pack_frame_internal(
 				data, frame, len, 4, 2, joint);
 	} else {
+		data[1] |= 0x01;
 		if (frame->channels == 1)
 			return sbc_pack_frame_internal(
 				data, frame, len, 8, 1, joint);
@@ -908,6 +930,16 @@ static ssize_t sbc_pack_frame(uint8_t *data, struct sbc_frame *frame, size_t len
 	}
 }
 
+static ssize_t msbc_pack_frame(uint8_t *data, struct sbc_frame *frame,
+						size_t len, int joint)
+{
+	data[0] = MSBC_SYNCWORD;
+	data[1] = 0;
+	data[2] = 0;
+
+	return sbc_pack_frame_internal(data, frame, len, 8, 1, joint);
+}
+
 static void sbc_encoder_init(unsigned long flags,
 		struct sbc_encoder_state *state, const struct sbc_frame *frame)
 {
@@ -923,10 +955,24 @@ struct sbc_priv {
 	struct SBC_ALIGNED sbc_frame frame;
 	struct SBC_ALIGNED sbc_decoder_state dec_state;
 	struct SBC_ALIGNED sbc_encoder_state enc_state;
+	int (*unpack_frame)(const uint8_t *data, struct sbc_frame *frame,
+			size_t len);
+	ssize_t (*pack_frame)(uint8_t *data, struct sbc_frame *frame,
+			size_t len, int joint);
 };
 
 static void sbc_set_defaults(sbc_t *sbc, unsigned long flags)
 {
+	struct sbc_priv *priv = sbc->priv;
+
+	if (flags & SBC_MSBC) {
+		priv->pack_frame = msbc_pack_frame;
+		priv->unpack_frame = msbc_unpack_frame;
+	} else {
+		priv->pack_frame = sbc_pack_frame;
+		priv->unpack_frame = sbc_unpack_frame;
+	}
+
 	sbc->flags = flags;
 	sbc->frequency = SBC_FREQ_44100;
 	sbc->mode = SBC_MODE_STEREO;
@@ -980,7 +1026,7 @@ SBC_EXPORT ssize_t sbc_decode(sbc_t *sbc, const void *input, size_t input_len,
 
 	priv = sbc->priv;
 
-	framelen = sbc_unpack_frame(input, &priv->frame, input_len);
+	framelen = priv->unpack_frame(input, &priv->frame, input_len);
 
 	if (!priv->init) {
 		sbc_decoder_init(&priv->dec_state, &priv->frame);
@@ -1112,13 +1158,15 @@ SBC_EXPORT ssize_t sbc_encode(sbc_t *sbc, const void *input, size_t input_len,
 		int j = priv->enc_state.sbc_calc_scalefactors_j(
 			priv->frame.sb_sample_f, priv->frame.scale_factor,
 			priv->frame.blocks, priv->frame.subbands);
-		framelen = sbc_pack_frame(output, &priv->frame, output_len, j);
+		framelen = priv->pack_frame(output,
+				&priv->frame, output_len, j);
 	} else {
 		priv->enc_state.sbc_calc_scalefactors(
 			priv->frame.sb_sample_f, priv->frame.scale_factor,
 			priv->frame.blocks, priv->frame.channels,
 			priv->frame.subbands);
-		framelen = sbc_pack_frame(output, &priv->frame, output_len, 0);
+		framelen = priv->pack_frame(output,
+				&priv->frame, output_len, 0);
 	}
 
 	if (written)
-- 
1.7.9.5

[PATCH v3 06/10] sbc: Add mmx primitive for 1b 8s analyse

[Frédéric Dalleau]

---
 sbc/sbc_primitives_mmx.c |   18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/sbc/sbc_primitives_mmx.c b/sbc/sbc_primitives_mmx.c
index cbacb4e..17065e5 100644
--- a/sbc/sbc_primitives_mmx.c
+++ b/sbc/sbc_primitives_mmx.c
@@ -276,6 +276,19 @@ static inline void sbc_analyze_4b_8s_mmx(struct sbc_encoder_state *state,
 	__asm__ volatile ("emms\n");
 }
 
+static inline void sbc_analyze_1b_8s_mmx(struct sbc_encoder_state *state,
+		int16_t *x, int32_t *out, int out_stride)
+{
+	if (state->odd)
+		sbc_analyze_eight_mmx(x, out, analysis_consts_fixed8_simd_odd);
+	else
+		sbc_analyze_eight_mmx(x, out, analysis_consts_fixed8_simd_even);
+
+	state->odd = !state->odd;
+
+	__asm__ volatile ("emms\n");
+}
+
 static void sbc_calc_scalefactors_mmx(
 	int32_t sb_sample_f[16][2][8],
 	uint32_t scale_factor[2][8],
@@ -366,7 +379,10 @@ void sbc_init_primitives_mmx(struct sbc_encoder_state *state)
 {
 	if (check_mmx_support()) {
 		state->sbc_analyze_4b_4s = sbc_analyze_4b_4s_mmx;
-		state->sbc_analyze_4b_8s = sbc_analyze_4b_8s_mmx;
+		if (state->increment == 1)
+			state->sbc_analyze_4b_8s = sbc_analyze_1b_8s_mmx;
+		else
+			state->sbc_analyze_4b_8s = sbc_analyze_4b_8s_mmx;
 		state->sbc_calc_scalefactors = sbc_calc_scalefactors_mmx;
 		state->implementation_info = "MMX";
 	}
-- 
1.7.9.5

Re: [PATCH v3 06/10] sbc: Add mmx primitive for 1b 8s analyse

[Siarhei Siamashka]

On Fri, 26 Oct 2012 19:20:32 +0200
Frédéric Dalleau  <frederic.dalleau@linux.intel.com> wrote:

> ---
>  sbc/sbc_primitives_mmx.c |   18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)

In the previous patches you are changing the behaviour of C
implementation to support mSBC configuration. By still allowing
assembly optimized implementations to override C functions, mSBC
does not work correctly for the architectures which support these
optimizations.

With this particular patch you are fixing MMX. The other
implementations (ARM NEON) are still broken.

It would be better if we could shape out this patch series so that
the code is always correct after every commit, preferably as:
1. SBC still works and uses assembly optimizations after
   every commit
2. mSBC works correctly after the commit where SBC_MSBC feature
   is advertised in the public header "sbc.h"
3. mSBC gets assembly optimizations enabled. Unsupported architectures
   use C implementation.

> diff --git a/sbc/sbc_primitives_mmx.c b/sbc/sbc_primitives_mmx.c
> index cbacb4e..17065e5 100644
> --- a/sbc/sbc_primitives_mmx.c
> +++ b/sbc/sbc_primitives_mmx.c
> @@ -276,6 +276,19 @@ static inline void sbc_analyze_4b_8s_mmx(struct sbc_encoder_state *state,
>  	__asm__ volatile ("emms\n");
>  }
>  
> +static inline void sbc_analyze_1b_8s_mmx(struct sbc_encoder_state *state,
> +		int16_t *x, int32_t *out, int out_stride)
> +{
> +	if (state->odd)
> +		sbc_analyze_eight_mmx(x, out, analysis_consts_fixed8_simd_odd);
> +	else
> +		sbc_analyze_eight_mmx(x, out, analysis_consts_fixed8_simd_even);
> +
> +	state->odd = !state->odd;
> +
> +	__asm__ volatile ("emms\n");
> +}
> +
>  static void sbc_calc_scalefactors_mmx(
>  	int32_t sb_sample_f[16][2][8],
>  	uint32_t scale_factor[2][8],
> @@ -366,7 +379,10 @@ void sbc_init_primitives_mmx(struct sbc_encoder_state *state)
>  {
>  	if (check_mmx_support()) {
>  		state->sbc_analyze_4b_4s = sbc_analyze_4b_4s_mmx;
> -		state->sbc_analyze_4b_8s = sbc_analyze_4b_8s_mmx;
> +		if (state->increment == 1)
> +			state->sbc_analyze_4b_8s = sbc_analyze_1b_8s_mmx;

Overriding the function with "_4b_" (4 blocks) in its name with "_1b_"
variant (1 block) looks wrong and is just asking for troubles.

Maybe we need a new function pointer? If not, then the function might
need to be at least renamed.

> +		else
> +			state->sbc_analyze_4b_8s = sbc_analyze_4b_8s_mmx;
>  		state->sbc_calc_scalefactors = sbc_calc_scalefactors_mmx;
>  		state->implementation_info = "MMX";
>  	}

-- 
Best regards,
Siarhei Siamashka

Re: [PATCH v3 06/10] sbc: Add mmx primitive for 1b 8s analyse

[Siarhei Siamashka]

On Sun, 28 Oct 2012 02:06:21 +0300
Siarhei Siamashka <siarhei.siamashka@gmail.com> wrote:

> On Fri, 26 Oct 2012 19:20:32 +0200
> Frédéric Dalleau  <frederic.dalleau@linux.intel.com> wrote:
> 
> > ---
> >  sbc/sbc_primitives_mmx.c |   18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> In the previous patches you are changing the behaviour of C
> implementation to support mSBC configuration. By still allowing
> assembly optimized implementations to override C functions, mSBC
> does not work correctly for the architectures which support these
> optimizations.
> 
> With this particular patch you are fixing MMX. The other
> implementations (ARM NEON) are still broken.
> 
> It would be better if we could shape out this patch series so that
> the code is always correct after every commit, preferably as:
> 1. SBC still works and uses assembly optimizations after
>    every commit
> 2. mSBC works correctly after the commit where SBC_MSBC feature
>    is advertised in the public header "sbc.h"
> 3. mSBC gets assembly optimizations enabled. Unsupported architectures
>    use C implementation.

Just to make this work, your "[PATCH v3 04/10] sbc: Add msbc flag and
generic C primitive" patch could update "sbc_init_primitives" in a
bit different way.

You changed it as:

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

But it might be better to move "if (state->increment == 1)" check to
the very end of the function after

	/* X86/AMD64 optimizations */
#ifdef SBC_BUILD_WITH_MMX_SUPPORT
	sbc_init_primitives_mmx(state);
#endif

	/* ARM optimizations */
#ifdef SBC_BUILD_WITH_ARMV6_SUPPORT
	sbc_init_primitives_armv6(state);
#endif
#ifdef SBC_BUILD_WITH_IWMMXT_SUPPORT
	sbc_init_primitives_iwmmxt(state);
#endif
#ifdef SBC_BUILD_WITH_NEON_SUPPORT
	sbc_init_primitives_neon(state);
#endif


So that the C implementation overrides the function pointers for
mSBC configuration after the initialization of mSBC-unaware assembly
implementations.

As soon as some assembly implementation gets mSBC support (MMX for
example), the initialization order in "sbc_init_primitives" can be
changed appropriately.

-- 
Best regards,
Siarhei Siamashka

Re: [PATCH v3 06/10] sbc: Add mmx primitive for 1b 8s analyse

[Dalleau, Frederic]

Hi Siarhei,

Since only neon is concerned by this, I'd rather add a one liner like this :

#ifdef SBC_BUILD_WITH_NEON_SUPPORT
       sbc_init_primitives_neon(state);
+
+     if (state->increment == 1)
+             state->sbc_analyze_4b_8s = sbc_analyze_1b_8s_simd;
#endif

It is more explicit, doesn't change priority and doesn't add needless code
to other implementations.

And what about sbc_analyze_8s?


Regarding point 2, this is the reason why patch 4 was a bit bigger, the simd
implementation is complete.

Regards,
Frédéric

Re: [PATCH v3 06/10] sbc: Add mmx primitive for 1b 8s analyse

[Siarhei Siamashka]

On Mon, 29 Oct 2012 08:42:08 +0100
"Dalleau, Frederic" <frederic.dalleau@intel.com> wrote:

> Hi Siarhei,
> 
> Since only neon is concerned by this, I'd rather add a one liner like this :
> 
> #ifdef SBC_BUILD_WITH_NEON_SUPPORT
>        sbc_init_primitives_neon(state);
> +
> +     if (state->increment == 1)
> +             state->sbc_analyze_4b_8s = sbc_analyze_1b_8s_simd;
> #endif
> 
> It is more explicit, doesn't change priority and doesn't add needless code
> to other implementations.

It's not just neon, but armv6 and iwmmxt are affected too. Additionally,
neon code also provides optimized "sbc_enc_process_input_*" functions,
which are not going to work correctly for mSBC:

	state->sbc_enc_process_input_8s_le = sbc_enc_process_input_8s_le_neon;
	state->sbc_enc_process_input_8s_be = sbc_enc_process_input_8s_be_neon;

So I still think that it is safer and cleaner to have
"sbc_init_primitives" function performing the following in order:
1. Initialize function pointers with C implementations.
2. Allow to override them with various platform specific
implementations (which would work fine for old SBC formats).
3. At the end of the function have a check if we are actually dealing
with mSBC format and restore all the function pointers back to C
implementations in this case.

So SBC is accelerated, and mSBC at least works correctly.

Then for the follow up patches just review/fix all the assembly
optimized implementations one at a time, make sure that they do work
with mSBC and move their initialization to the end of the
"sbc_init_primitives" function.

This allows to first take care of C implementation without MMX
getting in the way. Then fix and enable mSBC support for MMX in the
last patch from your series. Later somebody could take care of the
ARM implementations in a similar way.

> And what about sbc_analyze_8s?

Renaming "sbc_analyze_4b_8s" -> "sbc_analyze_8s" in a separate patch
prior to other changes is IMHO better than keeping the old misleading
name.

> Regarding point 2, this is the reason why patch 4 was a bit bigger, the simd
> implementation is complete.

Well, I just don't quite like that after the patches
    [PATCH 04/10] sbc: Add msbc flag and generic C primitive
    [PATCH 05/10] sbc: Add support for mSBC frame header
we already have a complete mSBC support in C code, which is still
unusable (as in buggy) if run on mmx, iwmmxt, armv6 or arm neon capable
systems.

And having another look at it, we actually may have one more problem.
The sbc-1.0 library is already starting to get packaged in some linux
distributions. If somebody tries to run some application to do mSBC
encoding/decoding, but happens to have an old sbc-1.0 library in his
system, then it would not be able to handle the new SBC flag
gracefully. The comment about returning error code "-3 Unsupported
number of blocks" is not quite true after
    http://git.kernel.org/?p=bluetooth/bluez.git;a=commit;h=ce342bf2524b

Having a proper standalone sbc library, we might want to be nicer to the
users and minimize any possible ABI related issues on the upgrade path.
>From this point of view, even introducing new msbc_encode/msbc_decode
functions might be safer. We may not go so far for the first library
update (after all, what would be the purpose of the flags argument in
this case?). But eventually making error checking code more sane
certainly looks like a good idea to me.

Regarding ABI stability in general, there is an interesting project,
which is tracking many open source libraries including bluez:
    http://upstream-tracker.org/versions/bluez.html

-- 
Best regards,
Siarhei Siamashka

Re: [PATCH v3 06/10] sbc: Add mmx primitive for 1b 8s analyse

[Dalleau, Frederic]

Hi Siarhei,

On Tue, Oct 30, 2012 at 2:46 AM, Siarhei Siamashka
<siarhei.siamashka@gmail.com> wrote:
> On Mon, 29 Oct 2012 08:42:08 +0100
> "Dalleau, Frederic" <frederic.dalleau@intel.com> wrote:

> So I still think that it is safer and cleaner to have
> "sbc_init_primitives" function performing the following in order:
> 1. Initialize function pointers with C implementations.
> 2. Allow to override them with various platform specific
> implementations (which would work fine for old SBC formats).
> 3. At the end of the function have a check if we are actually dealing
> with mSBC format and restore all the function pointers back to C
> implementations in this case.

That's only ok for simd, but as soon as mmx gets in there would be
a 4th category where implementation supporting msbc would override
the already defined pointer.

Imho, the definitive clean solution is to add the missing primitives.

> Well, I just don't quite like that after the patches
>     [PATCH 04/10] sbc: Add msbc flag and generic C primitive
>     [PATCH 05/10] sbc: Add support for mSBC frame header
> we already have a complete mSBC support in C code, which is still
> unusable (as in buggy) if run on mmx, iwmmxt, armv6 or arm neon capable
> systems.

This is experimental feature and the time of commit, it is unused.
And btw, you just suggested to split this patch, and this will also lead to a
transitional state where some new functionality can be broken.

> The sbc-1.0 library is already starting to get packaged in some linux
> distributions. If somebody tries to run some application to do mSBC
> encoding/decoding, but happens to have an old sbc-1.0 library in his

I may be mistaking but most projects uses sbc statically, so checking is mostly
done at compile time. So as I often hear in open source : we'll cross the bridge
when we get to it :)

Regards,
Frédéric

[PATCH v3 07/10] sbc: Update sbcdec for msbc

[Frédéric Dalleau]

---
 src/sbcdec.c |   17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/src/sbcdec.c b/src/sbcdec.c
index 0077a82..37d2e98 100644
--- a/src/sbcdec.c
+++ b/src/sbcdec.c
@@ -44,7 +44,7 @@
 
 static int verbose = 0;
 
-static void decode(char *filename, char *output, int tofile)
+static void decode(char *filename, char *output, int tofile, int msbc)
 {
 	unsigned char buf[BUF_SIZE], *stream;
 	struct stat st;
@@ -98,7 +98,7 @@ static void decode(char *filename, char *output, int tofile)
 		goto free;
 	}
 
-	sbc_init(&sbc, 0L);
+	sbc_init(&sbc, msbc ? SBC_MSBC : 0L);
 	sbc.endian = SBC_BE;
 
 	framelen = sbc_decode(&sbc, stream, streamlen, buf, sizeof(buf), &len);
@@ -228,14 +228,16 @@ static void usage(void)
 
 	printf("Options:\n"
 		"\t-h, --help           Display help\n"
-		"\t-v, --verbose        Verbose mode\n"
 		"\t-d, --device <dsp>   Sound device\n"
+		"\t-v, --verbose        Verbose mode\n"
+		"\t-m, --msbc           mSBC codec\n"
 		"\t-f, --file <file>    Decode to a file\n"
 		"\n");
 }
 
 static struct option main_options[] = {
 	{ "help",	0, 0, 'h' },
+	{ "msbc",	0, 0, 'm' },
 	{ "device",	1, 0, 'd' },
 	{ "verbose",	0, 0, 'v' },
 	{ "file",	1, 0, 'f' },
@@ -246,8 +248,9 @@ int main(int argc, char *argv[])
 {
 	char *output = NULL;
 	int i, opt, tofile = 0;
+	int msbc = 0;
 
-	while ((opt = getopt_long(argc, argv, "+hvd:f:",
+	while ((opt = getopt_long(argc, argv, "+hmvd:f:",
 						main_options, NULL)) != -1) {
 		switch(opt) {
 		case 'h':
@@ -258,6 +261,10 @@ int main(int argc, char *argv[])
 			verbose = 1;
 			break;
 
+		case 'm':
+			msbc = 1;
+			break;
+
 		case 'd':
 			free(output);
 			output = strdup(optarg);
@@ -285,7 +292,7 @@ int main(int argc, char *argv[])
 	}
 
 	for (i = 0; i < argc; i++)
-		decode(argv[i], output ? output : "/dev/dsp", tofile);
+		decode(argv[i], output ? output : "/dev/dsp", tofile, msbc);
 
 	free(output);
 
-- 
1.7.9.5

[PATCH v3 08/10] sbc: Update sbcenc for msbc

[Frédéric Dalleau]

---
 src/sbcenc.c |   25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/src/sbcenc.c b/src/sbcenc.c
index a723b03..71ad6bb 100644
--- a/src/sbcenc.c
+++ b/src/sbcenc.c
@@ -45,7 +45,7 @@ static int verbose = 0;
 static unsigned char input[BUF_SIZE], output[BUF_SIZE + BUF_SIZE / 4];
 
 static void encode(char *filename, int subbands, int bitpool, int joint,
-					int dualchannel, int snr, int blocks)
+				int dualchannel, int snr, int blocks, int msbc)
 {
 	struct au_header au_hdr;
 	sbc_t sbc;
@@ -87,7 +87,7 @@ static void encode(char *filename, int subbands, int bitpool, int joint,
 		goto done;
 	}
 
-	sbc_init(&sbc, 0L);
+	sbc_init(&sbc, msbc ? SBC_MSBC : 0L);
 
 	switch (BE_INT(au_hdr.sample_rate)) {
 	case 16000:
@@ -215,6 +215,7 @@ static void usage(void)
 	printf("Options:\n"
 		"\t-h, --help           Display help\n"
 		"\t-v, --verbose        Verbose mode\n"
+		"\t-m, --msbc           mSBC codec\n"
 		"\t-s, --subbands       Number of subbands to use (4 or 8)\n"
 		"\t-b, --bitpool        Bitpool value (default is 32)\n"
 		"\t-j, --joint          Joint stereo\n"
@@ -227,6 +228,7 @@ static void usage(void)
 static struct option main_options[] = {
 	{ "help",	0, 0, 'h' },
 	{ "verbose",	0, 0, 'v' },
+	{ "msbc",	0, 0, 'm' },
 	{ "subbands",	1, 0, 's' },
 	{ "bitpool",	1, 0, 'b' },
 	{ "joint",	0, 0, 'j' },
@@ -239,9 +241,9 @@ static struct option main_options[] = {
 int main(int argc, char *argv[])
 {
 	int i, opt, subbands = 8, bitpool = 32, joint = 0, dualchannel = 0;
-	int snr = 0, blocks = 16;
+	int snr = 0, blocks = 16, msbc = 0;
 
-	while ((opt = getopt_long(argc, argv, "+hvs:b:jdSB:",
+	while ((opt = getopt_long(argc, argv, "+hmvs:b:jdSB:",
 						main_options, NULL)) != -1) {
 		switch(opt) {
 		case 'h':
@@ -252,6 +254,10 @@ int main(int argc, char *argv[])
 			verbose = 1;
 			break;
 
+		case 'm':
+			msbc = 1;
+			break;
+
 		case 's':
 			subbands = atoi(optarg);
 			if (subbands != 8 && subbands != 4) {
@@ -300,9 +306,18 @@ int main(int argc, char *argv[])
 		exit(1);
 	}
 
+	if (msbc) {
+		subbands = 8;
+		bitpool = 26;
+		joint = 0;
+		dualchannel = 0;
+		snr = 0;
+		blocks = 15;
+	}
+
 	for (i = 0; i < argc; i++)
 		encode(argv[i], subbands, bitpool, joint, dualchannel,
-								snr, blocks);
+							snr, blocks, msbc);
 
 	return 0;
 }
-- 
1.7.9.5

[PATCH v3 09/10] sbc: Update sbcinfo for msbc

[Frédéric Dalleau]

---
 src/sbcinfo.c |   51 ++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 36 insertions(+), 15 deletions(-)

diff --git a/src/sbcinfo.c b/src/sbcinfo.c
index 8cfb54a..01426b8 100644
--- a/src/sbcinfo.c
+++ b/src/sbcinfo.c
@@ -61,12 +61,11 @@ struct sbc_frame_hdr {
 #error "Unknown byte order"
 #endif
 
-static int calc_frame_len(struct sbc_frame_hdr *hdr)
+static int calc_frame_len(struct sbc_frame_hdr *hdr, int nrof_blocks)
 {
-	int tmp, nrof_subbands, nrof_blocks;
+	int tmp, nrof_subbands;
 
 	nrof_subbands = (hdr->subbands + 1) * 4;
-	nrof_blocks = (hdr->blocks + 1) * 4;
 
 	switch (hdr->channel_mode) {
 	case 0x00:
@@ -89,13 +88,12 @@ static int calc_frame_len(struct sbc_frame_hdr *hdr)
 	return (nrof_subbands + ((tmp + 7) / 8));
 }
 
-static double calc_bit_rate(struct sbc_frame_hdr *hdr)
+static double calc_bit_rate(struct sbc_frame_hdr *hdr, int nrof_blocks)
 {
-	int nrof_subbands, nrof_blocks;
+	int nrof_subbands;
 	double f;
 
 	nrof_subbands = (hdr->subbands + 1) * 4;
-	nrof_blocks = (hdr->blocks + 1) * 4;
 
 	switch (hdr->sampling_frequency) {
 	case 0:
@@ -114,7 +112,7 @@ static double calc_bit_rate(struct sbc_frame_hdr *hdr)
 		return 0;
 	}
 
-	return ((8 * (calc_frame_len(hdr) + 4) * f) /
+	return ((8 * (calc_frame_len(hdr, nrof_blocks) + 4) * f) /
 			(nrof_subbands * nrof_blocks));
 }
 
@@ -175,7 +173,7 @@ static int analyze_file(char *filename)
 	double rate;
 	int bitpool[SIZE], frame_len[SIZE];
 	int subbands, blocks, freq, method;
-	int n, p1, p2, fd, size, num;
+	int n, p1, p2, fd, size, num, msbc;
 	ssize_t len;
 	unsigned int count;
 
@@ -191,17 +189,30 @@ static int analyze_file(char *filename)
 		fd = fileno(stdin);
 
 	len = __read(fd, &hdr, sizeof(hdr));
-	if (len != sizeof(hdr) || hdr.syncword != 0x9c) {
+	if (len != sizeof(hdr) || !(hdr.syncword == 0x9c ||
+			hdr.syncword == 0xad)) {
 		fprintf(stderr, "Not a SBC audio file\n");
 		return -1;
 	}
+	msbc = (hdr.syncword == 0xad) ? 1 : 0;
+
+	if (msbc) {
+		hdr.subbands = 1; /* 8 */
+		hdr.sampling_frequency = 0x00; /* 16000 */
+		hdr.allocation_method = 0; /* Loudness */
+		hdr.bitpool = 26;
+		hdr.channel_mode = 0x00; /* Mono */
+
+		blocks = 15;
+	} else {
+		blocks = (hdr.blocks + 1) * 4;
+	}
 
 	subbands = (hdr.subbands + 1) * 4;
-	blocks = (hdr.blocks + 1) * 4;
 	freq = hdr.sampling_frequency;
 	method = hdr.allocation_method;
 
-	count = calc_frame_len(&hdr);
+	count = calc_frame_len(&hdr, blocks);
 
 	bitpool[0] = hdr.bitpool;
 	frame_len[0] = count + 4;
@@ -213,7 +224,7 @@ static int analyze_file(char *filename)
 
 	if (lseek(fd, 0, SEEK_SET) < 0) {
 		num = 1;
-		rate = calc_bit_rate(&hdr);
+		rate = calc_bit_rate(&hdr, blocks);
 		while (count) {
 			size = count > sizeof(buf) ? sizeof(buf) : count;
 			len = __read(fd, buf, size);
@@ -237,14 +248,23 @@ static int analyze_file(char *filename)
 		if (len == 0)
 			break;
 
-		if ((size_t) len < sizeof(hdr) || hdr.syncword != 0x9c) {
+		if ((size_t) len < sizeof(hdr) || !(hdr.syncword == 0x9c ||
+				hdr.syncword == 0xad)) {
 			fprintf(stderr, "Corrupted SBC stream "
 					"(len %zd syncword 0x%02x)\n",
 					len, hdr.syncword);
 			break;
 		}
 
-		count = calc_frame_len(&hdr);
+		if (msbc) {
+			hdr.subbands = 1; /* 8 */
+			hdr.sampling_frequency = 0x00; /* 16000 */
+			hdr.allocation_method = 0; /* Loudness */
+			hdr.bitpool = 26;
+			hdr.channel_mode = 0x00; /* Mono */
+		}
+
+		count = calc_frame_len(&hdr, blocks);
 		len = count + 4;
 
 		p1 = -1;
@@ -273,10 +293,11 @@ static int analyze_file(char *filename)
 			count -= len;
 		}
 
-		rate += calc_bit_rate(&hdr);
+		rate += calc_bit_rate(&hdr, blocks);
 		num++;
 	}
 
+	printf("mSBC\t\t\t%d\n", msbc);
 	printf("Subbands\t\t%d\n", subbands);
 	printf("Block length\t\t%d\n", blocks);
 	printf("Sampling frequency\t%s\n", freq2str(freq));
-- 
1.7.9.5

[PATCH v3 10/10] sbc: Update copyrights

[Frédéric Dalleau]

---
 sbc/sbc.c                |    1 +
 sbc/sbc_primitives.c     |    1 +
 sbc/sbc_primitives.h     |    1 +
 sbc/sbc_primitives_mmx.c |    1 +
 src/sbcdec.c             |    1 +
 src/sbcenc.c             |    1 +
 src/sbcinfo.c            |    1 +
 7 files changed, 7 insertions(+)

diff --git a/sbc/sbc.c b/sbc/sbc.c
index 8236122..9a6445a 100644
--- a/sbc/sbc.c
+++ b/sbc/sbc.c
@@ -6,6 +6,7 @@
  *  Copyright (C) 2004-2010  Marcel Holtmann <marcel@holtmann.org>
  *  Copyright (C) 2004-2005  Henryk Ploetz <henryk@ploetzli.ch>
  *  Copyright (C) 2005-2008  Brad Midgley <bmidgley@xmission.com>
+ *  Copyright (C) 2012       Intel Corporation
  *
  *
  *  This library is free software; you can redistribute it and/or
diff --git a/sbc/sbc_primitives.c b/sbc/sbc_primitives.c
index 47caf11..4317586 100644
--- a/sbc/sbc_primitives.c
+++ b/sbc/sbc_primitives.c
@@ -6,6 +6,7 @@
  *  Copyright (C) 2004-2010  Marcel Holtmann <marcel@holtmann.org>
  *  Copyright (C) 2004-2005  Henryk Ploetz <henryk@ploetzli.ch>
  *  Copyright (C) 2005-2006  Brad Midgley <bmidgley@xmission.com>
+ *  Copyright (C) 2012       Intel Corporation
  *
  *
  *  This library is free software; you can redistribute it and/or
diff --git a/sbc/sbc_primitives.h b/sbc/sbc_primitives.h
index 9a27d3c..0043e89 100644
--- a/sbc/sbc_primitives.h
+++ b/sbc/sbc_primitives.h
@@ -6,6 +6,7 @@
  *  Copyright (C) 2004-2010  Marcel Holtmann <marcel@holtmann.org>
  *  Copyright (C) 2004-2005  Henryk Ploetz <henryk@ploetzli.ch>
  *  Copyright (C) 2005-2006  Brad Midgley <bmidgley@xmission.com>
+ *  Copyright (C) 2012       Intel Corporation
  *
  *
  *  This library is free software; you can redistribute it and/or
diff --git a/sbc/sbc_primitives_mmx.c b/sbc/sbc_primitives_mmx.c
index 17065e5..7733b6b 100644
--- a/sbc/sbc_primitives_mmx.c
+++ b/sbc/sbc_primitives_mmx.c
@@ -6,6 +6,7 @@
  *  Copyright (C) 2004-2010  Marcel Holtmann <marcel@holtmann.org>
  *  Copyright (C) 2004-2005  Henryk Ploetz <henryk@ploetzli.ch>
  *  Copyright (C) 2005-2006  Brad Midgley <bmidgley@xmission.com>
+ *  Copyright (C) 2012       Intel Corporation
  *
  *
  *  This library is free software; you can redistribute it and/or
diff --git a/src/sbcdec.c b/src/sbcdec.c
index 37d2e98..908292c 100644
--- a/src/sbcdec.c
+++ b/src/sbcdec.c
@@ -4,6 +4,7 @@
  *
  *  Copyright (C) 2008-2010  Nokia Corporation
  *  Copyright (C) 2004-2010  Marcel Holtmann <marcel@holtmann.org>
+ *  Copyright (C) 2012       Intel Corporation
  *
  *
  *  This program is free software; you can redistribute it and/or modify
diff --git a/src/sbcenc.c b/src/sbcenc.c
index 71ad6bb..0156b74 100644
--- a/src/sbcenc.c
+++ b/src/sbcenc.c
@@ -4,6 +4,7 @@
  *
  *  Copyright (C) 2008-2010  Nokia Corporation
  *  Copyright (C) 2004-2010  Marcel Holtmann <marcel@holtmann.org>
+ *  Copyright (C) 2012       Intel Corporation
  *
  *
  *  This program is free software; you can redistribute it and/or modify
diff --git a/src/sbcinfo.c b/src/sbcinfo.c
index 01426b8..676b949 100644
--- a/src/sbcinfo.c
+++ b/src/sbcinfo.c
@@ -4,6 +4,7 @@
  *
  *  Copyright (C) 2008-2010  Nokia Corporation
  *  Copyright (C) 2004-2010  Marcel Holtmann <marcel@holtmann.org>
+ *  Copyright (C) 2012       Intel Corporation
  *
  *
  *  This program is free software; you can redistribute it and/or modify
-- 
1.7.9.5