[PATCH v4 00/16] mSBC tests

[PATCH v4 00/16] mSBC tests

[Frédéric Dalleau]

Hi,

v4 integrate latest comments from Siarhei:
 - remove state->pending field from sbc_encoder_state
 - add armv6 and iwmmxt primitives
 - use simd primitive instead of neon
 - reorder patches so that the SBC_MSBC flag is exposed to users only when
implementation is complete.

I could only build armv6 code.

Regards,
Frederic


Frédéric Dalleau (16):
  sbc: Add encoder_state to analysis functions
  sbc: Break 4 blocks processing to variable steps
  sbc: Rename sbc_analyze_4b_xx to sbc_analyze_xx
  sbc: add odd member variable to sbc_encoder_state
  sbc: Add mmx primitive for 1b 8s analysis
  sbc: Add armv6 primitive for 1b 8s analysis
  sbc: Add iwmmxt primitive for 1b 8s encoding
  sbc: Add simd primitive for 1b 8s analysis
  sbc: Use simd primitive if doing msbc on neon
  sbc: simd support for 8 multiples block size
  sbc: Add SBC_MSBC flag to enable 15 block encoding
  sbc: Add support for mSBC frame header
  sbc: Update sbcdec for msbc
  sbc: Update sbcenc for msbc
  sbc: Update sbcinfo for msbc
  sbc: Update copyrights

 sbc/sbc.c                   |  277 ++++++++++++++++++++++++++-----------------
 sbc/sbc.h                   |    3 +
 sbc/sbc_primitives.c        |   71 +++++++++--
 sbc/sbc_primitives.h        |   14 ++-
 sbc/sbc_primitives_armv6.c  |   26 +++-
 sbc/sbc_primitives_iwmmxt.c |   28 ++++-
 sbc/sbc_primitives_mmx.c    |   29 ++++-
 sbc/sbc_primitives_neon.c   |   12 +-
 src/sbcdec.c                |   18 ++-
 src/sbcenc.c                |   26 +++-
 src/sbcinfo.c               |   52 +++++---
 11 files changed, 389 insertions(+), 167 deletions(-)

-- 
1.7.9.5

