December 2012 - Wine-Devel - list.winehq.org

Re: [PATCH] dsound: Use event based threads

by Maarten Lankhorst

Op 03-12-12 17:59, Andrew Eikum schreef: > (...) I think the best solution is to have dsound call into the system > openal to perform multichannel mixing and 3D processing. (...) > > Andrew ... Must print this out on poster format, and frame this above my bed.. ~Maarten

Dec. 3, 2012

Re: [PATCH] dsound: Use event based threads

by Maarten Lankhorst

Op 03-12-12 14:59, Joerg-Cyril.Hoehle(a)t-systems.com schreef: > Hi, > > Maarten Lankhorst queried: >> Bump, anything wrong with this patch? > Here's my 0.0$ ... (standard DSound disclaimer here...) > > Using mmdevapi's events in DSound is basically TRT. > Then, the DSound mixer will be synchronized with mmdevapi writes. > > >> + /* ALSA is retarded, add a timeout.. */ >> + ret = WaitForSingleObject(dev->sleepev, dev->sleeptime); > How does that comment help the reader of the code? > >> + if (period_ms <= 15) >> + device->sleeptime = period_ms * 5 / 2; >> + else >> + device->sleeptime = period_ms * 3 / 2; > I expect such threshold functions to be continuous or at least monotonic. > A sawtooth is certainly unexpected. What is its purpose? > > What I would understand is a comment in the code saying that as of 2012, > all 3 wine*.drv mmdevapi drivers have a bug that they stop sending events > after IAudioClient_Stop. Therefore add a short timeout to the wait. Yeah, but in a followup commit I'm going to stop sending IAudioClient_Stop entirely. :-) > If that's the reason for the timeout, then period_ms * 1.5 is > perfectly fine. There's no reason for a particular threshold. Note > that neither 1.5 nor 2.5 give you regular spacing around the time of > transition from playing to stopped. I just didn't want to make the timeout too short in case no processing is done yet. Alsa's native period is ~ 22ms (1024 samples / 44100 or 48000) with dmix despite claiming it to be otherwise.. I thought about doing something more complicated to make it smoother, but I'm really just increases it >2 buffers to compensate for jitter on lower latencies, so if it the timeout would be beyond 20 ms I don't care any more.. > > + IAudioClient_GetStreamLatency(device->client, &period); > + period_ms = (period + 9999) / 10000; > If IAC_Stop is the reason for the sleep time out, then it's obvious > that GetStreamLatency has no business here, rather than the device period. > > > I could understand a use of GetStreamLatency when it comes to computing > a reasonable size of the primary buffer. Got it right.. and this is a perfect valid use of IAudioClient GetStreamLatency here, the device could also be dead (AUDCLNT_E_DEVICE_INVALIDATED), in which case events are probably not fired any more. GetStreamLatency is also used for calculating the period size, see http://msdn.microsoft.com/en-us/library/windows/desktop/dd370874%28v=vs.85%… I don't think it's used as such in this commit yet, but in the mixer rework it's used it to calculate fragment length. > > + ret = WaitForSingleObject(dev->sleepev, dev->sleeptime); > + ... if (ret) > WaitFor* return values are defined in terms of WAIT_OBJECT_0, WAIT_TIMEOUT > and WAIT_FAILED etc., not zero or non-zero. Except WAIT_OBJECT_0 is defined as 0. > >> + device->sleepev = CreateEventW(0, 0, 0, 0); >> + device->thread = CreateThread(0, 0, DSOUND_mixthread, device, 0, 0); > I haven't checked the overall style of DSound, but I prefer FALSE and > TRUE be used for things advertised as BOOL. > (I tend to write NULL for null pointer initialisation. > Note that I easily read "if (foo) ..." for boolean and pointer types; > I don't expect "if (foo == TRUE) or (foo != NULL) ...") > > > BTW, I still believe that mixing and resampling would find their best place > in mmdevapi, not DSound. Well some way for dsound and winmm to use them both, so winmm can do resampling internally.

