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

[Jean-Baptiste Kempf]

Hello,

On Sat, Aug 14, 2010 at 11:57:34AM -0700, Sasha Koruga wrote :
> Sorry for taking so long to respond (my last email was sent at 4am, so
> I really needed to get some sleep). Thank you for helping out, Xxvcv.
> ^^
Don't be sorry.

> > diff --git a/modules/gui/qt4/menus.cpp b/modules/gui/qt4/menus.cpp
> > index 3e682e0..2e173f1 100644
> > --- a/modules/gui/qt4/menus.cpp
> > +++ b/modules/gui/qt4/menus.cpp
> >
> > Should be in a separate patch, without tabs.
> 
> The GUI code is in menus.cpp, vout_wrapper.c, and direct3d.c. That is
> how the original direct3d author also did his gui coding, so I am just
> following his example. To my knowledge, there is no guide on how to
> add to the VLC gui, so I did my best job of reading the code and
> testing things out in order to figure it out. If I made any mistakes,
> please let me know.
> Did you want me to submit menus.cpp in a separate patch, even though
> it's related to the gui code in vout_wrapper.c and direct3d.c?
Yes, I do.

> > +#define SHADERFX_MODE_LONGTEXT N_( "Shader effect to use for video post-processing.")
> >  #define D3D_HELP N_("Recommended video output for Windows Vista and later versions")
> >
> > Should those be comma separated or?
> 
> I don't understand what you mean. Comma separated how?
I mean: what is the syntax of the shader-effects option? How can you
activate two, etc...

> I'm just changing it to adhere to VLC's coding convention guide:
> http://wiki.videolan.org/Code_Conventions
> That's also how I wrote my code in the file, so this makes it
> consistent throughout the file.

Well, I think those conventions are outdated.
Some people prefer the normal K&R style and I believe the direct3d.c
file follows it.
The important is too keep the code consistent.

> Will change (didn't know you guys used strncpy & strncat)
I believe they should be used the more possible.

> I'll make a separate patch for the warning suppressions.
thx.

> > This should be also in a separate patch.
> Will do.
thx.

> Why not? I use it only in this one function.
> Would you prefer it be declared somewhere else?
OK. I am just not used to those, but totally fine for me.

> > %lX ? %li maybe is better
> 
> I just copied the method the original d3d author used to output HR
> errors. Did you want me to change all of the HR output to %li?
> I must admit, though, I think %lX is better. I think that's what's
> generally used for HResult errors (when using Microsoft's HR lookup
> tool, it defaults to hex).
Ok. dxva2.c uses %u, but, yes, %lX is probably the best here. U'r right.


> > -    HRESULT hr = IDirect3DSurface9_UnlockRect(picture->p_sys->surface);
> > -    if (FAILED(hr)) {
> > -        //msg_Dbg(vd, "%s:%d (hr=0x%0lX)", __FUNCTION__, __LINE__, hr);
> > -    }
> > +    IDirect3DSurface9_UnlockRect(picture->p_sys->surface);
> >  }
> >
> > No, this can be helpful for debugging the module.
> 
> How so? The debugging line was commented out (notice the "//" before
> "msg_Dbg"). The function doesn't have access to the "vd" variable, so
> it cannot output debugging information.

If you want, but still, this isn't the same patch.

> > Should all this code be in the wrapper?
> 
> The direct3d module has some of its GUI code in the wrapper, so I
> believe so. I did try to have it in direct3d.c, but that did not work
> for whatever reason. I assume the original d3d author ran into the
> same issue, and thus why he also has GUI code in the wrapper.

I'll let Laurent comment.

> Thank you for reviewing my code, JB.
> Did you want me to submit a separate patch for the shader logic &
> shader GUI? The problem is, without the GUI code to enable selection
> of different shaders, you can't really test the shader code.
Yes. Splitting commits is better for reviewing.

Best Regards,

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