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

[Henri Verbeet]

On 20 March 2016 at 19:24, Aaryaman Vasishta <jem456.vasishta at gmail.com> wrote:
> -    FIXME("iface %p, filename %s, texture %p stub!\n", iface, debugstr_a(filename), texture);
> +    TRACE("iface %p, filename %s, texture %p\n", iface, debugstr_a(filename), texture);
Missing period.

> @@ -853,15 +859,19 @@ static HRESULT WINAPI d3drm2_CreateUserVisual(IDirect3DRM2 *iface,
>  static HRESULT WINAPI d3drm2_LoadTexture(IDirect3DRM2 *iface,
>          const char *filename, IDirect3DRMTexture2 **texture)
>  {
> -    struct d3drm_texture *object;
> +    struct d3drm *d3drm = impl_from_IDirect3DRM2(iface);
> +    IDirect3DRMTexture3 *texture3;
>      HRESULT hr;
>
> -    FIXME("iface %p, filename %s, texture %p stub!\n", iface, debugstr_a(filename), texture);
> +    TRACE("iface %p, filename %s, texture %p.\n", iface, debugstr_a(filename), texture);
>
> -    if (FAILED(hr = d3drm_texture_create(&object)))
> +    if (FAILED(hr = IDirect3DRM3_LoadTexture(&d3drm->IDirect3DRM3_iface, filename, &texture3)))
>          return hr;
>
> -    *texture = &object->IDirect3DRMTexture2_iface;
> +    hr = IDirect3DRMTexture3_QueryInterface(texture3, &IID_IDirect3DRMTexture2, (void**)texture);
> +    IDirect3DRMTexture3_Release(texture3);
> +    if (FAILED(hr))
> +        return hr;
>
>      return D3DRM_OK;
>  }
This is probably ok, but you may want to consider just introducing a
helper function instead. I.e.:

    if (FAILED(hr = d3drm_load_texture(d3drm, filename, TRUE, &object)))
        return hr;

    IDirect3DRMTexture2_AddRef(*texture = &object->IDirect3DRMTexture2_iface);

    return D3DRM_OK;

What happens when d3drm_texture_load() is called multiple times on the
same texture? Should that be possible?

> +void d3drm_texture_destroy(struct d3drm_texture *texture);
Missing DECLSPEC_HIDDEN.

> +HRESULT d3drm_texture_load(struct d3drm_texture *texture, IDirect3DRM *d3drm, const char *path, BOOL load_upside_down) DECLSPEC_HIDDEN;
This line is a bit long. Try to stick to 120 columns as a maximum,
preferably a bit less.

> +D3DRMIMAGE *d3drm_create_image(unsigned char *buffer, BITMAPINFO *info, BOOL palette, BOOL upside_down);
Missing DECLSPEC_HIDDEN.

> @@ -4103,24 +4105,34 @@ static void test_load_texture(void)
>
>          hr = IDirect3DRM_LoadTexture(d3drm1, filename, &texture1);
>          ok(SUCCEEDED(hr), "Test %u: Failed to load texture, hr %#x.\n", i, hr);
> +        ref2 = get_refcount((IUnknown *)d3drm1);
> +        ok(ref2 > ref1, "expected ref2 > ref1, got ref1 = %u, ref2 = %u.\n", ref1, ref2);
The message should mention which test (i.e., "i") failed.

>          d3drm_img = IDirect3DRMTexture_GetImage(texture1);
> -        todo_wine ok(!!d3drm_img, "Test %u: Failed to get image.\n", i);
> +        ok(!!d3drm_img, "Test %u: Failed to get image.\n", i);
>          if (d3drm_img)
>              test_bitmap_data(i * 4, d3drm_img, FALSE, tests[i].w, tests[i].h, tests[i].palettized);
Now that IDirect3DRMTexture_GetImage() succeeds, you can remove the
"if (d3drm_img)".

> +void d3drm_texture_destroy(struct d3drm_texture *texture)
> +{
> +    if (texture->image)
> +    {
> +        TRACE("Releasing attached members.\n");
> +        HeapFree(GetProcessHeap(), 0, texture->image->buffer1);
> +        if(texture->image->palette)
Missing space between "if" and "(".
> +            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.

> +HRESULT d3drm_texture_load(struct d3drm_texture *texture, IDirect3DRM *d3drm, const char *path, BOOL load_upside_down)
> +{
> +    BITMAPINFO *info;
> +    BITMAPFILEHEADER *bmp_header;
> +    unsigned char *buffer;
> +    DWORD size;
> +    HANDLE hfile, hmapping;
> +
> +    hfile = CreateFileA(path, GENERIC_READ, FILE_SHARE_READ, 0, OPEN_EXISTING, 0, 0);
> +    if (hfile == INVALID_HANDLE_VALUE)
> +        return D3DRMERR_BADFILE;
> +
> +    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().

> +    buffer = MapViewOfFile(hmapping, FILE_MAP_READ, 0, 0, 0);
> +    if (buffer == NULL)
"if (!buffer)", or even "if (!(buffer = MapViewOfFile(...)))"

> +    bmp_header = (BITMAPFILEHEADER *)buffer;
> +    info = (BITMAPINFO *) (buffer + sizeof(*bmp_header));
Or just "info = (BITMAPINFO *)bmp_header + 1;". You should validate
that this is actually a BMP file. You should also check which version
of the BITMAPINFOHEADER structure the file has.

> +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?

> +    D3DRMIMAGE *image;
> +    BOOL black_used = FALSE;
> +    LONG w = info->bmiHeader.biWidth;
> +    LONG h = abs(info->bmiHeader.biHeight);
> +    int i, j, k;
> +    unsigned int idx, buffer1_idx;
> +    unsigned char *buffer1;
> +    int bpp = info->bmiHeader.biBitCount == 24 ? 32 : info->bmiHeader.biBitCount;
> +    int bpl = palette ? w : ((w + 3) & ~3) * bpp / 8;
> +    int num_colors = 0;
> +    struct colors_24bpp
> +    {
> +        BYTE red;
> +        BYTE green;
> +        BYTE blue;
> +    } *color;
> +    color = (struct colors_24bpp *)buffer;
> +
> +    image = (D3DRMIMAGE *)HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*image));
> +    image->buffer1 = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(unsigned char) * bpl * h);
This is unsafe. Consider e.g. what happens when the header gives you
0x10000 for width and height.

> +    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.