Dec. 3, 2012

Re: winepulse test data review

by Maarten Lankhorst

Op 03-12-12 15:42, Joerg-Cyril.Hoehle(a)t-systems.com schreef: > Hi, > > Maarten Lankhorst kindly posted mmdevapi test results for render and capture > gathered using his winepulse driver: > http://www.winehq.org/pipermail/wine-devel/2012-October/097602.html > >> render.c:1199: padding 1250 position 510000/21250 slept 470ms iteration 0 > I've run your data through some MS-Excel analysis of mine. > Here are my observations. > > >> Exclusive mode tests aren't done, I disabled exclusive mode >> because I felt tests were insufficient. > It's funny that you say this, because your winepulse driver behaves > much like an exclusive mode driver, not a shared mode one ;-) > > More specifically, > > a) winepulse and exclusive mode do not decrease padding in multiples > of the period size. It typically will keep it in multiples if pulseaudio is run in timer driven mode (glitchfree audio), however if you create a buffer that's 2x period size or less it seems to be more aggressive with updates. > b) winepulse and exclusive mode appear to equate > GetPosition with released frames minus GetCurrentPadding > exactly like ALSA hw:0 and dmix do: > delay + avail_update = buffer_size (~= 8192 frames) > > IOW, the driver pretends that the speaker position is exactly the > frame that just fell off the circular buffer. This may be ok for ALSA > hw:0 and MS-Windows exclusive mode drivers, but that's wrong for > anything with a filter chain in the back. > > Hence I assume that winepulse's GetPosition is lying. > The question is: does it matter? Well from how I understand with winepulse pulseaudio will only add 1 more period on top of that. I do or did have code for IAudioClock::GetPosition, winepulse v18 commit removed it entirely. I could reinstate it, but I didn't find the mmdevapi tests strict enough to justify it. > If using PA API calls of some sort, you manage to keep the PA-internal > latency well below what's the human brain can perceive, I'd say that's > ok. Likewise, hw:0 ignores Digital->Analog and wire->LC->speaker times. > > Our gripe with PA is precisely that using the regular ALSA->PA path > via the ALSA plugin, nobody found a means to keep PA's latency small. > > (I'll repeat myself saying that what actually matters is the sum of all > latencies and buffers: mmdevapi buffer, PA transformation chain, DAC etc.) > > The caveat is that with increasing latency, the behaviour of GCP and > GetPosition will differ more and more from native's mixer, until > it becomes too much for application X to bear, causing a bug report. > > > That was about the relationship between GCP and GetPosition. Now what > about GetPosition and wall time (as seen by the HighPerformanceTimer)? > The largest delta is 8ms, which suggests that your GetPosition is not > particularly regular, but as it stays below one 10ms period, it's > presumably good enough. By contrast, native's largest delta is < 1ms. Delta shouldn't be so high, are you using the timestamps from IAudioClock2::GetPosition ? Those are lies, I could probably fix that by recording them in pulse_wr_callback. But you could try testing with my tree, I enable rtkit in there which should decrease jitter regardless. >> Sometimes up to 2 tests fail due to position updates not being regular, > Indeed, the data reveals it. I guess the reported GetPosition is more related to > packet size job completion than actual sample accurate stream position. >> I suspect this is because it's nearly done playing though, but it can get worse >> when you use usb headphones, not much I can do about that though, except if I >> started lying, > What do you mean with "started lying"? I inferred above that it's already lying. Not directly passing the value in GetCurrentPadding through as was reported by pulseaudio, but hide the irregular pointner advancements. I prefer to just report the values from pulseaudio directly. >> but it's so far been much simpler when I don't.. > The data points look very reasonable -- if audible latency is really > low, which the tests cannot reveal. > > > BTW, I'd really like to see native test results with a USB headphone. > IF somebody has such data, pleas show them to me. Unless I'm mistaken, because I use early requests, pulseaudio will only have 1 more period internally than the buffer length of my sound buffer, so if the buffer is 30 ms, period is 10 ms, and I only fill it with 10 ms data, max latency would be 20 ms at that point. If you say jitter is 8 ms (which I find very high), you wouldn't notice that extra 10 ms.. > > I'm not saying that it's bad that winepulse's GetCurrentPadding and > GetPosition behave like an exclusive mode driver. After all, > winecoreaudio's GCP does the same. Just be aware of the differences. > > > Your capture results look good. >> My favorite capture results (a2dp phone as "microphone"): >> capture.c:247: Sleep.1 position -1 pad 0 flags abadcafe, amount of frames locked: 0 >> I love this one because it shows that when you have a source that's not >> capturing, winepulse still gets the correct results. > This is an area that needs more tests. I've seen native machines > behave similarly, OTOH I've read msdn blog entries mentioning that no > event is delivered when there's nothing to capture. So the expected > behaviour when there's nothing to capture is unclear to me so far. I guessed that it just has a bunch of packets that are re-used, like the way I'm implementing capture in winepulse. See ACPacket, it's stuffed in locked_ptr, empty ones in packet_free_head, and filled in packet_filled_head. > I'd recommend you simply enable (non-event) exclusive mode, that'll > yield twice as many data points per test run. ;-) > > BTW, I recommend that you play & increase the worst case test duration to > last for 5, 30 or 3000 seconds and see if figures are still regular > (e.g. with ALSA->PA, you'd see with 5 seconds that it eats too much data) > and whether it still plays without audible glitches. render.c:2290: Released 441441=1001x441 -441 frames at 44100 worth 10000ms in 9972ms render.c:2290: Released 4410441=10001x441 -441 frames at 44100 worth 100000ms in 99900ms Some interruption of the audio stream, that sleep(i % 10) is suspect... what are you really testing there? And that test seems to be sensitive to not being run in realtime, it seems to work slightly better if I change that to if I set THREAD_PRIORITY_TIME_CRITICAL on the thread, Sleep(0) (for i%10 = 0) looks kind of wrong though Oops and another bugfix[1], pulse_wr_callback can sometimes be called with no update, which can be a contributing factor why sometimes the tests finished early.. with that fixed and no sleep: render.c:2296: Released 4410441=10001x441 -441 frames at 44100 worth 100000ms in 99998ms And with rt prio set, and sleep: render.c:2291: Released 4410441=10001x441 -882 frames at 44100 worth 99990ms in 100014ms I may also be hitting some pulseaudio bug though, if I play some music on the background when one of those tests start the sound during the test works ok, even if I pause it after that. Come to think of it, what is more likely is that on underrun the device is paused since nothing else is playing, and it starts again when being fed again, that would explain why I'm getting interruptions like the +14 ms above.. Also played skyrim for a few hours just fine, it wanted a 20 ms buffer for some reason.. Still not sure why, but who am I to object.. ~Maarten [1] winepulse don't SetEvent on no pad change.. diff --git a/dlls/winepulse.drv/mmdevdrv.c b/dlls/winepulse.drv/mmdevdrv.c index 554a9fc..a4575d5 100644 --- a/dlls/winepulse.drv/mmdevdrv.c +++ b/dlls/winepulse.drv/mmdevdrv.c @@ -531,7 +531,10 @@ static void pulse_wr_callback(pa_stream *s, size_t bytes, void *userdata) else This->pad = 0; - assert(oldpad >= This->pad); + if (oldpad == This->pad) + return; + + assert(oldpad > This->pad); This->clock_written += oldpad - This->pad; TRACE("New pad: %zu (-%zu)\n", This->pad / pa_frame_size(&This->ss), (oldpad - This->pad) / pa_frame_size(&This->ss));

Dec. 3, 2012

Re: [PATCH 4/6] server: Store and return security attributes with extended file attributes (try 5).

by Erich E. Hoover

On Sun, Dec 2, 2012 at 5:28 PM, Anssi Hannula <anssi.hannula(a)iki.fi> wrote: > ... > I don't see the need for an uppercase "WINEACL" in the attribute name, > especially since all the examples in e.g. attr(5) are lowercase. > Moreover, [1] recommends using application specific namespaces in the > attribute name, so IMHO a better attribute name would be "user.wine.acl". > ... I copied the form of "user.DOSATTRIB" that I saw from posts about Samba. If you think it makes more sense to call it "user.wine.acl" then I can certainly do that. Erich

Dec. 3, 2012

Re: [PATCH] dsound: Use event based threads

by Andrew Eikum

On Mon, Dec 03, 2012 at 02:59:07PM +0100, Joerg-Cyril.Hoehle(a)t-systems.com wrote: > BTW, I still believe that mixing and resampling would find their best place > in mmdevapi, not DSound. > Perhaps, but that's provably not what Windows does. Given a format with: dwChannelMask = (CHANNEL_BACK_LEFT | CHANNEL_BACK_RIGHT) and a physical speaker setup containing only: (CHANNEL_FRONT_LEFT | CHANNEL_FRONT_RIGHT) dsound and mmdevapi behave differently. mmdevapi maps the BACK_X channel entirely out of the respective FRONT_X speaker. dsound splits about 2/3 of the matching channel, and 1/3 out of the other FRONT channel, so you hear both signals out of both speakers. I think the best solution is to have dsound call into the system openal to perform multichannel mixing and 3D processing. We can start by getting dsound to have rudimentary support for those features, then plug openal in as the last step. Andrew

