June 2011 - Wine-Devel - list.winehq.org

Re: GSoC: dinput8 Action Mapping

by Marcus Meissner

On Tue, Jun 14, 2011 at 02:07:32PM -0300, Lucas Zawacki wrote: > > Also please implement unicode method and call it from ascii. (IDirectInput8AImpl_EnumDevicesBySemantics -> IDirectInput8WImpl_EnumDevicesBySemantics) > Sorry if that's a stupid question, but what should I do with the > string parameters? Is it ok to call a W function with the A > parameters? No, it would need to converted back and forth. I think for the first committed patches having them seperate is fine, it could be factored out later, as W<->A conversion for the callback handler will be tricky. Ciao, Marfcus

June 14, 2011

Re: wined3d performance patches

by Stefan Dösinger

On Tuesday 14 June 2011 16:15:35 Henri Verbeet wrote: > Yes, but I think that by now GF7 GPUs are marginal enough that it's > not worth keeping the code around for. The Steam HW survey for example > reports over 90% D3D10+ cards. Even if it does regress something, I > think it makes more sense to tell people to either file a bug with > NVIDIA for that or help improve the nouveau driver for that card. Fair enough. Note that Matteo found a similar bug with GL_TEXTURE_BASE_LEVEL on newer cards, but I encouraged him to report it to Nvidia rather than adding a hack to this function. > I'm not so sure. E.g. the docs for the INTZ format say you can have an > INTZ texture bound as both depth buffer and texture as long as depth > writes are disabled. (This makes some sense, since in that case there > aren't any read/write conflicts.) Yeah, although Aras Pranckevičius d3d hacks page claims that it doesn't work well(slow). Still, there are corner cases where the sampler states of a current render target may be changed on the d3d side and applied to the GL texture. > I'm not sure that can actually happen. wined3d_surface_set_format() > insists the format must be WINED3DFMT_UNKNOWN, so it can't be part of > a working FBO entry before the format is changed. That probably also > means clearing the allocation flags there is a bit silly. Hmm, I think I should reinvestigate those ddraw apps that call SetSurfaceDesc with strange parameters. Unfortunately I haven't documented which apps do that. I remember Windows Media Player 9 which sets the lpSurface field and expects subsequent Lock() calls to return this pointer :-O > > It shouldn't be. RTs must be in the default pool, which can't be > > unloaded. > > Even in ddraw? We need more tests for the DDSCAPS* mess, but the Direct3D7 docs say DDSCAPS2_TEXTUREMANAGE is incompatible with DDSCAPS_VIDEOMEMORY. I believe that DDSCAPS_3DDEVICE mandates DDSCAPS_VIDEOMEMORY, but atm I can't find the proper place in the docs to back this up. The d3d7 docs also say that TEXTUREMANAGE surfaces can be blitted to, and suggests that this is done in software and the result is then uploaded to the video memory copy.

June 14, 2011

re: dxdiag: Add code for XML information output

by Dan Kegel

Andrew Nguyan's commit, http://www.winehq.org/pipermail/wine-cvs/2011-June/078592.html used the construct static BOOL output_xml_information(struct dxdiag_information *dxdiag_info, const WCHAR *filename) { const struct information_block { const WCHAR *tag_name; struct information_field { const WCHAR *tag_name; const WCHAR *value; } fields[50]; } output_table[] = { {SystemInformation, { {Time, dxdiag_info->system_info.szTimeEnglish}, ... Isn't Wine was supposed to avoid non-static struct initializers? I think it'll get "warning: initializer element is not computable at load time" with --std=c89 --pedantic.

June 14, 2011

Re: GSoC: dinput8 Action Mapping

by Lucas Zawacki

> Also please implement unicode method and call it from ascii. (IDirectInput8AImpl_EnumDevicesBySemantics -> IDirectInput8WImpl_EnumDevicesBySemantics) Sorry if that's a stupid question, but what should I do with the string parameters? Is it ok to call a W function with the A parameters? >> + IDirectInputDevice8_SetDataFormat(iface, This->data_format.wine_df); > You do not want to do that. It's internal data format which does not have to be anything like real data format. You need to build your data format based on action mapping. I had something like this, not sure if it is correct: Thanks, that's interesting. I'll look further into this.

June 14, 2011

Re: GSoC: dinput8 Action Mapping

by Lucas Zawacki

> keybd_event( key, 0, 0, 0); > Have you tried using more meaningful parameters then 0? Yes, I've tried issuing a scan code as the second parameter, it was no use. I tried sending the keybd_event and then sending another one with KEYEVENTF_KEYUP to "emulate" a key press and a release and still no change. I've tried setting dummy values as the last parameter, but had similar results. I don't know what it's for and msdn was not helpful...

June 14, 2011

Re: [2/3] cmd/tests: Add support for todo_wine constructions (try 2)

by Jacek Caban

On 6/14/11 4:48 PM, Frédéric Delanoy wrote: > 2011/6/14 Jacek Caban<jacek(a)codeweavers.com>: >> On 6/14/11 3:04 PM, Frédéric Delanoy wrote: >>> 2011/6/14 Jacek Caban<jacek(a)codeweavers.com>: >>>> On 6/14/11 10:55 AM, Frédéric Delanoy wrote: >>>>> 011/6/14 Jacek Caban<jacek(a)codeweavers.com>: >>>>>> Hi Frédéric, >>>>>> >>>>>> On 6/13/11 10:19 PM, Frédéric Delanoy wrote: >>>>>>> + }else if(err) { >>>>>>> + if (!is_todo_wine) >>>>>>> + ok(0, "unexpected char 0x%x position %d in line %d >>>>>>> (got >>>>>>> '%.*s', wanted '%.*s')\n", >>>>>>> + *err, (int)(err-out_ptr), line, >>>>>>> (int)(out_nl-out_ptr), >>>>>>> + out_ptr, (int)(exp_nl-exp_ptr), exp_ptr); >>>>>>> + else >>>>>>> + todo_wine >>>>>>> + ok(0, "unexpected char 0x%x position %d in line %d >>>>>>> (got >>>>>>> '%.*s', wanted '%.*s')\n", >>>>>>> + *err, (int)(err-out_ptr), line, >>>>>>> (int)(out_nl-out_ptr), >>>>>>> out_ptr, >>>>>>> + (int)(exp_nl-exp_ptr-sizeof(todo_wine_cmd)), >>>>>>> exp_ptr+sizeof(todo_wine_cmd)); >>>>>> You may change tests to ok(!err, ....) here and make this else >>>>>> unconditional >>>>>> to avoid having ok(TRUE, ...) tests later. >>>>> Which else are you talking about? >>>>> >>>>> The following ok(TRUE,...) serves when there's a todo_wine which has >>>>> "succeeded", to get messages like "Test succeeded inside todo block: >>>>> match at line XXX" >>>>> Without it (i.e. without at least one "true" OK), successful todos >>>>> aren't caught, as Dan Kegel pointed out (and I've verified). >>>> I was talking about something like this: >>>> >>>> }else { >>>> if(!is_todo_wine) >>>> ... >>>> else todo_wine >>>> ok(!err, ...); >>>> } >>>> >>>> If the tests succeeded, err will be NULL, so this ok will produce "Test >>>> succeeded inside todo block" error. >>> I tried what you say but it didn't work: the test runner doesn't even >>> go that far, and doesn't detect any todo, just says something like >>> >>> batch: 44 tests executed (0 marked as todo, 0 failures), 0 skipped. >>> >>> instead of the> 100 tests (with a couple of todos for the mkdir >>> patch) it should make. >>> >>> Simply replacing "ok(0,...)" and "ok(TRUE,...)" by "ok(!res,...)" >>> works, >> Sure it works. But... what were your previous sentences about? > see below > >>> but is not more clear IMHO >> ok(TRUE, ...) simply doesn't make sense. All we care is that tests don't >> fail. A number of succeeded tests doesn't matter and ok(TRUE, ...) is just >> an useless expression. todo_wine ok(TRUE, ...) makes some sense, but still, >> it looks hackish, so I'd rather avoid it. > I agree. And the "ok(0, ...)" you wrote some time ago don't look less > hackish IMHO ;). It does IMHO, ok(0, ...) is not no-op, ok(TRUE, ...) is. > The ok(TRUE,...) is used so that, when you have 123 and you expect > @todo_wine(a)123, you get warned about it ("Test succeeded inside todo > block"). todo_wine ok(TRUE, ...) is not the same as ok(TRUE, ...). As I said, it makes sense, it's just hackish and if it's easily avoidable, why not avoid it. > Without it, I couldn't find a way to get it working. > I may be incredibly stupid, but I think I still don't get what changes you want: > > Do you want this? No, that doesn't make sense. > if(err == out_nl) { > if (!is_todo_wine) > ok(!err, "unexpected end of line %d (got '%.*s', > wanted '%.*s')\n", > line, (int)(out_nl-out_ptr), out_ptr, > (int)(exp_nl-exp_ptr), exp_ptr); > else > todo_wine > ok(!err, "unexpected end of line %d (got '%.*s', > wanted '%.*s')\n", > line, (int)(out_nl-out_ptr), out_ptr, > (int)(exp_nl-exp_ptr-sizeof(todo_wine_cmd)), > exp_ptr+sizeof(todo_wine_cmd)); > }else if(err) { > if (!is_todo_wine) > ok(!err, "unexpected char 0x%x position %d in line %d > (got '%.*s', wanted '%.*s')\n", > *err, (int)(err-out_ptr), line, (int)(out_nl-out_ptr), > out_ptr, (int)(exp_nl-exp_ptr), exp_ptr); > else > todo_wine > ok(!err, "unexpected char 0x%x position %d in line %d > (got '%.*s', wanted '%.*s')\n", > *err, (int)(err-out_ptr), line, > (int)(out_nl-out_ptr), out_ptr, > (int)(exp_nl-exp_ptr-sizeof(todo_wine_cmd)), > exp_ptr+sizeof(todo_wine_cmd)); > }else { > if(!is_todo_wine) > ok(!err, "match at line %d\n", line); > else > todo_wine ok(!err, "match at line %d\n", line); > } > > Or did I misunderstand in > > + }else if(err) { > + if (!is_todo_wine) > + ok(0, "unexpected char 0x%x position %d in line %d > (got '%.*s', wanted '%.*s')\n", > + *err, (int)(err-out_ptr), line, (int)(out_nl-out_ptr), > + out_ptr, (int)(exp_nl-exp_ptr), exp_ptr); > + else > + todo_wine > + ok(0, "unexpected char 0x%x position %d in line %d > (got '%.*s', wanted '%.*s')\n", > + *err, (int)(err-out_ptr), line, > (int)(out_nl-out_ptr), out_ptr, > + (int)(exp_nl-exp_ptr-sizeof(todo_wine_cmd)), > exp_ptr+sizeof(todo_wine_cmd)); > "You may change tests to ok(!err, ....) here and make this else > unconditional to avoid having ok(TRUE, ...) tests later." > > the "make this else unconditional" to mean "remove the if(err)", and > "to avoid having ok(TRUE, ...) tests late" to mean "delete the last > else clause containing the 'ok(TRUE, ...)' calls, and remove the c" ? Yes, I meant to remove if(!err), make ok calls inside this else block ok(!err, ...) and remove the other else block you've added that contained ok(TRUE, ...). Jacek

June 14, 2011

Re: [2/3] cmd/tests: Add support for todo_wine constructions (try 2)

by Frédéric Delanoy

2011/6/14 Jacek Caban <jacek(a)codeweavers.com>: > On 6/14/11 3:04 PM, Frédéric Delanoy wrote: >> >> 2011/6/14 Jacek Caban<jacek(a)codeweavers.com>: >>> >>> On 6/14/11 10:55 AM, Frédéric Delanoy wrote: >>>> >>>> 011/6/14 Jacek Caban<jacek(a)codeweavers.com>: >>>>> >>>>> Hi Frédéric, >>>>> >>>>> On 6/13/11 10:19 PM, Frédéric Delanoy wrote: >>>>>> >>>>>> + }else if(err) { >>>>>> + if (!is_todo_wine) >>>>>> + ok(0, "unexpected char 0x%x position %d in line %d >>>>>> (got >>>>>> '%.*s', wanted '%.*s')\n", >>>>>> + *err, (int)(err-out_ptr), line, >>>>>> (int)(out_nl-out_ptr), >>>>>> + out_ptr, (int)(exp_nl-exp_ptr), exp_ptr); >>>>>> + else >>>>>> + todo_wine >>>>>> + ok(0, "unexpected char 0x%x position %d in line %d >>>>>> (got >>>>>> '%.*s', wanted '%.*s')\n", >>>>>> + *err, (int)(err-out_ptr), line, >>>>>> (int)(out_nl-out_ptr), >>>>>> out_ptr, >>>>>> + (int)(exp_nl-exp_ptr-sizeof(todo_wine_cmd)), >>>>>> exp_ptr+sizeof(todo_wine_cmd)); >>>>> >>>>> You may change tests to ok(!err, ....) here and make this else >>>>> unconditional >>>>> to avoid having ok(TRUE, ...) tests later. >>>> >>>> Which else are you talking about? >>>> >>>> The following ok(TRUE,...) serves when there's a todo_wine which has >>>> "succeeded", to get messages like "Test succeeded inside todo block: >>>> match at line XXX" >>>> Without it (i.e. without at least one "true" OK), successful todos >>>> aren't caught, as Dan Kegel pointed out (and I've verified). >>> >>> I was talking about something like this: >>> >>> }else { >>> if(!is_todo_wine) >>> ... >>> else todo_wine >>> ok(!err, ...); >>> } >>> >>> If the tests succeeded, err will be NULL, so this ok will produce "Test >>> succeeded inside todo block" error. >> >> I tried what you say but it didn't work: the test runner doesn't even >> go that far, and doesn't detect any todo, just says something like >> >> batch: 44 tests executed (0 marked as todo, 0 failures), 0 skipped. >> >> instead of the> 100 tests (with a couple of todos for the mkdir >> patch) it should make. >> >> Simply replacing "ok(0,...)" and "ok(TRUE,...)" by "ok(!res,...)" >> works, > > Sure it works. But... what were your previous sentences about? see below >> but is not more clear IMHO > > ok(TRUE, ...) simply doesn't make sense. All we care is that tests don't > fail. A number of succeeded tests doesn't matter and ok(TRUE, ...) is just > an useless expression. todo_wine ok(TRUE, ...) makes some sense, but still, > it looks hackish, so I'd rather avoid it. I agree. And the "ok(0, ...)" you wrote some time ago don't look less hackish IMHO ;). The ok(TRUE,...) is used so that, when you have 123 and you expect @todo_wine(a)123, you get warned about it ("Test succeeded inside todo block"). Without it, I couldn't find a way to get it working. I may be incredibly stupid, but I think I still don't get what changes you want: Do you want this? if(err == out_nl) { if (!is_todo_wine) ok(!err, "unexpected end of line %d (got '%.*s', wanted '%.*s')\n", line, (int)(out_nl-out_ptr), out_ptr, (int)(exp_nl-exp_ptr), exp_ptr); else todo_wine ok(!err, "unexpected end of line %d (got '%.*s', wanted '%.*s')\n", line, (int)(out_nl-out_ptr), out_ptr, (int)(exp_nl-exp_ptr-sizeof(todo_wine_cmd)), exp_ptr+sizeof(todo_wine_cmd)); }else if(err) { if (!is_todo_wine) ok(!err, "unexpected char 0x%x position %d in line %d (got '%.*s', wanted '%.*s')\n", *err, (int)(err-out_ptr), line, (int)(out_nl-out_ptr), out_ptr, (int)(exp_nl-exp_ptr), exp_ptr); else todo_wine ok(!err, "unexpected char 0x%x position %d in line %d (got '%.*s', wanted '%.*s')\n", *err, (int)(err-out_ptr), line, (int)(out_nl-out_ptr), out_ptr, (int)(exp_nl-exp_ptr-sizeof(todo_wine_cmd)), exp_ptr+sizeof(todo_wine_cmd)); }else { if(!is_todo_wine) ok(!err, "match at line %d\n", line); else todo_wine ok(!err, "match at line %d\n", line); } Or did I misunderstand in + }else if(err) { + if (!is_todo_wine) + ok(0, "unexpected char 0x%x position %d in line %d (got '%.*s', wanted '%.*s')\n", + *err, (int)(err-out_ptr), line, (int)(out_nl-out_ptr), + out_ptr, (int)(exp_nl-exp_ptr), exp_ptr); + else + todo_wine + ok(0, "unexpected char 0x%x position %d in line %d (got '%.*s', wanted '%.*s')\n", + *err, (int)(err-out_ptr), line, (int)(out_nl-out_ptr), out_ptr, + (int)(exp_nl-exp_ptr-sizeof(todo_wine_cmd)), exp_ptr+sizeof(todo_wine_cmd)); "You may change tests to ok(!err, ....) here and make this else unconditional to avoid having ok(TRUE, ...) tests later." the "make this else unconditional" to mean "remove the if(err)", and "to avoid having ok(TRUE, ...) tests late" to mean "delete the last else clause containing the 'ok(TRUE, ...)' calls, and remove the c" ? I must admit I'm a bit confused Frédéric

June 14, 2011

Re: [2/3] cmd/tests: Add support for todo_wine constructions (try 2)

by Jacek Caban

On 6/14/11 3:04 PM, Frédéric Delanoy wrote: > 2011/6/14 Jacek Caban<jacek(a)codeweavers.com>: >> On 6/14/11 10:55 AM, Frédéric Delanoy wrote: >>> 011/6/14 Jacek Caban<jacek(a)codeweavers.com>: >>>> Hi Frédéric, >>>> >>>> On 6/13/11 10:19 PM, Frédéric Delanoy wrote: >>>>> + }else if(err) { >>>>> + if (!is_todo_wine) >>>>> + ok(0, "unexpected char 0x%x position %d in line %d (got >>>>> '%.*s', wanted '%.*s')\n", >>>>> + *err, (int)(err-out_ptr), line, >>>>> (int)(out_nl-out_ptr), >>>>> + out_ptr, (int)(exp_nl-exp_ptr), exp_ptr); >>>>> + else >>>>> + todo_wine >>>>> + ok(0, "unexpected char 0x%x position %d in line %d (got >>>>> '%.*s', wanted '%.*s')\n", >>>>> + *err, (int)(err-out_ptr), line, >>>>> (int)(out_nl-out_ptr), >>>>> out_ptr, >>>>> + (int)(exp_nl-exp_ptr-sizeof(todo_wine_cmd)), >>>>> exp_ptr+sizeof(todo_wine_cmd)); >>>> You may change tests to ok(!err, ....) here and make this else >>>> unconditional >>>> to avoid having ok(TRUE, ...) tests later. >>> Which else are you talking about? >>> >>> The following ok(TRUE,...) serves when there's a todo_wine which has >>> "succeeded", to get messages like "Test succeeded inside todo block: >>> match at line XXX" >>> Without it (i.e. without at least one "true" OK), successful todos >>> aren't caught, as Dan Kegel pointed out (and I've verified). >> I was talking about something like this: >> >> }else { >> if(!is_todo_wine) >> ... >> else todo_wine >> ok(!err, ...); >> } >> >> If the tests succeeded, err will be NULL, so this ok will produce "Test >> succeeded inside todo block" error. > I tried what you say but it didn't work: the test runner doesn't even > go that far, and doesn't detect any todo, just says something like > > batch: 44 tests executed (0 marked as todo, 0 failures), 0 skipped. > > instead of the> 100 tests (with a couple of todos for the mkdir > patch) it should make. > > Simply replacing "ok(0,...)" and "ok(TRUE,...)" by "ok(!res,...)" > works, Sure it works. But... what were your previous sentences about? > but is not more clear IMHO ok(TRUE, ...) simply doesn't make sense. All we care is that tests don't fail. A number of succeeded tests doesn't matter and ok(TRUE, ...) is just an useless expression. todo_wine ok(TRUE, ...) makes some sense, but still, it looks hackish, so I'd rather avoid it. > Anyway, the current test runner code is a bit fragile/ugly at times > right now, and would need some refactoring (e.g. to factor out > @keyword@ expansion, give better error messages,...) but that is > probably out of scope of this patch series. I don't see what's so ugly about it. Factoring out @keyword@ expansions won't make things any cleaner, IMO. Parsing code is so simple here that I wouldn't make a big deal out of it. Error messages don't matter much - after all you should never see them. Perhaps printing the whole failed line may help writing tests a bit, but that's just one trivial change here. Jacek

June 14, 2011

Re: wined3d performance patches

by Henri Verbeet

On 14 June 2011 15:26, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote: >> As far as I'm concerned you can just submit this. I was going to do >> this myself, looks like you got there first. > Still didn't get around to test this on geforce 7 GPUs. It's possible that the > bug this was supposed to fix is still around. > Yes, but I think that by now GF7 GPUs are marginal enough that it's not worth keeping the code around for. The Steam HW survey for example reports over 90% D3D10+ cards. Even if it does regress something, I think it makes more sense to tell people to either file a bug with NVIDIA for that or help improve the nouveau driver for that card. > Besides, it is probably not necessary for the other patches. The consideration > was that we'd have to verify the filter each draw, but I don't think setting a > texture as sampler and render target simultaneously is allowed in d3d. > I'm not so sure. E.g. the docs for the INTZ format say you can have an INTZ texture bound as both depth buffer and texture as long as depth writes are disabled. (This makes some sense, since in that case there aren't any read/write conflicts.) >> > @@ -1913,6 +1928,10 @@ void surface_set_texture_name(struct >> > ... >> > + if (surface_is_framebuffer(surface)) >> > + { >> > + IWineD3DDeviceImpl_MarkStateDirty(surface->resource.device, >> > STATE_FRAMEBUFFER); + } >> > ... >> What are these for? > The texture name one is not needed, I've removed that from the patch already. > The allocate_surface check is needed in case a ddraw app changes the > pixelformat via SetSurfaceDesc. > I'm not sure that can actually happen. wined3d_surface_set_format() insists the format must be WINED3DFMT_UNKNOWN, so it can't be part of a working FBO entry before the format is changed. That probably also means clearing the allocation flags there is a bit silly. >> You may also have to handle an active RT getting unloaded, though I'm >> not entirely sure if that's allowed or not. > It shouldn't be. RTs must be in the default pool, which can't be unloaded. Even in ddraw? >> I wonder if >> the speedup is mostly for load_location(), modify_location() or both >> though? Maybe we can improve those functions themselves. > It's caused by both, I think it's plain call overhead. I'll double check that > though. It may also be hyper-sensitivity of the draw overhead test. 260->270 > fps isn't a lot when you consider that native gets ~1100 fps. But right now I > have to take what I can get. > Maybe surface_load_location() could do an initial location check a bit earlier. (And some of the code before the current check could also be removed when we get rid of texture == drawable.) For surface_modify_location(), the overlay code probably doesn't belong in there, we could do a similar early check if the flags already match, and maybe we should split it in two functions.

June 14, 2011

Re: [PATCH 2/2] d3dx9: Return E_FAIL for unsupported ascii effects.

by Alexandre Julliard

Rico Schüller <kgbricola(a)web.de> writes: > --- > dlls/d3dx9_36/effect.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) It breaks the tests: ../../../tools/runtest -q -P wine -M d3dx9_36.dll -T ../../.. -p d3dx9_36_test.exe.so effect.c && touch effect.ok effect.c:52: Test failed: Got result 80004005, expected 0 (D3D_OK) wine: Unhandled page fault on execute access to 0x00000000 at address (nil) (thread 0023), starting debugger... -- Alexandre Julliard julliard(a)winehq.org

June 14, 2011

Re: GSoC: dinput8 Action Mapping

by Vitaliy Margolen

On 06/13/2011 04:17 PM, Lucas Zawacki wrote: > OK, so here is my second try at some patches. These mostly implement > keyboard action mapping and there's plenty of tests to go together > with it. I believe the keyboard part is pretty much done, as is > SetActionMap. Of course I'm ignoring the username strings and all the > device ownership part, I'll leave that for later. > > As before I'll leave the patches here to get some feedback before I > try to commit. > > Subject: dinput: EnumDevicesBySemantics enumerating system keyboard and mouse > + DWORD callbackFlags = 0; > + ret = lpCallback(&didevi, lpdid, callbackFlags , 0, pvRef); I'd like to see a test for callbackFlags = 0. My notes indicate this should be DIEDBS_MAPPEDPRI1 instead. Also please implement unicode method and call it from ascii. (IDirectInput8AImpl_EnumDevicesBySemantics -> IDirectInput8WImpl_EnumDevicesBySemantics) > Subject: dinput: SetActionMap setting the device buffer and data format. > + IDirectInputDevice8_SetDataFormat(iface, This->data_format.wine_df); You do not want to do that. It's internal data format which does not have to be anything like real data format. You need to build your data format based on action mapping. I had something like this, not sure if it is correct: + DIDATAFORMAT data_format; + DIOBJECTDATAFORMAT *obj_df; + + data_format.dwSize = sizeof(data_format); + data_format.dwObjSize = sizeof(DIOBJECTDATAFORMAT); + data_format.dwFlags = DIDF_ABSAXIS; + data_format.dwDataSize = lpdiaf->dwDataSize; + data_format.dwNumObjs = lpdiaf->dwNumActions; + data_format.rgodf = (LPDIOBJECTDATAFORMAT)obj_df; + + for (i = 0; i < lpdiaf->dwNumActions; i++) + { + if ((lpdiaf->rgoAction[i].dwSemantic & 0x81000400) == 0x81000400) + /* Keyboard */ + memcpy(&obj_df[i], &c_dfDIKeyboard.rgodf[DIDFT_GETINSTANCE(lpdiaf->rgoAction[i].dwObjID)], c_dfDIKeyboard.dwObjSize); + else if ((lpdiaf->rgoAction[i].dwSemantic & 0x82000000) == 0x82000000) + /* Mouse */ + memcpy(&obj_df[i], &c_dfDIMouse2.rgodf[DIDFT_GETINSTANCE(lpdiaf->rgoAction[i].dwObjID)], c_dfDIMouse2.dwObjSize); + + obj_df[i].dwOfs = i * 4; + + This->action_map[DIDFT_GETINSTANCE(lpdiaf->rgoAction[i].dwObjID)] = lpdiaf->rgoAction[i].uAppData; + } > + /* search for actions for this device and apply them */ > + for (i=0; i < lpdiaf->dwNumActions; i++) { > + if (IsEqualGUID( &lpdiaf->rgoAction[i].guidInstance, &This->guid ) ) { As I asked before, please put curly braces on a separate line. Vitaliy.

June 14, 2011

Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9

by Dylan Smith

On Fri, Jun 10, 2011 at 2:08 AM, Dylan Smith <dylan.ah.smith(a)gmail.com> wrote: > D3DXCreateMesh fails when the declaration contains a non-zero Stream > value. I would expect UpdateSemantics to be as strict as D3DXCreateMesh, > otherwise a test could validate the different behaviour. Seems like you missed this comment I made before. /* e.g. */ memcpy(declaration, declaration0, sizeof(declaration0)); declaration[1].Stream = 1; hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration); ok(hr == D3DERR_INVALIDCALL, "Test UpdateSematics using multiple streams, " "got %#x expected D3DERR_INVALIDCALL\n", hr);

June 14, 2011

Re: GSoC: dinput8 Action Mapping

by Vitaliy Margolen

On 06/12/2011 07:45 PM, Lucas Zawacki wrote: > I'm having some issues with test that inject input. For instance this > code ( http://pastebin.com/gan0Niqj ) works as expected under wine and > wine with dinput8.dll , but fails in every windows version... I tried > a version with SendInput and had the same results... > > Anyone has some insight on this? > > keybd_event( key, 0, 0, 0); Have you tried using more meaningful parameters then 0? Vitaliy.

June 14, 2011

Re: wined3d performance patches

by Stefan Dösinger

Hi, Thanks for the comments, I've some replies inlined below. The bigger concern I have is that the patches don't improvement by a lot yet, about 1% on AMD GPUs and 1.5% to 2.5% on Nvidia GPUs(in real apps, my benchmarks are a different issue). Because of that my plan is to do more testing and experiment with things like separating the vdecl-streamsrc-vshader states et cetera and see how much impact this has. All in all I am not yet convinced that my approach with lots of very targeted tests is the way to salvation, and I want to wait a bit before I start screwing up the code. Unfortunately it is still the best way I have. I'm sending in some of the patches that make sense even without performance improvements. The MINFILTER stuff is a case like that, but I want to test it on pre-dx10 GPUs first. On Tuesday 14 June 2011 14:20:12 Henri Verbeet wrote: > I only looked over about half of this, doesn't look too crazy. You do > have trailing spaces in a couple of places though. Here are some Ya, I spotted them myself a while later, and didn't look for style issues before sending them to wine-devel. Though maybe I should look for better tools to watch out for slopiness like this. > As far as I'm concerned you can just submit this. I was going to do > this myself, looks like you got there first. Still didn't get around to test this on geforce 7 GPUs. It's possible that the bug this was supposed to fix is still around. Besides, it is probably not necessary for the other patches. The consideration was that we'd have to verify the filter each draw, but I don't think setting a texture as sampler and render target simultaneously is allowed in d3d. > > @@ -1913,6 +1928,10 @@ void surface_set_texture_name(struct > > ... > > + if (surface_is_framebuffer(surface)) > > + { > > + IWineD3DDeviceImpl_MarkStateDirty(surface->resource.device, > > STATE_FRAMEBUFFER); + } > > ... > What are these for? The texture name one is not needed, I've removed that from the patch already. The allocate_surface check is needed in case a ddraw app changes the pixelformat via SetSurfaceDesc. > You may also have to handle an active RT getting unloaded, though I'm > not entirely sure if that's allowed or not. It shouldn't be. RTs must be in the default pool, which can't be unloaded. Of course that doesn't stop our broken location management from screwing up, but that would be a separate issue. > > Subject: [PATCH 09/14] wined3d: Make render target dirtification in draws > > cheaper > I've been thinking about getting rid of the texture == drawable stuff > for FBOs, and instead just making texture <-> drawable loads a nop in > load_location(). At the very least this would allow the check to > always be against SFLAG_INDRAWABLE instead of > surface->rt_location_flags, which IMHO looks more sane. Yes, this sounds much better. > I wonder if > the speedup is mostly for load_location(), modify_location() or both > though? Maybe we can improve those functions themselves. It's caused by both, I think it's plain call overhead. I'll double check that though. It may also be hyper-sensitivity of the draw overhead test. 260->270 fps isn't a lot when you consider that native gets ~1100 fps. But right now I have to take what I can get.

June 14, 2011

Re: [2/3] cmd/tests: Add support for todo_wine constructions (try 2)

by Frédéric Delanoy

2011/6/14 Jacek Caban <jacek(a)codeweavers.com>: > On 6/14/11 10:55 AM, Frédéric Delanoy wrote: >> >> 011/6/14 Jacek Caban<jacek(a)codeweavers.com>: >>> >>> Hi Frédéric, >>> >>> On 6/13/11 10:19 PM, Frédéric Delanoy wrote: >>>> >>>> + }else if(err) { >>>> + if (!is_todo_wine) >>>> + ok(0, "unexpected char 0x%x position %d in line %d (got >>>> '%.*s', wanted '%.*s')\n", >>>> + *err, (int)(err-out_ptr), line, >>>> (int)(out_nl-out_ptr), >>>> + out_ptr, (int)(exp_nl-exp_ptr), exp_ptr); >>>> + else >>>> + todo_wine >>>> + ok(0, "unexpected char 0x%x position %d in line %d (got >>>> '%.*s', wanted '%.*s')\n", >>>> + *err, (int)(err-out_ptr), line, >>>> (int)(out_nl-out_ptr), >>>> out_ptr, >>>> + (int)(exp_nl-exp_ptr-sizeof(todo_wine_cmd)), >>>> exp_ptr+sizeof(todo_wine_cmd)); >>> >>> You may change tests to ok(!err, ....) here and make this else >>> unconditional >>> to avoid having ok(TRUE, ...) tests later. >> >> Which else are you talking about? >> >> The following ok(TRUE,...) serves when there's a todo_wine which has >> "succeeded", to get messages like "Test succeeded inside todo block: >> match at line XXX" >> Without it (i.e. without at least one "true" OK), successful todos >> aren't caught, as Dan Kegel pointed out (and I've verified). > > I was talking about something like this: > > }else { > if(!is_todo_wine) > ... > else todo_wine > ok(!err, ...); > } > > If the tests succeeded, err will be NULL, so this ok will produce "Test > succeeded inside todo block" error. I tried what you say but it didn't work: the test runner doesn't even go that far, and doesn't detect any todo, just says something like batch: 44 tests executed (0 marked as todo, 0 failures), 0 skipped. instead of the > 100 tests (with a couple of todos for the mkdir patch) it should make. Simply replacing "ok(0,...)" and "ok(TRUE,...)" by "ok(!res,...)" works, but is not more clear IMHO Anyway, the current test runner code is a bit fragile/ugly at times right now, and would need some refactoring (e.g. to factor out @keyword@ expansion, give better error messages,...) but that is probably out of scope of this patch series. Frédéric

June 14, 2011

Re: wined3d performance patches

by Henri Verbeet

I only looked over about half of this, doesn't look too crazy. You do have trailing spaces in a couple of places though. Here are some comments: > Subject: [PATCH 04/14] wined3d: Don't set FBO attachment filtering to GL_NEAREST As far as I'm concerned you can just submit this. I was going to do this myself, looks like you got there first. > Subject: [PATCH 05/14] wined3d: Move FBO application into a state handler This needs an update to debug_d3dstate() as well. > +static BOOL surface_is_framebuffer(struct wined3d_surface *surface) > +{ > + struct wined3d_device *device = surface->resource.device; > + unsigned int i; > + const struct wined3d_gl_info *gl_info = &device->adapter->gl_info; > + > + if (surface == device->fb.depth_stencil) return TRUE; > + if (!device->fb.render_targets) return FALSE; > + for (i = 0; i < gl_info->limits.buffers; i++) > + { > + if (surface == device->fb.render_targets[i]) return TRUE; > + } > + return FALSE; > +} I don't know how much this actually hurts, but we could introduce something along the lines of "SFLAG_ACTIVE_RT" to keep track of this. We may have to worry about surfaces being bound to more than one RT attachment point, in which case we'd need a count instead of a flag though. > @@ -1913,6 +1928,10 @@ void surface_set_texture_name(struct wined3d_surface *surface, GLuint new_name, > · > *name = new_name; > surface_force_reload(surface); > + if (surface_is_framebuffer(surface)) > + { > + IWineD3DDeviceImpl_MarkStateDirty(surface->resource.device, STATE_FRAMEBUFFER); > + } > } > · > void surface_set_texture_target(struct wined3d_surface *surface, GLenum target) > @@ -2317,6 +2336,10 @@ static void surface_allocate_surface(struct wined3d_surface *surface, const stru > checkGLcall("glPixelStorei(GL_UNPACK_CLIENT_STORAGE_APPLE, GL_TRUE)"); > } > LEAVE_GL(); > + if (surface_is_framebuffer(surface)) > + { > + IWineD3DDeviceImpl_MarkStateDirty(surface->resource.device, STATE_FRAMEBUFFER); > + } > } What are these for? You may also have to handle an active RT getting unloaded, though I'm not entirely sure if that's allowed or not. > + BOOL all_samplers_mapped = TRUE; ... > + all_samplers_mapped = FALSE; This looks unused. > + for(i = 0; i < swapchain->presentParms.BackBufferCount; i++) Style. > Subject: [PATCH 09/14] wined3d: Make render target dirtification in draws cheaper I'm not sure about this one. My first impression is that it looks fragile. The rt_location_flags setup makes assumptions about load_location() internals, which will likely change at some point. In particular, now that most of the location management goes through the appropriate functions instead of messing with the flags themselves, I've been thinking about getting rid of the texture == drawable stuff for FBOs, and instead just making texture <-> drawable loads a nop in load_location(). At the very least this would allow the check to always be against SFLAG_INDRAWABLE instead of surface->rt_location_flags, which IMHO looks more sane. I wonder if the speedup is mostly for load_location(), modify_location() or both though? Maybe we can improve those functions themselves.

June 14, 2011

Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9

by Stefan Dösinger

On Tuesday 14 June 2011 12:57:35 Michael Mc Donnell wrote: > I cache the vertex declaration (D3DVERTEXELEMENT9 array) and the size > of the vertex declaration, so that they can be used by > GetNumBytesPerVertex and GetDeclaration when an invalid declaration > has been passed. In GetDeclaration: > + memcpy(declaration, This->cached_declaration, sizeof(This- >cached_declaration)); You should probably test if native really writes the full MAX_FVF_DECL_SIZE declaration elements, or only up to the D3DDECL_END() marker in the real declaration. A similar consideration applies to UpdateSemantics, but that is harder to test. E.g.: + D3DVERTEXELEMENT9 declaration[MAX_FVF_DECL_SIZE]; + D3DVERTEXELEMENT9 declaration0[] = + { ... memset(declaration, 0xaa, sizeof(declaration)); memcpy(declaration, declaration0, sizeof(declaration0)); UpdateSemantics(declaration) memset(declaration, 0xbb, sizeof(declaration)); GetDeclaration(declaration); Now look at the bytes in 'declaration' after the D3DDECL_END() marker up to the end of the array. They could be 0xaa(UpdateSemantics reads everything, GetDeclaration writes everything), 0xbb(UpdateSemantics is unknown, GetDeclaration writes only the defined part), or something else(e.g. UpdateSemantics and/or GetDeclaration clear set the extra bytes to 0). In the way your implementation and tests are currently set up UpdateSemantics will read undefined memory because your test declarations like declaration0 don't have the full MAX_FVF_DECL_SIZE array size. Otoh I am not quite sure about the semantics of passing an array on the stack like this. Maybe the compiler allocates a new array with the full size and copies the content around. I have to say though that the way the API is defined is a bit odd. But I checked it, and UpdateSemantics and GetDeclaration are defined in this way in the DirectX SDK headers. Also I think this method has been pretty thoroughly tested and debated by now. If we implement all the functions this way we'd probably be pretty bug-free :-) Also, there's the obligatory style nitpick: > + IDirect3DDevice9 *device; > ... > +static struct test_context* new_test_context(void) For consistency this should probably be static struct test_context *new_test_context(void).

June 14, 2011

Re: winex11.drv: Only set MWM hints if they have not been modified

by Alexandre Julliard

Scott Talbert <talbert(a)techie.net> writes: > Hello, > > The below patch resolves an issue (BZ# 27473) where the window manager > removes window decorations and Wine restores them. The code in the > patch checks to see whether the window decorations have been modified > and if so, > it does not modify them further. That's a Unity bug. The window manager is not supposed to change the MWM hints, they are controlled by the client. -- Alexandre Julliard julliard(a)winehq.org

June 14, 2011

Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9

by Michael Mc Donnell

I have added more tests and changed the UpdateSemantics implementation quite a bit. Dylan Smith made me aware that GetNumBytesPerVertex and GetDeclaration work with an invalid declaration, so I've added tests for that and changed the implementation to do the same. I cache the vertex declaration (D3DVERTEXELEMENT9 array) and the size of the vertex declaration, so that they can be used by GetNumBytesPerVertex and GetDeclaration when an invalid declaration has been passed. Any comments?

June 14, 2011

Re: GSoC: dinput8 Action Mapping

by Marcus Meissner

On Mon, Jun 13, 2011 at 07:17:04PM -0300, Lucas Zawacki wrote: > OK, so here is my second try at some patches. These mostly implement > keyboard action mapping and there's plenty of tests to go together > with it. I believe the keyboard part is pretty much done, as is > SetActionMap. Of course I'm ignoring the username strings and all the > device ownership part, I'll leave that for later. > > As before I'll leave the patches here to get some feedback before I > try to commit. Looks good to me. I spotted one small flaw in binary logic: > +/* This is used for the action mapping of constants such as DIKEYBOARD_SPACE */ > +static DWORD keyboard_semantic_to_obj_id(DWORD dwSemantic) > +{ > + /* dwSemantic=810004df is dwObjID=0xdf04 */ > + return 0x0000ffff & ( (dwSemantic << 8) | (dwSemantic >> 8) ); > +} The >>8 shift will drag in bits 16-23 from dwSemantic, which is probably not intended. (think dwSemantic = 0x4242df04 , it will roate in the 0x42) dwSemantic &= 0x0000ffff; before the operation will help. If no negative comments appear, please post them as 1 patch per email to wine-patches ;) Ciao, Marcus

June 14, 2011

Re: [1/3] wined3d: Add GL_ARB_draw_elements_base_vertex

by Henri Verbeet

On 13 June 2011 22:59, Stefan Dösinger <stefan(a)codeweavers.com> wrote: > +typedef void (WINE_GLAPI *PGLFNDRAWELEMENTSBASEVERTEX)(GLenum mode, GLsizei count, GLenum type, > + const GLvoid *indices, GLint basevertex); With the usual naming convention this would be called "PGLFNDRAWELEMENTSBASEVERTEXPROC".

June 14, 2011

Re: [2/3] cmd/tests: Add support for todo_wine constructions (try 2)

by Jacek Caban

On 6/14/11 10:55 AM, Frédéric Delanoy wrote: > 011/6/14 Jacek Caban<jacek(a)codeweavers.com>: >> Hi Frédéric, >> >> On 6/13/11 10:19 PM, Frédéric Delanoy wrote: >>> + }else if(err) { >>> + if (!is_todo_wine) >>> + ok(0, "unexpected char 0x%x position %d in line %d (got >>> '%.*s', wanted '%.*s')\n", >>> + *err, (int)(err-out_ptr), line, (int)(out_nl-out_ptr), >>> + out_ptr, (int)(exp_nl-exp_ptr), exp_ptr); >>> + else >>> + todo_wine >>> + ok(0, "unexpected char 0x%x position %d in line %d (got >>> '%.*s', wanted '%.*s')\n", >>> + *err, (int)(err-out_ptr), line, (int)(out_nl-out_ptr), >>> out_ptr, >>> + (int)(exp_nl-exp_ptr-sizeof(todo_wine_cmd)), >>> exp_ptr+sizeof(todo_wine_cmd)); >> You may change tests to ok(!err, ....) here and make this else unconditional >> to avoid having ok(TRUE, ...) tests later. > Which else are you talking about? > > The following ok(TRUE,...) serves when there's a todo_wine which has > "succeeded", to get messages like "Test succeeded inside todo block: > match at line XXX" > Without it (i.e. without at least one "true" OK), successful todos > aren't caught, as Dan Kegel pointed out (and I've verified). I was talking about something like this: }else { if(!is_todo_wine) ... else todo_wine ok(!err, ...); } If the tests succeeded, err will be NULL, so this ok will produce "Test succeeded inside todo block" error. Jacek

June 14, 2011

Re: [2/3] cmd/tests: Add support for todo_wine constructions (try 2)

by Frédéric Delanoy

011/6/14 Jacek Caban <jacek(a)codeweavers.com>: > Hi Frédéric, > > On 6/13/11 10:19 PM, Frédéric Delanoy wrote: >> >> + }else if(err) { >> + if (!is_todo_wine) >> + ok(0, "unexpected char 0x%x position %d in line %d (got >> '%.*s', wanted '%.*s')\n", >> + *err, (int)(err-out_ptr), line, (int)(out_nl-out_ptr), >> + out_ptr, (int)(exp_nl-exp_ptr), exp_ptr); >> + else >> + todo_wine >> + ok(0, "unexpected char 0x%x position %d in line %d (got >> '%.*s', wanted '%.*s')\n", >> + *err, (int)(err-out_ptr), line, (int)(out_nl-out_ptr), >> out_ptr, >> + (int)(exp_nl-exp_ptr-sizeof(todo_wine_cmd)), >> exp_ptr+sizeof(todo_wine_cmd)); > > You may change tests to ok(!err, ....) here and make this else unconditional > to avoid having ok(TRUE, ...) tests later. Which else are you talking about? The following ok(TRUE,...) serves when there's a todo_wine which has "succeeded", to get messages like "Test succeeded inside todo block: match at line XXX" Without it (i.e. without at least one "true" OK), successful todos aren't caught, as Dan Kegel pointed out (and I've verified).

June 14, 2011

Re: [2/3] cmd/tests: Add support for todo_wine constructions (try 2)

by Jacek Caban

Hi Frédéric, On 6/13/11 10:19 PM, Frédéric Delanoy wrote: > + }else if(err) { > + if (!is_todo_wine) > + ok(0, "unexpected char 0x%x position %d in line %d (got '%.*s', wanted '%.*s')\n", > + *err, (int)(err-out_ptr), line, (int)(out_nl-out_ptr), > + out_ptr, (int)(exp_nl-exp_ptr), exp_ptr); > + else > + todo_wine > + ok(0, "unexpected char 0x%x position %d in line %d (got '%.*s', wanted '%.*s')\n", > + *err, (int)(err-out_ptr), line, (int)(out_nl-out_ptr), out_ptr, > + (int)(exp_nl-exp_ptr-sizeof(todo_wine_cmd)), exp_ptr+sizeof(todo_wine_cmd)); You may change tests to ok(!err, ....) here and make this else unconditional to avoid having ok(TRUE, ...) tests later. Jacek

June 14, 2011

Re: [4/6] winex11.drv: Grab registered hotkeys in the X server.

by Ken Thomases

On Jun 13, 2011, at 2:07 PM, Vincent Povirk wrote: > <0005-winex11.drv-Grab-registered-hotkeys-in-the-X-server.txt> Is it necessary to test QS_HOTKEY in dlls/winex11.drv/event.c:filter_event, where QS_KEY is currently tested? If somebody calls MsgWaitForMultipleObjectsEx with QS_HOTKEY but not QS_KEY, shouldn't KeyPress events be accepted? > +static void update_modifier_state(int modifier_mask, int actual_mask, > + BYTE *keystate, WORD vkey, WORD alt_vkey, WORD scan, DWORD event_time) > +{ > + int key_pressed = (actual_mask & modifier_mask) != 0; > + int vkey_pressed = (keystate[vkey] & 0x80) == 0x80 || > + (keystate[alt_vkey] & 0x80) == 0x80; > + DWORD flags; > + > + if (key_pressed != vkey_pressed) > + { > + if (key_pressed) > + flags = 0; > + else > + flags = KEYEVENTF_KEYUP; > + TRACE_(key)("Adjusting modifier state vkey=0x%x, state=%i\n", vkey, key_pressed); > + X11DRV_send_keyboard_input( NULL, vkey, scan, flags, event_time ); > + } > +} > + update_modifier_state(MetaMask, event->state, keystate, VK_LMENU, VK_RMENU, 0x38, event_time); > + update_modifier_state(ControlMask, event->state, keystate, VK_LCONTROL, VK_RCONTROL, 0x1D, event_time); > + update_modifier_state(ShiftMask, event->state, keystate, VK_LSHIFT, VK_RSHIFT, 0x2A, event_time); > + update_modifier_state(SuperMask, event->state, keystate, VK_LWIN, VK_RWIN, 0x15b, event_time); You may be able to use the non-left-right-sensitive vkeys (e.g. VK_MENU) here, and thus eliminate the alt_vkey parameter. And if you can't, then update_modifier_state doesn't do the right thing when alt_vkey is the one that's pressed in the keystate but it's no longer pressed in event->state. In that case, you send a key-up for vkey, when you should send it for alt_vkey. If both were down and now neither is, you'll need to send a key-up for both. Note, to send keyboard input for a right-hand modifier, you'll need to include KEYEVENTF_EXTENDEDKEY in the flags and different scan codes. -Ken

June 14, 2011