Hi Maxim, On Tue, Dec 15, 2015 at 3:27 PM, Maxim Mikityanskiy <maxtram95@xxxxxxxxx> wrote: > Hi Luiz, > > 2015-12-15 18:51 GMT+02:00 Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>: >> Hi Maxim, >> >> On Mon, Dec 14, 2015 at 3:27 PM, <maxtram95@xxxxxxxxx> wrote: >>> From: Maxim Mikityanskiy <maxtram95@xxxxxxxxx> >>> >>> 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 <maxtram95@xxxxxxxxx> >>> Cc: Marcel Holtmann <marcel@xxxxxxxxxxxx> >>> --- >>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
