Re: [1/4] ddraw/tests: Add a test for YV12 partial block locks and lock offsets
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=15324 Your paranoid android. === WINEBUILD (build) === Patch failed to apply
Re: [4/4] ddraw: Set correct HEL and HAL color models
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=15327 Your paranoid android. === WINEBUILD (build) === Patch failed to apply
Re: [3/4] ddraw: Add more tests and fixes for SetSurfaceDesc
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=15326 Your paranoid android. === WINEBUILD (build) === Patch failed to apply
Re: [PATCH 1/5] wined3d: Don't print FIXMEs for misaligned surface maps.
On 8 November 2011 21:33, Stefan Dösinger wrote: > On Tuesday 08 November 2011 20:48:55 Henri Verbeet wrote: >> - FIXME("Partial block lock with %s\n", >> debug_d3dpool(surface->resource.pool)); > I added this fixme because we have to tests for the offset that is returned in > this case. Do you have an application that triggers this fixme, or do you want > to suppress the fixmes in the tests? > Mostly because of the spam this introduces in the tests. Arguably this is something the tests you added should have covered, but I don't think it's a particularly interesting case and what we currently return is probably the only offset that makes some kind of sense anyway.
Re: [PATCH 1/5] wined3d: Don't print FIXMEs for misaligned surface maps.
On Tuesday 08 November 2011 21:33:39 Stefan Dösinger wrote: > I added this fixme because we have to tests for the offset that is returned > in this case. Sorry, typo: I meant "because we have no tests". signature.asc Description: This is a digitally signed message part.
Re: [PATCH 1/5] wined3d: Don't print FIXMEs for misaligned surface maps.
On Tuesday 08 November 2011 20:48:55 Henri Verbeet wrote: > -FIXME("Partial block lock with %s\n", > debug_d3dpool(surface->resource.pool)); I added this fixme because we have to tests for the offset that is returned in this case. Do you have an application that triggers this fixme, or do you want to suppress the fixmes in the tests? signature.asc Description: This is a digitally signed message part.
Re: Adopt a patch?
On Tue, Nov 8, 2011 at 19:41, Michael Ost wrote: > I have a patch for a bug that I won't be able to shepherd through the patch > submission process for some time. The bug is that GetFileVersionInfoSize > doesn't work for files with path names longer than 128 characters. > > The patch includes an untried unit test. And I've confirmed the behavior in > Windows Vista. > > We use an older version of Wine, and my boss won't let me take the time > right now to rework the patch for the newest Wine sources. If someone wants > to they can take this patch, tweak it for the new wine, update it to > wine-patch requirements and push it through the system. > > Otherwise I'll submit as soon as I can. You should probably create a new/update an existing bug in wine bugzilla and attach your patch there. Frédéric
Adopt a patch?
I have a patch for a bug that I won't be able to shepherd through the patch submission process for some time. The bug is that GetFileVersionInfoSize doesn't work for files with path names longer than 128 characters. The patch includes an untried unit test. And I've confirmed the behavior in Windows Vista. We use an older version of Wine, and my boss won't let me take the time right now to rework the patch for the newest Wine sources. If someone wants to they can take this patch, tweak it for the new wine, update it to wine-patch requirements and push it through the system. Otherwise I'll submit as soon as I can. Cheers, Michael Ost --- wine-1.1.7/dlls/kernel32/file.c.RPM 2011-11-02 14:04:34.0 -0700 +++ wine-1.1.7/dlls/kernel32/file.c 2011-11-02 14:06:42.0 -0700 @@ -2476,8 +2476,29 @@ { /* Now look for the file */ -if (!SearchPathA( NULL, name, NULL, sizeof(ofs->szPathName), ofs->szPathName, NULL )) -goto error; +#if 1 // todo + CHAR pathName[MAX_PATH]; + DWORD len; + + len = SearchPathA( NULL, name, NULL, MAX_PATH, pathName, NULL ); + if (len == 0) + goto error; + lstrcpynA(ofs->szPathName, pathName, sizeof(ofs->szPathName)); +// todo: copy as much as you can into ofs ... and confirm this in windows + + TRACE("found %s\n", debugstr_a(pathName) ); + + if (mode & OF_DELETE) + { + if (!DeleteFileA( pathName )) goto error; + TRACE("(%s): OF_DELETE return = OK\n", name); + return TRUE; + } + + handle = LongToHandle(_lopen( pathName, mode )); +#else + if (!SearchPathA( NULL, name, NULL, sizeof(ofs->szPathName), ofs->szPathName, NULL )) + goto error; TRACE("found %s\n", debugstr_a(ofs->szPathName) ); @@ -2489,6 +2510,7 @@ } handle = LongToHandle(_lopen( ofs->szPathName, mode )); +#endif if (handle == INVALID_HANDLE_VALUE) goto error; GetFileTime( handle, NULL, NULL, &filetime ); --- wine-1.1.7/dlls/kernel32/file16.c.RPM 2011-11-02 14:10:28.0 -0700 +++ wine-1.1.7/dlls/kernel32/file16.c 2011-11-02 15:30:13.0 -0700 @@ -122,6 +122,10 @@ FILETIME filetime; WORD filedatetime[2]; const char *p, *filename; +#if 1 // todo + CHAR pathName[MAX_PATH]; + DWORD len; +#endif if (!ofs) return HFILE_ERROR; @@ -194,11 +198,32 @@ if (filename) { -BOOL found; +#if 1 // todo char *path = get_search_path(); if (!path) goto error; -found = SearchPathA( path, filename, NULL, sizeof(ofs->szPathName), + len = SearchPathA( path, filename, NULL, MAX_PATH, pathName, NULL ); + HeapFree( GetProcessHeap(), 0, path ); + if (len == 0) goto error; + lstrcpynA(ofs->szPathName, pathName, sizeof(ofs->szPathName)); + } + + TRACE("found %s\n", debugstr_a(pathName) ); + + if (mode & OF_DELETE) + { + if (!DeleteFileA( pathName )) goto error; + TRACE("(%s): OF_DELETE return = OK\n", name); + return 1; + } + + handle = (HANDLE)_lopen( pathName, mode ); +#else + BOOL found; + char *path = get_search_path(); + + if (!path) goto error; + found = SearchPathA( path, filename, NULL, sizeof(ofs->szPathName), ofs->szPathName, NULL ); HeapFree( GetProcessHeap(), 0, path ); if (!found) goto error; @@ -214,6 +239,7 @@ } handle = (HANDLE)_lopen( ofs->szPathName, mode ); +#endif if (handle == INVALID_HANDLE_VALUE) goto error; GetFileTime( handle, NULL, NULL, &filetime ); --- wine-1.1.7/dlls/version/tests/info.c.RPM 2011-11-08 10:31:24.0 -0800 +++ wine-1.1.7/dlls/version/tests/info.c 2011-11-02 16:17:00.0 -0700 @@ -183,6 +183,19 @@ "Expected ERROR_RESOURCE_DATA_NOT_FOUND, got %d\n", GetLastError()); DeleteFileA("test.txt"); + + static const char* const kVeryLongFileName = "c:\\a very very long and absolute pathname that is longer than the one hundred and twenty-eight character limit of the OFSTRUCT szPathName member"; + create_file(kVeryLongFileName); + SetLastError(0xdeadbeef); + hdl = 0xcafe; + retval = GetFileVersionInfoSizeA(kVeryLongFileName, &hdl); + ok(retval == 0, "Expected 0, got %d\n", retval); + ok(hdl == 0, "Expected 0, got %d\n", hdl); + ok(GetLastError() == ERROR_RESOURCE_DATA_NOT_FOUND || + GetLastError() == ERROR_BAD_FORMAT || /* win9x */ + GetLastError() == ERROR_SUCCESS, /* win2k */ + "Expected ERROR_RESOURCE_DATA_NOT_FOUND, got %d\n", GetLastError()); + DeleteFileA(kVeryLongFileName); } static void VersionDwordLong2String(DWORDLONG Version, LPSTR lpszVerString)
Re: apphelp: add a stub for ApphelpCheckShellObject
Austin English wrote: > +BOOL WINAPI ApphelpCheckShellObject(REFCLSID ObjectCLSID, BOOL > ShimIfNecessary, ULONGLONG *pullFlags) > +{ > +FIXME("stub: %s %p\n", debugstr_guid(ObjectCLSID), pullFlags ); > +return TRUE; > +} It's a common convention in Wine to avoid hungarian notation by all means. In this case naming variables something like 'clsid, shim, flags' woud be better. -- Dmitry.
Re: d3dx9_36 [patch 1/2, resent]: Implement D3DXCreatePolygon
2011/11/7 David Adam : > Hello, > > any problem with this patch > http://source.winehq.org/patches/data/80433 > > and this one > http://source.winehq.org/patches/data/80434 > > Thanks in advance > > David > > -- Forwarded message -- > From: David Adam > Date: 2011/10/30 > Subject: d3dx9_36 [patch 1/2, resent]: Implement D3DXCreatePolygon > To: wine-patches > > > Fix a possible crash when calling D3DXCreateMeshFVF. > > A+ > > David > > > > > Hello David, I see Rico preceded me (his points are very much valid too, and I'm going to overlap a bit), but as I have already written a reply anyway... I think your patches generally look somewhat ugly. Many of the comments I gave in http://www.winehq.org/pipermail/wine-devel/2011-March/089173.html for another patch of yours apply here all the same. About specific issues I have noticed here: Patch 1: +hr = D3DXCreateMeshFVF((DWORD)sides, (DWORD)(sides + 1), D3DXMESH_MANAGED, D3DFVF_XYZ | D3DFVF_NORMAL, device, &polygon); Why those casts? +vertice = HeapAlloc(GetProcessHeap(), 0, 2 * (sides + 1) * sizeof(D3DXVECTOR3)); ... +memcpy(data, vertice, 2 * (sides + 1) * sizeof(D3DXVECTOR3)); What's the point in allocating and filling this additional array? You should just write into the vertex buffer. Same with the index buffer. +(polygon)->lpVtbl->UnlockVertexBuffer(polygon); those parentheses around polygon are useless. Patch 2: +hr = D3DXCreateBuffer(11 * sizeof(DWORD), &ppBuffer); Why are you creating a D3DXBuffer here? It is probably created and returned by D3DXCreatePolygon (and indeed your own implementation in patch 1 does exactly that). If you want to have the cleanup work even when the tests fail, you have to protect the Release() calls somehow (e.g. initializing the object pointers to NULL and checking for != NULL on release). +index = *( (WORD*)( data + ( 3 * i + j ) * sizeof(WORD) ) ); You could make that much simpler by introducing a WORD pointer. Similarly for the vertex data. I guess the objection about expecting a specific vertex/index ordering is moot as it was in D3DXCreateBox case (i.e. there are applications blindly modifying the mesh), right?
Re: [10/11] ddraw/tests: Add assorted D3D3 DrawPrimitive/visual tests.
On Tuesday 08 November 2011 12:08:21 Octavian Voicu wrote: > It gets a bit harder when you have more than a couple of tests, > because you have to follow the sequence of flashes, but the message > from the failures should be descriptive enough. I do that with things like if(i == 5) Sleep(99). If you want to see something you'll have to add a sleep anyway. > >> of d3d, d3d2, d3d3, d3d7, where to have most of the things we store > >> globally or in local vars now, and make the create/release functions > >> work with that. > > > > Yes, I was about to write something like that as a response to the > > cleanup patches. > > Is it OK to leave them like this for now? That kind of refactoring is > going to make this patch series endless. I think its better to have the cleanup done before adding the d3d2/3 tests and infrastructure. You don't have to send all of the patches at once. I usually don't send more than 5 patches a day. This makes it more likely all mails make it to wine-patches and keeps discussions of the patches focused. http://wiki.winehq.org/SubmittingPatches has the general patch submission guidelines. The "Small, clear and atomic changes" part is the trickiest aspect of this. All patches have to compile and pass the tests, otherwise Alexandre doesn't commit them. Single patches should be as small as possible, but make sense on their own. If you split up patches, and patch 1 prepares things for patch 2 and isn't needed otherwise, send them in the same patch series. In your patches you have a few separate things: * Patch 1 makes sense on its own * Patches 2 and 3 clean up variables. Introduction of a structure would fit into those * Patches 4, 5 and 6 fix issues on vmware * 7, 8 add d3d2 tests * 9, 10 add d3d3 tests * 11 adds a d3d7 test The separation into small patches looks good. You can send more than one of those patch blocks at once, but you don't have to. Don't be afraid to have a bigger set of unsent patches in your tree, git makes it fairly easy to work with them. For editing patches, git rebase -- interactive is very helpful. Branches can be very useful for bigger features or fixes you're working on. There are also other tools like stgit that are worth a look. For example, I currently have 7 unsent patches in my tree that are ready to go. I have 3 branches with features that I've put on ice until after Wine 1.4 and 4 tests and bugfixes that need some more work before I'll send them(They currently live as test patches on my git tree on the Windows partition). I also have a branch with hacks for personal use that aren't of any use for others and will never make it into Wine. signature.asc Description: This is a digitally signed message part.
Re: [01/11] ddraw/tests: Fix a couple of copy-paste typos in test messages.
This patch is OK, the others need some discussion and work. signature.asc Description: This is a digitally signed message part.
Re: [3/3] ddraw/tests: Add a partial block lock test
Stefan Dösinger writes: > On Tuesday 08 November 2011 13:45:20 Alexandre Julliard wrote: >> Adding a range of years in a copyright notice is usually discouraged, >> because it's only valid if you have actually made significant changes >> every year, which doesn't seem to be the case here. > What's the preferred way? Add separate lines for each year, or ranges? Enumerate the years, like "Copyright 2007, 2009, 2011". -- Alexandre Julliard julli...@winehq.org
Re: [PATCH 3/7] ntoskrnl: intialze unknown fields of the irp with 0xce in IoInitializeIrp
Bernhard Loos writes: > @@ -864,6 +873,10 @@ VOID WINAPI IoCompleteRequest( IRP *irp, UCHAR > priority_boost ) > > iosb = irp->UserIosb; > status = irp->IoStatus.u.Status; > + > +assert( status != STATUS_PENDING ); > +assert( status != 0xdeafbeef ); > + You can't assert on things that are set by the caller. -- Alexandre Julliard julli...@winehq.org
Re: [3/3] ddraw/tests: Add a partial block lock test
On Tuesday 08 November 2011 13:45:20 Alexandre Julliard wrote: > Adding a range of years in a copyright notice is usually discouraged, > because it's only valid if you have actually made significant changes > every year, which doesn't seem to be the case here. What's the preferred way? Add separate lines for each year, or ranges? signature.asc Description: This is a digitally signed message part.
Re: kernel32/tests: add tests for GetDllDirectory (try 2)
Thomas Faber writes: > +/* no buffer, determine length */ > +SetLastError(0xdeadbeef); > +ret = pGetDllDirectoryA(0, NULL); > +error = GetLastError(); > +ok(ret == length + 1, "Expected %u, got %u\n", length + 1, ret); > +ok(error == 0xdeadbeef, "Error is %u\n", error); Testing last error on success is generally not useful. -- Alexandre Julliard julli...@winehq.org
Re: [3/3] ddraw/tests: Add a partial block lock test
Stefan Dösinger writes: > @@ -4,7 +4,7 @@ > * Copyright (C) 2005 Antoine Chavasse (a.chava...@gmail.com) > * Copyright (C) 2005 Christian Costa > * Copyright 2005 Ivan Leo Puoti > - * Copyright (C) 2007 Stefan Dösinger > + * Copyright (C) 2007-2011 Stefan Dösinger for CodeWeavers Adding a range of years in a copyright notice is usually discouraged, because it's only valid if you have actually made significant changes every year, which doesn't seem to be the case here. -- Alexandre Julliard julli...@winehq.org
Re: Fwd: d3dx9_36 [patch 1/2, resent]: Implement D3DXCreatePolygon
Hi David, Impl: - Probably you may unlock and free the polygon when allocating the vertice/index fail, why do you need the vertice at all? Couldn't you just write directly to "data", since you're holding the lock anyway? - if locking fails, you're leaking the polygon memory, this happens for nearly all error checks - the TRACE shouldn't contain "stub" - there are some style issues Test: - it may be nice to test if polygon or ppBuffer are set to NULL on hr != D3D_OK or if they are untouched - Why are you creating a buffer ppBuffer at all? From your implementation it looks like it is done when using adjacency? Also I'd expect, when D3DXCreateBuffer fails, ppBuffer is NULL and the release at the end label would crash, even if it still has uninitialized memory. - there is an unlock for the vertexbuffer missing and you may check the return value from the unlock call - there are some copy and paste mistakes - I think a failing Lock*Buffer should also get a failing test, I wouldn't just print the skip. - I'd check the count from all Release calls, that way, you'd see if you forgot to release something Hope this helps Rico Am 07.11.2011 20:58, schrieb David Adam: Hello, any problem with this patch http://source.winehq.org/patches/data/80433 and this one http://source.winehq.org/patches/data/80434 Thanks in advance David -- Forwarded message -- From: David Adam Date: 2011/10/30 Subject: d3dx9_36 [patch 1/2, resent]: Implement D3DXCreatePolygon To: wine-patches Fix a possible crash when calling D3DXCreateMeshFVF. A+ David
Re: [10/11] ddraw/tests: Add assorted D3D3 DrawPrimitive/visual tests.
On Tue, Nov 8, 2011 at 12:18 PM, Stefan Dösinger wrote: > On Tuesday 08 November 2011 01:14:22 Octavian Voicu wrote: >> The idea behind using triangles was to fit everything in one screen so >> I could debug it visually very easily. I'm thinking I can still have >> that if I draw full-screen quads in a loop, like you suggest, then >> blit one small rectangle to the primary surface, in different >> positions for each test. > I don't think that's necessary, I found those one-quad screens easier to debug > than multi-quads, although that may be a matter of taste. It gets a bit harder when you have more than a couple of tests, because you have to follow the sequence of flashes, but the message from the failures should be descriptive enough. >> I think >> we should cover special error cases if there are any (that is, if >> D3DERR_INVALIDVERTEXTYPE is not returned for D3DVT_ constants). > Well, you can't test them. D3DVT_LVERTEX is 2, and D3DFVF_XYZ is 2. The number > 2 doesn't have any tags attached, so what you'd be testing is how > drawPrimitive responds to D3DFVF_RESERVED0, not what it does with D3DVT_* > enums. > > The statement in the SDK doesn't make sense with that in mind. I thought D3DVT_ values interpreted as FVFs didn't make sense. It seems Windows accepts those values and returns D3D_OK, so I'll drop those tests. > > About the ddraw/visual tests: I think we need one structure for each >> of d3d, d3d2, d3d3, d3d7, where to have most of the things we store >> globally or in local vars now, and make the create/release functions >> work with that. > Yes, I was about to write something like that as a response to the cleanup > patches. Is it OK to leave them like this for now? That kind of refactoring is going to make this patch series endless. Octavian
Re: [10/11] ddraw/tests: Add assorted D3D3 DrawPrimitive/visual tests.
[resent to include wine-devel] On Tue, Nov 8, 2011 at 1:23 AM, Stefan Dösinger wrote: > Design-wise I think the 8 triangles and readback grid make the test fairly > difficult to read. You could set up a structure that contains the d3d > parameters > you're changing between the tests and the expected color and then loop over > the table. Something like this: > > const struct > { > BOOL d3drs_lighting; > DWORD dp_flags; <- for things like D3DP_DONOTLIGHT > DWORD fvf; > void *vertices; > DWORD expected_color; > } > test_data[] = > { > ... > }; > > You can also think about sharing this control table between d3d2, d3d3 and > d3d7 by having different color fields for that, but I don't know how well this > will work. Henry wasn't very happy about this either. Your fix sounds interesting, I'm going to give it a try. The idea behind using triangles was to fit everything in one screen so I could debug it visually very easily. I'm thinking I can still have that if I draw full-screen quads in a loop, like you suggest, then blit one small rectangle to the primary surface, in different positions for each test. > On Monday 07 November 2011 23:27:13 Octavian Voicu wrote: >> + DWORD expected[] = { GRAY, WHITE, YELLOW2, YELLOW, GREEN, GREEN, BLUE, > BLUE }; > Please make data const where you can. Point taken. >> + /* Note: IDirect3DDevice3::DrawPrimitive calls with D3DVT_ vertex >> types should fail. */ + >> + /* Triangle 0 -- D3DFVF_VERTEX (vertices with normals, but without >> colors). */ + hr = IDirect3DDevice3_DrawPrimitive(Direct3DDevice3, >> D3DPT_TRIANGLELIST, D3DVT_VERTEX, >> + vertices, 3, D3DDP_WAIT); >> + ok(hr != DDERR_INVALIDPARAMS, "DrawPrimitive returned: %08x\n", >> hr); > I don't think there's a point in testing this, both D3DVT_* and D3DFVF_* are > defines / enums that map to integers, and it is obvious that you get wrong > behavior if you mix those. > > The ok condition looks strange. If you're trying to avoid a todo_wine, don't > do that, just mark tests that show a difference between Wine and Windows as > todo_wine. > (I guess Wine currently returns DD_OK, and Windows returns > D3DERR_INVALIDVERTEXFORMAT) DirectX SDK 6.1 docs says that "If you attempt to use one of the members of D3DVERTEXTYPE, the method fails, returning DDERR_INVALIDPARAMS", so I wanted to test exactly that. I accidentally wrote "!=" instead of "==" and forgot about it since Windows did not complain (SDK is probably inaccurate), nor did Wine. Will look into it and probably write some separate tests, outside the test loop. I think we should cover special error cases if there are any (that is, if D3DERR_INVALIDVERTEXTYPE is not returned for D3DVT_ constants). >> + /* Make sure getPixelColor works correctly. */ >> + color = getPixelColor(device, 320, 240); >> + ok(color == RED, "getPixelColor returned: %08x\n", color); > We have some sanity checks for that in the main function I know that, but I figured it didn't hurt to have an extra check there, since each D3D2, D3D3, and D3D7 test uses a different version of the getPixelColor function. > Most of this applies to the d3d2 and d3d3 tests as well. I haven't looked over > the cleanup patches yet. Thanks for the quick and thorough feedback, it was really helpful! About the ddraw/visual tests: I think we need one structure for each of d3d, d3d2, d3d3, d3d7, where to have most of the things we store globally or in local vars now, and make the create/release functions work with that. With my cleanup patches I think it will be easier to refactor code to use such a structure. However, my patch stack was getting uncomfortably large and I wanted to commit it, so I can get to the good part -- fixing the game I'm debugging. I might look into such a refactoring later. Octavian
Re: [10/11] ddraw/tests: Add assorted D3D3 DrawPrimitive/visual tests.
On Tuesday 08 November 2011 01:14:22 Octavian Voicu wrote: > The idea behind using triangles was to fit everything in one screen so > I could debug it visually very easily. I'm thinking I can still have > that if I draw full-screen quads in a loop, like you suggest, then > blit one small rectangle to the primary surface, in different > positions for each test. I don't think that's necessary, I found those one-quad screens easier to debug than multi-quads, although that may be a matter of taste. > I think > we should cover special error cases if there are any (that is, if > D3DERR_INVALIDVERTEXTYPE is not returned for D3DVT_ constants). Well, you can't test them. D3DVT_LVERTEX is 2, and D3DFVF_XYZ is 2. The number 2 doesn't have any tags attached, so what you'd be testing is how drawPrimitive responds to D3DFVF_RESERVED0, not what it does with D3DVT_* enums. The statement in the SDK doesn't make sense with that in mind. > About the ddraw/visual tests: I think we need one structure for each > of d3d, d3d2, d3d3, d3d7, where to have most of the things we store > globally or in local vars now, and make the create/release functions > work with that. Yes, I was about to write something like that as a response to the cleanup patches. PS: You forgot to CC wine-devel, I assume this was by accident so I've added it to the reply again. You may want to resend your original mail to wine- devel. signature.asc Description: This is a digitally signed message part.