On 20 March 2016 at 19:24, Aaryaman Vasishta jem456.vasishta@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.