Map D3D calls and HLSL shaders back to GLES2 calls and GLSL ES shaders in PIX...

Map D3D calls and HLSL shaders back to GLES2 calls and GLSL ES shaders in PIX.

This makes debugging and profiling using PIX a lot more convenient. The top level of events are the GLES calls with their arguments. Those can be expanded to see the D3D calls that were issued for a particular GLES call.

When PIX is attached, the shaders are saved out to temporary files and referenced from the translated HLSL shaders via #line directives. This enabled source level debugging of the original GLSL from PIX for pixel and vertex shaders. The HLSL is also saved to a temporary file so that intrinsic functions like texture2D can be stepped into.

It also avoids creating a text file in the current working directory, which has continued to be an issue.

I made the dependency on d3d9.dll static again so it can be accessed by GetModuleHandle witihin DllMain.

I added an EVENT macro that issues D3DPERF_BeginEvent and D3DPERF_EndEvent around a C++ block. I replaced TRACE with EVENT for all the entry points.

I removed the tracing of shader source since the source is visible in PIX.

The means by which the filename of the temporary shader file is passed into the shader compiler is a little clunky. I did it that way to avoid changing the function signatures and breaking folks using the translator.

I plan to make the compiler respect #pragma optimize so that optimization can be disabled for debugging purposes. For now it just disables shader optimization in debug builds of ANGLE.



Committed: http://code.google.com/p/angleproject/source/detail?r=541

[apatrick1, 2011-01-12 21:52:18]

Another one for you Daniel. Now back to flipping Y coordinates...

Thanks,

Al

http://codereview.appspot.com/3945043/diff/1/src/compiler/OutputHLSL.cpp
File src/compiler/OutputHLSL.cpp (left):

http://codereview.appspot.com/3945043/diff/1/src/compiler/OutputHLSL.cpp#oldc...
src/compiler/OutputHLSL.cpp:1392: "{\n";
Do you know if these braces are actually required? I was having trouble getting
line numbers for them and it looks like, for example in this case, the true
block will be generated inside braces anyway. Otherwise I'll try and figure out
what line number to use here.

[dgkoch, 2011-01-13 04:22:40]

Adding Nicolas as a reviewer.  

Nicolas: can you take a look at this as well?

Overall, looks very neat and like it should help with debugging a fair bit.
Few minor issues noted below.

Daniel

http://codereview.appspot.com/3945043/diff/1/src/common/debug.cpp
File src/common/debug.cpp (right):

http://codereview.appspot.com/3945043/diff/1/src/common/debug.cpp#newcode23
src/common/debug.cpp:23: typedef void (WINAPI *BeginEventProc)(D3DCOLOR col,
LPCWSTR wszName);
This function returns an 'int'.
See
http://msdn.microsoft.com/en-us/library/microsoft.directx_sdk.reference.d3dpe...

http://codereview.appspot.com/3945043/diff/1/src/common/debug.cpp#newcode24
src/common/debug.cpp:24: typedef void (WINAPI *EndEventProc)(void);
This function also returns an int.

http://codereview.appspot.com/3945043/diff/1/src/common/debug.cpp#newcode54
src/common/debug.cpp:54: GetStatus = (GetStatusProc) GetProcAddress(module,
"D3DPERF_GetStatus");
Is it actually necessary to use GetProcAddress for all these?  I was fairly
certain that d3d9.dll always exported the D3DPERF functions.

http://codereview.appspot.com/3945043/diff/1/src/common/debug.cpp#newcode63
src/common/debug.cpp:63: int len = vsprintf_s(message, format, vararg);
doesn't vsprintf_s take the size as the second argument? 
http://msdn.microsoft.com/en-us/library/xa1a1a6z(VS.80).aspx

http://codereview.appspot.com/3945043/diff/1/src/common/debug.cpp#newcode73
src/common/debug.cpp:73: void trace(const char *format, ...)
I'd still like if we could have a file-based logging system as well (even if
it's not enabled by default).  Could possibly have the output() function write
message to the file as well.

http://codereview.appspot.com/3945043/diff/1/src/common/debug.h
File src/common/debug.h (right):

http://codereview.appspot.com/3945043/diff/1/src/common/debug.h#newcode21
src/common/debug.h:21: extern bool traceOn;
Since this is a global it should be called 'gTraceOn' or something like that.

http://codereview.appspot.com/3945043/diff/1/src/common/debug.h#newcode35
src/common/debug.h:35: public:
we use a half-indent (ie 2 spaces) for these in ANGLE

