Bug 174483 – Web Inspector: Record actions performed on WebGLRenderingContext

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

Use the protocol/instrumentation logic created in <https://webkit.org/b/174481> Web Inspector: create protocol functions for recording Canvas contexts

[Devin Rousso]

Created attachment 317858 [details]
Patch

[Build Bot]

This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)

[Devin Rousso]

Created attachment 317906 [details]
Patch

[Devin Rousso]

Created attachment 317910 [details]
Patch

[Devin Rousso]

Created attachment 317913 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 317913 [details]
Patch

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

Nice! There are so many types that are just Strings right now, but I think this makes sense and we can just tackle them as we go.

> Source/WebCore/ChangeLog:18
> +        Modify the generated code for `CallTracingCallback` so that optional/nullable types are
> +        dereferenced if they have a value.

Might want to change "are dereferenced if" to "are only dereferenced if"

> Source/WebCore/inspector/InspectorCanvas.cpp:135
> +    if (is<WebGLRenderingContext>(m_canvas.renderingContext()) && (name == "clear" || name == "drawArrays" || name == "drawElements"))

Lets extract a helper for the list of WebGL actions that require a snapshot:

    static bool webGLActionNeedSnapshot(const String& name)
    {
        return name == "clear"
            || name == "drawArrays"
            || name == "drawElements";
    }

> Source/WebCore/inspector/InspectorCanvas.cpp:150
> +    if (m_actionNeedsSnapshot) {
> +        m_actionNeedsSnapshot->addItem(indexForData(getCanvasContent()));
> +        m_actionNeedsSnapshot = nullptr;
> +    }

This happens in multiple places. It could be a method `appendActionSnapshotIfNeeded()`.

> Source/WebCore/inspector/InspectorCanvas.cpp:506
> +            [&] (ImageData* value) { parametersData->addItem(indexForData(value)); },
> +#if ENABLE(WEBGL)
> +            [&] (const WebGLBuffer*) { parametersData->addItem(indexForData(String("WebGLBuffer"))); },
> +            [&] (const WebGLFramebuffer*) { parametersData->addItem(indexForData(String("WebGLFramebuffer"))); },
> +            [&] (const WebGLProgram*) { parametersData->addItem(indexForData(String("WebGLProgram"))); },
> +            [&] (const WebGLRenderbuffer*) { parametersData->addItem(indexForData(String("WebGLRenderbuffer"))); },
> +            [&] (const WebGLShader*) { parametersData->addItem(indexForData(String("WebGLShader"))); },
> +            [&] (const WebGLTexture*) { parametersData->addItem(indexForData(String("WebGLTexture"))); },
> +            [&] (const WebGLUniformLocation*) { parametersData->addItem(indexForData(String("WebGLUniformLocation"))); },
> +#endif
> +            [&] (const RefPtr<ArrayBuffer>&) { parametersData->addItem(indexForData("BufferDataSource")); },
> +            [&] (const RefPtr<ArrayBufferView>&) { parametersData->addItem(indexForData("BufferDataSource")); },

We should have a FIXME to handle many of these types later. For example we will want to be able to link to Programs/Shaders, View Texture/Buffer data, and UniformLocation seems like a small int/type tuple.

> Source/WebCore/inspector/InspectorCanvas.h:82
> +    String getCanvasContent();

How about "getCanvasContentAsDataURL" or "getCanvasContentAsPNGDataURL"

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:696
> +        "texImage2D",
> +        "texImage2D",

Duplicate

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:700
> +        "texSubImage2D",
> +        "texSubImage2D",

Duplicate

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:378
> +    _generateContentCanvasWebGL(index, options = {})

So unlike 2D Canvas, here we are just showing an <img> with the last "snapshot" (image) from the last draw operation (clear, draw*) on the context? Clever!

Will there be an opportunity to use the Show/Hide Path button on WebGL contexts?

> LayoutTests/inspector/canvas/recording-webgl-expected.txt:398
> +      "actions": [
> +        [
> +          56,
> +          [
> +            1
> +          ],
> +          [
> +            57,
> +            5
> +          ],
> +          58
> +        ]
> +      ]

Yay, an action with a snapshot. Gosh I wonder if we could pretty print this ourselves so its easier to read. For example this would be wonderful given the format:

    {
      "actions": [
        56,
        [1],
        [57, 5],
        58
      ]
    }

> LayoutTests/inspector/canvas/recording-webgl-expected.txt:833
> +      "actions": [
> +        [
> +          115,
> +          [
> +            1,
> +            2,
> +            3
> +          ],
> +          [
> +            116,
> +            5
> +          ],
> +          58
> +        ]
> +      ]
> +    },

Okay, its unfortunate that this snapshot is the same one (58). Because the last was from clear() which means the WebGL context here is still clear.

I think we will want to have a test specifically to show that snapshots progress:

    ctx.clear(); // produces an empty snapshot
    ...
    ctx.drawArrays(); // produces a new snapshot
    ...
    ctx.drawElements(); // produces a new snapshot
    ...
    ctx.clear(); // probably produces the empty snapshot

[Devin Rousso]

Comment on attachment 317913 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:378
>> +    _generateContentCanvasWebGL(index, options = {})
> 
> So unlike 2D Canvas, here we are just showing an <img> with the last "snapshot" (image) from the last draw operation (clear, draw*) on the context? Clever!
> 
> Will there be an opportunity to use the Show/Hide Path button on WebGL contexts?

The concept of Path doesn't exist for WebGL, so we don't show the NavigationItem.

>> LayoutTests/inspector/canvas/recording-webgl-expected.txt:833
>> +    },
> 
> Okay, its unfortunate that this snapshot is the same one (58). Because the last was from clear() which means the WebGL context here is still clear.
> 
> I think we will want to have a test specifically to show that snapshots progress:
> 
>     ctx.clear(); // produces an empty snapshot
>     ...
>     ctx.drawArrays(); // produces a new snapshot
>     ...
>     ctx.drawElements(); // produces a new snapshot
>     ...
>     ctx.clear(); // probably produces the empty snapshot

Good idea =D

[Devin Rousso]

Created attachment 318687 [details]
Patch

[Build Bot]

Comment on attachment 318687 [details]
Patch

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

New failing tests:
inspector/canvas/recording-webgl-snapshots.html

[Build Bot]

Created attachment 318705 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

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

[Matt Baker]

Comment on attachment 318687 [details]
Patch

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

> Source/WebCore/ChangeLog:35
> +        object, as we don't need the value since the preview will be generated by the `toDataURL`

Style: Comma splice. Easier to read as two sentences:

For all non-primitive WebGL objects, send a string corresponding to the object's name.
The value isn't needed since the preview will be generated by the `toDataURL` snapshots taken after every visual action.

> Source/WebInspectorUI/ChangeLog:20
> +        Include an optional 4th parameter `snapshot` that will be sent for visual actions when

Style: 4th -> fourth.

> Source/WebCore/inspector/InspectorCanvas.cpp:106
> +static bool webGLActionNeedSnapshot(const String& name)

This function name is off. What do you think about `shouldSnapshotWebGLAction()`?

> Source/WebCore/inspector/InspectorCanvas.h:112
> +    RefPtr<Inspector::Protocol::Array<Inspector::InspectorValue>> m_actionNeedsSnapshot;

This name sounds like a boolean, not an action. What about `m_actionNeedingSnapshot`? Would be more consistent with the rest of WebKit too (based on a case-sensitive search for "Needing" in WebCore).

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:33
> +        this._payloadSnapshot = snapshot || -1;

I think `this._snapshotIndex` might be clearer. These are all passed as indices to Recording.swizzle, how about renaming them all?

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:124
> +        if (!isNaN(this._payloadSnapshot))

I don't think this will ever be NaN, based on RecordingAction's constructor.

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:37
>          this._previewImageElement = null;

Left off here. Will complete review later.

[Build Bot]

Comment on attachment 318687 [details]
Patch

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

New failing tests:
inspector/canvas/recording-webgl-snapshots.html

[Build Bot]

Created attachment 318707 [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

[Build Bot]

Comment on attachment 318687 [details]
Patch

Attachment 318687 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4356547

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 318710 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Build Bot]

Comment on attachment 318687 [details]
Patch

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

New failing tests:
inspector/canvas/recording-webgl-snapshots.html

[Build Bot]

Created attachment 318713 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

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

[Devin Rousso]

Created attachment 318733 [details]
Patch

[Devin Rousso]

Comment on attachment 318687 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:33
>> +        this._payloadSnapshot = snapshot || -1;
> 
> I think `this._snapshotIndex` might be clearer. These are all passed as indices to Recording.swizzle, how about renaming them all?

