SBC encoder conformance test for version 4.25 passed!

SBC encoder conformance test for version 4.25 passed!

[Christian Hoene]

Hello Siarhei,

sorry for the delay - these days my family comes first ;-)

Good news! All tests passed! 
http://net.cs.uni-tuebingen.de/html/nexgenvoip/

Congratulations and a happy new year!

 Christian 


> -----Original Message-----
> From: Siarhei Siamashka [mailto:siarhei.siamashka@nokia.com]
> Sent: Tuesday, December 30, 2008 11:46 AM
> To: ext Christian Hoene
> Subject: SBC encoder conformance test for version 4.25
> 
> Hello Christian,
> 
> Could you please rebuild SBC encoder test results on
> http://net.cs.uni-tuebingen.de/html/nexgenvoip/html/
> for the just released version 4.25?
> 
> I'm going to submit SIMD optimizations for the analysis filter which are
> almost done now (using ARM NEON, but Intel MMX/SSE2 requires only a
> minor modification). In any case I would prefer to make sure that the
> current C code is in a good shape first.
> 
> Thanks a lot for your participation. I hope that with our combined
efforts,
> we can make SBC really much better and faster.
> 
> --
> Best regards,
> Siarhei Siamashka

Re: SBC encoder conformance test for version 4.25 passed!

[Siarhei Siamashka]

On Thursday 01 January 2009 16:24:21 ext Christian Hoene wrote:
> Hello Siarhei,
>
> sorry for the delay - these days my family comes first ;-)
>
> Good news! All tests passed!
> http://net.cs.uni-tuebingen.de/html/nexgenvoip/

Thank you very much. Sorry for the delay with reply too.

I wonder if we need to do something about test 08?

Also could you try running a special test of version 4.25 with
SBC_HIGH_PRECISION enabled? The point is that it uses the best
possible precision for the analysis filter and minimizes effects of
rounding errors (I would estimate that the precision should be even
better than single precision floating point). Having the test run with this
precision settings can expose some of the other minor bugs in the code
(if they exist), which can now be hidden under the slight precision loss
because of using 16-bit fixed point calculations.


Best regards,
Siarhei Siamashka

[PATCH] SBC Encoder program

[Christian Hoene]

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


> I wonder if we need to do something about test 08?

Yes, there was a bug in the encoder program. Attached the patch.

Greetings
 Christian