http://codereview.appspot.com/3945043/diff/1/src/compiler/OutputHLSL.cpp
File src/compiler/OutputHLSL.cpp (left):

http://codereview.appspot.com/3945043/diff/1/src/compiler/OutputHLSL.cpp#oldc...
src/compiler/OutputHLSL.cpp:1392: "{\n";
On 2011/01/12 21:52:18, apatrick1 wrote:
> Do you know if these braces are actually required? I was having trouble
getting
> line numbers for them and it looks like, for example in this case, the true
> block will be generated inside braces anyway. Otherwise I'll try and figure
out
> what line number to use here.

I don't know for sure.  Nicolas?

[apatrick1, 2011-01-13 23:54:57]

Main changes were to reinstate tracing to a file and not bind to D3DPERF API
with GetProcAddress.

Thanks,

Al

http://codereview.appspot.com/3945043/diff/1/src/common/debug.cpp
File src/common/debug.cpp (right):

http://codereview.appspot.com/3945043/diff/1/src/common/debug.cpp#newcode23
src/common/debug.cpp:23: typedef void (WINAPI *BeginEventProc)(D3DCOLOR col,
LPCWSTR wszName);
I used void so it had the same signature as SetMarkerProc() and could be passed
to output(). A bit of a hack I know. I can make it int by gratuitous use of
templates though.

http://codereview.appspot.com/3945043/diff/1/src/common/debug.cpp#newcode24
src/common/debug.cpp:24: typedef void (WINAPI *EndEventProc)(void);
Deleted.

http://codereview.appspot.com/3945043/diff/1/src/common/debug.cpp#newcode54
src/common/debug.cpp:54: GetStatus = (GetStatusProc) GetProcAddress(module,
"D3DPERF_GetStatus");
I did a Google search for these functions and found a lot of people on forums
complaining that the symbol could not be resolved. I think these functions might
not have been present in earlier versions of D3D9. Upon reconsideration, I
prefer ANGLE to fail if it's a really old version on D3D9. It'll reduce my crash
reports no end. Static dependency it is.

http://codereview.appspot.com/3945043/diff/1/src/common/debug.cpp#newcode63
src/common/debug.cpp:63: int len = vsprintf_s(message, format, vararg);
This is the template version of the function that infers the buffer size from
the first argument. See the signature for the final two overloads in the your
link.

http://codereview.appspot.com/3945043/diff/1/src/common/debug.cpp#newcode73
src/common/debug.cpp:73: void trace(const char *format, ...)
On 2011/01/13 04:22:45, dgkoch wrote:
> I'd still like if we could have a file-based logging system as well (even if
> it's not enabled by default).  Could possibly have the output() function write
> message to the file as well.

Done.

http://codereview.appspot.com/3945043/diff/1/src/common/debug.h
File src/common/debug.h (right):

http://codereview.appspot.com/3945043/diff/1/src/common/debug.h#newcode21
src/common/debug.h:21: extern bool traceOn;
On 2011/01/13 04:22:45, dgkoch wrote:
> Since this is a global it should be called 'gTraceOn' or something like that.
Deleted. It's not needed if the binding to the D3DPERF functions is static.

http://codereview.appspot.com/3945043/diff/1/src/common/debug.h#newcode35
src/common/debug.h:35: public:
On 2011/01/13 04:22:45, dgkoch wrote:
> we use a half-indent (ie 2 spaces) for these in ANGLE

Done.

[nicolas, 2011-01-14 12:57:20]

I haven't tested it in detail yet, but I believe the extra brackets could be
necessary in cases like:

    if(cond1) x = cond2 ? y : z;

This will currently translate into something like:

    if(cond1)
    {
        vec4 t;
        if(cond2) {t = y;} else {t = z;}
        x = t;
    }

Which wouldn't be correct if there were no brackets for cond1. HLSL has a
different semantic for the ternary operator, and they can be nested, so a
complicated translation like this is necessary.

[apatrick1, 2011-01-14 23:02:57]

The ternary operator was a good counter-example. I was able to make it generate
incorrect HLSL.

I put the braces for the conditional and loop back in. It's using the line
associated with the loop node for the closing brace, which is a little odd.

Loops generally don't debug very well but this seems to be true when debugging
the disassembly as well, perhaps something to do with loops being unrolled
behind the scene. 

Straight line code seems to work okay.

[dgkoch, 2011-01-16 16:24:47]

Just the disabling d3d9ex that needs to be addressed and then it's LGTM to me.