Dec. 3, 2012

Re: kernel32: Implement SetFileValidData. (try 2)

by Alexandre Julliard

Hans Leidekker <hans(a)codeweavers.com> writes: > QuickBooks 2008 calls this function. > --- > configure.ac | 1 + > dlls/kernel32/file.c | 12 +++- > dlls/kernel32/tests/file.c | 136 ++++++++++++++++++++++++++++++++++++++++++++ > dlls/ntdll/file.c | 28 +++++++++ > include/ddk/ntddk.h | 5 ++ > include/winbase.h | 1 + > 6 files changed, 181 insertions(+), 2 deletions(-) Still no luck: ../../../tools/runtest -q -P wine -M kernel32.dll -T ../../.. -p kernel32_test.exe.so file.c && touch file.ok file.c:940: Tests skipped: Either no authority to volume, or is todo_wine for C:\users\julliard\Temp\ err=5 should be 3 file.c:940: Tests skipped: Either no authority to volume, or is todo_wine for C:\users\julliard\Temp\removeme\ err=5 should be 3 file.c:940: Tests skipped: Either no authority to volume, or is todo_wine for C:\ err=5 should be 3 file.c:940: Tests skipped: Either no authority to volume, or is todo_wine for \\?\C:\ err=5 should be 3 file.c:3528: Test succeeded inside todo block: SetFileValidData succeeded file.c:3532: Test failed: SetFileValidData failed 50 file.c:3548: Test succeeded inside todo block: SetFileValidData succeeded file.c:3552: Test failed: SetFileValidData failed 50 file.c:3555: Test failed: SetFileValidData failed 50 file.c:3559: Test succeeded inside todo block: SetFileValidData succeeded file.c:3563: Test failed: SetFileValidData failed 50 make: *** [file.ok] Error 7 -- Alexandre Julliard julliard(a)winehq.org

Dec. 3, 2012

winepulse test data review

by Joerg-Cyril.Hoehle＠t-systems.com

