Web Inspector: [META] Instrument shaders for WebGL canvas in the backend

• Component: Web Inspector
• Status: NEW ---

[Matt Baker]

Summary:
Instrument shaders for WebGL canvas in the backend.

The Canvas protocol should be extended to include:

Types:
- ShaderType (Fragment, Vertex)

Commands:
- getProgramInfoLog
- getShaderInfoLog

Events:
- programCreated
- programDeleted
- programLinked
- shaderCompiled

[Radar WebKit Bug Importer]

<rdar://problem/32416490>

[Matt Baker]

Created attachment 316032 [details]
Patch

[Build Bot]

Attachment 316032 [details] did not pass style-queue:


ERROR: Source/WebCore/inspector/InspectorShaderProgram.cpp:47:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Matt Baker]

(In reply to Build Bot from comment #3)
> Attachment 316032 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebCore/inspector/InspectorShaderProgram.cpp:47:  Missing
> spaces around :  [whitespace/init] [4]
> Total errors found: 1 in 22 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

See https://bugs.webkit.org/show_bug.cgi?id=174315.

[Build Bot]

Comment on attachment 316032 [details]
Patch

Attachment 316032 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4157898

New failing tests:
inspector/canvas/shader-programs.html

[Build Bot]

Created attachment 316038 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Jeremy Jones]

Comment on attachment 316032 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316032&action=review

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:1584
> +

Deref of *program. Either validateWebGLObject, null check or ASSERT the pointer is valid.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:1000
> +    if (InspectorCanvasAgent* canvasAgent = instrumentingAgents->inspectorCanvasAgent())

Null check or assert instrumentingAgents before deref.
It looks like it is always null checked before making this call. Consider making the param a ref instead of a pointer.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:1006
> +    if (InspectorCanvasAgent* canvasAgent = instrumentingAgents->inspectorCanvasAgent())

Ditto

> Source/WebCore/inspector/InspectorInstrumentation.cpp:1012
> +    if (InspectorCanvasAgent* canvasAgent = instrumentingAgents->inspectorCanvasAgent())

Ditto.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:1018
> +    if (InspectorCanvasAgent* canvasAgent = instrumentingAgents->inspectorCanvasAgent())

Ditto.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:1024
> +    if (InspectorCanvasAgent* canvasAgent = instrumentingAgents->inspectorCanvasAgent())

Ditto.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:1030
> +    if (InspectorCanvasAgent* canvasAgent = instrumentingAgents->inspectorCanvasAgent())

Ditto.

[Build Bot]

Comment on attachment 316032 [details]
Patch

Attachment 316032 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4157944

New failing tests:
fast/canvas/webgl/program-test.html
webgl/1.0.2/conformance/glsl/misc/shader-with-short-circuiting-operators.html
webgl/1.0.2/conformance/glsl/misc/shader-with-non-reserved-words.html
inspector/canvas/shader-programs.html
fast/canvas/webgl/get-active-test.html
webgl/1.0.2/conformance/programs/program-test.html
webgl/1.0.2/conformance/misc/object-deletion-behaviour.html
webgl/1.0.2/conformance/more/conformance/quickCheckAPI-D_G.html
fast/canvas/webgl/object-deletion-behaviour.html
webgl/1.0.2/conformance/programs/get-active-test.html
webgl/1.0.2/conformance/uniforms/uniform-default-values.html

[Build Bot]

Created attachment 316045 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Devin Rousso]

Comment on attachment 316032 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316032&action=review

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:369
> +    m_identifierToInspectorProgram.set(inspectorProgram->identifier(), WTFMove(inspectorProgram));

