I'm looking into a problem that causes Supreme Commander to segfault (Bug #7643 http://bugs.winehq.org/show_bug.cgi?id=7643) and I believe I may have run into a problem with IWineD3DSurfaceImpl_LockRect. I believe the problem is a typo in the line:
This->resource.allocatedMemory = HeapAlloc(GetProcessHeap() ,0 , This-> resource.size + 4);
I think that the allocated memory should be the size of the resource multiplied by 4 (instead of adding 4). With the current allocation, for some textures, the operation to calculate the pBits location returns a memory address that is past the end of the allocated memory. So, after this operation:
pLockedRect->pBits = This->resource.allocatedMemory + (pLockedRect->Pitch
- pRect->top) + (pRect->left * This->bytesPerPixel)
the value of pLockedRect->pBits is greater than "This-> resource.allocatedMemory + This->resource.size + 4".
I've dug around a bit to see if This->resource.size was just allocated incorrectly but that does not appear the case. So, if someone could confirm or deny that this is a typo problem then I'd really appreciate it.
Erich Hoover ehoover@mines.edu
On 12/03/07, Erich Hoover ehoover@mines.edu wrote:
I think that the allocated memory should be the size of the resource multiplied by 4 (instead of adding 4).
What makes you think that?
IIRC the +4 there is for dodgy applications reading just past the end of the surface, but that line could do with a comment in either case.
I've dug around a bit to see if This->resource.size was just allocated incorrectly but that does not appear the case. So, if someone could confirm or deny that this is a typo problem then I'd really appreciate it.
Could you look into my comment in bugzilla? I'll probably write a test for that either way, but it's probably quite a bit faster for you to check if what I posted there makes any difference.
On 3/12/07, H. Verbeet hverbeet@gmail.com wrote:
On 12/03/07, Erich Hoover ehoover@mines.edu wrote:
I think that the allocated memory should be the size of the resource multiplied by 4 (instead of adding 4).
What makes you think that?
Just that (pLockedRect->Pitch * pRect->top) with pRect->top = This-> currentDesc.Height would return a value 4x that of This->resource.size.
IIRC the +4 there is for dodgy applications reading just past the end
of the surface, but that line could do with a comment in either case.
I've dug around a bit to see if This->resource.size was just allocated incorrectly but that does not appear the case. So, if someone could
confirm
or deny that this is a typo problem then I'd really appreciate it.
Could you look into my comment in bugzilla? I'll probably write a test for that either way, but it's probably quite a bit faster for you to check if what I posted there makes any difference.
Dividing pRect->top by 4 seems to solve the problem. Allocating 4 times as much memory also solves the problem (no apparent visual difference), but your explanation in bugzilla makes sense. Will you be taking care of this, or should I write up a patch with the divide by 4 and a bounds check?
On 12/03/07, Erich Hoover ehoover@mines.edu wrote:
your explanation in bugzilla makes sense. Will you be taking care of this, or should I write up a patch with the divide by 4 and a bounds check?
If you're willing to write a patch for this, sure. The patch should apply to DXT2-DXT5, and the DXT1 case will probably need to be changed as well.
Am Montag 12 März 2007 06:04 schrieb Erich Hoover:
I'm looking into a problem that causes Supreme Commander to segfault (Bug #7643 http://bugs.winehq.org/show_bug.cgi?id=7643) and I believe I may have run into a problem with IWineD3DSurfaceImpl_LockRect. I believe the
problem is a typo in the line:
This->resource.allocatedMemory = HeapAlloc(GetProcessHeap() ,0 , This-> resource.size + 4);
I think that the allocated memory should be the size of the resource multiplied by 4 (instead of adding 4). With the current allocation, for some textures, the operation to calculate the pBits location returns a memory address that is past the end of the allocated memory. So, after this
No, it should definitly be + 4. This->resource.size is supposed to contain the full surface. The + 4 is because old applications use DWORD-Aligned access to palettized textures and write up to 3 bytes beyond the surface.
So either the size calculation is broken, or something in LockRect
-
Erich Hoover -
H. Verbeet -
Stefan Dösinger