_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi again,
On 8/20/07, Fabien Chevalier <[email protected]> wrote:
> I think these issues you have (too fast, too slow) is because the poll()
> support in your version of pcm_bluetooth is broken.
> I suggest you have a look at the pulse and jack plugin (available in
> lastest alsa-plugins), and also to my patch to see how it fits in the
> picture.
Yep, it seems you are right on this. My solution even consume more
cpu and solves nothing, too bad I couldn't figure out this before.
> A correct implementation of poll is the only way you will be able to
> wake up the application only when you are ready to accept more data.
> This means you must never block in the bluetooth_a2dp_write functions,
> either explicitely (through pthread_mutex_lock for instance :-) ) or
> implicitely (using a blocking send call on a socket for instance).
> The blocking is done by libasound itself, by polling on the fd's you
> give to it.
Not sure how we could avoid blocking on transfer callback, that was
my idea but as I need to protect the buffer from being concurrent
accessed I end up with almost the same solution we already had.
Apart from that it seems you already solve the rate problem by properly
handle hw_ptr on a thread, adding the rate constrait spec mention
whould probably only add cpu consumption.
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
>> Oh... about this bitrate stuff i must say i'm quite confused : where did
>> you find anything in the spec ? (I have A2DP 1.0 but didn't find
>> anything :-( )
>
> Spec 1.2 is already available, but even in it is difficult to find as it is
> in Appendix part 12.9 page 66. Im not sure if the rate is mandatory
> or not, but as we have some headsets surfer from speed problem
> it might be a rate issue.
Ok thanks, i'm gonna have a closer look to it then. :-)
>
>>> I also implement a circular
>>> buffer for encoded sbc frames to be consumed by a
>>> thread.
>> That was another option, however it was not that clear what the benefits
>> from it would be, so i took the easiest to implement alternative, and
>> just sent data as they arrived ;-)
>
> It seems it is not always the case we are ready to send right away,
> this is what Im trying to figure out since the buffer sometimes got
> full (encoding process is too fast) or even (weird) I got no frames
> to sent on time thread (encoding too slow).
I think these issues you have (too fast, too slow) is because the poll()
support in your version of pcm_bluetooth is broken.
I suggest you have a look at the pulse and jack plugin (available in
lastest alsa-plugins), and also to my patch to see how it fits in the
picture.
A correct implementation of poll is the only way you will be able to
wake up the application only when you are ready to accept more data.
This means you must never block in the bluetooth_a2dp_write functions,
either explicitely (through pthread_mutex_lock for instance :-) ) or
implicitely (using a blocking send call on a socket for instance).
The blocking is done by libasound itself, by polling on the fd's you
give to it.
If you have any questions i'm gonna try to be o IRC tomorrow.
Cheers,
Fabien
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Marcel,
please find below some comments.
>
> I tested the patch with my Logitech headphone and it makes beep work
> fine. Using xmms has serious problems. This keeps happening:
>
> ** WARNING **: alsa_write_audio(): write error: Resource temporarily unavailable
Interesting... that didn't happen to me !
>
> However I do have blanks with beep, too. It can also crash:
Looks like the same issue XMMS has, where underrun can occasionally happen.
>
> beep-media-player: pcm_bluetooth.c:386: a2dp_playback_hw_thread:
> Assertion `ret == 1' failed.
>
> This is triggered by a song end where the stream is still open, but beep
> fails to enhance to the next track.
That basically means beep forgots to close the stream in a timely maneer
after those kind of issues. I should SNDERR() something rather than
assert on this one however...
>
> Using usleep() seems not precise enough to me.
Well considering that you definetely *don't need* any kind of precise
timing for streaming in general, and a2dp in particular, usleep is an
order of magnitude too precise already. In fact 10 ms granularity would
be more than enough.
Also we might not wanna
> use a pipe. Some other notification mechanism might be better. I don't
> really know what solution is best here.
Well, it has to be conveyed using a file descriptor due to ALSA using
poll() to be notifyed of hardware wakeup.
So unless anybody has a better idea, i consider a pipe as the best way
to go ;-)
>
> When using mplayer to watch a movie, I have sound blanks from time to
> time and it also crashed:
>
> mplayer: pcm_bluetooth.c:386: a2dp_playback_hw_thread: Assertion `ret == 1' failed.
Could you be more precise ? Which kind of codec ? For how long did you
play ?
> MPlayer interrupted by signal 6 in module: sleep_timer
>
> However it is a huge improvement and a big step in the right direction.
> I know that Luiz was working on a similar patch. Try to get on #bluez on
> freenode.org so the future work can be coordinated.
Will try, however ML is easier for me as when i work on bluez stuff i
usually only have an old RTC line :-(
>
> Besides that, please fix the coding style. I am serious about that. I am
> picky about whitespace placements, but you already knew that:
>
> Every "if" is following by a whitespace like "if (". No exceptions.
Hmmm... is this rule only for if's are is it valid for any kind of
reserved key word ??
>
> Also stuff like this is not acceptable:
>
> pfd[0].fd = ((struct bluetooth_data *)io->private_data)->stream_fd;
>
> If I have to twist my mind too much to understand why this code is
> correct it is too complex. Do something like this:
>
> struct bluetooth_data *data = io->private_data;
> pfd[0].fd = data->stream_fd;
As you wish...
>
> Don't invert the result of a function call:
>
> ret = -pthread_create(&a2dp_data->hw_thread,
>
> However thought this is a good idea is wrong. This is big hole for
> overlooking sign errors. Don't do it. Do this instead:
>
> return -ret
You're right that wasn't the smartest piece of code i wrote recently
:-)
>
> And while we are at it. Don't initiate variables with a value:
>
> int ret = 0;
>
> This is only needed in a few rare cases and will hide errors in the code
> flow that otherwise gcc might have caught.
Hmmm... I'm quite surprised : good programming practices tend to
recommend to always initialize variables, unless this is obvious enough
that the variable gets initialized right after declaration : could you
bring more arguments under the hood ? I'm far from being conviced...
>
> And instead of "ret" use "err" which I prefer.
Makes sense as it is an error code anyway...
>
> The assert() statements are nice for development, but we can't have them
> in the final version.
We have to prevent asserts to be executed on final version, that doesn't
mean we have to remove them from the code however.
The correct way to use asserts is to have them enabled only when
configuring using --enable-debug
If it's not currently the case on bluez-utils i suggest we patch its
build process... and keep the asserts :-)
> We have to survive as much errors as possible and
> better play nothing than crash.
Agree.
>
> The "for(;;) {" statement are better replaced with "while (1) {".
>
> For the "volatile snd_pcm_sframes_t hw_ptr;". Do you think that is
> enough and we not better use a lock or a mutex?
That's enough for my case, where i have a single writer. Volatile is
just to prevent the compiler to optimize away by keeping a variable in
registers while it's value in main memory is changed by another thread.
Regards,
Fabien
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Fabien,
On 8/20/07, Fabien Chevalier <[email protected]> wrote:
> Hi Luiz,
>
> Please find a few remarks below:
>
> > I got almost the same idea implemented in my git
> > (git://git.infradead.org/users/vudentz/bluez-utils.git).
>
> Should make sense then :-)
>
> There is
> > alot of simplification you already did, but I consider the
> > bitrate as a2dp spec says.
>
> Oh... about this bitrate stuff i must say i'm quite confused : where did
> you find anything in the spec ? (I have A2DP 1.0 but didn't find
> anything :-( )
Spec 1.2 is already available, but even in it is difficult to find as it is
in Appendix part 12.9 page 66. Im not sure if the rate is mandatory
or not, but as we have some headsets surfer from speed problem
it might be a rate issue.
> > I also implement a circular
> > buffer for encoded sbc frames to be consumed by a
> > thread.
>
> That was another option, however it was not that clear what the benefits
> from it would be, so i took the easiest to implement alternative, and
> just sent data as they arrived ;-)
It seems it is not always the case we are ready to send right away,
this is what Im trying to figure out since the buffer sometimes got
full (encoding process is too fast) or even (weird) I got no frames
to sent on time thread (encoding too slow).
-- =
Luiz Augusto von Dentz
Engenheiro de Computa=E7=E3o
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Luiz,
Please find a few remarks below:
> I got almost the same idea implemented in my git
> (git://git.infradead.org/users/vudentz/bluez-utils.git).
Should make sense then :-)
There is
> alot of simplification you already did, but I consider the
> bitrate as a2dp spec says.
Oh... about this bitrate stuff i must say i'm quite confused : where did
you find anything in the spec ? (I have A2DP 1.0 but didn't find
anything :-( )
> I also implement a circular
> buffer for encoded sbc frames to be consumed by a
> thread.
That was another option, however it was not that clear what the benefits
from it would be, so i took the easiest to implement alternative, and
just sent data as they arrived ;-)
Cheers,
Fabien
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Fabian,
I got almost the same idea implemented in my git
(git://git.infradead.org/users/vudentz/bluez-utils.git). There is
alot of simplification you already did, but I consider the
bitrate as a2dp spec says. I also implement a circular
buffer for encoded sbc frames to be consumed by a
thread.
On 8/19/07, Fabien Chevalier <[email protected]> wrote:
> Johan & All,
>
> Following recent mailing list discussions, i decided to implement some
> of the ideas that have been suggested recently. :-)
>
> So i heavily modified the A2DP streaming part of the pcm plugin
>
> The user-visible features of this patch:
> * much reduced latency (You should love this one Frederic)
> * supports headsets that are unable to throttle the data flow and
> that play too fast or drop packets or whatever (Marcel, could you
> please confirm that fixes the issues you had with some headsets ?).
>
> This patch is fairly intrusive, and removes some code that i judjed
> useless now, like the bandwidth measurement stuff, and has greatly
> simplifyed transfer loop for avdtp (see comments in the code for the
> justifications)
>
> It brings in a thread that has for only reponsibility to increment the
> so called hardware pointer (which doesn't point to anything in our
> case), and notify the application trough writing in a pipe that some
> room is available (virtual room again), so that it could send more data.
>
> The attached file details the tests i've run to check i didn't break
> anything. All is conclusive except playing with XMMS that goes underrun
> for a reason i haven spotted yet. Apart from that aplay, mplayer,
> gstreamer are fine.
>
> I'm looking forward for any questions you have on the patch.
>
> Nest step would be to fix the SCO part of the plugin, but this one is
> looks harder as i think it will require tweaking the kernel SCO part.
>
> Regards,
>
> Fabien
>
> Bluetooth alsa a2dp non regression cookbook
> -------------------------------------------
>
> I) PCM test tool shipped with ALSA
>
> Setup:
> *Download the alsa-utils source for the binary that run on your machine
> *build the pcm executable in the test directory
>
> Run:
> ./pcm -c 2 -r 48000 -Dbluetooth -m write -b 10000
> ./pcm -c 2 -r 48000 -Dbluetooth -m write_and_poll -b 10000
> ./pcm -c 2 -r 48000 -Dbluetooth -m direct_write -b 10000
>
> Result:
> You should hear a clear sine in the headset.
> pcm process should not eat 100% cpu (check with top)
> Occasionnal sound drops are OK (due to quite short buffering of 10 ms)
>
> II) Play a file from XMMS
>
> III) Play file from totem
> Setup:
> * gconftool -t string -s /system/gstreamer/0.10/default/musicaudiosink "a=
lsasink device=3Dbluetooth"
>
> Run:
> Launch totem, then select a file for play
>
> III) PLay a file from mplayer
> mplayer -ao alsa:device=3Dbluetooth filetoplay.wav
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Splunk Inc.
> Still grepping through log files to find problems? Stop.
> Now Search log events and configuration files using AJAX and a browser.
> Download your FREE copy of Splunk now >> http://get.splunk.com/
> _______________________________________________
> Bluez-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/bluez-devel
>
>
>
-- =
Luiz Augusto von Dentz
Engenheiro de Computa=E7=E3o
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Fabien,
> Following recent mailing list discussions, i decided to implement some
> of the ideas that have been suggested recently. :-)
>
> So i heavily modified the A2DP streaming part of the pcm plugin
>
> The user-visible features of this patch:
> * much reduced latency (You should love this one Frederic)
> * supports headsets that are unable to throttle the data flow and
> that play too fast or drop packets or whatever (Marcel, could you
> please confirm that fixes the issues you had with some headsets ?).
>
> This patch is fairly intrusive, and removes some code that i judjed
> useless now, like the bandwidth measurement stuff, and has greatly
> simplifyed transfer loop for avdtp (see comments in the code for the
> justifications)
>
> It brings in a thread that has for only reponsibility to increment the
> so called hardware pointer (which doesn't point to anything in our
> case), and notify the application trough writing in a pipe that some
> room is available (virtual room again), so that it could send more data.
>
> The attached file details the tests i've run to check i didn't break
> anything. All is conclusive except playing with XMMS that goes underrun
> for a reason i haven spotted yet. Apart from that aplay, mplayer,
> gstreamer are fine.
>
> I'm looking forward for any questions you have on the patch.
I tested the patch with my Logitech headphone and it makes beep work
fine. Using xmms has serious problems. This keeps happening:
** WARNING **: alsa_write_audio(): write error: Resource temporarily unavailable
However I do have blanks with beep, too. It can also crash:
beep-media-player: pcm_bluetooth.c:386: a2dp_playback_hw_thread:
Assertion `ret == 1' failed.
This is triggered by a song end where the stream is still open, but beep
fails to enhance to the next track.
Using usleep() seems not precise enough to me. Also we might not wanna
use a pipe. Some other notification mechanism might be better. I don't
really know what solution is best here.
When using mplayer to watch a movie, I have sound blanks from time to
time and it also crashed:
mplayer: pcm_bluetooth.c:386: a2dp_playback_hw_thread: Assertion `ret == 1' failed.
MPlayer interrupted by signal 6 in module: sleep_timer
However it is a huge improvement and a big step in the right direction.
I know that Luiz was working on a similar patch. Try to get on #bluez on
freenode.org so the future work can be coordinated.
Besides that, please fix the coding style. I am serious about that. I am
picky about whitespace placements, but you already knew that:
Every "if" is following by a whitespace like "if (". No exceptions.
Also stuff like this is not acceptable:
pfd[0].fd = ((struct bluetooth_data *)io->private_data)->stream_fd;
If I have to twist my mind too much to understand why this code is
correct it is too complex. Do something like this:
struct bluetooth_data *data = io->private_data;
pfd[0].fd = data->stream_fd;
Don't invert the result of a function call:
ret = -pthread_create(&a2dp_data->hw_thread,
However thought this is a good idea is wrong. This is big hole for
overlooking sign errors. Don't do it. Do this instead:
return -ret
And while we are at it. Don't initiate variables with a value:
int ret = 0;
This is only needed in a few rare cases and will hide errors in the code
flow that otherwise gcc might have caught.
And instead of "ret" use "err" which I prefer.
The assert() statements are nice for development, but we can't have them
in the final version. We have to survive as much errors as possible and
better play nothing than crash.
The "for(;;) {" statement are better replaced with "while (1) {".
For the "volatile snd_pcm_sframes_t hw_ptr;". Do you think that is
enough and we not better use a lock or a mutex?
Regards
Marcel
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel