Hi,
This mail is mainly addressed at Henri and Roderick, but I'll send it here to allow others to read it too.
These patches contain some cleanups of the d3d surface loading code. It's main aim is to put the code that copies the surface between system memory, texture and drawable into a centralized place, make LockRect simpler and pbo creation and surface memory allocation in one place(surface allocation is not completely there yet). It also makes other parts of the code simpler, avoids playing with the surface flags in other places, and it will allow us to centralize the logic that in the case of fbo offscreen rendering the drawable is the same as the texture(This is also not implemented yet).
The patches don't aim at fixing any bugs themselves, and I hardly tested them, so there may be a truckload of regressions. I'm mainly showing them to show the general direction I'm heading into.
Am Sonntag, 23. September 2007 00:14:06 schrieb Stefan Dösinger:
Hi,
This mail is mainly addressed at Henri and Roderick, but I'll send it here to allow others to read it too.
These patches contain some cleanups of the d3d surface loading code. It's main aim is to put the code that copies the surface between system memory, texture and drawable into a centralized place, make LockRect simpler and pbo creation and surface memory allocation in one place(surface allocation is not completely there yet). It also makes other parts of the code simpler, avoids playing with the surface flags in other places, and it will allow us to centralize the logic that in the case of fbo offscreen rendering the drawable is the same as the texture(This is also not implemented yet).
The patches don't aim at fixing any bugs themselves, and I hardly tested them, so there may be a truckload of regressions. I'm mainly showing them to show the general direction I'm heading into.
Ah, and the cleanup needs a cleanup itself too, e.g. RequestLocation should move most code into subfunctions.
On 23/09/2007, Stefan Dösinger stefandoesinger@gmx.at wrote:
/* DXTn textures are based on compressed blocks of 4x4 pixels, each* 16 bytes large (8 bytes in case of DXT1). Because of that Pitch has* slightly different meaning compared to regular textures. For DXTn* textures Pitch is the size of a row of blocks, 4 high and "width"* long. The x offset is calculated differently as well, since moving 4* pixels to the right actually moves an entire 4x4 block to right, ie* 16 bytes (8 in case of DXT1). */if (This->resource.format == WINED3DFMT_DXT1) {pLockedRect->pBits = This->resource.allocatedMemory + (pLockedRect->Pitch * pRect->top / 4) + (pRect->left * 2);} else if (This->resource.format == WINED3DFMT_DXT2 || This->resource.format == WINED3DFMT_DXT3|| This->resource.format == WINED3DFMT_DXT4 || This->resource.format == WINED3DFMT_DXT5) {pLockedRect->pBits = This->resource.allocatedMemory + (pLockedRect->Pitch * pRect->top / 4) + (pRect->left * 4);} else {pLockedRect->pBits = This->resource.allocatedMemory + (pLockedRect->Pitch * pRect->top) + (pRect->left * This->bytesPerPixel);}
Where did that part go?
Am Sonntag, 23. September 2007 00:14:06 schrieb Stefan Dösinger: Ah, and the cleanup needs a cleanup itself too, e.g. RequestLocation should move most code into subfunctions.
Yes.
The patches don't aim at fixing any bugs themselves, and I hardly tested them, so there may be a truckload of regressions. I'm mainly showing them to show the general direction I'm heading into.
The general direction of the patches seems ok, although I'm not sure if RequestLocation / ModifyLocation is optimal from a naming point of view. (Ie, RequestLocation typically moves data rather than returning a location for the caller to read/write to, while ModifyLocation just flips a flag rather than modifying the location of the actual data)
OT(?): Wrt. regressions, I would like to mention that I think that in general more care should be taken to verify a patch is correct when *writing* the patch. I do sometimes get the feeling that the general approach taken is one of "Throw some patches at it, see what sticks" :-/
Am Sonntag, 23. September 2007 01:29:32 schrieb H. Verbeet:
The general direction of the patches seems ok, although I'm not sure if RequestLocation / ModifyLocation is optimal from a naming point of view. (Ie, RequestLocation typically moves data rather than returning a location for the caller to read/write to, while ModifyLocation just flips a flag rather than modifying the location of the actual data)
Does LoadLocation sound better? I'm open to suggestions :-)
OT(?): Wrt. regressions, I would like to mention that I think that in general more care should be taken to verify a patch is correct when *writing* the patch. I do sometimes get the feeling that the general approach taken is one of "Throw some patches at it, see what sticks"
I admit that sometimes I am toying with the "see what sticks" approach, especially if I do not have access to the application the issue is about(e.g. Pirates! with the alpha fix in unlockrect). Otherwise I'm doing my best to make sure that the patch is correct when writing it, and often I write a regression test for it too. However, some areas are a bit too complex to proove that a patch is correct. Like in the surface code, where we have the interactions of FBOs, PBOs, apple_client_storage, various known driver bugs, surface conversion, performance considerations, etc(*). It's plainly impossible to make sure there are no regressions. So my "there will be tons of regressions" note was meant as a "I think the patches are correct, but I feel uncomfortable to send them in without testing" rather than "I have no idea if that works".
FWIW, I have found only one problem so far, an issue in Battlefield 1942, which interestingly shows up in my Vista installation too. Usually our continuously growing test suit does a good job in catching most problems.
(*): Before someone(ohsix ;-) ) suggests to use DDI with its perfectly defined resource management: Feel free to suggest a redesign that abstracts all this(including the driver bugs), I'd be thankful for one. But please describe it in your own words, a link to the DDI docs doesn't count(there are license issues too, propably).
Hi,
This mail is mainly addressed at Henri and Roderick, but I'll send it here to allow others to read it too.
These patches contain some cleanups of the d3d surface loading code. It's main aim is to put the code that copies the surface between system memory, texture and drawable into a centralized place, make LockRect simpler and pbo creation and surface memory allocation in one place(surface allocation is not completely there yet). It also makes other parts of the code simpler, avoids playing with the surface flags in other places, and it will allow us to centralize the logic that in the case of fbo offscreen rendering the drawable is the same as the texture(This is also not implemented yet).
The patches don't aim at fixing any bugs themselves, and I hardly tested them, so there may be a truckload of regressions. I'm mainly showing them to show the general direction I'm heading into.
I have taken a quick look at it. I think it looks ok (I don't fully understand all changes though but that's because I didn't know the old code that well yet).
What I have been wondering why remove flush_to_framebuffer_texture? In ddraw opengl it is a very useful mode. From what I have seen on modern cards textures seem to outperform drawpixels.
Roderick
Am Sonntag, 23. September 2007 00:34:56 schrieb Roderick Colenbrander:
Hi,
This mail is mainly addressed at Henri and Roderick, but I'll send it here to allow others to read it too.
These patches contain some cleanups of the d3d surface loading code. It's main aim is to put the code that copies the surface between system memory, texture and drawable into a centralized place, make LockRect simpler and pbo creation and surface memory allocation in one place(surface allocation is not completely there yet). It also makes other parts of the code simpler, avoids playing with the surface flags in other places, and it will allow us to centralize the logic that in the case of fbo offscreen rendering the drawable is the same as the texture(This is also not implemented yet).
The patches don't aim at fixing any bugs themselves, and I hardly tested them, so there may be a truckload of regressions. I'm mainly showing them to show the general direction I'm heading into.
I have taken a quick look at it. I think it looks ok (I don't fully understand all changes though but that's because I didn't know the old code that well yet).
What I have been wondering why remove flush_to_framebuffer_texture? In ddraw opengl it is a very useful mode. From what I have seen on modern cards textures seem to outperform drawpixels.
Ah, sorry for not explaining this. It is temporarily removed to make the move easier. There is a comment in LockRect explaining how two RequestLocation calls can be used to get the same thing flush_to_framebuffer_texture does, but this requires the last of the 6 copy types(texture->drawable) to be implemented, either using the code from drawprim.c, blt_to_drawable or the flush_to_framebuffer_texture code.
-
H. Verbeet -
Mirek Slugeň -
Roderick Colenbrander -
Stefan Dösinger