[-- Attachment #2: 0003-Bug-fix-in-SBC-encoder-program-to-correctly-handle-f.patch --]
[-- Type: application/mbox, Size: 936 bytes --]

Re: [PATCH] SBC Encoder program

[Luiz Augusto von Dentz]

Hi Christian

> Yes, there was a bug in the encoder program. Attached the patch.
>
> Greetings
>  Christian
>
>

Coding style:

return pos > len ? pos : len;


-- 
Luiz Augusto von Dentz
Engenheiro de Computação

Re: [PATCH] SBC Encoder program

[Christian Hoene]

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

On Mon, 2009-01-05 at 09:22 -0300, Luiz Augusto von Dentz wrote:
> Hi Christian
> 
> > Yes, there was a bug in the encoder program. Attached the patch.
> >
> > Greetings
> >  Christian
> >
> >
> 
> Coding style:
> 
> return pos > len ? pos : len;

Thanks. I corrected it and even found another bug.

Attached the new and corrected patch.

Greetings
 Christian


[-- Attachment #2: 0003-Fixed-correct-handling-of-frame-sizes-in-the-encoder.patch --]
[-- Type: application/mbox, Size: 2056 bytes --]

Re: [PATCH] SBC Encoder program

[Luiz Augusto von Dentz]

Hi Christian,

Just the codesize is 16 bit, right? So I guess the following changes
are not really necessary:


+	uint16_t subbands, channels, blocks;



-- 
Luiz Augusto von Dentz
Engenheiro de Computação

RE: [PATCH] SBC Encoder program

[Christian Hoene]

> Just the codesize is 16 bit, right? So I guess the following changes
> are not really necessary:
> 
> 
> +	uint16_t subbands, channels, blocks;

No, but then we have to change the last line of the function to
  return ((uint16_t)subbands) * blocks * channels * 2;

CH

Re: [PATCH] SBC Encoder program

[Siarhei Siamashka]

On Monday 05 January 2009 16:42:39 ext Christian Hoene wrote:
> > Just the codesize is 16 bit, right? So I guess the following changes
> > are not really necessary:
> >
> >
> > +	uint16_t subbands, channels, blocks;
>
> No, but then we have to change the last line of the function to
>   return ((uint16_t)subbands) * blocks * channels * 2;

Not quite so. There is such thing as "integer promotion" defined in C
standard. And integer type is guaranteed to be at least 16 bit. But I
agree that explicitly having uint16_t is much more likely not to raise
any doubts about its correctness :)

-- 
Best regards,
Siarhei Siamashka

Re: [PATCH] SBC Encoder program

[Siarhei Siamashka]

On Monday 05 January 2009 14:28:36 ext Christian Hoene wrote:
> On Mon, 2009-01-05 at 09:22 -0300, Luiz Augusto von Dentz wrote:
> > Hi Christian
> >
> > > Yes, there was a bug in the encoder program. Attached the patch.
> > >
> > > Greetings
> > >  Christian
> >
> > Coding style:
> >
> > return pos > len ? pos : len;
>
> Thanks. I corrected it and even found another bug.
>
> Attached the new and corrected patch.

There are some parts that look a bit redundant/suspicious:

> @@ -47,7 +47,7 @@ static ssize_t __read(int fd, void *buf, size_t count)
>         while (count > 0) {
>                 len = read(fd, buf + pos, count);
>                 if (len <= 0)
> -                       return len;
> +                       return pos > len ? pos : len;
>
>                 count -= len;
>                 pos   += len;

Is the ternary '?' operator really needed here? In this part of code we know
for sure that 'len' is less or equal to zero, also 'pos ' is a positive number
or zero. Having just 'return pos' should be enough.

> @@ -188,6 +188,8 @@ static void encode(char *filename, int subbands, int
> bitpool, int joint,
>
>                 len = sbc_encode(&sbc, input, size,
>                                         output, sizeof(output), &encoded);
> +               if (len <= 0)
> +                       break;
>                 if (len < size)
>                         memmove(input, input + len, size - len);

If the return value is negative, there was some error. Probably showing some
kind of error message is appropriate here instead of just silently bailing
out.

-- 
Best regards,
Siarhei Siamashka

RE: [PATCH] SBC Encoder program

[Christian Hoene]

> There are some parts that look a bit redundant/suspicious:
> 
> > @@ -47,7 +47,7 @@ static ssize_t __read(int fd, void *buf, size_t count)
> >         while (count > 0) {
> >                 len = read(fd, buf + pos, count);
> >                 if (len <= 0)
> > -                       return len;
> > +                       return pos > len ? pos : len;
> >
> >                 count -= len;
> >                 pos   += len;
> 
> Is the ternary '?' operator really needed here? In this part of code we know
> for sure that 'len' is less or equal to zero, also 'pos ' is a positive number
> or zero. Having just 'return pos' should be enough.

No. If read returns an error, the this error is passed to __read, too.

> > @@ -188,6 +188,8 @@ static void encode(char *filename, int subbands, int
> > bitpool, int joint,
> >
> >                 len = sbc_encode(&sbc, input, size,
> >                                         output, sizeof(output), &encoded);
> > +               if (len <= 0)
> > +                       break;
> >                 if (len < size)
> >                         memmove(input, input + len, size - len);
> 
> If the return value is negative, there was some error. Probably showing some
> kind of error message is appropriate here instead of just silently bailing
> out.

Yes, if len is -1 then errno depended message shall be printed and the program should quit.


CH

Re: [PATCH] SBC Encoder program

[Siarhei Siamashka]

On Monday 05 January 2009 16:42:39 ext Christian Hoene wrote:
> > There are some parts that look a bit redundant/suspicious:
> > > @@ -47,7 +47,7 @@ static ssize_t __read(int fd, void *buf, size_t
> > > count) while (count > 0) {
> > >                 len = read(fd, buf + pos, count);
> > >                 if (len <= 0)
> > > -                       return len;
> > > +                       return pos > len ? pos : len;
> > >
> > >                 count -= len;
> > >                 pos   += len;
> >
> > Is the ternary '?' operator really needed here? In this part of code we
> > know for sure that 'len' is less or equal to zero, also 'pos ' is a
> > positive number or zero. Having just 'return pos' should be enough.
>
> No. If read returns an error, the this error is passed to __read, too.

I mean that 'return pos > len ? pos : len' and 'return pos' expressions are
completely interexchangeable in this context.

Regarding this particular change itself, was there a reproducible problem in
your testing that strictly required this fix? Was it involving sockets or
pipes?

But the bug in 'sbc_get_codesize' is the unquestionable one for sure. Good job
finding and fixing it.

-- 
Best regards,
Siarhei Siamashka

Re: [PATCH] SBC Encoder program

[Siarhei Siamashka]

On Monday 05 January 2009 17:18:44 ext Siarhei Siamashka wrote:
> On Monday 05 January 2009 16:42:39 ext Christian Hoene wrote:
[...]
> But the bug in 'sbc_get_codesize' is the unquestionable one for sure. Good
> job finding and fixing it.

Sorry, I meant the need to promote 'codesize' to uint16_t is unquestionable,
not changing the 'sbc_get_codesize' function itself :)

-- 
Best regards,
Siarhei Siamashka

RE: [PATCH] SBC Encoder program

[Christian Hoene]

> I mean that 'return pos > len ? pos : len' and 'return pos' expressions are
> completely interexchangeable in this context.

They are slightly different but really, I do not care. Both fixes will work. At least one must be applied.
BTW: Why not use fread instead? ;-)

Greetings

 Christian

Re: [PATCH] SBC Encoder program

[Siarhei Siamashka]

On Monday 05 January 2009 17:33:10 ext Christian Hoene wrote:
> > I mean that 'return pos > len ? pos : len' and 'return pos' expressions
> > are completely interexchangeable in this context.
>
> They are slightly different but really, I do not care. Both fixes will work.

Yes, because they provide exactly identical results :-) In this sense, your
fix is a bit broken, because it will never allow to pass through and return a
negative error code.

> At least one must be applied. BTW: Why not use fread instead? ;-) 

Agreed here. What's the point of having double underscored versions (such
identifiers are reserved and not allowed to be used by programs) of read and
write functions? Maybe they can be purged and just normal read/write calls
could be used instead? Or alternatively fread/fwrite indeed?

Also there seems to be a performance issue: memove calls are redundant and
can be avoided in SBC encoder program.

I can confirm that getting rid of the __read and __write functions (or fixing
them with your patch or its modification) is the only change, needed to pass
encoder test 08.

All the other changes are not directly related to this particular testcase,
though a fix for 'codesize' is also nice to have. It would be required for a
faster, memmove-free version of the encoder.

You have found and fixed two definite bugs (as I can see it now), that's very
good. Now it would be nice to apply the fixes to git with the descriptive
commit messages and move forward ;-)

