Re: [pulseaudio-discuss] module combine not respecting requested latencies

2011-05-11 Thread Maarten Lankhorst 

Hello,

Maarten Bosmans wrote:

 Please have another look at your patch. This is setting BLOCK_USEC to
 10 seconds, not ms. Moreover, I don't think BLOCK_USEC is related in
 any way to the default adjust_time, so if changing BLOCK_USEC, better
 set it to some other hardcoded value, to avoid the suggestion that it
 has anything to do with the adjust_time.


Woops my bad, in my patch I just set it to 0, you're right that patch is wrong, 
my patch was to simply change

pa_sink_input_set_requested_latency(o-sink_input, BLOCK_USEC);
to:
pa_sink_input_set_requested_latency(o-sink_input, 0);


Which sets it to minimum latency possible, however I can imagine in some 
cases you want to allow bigger latencies, so I doubt that is the 
complete solution, the total minimum latency should probably be the 
maximum of the minimum latency of each individual sink being used, while 
the real latency set should be set to minimum tolerable, but that module 
confuses me because it's both an output and an input, so I don't see how 
it should be done.


Cheers,
Maarten
___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss

[pulseaudio-discuss] module combine not respecting requested latencies

2011-05-09 Thread Maarten Lankhorst 

Hi all,

I was toying around with the combine module, but it appears it adds a 
lot latency for no good reason, changing it from 200 ms to 10 ms makes 
some applications like wine happier. I wanted to see if I could fix it 
myself, but I must admit I don't understand what the various parts of 
that code do. The fact that is's both an input and output make it even 
harder..
A naive patch to fix it is here, but it would have been nice if module 
combine would just respect the latency I ask it to set, and maybe just 
set a high latency by default otherwise.


~Maarten

diff --git a/src/modules/module-combine-sink.c 
b/src/modules/module-combine-sink.c
index 09af942..138e3e8 100644
--- a/src/modules/module-combine-sink.c
+++ b/src/modules/module-combine-sink.c
@@ -72,7 +72,7 @@ PA_MODULE_USAGE(

 #define DEFAULT_ADJUST_TIME_USEC (10*PA_USEC_PER_SEC)

-#define BLOCK_USEC (PA_USEC_PER_MSEC * 200)
+#define BLOCK_USEC DEFAULT_ADJUST_TIME_USEC

 static const char* const valid_modargs[] = {
 sink_name,


___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] underrun behavior with alsa-plugins

2011-04-20 Thread Maarten Lankhorst 

Hi David,

Op 20-04-11 09:33, David Henningsson schreef:

On 2011-04-19 18:12, Maarten Lankhorst wrote:

Hi all,

For wine I was investigating a bug with pulseaudio, it seems
alsa-plugins' pulse driver ignores underruns.


Hmm, doesn't wine come with a native pulse driver these days?
Nope, but distros patch in a buggy winepulse driver, wine is working on 
rearchitecting its driver model, so that a pulseaudio driver might be 
added after that, but the current winepulse driver was a bad 
copy/replace job of the wrong sound driver. :)



This is probably because
an underrun will force you to call snd_pcm_prepare, this will destroy
the stream and set up a similar new one again.


...whereas the proper way to resolve the underrun in the pulse plugin is
just to feed it with more data, which is likely what would happen if you
do not report the underrun.


This causes more
susceptibility to underruns, so that code was disabled.


Takashi added some kind of option so you can turn the reporting of
underruns on again, if that helps.

If you read my patch, that's what it does.


However if I
force underruns to occur,the state will stay running and it appears
there is still some data left in the buffer, so sound stalls entirely.


This is nothing I've heard of.
Sadly all too common here, if I feed data and it underruns, it may 
appear to be running but is stalled entirely.



The latency gets updated to something like 0x7bdX which
looks suspiciously much like a pointer to me, which may be a bug. If I
instead make pulse_prepare call pulse_start on underrun, it's handled
properly and sound will continue to work.

So my questions are.
1. Is the weird latency value update a bug?


I guess so, but what sense would it make to read the latency if you're
in an underrun condition - and btw, exactly what latency variable is this?
In a response to a latency event I printf the latency inside the 
callback, with pa_stream_get_latency.