[PATCH v4 01/16] 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 f0c77c7..e51ed57 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 ad780d0..f8cc4b6 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 17ad4f7..a7bbef1 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)(int position,
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 5d4d0e3..aeca8df 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 v4 02/16] 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            |   19 +++++++++++--------
 sbc/sbc_primitives.h |    6 ++++--
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/sbc/sbc.c b/sbc/sbc.c
index e51ed57..4a7dd2e 100644
--- a/sbc/sbc.c
+++ b/sbc/sbc.c
@@ -688,30 +688,32 @@ 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 +
-							frame->blocks * 4];
-			for (blk = 0; blk < frame->blocks; blk += 4) {
+			x = &state->X[ch][state->position - 4 *
+					state->increment + frame->blocks * 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 +
-							frame->blocks * 8];
-			for (blk = 0; blk < frame->blocks; blk += 4) {
+			x = &state->X[ch][state->position - 8 *
+					state->increment + frame->blocks * 8];
+			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 +908,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 a7bbef1..2a7d4a1 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 v4 03/16] sbc: Rename sbc_analyze_4b_xx to sbc_analyze_xx

[Frédéric Dalleau]

---
 sbc/sbc.c                   |    4 ++--
 sbc/sbc_primitives.c        |    4 ++--
 sbc/sbc_primitives.h        |    4 ++--
 sbc/sbc_primitives_armv6.c  |    4 ++--
 sbc/sbc_primitives_iwmmxt.c |    4 ++--
 sbc/sbc_primitives_mmx.c    |    4 ++--
 sbc/sbc_primitives_neon.c   |    4 ++--
 7 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/sbc/sbc.c b/sbc/sbc.c
index 4a7dd2e..ffdf05d 100644
--- a/sbc/sbc.c
+++ b/sbc/sbc.c
@@ -692,7 +692,7 @@ static int sbc_analyze_audio(struct sbc_encoder_state *state,
 					state->increment + frame->blocks * 4];
 			for (blk = 0; blk < frame->blocks;
 						blk += state->increment) {
-				state->sbc_analyze_4b_4s(
+				state->sbc_analyze_4s(
 					state, x,
 					frame->sb_sample_f[blk][ch],
 					frame->sb_sample_f[blk + 1][ch] -
@@ -708,7 +708,7 @@ static int sbc_analyze_audio(struct sbc_encoder_state *state,
 					state->increment + frame->blocks * 8];
 			for (blk = 0; blk < frame->blocks;
 						blk += state->increment) {
-				state->sbc_analyze_4b_8s(
+				state->sbc_analyze_8s(
 					state, x,
 					frame->sb_sample_f[blk][ch],
 					frame->sb_sample_f[blk + 1][ch] -
diff --git a/sbc/sbc_primitives.c b/sbc/sbc_primitives.c
index f8cc4b6..dce0ed2 100644
--- a/sbc/sbc_primitives.c
+++ b/sbc/sbc_primitives.c
@@ -522,8 +522,8 @@ static int sbc_calc_scalefactors_j(
 void sbc_init_primitives(struct sbc_encoder_state *state)
 {
 	/* 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;
+	state->sbc_analyze_4s = sbc_analyze_4b_4s_simd;
+	state->sbc_analyze_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 2a7d4a1..267c277 100644
--- a/sbc/sbc_primitives.h
+++ b/sbc/sbc_primitives.h
@@ -43,11 +43,11 @@ struct sbc_encoder_state {
 	int16_t SBC_ALIGNED X[2][SBC_X_BUFFER_SIZE];
 	/* Polyphase analysis filter for 4 subbands configuration,
 	 * it handles "increment" blocks at once */
-	void (*sbc_analyze_4b_4s)(struct sbc_encoder_state *state,
+	void (*sbc_analyze_4s)(struct sbc_encoder_state *state,
 			int16_t *x, int32_t *out, int out_stride);
 	/* Polyphase analysis filter for 8 subbands configuration,
 	 * it handles "increment" blocks at once */
-	void (*sbc_analyze_4b_8s)(struct sbc_encoder_state *state,
+	void (*sbc_analyze_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 */
diff --git a/sbc/sbc_primitives_armv6.c b/sbc/sbc_primitives_armv6.c
index 6ad94c6..068648a 100644
--- a/sbc/sbc_primitives_armv6.c
+++ b/sbc/sbc_primitives_armv6.c
@@ -293,8 +293,8 @@ static void sbc_analyze_4b_8s_armv6(struct sbc_encoder_state *state,
 
 void sbc_init_primitives_armv6(struct sbc_encoder_state *state)
 {
-	state->sbc_analyze_4b_4s = sbc_analyze_4b_4s_armv6;
-	state->sbc_analyze_4b_8s = sbc_analyze_4b_8s_armv6;
+	state->sbc_analyze_4s = sbc_analyze_4b_4s_armv6;
+	state->sbc_analyze_8s = sbc_analyze_4b_8s_armv6;
 	state->implementation_info = "ARMv6 SIMD";
 }
 
diff --git a/sbc/sbc_primitives_iwmmxt.c b/sbc/sbc_primitives_iwmmxt.c
index 39cc390..0c8f329 100644
--- a/sbc/sbc_primitives_iwmmxt.c
+++ b/sbc/sbc_primitives_iwmmxt.c
@@ -296,8 +296,8 @@ static inline void sbc_analyze_4b_8s_iwmmxt(struct sbc_encoder_state *state,
 
 void sbc_init_primitives_iwmmxt(struct sbc_encoder_state *state)
 {
-	state->sbc_analyze_4b_4s = sbc_analyze_4b_4s_iwmmxt;
-	state->sbc_analyze_4b_8s = sbc_analyze_4b_8s_iwmmxt;
+	state->sbc_analyze_4s = sbc_analyze_4b_4s_iwmmxt;
+	state->sbc_analyze_8s = sbc_analyze_4b_8s_iwmmxt;
 	state->implementation_info = "IWMMXT";
 }
 
diff --git a/sbc/sbc_primitives_mmx.c b/sbc/sbc_primitives_mmx.c
index cbacb4e..03070f5 100644
--- a/sbc/sbc_primitives_mmx.c
+++ b/sbc/sbc_primitives_mmx.c
@@ -365,8 +365,8 @@ static int check_mmx_support(void)
 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;
+		state->sbc_analyze_4s = sbc_analyze_4b_4s_mmx;
+		state->sbc_analyze_8s = sbc_analyze_4b_8s_mmx;
 		state->sbc_calc_scalefactors = sbc_calc_scalefactors_mmx;
 		state->implementation_info = "MMX";
 	}
diff --git a/sbc/sbc_primitives_neon.c b/sbc/sbc_primitives_neon.c
index aeca8df..eda4ed3 100644
--- a/sbc/sbc_primitives_neon.c
+++ b/sbc/sbc_primitives_neon.c
@@ -879,8 +879,8 @@ static int sbc_enc_process_input_8s_le_neon(int position, const uint8_t *pcm,
 
 void sbc_init_primitives_neon(struct sbc_encoder_state *state)
 {
-	state->sbc_analyze_4b_4s = sbc_analyze_4b_4s_neon;
-	state->sbc_analyze_4b_8s = sbc_analyze_4b_8s_neon;
+	state->sbc_analyze_4s = sbc_analyze_4b_4s_neon;
+	state->sbc_analyze_8s = sbc_analyze_4b_8s_neon;
 	state->sbc_calc_scalefactors = sbc_calc_scalefactors_neon;
 	state->sbc_calc_scalefactors_j = sbc_calc_scalefactors_j_neon;
 	state->sbc_enc_process_input_4s_le = sbc_enc_process_input_4s_le_neon;
-- 
1.7.9.5

[PATCH v4 04/16] sbc: add odd member variable to sbc_encoder_state

[Frédéric Dalleau]

---
 sbc/sbc_primitives.c |    2 ++
 sbc/sbc_primitives.h |    1 +
 2 files changed, 3 insertions(+)

diff --git a/sbc/sbc_primitives.c b/sbc/sbc_primitives.c
index dce0ed2..0b48ddb 100644
--- a/sbc/sbc_primitives.c
+++ b/sbc/sbc_primitives.c
@@ -521,6 +521,8 @@ static int sbc_calc_scalefactors_j(
  */
 void sbc_init_primitives(struct sbc_encoder_state *state)
 {
+	state->odd = 1;
+
 	/* Default implementation for analyze functions */
 	state->sbc_analyze_4s = sbc_analyze_4b_4s_simd;
 	state->sbc_analyze_8s = sbc_analyze_4b_8s_simd;
diff --git a/sbc/sbc_primitives.h b/sbc/sbc_primitives.h
index 267c277..ffee339 100644
--- a/sbc/sbc_primitives.h
+++ b/sbc/sbc_primitives.h
@@ -40,6 +40,7 @@ struct sbc_encoder_state {
 	int position;
 	/* Number of consecutive blocks handled by the encoder */
 	int increment;
+	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

[PATCH v4 05/16] sbc: Add mmx primitive for 1b 8s analysis

[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 03070f5..21d7b74 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_4s = sbc_analyze_4b_4s_mmx;
-		state->sbc_analyze_8s = sbc_analyze_4b_8s_mmx;
+		if (state->increment == 1)
+			state->sbc_analyze_8s = sbc_analyze_1b_8s_mmx;
+		else
+			state->sbc_analyze_8s = sbc_analyze_4b_8s_mmx;
 		state->sbc_calc_scalefactors = sbc_calc_scalefactors_mmx;
 		state->implementation_info = "MMX";
 	}
-- 
1.7.9.5

Re: [PATCH v4 05/16] sbc: Add mmx primitive for 1b 8s analysis

[Frédéric Dalleau]

Hi,

Since I'm gonna resend a new series, I'll comment myself ;)

On 10/30/2012 10:39 AM, Frédéric Dalleau wrote:
> +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");
> +}
> +

One thing bother me about this patch : the emms instruction is called
after every block, instead of every four blocks until now. I have very
little knowledge about this, but I read that emms instruction is
somewhat expensive.
Some quick tests haven't shown differences, but it is possible to add a
post analyze callback to overcome this. In this case, emms instruction
could be run every 15 blocks or whatever is defined.

Is it worth it?

Thanks,
Frédéric

Re: [PATCH v4 05/16] sbc: Add mmx primitive for 1b 8s analysis

[Siarhei Siamashka]

On Wed, 14 Nov 2012 16:43:44 +0100
Frédéric Dalleau  <frederic.dalleau@linux.intel.com> wrote:

> Hi,
> 
> Since I'm gonna resend a new series, I'll comment myself ;)
> 
> On 10/30/2012 10:39 AM, Frédéric Dalleau wrote:
> > +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");
> > +}
> > +
> 
> One thing bother me about this patch : the emms instruction is called
> after every block, instead of every four blocks until now. I have very
> little knowledge about this, but I read that emms instruction is
> somewhat expensive.
> Some quick tests haven't shown differences, but it is possible to add a
> post analyze callback to overcome this. In this case, emms instruction
> could be run every 15 blocks or whatever is defined.

The EMMS instruction must be used after the use of MMX instructions,
otherwise the subsequent floating point calculations are broken.

As part of calling conventions, FPU state must be clear after returning
from any function:
    http://www.agner.org/optimize/calling_conventions.pdf
It means that normally every MMX function needs to execute EMMS
instruction before returning. We were already cutting the corners a bit
by putting MMX code into static inline functions which don't have
EMMS themselves. But using the post analyze callback would be really
wrong as that's going to explicitly cross function boundaries with
inconsistent FPU state.

> 
> Is it worth it?

If benchmarks do not show a significant performance drop, then it does
not really matter. A minor performance regression is fine, as long as
the MMX code is still significantly faster than C.

Nowadays using SSE2 is a much better idea. And SSE2 does not suffer
from EMMS-alike warts.

-- 
Best regards,
Siarhei Siamashka

[PATCH v4 06/16] sbc: Add armv6 primitive for 1b 8s analysis

[Frédéric Dalleau]

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

diff --git a/sbc/sbc_primitives_armv6.c b/sbc/sbc_primitives_armv6.c
index 068648a..a2cb2a8 100644
--- a/sbc/sbc_primitives_armv6.c
+++ b/sbc/sbc_primitives_armv6.c
@@ -291,10 +291,26 @@ static void sbc_analyze_4b_8s_armv6(struct sbc_encoder_state *state,
 	sbc_analyze_eight(x + 0, out, analysis_consts_fixed8_simd_even);
 }
 
+static void sbc_analyze_1b_8s_armv6(struct sbc_encoder_state *state,
+		int16_t *x, int32_t *out, int out_stride)
+{
+	if (state->odd)
+		sbc_analyze_eight_armv6(x, out,
+					analysis_consts_fixed8_simd_odd);
+	else
+		sbc_analyze_eight_armv6(x, out,
+					analysis_consts_fixed8_simd_even);
+
+	state->odd = !state->odd;
+}
+
 void sbc_init_primitives_armv6(struct sbc_encoder_state *state)
 {
 	state->sbc_analyze_4s = sbc_analyze_4b_4s_armv6;
-	state->sbc_analyze_8s = sbc_analyze_4b_8s_armv6;
+	if (state->increment == 1)
+		state->sbc_analyze_8s = sbc_analyze_1b_8s_armv6;
+	else
+		state->sbc_analyze_8s = sbc_analyze_4b_8s_armv6;
 	state->implementation_info = "ARMv6 SIMD";
 }
 
-- 
1.7.9.5

[PATCH v4 07/16] sbc: Add iwmmxt primitive for 1b 8s encoding

[Frédéric Dalleau]

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

diff --git a/sbc/sbc_primitives_iwmmxt.c b/sbc/sbc_primitives_iwmmxt.c
index 0c8f329..44bfd47 100644
--- a/sbc/sbc_primitives_iwmmxt.c
+++ b/sbc/sbc_primitives_iwmmxt.c
@@ -294,10 +294,26 @@ static inline void sbc_analyze_4b_8s_iwmmxt(struct sbc_encoder_state *state,
 	sbc_analyze_eight_iwmmxt(x + 0, out, analysis_consts_fixed8_simd_even);
 }
 
+static inline void sbc_analyze_1b_8s_iwmmxt(struct sbc_encoder_state *state,
+		int16_t *x, int32_t *out, int out_stride)
+{
+	if (state->odd)
+		sbc_analyze_eight_iwmmxt(x, out,
+					analysis_consts_fixed8_simd_odd);
+	else
+		sbc_analyze_eight_iwmmxt(x, out,
+					analysis_consts_fixed8_simd_even);
+
+	state->odd = !state->odd;
+}
+
 void sbc_init_primitives_iwmmxt(struct sbc_encoder_state *state)
 {
 	state->sbc_analyze_4s = sbc_analyze_4b_4s_iwmmxt;
-	state->sbc_analyze_8s = sbc_analyze_4b_8s_iwmmxt;
+	if (state->increment == 1)
+		state->sbc_analyze_8s = sbc_analyze_1b_8s_iwmmxt;
+	else
+		state->sbc_analyze_8s = sbc_analyze_4b_8s_iwmmxt;
 	state->implementation_info = "IWMMXT";
 }
 
-- 
1.7.9.5

[PATCH v4 08/16] sbc: Add simd primitive for 1b 8s analysis

[Frédéric Dalleau]

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

diff --git a/sbc/sbc_primitives.c b/sbc/sbc_primitives.c
index 0b48ddb..a4767ef 100644
--- a/sbc/sbc_primitives.c
+++ b/sbc/sbc_primitives.c
@@ -209,6 +209,19 @@ 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]);
@@ -525,7 +538,10 @@ void sbc_init_primitives(struct sbc_encoder_state *state)
 
 	/* Default implementation for analyze functions */
 	state->sbc_analyze_4s = sbc_analyze_4b_4s_simd;
-	state->sbc_analyze_8s = sbc_analyze_4b_8s_simd;
+	if (state->increment == 1)
+		state->sbc_analyze_8s = sbc_analyze_1b_8s_simd;
+	else
+		state->sbc_analyze_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;
-- 
1.7.9.5

[PATCH v4 09/16] sbc: Use simd primitive if doing msbc on neon

[Frédéric Dalleau]

---
 sbc/sbc_primitives.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/sbc/sbc_primitives.c b/sbc/sbc_primitives.c
index a4767ef..9311848 100644
--- a/sbc/sbc_primitives.c
+++ b/sbc/sbc_primitives.c
@@ -568,5 +568,8 @@ void sbc_init_primitives(struct sbc_encoder_state *state)
 #endif
 #ifdef SBC_BUILD_WITH_NEON_SUPPORT
 	sbc_init_primitives_neon(state);
+
+	if (state->increment == 1)
+		state->sbc_analyze_8s = sbc_analyze_1b_8s_simd;
 #endif
 }
-- 
1.7.9.5

Re: [PATCH v4 09/16] sbc: Use simd primitive if doing msbc on neon

[Siarhei Siamashka]

On Tue, 30 Oct 2012 10:39:28 +0100
Frédéric Dalleau  <frederic.dalleau@linux.intel.com> wrote:

> ---
>  sbc/sbc_primitives.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/sbc/sbc_primitives.c b/sbc/sbc_primitives.c
> index a4767ef..9311848 100644
> --- a/sbc/sbc_primitives.c
> +++ b/sbc/sbc_primitives.c
> @@ -568,5 +568,8 @@ void sbc_init_primitives(struct sbc_encoder_state *state)
>  #endif
>  #ifdef SBC_BUILD_WITH_NEON_SUPPORT
>  	sbc_init_primitives_neon(state);
> +
> +	if (state->increment == 1)
> +		state->sbc_analyze_8s = sbc_analyze_1b_8s_simd;
>  #endif
>  }

This is not enough. As I commented earlier in
    http://permalink.gmane.org/gmane.linux.bluez.kernel/31567

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

And if we had, for example, an SSSE3 implementation of
"sbc_enc_process_input_*" functions for x86 (via PSHUFB instruction),
then it would also have the same problem.

-- 
Best regards,
Siarhei Siamashka

Re: [PATCH v4 09/16] sbc: Use simd primitive if doing msbc on neon

[Frédéric Dalleau]

Hi,
On 11/14/2012 08:27 PM, Siarhei Siamashka wrote:
> On Tue, 30 Oct 2012 10:39:28 +0100
>> +	if (state->increment == 1)
>> +		state->sbc_analyze_8s = sbc_analyze_1b_8s_simd;
>>   #endif
>>   }
>
> This is not enough. As I commented earlier in
>      http://permalink.gmane.org/gmane.linux.bluez.kernel/31567
>
> "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;"

Indeed, this is a mistake :
I wanted to use the neon analysis in conjonction with simd input processing.

Instead, the patch would look like this :
+ if (state->increment == 1) {
+ 	state->sbc_enc_process_input_8s_be = 	
+					sbc_enc_process_input_8s_be;
+	state->sbc_enc_process_input_8s_le =
+					sbc_enc_process_input_8s_le;
+ }
Damned it is 82 chars long!

Unfortunately I can't test this one. I can't even build it. I could but 
that's gonna take a lot of time.

Regards,
Frédéric

Re: [PATCH v4 09/16] sbc: Use simd primitive if doing msbc on neon

[Siarhei Siamashka]

On Thu, 15 Nov 2012 11:23:59 +0100
Frédéric Dalleau <frederic.dalleau@linux.intel.com> wrote:

> Hi,
> On 11/14/2012 08:27 PM, Siarhei Siamashka wrote:
> > On Tue, 30 Oct 2012 10:39:28 +0100
> >> +	if (state->increment == 1)
> >> +		state->sbc_analyze_8s = sbc_analyze_1b_8s_simd;
> >>   #endif
> >>   }
> >
> > This is not enough. As I commented earlier in
> >      http://permalink.gmane.org/gmane.linux.bluez.kernel/31567
> >
> > "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;"
> 
> Indeed, this is a mistake :
> I wanted to use the neon analysis in conjonction with simd input processing.
> 
> Instead, the patch would look like this :
> + if (state->increment == 1) {
> + 	state->sbc_enc_process_input_8s_be = 	
> +					sbc_enc_process_input_8s_be;
> +	state->sbc_enc_process_input_8s_le =
> +					sbc_enc_process_input_8s_le;
> + }
> Damned it is 82 chars long!
> 
> Unfortunately I can't test this one. I can't even build it. I could but 
> that's gonna take a lot of time.

I can test the patches on ARM. But what I'm asking is just to restore
*all* function pointers (not state->sbc_analyze_8s alone) to C
implementations for ARM NEON in the case if "state->increment == 1".
This minor modification should be enough.

Also building and testing is not too difficult when having an arm
crosscompiler (I believe ubuntu even has it packaged) and qemu. The
whole process may look like this:

$ apt-get install qemu-user
$ apt-get install gcc-arm-linux-gnueabihf
$ arm-linux-gnueabihf-gcc -O2 -march=armv7-a -mfpu=neon \
          -static -I. -DVERSION=\"\" \
          -o sbcenc src/sbcenc.c sbc/*.c
$ qemu-arm -cpu cortex-a8 ./sbcenc test.au > test.sbc

-- 
Best regards,
Siarhei Siamashka

[PATCH v4 10/16] sbc: simd support for 8 multiples block size

[Frédéric Dalleau]

---
 sbc/sbc_primitives.c |   37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/sbc/sbc_primitives.c b/sbc/sbc_primitives.c
index 9311848..b189320 100644
--- a/sbc/sbc_primitives.c
+++ b/sbc/sbc_primitives.c
@@ -309,8 +309,26 @@ 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 (position % 16 == 8) {
+		position -= 8;
+		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);
+		}
+		/* mSBC is designed for 1 channel */
+		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];
@@ -351,6 +369,23 @@ 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 -= 8;
+		if (nchannels > 0) {
+			int16_t *x = &X[0][position];
+			x[-7] = PCM(0 + 7 * nchannels);
+			x[1]  = PCM(0 + 3 * nchannels);
+			x[2]  = PCM(0 + 6 * nchannels);
+			x[3]  = PCM(0 + 0 * nchannels);
+			x[4]  = PCM(0 + 5 * nchannels);
+			x[5]  = PCM(0 + 1 * nchannels);
+			x[6]  = PCM(0 + 4 * nchannels);
+			x[7]  = PCM(0 + 2 * nchannels);
+		}
+		/* mSBC is designed for 1 channel */
 	}
 	#undef PCM
 
-- 
1.7.9.5

Re: [PATCH v4 10/16] sbc: simd support for 8 multiples block size

[Siarhei Siamashka]

On Tue, 30 Oct 2012 10:39:29 +0100
Frédéric Dalleau  <frederic.dalleau@linux.intel.com> wrote:

This looks good. The only potential problem is here:

    http://git.kernel.org/?p=bluetooth/sbc.git;a=blob;f=sbc/sbc_primitives.h;h=17ad4f74da4c#l31

 31 #define SBC_X_BUFFER_SIZE 328

    http://git.kernel.org/?p=bluetooth/sbc.git;a=blob;f=sbc/sbc.c;h=f0c77c7cb546#l908

908         state->position = (SBC_X_BUFFER_SIZE - frame->subbands * 9) & ~7;


    http://git.kernel.org/?p=bluetooth/sbc.git;a=blob;f=sbc/sbc_primitives.c;h=ad780d0800de#l285

285         /* handle X buffer wraparound */
286         if (position < nsamples) {
287                 if (nchannels > 0)
288                         memcpy(&X[0][SBC_X_BUFFER_SIZE - 72], &X[0][position],
289                                                         72 * sizeof(int16_t));
290                 if (nchannels > 1)
291                         memcpy(&X[1][SBC_X_BUFFER_SIZE - 72], &X[1][position],
292                                                         72 * sizeof(int16_t));
293                 position = SBC_X_BUFFER_SIZE - 72;
294         }

As we are going to use divisibility by 16 of position variable as a
way to distinguish between "even" and "odd" blocks (chunks of 8
samples) in the buffer, we need to be sure that the following
conditions are true:

1. assert((SBC_X_BUFFER_SIZE - frame->subbands * 9) & ~7 ==
           SBC_X_BUFFER_SIZE - 72);

Buffer wraparound is handled by copying a portion of still
needed old data to the initial position.

2. assert((SBC_X_BUFFER_SIZE - 72) % 16 == 0);

The initial position is divisible by 16.

3. assert((SBC_X_BUFFER_SIZE - 72) % (15 * 8) % 16 == 0);

When we handle buffer wraparound, the position before memcpy has the
same divisibility by 16 as the new updated position after memcpy.

4. assert((SBC_X_BUFFER_SIZE - 72) % (15 * 8) >= 8);

Just to be sure that "x[-7] = PCM(0 + 7 * nchannels)" does not
do invalid memory writes below the X array. This would be not
necessary if we adjust the "if (position < nsamples)" check.

If anybody decides to change SBC_X_BUFFER_SIZE constant, there is a
risk of doing it wrong. But right now the code seems to fulfil these
requirements :)


And some cosmetic nitpicks.

First the text of the summary:
1. Why "simd"? It's not totally wrong, but still way too generic.
You could use "input permutation" or "input processing" or
"sbc_encoder_process_input_s8 function" instead.
2. That's a multiple of 1 block (or alternatively a multiple of 8
samples).

> ---
>  sbc/sbc_primitives.c |   37 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/sbc/sbc_primitives.c b/sbc/sbc_primitives.c
> index 9311848..b189320 100644
> --- a/sbc/sbc_primitives.c
> +++ b/sbc/sbc_primitives.c
> @@ -309,8 +309,26 @@ 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 (position % 16 == 8) {
> +		position -= 8;
> +		nsamples -= 8;
> +		if (nchannels > 0) {

That's a somewhat redundant "if" here. Yes, it is also redundant in
the code where both "if (nchannels > 0)" and "if (nchannels > 1)" are
used, but it was there just for cosmetic reasons in order to keep the
same level of indentation.

> +			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);
> +		}
> +		/* mSBC is designed for 1 channel */

Thanks for adding this comment. But IMHO it still feels a bit
incomplete. It could be a extended to:

                /* mSBC codec is the only case when the number of
                 * samples to process may be not multiple of 16.
                 * mSBC only supports 1 channel */

Or alternatively just add back the omitted "if (nchannels > 1)" branch
too. Yes, it is not needed for mSBC now, but what if somebody introduces
something like a stereo variant of mSBC later? This way the code is
more orthogonal, "futureproof" and does not need lengthy excuses and
explanations in comments for the parts which are cutting the corners.

Either way is fine.

> +		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];
> @@ -351,6 +369,23 @@ 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 -= 8;
> +		if (nchannels > 0) {
> +			int16_t *x = &X[0][position];
> +			x[-7] = PCM(0 + 7 * nchannels);
> +			x[1]  = PCM(0 + 3 * nchannels);
> +			x[2]  = PCM(0 + 6 * nchannels);
> +			x[3]  = PCM(0 + 0 * nchannels);
> +			x[4]  = PCM(0 + 5 * nchannels);
> +			x[5]  = PCM(0 + 1 * nchannels);
> +			x[6]  = PCM(0 + 4 * nchannels);
> +			x[7]  = PCM(0 + 2 * nchannels);
> +		}
> +		/* mSBC is designed for 1 channel */

Same here.

>  	}
>  	#undef PCM


Well, none of these are really serious issues which could
immediately affect end users. But I would definitely prefer more
verbose commit messages, showing that you know what you are
doing, that the corner cases are under control, the rationale for
your decisions, etc.

-- 
Best regards,
Siarhei Siamashka

[PATCH v4 11/16] sbc: Add SBC_MSBC flag to enable 15 block encoding

[Frédéric Dalleau]

---
 sbc/sbc.c |   23 +++++++++++++++--------
 sbc/sbc.h |    3 +++
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/sbc/sbc.c b/sbc/sbc.c
index ffdf05d..22fa898 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 {
@@ -903,12 +906,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);
 }
