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

Re: [PATCH 2/5] d3dx9_36: Implement ID3DXFileImpl_RegisterTemplates.
by Rico Schüller 22 Oct '12

22 Oct '12

On 22.10.2012 21:28, Christian Costa wrote: > } ID3DXFileImpl; > > > - > static inline ID3DXFileImpl *impl_from_ID3DXFile(ID3DXFile *iface) It doesn't make much sense to add an empty line and remove it in the next patch.

1 0

Re: [PATCH 5/5] d3dx9_36: Implement ID3DXFileDataImpl_GetType.
by Rico Schüller 22 Oct '12

22 Oct '12

On 22.10.2012 21:28, Christian Costa wrote: > static HRESULT WINAPI ID3DXFileDataImpl_Unlock(ID3DXFileData *iface) > { > + > FIXME("(%p)->(): stub\n", iface); Why?

1 0

Re: [PATCH 1/5] d3dx9_36: Implement D3DXFileCreate.
by Rico Schüller 22 Oct '12

22 Oct '12

On 22.10.2012 21:27, Christian Costa wrote: > +static HRESULT WINAPI ID3DXFileImpl_QueryInterface(ID3DXFile *iface, REFIID riid, LPVOID *ret_iface) > > + ID3DXFileImpl *This = impl_from_ID3DXFile(iface); > > + ID3DXFileImpl* object; Please be a bit more consistent across your patch... There are a lot more occurrences where you mix both usages.

1 0

Re: [PATCH 6/8] ntdll: Standardize on sysconf to detect the number of processors for all platforms.
by Alexandre Julliard 22 Oct '12

22 Oct '12

James Eder <jimportal(a)gmail.com> writes: > @@ -1010,6 +1010,16 @@ static inline void get_cpuinfo(SYSTEM_CPU_INFORMATION* info) > */ > void fill_cpu_info(void) > { > + long num; > + > + num = sysconf(_SC_NPROCESSORS_ONLN); > + if (num < 1) > + { > + num = 1; > + WARN("Failed to detect the number of processors.\n"); > + } > + NtCurrentTeb()->Peb->NumberOfProcessors = num; Have you verified that this works correctly on all the platforms you are changing? -- Alexandre Julliard julliard(a)winehq.org

2 1

Re: [PATCH] user32: Fix error handling in MapWindowPoints, ClientToScreen and ScreenToClient and add tests for them. (try 2)
by Alexandre Julliard 22 Oct '12

22 Oct '12

Christian Costa <titan.costa(a)gmail.com> writes: > @@ -236,20 +236,32 @@ BOOL WINAPI GetClientRect( HWND hwnd, LPRECT rect ) > > > /******************************************************************* > - * ClientToScreen (USER32.@) > + * ClientToScreen (USER32.@) > */ > BOOL WINAPI ClientToScreen( HWND hwnd, LPPOINT lppnt ) > { > + if (!IsWindow( hwnd )) > + { > + SetLastError( ERROR_INVALID_WINDOW_HANDLE ); > + return FALSE; > + } > + You should never use IsWindow(). Handles should be checked at the point where they are used. -- Alexandre Julliard julliard(a)winehq.org

2 1

[PATCH] try2 wineboot: detect correct processor name and vendorID, use Intel Pentium 4 as fallback
by Patrick Rudolph 22 Oct '12

22 Oct '12

This patch super-seeds my previous attempt ID 91173. This time I used do_cpuid from ntdll / wbemprox. If cpuid is not available, the default "Intel Pentium 4" string is used. I modified get_processor_name to remove leading white-spaces. I also updated the code for Identifier REG_SZ. This applies to 64bit wine only, as 32bit still reports x86.

1 0

Re: [PATCH 5/6] dsound: rework ugly mixer logic
by Andrew Eikum 22 Oct '12

22 Oct '12

Mostly good cleanup in this one. Some thoughts below... On Tue, Oct 16, 2012 at 02:06:29PM +0200, Maarten Lankhorst wrote: > diff --git a/dlls/dsound/dsound_private.h b/dlls/dsound/dsound_private.h > index feef787..7817b88 100644 > --- a/dlls/dsound/dsound_private.h > +++ b/dlls/dsound/dsound_private.h > @@ -73,10 +73,8 @@ struct DirectSoundDevice > - DWORD writelead, buflen, state, playpos, mixpos; > + DWORD writelead, buflen, aclen, fraglen, state, playpos, pad; If you're introducing new variables, surely you can come up with something more descriptive than "aclen." Something like "ac_buffer_frames," maybe? I'm a big fan of units on my variable names so mistakes like "pos_bytes = ac_frames;" are obvious. Similar for "pad," maybe something like "in_mmdev_bytes" (Is that actually used any more after your patches? You could re-use it if not.) > @@ -209,7 +207,6 @@ HRESULT DSOUND_PrimaryCreate(DirectSoundDevice *device) DECLSPEC_HIDDEN; > HRESULT DSOUND_PrimaryDestroy(DirectSoundDevice *device) DECLSPEC_HIDDEN; > HRESULT DSOUND_PrimaryPlay(DirectSoundDevice *device) DECLSPEC_HIDDEN; > HRESULT DSOUND_PrimaryStop(DirectSoundDevice *device) DECLSPEC_HIDDEN; > -HRESULT DSOUND_PrimaryGetPosition(DirectSoundDevice *device, LPDWORD playpos, LPDWORD writepos) DECLSPEC_HIDDEN; Good riddance. > @@ -682,147 +623,100 @@ static void DSOUND_PerformMix(DirectSoundDevice *device) > LeaveCriticalSection(&device->mixlock); > return; > } > - > - to_mix_frags = device->prebuf - (pad * device->pwfx->nBlockAlign + device->fraglen - 1) / device->fraglen; > - > - to_mix_bytes = to_mix_frags * device->fraglen; > - > - if(device->in_mmdev_bytes > 0){ > - DWORD delta_bytes = min(to_mix_bytes, device->in_mmdev_bytes); > - device->in_mmdev_bytes -= delta_bytes; > - device->playing_offs_bytes += delta_bytes; > - device->playing_offs_bytes %= device->buflen; > + block = device->pwfx->nBlockAlign; Bleh. Do we really need to alias that? > + if (maxq > device->fraglen * 3) > + maxq = device->fraglen * 3; > + This seems to be replacing ds_snd_queue_max, right? Why remove the configurability? Why 3 instead of the old default of 10? This should be a separate patch at least. > + writepos = (device->playpos + pad) % device->buflen; > > if (device->priolevel != DSSCL_WRITEPRIMARY) { > - BOOL recover = FALSE, all_stopped = FALSE; > - DWORD playpos, writepos, writelead, maxq, prebuff_max, prebuff_left, size1, size2; > - LPVOID buf1, buf2; > + BOOL all_stopped = FALSE; > int nfiller; > + BOOL native = device->normfunction == normfunctions[4]; > + DWORD bpp = device->pwfx->wBitsPerSample>>3; Again, do we need to alias that? > -static DWORD DSOUND_fraglen(DirectSoundDevice *device) > -{ > - REFERENCE_TIME period; > - HRESULT hr; > - DWORD ret; > - > - hr = IAudioClient_GetDevicePeriod(device->client, &period, NULL); > - if(FAILED(hr)){ > - /* just guess at 10ms */ > - WARN("GetDevicePeriod failed: %08x\n", hr); > - ret = MulDiv(device->pwfx->nBlockAlign, device->pwfx->nSamplesPerSec, 100); > - }else > - ret = MulDiv(device->pwfx->nSamplesPerSec * device->pwfx->nBlockAlign, period, 10000000); > - > - ret -= ret % device->pwfx->nBlockAlign; > - return ret; > -} > - ... > + device->fraglen = MulDiv(device->pwfx->nSamplesPerSec, period, 10000000) * device->pwfx->nBlockAlign; This should be a separate patch. I don't have an argument /against/ it, but why do you prefer 10ms over whatever the driver prefers? Andrew

3 3

Re: [PATCH 4/6] dsound: Use event based threads
by Andrew Eikum 22 Oct '12

22 Oct '12

I like it. Before I can give a sign-off, I need to run it through my usual battery of tests on all the platforms. I'll do that after patch 3 gets figured out. Some thoughts below... On Tue, Oct 16, 2012 at 02:06:28PM +0200, Maarten Lankhorst wrote: > + /* ALSA is retarded, add a timeout.. */ > + ret = WaitForSingleObject(dev->sleepev, dev->sleeptime); No arguments here :) but I'm curious what you mean. Presumably winealsa isn't sending events when it ought to? I know Jorg has discovered that failure, I wonder if it's worth testing and fixing across all the drivers. > diff --git a/dlls/dsound/primary.c b/dlls/dsound/primary.c > index 94bdf9c..9d9fa27 100644 > --- a/dlls/dsound/primary.c > +++ b/dlls/dsound/primary.c > @@ -170,6 +172,7 @@ HRESULT DSOUND_ReopenDevice(DirectSoundDevice *device, BOOL forcewave) > device->pwfx->wFormatTag = WAVE_FORMAT_PCM; > device->pwfx->cbSize = 0; > } > + IAudioClient_SetEventHandle(device->client, device->sleepev); > > hres = IAudioClient_GetService(device->client, &IID_IAudioRenderClient, > (void**)&device->render); > @@ -203,6 +206,19 @@ HRESULT DSOUND_ReopenDevice(DirectSoundDevice *device, BOOL forcewave) > WARN("GetService failed: %08x\n", hres); > return hres; > } > + /* Now kick off the timer so the event fires periodically */ > + IAudioClient_Start(device->client); > + > + IAudioClient_GetStreamLatency(device->client, &period); I'm a big fan of error checking (or at least reporting), so these unchecked calls make me nervous. They'll probably never fail, but if they do, I think they should fail loudly. Andrew

2 2

Re: [PATCH 5/5] services: Added tests for service thread termination without setting service state to stopped
by Marvin 22 Oct '12

22 Oct '12

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=22392 Your paranoid android. === WVISTAADM (32 bit service) === service.c:397: Test failed: status.dwCurrentState = 3 service.c:411: Test failed: status.dwCurrentState = 3 === W2K8SE (32 bit service) === service.c:397: Test failed: status.dwCurrentState = 3 service.c:411: Test failed: status.dwCurrentState = 3

1 0

ntdll feature detection patch set
by James Eder 22 Oct '12

22 Oct '12

I've been kicking this patch set around in my local git tree for a while and I want to get it out there for comments and maybe even inclusion in Wine. Seeing some comments in another thread about wineboot made me think I should get these patches out rather than holding onto them much longer. I feel fairly good about the first 6 patches. They're small, bite-sized, and all x86/x86_64 so I can test them easily. However, the last two in the series are some what large and do things with ARM and PowerPC code. While it looks good to me, I don't have hardware or a build environment for them. I may have some ARM+Linux hardware in the coming months but I doubt I'll ever see a PowerPC come my way. If you have a build environment for ARM or PowerPC I would very much appreciate some feedback. Also I did some greping and found two other locations in Wine with similar do_cpuid type functions: dlls/wbemprox/builtin.c and dlls/dbghelp/minidump.c. Would it be worthwhile to have the function in a common header somewhere? -- Jim

2 1