SBC encoder/decoder API & errors handling

SBC encoder/decoder API & errors handling

[Siarhei Siamashka]

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

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

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

Re: SBC encoder/decoder API & errors handling

[Johan Hedberg]

[-- Attachment #1: Type: text/plain, Size: 1449 bytes --]

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

[-- Attachment #2: sbc.patch --]
[-- Type: text/x-diff, Size: 2593 bytes --]

diff --git a/audio/pcm_bluetooth.c b/audio/pcm_bluetooth.c
index cddeda2..4c0ab6f 100644
--- a/audio/pcm_bluetooth.c
+++ b/audio/pcm_bluetooth.c
@@ -1007,7 +1007,7 @@ static snd_pcm_sframes_t bluetooth_a2dp_write(snd_pcm_ioplug_t *io,
 	snd_pcm_sframes_t ret = 0;
 	unsigned int bytes_left;
 	int frame_size, encoded;
-	size_t written;
+	ssize_t written;
 	uint8_t *buff;
 
 	DBG("areas->step=%u areas->first=%u offset=%lu size=%lu",
diff --git a/sbc/sbc.c b/sbc/sbc.c
index 569dd7c..fa542e3 100644
--- a/sbc/sbc.c
+++ b/sbc/sbc.c
@@ -743,7 +743,7 @@ static int sbc_analyze_audio(struct sbc_encoder_state *state,
  * -99 not implemented
  */
 
-static SBC_ALWAYS_INLINE int sbc_pack_frame_internal(uint8_t *data,
+static SBC_ALWAYS_INLINE ssize_t sbc_pack_frame_internal(uint8_t *data,
 					struct sbc_frame *frame, size_t len,
 					int frame_subbands, int frame_channels,
 					int joint)
@@ -860,7 +860,7 @@ static SBC_ALWAYS_INLINE int sbc_pack_frame_internal(uint8_t *data,
 	return data_ptr - data;
 }
 
-static int sbc_pack_frame(uint8_t *data, struct sbc_frame *frame, size_t len,
+static ssize_t sbc_pack_frame(uint8_t *data, struct sbc_frame *frame, size_t len,
 								int joint)
 {
 	if (frame->subbands == 4) {
@@ -1005,10 +1005,10 @@ ssize_t sbc_decode(sbc_t *sbc, const void *input, size_t input_len,
 }
 
 ssize_t sbc_encode(sbc_t *sbc, const void *input, size_t input_len,
-			void *output, size_t output_len, size_t *written)
+			void *output, size_t output_len, ssize_t *written)
 {
 	struct sbc_priv *priv;
-	int framelen, samples;
+	ssize_t framelen, samples;
 	int (*sbc_enc_process_input)(int position,
 			const uint8_t *pcm, int16_t X[2][SBC_X_BUFFER_SIZE],
 			int nsamples, int nchannels);
diff --git a/sbc/sbc.h b/sbc/sbc.h
index 91422a9..2f830ad 100644
--- a/sbc/sbc.h
+++ b/sbc/sbc.h
@@ -92,7 +92,7 @@ ssize_t sbc_decode(sbc_t *sbc, const void *input, size_t input_len,
 
 /* Encodes ONE input block into ONE output block */
 ssize_t sbc_encode(sbc_t *sbc, const void *input, size_t input_len,
-			void *output, size_t output_len, size_t *written);
+			void *output, size_t output_len, ssize_t *written);
 
 /* Returns the output block size in bytes */
 size_t sbc_get_frame_length(sbc_t *sbc);
diff --git a/sbc/sbcenc.c b/sbc/sbcenc.c
index b5e0541..3d3a7dc 100644
--- a/sbc/sbcenc.c
+++ b/sbc/sbcenc.c
@@ -50,7 +50,7 @@ static void encode(char *filename, int subbands, int bitpool, int joint,
 	struct au_header au_hdr;
 	sbc_t sbc;
 	int fd, size, srate, codesize, nframes;
-	size_t encoded;
+	ssize_t encoded;
 	ssize_t len;
 
 	if (sizeof(au_hdr) != 24) {

Re: SBC encoder/decoder API & errors handling

[Luiz Augusto von Dentz]

Hi,

On Wed, Jun 30, 2010 at 10:53 AM, Johan Hedberg <johan.hedberg@gmail.com> 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

Re: SBC encoder/decoder API & errors handling

[Johan Hedberg]

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

Re: SBC encoder/decoder API & errors handling

[Johan Hedberg]

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

Re: SBC encoder/decoder API & errors handling

[Siarhei Siamashka]

[-- 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 --]