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

[Sasha Koruga]

Hey JB,
I missed your "Can't this be separated in special files?" comment.
Yes, I originally had everything in a .fx file, but having a .fx file
would mean that that .fx file would have be included in the VLC
distribution (not just the source code). There's also the option of
distributing the compiled shader code, but I don't believe that is as
effective (the HLSL compiler can optimize the code better if it knows
the computer architecture).
I noticed that should be a *const* char, and I have changed that. I
could also just put the string declaration in another header file. Let
me know what you want me to do.

Regards,
Sasha

On Sat, Aug 14, 2010 at 11:57 AM, Sasha Koruga <skoruga at gmail.com> wrote:
> 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
>