NIT: just use programIdentifier:

    m_identifierToInspectorProgram.set(programIdentifier, WTFMove(inspectorProgram));

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:405
> +    if (!logMessages.isEmpty()) {

NIT: I don't think this if check is necessary.

> Source/WebCore/inspector/InspectorCanvasAgent.h:30
> +#include "InspectorShaderProgram.h"

I think you can remove this since you forward declare down below.

> Source/WebCore/inspector/InspectorShaderProgram.cpp:57
> +    ASSERT(context && context->isWebGL());
> +    return static_cast<WebGLRenderingContextBase*>(context);

I think you're better off using the type traits system:

    ASSERT(context && is<WebGLRenderingContextBase>(context);
    return downcast<WebGLRenderingContextBase>(context);

> Source/WebCore/inspector/InspectorShaderProgram.h:48
> +    const String& identifier() { return m_identifier; }
> +    InspectorCanvas& canvas() { return m_canvas; }
> +    WebGLRenderingContextBase* context();
> +    WebGLProgram& program() { return m_program; }
> +    WebGLShader* shaderForType(const String& protocolType);

These should all be const.

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:100
> +    programCreated(canvasIdentifier, programIdentifier)

NIT: please add:

    // Called from WebInspector.CanvasObserver.

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:112
> +        this.dispatchEventToListeners(WebInspector.CanvasManager.Event.ShaderProgramAdded, {program});

I would prefer if these event dispatches were fired from the Canvas that is responsible for the program.  This would make dealing with removing event listeners a lot easier and would allow listeners to not have to worry about getting an unrelated program event fired.

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:169
> +    getProgramInfoLog(program, callback)
> +    {
> +        CanvasAgent.getProgramInfoLog(program.identifier, callback);
> +    }
> +
> +    getShaderInfoLog(program, shaderType, callback)
> +    {
> +        let protocolValue;
> +        if (shaderType === WebInspector.ShaderProgram.ShaderType.Vertex)
> +            protocolValue = CanvasAgent.ShaderType.Vertex;
> +        else if (shaderType === WebInspector.ShaderProgram.ShaderType.Fragment)
> +            protocolValue = CanvasAgent.ShaderType.Fragment;
> +        else
> +            console.error("Unknown shader type: " + shaderType);
> +
> +        CanvasAgent.getShaderInfoLog(program.identifier, protocolValue, callback);
> +    }

Is there a reason that these two functions can't be moved to the ShaderProgram object?

> LayoutTests/inspector/canvas/shader-programs.html:51
> +                    InspectorTest.expectThat(typeof status === "boolean", "Should have boolean compile status.");

This could probably just be an assertion.

> LayoutTests/inspector/canvas/shader-programs.html:73
> +                InspectorTest.expectThat(typeof status === "boolean", "Should have boolean link status.");

Ditto.

> LayoutTests/inspector/canvas/shader-programs.html:144
> +        }

Style: add trailing comma

[Matt Baker]

Comment on attachment 316032 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316032&action=review

>> Source/WebCore/inspector/InspectorCanvasAgent.h:30
>> +#include "InspectorShaderProgram.h"
> 
> I think you can remove this since you forward declare down below.

Actually it's the forward declare that should be removed. The includes are needed in order to compile.

With forward declarations alone:
.../OpenSource/WebKitBuild/Debug/usr/local/include/wtf/RefPtr.h:45:12: error: member access into incomplete type 'WebCore::InspectorCanvas'
.../OpenSource/WebKitBuild/Debug/usr/local/include/wtf/RefPtr.h:69:31: note: in instantiation of function template specialization 'WTF::derefIfNotNull<WebCore::InspectorCanvas>' requested here

>> Source/WebCore/inspector/InspectorInstrumentation.cpp:1000
>> +    if (InspectorCanvasAgent* canvasAgent = instrumentingAgents->inspectorCanvasAgent())
> 
> Null check or assert instrumentingAgents before deref.
> It looks like it is always null checked before making this call. Consider making the param a ref instead of a pointer.

Good catch. This code was originally written before we switched to refs.

>> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:112
>> +        this.dispatchEventToListeners(WebInspector.CanvasManager.Event.ShaderProgramAdded, {program});
> 
> I would prefer if these event dispatches were fired from the Canvas that is responsible for the program.  This would make dealing with removing event listeners a lot easier and would allow listeners to not have to worry about getting an unrelated program event fired.

I chose not to have Canvas manage a collection of programs and their events, since 2D canvas wouldn't be using it, and extending Canvas with Canvas2D and WebGLCanvas seemed overkill. That said, we already have context attributes in Canvas, and this patch added Canvas.prototype.nextShaderProgramDisplayNumber, so canvas already has WebGL stuff.

I'll try to improve this.

>> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:169
>> +    }
> 
> Is there a reason that these two functions can't be moved to the ShaderProgram object?

Model objects don't typically talk to agents directly, and instead go through their manager. However, some models do: CSSCompletions, CSSStyleSheet, DatabaseObject, DOMNode, etc. Since this feels like more of a guideline that a rule, making this change seems like a good idea, especially since it improves the design.

[BJ Burg]

Comment on attachment 316032 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316032&action=review

I don't see any reason why this can't be split into smaller patches. You are unlikely to get thorough code review given the current size. Splitting will also make it more obvious where test coverage is lacking. For example, there is no test of what happens when the canvas is deleted from the page, the user tries to attach multiple programs, or what exactly happens at the protocol level when the canvas context is lost and restored.

Here's a series you could use that would be easier to review, and let you test things separately:

1. Program creation and deletion
2. Shader compilation
3. Attaching/detaching shaders
4. Linking programs
5. Any other error conditions (context lost/restored) if they need extra handling.

