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

Re: [PATCH 1/5] d3dx9_36: Implement D3DXFileCreate. (try 3)

by Dmitry Timoshkov

Christian Costa <titan.costa(a)gmail.com> wrote: > > > +static HRESULT WINAPI ID3DXFileImpl_QueryInterface(ID3DXFile *iface, > > REFIID riid, void **ret_iface) > > > +{ > > > + TRACE("(%p)->(%s, %p)\n", iface, debugstr_guid(riid), ret_iface); > > > + > > > + if (IsEqualGUID(riid, &IID_IUnknown) || > > > + IsEqualGUID(riid, &IID_ID3DXFile)) > > > + { > > > + iface->lpVtbl->AddRef(iface); > > > > Isn't there an appropriate xxx_AddRef() macro? > > > > No. Is there a reason why? > > > + *ret_iface = iface; > > > + return S_OK; > > > + } > > > + > > > + ERR("(%p)->(%s, %p), not found\n", iface, debugstr_guid(riid), > > ret_iface); > > > > FIXME seems more appropriate here. > > > > No. All interfaces are implemented here. Then ERR() is even more inappropriate here. > > > +HRESULT WINAPI D3DXFileCreate(ID3DXFile **dxfile) > > > +{ > > > + ID3DXFileImpl *object; > > > + > > > + TRACE("(%p)\n", dxfile); > > > + > > > + object = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, > > sizeof(*object)); > > > > What's the reason for HEAP_ZERO_MEMORY here? > > > > It's common to do it this way. Only non zero value are set on creation. > Usually several members are set in methods. There is no memebers left at 0 here. > > > + if (!object) > > > + { > > > + ERR("Out of memory\n"); > > > + return E_OUTOFMEMORY; > > > + } > > > > The ERR() is useless here, just return E_OUTOFMEMORY in such situations. > > > > > > > It's done this way in many places in wine. Then that other places need to be fixed as well. Printing an ERR() on memory allocation errors is not helpful. -- Dmitry.

Oct. 24, 2012

Re: vcomp: single-threaded implementation of _vcomp_fork (try 6)

by Dan Kegel

On Wed, Oct 24, 2012 at 5:38 AM, Alexandre Julliard <julliard(a)winehq.org> wrote: > What I'm actually saying is that you should stop playing games with the > compiler and implement this in assembler... Thanks for the cleartext.

Oct. 24, 2012

Re: vcomp: single-threaded implementation of _vcomp_fork (try 6)

by Alexandre Julliard

Dan Kegel <dank(a)kegel.com> writes: > I don't think he's saying that. > It suffices that taking the address of the fourth parameter saves it > to the stack. > We are lucky in this case that there are already three > non-varargs parameter, so after taking the address of the fourth > parameter, all the varargs params are in memory. > It all just happens to work (at least on ia32 and x64). What I'm actually saying is that you should stop playing games with the compiler and implement this in assembler, it's really not hard. -- Alexandre Julliard julliard(a)winehq.org

Oct. 24, 2012

Re: [PATCH 5/6] dsound: rework ugly mixer logic

by Michael Stefaniuc

Hello Jörg, On 10/24/2012 12:36 PM, Joerg-Cyril.Hoehle(a)t-systems.com wrote: > I'm not happy with Wine's DSound+WinMM->mmdevapi. That may be a good > move for MS, but it's a bad move for Wine. Wine's audio drivers don't what would you do instead? Go back to the old way and have dsound and winmm drivers too? That road leads to madness as it will triple the amount of sound drivers in Wine. > bang the HW, they have huge SW stacks like PA or Jack in their back. bye michael

Oct. 24, 2012

Re: vcomp: single-threaded implementation of _vcomp_fork (try 6)

by Dan Kegel

On Wed, Oct 24, 2012 at 3:26 AM, <Joerg-Cyril.Hoehle(a)t-systems.com> wrote: > Alexandre Julliard wrote: >> +void WINAPIV _vcomp_fork(BOOL ifval, int nargs, void *wrapper, ...) >> + __ms_va_list valist; >> >>it's ugly to use varargs only as a hack to get a pointer to the first >>argument. > > Would taking the address of &wrapper (as in asm("...","r=&wrapper") or > any C code like "void* stack = &wrapper") suffice to tell the x64 > compiler to save register parameters into their allocated stack > locations? > > My belief was that this is specifically the magic performed when the > compiler sees a [__ms_]va_list. > > You seem to imply that va_list is superfluous and that > taking the address of any parameter (not necessarily &wrapper > but &ifval or &nargs too) would achieve the same effect: all 4 registers > rcx, rdx, r8 and r9 get saved to the stack such that the & abstraction > works even when the user performs pointer arithmetic. I don't think he's saying that. It suffices that taking the address of the fourth parameter saves it to the stack. We are lucky in this case that there are already three non-varargs parameter, so after taking the address of the fourth parameter, all the varargs params are in memory. It all just happens to work (at least on ia32 and x64). > In such a case he would use &wrapper, bypass va_list and correct the > stack-relative offsets in the x86 and x64 assembly code by one pointer unit. I don't think it's that complicated. Here's a draft that does what Alexandre suggests. It seems fine to me and to Daniel Lehman, and I was about to submit it when I got Jörg's reply. - Dan

Oct. 24, 2012

Re: [PATCH 5/6] dsound: rework ugly mixer logic

by Maarten Lankhorst

Op 24-10-12 12:52, Joerg-Cyril.Hoehle(a)t-systems.com schreef: > Hi, > > Maarten Lankhorst wrote: >> For example Skyrim with a 36 ms stream latency will just buffer in more data to >> compensate instead. But it can't do it if you report that it's fine to feed data every 5 ms. > Please elaborate. Every now and then I wish you used more words to > explain your observations. > > Where do the 5ms come from? What's the relationship with the 36ms latency? Well skyrim is pretty aggressive with its low latency. It seems to want to buffer 20 ms only when default period is 10 ms, but when I did a bunch of tests with tons of underruns my minimum and default period ended up at 36 ms, skyrim still ran correctly, and looking at 'pacmd list-sink-inputs' it just increased buffer length in that case. > You patch DSound so I assume that Skyrim uses DSound, not mmdevapi > directly. Shouldn't DSound export capabilities that make sense given > its abstraction? IIRC, there's some former mailto wine-devel or bug > report where I talked about the alternating period-sized buffer > abstraction that is underlying DSound's API and how all positions > being reported modulo buffer size are problematic if someone misses > the period intervals. Nah, originally when I started hacking on dsound I assumed skyrim was using dsound, but it's only used for the intro. I'm running the Skyrim steam version with windows set to win7. I'm not sure yet how dsound should be fixed on bigger periods, I believe using IAudioClock for reporting position *might* be the correct answer, but it's not properly test yet on behavior, so I chose for the simple solution of directly reporting next mix position as play pos, which is the same as current code. > Of course, that abstraction cannot explain a 36ms latency with two > alternating period-sized "direct hardware" buffers of 5ms. Indeed, skyrim was creating the 20 ms buffer directly, and allocating a bigger buffer didn't help. So instead when an application requests 2 default periods I try to set latency to half requested period (5 ms instead of 10ms) and I'm more aggressive about firing events when data can be written. Pulseaudio seems to behave differently at lower latencies though, pointer updates are no longer done at a fixed size, presumably because it puts pulseaudio in a real danger of underruns so it mixes as much as it can each pass. > Wine's DSound must maintain a set of mutually consistent variables > { play position, write position, buffer size, period size } > that in addition need to make sense w.r.t. audio output. > I'm not convinced that it does. There should be tests to verify that. run the dsound tests with WINETEST_INTERACTIVE=1. > In particular, wine's DSound must not let play position leave the virtual DSound > buffer, even if mmdevapi reports that it's 2 seconds behind with PulseAudio. > (IMHO, should that happen, then DSound must cap the reported position). At the point dsound queues the data to mmdevapi it can no longer do anything about it, so instead of trying something more complicated the best way to deal with it is to accept that fact and keep queued latency as fixed as possible, and ideally small. I create a 80 ms buffer, please read msdn's documentation of iaudioclient::initialize, the requirements for shared mode is that it will create a buffer that's at least that large to prevent underruns, so if a 2 second buffer is created as a result, then somehow the audio system believes we should buffer 2 seconds. Not much we can do about that, and according to docs looks like we should keep the buffer as filled as possible in that case. However I believe that this is unlikely happen, and more likely we end up with only having to buffer a fixed 80 ms. 80 ms is generous btw, if you're feeling lucky you can set dsound latency much lower, 40 ms buffering would probably be the lowest safe value, but if you're feeling adventerous initialize buffer length to 100 ns and get the driver to come up with the minimal period and buffer length it supports. But things do seem to break down when you go that low. Pulseaudio got angry at me for those underruns and increased minimum latency to 8 ms after I did some testing at sub 1 ms buffers. :-( > (BTW, I'd find it easier for Wine if DSound's primary were equal to mmdevapi's > buffer, except that with winecoreaudio, there's no such mmdevapi buffer...) I've been doing some cleanups and there's really no need to do this. I handled the primary buffer mixing as a special case of secondary buffer and it works just fine. The idea behind my patches has been to simply remove primary buffer altogether, as far as possible. device->buffer is equal to primary buffer size in writeprimary mode, before being copied to audio client. It's used for mixing to float if in secondary mode when iaudioclient uses a non-float format, and it's NULL for float format. In case of float it gets mixed to the audio buffer directly, saving a memcpy and a conversion. ~Maarten

Oct. 24, 2012

Re: [PATCH] user32: Fix error handling in MapWindowPoints, ClientToScreen and ScreenToClient and add tests for them. (try 3)

by Christian Costa

2012/10/24 Nikolay Sivov <bunglehead(a)gmail.com> > On 10/24/2012 11:33, Christian Costa wrote: > > > > 2012/10/24 Dmitry Timoshkov <dmitry(a)baikal.ru> > >> Christian Costa <titan.costa(a)gmail.com> wrote: >> >> > BOOL WINAPI ClientToScreen( HWND hwnd, LPPOINT lppnt ) >> > { >> > + DWORD error = GetLastError(); >> > + >> > + if (!hwnd) >> > + { >> > + SetLastError( ERROR_INVALID_WINDOW_HANDLE ); >> > + return FALSE; >> > + } >> > + >> > + SetLastError( 0xdeadbeef ); >> > MapWindowPoints( hwnd, 0, lppnt, 1 ); >> > + >> > + if (GetLastError() != 0xdeadbeef) >> > + return FALSE; >> > + >> > + SetLastError(error); >> > return TRUE; >> > } >> >> As been said before these games with saving/restoring last error value >> are broken. >> >> >> Last time you said wrong so what do you mean by wrong or broken? > The only way to know if MapWindowPoints fails is to set last error first > and check it > after. If I don't restore the previous value, the tests will not pass. Of > course I can > arrange the tests but hey. And I can check windows handle here as > Alexandre said. > > If error reporting from MapWindowPoints is not sufficient to use it > somewhere else you can always move its guts to > a separate function and report failure as you want. By the way, it only > fails on invalid window handles I guess, is that right? > If you need this behaviour you have a failing WIN_GetPtr() in this case. > Messing with last error is not a solution obviously, > and I don't see where MapWindowPoints even sets it. > > > Sounds good. I'll do that. Basically WINPOS_GetWinOffset get most part the job in our case and I already modified it to return an error. Yes it fails only for invalid handles. WIN_GetPtr is not enough because it just return WND_OTHER_PROCESS to call the server to ask if it exists on another process. It's what IsWindow does (and WINPOS_GetWinOffset). MapWindowPoints can fail (see test). MSDN says to call SetLastError first before calling it and read the value. I will send a patch using WINPOS_GetWinOffset. Thanks.

Oct. 24, 2012

[PATCH 5/6] dsound: rework ugly mixer logic

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

Hi, Maarten Lankhorst wrote: >For example Skyrim with a 36 ms stream latency will just buffer in more data to >compensate instead. But it can't do it if you report that it's fine to feed data every 5 ms. Please elaborate. Every now and then I wish you used more words to explain your observations. Where do the 5ms come from? What's the relationship with the 36ms latency? You patch DSound so I assume that Skyrim uses DSound, not mmdevapi directly. Shouldn't DSound export capabilities that make sense given its abstraction? IIRC, there's some former mailto wine-devel or bug report where I talked about the alternating period-sized buffer abstraction that is underlying DSound's API and how all positions being reported modulo buffer size are problematic if someone misses the period intervals. Of course, that abstraction cannot explain a 36ms latency with two alternating period-sized "direct hardware" buffers of 5ms. Wine's DSound must maintain a set of mutually consistent variables { play position, write position, buffer size, period size } that in addition need to make sense w.r.t. audio output. I'm not convinced that it does. There should be tests to verify that. In particular, wine's DSound must not let play position leave the virtual DSound buffer, even if mmdevapi reports that it's 2 seconds behind with PulseAudio. (IMHO, should that happen, then DSound must cap the reported position). (BTW, I'd find it easier for Wine if DSound's primary were equal to mmdevapi's buffer, except that with winecoreaudio, there's no such mmdevapi buffer...) Regards, Jörg Höhle

Oct. 24, 2012

Re: [PATCH 1/2] user32: Remove redundant sizeof check.

by Michael Stefaniuc

On 10/24/2012 11:04 AM, Ken Thomases wrote: > On Oct 24, 2012, at 3:24 AM, Michael Stefaniuc wrote: > >> @@ -1330,9 +1330,6 @@ static HICON CURSORICON_LoadFromFile( LPCWSTR filename, >> } >> >> dir = (const CURSORICONFILEDIR*) bits; >> - if ( filesize < sizeof(*dir) ) >> - goto end; >> - >> if ( filesize < (sizeof(*dir) + sizeof(dir->idEntries[0])*(dir->idCount-1)) ) >> goto end; >> > > That doesn't seem redundant to me. It's not safe to access dir->idCount if the file isn't known to be big enough. Actually it is safe. The file is mapped in page size chunks. So accessing dir->idCount is safe aka no exception will be generated. And filesize will be smaller for any random WORD that ends up being in dir->idCount. bye michael

Oct. 24, 2012

[PATCH 5/6] dsound: rework ugly mixer logic

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

Hi, Maarten Lankhorst wrote: >only oss4 and (maybe, just guessing?) coreaudio don't support float MacOS' CoreAudio supports floats. Actually, given that it's an entire audio pipeline like quartz, I bet it even prefers floats. >I also refer you to GetStreamLatency where I was getting the period from, >Please take it from me: IT'S OK TO FAIL THOSE MMDEVAPI PERIOD TESTS, I agree that some tests are extremely strict and that a "good enough" driver could work extremely well in practice despite failing some tests. However, please specify exactly which tests you have in mind so that we all talk about the same set. >BUT PLEASE DON'T LIE ABOUT THE HARDWARE CAPABILITIES REGARDING >TO MINIMUM PERIOD, DEFAULT PERIOD AND STREAM LATENCY. The reasoning is as follows: First, tests show that native exhibits a 10ms period, hence we can assume that every PC SW has been tested with that period only. Using another period in Wine might reveal bugs in the apps, but users would blame Wine. This is why I've been arguing no later than last week in bug #28622, comment #58 that Wine should fully decouple ALSA/OSS periods from the published 10ms mmdevapi periods. OTOH, winecoreaudio has been using 20ms periods since day 1 in 2011. I'm not aware of bugs in bugzilla caused by that deviation. Second, the shared mode audio engine IMHO presents an abstraction over any particular HW, providing the same behviour on all machines and eliminating the need for app X to talk to or work around bugs in sound driver Y. Thus I wonder why you talk about hardware capabilities at all. IOW the shared mode caps are: fixed period=10ms and unlimited buffer (MSDN says 2s). a) Minimum Period Why do you care? It's only relevant in exclusive mode. I'd say that minimum = shared mode default period is a reasonable value. And if default=10ms, minimum = 5ms seems ok if it has been tested and works well. OTOH, 200 periods and wakeups per second may be unrealistic as we alreadty struggle to sustain 100. Do you know an app that uses mmdevapi in exclusive mode? b) Default Period == shared mode audio engin period The target is 10ms, see above. Yet we could make exemptions: for instance, we might observe that all apps using mmdevapi use it via the XAudio2 libraries and that XA2 is well debugged and would bear a larger period, e.g. 50ms. There should be a registry setting for users to play with. I'm not happy with Wine's DSound+WinMM->mmdevapi. That may be a good move for MS, but it's a bad move for Wine. Wine's audio drivers don't bang the HW, they have huge SW stacks like PA or Jack in their back. c) Stream Latency Bear in mind that the mmdevapi stream latency is a static property of an audio SW transformation graph. It does not vary if some buffer is full or empty. It is unlike snd_pcm_delay. I believe winealsa outputs a reasonable stream latency. One may argue that it is too low: as winealsa always buffers 3 periods internally the latency should count those. I kept it low because native's mmdevapi is reported to introduce 30-40ms delays yet its GetStreamLatency ignores that and reports values like 10.6666ms. Perhaps we are wrong about GetStreamLatency. Perhaps it's always a local call to one element of the audio transformation graph and should never sum other elements itself. Perhaps the app should use some enumeration facility to walk the audio graph and sum each element's latency? Perhaps there's some native graph element that would report said 30-40ms latency? Regards, Jörg Höhle

Oct. 24, 2012