-- 
Best regards,
Siarhei Siamashka

Re: [PATCH] SBC Encoder program

[Marcin Tolysz]

2009/1/5 Siarhei Siamashka <siarhei.siamashka@nokia.com>:
> On Monday 05 January 2009 16:42:39 ext Christian Hoene wrote:
>> > There are some parts that look a bit redundant/suspicious:
>> > > @@ -47,7 +47,7 @@ static ssize_t __read(int fd, void *buf, size_t
>> > > count) while (count > 0) {
>> > >                 len = read(fd, buf + pos, count);
>> > >                 if (len <= 0)
>> > > -                       return len;
>> > > +                       return pos > len ? pos : len;
>> > >
>> > >                 count -= len;
>> > >                 pos   += len;
>> >
>> > Is the ternary '?' operator really needed here? In this part of code we
>> > know for sure that 'len' is less or equal to zero, also 'pos ' is a
>> > positive number or zero. Having just 'return pos' should be enough.
>>
>> No. If read returns an error, the this error is passed to __read, too.
>
> I mean that 'return pos > len ? pos : len' and 'return pos' expressions are
> completely interexchangeable in this context.
>

I wonder if you
shouldn't check before you read if position is not exceeding the length?

+                   if (pos > len)  // if we are beyond end then why
bother to read more
+                         return pos;

>                 len = read(fd, buf + pos, count);
>                 if (len <= 0)
+                        return len; //to report an error
-> -                       return len;
-> +                       return pos > len ? pos : len;
and as position is always >0 and len < 0 in this place it would always
be reduced to pos
>
>                 count -= len;
>                 pos   += len;

Best wishes
Marcin Tolysz

Re: [PATCH] SBC Encoder program

[David Sainty]

Siarhei Siamashka wrote:
> On Monday 05 January 2009 16:42:39 ext Christian Hoene wrote:
>   
>>> There are some parts that look a bit redundant/suspicious:
>>>       
>>>> @@ -47,7 +47,7 @@ static ssize_t __read(int fd, void *buf, size_t
>>>> count) while (count > 0) {
>>>>                 len = read(fd, buf + pos, count);
>>>>                 if (len <= 0)
>>>> -                       return len;
>>>> +                       return pos > len ? pos : len;
>>>>
>>>>                 count -= len;
>>>>                 pos   += len;
>>>>         
>>> Is the ternary '?' operator really needed here? In this part of code we
>>> know for sure that 'len' is less or equal to zero, also 'pos ' is a
>>> positive number or zero. Having just 'return pos' should be enough.
>>>       
>> No. If read returns an error, the this error is passed to __read, too.
>>     
>
> I mean that 'return pos > len ? pos : len' and 'return pos' expressions are
> completely interexchangeable in this context.
>   

Is it trying to say: return pos > 0 ? pos : len;

That's slightly functionally different, but makes it make sense to have 
a ternary there :)  Dodgy to ignore the error though (when pos > 0).

Re: [PATCH] SBC Encoder program

[Marcin Tolysz]

Hi all,

I want to apologize  for my previous post :)

