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

[Jean-Baptiste Kempf]

Sasha,

On Mon, Aug 16, 2010 at 02:07:05PM -0700, Sasha Koruga wrote :
> 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...

> >> 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?

> >> +#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!).

> >> +        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"

> >> -    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;


> 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.

> >> +    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.

> >  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 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


-- 
Jean-Baptiste Kempf
http://www.jbkempf.com/
+33 672 704 734