This only really applies to name and snapshot, as parameters and trace are both arrays.  Furthermore, the items in parameters *might* be a swizzle index and they might also just be a regular number.  I'd rather keep "payload" somewhere in the name to keep it clear that these values are from the JSON payload/import.

[Devin Rousso]

Created attachment 318793 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/34040722>

[Devin Rousso]

Created attachment 318945 [details]
Patch

[Matt Baker]

Comment on attachment 318945 [details]
Patch

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

r=me, with nites/style stuff.

> Source/WebCore/bindings/scripts/test/JS/JSTestCallTracer.cpp:428
> +    UNUSED_PARAM(throwScope);

Both of these are used below.

> Source/WebCore/bindings/scripts/test/JS/JSTestCallTracer.cpp:452
> +    UNUSED_PARAM(throwScope);

Ditto.

> Source/WebCore/bindings/scripts/test/JS/JSTestCallTracer.cpp:474
> +    UNUSED_PARAM(throwScope);

Ditto.

> Source/WebCore/inspector/InspectorCanvas.cpp:447
> +        WebGLRenderingContextBase* contextWebGLBase = downcast<WebGLRenderingContextBase>(canvasRenderingContext);

Style: the return type is specified in the template parameter, so auto* would be fine here.

> Source/WebInspectorUI/UserInterface/Models/Recording.js:173
> +                // FIXME: regenerate the WebGL objects from the sent data instead of using the placeholder string.

If you're still considering this enhancement, file a follow-up and add a link here.

> Source/WebInspectorUI/UserInterface/Models/Recording.js:194
> +                        this._swizzle[index][type] = new ImageData(new Uint8ClampedArray(data[0]), parseInt(data[1]), parseInt(data[2]));

Might be nice to break out the array accesses into consts, so we know what the ImageData parameters mean.

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:409
> +            } else if (actions[visualIndex] instanceof WI.RecordingInitialStateAction)

Should just be an `if`, since you return inside the branch above.

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:420
> +        if (options.onCompleteCallback)

I realize this was already in place from a previous patch, but generally we don't prefix event handlers with "on". What about `actionCompletedCallback`? It also better describes the purpose of the callback.

> LayoutTests/inspector/canvas/recording-webgl-snapshots.html:119
> +            CanvasAgent.requestRecording(canvases[0].identifier, singleFrame, (error) => {

Neat, I didn't know we could record a single frame!

> LayoutTests/inspector/canvas/recording-webgl.html:53
> +function ignoreException(f){

Variable `f` should have a better name.

> LayoutTests/inspector/canvas/recording-webgl.html:70
> +    let frames = [

Aren't these "actions"?

> LayoutTests/inspector/canvas/recording-webgl.html:499
> +    function f() {

This function should have a better name.

[Joseph Pecoraro]

Comment on attachment 318945 [details]
Patch

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

>> Source/WebCore/bindings/scripts/test/JS/JSTestCallTracer.cpp:428
>> +    UNUSED_PARAM(throwScope);
> 
> Both of these are used below.

This is generated code. The generator probably generates this for simplicity since it compiles to nothing we probably do it to keep the generator simple.

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:-474
> -    // <https://webkit.org/b/174483> Web Inspector: Record actions performed on WebGLRenderingContext

We should file a new bug for WebGL2. It seems that would be pretty similar to this but WebGL2RenderingContext.

[Devin Rousso]

Comment on attachment 318945 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/Recording.js:194
>> +                        this._swizzle[index][type] = new ImageData(new Uint8ClampedArray(data[0]), parseInt(data[1]), parseInt(data[2]));
> 
> Might be nice to break out the array accesses into consts, so we know what the ImageData parameters mean.

I think that can be determined by what the parameters for ImageData expect.

>> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:409
>> +            } else if (actions[visualIndex] instanceof WI.RecordingInitialStateAction)
> 
> Should just be an `if`, since you return inside the branch above.

Oops.  Good catch!

>> LayoutTests/inspector/canvas/recording-webgl.html:70
>> +    let frames = [
> 
> Aren't these "actions"?

So each of these will be called in a separate requestAnimationFrame().  The idea is that every unique function/getter/setter gets its own frame so that it's easier to distinguish each grouping of recorded actions.  This pattern follows what Canvas-2D uses.

[Devin Rousso]

Created attachment 319152 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 319152 [details]
Patch

Clearing flags on attachment: 319152

Committed r221232: <http://trac.webkit.org/changeset/221232>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.