Re: [PATCH 2/4] d3drm: Implement IDirect3DRMTexture{2-3}_InitFromImage.

[Stefan Dösinger]

> Am 28.04.2016 um 20:17 schrieb Aaryaman Vasishta :
> - if (texture->d3drm && texture->initialized)
> -
>    IDirect3DRM_Release(texture->d3drm);

I thought the idea was to use AddDestroyCallback instead of a flag to control the Release call. Do I remember this incorrectly, or what's the reason for going back to a flag?

Also, what's the difference between image != NULL and initialized? Or, in a later stage when you have initFromTexture or what it is called, image != NULL || ddraw_texture != NULL?
> IDirect3DRM_AddRef(texture->d3drm);
> -
> - if (texture->image)
> -
>    return D3DRMERR_BADOBJECT;

Afair  the ref leak here is intentional. If it is, I think it's worth adding a comment.

[PATCH 2/4] d3drm: Implement IDirect3DRMTexture{2-3}_InitFromImage.

[Stefan Dösinger]

> > IDirect3DRM_AddRef(texture->d3drm);
> > -
> > - if (texture->image)
> > -
> >    return D3DRMERR_BADOBJECT;
>
> Afair  the ref leak here is intentional. If it is, I think it's worth adding a comment.

I just saw the other mails that Apple mail has been hiding from me... Still, a comment would be a good idea :-)

[PATCH 2/4] d3drm: Implement IDirect3DRMTexture{2-3}_InitFromImage.

[Aaryaman Vasishta]

On Fri, Apr 29, 2016 at 12:56 PM, Stefan Dösinger <[e-mail]
> wrote:
> > Am 28.04.2016 um 20:17 schrieb Aaryaman Vasishta <
>
> [e-mail]>:
> > - if (texture->d3drm && texture->initialized)
> > -
> >    IDirect3DRM_Release(texture->d3drm);
>
> I thought the idea was to use AddDestroyCallback instead of a flag to
> control the Release call. Do I remember this incorrectly, or what's the
> reason for going back to a flag?
>
> Also, what's the difference between image != NULL and initialized? Or, in
> a later stage when you have initFromTexture or what it is called, image !=
> NULL || ddraw_texture != NULL?

AddDestroyCallback would be used to destroy the image struct, which will be
used when InitFromFile will be implemented. Since the image here is
provided by the application, there wasn't any need to use a destroy
callback right now.

texture->initialized is only used to indicate whether InitFrom* was called
on texture or not. As you can see from the tests, d3drm AddRef's
IDirect3DRM only after InitFrom* is called. (I had initially thought that
CreateObject would do that, but the tests say otherwise).

Since we needed the reference to IDirect3DRM when AddRefing it during the
InitFrom* calls, I decided to initialize texture->d3drm at CreateObject
without AddRefing it there. So if someone decides to release a texture
created using CreateObject but not initialized using InitFrom*, the
texture->initialized member will prevent d3drm from releasing
texture->d3drm.
> > IDirect3DRM_AddRef(texture->d3drm);
> > -
> > - if (texture->image)
> > -
> >    return D3DRMERR_BADOBJECT;
>
> Afair  the ref leak here is intentional. If it is, I think it's worth
> adding a comment.

Right, I will add comments above the related test as well.

Cheers,
Aaryaman

[PATCH 2/4] d3drm: Implement IDirect3DRMTexture{2-3}_InitFromImage.

[Nikolay Sivov]

On 29.04.2016 12:16, Aaryaman Vasishta wrote:
> On Fri, Apr 29, 2016 at 12:56 PM, Stefan Dösinger
> <[e-mail] > wrote:
> > Am 28.04.2016 um 20:17 schrieb Aaryaman Vasishta <[e-mail] <mailto:[e-mail]>>:
> >
> > +    if (texture->d3drm && texture->initialized)
> > +        IDirect3DRM_Release(texture->d3drm);
> I thought the idea was to use AddDestroyCallback instead of a flag
> to control the Release call. Do I remember this incorrectly, or
> what's the reason for going back to a flag?
>
> Also, what's the difference between image != NULL and initialized?
> Or, in a later stage when you have initFromTexture or what it is
> called, image != NULL || ddraw_texture != NULL?
>
> AddDestroyCallback would be used to destroy the image struct, which will
> be used when InitFromFile will be implemented. Since the image here is
> provided by the application, there wasn't any need to use a destroy
> callback right now.
>
> texture->initialized is only used to indicate whether InitFrom* was
> called on texture or not. As you can see from the tests, d3drm AddRef's
> IDirect3DRM only after InitFrom* is called. (I had initially thought
> that CreateObject would do that, but the tests say otherwise).
>
> Since we needed the reference to IDirect3DRM when AddRefing it during
> the InitFrom* calls, I decided to initialize texture->d3drm at
> CreateObject without AddRefing it there. So if someone decides to
> release a texture created using CreateObject but not initialized using
> InitFrom*, the texture->initialized member will prevent d3drm from
> releasing texture->d3drm.

What will happen if you release IDirect3DRM before calling InitFrom*,
and then call InitFrom* expecting already released IDirect3DRM to be valid?
> >  IDirect3DRM_AddRef(texture->d3drm);
> > +
> > +    if (texture->image)
> > +        return D3DRMERR_BADOBJECT;
> Afair  the ref leak here is intentional. If it is, I think it's
> worth adding a comment.
>
> Right, I will add comments above the related test as well.
>
> Cheers,
> Aaryaman

[PATCH 2/4] d3drm: Implement IDirect3DRMTexture{2-3}_InitFromImage.

[Aaryaman Vasishta]

> What will happen if you release IDirect3DRM before calling InitFrom*,
> and then call InitFrom* expecting already released IDirect3DRM to be valid?

As long as you haven't QI'd version 2 and/or version 3 from version 1, each
version having their own refcounts (see the refcount tests) it will crash
with a segfault. If in case you did QI any other version, the refcount will
go to zero but the object won't be destroyed until the other refcounts go
down to zero as well (see IDirect3DRM3_Release). I ran a quick test and can
confirm this. So overall I guess we can say that it's upto the
application's handling of the objects.

It's odd, but it seems that d3drm will only use version 1's refcount and
not touch version 2 and version 3's refcount for version 2 and version 3's
InitFromImage respectively.
I'll go ahead and resend the patch with the comment additions.

Cheers,
Aaryaman