[vlc-devel] [PATCH] Direct3D shader support (HLSL)
Sasha Koruga
skoruga at gmail.com
Sat Aug 14 23:42:52 CEST 2010
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: 0001-Updated-direct3d.c-to-better-comply-with-VLC-coding-.patch
Type: application/octet-stream
Size: 20168 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20100814/32f08a59/attachment.obj>
More information about the vlc-devel
mailing list