Hi, Maarten Lankhorst kindly posted mmdevapi test results for render and capture gathered using his winepulse driver: http://www.winehq.org/pipermail/wine-devel/2012-October/097602.html >render.c:1199: padding 1250 position 510000/21250 slept 470ms iteration 0 I've run your data through some MS-Excel analysis of mine. Here are my observations. >Exclusive mode tests aren't done, I disabled exclusive mode > because I felt tests were insufficient. It's funny that you say this, because your winepulse driver behaves much like an exclusive mode driver, not a shared mode one ;-) More specifically, a) winepulse and exclusive mode do not decrease padding in multiples of the period size. b) winepulse and exclusive mode appear to equate GetPosition with released frames minus GetCurrentPadding exactly like ALSA hw:0 and dmix do: delay + avail_update = buffer_size (~= 8192 frames) IOW, the driver pretends that the speaker position is exactly the frame that just fell off the circular buffer. This may be ok for ALSA hw:0 and MS-Windows exclusive mode drivers, but that's wrong for anything with a filter chain in the back. Hence I assume that winepulse's GetPosition is lying. The question is: does it matter? If using PA API calls of some sort, you manage to keep the PA-internal latency well below what's the human brain can perceive, I'd say that's ok. Likewise, hw:0 ignores Digital->Analog and wire->LC->speaker times. Our gripe with PA is precisely that using the regular ALSA->PA path via the ALSA plugin, nobody found a means to keep PA's latency small. (I'll repeat myself saying that what actually matters is the sum of all latencies and buffers: mmdevapi buffer, PA transformation chain, DAC etc.) The caveat is that with increasing latency, the behaviour of GCP and GetPosition will differ more and more from native's mixer, until it becomes too much for application X to bear, causing a bug report. That was about the relationship between GCP and GetPosition. Now what about GetPosition and wall time (as seen by the HighPerformanceTimer)? The largest delta is 8ms, which suggests that your GetPosition is not particularly regular, but as it stays below one 10ms period, it's presumably good enough. By contrast, native's largest delta is < 1ms. >Sometimes up to 2 tests fail due to position updates not being regular, Indeed, the data reveals it. I guess the reported GetPosition is more related to packet size job completion than actual sample accurate stream position. >I suspect this is because it's nearly done playing though, but it can get worse >when you use usb headphones, not much I can do about that though, except if I >started lying, What do you mean with "started lying"? I inferred above that it's already lying. > but it's so far been much simpler when I don't.. The data points look very reasonable -- if audible latency is really low, which the tests cannot reveal. BTW, I'd really like to see native test results with a USB headphone. IF somebody has such data, pleas show them to me. I'm not saying that it's bad that winepulse's GetCurrentPadding and GetPosition behave like an exclusive mode driver. After all, winecoreaudio's GCP does the same. Just be aware of the differences. Your capture results look good. >My favorite capture results (a2dp phone as "microphone"): >capture.c:247: Sleep.1 position -1 pad 0 flags abadcafe, amount of frames locked: 0 >I love this one because it shows that when you have a source that's not >capturing, winepulse still gets the correct results. This is an area that needs more tests. I've seen native machines behave similarly, OTOH I've read msdn blog entries mentioning that no event is delivered when there's nothing to capture. So the expected behaviour when there's nothing to capture is unclear to me so far. I'd recommend you simply enable (non-event) exclusive mode, that'll yield twice as many data points per test run. ;-) BTW, I recommend that you play & increase the worst case test duration to last for 5, 30 or 3000 seconds and see if figures are still regular (e.g. with ALSA->PA, you'd see with 5 seconds that it eats too much data) and whether it still plays without audible glitches. Regards, Jörg Höhle

Dec. 3, 2012

[PATCH] dsound: Use event based threads

by Joerg-Cyril.Hoehle＠t-systems.com

