[vlc-devel] [PATCH] Direct3D shader support (HLSL)

[Sasha Koruga]

Hi JB,

>> Yes, in the GUI patch, which will be submitted separately (as JB requested).
> No, the wrapper.c code should be in the same patch, as I said on IRC...

Ok, I'll do that then (vout_wrapper.c only has gui code, though).

>
>> >> d3d9x dll */
>> >> +    void*                   d3dxShader;
>> > ID3DXEffect *d3dxShader
>> > would make the code easier to read.
>>
>> Trust me, I tried. I obviously don't prefer declaring it as a void*.
>> However, including the necessary header to define ID3DXEffect* in
>> common.h is too much of a mess for it to be worth it. It conflicts
>> with other headers, etc.
> Why?

I think it conflicted with the old directx headers in common.h. I'll
try it again when I get home, and I'll let you know. I don't think
Microsoft allows mixing of those two headers, because I think you only
end up with one or the other depending on which version of DirectX
you're using.

>
>> >> +#ifndef D3DXFX_NOT_CLONEABLE
>> >> +#define D3DXFX_NOT_CLONEABLE    (1 << 11)
>> >> +#endif
>> >  Why is it defined here and not in a relevent header?
>>
>> Read xxdc's response. I'm using the original dx header, and thus I
>> didn't have an issue. However, the mingw-64 or whatnot headers are
>> missing the defines. xxdc already petitioned for them to add that, and
>> once they do, that line of code can be removed.
> Mingw-w64 issues are not ours (yet!).

Well, that small snippet of code allows ppl like xxdc who don't want
to download the entire (HUGE) DirectX SDK to easily and quickly
compile the code. Otherwise, they get a compile error just because
D3DXFX_NOT_CLONEABLE is undefined. I'll remove it if you want, though.

>
>> >> +        for(unsigned int i = 0; i < SHADEROPTIONSCOUNT; ++i)
>> > unsigned i
>> > is enough (and more consistant, same for following code).
>> >
>>
>> Don't understand your comment. Please elaborate.
>> Come to think of it, I can just memset the entire array to zero in one
>> swoop; that would be faster than a loop.
>
> What he means is just use "unsigned i" instead of "unsigned int i"

Oh, I totally missed that! I just always read "unsigned int" and
"unsigned" the same, so I couldn't see the difference. I'll change it.

>
>> >> -    if (sys->d3dobj)
>> >> -       IDirect3D9_Release(sys->d3dobj);
>> >> -    if (sys->hd3d9_dll)
>> >> +    if (sys->d3dobj) {
>> >> +        IDirect3D9_Release(sys->d3dobj);
>> >> +        sys->d3dobj = NULL;
>> >> +    }
>> >> +    if (sys->hd3d9_dll) {
>> >>          FreeLibrary(sys->hd3d9_dll);
>> >> +        sys->hd3d9_dll = NULL;
>> >> +    }
>> >> +    if (sys->hd3d9x_dll) {
>> >> +        FreeLibrary(sys->hd3d9x_dll);
>> >> +        sys->hd3d9x_dll = NULL;
>> >> +    }
>> >> +    if (sys->shaderFXOption) {
>> >> +        free(sys->shaderFXOption);
>> >> +        sys->shaderFXOption = NULL;
>> >> +    }
>> >>
>> >> -    sys->d3dobj = NULL;
>> >> -    sys->hd3d9_dll = NULL;
>> >  I would prefer to let all the = NULL at the end (simpler to read, less lines).
>> > (Same for all occurence if the same reasons applies).
>>
>> I feel that when making code adjustments, it's easier to miss
>> something if code that belongs together is apart. Generally if you're
>> moving code around, you want to be able to cut&paste in one go when
>> possible. If lines of code is an issue, though, the brackets are not
>> actually necessary; they are only there to improve readability.
>
> No, what fenrir means is:
> "just remove all the sys->* = NULL and put them at the end, outside of
> the brackets."
>
> Something like this:
>    if (sys->d3dobj) {
>        IDirect3D9_Release(sys->d3dobj);
>    }
>    if (sys->hd3d9_dll) {
>        FreeLibrary(sys->hd3d9_dll);
>    }
>    if (sys->hd3d9x_dll) {
>        FreeLibrary(sys->hd3d9x_dll);
>    }
>    sys->hd3d9_dll = NULL;
>    sys->hd3d9x_dll = NULL;
>    sys->d3dobj = NULL;
>    free(sys->shaderFXOption);
>    sys->shaderFXOption = NULL;

Ok, I'll do that.

>
>
>> Per JB's request, I'm changing it to a W to enforce utf-8 reading. I
>> did originally attempt to use "D3DXCreateEffectFromFile" (which would
>> have been simpler for me, of course), but that approach had issues.
> If Unicode is set, D3DXCreateEffectFromFile should map to W. But, this
> is minor, IMVHO.

I'll try it again when I get home, but when I originally tried to
query the D3DXCreateEffectFromFile function, it did not work.
Compiling Direct3D through mingw causes a lot of complications that
require work-arounds.


>
>> >> +    if(((ID3DXEffect*)(sys->d3dxShader)))
>> > if (sys->d3dxShader)
>> > is enougth.
>>
>> JB thinks (ID3DXEffect*)(sys->d3dxShader) is messy, so I'll fix the
>> readability of that in the entire function.
>> I think I did have "if (sys->d3dxShader)" in there originally, but
>> there was something wrong with it (maybe a compile warning...can't
>> remember).
> It should work.
>
>> Anyway, this will not be an issue.
> Well, fixing it would be better.

What I meant was that I won't be using
"(ID3DXEffect*)(sys->d3dxShader)" in the code at all anymore in order
to improve readability, and thus this issue will automatically be
fixed.

>
>> >  The code also needs to be checked for K&R style (consistant with the file you are editing).
>> If I am missing it somewhere, please elaborate.
>
> You should use K&R style and not VLC's style in this file.

I thought I was using K&R style... what am I missing it?

>
>> >  I am a bit sceptical about the way shaders are activated/disabled. It seems a bit too
>> > much hardcoded, but that could be changed later if needed.
>>
>> JB mentioned that as well, and I'm in the process of changing it to be
>> more dynamic. Usually when JB responds, I try to catch him on IRC, and
>> then he explains & elaborates on what he wants to see. Unfortunetly,
>> that means our dialog is left out of the email chain.
> Pff :D

^^
Thanks, JB.

Regards,
Sasha