@@ -922,6 +925,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;
@@ -1057,12 +1061,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);
@@ -1141,7 +1146,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;
@@ -1165,7 +1170,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;
@@ -1202,7 +1208,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;
 
-- 
1.7.9.5

Re: [PATCH v4 11/16] sbc: Add SBC_MSBC flag to enable 15 block encoding

[Marcel Holtmann]

Hi Fred,

> ---
>  sbc/sbc.c |   23 +++++++++++++++--------
>  sbc/sbc.h |    3 +++
>  2 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/sbc/sbc.c b/sbc/sbc.c
> index ffdf05d..22fa898 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 {
> @@ -903,12 +906,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;

just for pure cosmetics, use an if statement.

>  
>  	sbc_init_primitives(state);
>  }
> @@ -922,6 +925,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;
> @@ -1057,12 +1061,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);

Same here.

>  		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);
> @@ -1141,7 +1146,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);

And for this one as well.

>  	channels = sbc->mode == SBC_MODE_MONO ? 1 : 2;
>  	joint = sbc->mode == SBC_MODE_JOINT_STEREO ? 1 : 0;
>  	bitpool = sbc->bitpool;
> @@ -1165,7 +1170,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);

Even here it is better cosmetics.

>  	} else {
>  		subbands = priv->frame.subbands;
>  		blocks = priv->frame.blocks;
> @@ -1202,7 +1208,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);

