From: Maxim Mikityanskiy <[email protected]>
If the first data chunk passed to sbc_decode was not long enough and
didn't contain full SBC packet, don't try to initialize codec parameters
with random values.
Signed-off-by: Maxim Mikityanskiy <[email protected]>
Cc: Marcel Holtmann <[email protected]>
---
The patch is for SBC library located at https://git.kernel.org/cgit/bluetooth/sbc.git/
sbc/sbc.c | 34 ++++++++++++++++++----------------
1 file changed, 18 insertions(+), 16 deletions(-)
diff --git a/sbc/sbc.c b/sbc/sbc.c
index 606f11c..7da476b 100644
--- a/sbc/sbc.c
+++ b/sbc/sbc.c
@@ -1222,22 +1222,24 @@ SBC_EXPORT ssize_t sbc_decode(sbc_t *sbc, const void *input, size_t input_len,
framelen = priv->unpack_frame(input, &priv->frame, input_len);
- if (!priv->init) {
- sbc_decoder_init(&priv->dec_state, &priv->frame);
- priv->init = true;
-
- sbc->frequency = priv->frame.frequency;
- sbc->mode = priv->frame.mode;
- sbc->subbands = priv->frame.subband_mode;
- sbc->blocks = priv->frame.block_mode;
- sbc->allocation = priv->frame.allocation;
- sbc->bitpool = priv->frame.bitpool;
-
- priv->frame.codesize = sbc_get_codesize(sbc);
- priv->frame.length = framelen;
- } else if (priv->frame.bitpool != sbc->bitpool) {
- priv->frame.length = framelen;
- sbc->bitpool = priv->frame.bitpool;
+ if (framelen >= 0) {
+ if (!priv->init) {
+ sbc_decoder_init(&priv->dec_state, &priv->frame);
+ priv->init = true;
+
+ sbc->frequency = priv->frame.frequency;
+ sbc->mode = priv->frame.mode;
+ sbc->subbands = priv->frame.subband_mode;
+ sbc->blocks = priv->frame.block_mode;
+ sbc->allocation = priv->frame.allocation;
+ sbc->bitpool = priv->frame.bitpool;
+
+ priv->frame.codesize = sbc_get_codesize(sbc);
+ priv->frame.length = framelen;
+ } else if (priv->frame.bitpool != sbc->bitpool) {
+ priv->frame.length = framelen;
+ sbc->bitpool = priv->frame.bitpool;
+ }
}
if (!output)
--
2.5.4 (Apple Git-61)
2015-12-15 20:54 GMT+02:00 Luiz Augusto von Dentz <[email protected]>:
> Hi Maxim,
>
> On Tue, Dec 15, 2015 at 4:27 PM, Maxim Mikityanskiy <[email protected]> wrote:
>> 2015-12-15 20:04 GMT+02:00 Luiz Augusto von Dentz <[email protected]>:
>>> Hi Maxim,
>>>
>>> On Tue, Dec 15, 2015 at 3:27 PM, Maxim Mikityanskiy <[email protected]> wrote:
>>>> Hi Luiz,
>>>>
>>>> 2015-12-15 18:51 GMT+02:00 Luiz Augusto von Dentz <[email protected]>:
>>>>> Hi Maxim,
>>>>>
>>>>> On Mon, Dec 14, 2015 at 3:27 PM, <[email protected]> wrote:
>>>>>> From: Maxim Mikityanskiy <[email protected]>
>>>>>>
>>>>>> If the first data chunk passed to sbc_decode was not long enough and
>>>>>> didn't contain full SBC packet, don't try to initialize codec parameters
>>>>>> with random values.
>>>>>>
>>>>>> Signed-off-by: Maxim Mikityanskiy <[email protected]>
>>>>>> Cc: Marcel Holtmann <[email protected]>
>>>>>> ---
>>>>>> The patch is for SBC library located at https://git.kernel.org/cgit/bluetooth/sbc.git/
>>>>>
>>>>> We don't use Signed-off-by in userspace.
>>>>
>>>> OK, I'll consider it.
>>>>
>>>>>>
>>>>>> sbc/sbc.c | 34 ++++++++++++++++++----------------
>>>>>> 1 file changed, 18 insertions(+), 16 deletions(-)
>>>>>>
>>>>>> diff --git a/sbc/sbc.c b/sbc/sbc.c
>>>>>> index 606f11c..7da476b 100644
>>>>>> --- a/sbc/sbc.c
>>>>>> +++ b/sbc/sbc.c
>>>>>> @@ -1222,22 +1222,24 @@ SBC_EXPORT ssize_t sbc_decode(sbc_t *sbc, const void *input, size_t input_len,
>>>>>>
>>>>>> framelen = priv->unpack_frame(input, &priv->frame, input_len);
>>>>>>
>>>>>> - if (!priv->init) {
>>>>>> - sbc_decoder_init(&priv->dec_state, &priv->frame);
>>>>>> - priv->init = true;
>>>>>> -
>>>>>> - sbc->frequency = priv->frame.frequency;
>>>>>> - sbc->mode = priv->frame.mode;
>>>>>> - sbc->subbands = priv->frame.subband_mode;
>>>>>> - sbc->blocks = priv->frame.block_mode;
>>>>>> - sbc->allocation = priv->frame.allocation;
>>>>>> - sbc->bitpool = priv->frame.bitpool;
>>>>>> -
>>>>>> - priv->frame.codesize = sbc_get_codesize(sbc);
>>>>>> - priv->frame.length = framelen;
>>>>>> - } else if (priv->frame.bitpool != sbc->bitpool) {
>>>>>> - priv->frame.length = framelen;
>>>>>> - sbc->bitpool = priv->frame.bitpool;
>>>>>> + if (framelen >= 0) {
>>>>>> + if (!priv->init) {
>>>>>> + sbc_decoder_init(&priv->dec_state, &priv->frame);
>>>>>> + priv->init = true;
>>>>>> +
>>>>>> + sbc->frequency = priv->frame.frequency;
>>>>>> + sbc->mode = priv->frame.mode;
>>>>>> + sbc->subbands = priv->frame.subband_mode;
>>>>>> + sbc->blocks = priv->frame.block_mode;
>>>>>> + sbc->allocation = priv->frame.allocation;
>>>>>> + sbc->bitpool = priv->frame.bitpool;
>>>>>> +
>>>>>> + priv->frame.codesize = sbc_get_codesize(sbc);
>>>>>> + priv->frame.length = framelen;
>>>>>> + } else if (priv->frame.bitpool != sbc->bitpool) {
>>>>>> + priv->frame.length = framelen;
>>>>>> + sbc->bitpool = priv->frame.bitpool;
>>>>>> + }
>>>>>
>>>>> Im fine with the change but I would prefix that you check the framelen
>>>>> separately e.g.:
>>>>>
>>>>> if (framelen < 0) {
>>>>> return framelen;
>>>>> }
>>>>>
>>>>> There is probably no use to continue if unpack_frame has failed.
>>>>
>>>> Actually, we can't just return immediately, because we will lose an
>>>> assignment "*written = 0;" that follows this block of code. And
>>>> putting that assignment also in "if (framelen < 0)" will lead to code
>>>> duplication. We could place that assignment before the check for
>>>> framelen, but it will change the behavior when output == NULL (now
>>>> *written is not touched if output == NULL).
>>>
>>> And with the changed you are proposing the code will evaluate framelen
>>> twice so how about the following:
>>
>> That has even worse problems, because if output == NULL (the caller
>> doesn't want the result to be saved) and framelen > 0, we'll just
>> return immediately and skip initialization of decoder.
>
> Yep, perhaps it would make more sense to have sbc_decode call
> sbc_parse and not the other way around that way we can actually return
> an error if output is NULL:
I agree, the code really looks better with this change. However, there
may be projects that call sbc_decode with output == NULL directly
instead of using sbc_parse, so that projects will break after this
change. Don't know if we should worry about them (even I don't know if
such projects exist). Anyway, if we want to maintain compatibility
with them, we could check "if (!output) return framelen;" after
calling sbc_parse. But the decision is up to you.
> diff --git a/sbc/sbc.c b/sbc/sbc.c
> index 606f11c..5af900d 100644
> --- a/sbc/sbc.c
> +++ b/sbc/sbc.c
> @@ -1205,15 +1205,8 @@ int sbc_reinit_a2dp(sbc_t *sbc, unsigned long flags,
>
> SBC_EXPORT ssize_t sbc_parse(sbc_t *sbc, const void *input, size_t input_len)
> {
> - return sbc_decode(sbc, input, input_len, NULL, 0, NULL);
> -}
> -
> -SBC_EXPORT ssize_t sbc_decode(sbc_t *sbc, const void *input, size_t input_len,
> - void *output, size_t output_len, size_t *written)
> -{
> struct sbc_priv *priv;
> - char *ptr;
> - int i, ch, framelen, samples;
> + int framelen;
>
> if (!sbc || !input)
> return -EIO;
> @@ -1222,6 +1215,9 @@ SBC_EXPORT ssize_t sbc_decode(sbc_t *sbc, const
> void *input, size_t input_len,
>
> framelen = priv->unpack_frame(input, &priv->frame, input_len);
>
> + if (framelen <= 0)
> + return framelen;
> +
> if (!priv->init) {
> sbc_decoder_init(&priv->dec_state, &priv->frame);
> priv->init = true;
> @@ -1240,8 +1236,20 @@ SBC_EXPORT ssize_t sbc_decode(sbc_t *sbc, const
> void *input, size_t input_len,
> sbc->bitpool = priv->frame.bitpool;
> }
>
> + return framelen;
> +}
> +
> +SBC_EXPORT ssize_t sbc_decode(sbc_t *sbc, const void *input, size_t input_len,
> + void *output, size_t output_len, size_t *written)
> +{
> + struct sbc_priv *priv;
> + char *ptr;
> + int i, ch, framelen, samples;
> +
> if (!output)
> - return framelen;
> + return -EINVAL;
> +
> + framelen = sbc_parse(sbc, input, input_len);
>
> if (written)
> *written = 0;
>
>
> Luiz Augusto von Dentz
Hi Maxim,
On Tue, Dec 15, 2015 at 4:27 PM, Maxim Mikityanskiy <[email protected]> wrote:
> 2015-12-15 20:04 GMT+02:00 Luiz Augusto von Dentz <[email protected]>:
>> Hi Maxim,
>>
>> On Tue, Dec 15, 2015 at 3:27 PM, Maxim Mikityanskiy <[email protected]> wrote:
>>> Hi Luiz,
>>>
>>> 2015-12-15 18:51 GMT+02:00 Luiz Augusto von Dentz <[email protected]>:
>>>> Hi Maxim,
>>>>
>>>> On Mon, Dec 14, 2015 at 3:27 PM, <[email protected]> wrote:
>>>>> From: Maxim Mikityanskiy <[email protected]>
>>>>>
>>>>> If the first data chunk passed to sbc_decode was not long enough and
>>>>> didn't contain full SBC packet, don't try to initialize codec parameters
>>>>> with random values.
>>>>>
>>>>> Signed-off-by: Maxim Mikityanskiy <[email protected]>
>>>>> Cc: Marcel Holtmann <[email protected]>
>>>>> ---
>>>>> The patch is for SBC library located at https://git.kernel.org/cgit/bluetooth/sbc.git/
>>>>
>>>> We don't use Signed-off-by in userspace.
>>>
>>> OK, I'll consider it.
>>>
>>>>>
>>>>> sbc/sbc.c | 34 ++++++++++++++++++----------------
>>>>> 1 file changed, 18 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/sbc/sbc.c b/sbc/sbc.c
>>>>> index 606f11c..7da476b 100644
>>>>> --- a/sbc/sbc.c
>>>>> +++ b/sbc/sbc.c
>>>>> @@ -1222,22 +1222,24 @@ SBC_EXPORT ssize_t sbc_decode(sbc_t *sbc, const void *input, size_t input_len,
>>>>>
>>>>> framelen = priv->unpack_frame(input, &priv->frame, input_len);
>>>>>
>>>>> - if (!priv->init) {
>>>>> - sbc_decoder_init(&priv->dec_state, &priv->frame);
>>>>> - priv->init = true;
>>>>> -
>>>>> - sbc->frequency = priv->frame.frequency;
>>>>> - sbc->mode = priv->frame.mode;
>>>>> - sbc->subbands = priv->frame.subband_mode;
>>>>> - sbc->blocks = priv->frame.block_mode;
>>>>> - sbc->allocation = priv->frame.allocation;
>>>>> - sbc->bitpool = priv->frame.bitpool;
>>>>> -
>>>>> - priv->frame.codesize = sbc_get_codesize(sbc);
>>>>> - priv->frame.length = framelen;
>>>>> - } else if (priv->frame.bitpool != sbc->bitpool) {
>>>>> - priv->frame.length = framelen;
>>>>> - sbc->bitpool = priv->frame.bitpool;
>>>>> + if (framelen >= 0) {
>>>>> + if (!priv->init) {
>>>>> + sbc_decoder_init(&priv->dec_state, &priv->frame);
>>>>> + priv->init = true;
>>>>> +
>>>>> + sbc->frequency = priv->frame.frequency;
>>>>> + sbc->mode = priv->frame.mode;
>>>>> + sbc->subbands = priv->frame.subband_mode;
>>>>> + sbc->blocks = priv->frame.block_mode;
>>>>> + sbc->allocation = priv->frame.allocation;
>>>>> + sbc->bitpool = priv->frame.bitpool;
>>>>> +
>>>>> + priv->frame.codesize = sbc_get_codesize(sbc);
>>>>> + priv->frame.length = framelen;
>>>>> + } else if (priv->frame.bitpool != sbc->bitpool) {
>>>>> + priv->frame.length = framelen;
>>>>> + sbc->bitpool = priv->frame.bitpool;
>>>>> + }
>>>>
>>>> Im fine with the change but I would prefix that you check the framelen
>>>> separately e.g.:
>>>>
>>>> if (framelen < 0) {
>>>> return framelen;
>>>> }
>>>>
>>>> There is probably no use to continue if unpack_frame has failed.
>>>
>>> Actually, we can't just return immediately, because we will lose an
>>> assignment "*written = 0;" that follows this block of code. And
>>> putting that assignment also in "if (framelen < 0)" will lead to code
>>> duplication. We could place that assignment before the check for
>>> framelen, but it will change the behavior when output == NULL (now
>>> *written is not touched if output == NULL).
>>
>> And with the changed you are proposing the code will evaluate framelen
>> twice so how about the following:
>
> That has even worse problems, because if output == NULL (the caller
> doesn't want the result to be saved) and framelen > 0, we'll just
> return immediately and skip initialization of decoder.
Yep, perhaps it would make more sense to have sbc_decode call
sbc_parse and not the other way around that way we can actually return
an error if output is NULL:
diff --git a/sbc/sbc.c b/sbc/sbc.c
index 606f11c..5af900d 100644
--- a/sbc/sbc.c
+++ b/sbc/sbc.c
@@ -1205,15 +1205,8 @@ int sbc_reinit_a2dp(sbc_t *sbc, unsigned long flags,
SBC_EXPORT ssize_t sbc_parse(sbc_t *sbc, const void *input, size_t input_len)
{
- return sbc_decode(sbc, input, input_len, NULL, 0, NULL);
-}
-
-SBC_EXPORT ssize_t sbc_decode(sbc_t *sbc, const void *input, size_t input_len,
- void *output, size_t output_len, size_t *written)
-{
struct sbc_priv *priv;
- char *ptr;
- int i, ch, framelen, samples;
+ int framelen;
if (!sbc || !input)
return -EIO;
@@ -1222,6 +1215,9 @@ SBC_EXPORT ssize_t sbc_decode(sbc_t *sbc, const
void *input, size_t input_len,
framelen = priv->unpack_frame(input, &priv->frame, input_len);
+ if (framelen <= 0)
+ return framelen;
+
if (!priv->init) {
sbc_decoder_init(&priv->dec_state, &priv->frame);
priv->init = true;
@@ -1240,8 +1236,20 @@ SBC_EXPORT ssize_t sbc_decode(sbc_t *sbc, const
void *input, size_t input_len,
sbc->bitpool = priv->frame.bitpool;
}
+ return framelen;
+}
+
+SBC_EXPORT ssize_t sbc_decode(sbc_t *sbc, const void *input, size_t input_len,
+ void *output, size_t output_len, size_t *written)
+{
+ struct sbc_priv *priv;
+ char *ptr;
+ int i, ch, framelen, samples;
+
if (!output)
- return framelen;
+ return -EINVAL;
+
+ framelen = sbc_parse(sbc, input, input_len);
if (written)
*written = 0;
Luiz Augusto von Dentz
2015-12-15 20:04 GMT+02:00 Luiz Augusto von Dentz <[email protected]>:
> Hi Maxim,
>
> On Tue, Dec 15, 2015 at 3:27 PM, Maxim Mikityanskiy <[email protected]> wrote:
>> Hi Luiz,
>>
>> 2015-12-15 18:51 GMT+02:00 Luiz Augusto von Dentz <[email protected]>:
>>> Hi Maxim,
>>>
>>> On Mon, Dec 14, 2015 at 3:27 PM, <[email protected]> wrote:
>>>> From: Maxim Mikityanskiy <[email protected]>
>>>>
>>>> If the first data chunk passed to sbc_decode was not long enough and
>>>> didn't contain full SBC packet, don't try to initialize codec parameters
>>>> with random values.
>>>>
>>>> Signed-off-by: Maxim Mikityanskiy <[email protected]>
>>>> Cc: Marcel Holtmann <[email protected]>
>>>> ---
>>>> The patch is for SBC library located at https://git.kernel.org/cgit/bluetooth/sbc.git/
>>>
>>> We don't use Signed-off-by in userspace.
>>
>> OK, I'll consider it.
>>
>>>>
>>>> sbc/sbc.c | 34 ++++++++++++++++++----------------
>>>> 1 file changed, 18 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/sbc/sbc.c b/sbc/sbc.c
>>>> index 606f11c..7da476b 100644
>>>> --- a/sbc/sbc.c
>>>> +++ b/sbc/sbc.c
>>>> @@ -1222,22 +1222,24 @@ SBC_EXPORT ssize_t sbc_decode(sbc_t *sbc, const void *input, size_t input_len,
>>>>
>>>> framelen = priv->unpack_frame(input, &priv->frame, input_len);
>>>>
>>>> - if (!priv->init) {
>>>> - sbc_decoder_init(&priv->dec_state, &priv->frame);
>>>> - priv->init = true;
>>>> -
>>>> - sbc->frequency = priv->frame.frequency;
>>>> - sbc->mode = priv->frame.mode;
>>>> - sbc->subbands = priv->frame.subband_mode;
>>>> - sbc->blocks = priv->frame.block_mode;
>>>> - sbc->allocation = priv->frame.allocation;
>>>> - sbc->bitpool = priv->frame.bitpool;
>>>> -
>>>> - priv->frame.codesize = sbc_get_codesize(sbc);
>>>> - priv->frame.length = framelen;
>>>> - } else if (priv->frame.bitpool != sbc->bitpool) {
>>>> - priv->frame.length = framelen;
>>>> - sbc->bitpool = priv->frame.bitpool;
>>>> + if (framelen >= 0) {
>>>> + if (!priv->init) {
>>>> + sbc_decoder_init(&priv->dec_state, &priv->frame);
>>>> + priv->init = true;
>>>> +
>>>> + sbc->frequency = priv->frame.frequency;
>>>> + sbc->mode = priv->frame.mode;
>>>> + sbc->subbands = priv->frame.subband_mode;
>>>> + sbc->blocks = priv->frame.block_mode;
>>>> + sbc->allocation = priv->frame.allocation;
>>>> + sbc->bitpool = priv->frame.bitpool;
>>>> +
>>>> + priv->frame.codesize = sbc_get_codesize(sbc);
>>>> + priv->frame.length = framelen;
>>>> + } else if (priv->frame.bitpool != sbc->bitpool) {
>>>> + priv->frame.length = framelen;
>>>> + sbc->bitpool = priv->frame.bitpool;
>>>> + }
>>>
>>> Im fine with the change but I would prefix that you check the framelen
>>> separately e.g.:
>>>
>>> if (framelen < 0) {
>>> return framelen;
>>> }
>>>
>>> There is probably no use to continue if unpack_frame has failed.
>>
>> Actually, we can't just return immediately, because we will lose an
>> assignment "*written = 0;" that follows this block of code. And
>> putting that assignment also in "if (framelen < 0)" will lead to code
>> duplication. We could place that assignment before the check for
>> framelen, but it will change the behavior when output == NULL (now
>> *written is not touched if output == NULL).
>
> And with the changed you are proposing the code will evaluate framelen
> twice so how about the following:
That has even worse problems, because if output == NULL (the caller
doesn't want the result to be saved) and framelen > 0, we'll just
return immediately and skip initialization of decoder.
> diff --git a/sbc/sbc.c b/sbc/sbc.c
> index 606f11c..ee59086 100644
> --- a/sbc/sbc.c
> +++ b/sbc/sbc.c
> @@ -1222,6 +1222,15 @@ SBC_EXPORT ssize_t sbc_decode(sbc_t *sbc, const
> void *input, size_t input_len,
>
> framelen = priv->unpack_frame(input, &priv->frame, input_len);
>
> + if (!output)
> + return framelen;
> +
> + if (written)
> + *written = 0;
> +
> + if (framelen <= 0)
> + return framelen;
> +
> if (!priv->init) {
> sbc_decoder_init(&priv->dec_state, &priv->frame);
> priv->init = true;
> @@ -1240,15 +1249,6 @@ SBC_EXPORT ssize_t sbc_decode(sbc_t *sbc, const
> void *input, size_t input_len,
> sbc->bitpool = priv->frame.bitpool;
> }
>
> - if (!output)
> - return framelen;
> -
> - if (written)
> - *written = 0;
> -
> - if (framelen <= 0)
> - return framelen;
> -
> samples = sbc_synthesize_audio(&priv->dec_state, &priv->frame);
>
> ptr = output;
>
> --
> Luiz Augusto von Dentz
Hi Maxim,
On Tue, Dec 15, 2015 at 3:27 PM, Maxim Mikityanskiy <[email protected]> wrote:
> Hi Luiz,
>
> 2015-12-15 18:51 GMT+02:00 Luiz Augusto von Dentz <[email protected]>:
>> Hi Maxim,
>>
>> On Mon, Dec 14, 2015 at 3:27 PM, <[email protected]> wrote:
>>> From: Maxim Mikityanskiy <[email protected]>
>>>
>>> If the first data chunk passed to sbc_decode was not long enough and
>>> didn't contain full SBC packet, don't try to initialize codec parameters
>>> with random values.
>>>
>>> Signed-off-by: Maxim Mikityanskiy <[email protected]>
>>> Cc: Marcel Holtmann <[email protected]>
>>> ---
>>> The patch is for SBC library located at https://git.kernel.org/cgit/bluetooth/sbc.git/
>>
>> We don't use Signed-off-by in userspace.
>
> OK, I'll consider it.
>
>>>
>>> sbc/sbc.c | 34 ++++++++++++++++++----------------
>>> 1 file changed, 18 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/sbc/sbc.c b/sbc/sbc.c
>>> index 606f11c..7da476b 100644
>>> --- a/sbc/sbc.c
>>> +++ b/sbc/sbc.c
>>> @@ -1222,22 +1222,24 @@ SBC_EXPORT ssize_t sbc_decode(sbc_t *sbc, const void *input, size_t input_len,
>>>
>>> framelen = priv->unpack_frame(input, &priv->frame, input_len);
>>>
>>> - if (!priv->init) {
>>> - sbc_decoder_init(&priv->dec_state, &priv->frame);
>>> - priv->init = true;
>>> -
>>> - sbc->frequency = priv->frame.frequency;
>>> - sbc->mode = priv->frame.mode;
>>> - sbc->subbands = priv->frame.subband_mode;
>>> - sbc->blocks = priv->frame.block_mode;
>>> - sbc->allocation = priv->frame.allocation;
>>> - sbc->bitpool = priv->frame.bitpool;
>>> -
>>> - priv->frame.codesize = sbc_get_codesize(sbc);
>>> - priv->frame.length = framelen;
>>> - } else if (priv->frame.bitpool != sbc->bitpool) {
>>> - priv->frame.length = framelen;
>>> - sbc->bitpool = priv->frame.bitpool;
>>> + if (framelen >= 0) {
>>> + if (!priv->init) {
>>> + sbc_decoder_init(&priv->dec_state, &priv->frame);
>>> + priv->init = true;
>>> +
>>> + sbc->frequency = priv->frame.frequency;
>>> + sbc->mode = priv->frame.mode;
>>> + sbc->subbands = priv->frame.subband_mode;
>>> + sbc->blocks = priv->frame.block_mode;
>>> + sbc->allocation = priv->frame.allocation;
>>> + sbc->bitpool = priv->frame.bitpool;
>>> +
>>> + priv->frame.codesize = sbc_get_codesize(sbc);
>>> + priv->frame.length = framelen;
>>> + } else if (priv->frame.bitpool != sbc->bitpool) {
>>> + priv->frame.length = framelen;
>>> + sbc->bitpool = priv->frame.bitpool;
>>> + }
>>
>> Im fine with the change but I would prefix that you check the framelen
>> separately e.g.:
>>
>> if (framelen < 0) {
>> return framelen;
>> }
>>
>> There is probably no use to continue if unpack_frame has failed.
>
> Actually, we can't just return immediately, because we will lose an
> assignment "*written = 0;" that follows this block of code. And
> putting that assignment also in "if (framelen < 0)" will lead to code
> duplication. We could place that assignment before the check for
> framelen, but it will change the behavior when output == NULL (now
> *written is not touched if output == NULL).
And with the changed you are proposing the code will evaluate framelen
twice so how about the following:
diff --git a/sbc/sbc.c b/sbc/sbc.c
index 606f11c..ee59086 100644
--- a/sbc/sbc.c
+++ b/sbc/sbc.c
@@ -1222,6 +1222,15 @@ SBC_EXPORT ssize_t sbc_decode(sbc_t *sbc, const
void *input, size_t input_len,
framelen = priv->unpack_frame(input, &priv->frame, input_len);
+ if (!output)
+ return framelen;
+
+ if (written)
+ *written = 0;
+
+ if (framelen <= 0)
+ return framelen;
+
if (!priv->init) {
sbc_decoder_init(&priv->dec_state, &priv->frame);
priv->init = true;
@@ -1240,15 +1249,6 @@ SBC_EXPORT ssize_t sbc_decode(sbc_t *sbc, const
void *input, size_t input_len,
sbc->bitpool = priv->frame.bitpool;
}
- if (!output)
- return framelen;
-
- if (written)
- *written = 0;
-
- if (framelen <= 0)
- return framelen;
-
samples = sbc_synthesize_audio(&priv->dec_state, &priv->frame);
ptr = output;
--
Luiz Augusto von Dentz
Hi Luiz,
2015-12-15 18:51 GMT+02:00 Luiz Augusto von Dentz <[email protected]>:
> Hi Maxim,
>
> On Mon, Dec 14, 2015 at 3:27 PM, <[email protected]> wrote:
>> From: Maxim Mikityanskiy <[email protected]>
>>
>> If the first data chunk passed to sbc_decode was not long enough and
>> didn't contain full SBC packet, don't try to initialize codec parameters
>> with random values.
>>
>> Signed-off-by: Maxim Mikityanskiy <[email protected]>
>> Cc: Marcel Holtmann <[email protected]>
>> ---
>> The patch is for SBC library located at https://git.kernel.org/cgit/bluetooth/sbc.git/
>
> We don't use Signed-off-by in userspace.
OK, I'll consider it.
>>
>> sbc/sbc.c | 34 ++++++++++++++++++----------------
>> 1 file changed, 18 insertions(+), 16 deletions(-)
>>
>> diff --git a/sbc/sbc.c b/sbc/sbc.c
>> index 606f11c..7da476b 100644
>> --- a/sbc/sbc.c
>> +++ b/sbc/sbc.c
>> @@ -1222,22 +1222,24 @@ SBC_EXPORT ssize_t sbc_decode(sbc_t *sbc, const void *input, size_t input_len,
>>
>> framelen = priv->unpack_frame(input, &priv->frame, input_len);
>>
>> - if (!priv->init) {
>> - sbc_decoder_init(&priv->dec_state, &priv->frame);
>> - priv->init = true;
>> -
>> - sbc->frequency = priv->frame.frequency;
>> - sbc->mode = priv->frame.mode;
>> - sbc->subbands = priv->frame.subband_mode;
>> - sbc->blocks = priv->frame.block_mode;
>> - sbc->allocation = priv->frame.allocation;
>> - sbc->bitpool = priv->frame.bitpool;
>> -
>> - priv->frame.codesize = sbc_get_codesize(sbc);
>> - priv->frame.length = framelen;
>> - } else if (priv->frame.bitpool != sbc->bitpool) {
>> - priv->frame.length = framelen;
>> - sbc->bitpool = priv->frame.bitpool;
>> + if (framelen >= 0) {
>> + if (!priv->init) {
>> + sbc_decoder_init(&priv->dec_state, &priv->frame);
>> + priv->init = true;
>> +
>> + sbc->frequency = priv->frame.frequency;
>> + sbc->mode = priv->frame.mode;
>> + sbc->subbands = priv->frame.subband_mode;
>> + sbc->blocks = priv->frame.block_mode;
>> + sbc->allocation = priv->frame.allocation;
>> + sbc->bitpool = priv->frame.bitpool;
>> +
>> + priv->frame.codesize = sbc_get_codesize(sbc);
>> + priv->frame.length = framelen;
>> + } else if (priv->frame.bitpool != sbc->bitpool) {
>> + priv->frame.length = framelen;
>> + sbc->bitpool = priv->frame.bitpool;
>> + }
>
> Im fine with the change but I would prefix that you check the framelen
> separately e.g.:
>
> if (framelen < 0) {
> return framelen;
> }
>
> There is probably no use to continue if unpack_frame has failed.
Actually, we can't just return immediately, because we will lose an
assignment "*written = 0;" that follows this block of code. And
putting that assignment also in "if (framelen < 0)" will lead to code
duplication. We could place that assignment before the check for
framelen, but it will change the behavior when output == NULL (now
*written is not touched if output == NULL).
That's why I put that big "if (framelen >= 0)" around that block of
code; any other options alter the behavior of the function.
>> }
>>
>> if (!output)
>> --
>> 2.5.4 (Apple Git-61)
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Luiz Augusto von Dentz
Hi Maxim,
On Mon, Dec 14, 2015 at 3:27 PM, <[email protected]> wrote:
> From: Maxim Mikityanskiy <[email protected]>
>
> If the first data chunk passed to sbc_decode was not long enough and
> didn't contain full SBC packet, don't try to initialize codec parameters
> with random values.
>
> Signed-off-by: Maxim Mikityanskiy <[email protected]>
> Cc: Marcel Holtmann <[email protected]>
> ---
> The patch is for SBC library located at https://git.kernel.org/cgit/bluetooth/sbc.git/
We don't use Signed-off-by in userspace.
>
> sbc/sbc.c | 34 ++++++++++++++++++----------------
> 1 file changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/sbc/sbc.c b/sbc/sbc.c
> index 606f11c..7da476b 100644
> --- a/sbc/sbc.c
> +++ b/sbc/sbc.c
> @@ -1222,22 +1222,24 @@ SBC_EXPORT ssize_t sbc_decode(sbc_t *sbc, const void *input, size_t input_len,
>
> framelen = priv->unpack_frame(input, &priv->frame, input_len);
>
> - if (!priv->init) {
> - sbc_decoder_init(&priv->dec_state, &priv->frame);
> - priv->init = true;
> -
> - sbc->frequency = priv->frame.frequency;
> - sbc->mode = priv->frame.mode;
> - sbc->subbands = priv->frame.subband_mode;
> - sbc->blocks = priv->frame.block_mode;
> - sbc->allocation = priv->frame.allocation;
> - sbc->bitpool = priv->frame.bitpool;
> -
> - priv->frame.codesize = sbc_get_codesize(sbc);
> - priv->frame.length = framelen;
> - } else if (priv->frame.bitpool != sbc->bitpool) {
> - priv->frame.length = framelen;
> - sbc->bitpool = priv->frame.bitpool;
> + if (framelen >= 0) {
> + if (!priv->init) {
> + sbc_decoder_init(&priv->dec_state, &priv->frame);
> + priv->init = true;
> +
> + sbc->frequency = priv->frame.frequency;
> + sbc->mode = priv->frame.mode;
> + sbc->subbands = priv->frame.subband_mode;
> + sbc->blocks = priv->frame.block_mode;
> + sbc->allocation = priv->frame.allocation;
> + sbc->bitpool = priv->frame.bitpool;
> +
> + priv->frame.codesize = sbc_get_codesize(sbc);
> + priv->frame.length = framelen;
> + } else if (priv->frame.bitpool != sbc->bitpool) {
> + priv->frame.length = framelen;
> + sbc->bitpool = priv->frame.bitpool;
> + }
Im fine with the change but I would prefix that you check the framelen
separately e.g.:
if (framelen < 0) {
return framelen;
}
There is probably no use to continue if unpack_frame has failed.
> }
>
> if (!output)
> --
> 2.5.4 (Apple Git-61)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Luiz Augusto von Dentz