On Wed, Mar 23, 2016 at 6:50 PM, Henri Verbeet <hverbeet@gmail.com> wrote:
What happens when d3drm_texture_load() is called multiple times on the
same texture? Should that be possible?
On windows, IDirect3DRMTexture*::Load creates a new texture every time you call it on the same texture pointer, effectively losing the old one and creating a leak. The refcount also increases by one every time a new texture is created. So if windows allows this, I guess it's fine. Do let me know if I've missed testing something here though.
 
> +            HeapFree(GetProcessHeap(), 0, texture->image->palette);
> +        HeapFree(GetProcessHeap(), 0, texture->image);
This seems a bit suspicious. Supposedly the "image" argument to
d3drm3_CreateTexture() can be used to create a texture on an existing
image. The documentation isn't very explicit about it, but suggests
that the image data isn't copied in that case. So I'm not sure the
texture is supposed to be responsible for destroying the image. In
general I think it's not entirely clear how things like
CreateTexture(), LoadTexture() and InitFromFile() etc. are supposed to
interact.
AFAICS on the trace logs of d3drm games ran so far, I haven't encountered CreateTexture spefically. Though you can find usage of CreateTexture in the "Trans" demo from the DX7 SDK, within trans.cpp. After a quick glance I can confirm that it does indeed create a texture on an existing image in memory. The implementation for this should be fairly trivial, but some tests would be needed to check if it does a shallow/deep copy of the image struct (as you mentioned), validation checks, if any, and its refcounting behavior.

I'm not sure about the Init methods, but so far I haven't been able to see them being used anywhere.

I've initially decided to prioritize on functions that are actually used by games rather than implementing functions that are probably not used at all in the d3drm apps that ran on wine so far. But I don't mind doing them if they're required somehow, or if a bug reports it.

> +    size = GetFileSize(hfile, NULL);
> +    if (size == INVALID_FILE_SIZE)
> +    {
> +        DeleteObject(hfile);
> +        return D3DRMERR_BADVALUE;
> +    }
You never use "size" after this, but you probably should. I also think
you meant CloseHandle() instead of DeleteObject().
Right, I could use it to check if the size matches the size specifed in the header field bfSize within BITMAPFILEHEADER. Or maybe there's another use than this?

> +D3DRMIMAGE *d3drm_create_image(unsigned char *buffer, BITMAPINFO *info, BOOL palette, BOOL upside_down)
> +{
> +    D3DRMPALETTEENTRY *colors = (D3DRMPALETTEENTRY *)HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, 257 * sizeof(*colors));
Is "257" there intentional? The cast is unnecessary. HeapAlloc() can
fail. Do you need HEAP_ZERO_MEMORY here? 
I kept it as 257 so that accessing colors[256] shouldn't segfault. The approach I used here is to check if the total number of unique colors is greater than 256, which happens when the 257th unique color is also stored into the array.

I guess statically allocating it on the stack i.e declaring D3DRMPALETTEENTRY colors[257]; could possibly help avoid the HeapAlloc failures. Makes sense since the 257 is hard-coded anyways. Should I do that? Or is the HeapAlloc a better approach?

As for HEAP_ZERO_MEMORY, see below.
This is unsafe. Consider e.g. what happens when the header gives you
0x10000 for width and height.
You're right. d3drm returns D3DRMERR_BADVALUE for textures with width and height that large. I'll add a test accordingly.
I'll also be adding validation checks for bitmap format from the header, as you mentioned.

> +    buffer1 = image->buffer1;
> +    memset(buffer1, 255, sizeof(unsigned char) * bpl * h);
You just initialised the memory with HEAP_ZERO_MEMORY, and now you're
overwriting it again. At least one of these two initialisations is
unnecessary, and quite possibly both of them.

I actually intended to initialize it to 0xFF rather than zero. But you're right about the alloc's being unnecessary. I'll remove both initializations, and keep the default initialization to 0xFF as the byte padding in 32bpp bitmaps are all 0xFF.

I also agree with the rest of the changes and will submit another patch very soon.

Thanks for the detailed review! Really appreciate it.

Cheers,
Aaryaman