Hello,
When I tried to run some SBC encoder tests a few days ago, I noticed that
there is a regression introduced by commit:
http://git.kernel.org/?p=bluetooth/bluez.git;a=commit;h=c43f8bdcc1d527e2d77481a66217771038be3acd
It is caused by the change from 'int' to 'size_t' for the type of variable
'encoded' in 'sbcenc.c'. After this modification, the check for 'encoded <= 0'
does not catch the case when 'sbc_encode' tries to return a negative number in
'encoded' variable. Later we end up calling 'write' function with a negative
size for the data block.
Right now, in the case of error 'sbc_encode' function may either return a
negative number as a return value, or return a negative value in 'encoded'
variable. But this second type of error is apparently not handled by anything
other than 'sbcenc' tool at the moment.
Any opinions about how to fix it in the best way? Because it is a flaw in the
library API, comments from the interested parties are welcome.
--
Best regards,
Siarhei Siamashka
Hi Siarhei,
On Wed, Jun 30, 2010, Siarhei Siamashka wrote:
> So would it be a good idea to just fix the issues in the bluez SBC
> code as we feel appropriate and then notify pulseaudio developers
> about the changes?
Yes, I think that's a reasonable way to proceed.
Johan
On Wednesday 30 June 2010 08:53:47 Johan Hedberg wrote:
> Hi Luiz,
>
> On Wed, Jun 30, 2010, Luiz Augusto von Dentz wrote:
> > audio/pcm_bluetooth.c:1060: encoded = sbc_encode(&a2dp->sbc,
> > data->buffer, a2dp->codesize,
> > audio/pcm_bluetooth.c:1096: encoded = sbc_encode(&a2dp->sbc, buff,
> > a2dp->codesize,
>
> My patch already contained the fix for these two.
>
> > audio/gstsbcenc.c:391: consumed = sbc_encode(&enc->sbc,
(gpointer) data,
>
> This place actually passes NULL as the output parameter so no issue
> there.
Well, it is still kind of an issue because some types of errors are not
detected here at all.
> I'll push in a minute the fix in two separate commits (the libsbc
> changes should be in an independent patch so they can easily be exported
> to external copies like pulseaudio).
Yes, I used exactly the same patch myself and it helped. It's a bit artificial
problem and does not need an urgent fix. I found it by running a script which
uses 'sbcenc' with lots with different permutations of configuration options
(some of them are invalid) and logs md5 hashes of the encoded results. But as
long as the SBC encoder is always used with valid settings, no problems should
happen.
Anyway, API inconsistency is a bit ugly. Maybe it's better to always report
errors as a negative function return value (and have real constants defined
instead of some magic numbers)? Or just adjust encoding settings to the closest
valid configuration automatically and never fail there?
Another SBC issue (I found this report just on the last weekend) is the missing
support for the dynamically changing bitpool value:
https://tango.0pointer.de/pipermail/pulseaudio-discuss/2010-January/006042.html
It is a clear problem for the SBC decoder (in addition to the other issues like
not so good audio quality and poor performance, which would have to be
addressed eventually).
But SBC encoder could also make some use of this changing bitpool feature to
adaptively change it when connection quality is not very good and audio would
start skipping otherwise (due to the interference from wifi, walking away from
the bluetooth dongle behind a concrete wall or anything else).
So would it be a good idea to just fix the issues in the bluez SBC code as we
feel appropriate and then notify pulseaudio developers about the changes?
--
Best regards,
Siarhei Siamashka
Hi Luiz,
On Wed, Jun 30, 2010, Luiz Augusto von Dentz wrote:
> audio/pcm_bluetooth.c:1060: encoded = sbc_encode(&a2dp->sbc,
> data->buffer, a2dp->codesize,
> audio/pcm_bluetooth.c:1096: encoded = sbc_encode(&a2dp->sbc, buff,
> a2dp->codesize,
My patch already contained the fix for these two.
> audio/gstsbcenc.c:391: consumed = sbc_encode(&enc->sbc, (gpointer) data,
This place actually passes NULL as the output parameter so no issue
there.
I'll push in a minute the fix in two separate commits (the libsbc
changes should be in an independent patch so they can easily be exported
to external copies like pulseaudio).
Johan
Hi,
On Wed, Jun 30, 2010 at 10:53 AM, Johan Hedberg <[email protected]> wrote:
> Hi Siarhei,
>
> On Tue, Jun 29, 2010, Siarhei Siamashka wrote:
>> When I tried to run some SBC encoder tests a few days ago, I noticed that
>> there is a regression introduced by commit:
>> http://git.kernel.org/?p=bluetooth/bluez.git;a=commit;h=c43f8bdcc1d527e2d77481a66217771038be3acd
>>
>> It is caused by the change from 'int' to 'size_t' for the type of variable
>> 'encoded' in 'sbcenc.c'. After this modification, the check for 'encoded <= 0'
>> does not catch the case when 'sbc_encode' tries to return a negative number in
>> 'encoded' variable. Later we end up calling 'write' function with a negative
>> size for the data block.
>>
>> Right now, in the case of error 'sbc_encode' function may either return a
>> negative number as a return value, or return a negative value in 'encoded'
>> variable. But this second type of error is apparently not handled by anything
>> other than 'sbcenc' tool at the moment.
>>
>> Any opinions about how to fix it in the best way? Because it is a flaw in the
>> library API, comments from the interested parties are welcome.
>
> In general the API feels a bit weird in that it can return errors in two
> different ways. A less obtrusive fix would be to make the variable type
> ssize_t instead of size_t. Regarding breaking the SBC library API, I
> don't see any big issue with that since it's internal to bluetoothd and
> we can easily update the pulseaudio copy accordingly. Would the attached
> patch make sense?
I guess it make sense, but we should also fix alsa and gstreamer code
to use variables of type ssize_t instead of int:
audio/pcm_bluetooth.c:1060: encoded = sbc_encode(&a2dp->sbc,
data->buffer, a2dp->codesize,
audio/pcm_bluetooth.c:1096: encoded = sbc_encode(&a2dp->sbc, buff,
a2dp->codesize,
audio/gstsbcenc.c:391: consumed = sbc_encode(&enc->sbc, (gpointer) data,
--
Luiz Augusto von Dentz
Computer Engineer
Hi Siarhei,
On Tue, Jun 29, 2010, Siarhei Siamashka wrote:
> When I tried to run some SBC encoder tests a few days ago, I noticed that
> there is a regression introduced by commit:
> http://git.kernel.org/?p=bluetooth/bluez.git;a=commit;h=c43f8bdcc1d527e2d77481a66217771038be3acd
>
> It is caused by the change from 'int' to 'size_t' for the type of variable
> 'encoded' in 'sbcenc.c'. After this modification, the check for 'encoded <= 0'
> does not catch the case when 'sbc_encode' tries to return a negative number in
> 'encoded' variable. Later we end up calling 'write' function with a negative
> size for the data block.
>
> Right now, in the case of error 'sbc_encode' function may either return a
> negative number as a return value, or return a negative value in 'encoded'
> variable. But this second type of error is apparently not handled by anything
> other than 'sbcenc' tool at the moment.
>
> Any opinions about how to fix it in the best way? Because it is a flaw in the
> library API, comments from the interested parties are welcome.
In general the API feels a bit weird in that it can return errors in two
different ways. A less obtrusive fix would be to make the variable type
ssize_t instead of size_t. Regarding breaking the SBC library API, I
don't see any big issue with that since it's internal to bluetoothd and
we can easily update the pulseaudio copy accordingly. Would the attached
patch make sense?
Johan