I would like Nicolas to look at it again, but if you need to get it, we can do
that after the fact as well.

Daniel

http://codereview.appspot.com/3945043/diff/1/src/common/debug.cpp
File src/common/debug.cpp (right):

http://codereview.appspot.com/3945043/diff/1/src/common/debug.cpp#newcode23
src/common/debug.cpp:23: typedef void (WINAPI *BeginEventProc)(D3DCOLOR col,
LPCWSTR wszName);
On 2011/01/13 23:54:57, apatrick1 wrote:
> I used void so it had the same signature as SetMarkerProc() and could be
passed
> to output(). A bit of a hack I know. I can make it int by gratuitous use of
> templates though.

I thought maybe that was the case, but wanted to make sure it wasn't a mistake. 
Using templates to get around it seems like overkill.

http://codereview.appspot.com/3945043/diff/1/src/common/debug.cpp#newcode54
src/common/debug.cpp:54: GetStatus = (GetStatusProc) GetProcAddress(module,
"D3DPERF_GetStatus");
On 2011/01/13 23:54:57, apatrick1 wrote:
> I did a Google search for these functions and found a lot of people on forums
> complaining that the symbol could not be resolved. I think these functions
might
> not have been present in earlier versions of D3D9. Upon reconsideration, I
> prefer ANGLE to fail if it's a really old version on D3D9. It'll reduce my
crash
> reports no end. Static dependency it is.
I like the sound of that. (As long as a good d3d9.dll on WinXP has the entry
points as well - haven't had a chance to verify that myself).

http://codereview.appspot.com/3945043/diff/1/src/common/debug.cpp#newcode63
src/common/debug.cpp:63: int len = vsprintf_s(message, format, vararg);
On 2011/01/13 23:54:57, apatrick1 wrote:
> This is the template version of the function that infers the buffer size from
> the first argument. See the signature for the final two overloads in the your
> link.
ok thanks.

http://codereview.appspot.com/3945043/diff/1/src/common/debug.h
File src/common/debug.h (right):

http://codereview.appspot.com/3945043/diff/1/src/common/debug.h#newcode21
src/common/debug.h:21: extern bool traceOn;
On 2011/01/13 23:54:57, apatrick1 wrote:
> On 2011/01/13 04:22:45, dgkoch wrote:
> > Since this is a global it should be called 'gTraceOn' or something like
that.
> Deleted. It's not needed if the binding to the D3DPERF functions is static.

Maybe not, but you were basing that on the results of GetStatus before..

http://codereview.appspot.com/3945043/diff/10001/src/libEGL/Display.cpp
File src/libEGL/Display.cpp (right):

http://codereview.appspot.com/3945043/diff/10001/src/libEGL/Display.cpp#newco...
src/libEGL/Display.cpp:21: #define ENABLE_D3D9EX 0   // Enables use of the
IDirect3D9Ex interface, when available
I presume this was for debugging and you don't actually want to check this
change in...

[apatrick1, 2011-01-18 20:22:01]

Done. Changes were to debug.* and Display.cpp.

[nicolas, 2011-01-19 15:15:42]

The change at the bottom of glslang.y causes a crash (null dereference) in one
of the conformance tests.

Anyway, I got the chance to test the functionality in PIX and it's brilliant!
With the above little regression fixed it looks good to me.

[apatrick1, 2011-01-21 22:26:55]

I couldn't reproduce the crash by running the conformance tests but I think I
fixed the null dereference. I also made EVENT not output to the trace file in
release builds.

The diff is becoming increasingly unhelpful. The changes were to debug.cpp,
glslang.y and the generated glslang_tab.cpp.

[apatrick1, 2011-01-25 22:29:01]

nicolas, okay to check in?

Thanks,

Al

[nicolas, 2011-01-26 16:51:44]

The crash is fixed. All looks good to me!

[dgkoch, 2011-01-26 17:30:49]

Hi Al,

I spotted a couple of final bugs in some of the preprocessing. 

Note that you'll also want to update BUILD_REVISION in common/version.h to match
the expected svn revision when you check this in as well.

LGTM after those fixes.
Daniel

http://codereview.appspot.com/3945043/diff/33001/src/build_angle.gyp
File src/build_angle.gyp (right):

http://codereview.appspot.com/3945043/diff/33001/src/build_angle.gyp#newcode8
src/build_angle.gyp:8: 'ANGLE_DISABLE_TRACE',
did you also want to add ANGLE_DISABLE_PERF in here?

http://codereview.appspot.com/3945043/diff/33001/src/common/debug.h
File src/common/debug.h (right):

http://codereview.appspot.com/3945043/diff/33001/src/common/debug.h#newcode50
src/common/debug.h:50: #define TRACE(message, ...) (void(0))
should define FIXME not TRACE here

http://codereview.appspot.com/3945043/diff/33001/src/common/debug.h#newcode57
src/common/debug.h:57: #define TRACE(message, ...) (void(0))
should define ERR not TRACE here

http://codereview.appspot.com/3945043/diff/33001/src/common/debug.h#newcode64
src/common/debug.h:64: #define TRACE(message, ...) (void(0))
should define EVENT here not TRACE

[apatrick1, 2011-01-26 19:30:46]

http://codereview.appspot.com/3945043/diff/33001/src/build_angle.gyp
File src/build_angle.gyp (right):

http://codereview.appspot.com/3945043/diff/33001/src/build_angle.gyp#newcode8
src/build_angle.gyp:8: 'ANGLE_DISABLE_TRACE',
For Chrome I was going to try having it on by default so end users can use PIX
to debug WebGL. If there is a performance issue I'll try caching whether PIX it
attached in a static bool in perfActive().

For the vcproj build, I have no preference as to whether the D3DPERF stuff is on
by default.

http://codereview.appspot.com/3945043/diff/33001/src/common/debug.h
File src/common/debug.h (right):

http://codereview.appspot.com/3945043/diff/33001/src/common/debug.h#newcode50
src/common/debug.h:50: #define TRACE(message, ...) (void(0))
On 2011/01/26 17:30:49, dgkoch wrote:
> should define FIXME not TRACE here

Done.

http://codereview.appspot.com/3945043/diff/33001/src/common/debug.h#newcode57
src/common/debug.h:57: #define TRACE(message, ...) (void(0))
On 2011/01/26 17:30:49, dgkoch wrote:
> should define ERR not TRACE here

Done.

http://codereview.appspot.com/3945043/diff/33001/src/common/debug.h#newcode64
src/common/debug.h:64: #define TRACE(message, ...) (void(0))
On 2011/01/26 17:30:49, dgkoch wrote:
> should define EVENT here not TRACE

Done.

[dgkoch, 2011-01-26 19:53:43]

http://codereview.appspot.com/3945043/diff/33001/src/build_angle.gyp
File src/build_angle.gyp (right):

http://codereview.appspot.com/3945043/diff/33001/src/build_angle.gyp#newcode8
src/build_angle.gyp:8: 'ANGLE_DISABLE_TRACE',
On 2011/01/26 19:30:46, apatrick1 wrote:
> For Chrome I was going to try having it on by default so end users can use PIX
> to debug WebGL. If there is a performance issue I'll try caching whether PIX
it
> attached in a static bool in perfActive().
> 
> For the vcproj build, I have no preference as to whether the D3DPERF stuff is
on
> by default.

Ok that's fine as long as it was intentional.  I might suggest using the static
bool for that.  One caveat here is that they might end up debugging the
compositor as well when it is enabled :-)

