Re: SBC encoder/decoder API & errors handling

[Siarhei Siamashka <siarhei.siamashka@gmail.com>]

[-- Attachment #1: Type: Text/Plain, Size: 2451 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]