Dean/Jeremy, are there any other error conditions / lifetime issues / interactions that we are not thinking about right now?

> Source/WebCore/inspector/InspectorShaderProgram.h:40
> +class InspectorShaderProgram final : public RefCounted<InspectorShaderProgram> {

Please put this in namespace Inspector and rename to simply ShaderProgram, unless there's a reason not to (like template specializations from other namespaces).

Do you think this class need to change/clone for WebGL2 / WebGPU?

[Matt Baker]

(In reply to Brian Burg from comment #12)
> Comment on attachment 316032 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316032&action=review
> 
> I don't see any reason why this can't be split into smaller patches. You are
> unlikely to get thorough code review given the current size. Splitting will
> also make it more obvious where test coverage is lacking. For example, there
> is no test of what happens when the canvas is deleted from the page, the
> user tries to attach multiple programs, or what exactly happens at the
> protocol level when the canvas context is lost and restored.
> 
> Here's a series you could use that would be easier to review, and let you
> test things separately:
> 
> 1. Program creation and deletion
> 2. Shader compilation
> 3. Attaching/detaching shaders
> 4. Linking programs
> 5. Any other error conditions (context lost/restored) if they need extra
> handling.

I'm happy to split this up. We haven't discussed whether to handle lost contexts, so that's beyond the scope of this patch.

> Dean/Jeremy, are there any other error conditions / lifetime issues /
> interactions that we are not thinking about right now?
> 
> > Source/WebCore/inspector/InspectorShaderProgram.h:40
> > +class InspectorShaderProgram final : public RefCounted<InspectorShaderProgram> {
> 
> Please put this in namespace Inspector and rename to simply ShaderProgram,
> unless there's a reason not to (like template specializations from other
> namespaces).
> 
> Do you think this class need to change/clone for WebGL2 / WebGPU?

The frontend ShaderProgram model could work for WebGPU as well. On the backend, InspectorShaderProgram could be made more generic, and support WebGPULibrary and MSL shaders.

Since this will be split up, support for WebGPU can be added now instead of as a follow-up.

[Matt Baker]

(In reply to Brian Burg from comment #12)
> Comment on attachment 316032 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316032&action=review
> 
> I don't see any reason why this can't be split into smaller patches. You are
> unlikely to get thorough code review given the current size. Splitting will
> also make it more obvious where test coverage is lacking. For example, there
> is no test of what happens when the canvas is deleted from the page, the
> user tries to attach multiple programs, or what exactly happens at the
> protocol level when the canvas context is lost and restored.

After more thought, I'm not convinced this patch needs to be split up.

Lost contexts are beyond the scope of this patch. As far as the other scenarios, I can add test coverage.

> Here's a series you could use that would be easier to review, and let you
> test things separately:
> 
> 1. Program creation and deletion
> 2. Shader compilation
Instrumenting compilation is tied with instrumenting attacked/detached. Since the frontend deals in programs, not individual shaders, the frontend only receives compile events for shaders that are attached.
> 3. Attaching/detaching shaders
See above.
> 4. Linking programs
> 5. Any other error conditions (context lost/restored) if they need extra
> handling.
This is beyond the scope of this patch, and isn't something we've discussed much offline.

> > Source/WebCore/inspector/InspectorShaderProgram.h:40
> > +class InspectorShaderProgram final : public RefCounted<InspectorShaderProgram> {
> 
> Please put this in namespace Inspector and rename to simply ShaderProgram,
> unless there's a reason not to (like template specializations from other
> namespaces).

I'm fine with this, but this pattern is applied a bit haphazardly in WebCore. JSC is much more consistent in its use of the Inspector namespace. This would also require changing InspectorCanvas in order to be consistent.

> Do you think this class need to change/clone for WebGL2 / WebGPU?
Should be a follow-up.

[Matt Baker]

The shader-programs.html tests can be split into lifecycle events and compile/link events.

[Matt Baker]

(In reply to Matt Baker from comment #14)
> (In reply to Brian Burg from comment #12)
> > Comment on attachment 316032 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=316032&action=review
> > 
> > I don't see any reason why this can't be split into smaller patches. You are
> > unlikely to get thorough code review given the current size. Splitting will
> > also make it more obvious where test coverage is lacking. For example, there
> > is no test of what happens when the canvas is deleted from the page, the
> > user tries to attach multiple programs, or what exactly happens at the
> > protocol level when the canvas context is lost and restored.
> 
> After more thought, I'm not convinced this patch needs to be split up.

Now I'm convinced.

<https://webkit.org/b/175059> Web Inspector: Instrument WebGLProgram created/deleted
<https://webkit.org/b/175060> Web Inspector: Instrument WebGLProgram link and WebGLShader compile