<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 29, 2016 at 4:59 PM, Henri Verbeet <span dir="ltr"><<a href="mailto:hverbeet@gmail.com" target="_blank">hverbeet@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 28 March 2016 at 20:02, Aaryaman Vasishta <<a href="mailto:jem456.vasishta@gmail.com">jem456.vasishta@gmail.com</a>> wrote:<br>
> v3: Add bitmap verification, use InitFromFile and other minor improvements.<br>
><br>
</span>Implementing InitFromFile() should be a separate patch from<br>
implementing LoadTexture().<br>
<span class=""><br>
>      if (FAILED(hr = d3drm_texture_create(&object)))<br>
>          return hr;<br>
><br>
> +    if (FAILED(hr = IDirect3DRMTexture_InitFromFile(&object->IDirect3DRMTexture_iface, filename)))<br>
> +    {<br>
> +        d3drm_texture_destroy(object);<br>
> +        return hr;<br>
> +    }<br>
> +<br>
> +    object->d3drm = iface;<br>
> +    IDirect3DRM_AddRef(iface);<br>
</span>It would be good to back this up with tests. It would seem more<br>
natural to pass the IDirect3DRM interface to d3drm_texture_create().<br>
If that's how it's supposed to work, you can make that change in a<br>
separate patch.<br></blockquote><div>Right. <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> @@ -39,6 +40,9 @@ struct d3drm_texture<br>
>      IDirect3DRMTexture IDirect3DRMTexture_iface;<br>
>      IDirect3DRMTexture2 IDirect3DRMTexture2_iface;<br>
>      IDirect3DRMTexture3 IDirect3DRMTexture3_iface;<br>
> +    IDirect3DRM *d3drm;<br>
> +    D3DRMIMAGE *image;<br>
> +    BOOL internal_image_created;<br>
</span>You don't really need internal_image_created, you can probably just<br>
add a destroy callback.<br></blockquote><div>Are there any existing examples within wine where a destroy callback was used in similar cases? <span class=""></span><span class=""></span><br><span class=""></span></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
</span>This is very suspicious, since you always assign "object->d3drm" after<br>
calling InitFromFile(). It's not backed up by tests either.<br></blockquote><div>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.<br><br></div><div>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.<br><br><br></div><div>Cheers,<br></div><div>Aaryaman <br></div></div><br></div></div>