If what you want is to know is if there was an error & how much you red?
that it might not work as you cann't return two value at once (not this way).

But if you want to now if there was an error on first reading?

> @@ -47,7 +47,7 @@ static ssize_t __read(int fd, void *buf, size_t count)
>         while (count > 0) {
>                 len = read(fd, buf + pos, count);
>                 if (len <= 0)
-> -                       return len;
-> +                       return pos > len ? pos : len;
+                        return pos > 0 ? pos : len;
>
>                 count -= len;
>                 pos   += len;
this will return error code on first reading and the position
successfully red (i.e. before the error) on sequent readings. (all
assuming that pos==on entry)

You probably do not want to know the variant with position(=0) on the
first reading and error code on subsequent(before your patch)

Before your patch it was an error code all the time
After your patch it was the position all the time

Best Wishes
Marcin Tolysz

Re: [PATCH] SBC Encoder program

[Marcel Holtmann]

Hi Christian,

> > > Yes, there was a bug in the encoder program. Attached the patch.
> >
> > 
> > Coding style:
> > 
> > return pos > len ? pos : len;
> 
> Thanks. I corrected it and even found another bug.
> 
> Attached the new and corrected patch.

patch has been applied. Thanks.

Regards

Marcel

Re: SBC encoder conformance test for version 4.25 passed!

[Luiz Augusto von Dentz]

Hi,

Christian did your changes to codesize fixes the test 08, or there a
bug on 16 bit version?

--=20
Luiz Augusto von Dentz
Engenheiro de Computa=E7=E3o

Is it possible that user change a stream path between Speaker and Bluetooth like normal phone?

[Chan-Yeol Park]

Hi,

If you know or possible,
Could you explain me how to change stream path between Speaker or Bluetooth
with BlueZ gstreamer plug-in like normal phone?

In my case, I could see it works well with a gstreamer output-selector
element except some case.

1. User listens to Bluetooth Stereo Audio.
2. Switch off the headset when stream position is about 5 second from the
start of the song.
3. Gstreamer Plug-in gst_avdtp_sink_render() we could see
g_io_channel_write()works well even though headset is already off.
4. When stream position is about 9 second, g_io_channel_write() function is
blocked....-.-;
5. When it is 14 second , g_io_chhanel_write() returns with timeout error.

It's problem that at 14 second , gstreamer could not do anything even if
they know the error...

Related to this scenario , Do you have a good idea?

As a matter of fact, I try to avoid blocking , I change g_io_channel_write()
as non-blocking call.
Because Non blocking call makes the situation to the gstreamer like normal
condition. So we can change it easily as usual.

But the side effects occurs that music stream is mixed..when wireless
communication doesn't work well-.-;



Regards.
Chanyeol

Re: Is it possible that user change a stream path between Speaker and Bluetooth like normal phone?

[Bastien Nocera]

On Wed, 2009-07-15 at 23:49 +0900, Chan-Yeol Park wrote:
> Hi,
> 
> If you know or possible,
> Could you explain me how to change stream path between Speaker or Bluetooth
> with BlueZ gstreamer plug-in like normal phone?
> 
> In my case, I could see it works well with a gstreamer output-selector
> element except some case.

Use PulseAudio's Bluetooth module, that's what it's made for.

RE: SBC encoder conformance test for version 4.25 passed!

[Christian Hoene]

> > Good news! All tests passed!
> > http://net.cs.uni-tuebingen.de/html/nexgenvoip/
> 
> Thank you very much. Sorry for the delay with reply too.
> 
> I wonder if we need to do something about test 08?

Done.

> Also could you try running a special test of version 4.25 with
> SBC_HIGH_PRECISION enabled? The point is that it uses the best
> possible precision for the analysis filter and minimizes effects of
> rounding errors (I would estimate that the precision should be even
> better than single precision floating point). Having the test run with
this
> precision settings can expose some of the other minor bugs in the code
> (if they exist), which can now be hidden under the slight precision loss
> because of using 16-bit fixed point calculations.

I have uploaded the results to
http://net.cs.uni-tuebingen.de/html/nexgenvoip/4.25_high_precision

The encoding results did not change. 

Best regards,

 Christian 

Re: SBC encoder conformance test for version 4.25 passed!

[Siarhei Siamashka]

On Monday 05 January 2009 16:55:27 ext Christian Hoene wrote:
> > > Good news! All tests passed!
> > > http://net.cs.uni-tuebingen.de/html/nexgenvoip/
> >
> > Thank you very much. Sorry for the delay with reply too.
> >
> > I wonder if we need to do something about test 08?
>
> Done.
>
> > Also could you try running a special test of version 4.25 with
> > SBC_HIGH_PRECISION enabled? The point is that it uses the best
> > possible precision for the analysis filter and minimizes effects of
> > rounding errors (I would estimate that the precision should be even
> > better than single precision floating point). Having the test run with
>
> this
>
> > precision settings can expose some of the other minor bugs in the code
> > (if they exist), which can now be hidden under the slight precision loss
> > because of using 16-bit fixed point calculations.
>
> I have uploaded the results to
> http://net.cs.uni-tuebingen.de/html/nexgenvoip/4.25_high_precision
>
> The encoding results did not change.

Great, it means that sbc encoder is mostly bug free (at least none of them
shows up on this test set with all the available bugfixes applied). 

Now a good improvement would be to tweak the default 16-bit version of
analysis filter to improve its precision. It is a lot faster than the high
precision variant and already provides sufficient quality :)

-- 
Best regards,
Siarhei Siamashka

Re: SBC encoder conformance test for version 4.25 passed!

[Nick Pelly]

On Mon, Jan 5, 2009 at 12:27 AM, Siarhei Siamashka
<siarhei.siamashka@nokia.com> wrote:
> On Thursday 01 January 2009 16:24:21 ext Christian Hoene wrote:
>> Hello Siarhei,
>>
>> sorry for the delay - these days my family comes first ;-)
>>
>> Good news! All tests passed!
>> http://net.cs.uni-tuebingen.de/html/nexgenvoip/

Congratulations!