Digging it up I can only
assume it's a bug in src/stream.c , but I haven't figured out why yet.
Tested with pulseaudio.c
2. Any comments on this patch for alsa-plugins?


IIRC, by setting handle_underrun to 1, you're reopening bugs of
broken/skipping audio for mpg123 and other programs, which get stuck in
an infinite loop of write one sample, force start the stream, write
more samples, asynchronusly get an underrun, drop the stream and drop
samples already written.

There should be threads on at least alsa-devel about this, from a year
back or so.
This is exactly what my patch was addressing, I was fixing that bug by 
handling underrun correctly. snd_pcm_prepare() is called when an 
underrun occurs, so instead I make it only restart the stream if 
underrun. Not handling underruns at all seems to allow it to stall on 
xrun, while claiming to be running. This sometimes appears to happen in 
other programs too though, like mplayer. Could it be because of the 
weird latency value?


Cheers,
Maarten
___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] underrun behavior with alsa-plugins

2011-04-20 Thread Maarten Lankhorst 

Hey,

Op 20-04-11 15:18, David Henningsson schreef:

However if I
force underruns to occur,the state will stay running and it appears
there is still some data left in the buffer, so sound stalls entirely.


This is nothing I've heard of.

Sadly all too common here, if I feed data and it underruns, it may
appear to be running but is stalled entirely.


Could you provide a test case where this occurs? (Pulseaudio/ALSA
version, client applications/libraries, etc)


Well this could be a basic test version I suppose, although it's not 
fair since it's guaranteed to underrun and doesn't work with dmix since 
it specifies period sizes exactly.



Digging it up I can only
assume it's a bug in src/stream.c , but I haven't figured out why yet.
Tested with pulseaudio.c
2. Any comments on this patch for alsa-plugins?


IIRC, by setting handle_underrun to 1, you're reopening bugs of
broken/skipping audio for mpg123 and other programs, which get stuck in
an infinite loop of write one sample, force start the stream, write
more samples, asynchronusly get an underrun, drop the stream and drop
samples already written.

There should be threads on at least alsa-devel about this, from a year
back or so.

This is exactly what my patch was addressing, I was fixing that bug by
handling underrun correctly. snd_pcm_prepare() is called when an
underrun occurs, so instead I make it only restart the stream if
underrun. Not handling underruns at all seems to allow it to stall on
xrun, while claiming to be running. This sometimes appears to happen in
other programs too though, like mplayer. Could it be because of the
weird latency value?


Ok, thanks for the clarifications. I've taken a closer look at your
patch now and have the following comments:

When you're calling pulse_start instead of continuing in pulse_prepare -
pulse_start will call uncork/trigger, won't that just cause another
underrun? Would it perhaps be better to just return without doing anything?

mpg123 was used as a test case for the original bug, and mpg123 calls
snd_pcm_drop on an underrun, so you will be regressing mpg123 by
changing handle_underrun to 1. (Now of course we could fix mpg123, but I
don't know if there are a lot of other programs out there doing similar
things?)

My main point is that the underrun is often obsolete when the message
reached the application, because more data has already been written,
therefore reporting it to the app does more harm than good.
At least until the underrun callback (and PA protocol) supports sending
the position of underrun, together with the underrun message. If we had
that position, we could compare that with the current write pointer to
determine whether the underrun is actually obsolete or not.
Well, this is a dumb unfair test program that forcibly underruns, 
probably should work if you change default to hw:0, but I think dmix 
doesn't allow you to set random period sizes. If I try this with the 
pulseaudio default plugin, it will break.


With my patch:
~$ ./a.out
Written: 2047 - available: 2047
Successfully underrun
Written: 960 - available: 960
Successfully underrun
Written: 1890 - available: 1890
Successfully underrun

Without:

~$ ./a.out 2/dev/null
Written: 2047 - available: 2047
Written: 0 - available: 0
Written: 0 - available: 0

Note: not any form of error checking is done here, just cobbled this 
dumb program together for demonstration purposes. :)


Cheers,
Maarten
#include stdio.h
#include unistd.h
#include asoundlib.h

