From: Luiz Augusto von Dentz <[email protected]>
---
sbc/sbc.c | 8 +++++---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/sbc/sbc.c b/sbc/sbc.c
index a6391ae..b3a7a09 100644
--- a/sbc/sbc.c
+++ b/sbc/sbc.c
@@ -965,7 +965,8 @@ ssize_t sbc_decode(sbc_t *sbc, const void *input, size_t input_len,
framelen = sbc_unpack_frame(input, &priv->frame, input_len);
- if (!priv->init) {
+ /* init if no previous frame was encode or bitpool has changed */
+ if (!priv->init || priv->frame.bitpool != sbc->bitpool) {
sbc_decoder_init(&priv->dec_state, &priv->frame);
priv->init = 1;
@@ -1035,7 +1036,8 @@ ssize_t sbc_encode(sbc_t *sbc, const void *input, size_t input_len,
if (written)
*written = 0;
- if (!priv->init) {
+ /* init if no previous frame was encode or bitpool has changed */
+ if (!priv->init || priv->frame.bitpool != sbc->bitpool) {
priv->frame.frequency = sbc->frequency;
priv->frame.mode = sbc->mode;
priv->frame.channels = sbc->mode == SBC_MODE_MONO ? 1 : 2;
@@ -1120,7 +1122,7 @@ size_t sbc_get_frame_length(sbc_t *sbc)
struct sbc_priv *priv;
priv = sbc->priv;
- if (priv->init)
+ if (priv->init && priv->frame.bitpool == sbc->bitpool)
return priv->frame.length;
subbands = sbc->subbands ? 8 : 4;
--
1.7.1
Hi,
On Tue, Dec 21, 2010 at 8:02 PM, Brian Gix <[email protected]> wrote:
> Hello Luiz,
>
> On Tue, 2010-12-21 at 18:58 +0200, Luiz Augusto von Dentz wrote:
>> From: Luiz Augusto von Dentz <[email protected]>
>>
>> ---
>> ?sbc/sbc.c | ? ?8 +++++---
>> ?1 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/sbc/sbc.c b/sbc/sbc.c
>> index a6391ae..b3a7a09 100644
>> --- a/sbc/sbc.c
>> +++ b/sbc/sbc.c
>> @@ -965,7 +965,8 @@ ssize_t sbc_decode(sbc_t *sbc, const void *input, size_t input_len,
>>
>> ? ? ? framelen = sbc_unpack_frame(input, &priv->frame, input_len);
>>
>> - ? ? if (!priv->init) {
>> + ? ? /* init if no previous frame was encode or bitpool has changed */
>> + ? ? if (!priv->init || priv->frame.bitpool != sbc->bitpool) {
>> ? ? ? ? ? ? ? sbc_decoder_init(&priv->dec_state, &priv->frame);
>> ? ? ? ? ? ? ? priv->init = 1;
>
> I don't think it is a good idea to reinitialize the SBC decoder if a
> change in bitpool size is detected.
>
> The A2DP and AVDTP specifications do not allow most of the parameters to
> change from one frame to the next (mode, channels, subbands, blocks,
> allocation, frequency) but explicitly DO allow changes in bitpool size
> midstream. ?This allows for Variable Bit Rate (VBR) streaming, and adds
> efficiency to the over-the-air link with no quality loss. ?Also, the
> space required for decoding does not change just because the bitpool has
> changed, so re-initialization should not be required.
>
> The only things that should be different from one SBC frame to the next
> is the bitpool of course, and the framelen. That said, if a bitpool
> change is detected the only thing within that "IF" block that should be
> updated is:
> sbc->bitpool
You are mostly right, but we should also do priv->frame.length =
framelen to make sure sbc_get_frame_length return it properly.
>
> If any of the other afore mentioned parameters change, the stream should
> be terminated.
>
>>
>> @@ -1035,7 +1036,8 @@ ssize_t sbc_encode(sbc_t *sbc, const void *input, size_t input_len,
>> ? ? ? if (written)
>> ? ? ? ? ? ? ? *written = 0;
>>
>> - ? ? if (!priv->init) {
>> + ? ? /* init if no previous frame was encode or bitpool has changed */
>> + ? ? if (!priv->init || priv->frame.bitpool != sbc->bitpool) {
>> ? ? ? ? ? ? ? priv->frame.frequency = sbc->frequency;
>> ? ? ? ? ? ? ? priv->frame.mode = sbc->mode;
>> ? ? ? ? ? ? ? priv->frame.channels = sbc->mode == SBC_MODE_MONO ? 1 : 2;
>
> Same basic comment here, although it is perfectly allowable for the
> Encoder to not support VBR. However if the Encoder does change the
> bitpool, then the list of fields that need to be updated are:
> priv->frame.bitpool
> priv->frame.length
>
> But again, the sbc_encoder_init should *not* be re-run. ?I would pull
> those two variables out of that IF block, and set them in their own IF
> block either directly before or directly after the !priv->init IF block.
>
>
>> @@ -1120,7 +1122,7 @@ size_t sbc_get_frame_length(sbc_t *sbc)
>> ? ? ? struct sbc_priv *priv;
>>
>> ? ? ? priv = sbc->priv;
>> - ? ? if (priv->init)
>> + ? ? if (priv->init && priv->frame.bitpool == sbc->bitpool)
>> ? ? ? ? ? ? ? return priv->frame.length;
>>
>> ? ? ? subbands = sbc->subbands ? 8 : 4;
>
> This is a valid change, because bitpool changes do indeed change the
> frame.length.
Ive sent a v2 patch but will update with a v3 with a proper
description why it is needed.
--
Luiz Augusto von Dentz
Computer Engineer
Hello Luiz,
On Tue, 2010-12-21 at 18:58 +0200, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> ---
> sbc/sbc.c | 8 +++++---
> 1 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/sbc/sbc.c b/sbc/sbc.c
> index a6391ae..b3a7a09 100644
> --- a/sbc/sbc.c
> +++ b/sbc/sbc.c
> @@ -965,7 +965,8 @@ ssize_t sbc_decode(sbc_t *sbc, const void *input, size_t input_len,
>
> framelen = sbc_unpack_frame(input, &priv->frame, input_len);
>
> - if (!priv->init) {
> + /* init if no previous frame was encode or bitpool has changed */
> + if (!priv->init || priv->frame.bitpool != sbc->bitpool) {
> sbc_decoder_init(&priv->dec_state, &priv->frame);
> priv->init = 1;
I don't think it is a good idea to reinitialize the SBC decoder if a
change in bitpool size is detected.
The A2DP and AVDTP specifications do not allow most of the parameters to
change from one frame to the next (mode, channels, subbands, blocks,
allocation, frequency) but explicitly DO allow changes in bitpool size
midstream. This allows for Variable Bit Rate (VBR) streaming, and adds
efficiency to the over-the-air link with no quality loss. Also, the
space required for decoding does not change just because the bitpool has
changed, so re-initialization should not be required.
The only things that should be different from one SBC frame to the next
is the bitpool of course, and the framelen. That said, if a bitpool
change is detected the only thing within that "IF" block that should be
updated is:
sbc->bitpool
If any of the other afore mentioned parameters change, the stream should
be terminated.
>
> @@ -1035,7 +1036,8 @@ ssize_t sbc_encode(sbc_t *sbc, const void *input, size_t input_len,
> if (written)
> *written = 0;
>
> - if (!priv->init) {
> + /* init if no previous frame was encode or bitpool has changed */
> + if (!priv->init || priv->frame.bitpool != sbc->bitpool) {
> priv->frame.frequency = sbc->frequency;
> priv->frame.mode = sbc->mode;
> priv->frame.channels = sbc->mode == SBC_MODE_MONO ? 1 : 2;
Same basic comment here, although it is perfectly allowable for the
Encoder to not support VBR. However if the Encoder does change the
bitpool, then the list of fields that need to be updated are:
priv->frame.bitpool
priv->frame.length
But again, the sbc_encoder_init should *not* be re-run. I would pull
those two variables out of that IF block, and set them in their own IF
block either directly before or directly after the !priv->init IF block.
> @@ -1120,7 +1122,7 @@ size_t sbc_get_frame_length(sbc_t *sbc)
> struct sbc_priv *priv;
>
> priv = sbc->priv;
> - if (priv->init)
> + if (priv->init && priv->frame.bitpool == sbc->bitpool)
> return priv->frame.length;
>
> subbands = sbc->subbands ? 8 : 4;
This is a valid change, because bitpool changes do indeed change the
frame.length.
--
Brian Gix
[email protected]
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum