September 2012 - Wine-Devel

Re: [PATCH] msi: Use next cabinet from the media table instead of failing when there is a mismatch with continuous cabinet.

by Hans Leidekker

On Thu, 2012-09-27 at 09:59 +0200, Christian Costa wrote: > diff --git a/dlls/msi/media.c b/dlls/msi/media.c > index 612624d..8a39fcd 100644 > --- a/dlls/msi/media.c > +++ b/dlls/msi/media.c > @@ -358,8 +358,18 @@ static INT_PTR cabinet_next_cabinet(FDINOTIFICATIONTYPE fdint, > > if (strcmpiW( mi->cabinet, cab )) > { > - ERR("Continuous cabinet does not match the next cabinet in the Media table\n"); > - goto done; > + LPSTR next_cab; > + > + WARN("Continuous cabinet %s does not match the next cabinet %s in the media table\n", debugstr_w(cab), debugstr_w(mi->cabinet)); > + > + /* Use cabinet name from the media table */ > + next_cab = strdupWtoA(mi->cabinet); > + /* Modify path to cabinet file with full filename (psz3 points to a 256 bytes buffer that can be modified contrary to psz1 and psz2) */ > + strcat(pfdin->psz3, "\\"); > + strcat(pfdin->psz3, next_cab); > + /* Path psz3 and cabinet psz1 are concatenated by FDI so just reset psz1 */ > + *pfdin->psz1 = 0; > + msi_free(next_cab); > } > > if (!(cabinet_file = get_cabinet_filename(mi))) The requirement that the continuous cabinet name matches the next cabinet in the media table might simply be too strict. Perhaps we need to try them one by one and let FDI decide based on the setID and iCabinet fields. Can you please add some tests?

Sept. 27, 2012

Re: dsound: Don't bother shrinking the secondary buffer list.

by Michael Stefaniuc

Hello Jörg, as always it depends. On 09/27/2012 09:37 AM, Joerg-Cyril.Hoehle(a)t-systems.com wrote: >> +#include <assert.h> > > your patch is already committed, yet I believe that the least that dsound needs is new asserts. > > Asserts in dsound have been a PITA in Wine for the last decade. > > It's ok for the kernel to crash in an assert. IMHO it is not ok for an optional thing like sound. > > The assert's backtrace will not help anybody, because the logical error will have happened much earlier. I disagree. It helps GyB to open regression bug reports and it helps me to fix them. > Instead, a plain ERR would have been just as telling. Let's quote > http://www.winehq.org/docs/winedev-guide/debugging#DBG-CLASSES > "ERR: Messages in this class indicate serious errors in Wine, > such as as conditions that should never happen by design." > > I recommend: > - assert(device->buffers[0] == pDSB); > + if (pdevice->buffers[0] != pDSB) > + ERR("broken device buffer %p\n", device->buffers[0]); > HeapFree(GetProcessHeap(), 0, device->buffers); > device->buffers = NULL; > > The ERR is IMHO revealing enough in a user's log. That's too late if it hits the user. I had reviewed the code: That code is an implementation detail completely outside the control of the applications. Barring a memory corruption there is no way for it to fail. The assert is there for two reasons: - To crash during "make test" when the next developer touches that code. - As a comment less likely to be removed by Alexandre ;) > If a sound component detects a bogus state, > I'd very much prefer that it disables sound -- without crashing. In general "it depends" of course. In this particular case the ERR would be useless noise as the assert is only for the last sound buffer that is removed; usually at program exit. Until then the code does what you prefer: silently drops sound buffers without crashing thus deferring the crash to the program exit. A crash on program exit does get reported, more so than an ERR on the command line. bye michael

Sept. 27, 2012

Re: d3dx9_36 [patch 1/6]: Implement D3DXSHMultiply4

by Rico Schüller

On 27.09.2012 09:56, Nozomi Kodama wrote: > +FLOAT* WINAPI D3DXSHMultiply4(FLOAT *out, CONST FLOAT *a, CONST FLOAT *b) FLOAT * WINAPI D3DXSHMultiply4(FLOAT *out, const FLOAT *a, const FLOAT *b) A minor nitpick, anyone against something like? I think that was what Matteo meant in http://www.winehq.org/pipermail/wine-devel/2012-September/096924.html with "Fix the FLOAT*"? > + TRACE("(%p, %p, %p)\n", out, a, b); I would probably prefer: TRACE("out %p, a %p, b %p\n", out, a, b); I know the other Multiply functions and so on use your trace format... > + out[0] = 0.28209479f * a[0] * b[0]; There are still double spaces ... Hint: it's not the only occurrence. Cheers Rico

Sept. 27, 2012

Re: d3dx9_36 [patch 2/6]: Speed-up some tests

by Rico Schüller

On 27.09.2012 09:57, Nozomi Kodama wrote: > - FLOAT a[64], b[64], got; > + FLOAT a[49], b[49], got; > CONST FLOAT expected[] = > { 0.5f, 0.5f, 25.0f, 262.5f, 1428.0f, 5362.0f, 15873.0f, 39812.0f, 88400.0f, }; > > - for (i = 0; i < 64; i++) > + for (i = 0; i < 49; i++) > > - for (i = 0; i < 9; i++) > + for (i = 0; i <= D3DXSH_MAXORDER + 1; i++) Again: You may correct the (array) size for the expected value ... If we run til element 7 instead of 8, I think the last element could be thrown away? Cheers Rico

Sept. 27, 2012

dsound: Don't bother shrinking the secondary buffer list.

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

Michael, >+#include <assert.h> your patch is already committed, yet I believe that the least that dsound needs is new asserts. Asserts in dsound have been a PITA in Wine for the last decade. It's ok for the kernel to crash in an assert. IMHO it is not ok for an optional thing like sound. The assert's backtrace will not help anybody, because the logical error will have happened much earlier. Instead, a plain ERR would have been just as telling. Let's quote http://www.winehq.org/docs/winedev-guide/debugging#DBG-CLASSES "ERR: Messages in this class indicate serious errors in Wine, such as as conditions that should never happen by design." I recommend: - assert(device->buffers[0] == pDSB); + if (pdevice->buffers[0] != pDSB) + ERR("broken device buffer %p\n", device->buffers[0]); HeapFree(GetProcessHeap(), 0, device->buffers); device->buffers = NULL; The ERR is IMHO revealing enough in a user's log. If a sound component detects a bogus state, I'd very much prefer that it disables sound -- without crashing. Regards, Jörg Höhle

Sept. 27, 2012

is CreateThread(CREATE_SUSPENDED) fully functional?

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

Hi, Recently I thought I found a use-case for CreateThread(CREATE_SUSPENDED). The idiom is to get the thread handle before the thread starts a life on its own. I looked at the kernel tests, http://source.winehq.org/source/dlls/kernel32/tests/thread.c#L460 However, both invocations with CREATE_SUSPENDED actually test the pair CreateThread + SuspendThread. The tests do not prove that CREATE_SUSPENDED works! They only prove that the suspend counter and perhaps SuspendThread work. a) Does Wine not support CREATE_SUSPENDED, so that the tests do the best they can? In such a case, a WARN("ignoring SUSPENDED flag, beware of race conditions\n") would be appropriate. or b) CREATE_SUSPENDED is fully supported on all platforms. Then the tests need some refinement. Thank you, Jörg Höhle

Sept. 27, 2012

Re: Re : d3dx9 [patch 1/5]: Implement D3DXSHMultiply4

by Rico Schüller

On 26.09.2012 19:09, Nozomi Kodama wrote: > >> + D3DXSHMultiply4(c, a, b); > >>What happens if you use something like D3DXSHMultiply4(c, c, c). Is > that allowed? > > Tests in native show that D3DXSHMultiply gives what I implemented. > We can not reuse a input variable as output (we don't obtain what we > expect logically). I gave it a try, and your implementation seems to be correct. I tried: D3DXSHMultiply4(c, a, c); D3DXSHMultiply4(c, c, b); // used c[i] = ((float)i) / 20; to not give always -nan D3DXSHMultiply4(c, c, c); They look pretty much the same for wine and native. Why then not add a test for this? Sure, you don't get what you logically expect, but we need to reproduce the results and that has not always something to do with logic :-) . It is a little bit contrary to the part below, because when you change the order and reuse code, the calculation for this corner cases might get wrong (I'm not sure, haven't tried it). > > >Also is there a reason why it uses slightly different values than e.g. > D3DXSHMultiply3? Why don't we >use defines? A problem might be to find > good names... Maybe the Multiply functions could share the >same base? > To me it looks like they do share a lot of calculations, And if the > order doesn't matter (see >comment above) then it may be possible to > rearrange the calculation. > > > >ta = 0.28209479f * a[0] - 0.12615663f * a[6] - 0.21850969f * a[8]; > >vs > >ta = 0.28209479f * a[0] - 0.12615662f * a[6] - 0.21850968f * a[8]; > > I do the computations with a formal computation software. Unfortunately > I did not use the same for D3DXSHMultiply4 and D3DXSHMultiply3 ..... > (Xcas vs Maple). And obtain the exact values look beyond the capacities > of these softwares. > > And even the worst, since the integrated functions oscillate, the > internal algorithm of the software are not robust to compute the > integrals. For a given number of significative digits, that's why we > obtain slightly different values for the both softwares. > > Addind defines will add tons on variables for no gain. But I can > simplify slightly the function by calling D3DXSHMultiply3. Ok, no problem. See above. I'd say compatibility before simplicity. It was just a thought, because there was no test / comment in the code that makes my suggestion a stupid idea, but the test above makes this idea obsolete. Cheers Rico

Sept. 26, 2012

Re : d3dx9 [patch 1/5]: Implement D3DXSHMultiply4

by Nozomi Kodama

>On 26.09.2012 12:17, Nozomi Kodama wrote: >> + ta =0.28209479f * a[0]+ 0.14567312f * a[8]+ 0.12615663f * a[6]; >> + tb =0.28209479f * b[0]+ 0.14567312f * b[8]+ 0.12615663f * b[6]; >> + out[13] += ta * b[13] + tb * a[13]; <> + t = a[13] * b[13]; >> + out[0] +=0.28209479f *t; >Please use spaces with care. >The preferred way is "x = 123 * a[0] + 456 * b[234]; Mostly one space is enough, only use two or more if >you like to indent something, that's mostly not the case when calculating a value. Even then, please be >consistent across your patch. oops!!! I will take care of this one. >> + D3DXSHMultiply4(c, a, b); >>What happens if you use something like D3DXSHMultiply4(c, c, c). Is that allowed? Tests in native show that D3DXSHMultiply gives what I implemented. We can not reuse a input variable as output (we don't obtain what we expect logically). >Also is there a reason why it uses slightly different values than e.g. D3DXSHMultiply3? Why don't we >use defines? A problem might be to find good names... Maybe the Multiply functions could share the >same base? To me it looks like they do share a lot of calculations, And if the order doesn't matter (see >comment above) then it may be possible to rearrange the calculation. > >ta = 0.28209479f * a[0] - 0.12615663f * a[6] - 0.21850969f * a[8]; >vs >ta = 0.28209479f * a[0] - 0.12615662f * a[6] - 0.21850968f * a[8]; I do the computations with a formal computation software. Unfortunately I did not use the same for D3DXSHMultiply4 and D3DXSHMultiply3 ..... (Xcas vs Maple). And obtain the exact values look beyond the capacities of these softwares. And even the worst, since the integrated functions oscillate, the internal algorithm of the software are not robust to compute the integrals. For a given number of significative digits, that's why we obtain slightly different values for the both softwares. Addind defines will add tons on variables for no gain. But I can simplify slightly the function by calling D3DXSHMultiply3. I will give it a try. I worry about efficiency. Best regards Nozomi >Cheers >Rico

Sept. 26, 2012

Re: oledb32: Add support for IDBInitialize interface in IDataInitialize

by Alexandre Julliard

Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> writes: > Changelog: > oledb32: Add support for IDBInitialize interface in IDataInitialize It doesn't work here: ../../../tools/runtest -q -P wine -M oledb32.dll -T ../../.. -p oledb32_test.exe.so database.c && touch database.ok database.c:43: Test failed: Unable to load oledb library make[1]: *** [database.ok] Error 1 -- Alexandre Julliard julliard(a)winehq.org

Sept. 26, 2012

Re: (try 3) imm32: there is only 1 default Hwnd per thread

by Aric Stewart

Ok I will continue my investigates and try to resolve this or at least generate good test cases. This would also be easier with the thread work with user handles that I have needed in imm32 for longer than I remember the details about. :) But they relate to patches 8d12fc61d5321350ad63b173bdfb50feef1f4b56 and cc945706a4933cda0d204ba07a13e93fccb66d18 -aric On 9/26/12 10:26 AM, Alexandre Julliard wrote: > Aric Stewart <aric(a)codeweavers.com> writes: > >> Do you want to delay on this fix until I figure our what should be >> happening with IMMGWL_IMC. Or I can have a second patch addressing >> IMMGWL_IMC and how that is suppose to be handled. > > I think a few more test cases would be a good idea at this point. >

Sept. 26, 2012