[apatrick1, 2011-01-26 20:09:18]

On 2011/01/26 19:53:43, dgkoch wrote:
> http://codereview.appspot.com/3945043/diff/33001/src/build_angle.gyp
> File src/build_angle.gyp (right):
> 
> http://codereview.appspot.com/3945043/diff/33001/src/build_angle.gyp#newcode8
> src/build_angle.gyp:8: 'ANGLE_DISABLE_TRACE',
> On 2011/01/26 19:30:46, apatrick1 wrote:
> > For Chrome I was going to try having it on by default so end users can use
PIX
> > to debug WebGL. If there is a performance issue I'll try caching whether PIX
> it
> > attached in a static bool in perfActive().
> > 
> > For the vcproj build, I have no preference as to whether the D3DPERF stuff
is
> on
> > by default.
> 
> Ok that's fine as long as it was intentional.  I might suggest using the
static
> bool for that.  One caveat here is that they might end up debugging the
> compositor as well when it is enabled :-)

I was thinking I would add two more levels of hierarchy for the events. The top
would group by the HTML element id of the issuing WebGL element (or label as
"Compositor"). The next level would be the actual WebGL calls with arguments,
with the corresponding but slightly different ANGLE GLES calls below that.

I'm also going to try and label  the D3D resources with the corresponding GLES
id using this approach, assuming it works in D3D9:

http://blogs.msdn.com/b/chuckw/archive/2010/04/15/object-naming.aspx