Here as well.

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

I am debating to actually call this SBC_FLAG_MSBC instead of just
SBC_MSBC. Anyone any comments?

Regards

Marcel

Re: [PATCH v4 11/16] sbc: Add SBC_MSBC flag to enable 15 block encoding

[Frédéric Dalleau]

Hi Marcel,
On 11/14/2012 03:49 PM, Marcel Holtmann wrote:
> I am debating to actually call this SBC_FLAG_MSBC instead of just
> SBC_MSBC. Anyone any comments?

The use of the flag may not be the most elegant API. As an alternative,
maybe we could have a specific function for msbc initialization. Then,
instead of sbc_init(), sbc_encode() sbc_decode(), we could have
msbc_init(), sbc_encode(), sbc_decode().
The biggest advantage is that parameters like bitpool, subbands,
allocation, and other parts could be specified inside this function
instead of being given from the application.

Regards,
Frédéric

Re: [PATCH v4 11/16] sbc: Add SBC_MSBC flag to enable 15 block encoding

[Marcel Holtmann]

Hi Fred,

> > I am debating to actually call this SBC_FLAG_MSBC instead of just
> > SBC_MSBC. Anyone any comments?
> 
> The use of the flag may not be the most elegant API. As an alternative,
> maybe we could have a specific function for msbc initialization. Then,
> instead of sbc_init(), sbc_encode() sbc_decode(), we could have
> msbc_init(), sbc_encode(), sbc_decode().
> The biggest advantage is that parameters like bitpool, subbands,
> allocation, and other parts could be specified inside this function
> instead of being given from the application.