int main() {
unsigned t = 1, rate = 44100;
char silence[4096] = {};
snd_pcm_hw_params_t *hw_parms;
snd_pcm_sw_params_t *sw_parms;
snd_pcm_t *pcm;

snd_pcm_hw_params_alloca(hw_parms);
snd_pcm_sw_params_alloca(sw_parms);

if (snd_pcm_open(pcm, default, SND_PCM_STREAM_PLAYBACK, SND_PCM_NONBLOCK)  0) return 1;
snd_pcm_hw_params_any(pcm, hw_parms);
snd_pcm_hw_params_set_period_size(pcm, hw_parms, 512, 0);
snd_pcm_hw_params_set_periods(pcm, hw_parms, 4, 0);
snd_pcm_hw_params_set_format(pcm, hw_parms, SND_PCM_FORMAT_S16_LE);
snd_pcm_hw_params_set_rate_near(pcm, hw_parms, rate, NULL);
snd_pcm_hw_params_set_channels(pcm, hw_parms, 1);
snd_pcm_hw_params_set_access(pcm, hw_parms, SND_PCM_ACCESS_RW_INTERLEAVED);
snd_pcm_hw_params(pcm, hw_parms);
snd_pcm_prepare(pcm);
snd_pcm_start(pcm);
snd_pcm_writei(pcm, silence, sizeof(silence)/2);

while (1) {
snd_pcm_uframes_t avail = snd_pcm_avail_update(pcm);
int state = snd_pcm_state(pcm);
if (state == SND_PCM_STATE_XRUN) {
printf(Successfully underrun\n);
avail = sizeof(silence)/2;
snd_pcm_prepare(pcm);
snd_pcm_writei(pcm, silence, avail);
} else if (state == SND_PCM_STATE_RUNNING) {
long ret = snd_pcm_writei(pcm, silence, avail);
printf(Written: %li - available: %lu\n, ret, avail);
usleep(10);
}
usleep(1);
}
}
___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de

[pulseaudio-discuss] underrun behavior with alsa-plugins

2011-04-19 Thread Maarten Lankhorst 

Hi all,

For wine I was investigating a bug with pulseaudio, it seems 
alsa-plugins' pulse driver ignores underruns. This is probably because 
an underrun will force you to call snd_pcm_prepare, this will destroy 
the stream and set up a similar new one again. This causes more 
susceptibility to underruns, so that code was disabled. However if I 
force underruns to occur, the state will stay running and it appears 
there is still some data left in the buffer, so sound stalls entirely. 
The latency gets updated to something like 0x7bdX which 
looks suspiciously much like a pointer to me, which may be a bug. If I 
instead make pulse_prepare call pulse_start on underrun, it's handled 
properly and sound will continue to work.


So my questions are.
1. Is the weird latency value update a bug? Digging it up I can only 
assume it's a bug in src/stream.c , but I haven't figured out why yet. 
Tested with pulseaudio.c

2. Any comments on this patch for alsa-plugins?

Cheers,
Maarten


xrun.patch
Description: application/mbox
___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] bluetooth: Allow frame length changes when decoding a2dp

2011-03-05 Thread Maarten Lankhorst 

Hey,

Op 05-03-11 12:04, Colin Guthrie schreef:

'Twas brillig, and Luiz Augusto von Dentz at 04/03/11 23:24 did gyre and
gimble:

Hi,

On Fri, Mar 4, 2011 at 10:48 AM, Maarten Lankhorst
m.b.lankho...@gmail.com  wrote:

My android phone sends packets with a different frame length than the
decoder initially expects, since libsbc already allows this case, just check
the current frame length.


I believe Ive send you a patch to fix this already, the patch is here:

http://gitorious.org/pulseaudio/mainline/commit/b3595d00331961164b3acf6be89c459758b47738


Luiz,

Should this either/both your master or bluetooth trees be pulled into
upstream master?

I've I've messed up doing this earlier, I apologise... :s
On a slightly related note, can sbc be upgraded using the bluez-git 
version? I seem to be hit by a clipping bug on maximum volume.


Cheers,
Maarten
___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss

[pulseaudio-discuss] [PATCH] bluetooth: Allow frame length changes when decoding a2dp

2011-03-04 Thread Maarten Lankhorst 
My android phone sends packets with a different frame length than the 
decoder initially expects, since libsbc already allows this case, just 
check the current frame length.

