[vlc-devel] [PATCH] Direct3D shader support (HLSL)
Sasha Koruga
skoruga at gmail.com
Mon Aug 16 23:07:05 CEST 2010
Hi Fenrir,
>> bool use_desktop;
>> + bool *shaderFXOption;
> I don't see why shaderFXOption is present. Is it used by another patch?
>
Yes, in the GUI patch, which will be submitted separately (as JB requested).
>> struct {
>> bool is_fullscreen;
>> bool is_on_top;
>> @@ -177,6 +178,8 @@ struct vout_display_sys_t
>>
>> // core objects
>> HINSTANCE hd3d9_dll; /* handle of the opened
>> d3d9 dll */
>> + HINSTANCE hd3d9x_dll; /* handle of the opened
>> d3d9x dll */
>> + void* d3dxShader;
> ID3DXEffect *d3dxShader
> would make the code easier to read.
Trust me, I tried. I obviously don't prefer declaring it as a void*.
However, including the necessary header to define ID3DXEffect* in
common.h is too much of a mess for it to be worth it. It conflicts
with other headers, etc.
>
> Btw, please do not use camel case variable name (shader_option/d3dx_shader/d3dxshader
> or something like that for example, same for following code).
Ok, will do.
>
>> LPDIRECT3D9 d3dobj;
>> LPDIRECT3DDEVICE9 d3ddev;
>> D3DPRESENT_PARAMETERS d3dpp;
>
>> diff --git a/modules/video_output/msw/direct3d.c
>> b/modules/video_output/msw/direct3d.c
>> index 9237c30..4ce7327 100644
>> --- a/modules/video_output/msw/direct3d.c
>> +++ b/modules/video_output/msw/direct3d.c
>> @@ -1,10 +1,11 @@
>> /*****************************************************************************
>> * direct3d.c: Windows Direct3D video output module
>> *****************************************************************************
>> - * Copyright (C) 2006-2009 the VideoLAN team
>> + * Copyright (C) 2006-2010 the VideoLAN team
>> *$Id$
>> *
>> - * Authors: Damien Fouilleul <damienf at videolan.org>
>> + * Authors: Damien Fouilleul <damienf at videolan.org>,
>> + * Sasha Koruga <skoruga at gmail.com>
>> *
>> * This program is free software; you can redistribute it and/or modify
>> * it under the terms of the GNU General Public License as published by
>> @@ -44,8 +45,16 @@
>>
>> #include <windows.h>
>> #include <d3d9.h>
>> +#include <d3dx9.h>
>>
>> #include "common.h"
>> +#include "hlsl.h"
>> +
>> +#ifndef D3DXFX_NOT_CLONEABLE
>> +#define D3DXFX_NOT_CLONEABLE (1 << 11)
>> +#endif
> Why is it defined here and not in a relevent header?
Read xxdc's response. I'm using the original dx header, and thus I
didn't have an issue. However, the mingw-64 or whatnot headers are
missing the defines. xxdc already petitioned for them to add that, and
once they do, that line of code can be removed.
>
>> +#define SHADEROPTIONSCOUNT 8
>>
>> /*****************************************************************************
>> * Module descriptor
>> @@ -456,6 +465,31 @@ static int Direct3DCreate(vout_display_t *vd)
>> }
>> sys->d3dobj = d3dobj;
>>
>> + //load shader support
>> + for(int i = 43; i > 23; --i) {
>> + char buffer[3], filename[14];
>> + const char pre[] = "D3dx9_", post[] = ".dll";
>> + strncpy(filename, pre, sizeof(pre));
>> + strncat(filename, itoa(i,buffer,10), 2);
>> + strncat(filename, post, sizeof(post));
>> + sys->hd3d9x_dll = LoadLibrary(filename);
> char *filename;
> if (asprintf(&filename, "D3dx9_%d.dll", i) == -1) {
> continue;
> }
> free(filename);
> would be simpler and less error prone.
Will do (JB told me that in IRC as well)
>
>> + msg_Dbg(vd, "Attempting to load: %s", filename);
>> + if(sys->hd3d9x_dll) {
>> + msg_Dbg(vd, "loaded: %s", filename);
>> + break;
>> + }
>> + }
>> + if(!sys->hd3d9x_dll) {
>> + msg_Warn(vd, "cannot load D3dx9d_[24-43].dll; HLSL pixel
>> shading will be disabled. (Please install DirectX end-user runtime)");
>> + sys->d3dxShader = NULL;
>> + sys->shaderFXOption = NULL;
>> + }
>> + else {
>> + sys->shaderFXOption = malloc(sizeof(bool) * SHADEROPTIONSCOUNT);
> calloc(SHADEROPTIONSCOUNT, sizeof(*sys->shaderFXOption ));
Will fix.
>
>> + for(unsigned int i = 0; i < SHADEROPTIONSCOUNT; ++i)
> unsigned i
> is enough (and more consistant, same for following code).
>
Don't understand your comment. Please elaborate.
Come to think of it, I can just memset the entire array to zero in one
swoop; that would be faster than a loop.
>> + sys->shaderFXOption[i] = false;
>> + }
>> +
> Also, declaring bool shaderFXOption[SHADEROPTIONSCOUNT] directly would be simpler.
> But I am not sure why/how shaderFXOption is used, so it may depends on another of
> your patch.
I originally had it like that, but changed it in order to support
dynamic additions of shaders (per JB's request).
>
>> /*
>> ** Get device capabilities
>> */
>> @@ -478,13 +512,23 @@ static void Direct3DDestroy(vout_display_t *vd)
>> {
>> vout_display_sys_t *sys = vd->sys;
>>
>> - if (sys->d3dobj)
>> - IDirect3D9_Release(sys->d3dobj);
>> - if (sys->hd3d9_dll)
>> + if (sys->d3dobj) {
>> + IDirect3D9_Release(sys->d3dobj);
>> + sys->d3dobj = NULL;
>> + }
>> + if (sys->hd3d9_dll) {
>> FreeLibrary(sys->hd3d9_dll);
>> + sys->hd3d9_dll = NULL;
>> + }
>> + if (sys->hd3d9x_dll) {
>> + FreeLibrary(sys->hd3d9x_dll);
>> + sys->hd3d9x_dll = NULL;
>> + }
>> + if (sys->shaderFXOption) {
>> + free(sys->shaderFXOption);
>> + sys->shaderFXOption = NULL;
>> + }
>>
>> - sys->d3dobj = NULL;
>> - sys->hd3d9_dll = NULL;
> I would prefer to let all the = NULL at the end (simpler to read, less lines).
> (Same for all occurence if the same reasons applies).
I feel that when making code adjustments, it's easier to miss
something if code that belongs together is apart. Generally if you're
moving code around, you want to be able to cut&paste in one go when
possible. If lines of code is an issue, though, the brackets are not
actually necessary; they are only there to improve readability.
>
>> @@ -589,6 +633,40 @@ static int Direct3DOpen(vout_display_t *vd,
>> video_format_t *fmt)
>> return VLC_EGENERIC;
>> }
>>
>> + if(sys->hd3d9x_dll) {
>> + HRESULT (WINAPI * OurD3DXCreateEffectFromFileA)(
>> + LPDIRECT3DDEVICE9 pDevice,
>> + LPCSTR pSrcFile,
>> + CONST D3DXMACRO* pDefines,
>> + LPD3DXINCLUDE pInclude,
>> + DWORD Flags,
>> + LPD3DXEFFECTPOOL pPool,
>> + LPD3DXEFFECT* ppEffect,
>> + LPD3DXBUFFER* ppCompilationErrors);
> Please, correctly align. No need for the terminal A.
Per JB's request, I'm changing it to a W to enforce utf-8 reading. I
did originally attempt to use "D3DXCreateEffectFromFile" (which would
have been simpler for me, of course), but that approach had issues.
>> +
>> + OurD3DXCreateEffectFromFileA =
>> (void*)GetProcAddress(sys->hd3d9x_dll,
>> TEXT("D3DXCreateEffectFromFileA"));
>> + if (!OurD3DXCreateEffectFromFileA) {
>> + msg_Warn(vd, "Cannot locate reference to
>> D3DXCreateEffectFromFileA ABI in DLL; pixel shading will be
>> disabled");
>> + sys->d3dxShader = NULL;
>> + }
>> + else {
>> + LPD3DXBUFFER buffer = NULL;
>> + DWORD dwShaderFlags = D3DXFX_NOT_CLONEABLE;
>> + char fileLocation[512];
>> + strncpy(fileLocation, config_GetUserDir( VLC_DATA_DIR ), 512);
>> + strncat(fileLocation, "\\pixelShader.fx", 512 -
>> strlen(fileLocation));
> asprintf may also help here.
Will do.
>
>> + HRESULT hr = OurD3DXCreateEffectFromFileA(sys->d3ddev,
>> fileLocation, NULL, NULL, dwShaderFlags, NULL,
>> ((ID3DXEffect**)(&sys->d3dxShader)), &buffer);
>> + if(FAILED(hr)) //try loading it from current directory as
>> oppose to VLC_DATA_DIR
>> + hr = OurD3DXCreateEffectFromFileA(sys->d3ddev,
>> "pixelShader.fx", NULL, NULL, dwShaderFlags, NULL,
>> ((ID3DXEffect**)(&sys->d3dxShader)), &buffer);
> I am not sure we want that one.
You don't want the back-up measure? Ok, I'll remove it. I honestly
just had it there to make it easier for me to debug the .fx file.
>> + if(FAILED(hr)) {
>> + msg_Warn(vd, "D3DXCreateEffectFromFileA Error
>> (hr=0x%lX) -- pixel shading will be disabled (pixelShader.fx possibly
>> missing).", hr);
>> + if(buffer)
>> + msg_Warn(vd, "HLSL Compilation Error: %s",
>> (char*)buffer->lpVtbl->GetBufferPointer(buffer));
>> + sys->d3dxShader = NULL;
>> + }
>> + }
>> + }
>> +
>> /* Change the window title bar text */
>> EventThreadUpdateTitle(sys->event, VOUT_TITLE " (Direct3D output)");
>>
>> @@ -605,10 +683,14 @@ static void Direct3DClose(vout_display_t *vd)
>>
>> Direct3DDestroyResources(vd);
>>
>> - if (sys->d3ddev)
>> + if(sys->d3dxShader) {
>> + ((ID3DXEffect*)(sys->d3dxShader))->lpVtbl->Release(sys->d3dxShader);
>> + sys->d3dxShader = NULL;
>> + }
>> + if (sys->d3ddev) {
>> IDirect3DDevice9_Release(sys->d3ddev);
>> -
>> - sys->d3ddev = NULL;
>> + sys->d3ddev = NULL;
>> + }
>> }
>>
>> /**
>> @@ -1035,6 +1117,7 @@ static void Direct3DDestroyScene(vout_display_t *vd)
>> */
>> static void Direct3DRenderScene(vout_display_t *vd, LPDIRECT3DSURFACE9 surface)
>> {
>> + unsigned int shaderPasses;
>> vout_display_sys_t *sys = vd->sys;
>> LPDIRECT3DDEVICE9 d3ddev = sys->d3ddev;
>> HRESULT hr;
>> @@ -1145,39 +1228,88 @@ static void Direct3DRenderScene(vout_display_t
>> *vd, LPDIRECT3DSURFACE9 surface)
>> return;
>> }
>>
>> - // Setup our texture. Using textures introduces the texture stage states,
>> - // which govern how textures get blended together (in the case of multiple
>> - // textures) and lighting information. In this case, we are modulating
>> - // (blending) our texture with the diffuse color of the vertices.
>> - hr = IDirect3DDevice9_SetTexture(d3ddev, 0,
>> (LPDIRECT3DBASETEXTURE9)d3dtex);
>> - if (FAILED(hr)) {
>> - msg_Dbg(vd, "%s:%d (hr=0x%0lX)", __FUNCTION__, __LINE__, hr);
>> - IDirect3DDevice9_EndScene(d3ddev);
>> - return;
>> - }
>> + if(((ID3DXEffect*)(sys->d3dxShader)))
> if (sys->d3dxShader)
> is enougth.
JB thinks (ID3DXEffect*)(sys->d3dxShader) is messy, so I'll fix the
readability of that in the entire function.
I think I did have "if (sys->d3dxShader)" in there originally, but
there was something wrong with it (maybe a compile warning...can't
remember).
Anyway, this will not be an issue.
>
>> + {
>> + hr = ((ID3DXEffect*)(sys->d3dxShader))->lpVtbl->SetTechnique(((ID3DXEffect*)(sys->d3dxShader)),
> Defining all the ID3DXEffect_* wrapper (as done for other D3D object like
> IDirect3DSurface9) will make the code more readable.
> (Same for following code).
Ok
>
>> "Shade");
>> + if (FAILED(hr))
>> + {
>> + msg_Err(vd, "SetTechnique failed (hr=0x%lX)", hr);
>> + }
>>
>> - // Render the vertex buffer contents
>> - hr = IDirect3DDevice9_SetStreamSource(d3ddev, 0, d3dvtc, 0,
>> sizeof(CUSTOMVERTEX));
>> - if (FAILED(hr)) {
>> - msg_Dbg(vd, "%s:%d (hr=0x%0lX)", __FUNCTION__, __LINE__, hr);
>> - IDirect3DDevice9_EndScene(d3ddev);
>> - return;
>> + const char * shaderStrings[SHADEROPTIONSCOUNT] =
>> {"convertbt", "widen", "detect", "gamma18", "gamma22", "gammabt",
>> "invert", "grayscale"};
>> + for(unsigned int i = 0; i < SHADEROPTIONSCOUNT; ++i)
>> + {
>> + hr =
>> ((ID3DXEffect*)(sys->d3dxShader))->lpVtbl->SetBool(((ID3DXEffect*)(sys->d3dxShader)),
>> shaderStrings[i], sys->shaderFXOption[i]);
>> + if (FAILED(hr))
>> + {
>> + msg_Err(vd, "SetBool #%d failed (hr=0x%lX)", i, hr);
>> + }
>> + }
>> +
>> + hr = ((ID3DXEffect*)(sys->d3dxShader))->lpVtbl->Begin(((ID3DXEffect*)(sys->d3dxShader)),
>> &shaderPasses, 0);
>> + if (FAILED(hr))
>> + {
>> + msg_Err(vd, "Begin failed (hr=0x%lX)", hr);
>> + }
>> }
>> + else
>> + shaderPasses = 1;
>> +
>> + for( UINT uPass = 0; uPass < shaderPasses; ++uPass ) {
>> + if(((ID3DXEffect*)(sys->d3dxShader))) {
>> + hr =
>> ((ID3DXEffect*)(sys->d3dxShader))->lpVtbl->BeginPass(((ID3DXEffect*)(sys->d3dxShader)),
>> uPass);
>> + if (FAILED(hr))
>> + msg_Err(vd, "BeginPass failed (hr=0x%lX)", hr);
>> + }
>> +
>> + // Setup our texture. Using textures introduces the texture
>> stage states,
>> + // which govern how textures get blended together (in the
>> case of multiple
>> + // textures) and lighting information. In this case, we are modulating
>> + // (blending) our texture with the diffuse color of the vertices.
>> + hr = IDirect3DDevice9_SetTexture(d3ddev, 0,
>> (LPDIRECT3DBASETEXTURE9)d3dtex);
>> + if (FAILED(hr)) {
>> + msg_Dbg(vd, "%s:%d (hr=0x%0lX)", __FUNCTION__, __LINE__, hr);
>> + IDirect3DDevice9_EndScene(d3ddev);
>> + return;
>> + }
>> +
>> + // Render the vertex buffer contents
>> + hr = IDirect3DDevice9_SetStreamSource(d3ddev, 0, d3dvtc, 0,
>> sizeof(CUSTOMVERTEX));
>> + if (FAILED(hr)) {
>> + msg_Dbg(vd, "%s:%d (hr=0x%0lX)", __FUNCTION__, __LINE__, hr);
>> + IDirect3DDevice9_EndScene(d3ddev);
>> + return;
>> + }
>> +
>> + // we use FVF instead of vertex shader
>> + hr = IDirect3DDevice9_SetFVF(d3ddev, D3DFVF_CUSTOMVERTEX);
>> + if (FAILED(hr)) {
>> + msg_Dbg(vd, "%s:%d (hr=0x%0lX)", __FUNCTION__, __LINE__, hr);
>> + IDirect3DDevice9_EndScene(d3ddev);
>> + return;
>> + }
>> +
>> + // draw rectangle
>> + hr = IDirect3DDevice9_DrawPrimitive(d3ddev, D3DPT_TRIANGLEFAN, 0, 2);
>> + if (FAILED(hr)) {
>> + msg_Dbg(vd, "%s:%d (hr=0x%0lX)", __FUNCTION__, __LINE__, hr);
>> + IDirect3DDevice9_EndScene(d3ddev);
>> + return;
>> + }
>> +
>> +
>> + if(((ID3DXEffect*)(sys->d3dxShader))) {
>> + hr =
>> ((ID3DXEffect*)(sys->d3dxShader))->lpVtbl->EndPass(((ID3DXEffect*)(sys->d3dxShader)));
>> + if (FAILED(hr))
>> + msg_Err(vd, "EndPass failed (hr=0x%lX)", hr);
>> + }
> I don't know how shader works so I can be wrong, but is it really needed to do
> all that for each shader?
Yes. I don't put in unnecessary code, of course ^_^
If you think that's wordy, consider that the original had all of those
lines in order to just draw 2 textured triangles.
At least the shader code is doing a lot more (and all on the GPU).
>
>>
>> - // we use FVF instead of vertex shader
>> - hr = IDirect3DDevice9_SetFVF(d3ddev, D3DFVF_CUSTOMVERTEX);
>> - if (FAILED(hr)) {
>> - msg_Dbg(vd, "%s:%d (hr=0x%0lX)", __FUNCTION__, __LINE__, hr);
>> - IDirect3DDevice9_EndScene(d3ddev);
>> - return;
>> }
>>
>> - // draw rectangle
>> - hr = IDirect3DDevice9_DrawPrimitive(d3ddev, D3DPT_TRIANGLEFAN, 0, 2);
>> - if (FAILED(hr)) {
>> - msg_Dbg(vd, "%s:%d (hr=0x%0lX)", __FUNCTION__, __LINE__, hr);
>> - IDirect3DDevice9_EndScene(d3ddev);
>> - return;
>> + if(((ID3DXEffect*)(sys->d3dxShader))) {
>> + hr = ((ID3DXEffect*)(sys->d3dxShader))->lpVtbl->End(((ID3DXEffect*)(sys->d3dxShader)));
>> + if (FAILED(hr))
>> + msg_Err(vd, "End failed (hr=0x%lX)", hr);
>> }
>
> The code also needs to be checked for K&R style (consistant with the file you are editing).
I actually did go through all the code I originally wrote and change all:
if(x)
{
}
to
if(x) {
}
If I am missing it somewhere, please elaborate.
>
> I am a bit sceptical about the way shaders are activated/disabled. It seems a bit too
> much hardcoded, but that could be changed later if needed.
JB mentioned that as well, and I'm in the process of changing it to be
more dynamic. Usually when JB responds, I try to catch him on IRC, and
then he explains & elaborates on what he wants to see. Unfortunetly,
that means our dialog is left out of the email chain.
Thank you for reviewing my code, Fenrir.
Regards,
Sasha
More information about the vlc-devel
mailing list