[vlc-devel] [PATCH] Direct3D shader support (HLSL)
Sasha Koruga
skoruga at gmail.com
Sat Aug 14 23:49:30 CEST 2010
Hello,
Here's another minor patch. This one removes three compiler warnings
in direct3d.c by casting variables.
More patches incoming.
Regards,
Sasha
On Sat, Aug 14, 2010 at 2:42 PM, Sasha Koruga <skoruga at gmail.com> wrote:
> Hello,
>
> I'm starting to break up the patches, as requested.
> Here's the first patch [attached]. It updates direct3d.c to better
> comply with VLC's coding conventions [
> http://wiki.videolan.org/Code_Conventions ].
> Since I complied with VLC's convention of "Leave braces alone on their
> lines," I wanted the original code to comply too, so that the
> convention was both consistent throughout the entire file and more
> compliant with VLC's standards.
> More patches incoming.
>
> Best Regards,
> Sasha
>
> On Sat, Aug 14, 2010 at 2:08 PM, Sasha Koruga <skoruga at gmail.com> wrote:
>> 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
>>>
>>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Removes-three-compile-warnings-by-casting.patch
Type: application/octet-stream
Size: 1807 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20100814/2176f2da/attachment.obj>
More information about the vlc-devel
mailing list