---
From d98ec65d36fad5e523703350485d323926e82bb6 Mon Sep 17 00:00:00 2001
From: Maarten Lankhorst m.b.lankho...@gmail.com
Date: Fri, 4 Mar 2011 14:42:17 +0100
Subject: [PATCH 1/2] bluetooth: Allow frame length changes in a2dp

---
 src/modules/bluetooth/module-bluetooth-device.c |8 +---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/modules/bluetooth/module-bluetooth-device.c 
b/src/modules/bluetooth/module-bluetooth-device.c
index dc09ffc..6a33ef0 100644
--- a/src/modules/bluetooth/module-bluetooth-device.c
+++ b/src/modules/bluetooth/module-bluetooth-device.c
@@ -1386,7 +1386,7 @@ static int a2dp_process_push(struct userdata *u) {
 pa_assert(u-source);
 pa_assert(u-read_smoother);
 
-memchunk.memblock = pa_memblock_new(u-core-mempool, u-block_size);
+memchunk.memblock = pa_memblock_new(u-core-mempool, 2 * u-block_size);
 memchunk.index = memchunk.length = 0;
 
 for (;;) {
@@ -1441,7 +1441,8 @@ static int a2dp_process_push(struct userdata *u) {
 to_decode = l - sizeof(*header) - sizeof(*payload);
 
 d = pa_memblock_acquire(memchunk.memblock);
-to_write = memchunk.length = pa_memblock_get_length(memchunk.memblock);
+to_write = pa_memblock_get_length(memchunk.memblock);
+memchunk.length = 0;
 
 while (PA_LIKELY(to_decode  0  to_write  0)) {
 size_t written;
@@ -1463,7 +1464,7 @@ static int a2dp_process_push(struct userdata *u) {
 /* pa_log_debug(SBC: frame_length: %lu; codesize: %lu, (unsigned 
long) a2dp-frame_length, (unsigned long) a2dp-codesize); */
 
 pa_assert_fp((size_t) decoded = to_decode);
-pa_assert_fp((size_t) decoded == a2dp-frame_length);
+pa_assert_fp((size_t) decoded == sbc_get_frame_length(a2dp-sbc));
 
 pa_assert_fp((size_t) written = to_write);
 pa_assert_fp((size_t) written == a2dp-codesize);
@@ -1473,6 +1474,7 @@ static int a2dp_process_push(struct userdata *u) {
 
 d = (uint8_t*) d + written;
 to_write -= written;
+memchunk.length += written;
 
 frame_count++;
 }
-- 
1.7.2.3

___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss

[pulseaudio-discuss] [PATCH] bluetooth: Do not stop decoding because of a crc error

2011-03-04 Thread Maarten Lankhorst 

---
Letting my phone play music all night I noticed it was gone in the 
morning because a crc error occurred. I don't think it should be as 
fatal as it currently is, so just try our luck again on next frame 
instead of stopping decode entirely.
From 0d3062e988c9a4f01b5bf38a48a90a685cf97078 Mon Sep 17 00:00:00 2001
From: Maarten Lankhorst m.b.lankho...@gmail.com
Date: Fri, 4 Mar 2011 14:43:58 +0100
Subject: [PATCH 2/2] bluetooth: Do not stop decoding because of a crc error

---
 src/modules/bluetooth/module-bluetooth-device.c |7 +--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/modules/bluetooth/module-bluetooth-device.c 
b/src/modules/bluetooth/module-bluetooth-device.c
index 6a33ef0..fad8e45 100644
--- a/src/modules/bluetooth/module-bluetooth-device.c
+++ b/src/modules/bluetooth/module-bluetooth-device.c
@@ -1457,7 +1457,7 @@ static int a2dp_process_push(struct userdata *u) {
 pa_log_error(SBC decoding error (%li), (long) decoded);
 pa_memblock_release(memchunk.memblock);
 pa_memblock_unref(memchunk.memblock);
-return -1;
+return decoded  0 ? decoded : -1;
 }
 
 /* pa_log_debug(SBC: decoded: %lu; written: %lu, (unsigned long) 
decoded, (unsigned long) written); */
@@ -1535,7 +1535,10 @@ static void thread_func(void *userdata) {
 else
 n_read = a2dp_process_push(u);
 
-if (n_read  0)
+if (n_read == -3)
+/* CRC error */
+n_read = 0;
+else if (n_read  0)
 goto fail;
 
 /* We just read something, so we are supposed to write 
something, too */
-- 
1.7.2.3

___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] bluetooth: Allow frame length changes when decoding a2dp

2011-03-04 Thread Maarten Lankhorst 

Hey Luiz, Op 05-03-11 00:24, Luiz Augusto von Dentz schreef:

Hi,

On Fri, Mar 4, 2011 at 10:48 AM, Maarten Lankhorst
m.b.lankho...@gmail.com  wrote:

My android phone sends packets with a different frame length than the
decoder initially expects, since libsbc already allows this case, just check
the current frame length.


I believe Ive send you a patch to fix this already, the patch is here:

http://gitorious.org/pulseaudio/mainline/commit/b3595d00331961164b3acf6be89c459758b47738
It's not in git://git.0pointer.de/pulseaudio.git ? And no I received no 
patch from you. Quick test seems to indicate it still wants to play 
though. You're probably still interested in the CRC patch, keeps it 
stable after a night of playing. ;)


Also I'm wondering, should the handsfree gateway and source support 
really be a source? It seems to me like it should be piped to output 
directly instead of being forced to use module-loopback..


Cheers,
Maarten
___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] bluetooth: Fix rendering a2dp data

2010-12-14 Thread Maarten Lankhorst 

 Hi Luiz,

Op 13-12-10 09:55, Luiz Augusto von Dentz schreef:

Hi,

On Sat, Dec 11, 2010 at 1:05 AM, Maarten Lankhorst
m.b.lankho...@gmail.com  wrote:

makes my android phone slightly happier
---
  src/modules/bluetooth/module-bluetooth-device.c |   11 ---
  1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/src/modules/bluetooth/module-bluetooth-device.c 
b/src/modules/bluetooth/module-bluetooth-device.c
index 6d31c1e..8664001 100644
--- a/src/modules/bluetooth/module-bluetooth-device.c
+++ b/src/modules/bluetooth/module-bluetooth-device.c
@@ -1387,7 +1387,7 @@ static int a2dp_process_push(struct userdata *u) {
 pa_assert(u-source);
 pa_assert(u-read_smoother);

-memchunk.memblock = pa_memblock_new(u-core-mempool, u-block_size);
+memchunk.memblock = pa_memblock_new(u-core-mempool, u-block_size * 2);
 memchunk.index = memchunk.length = 0;

Im not sure how this would help, we decode sbc frame by frame so
having twice as much memory doesn't really make any sense, there could
be that we should decode the entire buffer read, but that would take
much more memory. What could be the real problem is that the source is
changing the bitpool but that shouldn't be a problem since the
block_size is supposed to be calculated from maximum bitpool range so
the buffer will always be big enough.
Well it seems the block_size is not fixed on my phone, sometimes it is 1 
sample more, sometimes 2, the net effect is 1 or 2 frames stay undecoded 
leading to massive underruns.


I don't think u-block_size has to be fixed per se, a2dp-code_size 
seems to be though.

 for (;;) {
@@ -1442,7 +1442,8 @@ static int a2dp_process_push(struct userdata *u) {
 to_decode = l - sizeof(*header) - sizeof(*payload);

 d = pa_memblock_acquire(memchunk.memblock);
-to_write = memchunk.length = pa_memblock_get_length(memchunk.memblock);
+to_write = pa_memblock_get_length(memchunk.memblock);
+memchunk.length = 0;

 while (PA_LIKELY(to_decode  0  to_write  0)) {
 size_t written;
@@ -1464,7 +1465,7 @@ static int a2dp_process_push(struct userdata *u) {
  /* pa_log_debug(SBC: frame_length: %lu; codesize: %lu, (unsigned 
long) a2dp-frame_length, (unsigned long) a2dp-codesize); */

 pa_assert_fp((size_t) decoded= to_decode);
-pa_assert_fp((size_t) decoded == a2dp-frame_length);
+pa_assert_fp((size_t) decoded= a2dp-frame_length);

This is the real problem, either we do this if the bitpool is changing
or we have to call sbc_reinit in each frame.
Well, a2dp-frame_length was calculated based on the maximum bitpool 
size. The frames decoded have a smaller bitpool size. a2dp-codesize is 
still the same though, so I don't think this change matters much.

 pa_assert_fp((size_t) written= to_write);
 pa_assert_fp((size_t) written == a2dp-codesize);
@@ -1474,10 +1475,14 @@ static int a2dp_process_push(struct userdata *u) {

 d = (uint8_t*) d + written;
 to_write -= written;
+memchunk.length += written;

 frame_count++;
 }

+if (to_decode)
+pa_log_error(SBC: %lu bytes not decoded\n, to_decode);
+
 pa_memblock_release(memchunk.memblock);

Actually I think we should be solving this in libsbc, both sbc_encode
and sbc_decode should be checking if the bitpool has changed and
reinit automatically, otherwise we gonna get performance penalty doing
sbc_reinit for every frame.
I don't think the bitpool changed, the code just assumes the maximum 
bitpool size, while my phone sends data at a lower one.



Of course I still don't know how to handle packet loss correctly, which 
is easy to create by moving phone out of the range of my bluetooth device.


Cheers,
Maarten
___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss

[pulseaudio-discuss] [PATCH] module-loopback: Prevent an infinite loop when rate adjusting is disabled

2010-12-11 Thread Maarten Lankhorst 
---
 src/modules/module-loopback.c |3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/src/modules/module-loopback.c b/src/modules/module-loopback.c
index d94ea93..fee17f4 100644
--- a/src/modules/module-loopback.c
+++ b/src/modules/module-loopback.c
@@ -503,7 +503,8 @@ static int sink_input_process_msg_cb(pa_msgobject *obj, int 
code, void *data, in
 
 pa_assert_ctl_context();
 
-adjust_rates(u);
+if (u-adjust_time  0)
+adjust_rates(u);
 return 0;
 }
 }
-- 
1.7.1

___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] rtkit: add pid as argument

2010-05-08 Thread Maarten Lankhorst 
Hello,

2010/5/7 David Henningsson launchpad@epost.diwic.se:

 On 2010-04-25 22:11, Lennart Poettering wrote:
 On Sun, 25.04.10 21:41, David Henningsson (launchpad@epost.diwic.se) 
 wrote:


 which handle corresponds to the thread, or even if that handle is local
 or not. Wineserver controls this information so all requests that
 involve handles involve a wineserver call, in general. So racing cannot
 happen, because wineserver is the only one making the requests.

 Just a question, what about RLIMIT_RTTIME, which rtkit requires to be
 set for enabling rt? AFAIK there is no equivalent in Windows.

 Well, we enforce RLIMIT_RTTIME mostly because we can, not because it
 would really inhibit all kind of RT-related misbehaviour of the clients.

 Or in other words: RLIMIT_RTTIME is quite useful as a protection against
 programming errors somewhre, however it is still easily circumventable
 by people who really try to do evil things, unless combined with other
 tricks, such as SCHED_RESET_ON_FORK.

 So let my rephrase my question to Maarten: Since there is no equivalent
 to RLIMIT_RTTIME in Windows, applications might assume they can run in
 RT for extended periods of time. This might be considered bad
 application behavior in any operating system, but nonetheless - do we
 risc regressions when these applications are being killed, instead of
 the previous behavior which just let them run in non-RT?
I don't think this will happen, if it is going to I will handle
SIGXCPU in wine, and just demote all realtime threads back to normal
priority when that signal is sent by the kernel.

Cheers,
Maarten
___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] rtkit: add pid as argument

2010-05-04 Thread Maarten Lankhorst 

Hello,

On 25-04-10 02:52, Lennart Poettering wrote:

...snip
OK, I am convinced. ;-)

And yes, your use of rktit certainly makes a lot of sense.

Patch looks goot to me. But could you fix one thing: I think
MakeThreadRealtimeWithPID() (or something like tht) would be a better
name than MakeThreadRealtime2(). After examples like wait(), wait3() and
wait4() in Linux, I cannot say I am a big fan of numbering function
calls. ;-)
   
Ok, renamed them to WithPID with a sed, and tested in wine just to be 
sure. :)


Cheers,
Maarten
From 1c32863c194611f39d523b6cb76082e7d33df44b Mon Sep 17 00:00:00 2001
From: Maarten Lankhorst m.b.lankho...@gmail.com
Date: Wed, 5 May 2010 00:50:08 +0200
Subject: [PATCH] rtkit: Add WithPID functions to allow setting scheduler 
remotely

---
 rtkit-daemon.c |   70 +--
 1 files changed, 52 insertions(+), 18 deletions(-)

diff --git a/rtkit-daemon.c b/rtkit-daemon.c
index 6b12663..154889a 100644
--- a/rtkit-daemon.c
+++ b/rtkit-daemon.c
@@ -4,6 +4,7 @@
   This file is part of RealtimeKit.
 
   Copyright 2009 Lennart Poettering
+  Copyright 2010 Maarten Lankhorst
 
   RealtimeKit is free software: you can redistribute it and/or modify
   it under the terms of the GNU General Public License as published by
@@ -72,10 +73,20 @@
 arg name=\thread\ type=\t\ 
direction=\in\/\n \
 arg name=\priority\ type=\u\ 
direction=\in\/\n \
 /method\n   \
+method name=\MakeThreadRealtimeWithPID\\n
\
+arg name=\process\ type=\t\ 
direction=\in\/\n \
+arg name=\thread\ type=\t\ 
direction=\in\/\n \
+arg name=\priority\ type=\u\ 
direction=\in\/\n \
+/method\n   \
 method name=\MakeHighPriority\\n  \
 arg name=\thread\ type=\t\ 
direction=\in\/\n \
 arg name=\priority\ type=\i\ 
direction=\in\/\n \
 /method\n   \
+method name=\MakeHighPriorityWithPID\\n  
\
+arg name=\process\ type=\t\ 
direction=\in\/\n \
+arg name=\thread\ type=\t\ 
direction=\in\/\n \
+arg name=\priority\ type=\i\ 
direction=\in\/\n \
+/method\n   \
 method name=\ResetKnown\/\n   \
 method name=\ResetAll\/\n \
 method name=\Exit\/\n \
@@ -1064,11 +1075,12 @@ static int lookup_client(
 struct thread **_t,
 DBusConnection *c,
 DBusMessage *m,
+pid_t pid,
 pid_t tid) {
 
 DBusError error;
 int r;
-unsigned long pid, uid;
+unsigned long uid;
 unsigned long long starttime;
 struct rtkit_user *u;
 struct process *p;
@@ -1083,7 +1095,8 @@ static int lookup_client(
 goto fail;
 }
 
-if ((pid = get_unix_process_id(c, dbus_message_get_sender(m), error)) 
== (unsigned long) -1) {
+if (pid == (pid_t) -1 
+(pid = get_unix_process_id(c, dbus_message_get_sender(m), error)) 
== (pid_t) -1) {
 syslog(LOG_ERR, get_unix_process_id() failed: %s\n, 
error.message);
 r = -EIO;
 goto fail;
@@ -1292,14 +1305,16 @@ static int handle_dbus_prop_get(const char* property, 
DBusMessage *r) {
 static DBusHandlerResult dbus_handler(DBusConnection *c, DBusMessage *m, void 
*userdata) {
 DBusError error;
 DBusMessage *r = NULL;
+int is2 = 0;
 
 dbus_error_init(error);
 
 /* We garbage collect on every user call */
 user_gc();
 
-if (dbus_message_is_method_call(m, org.freedesktop.RealtimeKit1, 
MakeThreadRealtime)) {
-uint64_t thread;
+if (dbus_message_is_method_call(m, org.freedesktop.RealtimeKit1, 
MakeThreadRealtime) ||
+(is2 = dbus_message_is_method_call(m, 
org.freedesktop.RealtimeKit1, MakeThreadRealtimeWithPID))) {
+uint64_t thread, process = (uint64_t) -1;
 uint32_t priority;
 struct rtkit_user *u;
 struct process *p;
@@ -1311,18 +1326,27 @@ static DBusHandlerResult dbus_handler(DBusConnection 
*c, DBusMessage *m, void *u
 goto finish;
 }
 
-if (!dbus_message_get_args(m, error,
-   DBUS_TYPE_UINT64, thread,
-   DBUS_TYPE_UINT32, priority

[pulseaudio-discuss] rtkit: add pid as argument

2010-04-24 Thread Maarten Lankhorst 
I'm not sure if I have the right mailing list, since there doesn't 
appear to be a a mailing list for rtkit.


This is a patch that would allow you to set the realtime priority for 
another process then the one calling rtkit over dbus. This can be useful 
for a 'setsched' like utility, or for wine. Extra validation on the 
given pid isn't needed, since verify_process_user will do this anyhow.


Cheers,
Maarten


rtkit-add-pid-as-argument.patch
Description: application/mbox
___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] rtkit: add pid as argument

2010-04-24 Thread Maarten Lankhorst 

Hello Lennart,

On 25-04-10 01:42, Lennart Poettering wrote:

On Sat, 24.04.10 01:18, Maarten Lankhorst (m.b.lankho...@gmail.com) wrote:

   

I'm not sure if I have the right mailing list, since there doesn't
appear to be a a mailing list for rtkit.

This is a patch that would allow you to set the realtime priority
for another process then the one calling rtkit over dbus. This can
be useful for a 'setsched' like utility, or for wine. Extra
validation on the given pid isn't needed, since verify_process_user
will do this anyhow.
 

Hmm, while I am not strictly against this I don't really see the use for
this. The primary reason I came up with rtkit was to allow us to ship PA
and similar software with RT privs out-of-the-box and strictly control
access to RT for that. A command line tool like you seem to suggest does
not really fit into that out-of-the-box scheme.

What concerns me a bit here is that RT programs must be written in an
RT specific style to make sense. That means they must internally know
which threads to make RT and which ones not, and when. From the
outside of the codebase that is difficult to control, especially since
information about threads in processes is not really available from the
outside on Linux (yes, you can find out about them, but you don't really
know which one is which...).
   
Well, the use case would be wine's wineserver. On windows programs 
usually set audio threads to THREAD_PRIORITY_TIME_CRITICAL to indicate 
that they have to have a certain priority. But in windows thread handles 
are global, so doing it inside wine's 'ntdll' library wouldn't make much 
sense, since the request to set the priority would go to wineserver 
first, since there's no way to tell from inside a wine program to tell 
which handle corresponds to the thread, or even if that handle is local 
or not. Wineserver controls this information so all requests that 
involve handles involve a wineserver call, in general. So racing cannot 
happen, because wineserver is the only one making the requests.


If those applications would try to do bad things on windows xp you could 
lock up the entire system, since it will not reset the scheduler, even 
if that means on a single core system that it will lock everything else 
out. ;)



Also, RT programs must be RT from beginning of their RT code to the
end. If you control their RT scheduling only from the outside this is
almost definitely racy. If you want to control RT privs from outside of the
process, then the only non-racy way would be to use inheriting across an
exec(), which however is something rtkit explcitily prohibits.
   
I am also wondering how many people would use this functionality instead

of the lower level, chrt(1) command?
   
Well, I'm aware rtkit is pulseaudio only, but there have been some 
people who already use SCHED_RR with wine, mainly people who use it for 
professional audio with jack or asio. This patch is not officially part 
of wine, because of the possibility of abuse and forkbombs. Out of tree 
patches are used.


For wine I already had a patch that used rtkit, but it queued a 
wine-rtkit-specific apc to the thread involved, and involved opening 
dbus inside wine's ntdll, which is not really a good place to do it. And 
not likely to get accepted to wine as result.

So, as mentioned I am not strictly against this, but I'd be interested
to hear some good use cases for this, simply because I fear that people
might otherwise be invited to write improper RT code.
   
I'd like to point out I already had it working without the patch, but 
the methods I used were a lot racier and uglier, because I had to queue 
the request to the target thread involving a wine-rtkit-specific apc. Of 
course this was just a proof of concept, so I wanted a cleaner solution. 
You could also write a chrt now that doesn't require cap_sys_nice to 
use, but this patch was mainly written wine.


What would prevent a process from using ptrace on another process with 
the same uid, and then request realtime priorities from it? I just want 
it somewhat easier, in a way that I can use it in wine. :)

So, convince me!
   
I'm not sure how this patch would encourage writing improper rt code, 
since you could just use rtkit.c in your project for rt already. This 
will just let wine benefit from the existing functionality in a saner way.


Cheers,
Maarten
___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss