Re: [PATCH] d3drm: Partially Implement IDirect3DRM*::LoadTexture(v3).

[Henri Verbeet]

On 28 March 2016 at 20:02, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote:
> v3: Add bitmap verification, use InitFromFile and other minor improvements.

Implementing InitFromFile() should be a separate patch from
implementing LoadTexture().
> if (FAILED(hr = d3drm_texture_create(&object)))
>          return hr;
>
> +    if (FAILED(hr = IDirect3DRMTexture_InitFromFile(&object->IDirect3DRMTexture_iface, filename)))
> +    {
> +        d3drm_texture_destroy(object);
> +        return hr;
> +    }
> +
> +    object->d3drm = iface;
> +    IDirect3DRM_AddRef(iface);

It would be good to back this up with tests. It would seem more
natural to pass the IDirect3DRM interface to d3drm_texture_create().
If that's how it's supposed to work, you can make that change in a
separate patch.
> @@ -39,6 +40,9 @@ struct d3drm_texture
>      IDirect3DRMTexture IDirect3DRMTexture_iface;
>      IDirect3DRMTexture2 IDirect3DRMTexture2_iface;
>      IDirect3DRMTexture3 IDirect3DRMTexture3_iface;
> +    IDirect3DRM *d3drm;
> +    D3DRMIMAGE *image;
> +    BOOL internal_image_created;
> You don't really need internal_image_created, you can probably just
> add a destroy callback.
> +HRESULT d3drm_texture_load(struct d3drm_texture *texture, IDirect3DRM *d3drm, const char *path,
> +            BOOL load_upside_down) DECLSPEC_HIDDEN;
> +
> +D3DRMIMAGE *d3drm_create_image(unsigned char *buffer, BITMAPINFO *info, BOOL palette, BOOL upside_down) DECLSPEC_HIDDEN;
> These are internal helper functions now.
> @@ -4109,36 +4111,94 @@ 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, "Test %u: expected ref2 > ref1, got ref1 = %u, ref2 = %u.\n", i, ref1, ref2);
> +
> +        hr = IDirect3DRMTexture_InitFromFile(texture1, filename);
> +        ok(hr == D3DRMERR_BADOBJECT, "Test %u: Expected hr == D3DRMERR_BADOBJECT, got %#x.\n", i, hr);
> +        /* It's odd, but InitFromFile seems to AddRef IDirect3DRM even if it fails. */
> So a test for the refcounts would be appropriate here.
> +HRESULT d3drm_texture_load(struct d3drm_texture *texture, IDirect3DRM *d3drm, const char *path, BOOL load_upside_down)
> +{
> This is an internal helper function now and should be static. You
> never use "d3drm".
> +D3DRMIMAGE *d3drm_create_image(unsigned char *buffer, BITMAPINFO *info, BOOL palette, BOOL upside_down)
> +{
> +    D3DRMPALETTEENTRY *colors;
> +    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;
> There's not reason to use signed types for any of these.
> +    struct colors_24bpp
> +    {
> +        BYTE red;
> +        BYTE green;
> +        BYTE blue;
> +    } *color;
> +    color = (struct colors_24bpp *)buffer;
> +
> +    if (bpl * h > UINT_MAX)
> +    {
> +        return NULL;
> +    }
> This doesn't work. If "bpl * h" would be larger than UINT_MAX it would
> overflow before the comparison. In fact, strictly speaking things
> become implementation defined above INT_MAX already, since you're
> using signed types for "bpl" and "h". What you'd typically do is
> something like "if (h > UINT_MAX / bpl)". Note that the calculation of
> "bpl" itself can overflow as well. So you'd need something like "if
> (align(w, 4) > UINT_MAX / (bpp / 8))" there as well.
> +    if (!(colors = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, 256 * sizeof(*colors))))
> +    {
> +        WARN("Not enough memory to allocate palette, returning NULL.\n");
> +        return NULL;
> +    }
> +
> +
> +    if (!(image = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*image))))
> +    {
> +       WARN("Not enough memory to allocate image struct, returning NULL.\n");
> +        return NULL;
> +    }
> You probably don't need HEAP_ZERO_MEMORY for either of these. You
> certainly don't for "colors", and you should probably just initialise
> the few fields in "image" that you don't already. You also leak
> "colors" if the allocation for "image" fails. The indentation for the
> WARN is wrong.
> +    if (!(image->buffer1 = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, bpl * h)))
> +    {
> +        WARN("Not enough memory to allocate image buffer, returning NULL.\n");
> +        return NULL;
> +    }
> You don't need HEAP_ZERO_MEMORY here if you're going to memset() it
> again below. You leak "colors" and "image" on failure.
> +    buffer1 = image->buffer1;
> +    memset(buffer1, 255, sizeof(unsigned char) * bpl * h);
> I think 0xff is clearer than 255. The "sizeof(unsigned char)" doesn't add much.
> +            if (!(image->palette = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, num_colors * sizeof(*image->palette))))
> +            {
> +                WARN("Not enough memory to allocate image palette, returning NULL.\n");
> +                return NULL;
> +            }
> Like above, you use HEAP_ZERO_MEMORY here, but immediately memcpy()
> over it. You leak on error.
> +        if (!(image->palette = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, 256 * sizeof(*image->palette))))
> +        {
> +            WARN("Not enough memory to allocate image palette, returning NULL.\n");
> +            return NULL;
> +        }
> Leak on error.
> static HRESULT WINAPI d3drm_texture1_InitFromFile(IDirect3DRMTexture *iface, const char *filename)
>  {
> -    FIXME("iface %p, filename %s stub!\n", iface, debugstr_a(filename));
> +    struct d3drm_texture *object = impl_from_IDirect3DRMTexture(iface);
> I'd suggest "texture" instead of "object".
> +    if (object->d3drm)
> +    {
> +        /* InitFromFile seems to AddRef IDirect3DRM even if it fails. */
> +        IDirect3DRM_AddRef(object->d3drm);
> +    }
> This is very suspicious, since you always assign "object->d3drm" after
> calling InitFromFile(). It's not backed up by tests either.

[PATCH] d3drm: Partially Implement IDirect3DRM*::LoadTexture(v3).

[Aaryaman Vasishta]

On Tue, Mar 29, 2016 at 4:59 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
> On 28 March 2016 at 20:02, Aaryaman Vasishta <jem456.vasishta(a)gmail.com>
> wrote:
> > v3: Add bitmap verification, use InitFromFile and other minor
> > improvements.
>
> Implementing InitFromFile() should be a separate patch from
> implementing LoadTexture().
> > if (FAILED(hr = d3drm_texture_create(&object)))
> >          return hr;
> >
> > +    if (FAILED(hr =
>
> IDirect3DRMTexture_InitFromFile(&object->IDirect3DRMTexture_iface,
> filename)))
> > +    {
> > +        d3drm_texture_destroy(object);
> > +        return hr;
> > +    }
> > +
> > +    object->d3drm = iface;
> > +    IDirect3DRM_AddRef(iface);
> > It would be good to back this up with tests. It would seem more
> > natural to pass the IDirect3DRM interface to d3drm_texture_create().
> > If that's how it's supposed to work, you can make that change in a
> > separate patch.

Right.
> > @@ -39,6 +40,9 @@ struct d3drm_texture
> >      IDirect3DRMTexture IDirect3DRMTexture_iface;
> >      IDirect3DRMTexture2 IDirect3DRMTexture2_iface;
> >      IDirect3DRMTexture3 IDirect3DRMTexture3_iface;
> > +    IDirect3DRM *d3drm;
> > +    D3DRMIMAGE *image;
> > +    BOOL internal_image_created;
> > You don't really need internal_image_created, you can probably just
> > add a destroy callback.

Are there any existing examples within wine where a destroy callback was
used in similar cases?
> This is very suspicious, since you always assign "object->d3drm" after
> calling InitFromFile(). It's not backed up by tests either.

I think I had put that while debugging a segfault while the tests ran on
wine. LoadTexture called InitFromFile first, which then AddRef'd
IDirect3DRM. Since IDirect3DRM member wasn't initialized in the object by
then, the if check is needed. Also since LoadTexture needs to AddRef only
after IDirect3DRM after it's successfully loaded, I put the AddRef below
d3drm_texture_load.

I agree with all the other changes you mentioned. The leaks and signed
local vars are definitely a problem. I'll fix them, divide the patches up
and begin work on CreateObject first before submitting these again.


Cheers,
Aaryaman

[PATCH] d3drm: Partially Implement IDirect3DRM*::LoadTexture(v3).

[Henri Verbeet]

On 29 March 2016 at 17:01, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote:
> Are there any existing examples within wine where a destroy callback was
> used in similar cases?
> If you mean AddDestroyCallback() specifically, that's pretty specific
> to d3drm and you'd be limited to test_Viewport() in the d3drm tests.
> The concept of a callback that gets called on destruction is much more
> generic of course, and in that case you should be able to find plenty.
> E.g. surface_wined3d_object_destroyed() in d3d9.

[PATCH] d3drm: Partially Implement IDirect3DRM*::LoadTexture(v3).

[Aaryaman Vasishta]

AddDestroyCallback would need to be implemened as seperate patches for
IDirect3DRMTexture*. Using the BOOL avoids that, but I have a feeling these
callbacks might be useful in games as well, so I'll implement these first
before LoadTexture patches.

So the overall order of implementations/tests patches will be so:

CreateObject implementation and test
InitFromImage implementation and test
CreateTexture implementation and test
IDirect3DRMObject function implementations for IDirect3DRMTexture*.
InitFromFile implementation and test
LoadTexture implementations

If the order is not proper, do let me know.
Thanks for the review!

Cheers,
Aaryaman



On Tue, Mar 29, 2016 at 8:52 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
> On 29 March 2016 at 17:01, Aaryaman Vasishta <jem456.vasishta(a)gmail.com>
> wrote:
> > Are there any existing examples within wine where a destroy callback was
> > used in similar cases?
> > If you mean AddDestroyCallback() specifically, that's pretty specific
> > to d3drm and you'd be limited to test_Viewport() in the d3drm tests.
> > The concept of a callback that gets called on destruction is much more
> > generic of course, and in that case you should be able to find plenty.
> > E.g. surface_wined3d_object_destroyed() in d3d9.

[PATCH] d3drm: Partially Implement IDirect3DRM*::LoadTexture(v3).

[Henri Verbeet]

On 29 March 2016 at 18:43, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote:
> AddDestroyCallback would need to be implemened as seperate patches for
> IDirect3DRMTexture*.
> Yeah, but it mostly comes down to adding a struct d3drm_object field
> to struct d3drm_texture, calling d3drm_object_add_destroy_callback()
> and d3drm_object_delete_destroy_callback() in the appropriate places,
> and some initialisation and cleanup code. Shouldn't be hard.
> So the overall order of implementations/tests patches will be so:
>
> CreateObject implementation and test
> InitFromImage implementation and test
> CreateTexture implementation and test
> IDirect3DRMObject function implementations for IDirect3DRMTexture*.
> InitFromFile implementation and test
> LoadTexture implementations

Yeah, that makes sense. The IDirect3DRMObject only need to be before
InitFromFile(), so you could send them e.g. before the CreateObject()
implementation as well, if you like.