we could do an sbc_init_msbc() function.

As this is the only real external visible API change, we need to be 100%
sure with this one.

Regards

Marcel

[PATCH v4 12/16] 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 22fa898..cd3fd8c 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)
 {
@@ -790,38 +821,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];
@@ -889,6 +888,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(
@@ -897,6 +918,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);
@@ -906,6 +928,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)
 {
@@ -921,10 +953,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;
@@ -978,7 +1024,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);
@@ -1110,13 +1156,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 v4 13/16] 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 v4 14/16] 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 v4 15/16] 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 v4 16/16] 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 cd3fd8c..e00b256 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 b189320..0947cf5 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 ffee339..5c4c0af 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 21d7b74..02f23d9 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

Re: [PATCH v4 00/16] mSBC tests

[Frédéric Dalleau]

Hi folks, Siarhei,

On 10/30/2012 10:39 AM, Frédéric Dalleau wrote:
> Hi,
>
> v4 integrate latest comments from Siarhei:
>   - remove state->pending field from sbc_encoder_state
>   - add armv6 and iwmmxt primitives
>   - use simd primitive instead of neon
>   - reorder patches so that the SBC_MSBC flag is exposed to users only when
> implementation is complete.

Any feedback ?

Thanks,
Frédéric

Re: [PATCH v4 00/16] mSBC tests

