[PATCH 2/2] d3drm: Partially Implement IDirect3DRM*::LoadTexture(v2).

[Aaryaman Vasishta]

On Wed, Mar 23, 2016 at 6:50 PM, Henri Verbeet <hverbeet at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20160324/8ae86333/attachment.html>