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

[Henri Verbeet]

On 23 March 2016 at 20:05, Aaryaman Vasishta <jem456.vasishta at gmail.com> wrote:
> 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.
>
So the image can't be replaced after it has been set.

> 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.
>
Sure, but it's useful to have some idea what those functions are
supposed to do. Right now, I think it looks like you should be able to
implement LoadTexture() by creating a texture and then calling
InitFromFile() on it. And related to above, InitFromFile() should then
probably check that the texture hasn't been initialised before.

> 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?
>
Mostly to make sure you don't read past the end of the buffer.

>> > +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.
>
It's probably better to just avoid writing it.

> 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?
>
HeapAlloc() is fine, you just need to handle the failure.