[Marcel Holtmann]

Hi Fred,

> > v4 integrate latest comments from Siarhei:
> >   - remove state->pending field from sbc_encoder_state
> >   - add armv6 and iwmmxt primitives
> >   - use simd primitive instead of neon
> >   - reorder patches so that the SBC_MSBC flag is exposed to users only when
> > implementation is complete.
> 
> Any feedback ?

beside a tiny cosmetic comment, I have nothing. However I am not the
expert here. If Siarhei looks over this and is fine with it, I will be
as well.

Regards

Marcel

Re: [PATCH v4 00/16] mSBC tests

[Siarhei Siamashka]

On Wed, 14 Nov 2012 23:50:55 +0900
Marcel Holtmann <marcel@holtmann.org> wrote:

> Hi Fred,
> 
> > > v4 integrate latest comments from Siarhei:
> > >   - remove state->pending field from sbc_encoder_state
> > >   - add armv6 and iwmmxt primitives
> > >   - use simd primitive instead of neon
> > >   - reorder patches so that the SBC_MSBC flag is exposed to users only when
> > > implementation is complete.
> > 
> > Any feedback ?
> 
> beside a tiny cosmetic comment, I have nothing. However I am not the
> expert here. If Siarhei looks over this and is fine with it, I will be
> as well.

Hi,

My biggest concern is the way how we handle the mSBC API/ABI extension.
Because once the new version of sbc library is out, we can't
(easily/painlessly) change it any more. Everything else is fixable.

Anyway, as far as I can see, there are no regressions in the existing
SBC functionality, which is the most important requirement.

I personally would prefer a bit more verbose commit messages. But let
me know if these demands are unreasonable :)

-- 
Best regards,
Siarhei Siamashka