Re: [PATCH] d3dx9_36: Implement D3DXCheckTextureRequirements + tests

[Tony Wasserka]

> +static inline void check_ref(IUnknown *obj, int exp)
> +static inline void check_release(IUnknown *obj, int exp)
> We shouldn't use these function definitions anymore IMO, either do the ref check in the code itself, or replace the check_ref and check_release functions with macros; the reason is that if there really is a test error, the given line refers to the function source and thus won't help us at all, i.e. before actually examining an error you'd first have to find out what check actually failed...

Also I'm not too sure whether it's necessary to include resources.h ...
> +        if (SUCCEEDED(hr)) {
> +            if (*format == D3DFMT_UNKNOWN || *format == D3DX_DEFAULT) *format = D3DFMT_A8R8G8B8;
> +
> +            hr = IDirect3D9_CheckDeviceFormat(d3d, params.AdapterOrdinal, params.DeviceType, mode.Format, usage,
> +                D3DRTYPE_TEXTURE, *format);
> +        }
> +
> +        if (d3d)
> +            IDirect3D9_Release(d3d);
> +
> +        if (FAILED(hr))
> +            return D3DERR_INVALIDCALL;
> Maybe we should add a FIXME within that if (FAILED(hr)), since that part is something which still needs to be implemented.

Best regards, Tony

[PATCH] d3dx9_36: Implement D3DXCheckTextureRequirements + tests

[Christian Costa]

Tony Wasserka a écrit :
> > +static inline void check_ref(IUnknown *obj, int exp)
> > +static inline void check_release(IUnknown *obj, int exp)
>
> We shouldn't use these function definitions anymore IMO, either do the ref check in the code itself, or replace the check_ref and check_release functions with macros; the reason is that if there really is a test error, the given line refers to the function source and thus won't help us at all, i.e. before actually examining an error you'd first have to find out what check actually failed...

Well, it's your code, correct ?
I suggest to simply remove these checks. There does not bring any added 
values.
> Also I'm not too sure whether it's necessary to include resources.h ...
> > +        if (SUCCEEDED(hr)) {
> > +            if (*format == D3DFMT_UNKNOWN || *format == D3DX_DEFAULT) *format = D3DFMT_A8R8G8B8;
> > +
> > +            hr = IDirect3D9_CheckDeviceFormat(d3d, params.AdapterOrdinal, params.DeviceType, mode.Format, usage,
> > +                D3DRTYPE_TEXTURE, *format);
> > +        }
> > +
> > +        if (d3d)
> > +            IDirect3D9_Release(d3d);
> > +
> > +        if (FAILED(hr))
> > +            return D3DERR_INVALIDCALL;
>
> Maybe we should add a FIXME within that if (FAILED(hr)), since that part is something which still needs to be implemented.

Could you be more specific on what's need to be implemented ?
The if statement catches several errors. Maybe you want to catch an 
error on CheckDeviceFormat, correct ?
> Best regards, Tony

BTW, you are reviewing your own patch (albeit slighty modified) ;-) , 
does that mean you plan to make your work
go into mainline ?

Regards
Christian

[PATCH] d3dx9_36: Implement D3DXCheckTextureRequirements + tests

[Tony Wasserka]

Am 19.04.2010 20:48, schrieb Christian Costa:
> Well, it's your code, correct ?
> I suggest to simply remove these checks. There does not bring any
> added values.
> It's my own code, but I noticed this flaw some time after GSoC was over,
> it can be quite disturbing.
> Additionally, the checks might not be useful in this case, but when/if
> D3DXCreateTexture gets implemented it'll make it easier to spot subtle
> memory leaks (like not Release()ing some buffer surface the function
> might use internally) which otherwise would go unnoticed.
> Could you be more specific on what's need to be implemented ?
> The if statement catches several errors. Maybe you want to catch an
> error on CheckDeviceFormat, correct ? 
> Yeah, I was refering to this particular error. I just saw I didn't
> commit the remaining code to my public git repo (... because it's a bit
> ugly), this ( http://tonwas.freefronthost.com/gsocwork.tar.gz ... early
> snapshot of my work, which I vastly cleaned up lateron) tarball provides
> the remaining code at snapshot/texture.c.
> What this code does is if the requested format is not supported, try to
> find some other format which hopefully won't reduce image quality.
> This is done by using some heuristics, i.e. rules (some of which are
> more important than others). Generally speaking, FourCC and 24-bit
> formats are avoided unless directly requested. Also we won't change
> format _types_, e.g. no ARGB to DXT conversion.
> Then, we try to not remove any color component channel; then we try to
> not reduce the number of bits per requested channel; then (of the few
> remaining formats) we pick the one with the lowest INCREASE in number of
> bits per channel (i.e. the best would be the same bpc); the last
> heuristic is the increase in number of channels, which ofc should be
> kept to a minimum.
> btw note that the heuristics given on the MSDN site of this function
> aren't the real ones; they have similiarity with the real ones, but my
> version of them is closer to actual native behavior. I've thoroughly
> tested this code and it works in almost 99% of all possible cases for
> ARGB/QWVU/Paletted/Luminance (i.e. "simple" uncompressed formats)) and
> in the very few cases it doesn't copy native behavior it at least
> returns a usuable fallback format.

Anyway, I strongly advise you to add this stuff in a later patch and
replace it with a FIXME message or some comment for now.
> BTW, you are reviewing your own patch (albeit slighty modified) ;-) ,
> does that mean you plan to make your work
> go into mainline ?

I'm glad someone picks up my patches, so I try to support this work as
far as I can. I just have no time on working on this stuff myself
anymore, i.e. no (direct) further work on this planned...


By the way, IMO the function documentation is pretty useful, at least
description and the stuff about the return values (and additional
NOTES), since it kind of verifies that the behavior is intended and not
the result of some ugly coding..

[PATCH] d3dx9_36: Implement D3DXCheckTextureRequirements + tests

[Christian Costa]

> Yeah, I was refering to this particular error. I just saw I didn't
> commit the remaining code to my public git repo (... because it's a bit
> ugly), this ( http://tonwas.freefronthost.com/gsocwork.tar.gz ... early
> snapshot of my work, which I vastly cleaned up lateron) tarball provides
> the remaining code at snapshot/texture.c.
> What this code does is if the requested format is not supported, try to
> find some other format which hopefully won't reduce image quality.
> This is done by using some heuristics, i.e. rules (some of which are
> more important than others). Generally speaking, FourCC and 24-bit
> formats are avoided unless directly requested. Also we won't change
> format _types_, e.g. no ARGB to DXT conversion.
> Then, we try to not remove any color component channel; then we try to
> not reduce the number of bits per requested channel; then (of the few
> remaining formats) we pick the one with the lowest INCREASE in number of
> bits per channel (i.e. the best would be the same bpc); the last
> heuristic is the increase in number of channels, which ofc should be
> kept to a minimum.
> btw note that the heuristics given on the MSDN site of this function
> aren't the real ones; they have similiarity with the real ones, but my
> version of them is closer to actual native behavior. I've thoroughly
> tested this code and it works in almost 99% of all possible cases for
> ARGB/QWVU/Paletted/Luminance (i.e. "simple" uncompressed formats)) and
> in the very few cases it doesn't copy native behavior it at least
> returns a usuable fallback format.
>
> Anyway, I strongly advise you to add this stuff in a later patch and
> replace it with a FIXME message or some comment for now.

I was thinking about something like this. I put a FIXME if 
CheckDeviceFormat fails until
there is a real implementation.