Hi, Maarten Lankhorst queried: >Bump, anything wrong with this patch? Here's my 0.0$ ... (standard DSound disclaimer here...) Using mmdevapi's events in DSound is basically TRT. Then, the DSound mixer will be synchronized with mmdevapi writes. >+ /* ALSA is retarded, add a timeout.. */ >+ ret = WaitForSingleObject(dev->sleepev, dev->sleeptime); How does that comment help the reader of the code? >+ if (period_ms <= 15) >+ device->sleeptime = period_ms * 5 / 2; >+ else >+ device->sleeptime = period_ms * 3 / 2; I expect such threshold functions to be continuous or at least monotonic. A sawtooth is certainly unexpected. What is its purpose? What I would understand is a comment in the code saying that as of 2012, all 3 wine*.drv mmdevapi drivers have a bug that they stop sending events after IAudioClient_Stop. Therefore add a short timeout to the wait. If that's the reason for the timeout, then period_ms * 1.5 is perfectly fine. There's no reason for a particular threshold. Note that neither 1.5 nor 2.5 give you regular spacing around the time of transition from playing to stopped. + IAudioClient_GetStreamLatency(device->client, &period); + period_ms = (period + 9999) / 10000; If IAC_Stop is the reason for the sleep time out, then it's obvious that GetStreamLatency has no business here, rather than the device period. I could understand a use of GetStreamLatency when it comes to computing a reasonable size of the primary buffer. + ret = WaitForSingleObject(dev->sleepev, dev->sleeptime); + ... if (ret) WaitFor* return values are defined in terms of WAIT_OBJECT_0, WAIT_TIMEOUT and WAIT_FAILED etc., not zero or non-zero. >+ device->sleepev = CreateEventW(0, 0, 0, 0); >+ device->thread = CreateThread(0, 0, DSOUND_mixthread, device, 0, 0); I haven't checked the overall style of DSound, but I prefer FALSE and TRUE be used for things advertised as BOOL. (I tend to write NULL for null pointer initialisation. Note that I easily read "if (foo) ..." for boolean and pointer types; I don't expect "if (foo == TRUE) or (foo != NULL) ...") BTW, I still believe that mixing and resampling would find their best place in mmdevapi, not DSound. Regards, Jörg Höhle

Dec. 3, 2012

Re: [PATCH 3/3] shell32/tests: Add tests for DoEnvironmentSubstA/W

by Alexandre Julliard

Detlef Riekenberg <wine.dev(a)web.de> writes: > + /* Not all variables are supported before vista */ > + if (broken(*bufferA == '%')) > + trace("%d: %s not supported\n", i, bufferA); > + else > + { > + if (i < 3) todo_wine > + ok(HIWORD(res) && (LOWORD(res) == (lstrlenA(bufferA)+1)) && (*bufferA != '%'), > + "%d: got %d/%d and %s (len %d)\n", > + i, HIWORD(res), LOWORD(res), bufferA, lstrlenA(bufferA)); > + > + if (i < 3) todo_wine > + ok(HIWORD(res2) && (LOWORD(res2) == (lstrlenW(bufferW)+1)) && (*bufferW != '%'), > + "%d: got %d/%d and %s (len %d)\n", > + i, HIWORD(res2), LOWORD(res2), wine_dbgstr_w(bufferW), lstrlenW(bufferW)); > + } > + } This doesn't make sense, you are only testing the cases that don't work. You should pick a few variables that work everywhere, and test them properly, including the resulting buffer contents. -- Alexandre Julliard julliard(a)winehq.org

Dec. 3, 2012

Re: wshom: Basic support for REG_SZ values in RegRead()

by Jacek Caban

Hi Nikolay, On 12/03/12 12:29, Nikolay Sivov wrote: > + /* skip '\' after root name */ > + if (name[len] == '\\') len++; It looks like if name[len] != '\\', then name should be considered invalid. > + > + val = strlenW(name); > + while (name[--val] != '\\' && val) > + ; > + keyname = SysAllocStringLen(&name[len], val-len); This won't work if name has no backslashes. > + /* skip leading '\' */ > + val++; > + > + type = REG_NONE; > + datalen = 0; > + ret = RegGetValueW(root, keyname, &name[val], RRF_RT_ANY, &type, NULL, &datalen); > + if (ret == ERROR_SUCCESS) > + { > + if (type == REG_SZ) > + { > + V_VT(value) = VT_BSTR; > + V_BSTR(value) = SysAllocStringLen(NULL, datalen-1); > + ret = RegGetValueW(root, keyname, &name[val], RRF_RT_ANY, &type, V_BSTR(value), &datalen); > + } > + else > + FIXME("type %d not handled\n", type); This should return an error. Jacek

Dec. 3, 2012