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

[Sasha Koruga]

Hey JB,
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.
^^

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

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

>     add_bool("direct3d-desktop", false, NULL, DESKTOP_TEXT, DESKTOP_LONGTEXT, true)
> +    add_string( "shader-effects", "Disabled", NULL, SHADERFX_MODE_TEXT, SHADERFX_MODE_LONGTEXT, true )
> Why "Disabled" instead of empty?

This is the first option that appears in multi-select shader menu, and
that sets its name to "Disabled"

>
> +char shaderCode[] = " \
> Can't this be separated in special files?
>
> -    if (Direct3DCreate(vd)) {
> +    if (Direct3DCreate(vd))
> +    {

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.

> +    //load shader support
> +    for(int i = 43; i > 23; --i)
> +    {
> +        char buffer[3], filename[14];
> +        strcpy(filename, "D3dx9_");
>
> why not strncpy?

Will change (didn't know you guys used strncpy & strncat)


> -    d3dpp->BackBufferWidth        = __MAX(GetSystemMetrics(SM_CXVIRTUALSCREEN),
> +    d3dpp->BackBufferWidth        = __MAX((unsigned int)GetSystemMetrics(SM_CXVIRTUALSCREEN),
>                                           d3ddm.Width);
> -    d3dpp->BackBufferHeight       = __MAX(GetSystemMetrics(SM_CYVIRTUALSCREEN),
> +    d3dpp->BackBufferHeight       = __MAX((unsigned int)GetSystemMetrics(SM_CYVIRTUALSCREEN),
>                                           d3ddm.Height);
> Those 2 things should be in a separate patch

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

>
>     // Create the D3DDevice
>     LPDIRECT3DDEVICE9 d3ddev;
> -    HRESULT hr = IDirect3D9_CreateDevice(d3dobj, D3DADAPTER_DEFAULT,
> -                                         D3DDEVTYPE_HAL, sys->hvideownd,
> +
> +    // Look for 'NVIDIA PerfHUD' adapter
> +    // If it is present, override default settings
> +    UINT AdapterToUse = D3DADAPTER_DEFAULT;
> +    D3DDEVTYPE DeviceType = D3DDEVTYPE_HAL;
> +    for (UINT Adapter=0; Adapter< IDirect3D9_GetAdapterCount(d3dobj); ++Adapter)
> +    {
> +        D3DADAPTER_IDENTIFIER9 Identifier;
> +        HRESULT Res;
> +        Res = IDirect3D9_GetAdapterIdentifier(d3dobj,Adapter,0,&Identifier);
> +        if (strstr(Identifier.Description,"PerfHUD") != 0)
> +        {
> +            AdapterToUse = Adapter;
> +            DeviceType = D3DDEVTYPE_REF;
> +            break;
> +        }
> +    }
> +
> +    HRESULT hr = IDirect3D9_CreateDevice(d3dobj, AdapterToUse,
> +                                         DeviceType, sys->hvideownd,
>                                          D3DCREATE_SOFTWARE_VERTEXPROCESSING|
>                                          D3DCREATE_MULTITHREADED,
>                                          &sys->d3dpp, &d3ddev);
>
> This should be also in a separate patch.

Will do.
>
> +    if(sys->hd3d9x_dll)
> +    {
> +        HRESULT (WINAPI * OurD3DXCreateEffect)(
> +            LPDIRECT3DDEVICE9               pDevice,
> +            LPCVOID                         pSrcData,
> +            UINT                            SrcDataLen,
> +            CONST D3DXMACRO*                pDefines,
> +            LPD3DXINCLUDE                   pInclude,
> +            DWORD                           Flags,
> +            LPD3DXEFFECTPOOL                pPool,
> +            LPD3DXEFFECT*                   ppEffect,
> +            LPD3DXBUFFER*                   ppCompilationErrors);
>
> Why declaring it here?

Why not? I use it only in this one function.
Would you prefer it be declared somewhere else?

>
> +        OurD3DXCreateEffect = (void*)GetProcAddress(sys->hd3d9x_dll, TEXT("D3DXCreateEffect"));
> +        if (!OurD3DXCreateEffect)
> +        {
> +            msg_Warn(vd, "Cannot locate reference to D3DXCreateEffect ABI in DLL; pixel shading will be disabled");
> +            sys->d3dxShader = NULL;
> +        }
> +        else
> +        {
> +            LPD3DXBUFFER buffer = NULL;
> +            DWORD dwShaderFlags = D3DXFX_NOT_CLONEABLE;
> +            HRESULT hr = OurD3DXCreateEffect(sys->d3ddev, shaderCode, strlen(shaderCode), NULL, NULL, dwShaderFlags, NULL, ((ID3DXEffect**)(&sys->d3dxShader)), &buffer);
> +            if (FAILED(hr))
> +            {
> +               msg_Warn(vd, "OurD3DXCreateEffect Error (hr=0x%lX) -- pixel shading will be disabled.", hr);
>
> %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).

>
> @@ -771,10 +1072,7 @@ static int Direct3DLockSurface(picture_t *picture)
>  static void Direct3DUnlockSurface(picture_t *picture)
>  {
>     /* Unlock the Surface */
> -    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.


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

>
> Be careful, trailing spaces and tabs here too

Will do.


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.

Best Regards,
Sasha