Bug 178185

Summary: Web Inspector: Canvas tab: show detailed status during canvas recording
Product: WebKit Reporter: Matt Baker <mattbaker>
Component: Web InspectorAssignee: Devin Rousso <drousso>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, buildbot, commit-queue, drousso, inspector-bugzilla-changes, joepeck, keith_miller, mark.lam, msaboff, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 173807, 175485    
Attachments:
Description Flags
[Image] Waiting for frames
none
[Image] Live frame count/buffer size
none
Patch
none
Patch
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
Patch
none
Patch none

Description Matt Baker 2017-10-11 13:09:23 PDT
Summary:
Show detailed status during canvas recording.

Right now it's impossible to tell what's going on during a recording, or whether any data is being recorded. We should add a progress event to fire after every frame, throttled to a maximum rate (maybe 1/sec):

This will require adding an event to Canvas.json:

{
    "name": "recordingProgress",
    "parameters": [
        { "name": "canvasId", "$ref": "CanvasId" },
        { "name": "frameCount", "type": "integer" },
        { "name": "bufferUsed", "type": "number" }
    ]
}

See attached mockups for proposed UI.
Comment 1 Radar WebKit Bug Importer 2017-10-11 13:09:35 PDT
<rdar://problem/34939862>
Comment 2 Matt Baker 2017-10-11 13:10:37 PDT
Created attachment 323451 [details]
[Image] Waiting for frames
Comment 3 Matt Baker 2017-10-11 13:11:36 PDT
Created attachment 323452 [details]
[Image] Live frame count/buffer size
Comment 4 Brian Burg 2017-10-12 20:10:45 PDT
Looks cool.
Comment 5 Devin Rousso 2017-11-06 15:51:03 PST
One thing I think we may want to do here is send the Recording::Frame protocol objects with each progress event.  This way, we can distribute the work of parsing the entire JSON structure, instead of all at once at the very end.

    {
        "name": "recordingProgress",
        "parameters": [
            { "name": "canvasId", "$ref": "CanvasId" },
            { "name": "frames", "type": "array", "items": { "$ref": "Recording.Frame" }},
            { "name": "bufferUsed", "type": "number" }
        ]
    }

Then in the frontend all we have to do is concatenate each of these calls together to pass into `_frames` on WI.Recording.  We'd also modify Recording::Recording so that it doesn't have a `frames` member.
Comment 6 Devin Rousso 2017-11-07 14:31:05 PST
Created attachment 326262 [details]
Patch
Comment 7 Devin Rousso 2017-11-07 14:50:32 PST
Created attachment 326266 [details]
Patch

Minor style and string adjustments
Comment 8 Build Bot 2017-11-07 19:27:38 PST
Comment on attachment 326266 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/streams/readable-byte-streams/properties.serviceworker.https.html
Comment 9 Build Bot 2017-11-07 19:27:39 PST
Created attachment 326287 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 10 Brian Burg 2017-11-08 13:27:45 PST
Comment on attachment 326266 [details]
Patch

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

r- because backend changes don't seem to be included. That would also imply new tests or improving existing ones to log the progress events. =)

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1016
> +localizedStrings["Waiting for frames..."] = "Waiting for frames...";

We typically use '…' such as in "Edit Breakpoint…" Please fix that.

> Source/WebInspectorUI/UserInterface/Protocol/CanvasObserver.js:50
> +    recordingProgress(canvasId, frames, bufferUsed)

You need to include the backend and protocol changes for this in the patch. And add tests...

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:271
> +        this._recordingProgressElement.textContent = WI.UIString("Waiting for frames...");

Nit: …
Comment 11 Devin Rousso 2017-11-08 14:34:37 PST
Created attachment 326378 [details]
Patch

Oops.  Ran webkit-patch from the wrong directory :P
Comment 12 Brian Burg 2017-11-09 10:29:57 PST
Comment on attachment 326378 [details]
Patch

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

r- for now but overall it looks good. I'd like some clarity on what bufferUsed means, and see that it's exercised in the updated tests if it is deterministic.

> Source/WebCore/ChangeLog:13
> +        After each recorded frame, fire a progress event with the recorded frames as data. This will

If this event fires for every frame, why does it include an array of frames? I prefer the array approach since it lets us use a different coalescing strategy without changing the protocol or frontend. But maybe we should adjust this comment and say something like this in the protocol documentation.

> Source/JavaScriptCore/inspector/protocol/Canvas.json:191
> +                { "name": "bufferUsed", "type": "integer" }

This needs explanation. What is the unit of measure? is it the amount used by the reported frames, the amount used by the backend before/after releasing the frames, ??

> Source/WebCore/inspector/InspectorCanvas.cpp:118
> +    return m_frames;

Please write this as !!m_frames for clarity

> Source/WebCore/inspector/InspectorCanvas.h:65
> +    RefPtr<Inspector::Protocol::Recording::InitialState>&& releaseInitialState() WARN_UNUSED_RETURN;

What does this do?

> Source/WebCore/inspector/InspectorCanvas.h:74
> +    long bufferUsed() const { return m_bufferUsed; }

Ditto the above comment in the change log. This could use a better name, like bufferCapacityInBytes or whatever the semantics are.

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:163
> +            bufferUsed,

Ditto.

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:124
> +            if (context.getImageData)

Pardon the ignorance but are these different cases intended for 2d vs webgl? Would it be clearer to switch on the context type? And then what is the last fallback for.

> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.css:139
> +    height: calc(var(--preview-height) - 1em - (2 * var(--progress-padding)));

Wow. Yo might want to translate this into English.

> LayoutTests/inspector/canvas/resources/recording-utilities.js:109
> +        let frameCount = 0;

Shouldn't we be checking the size of the frames somewhere (bufferUsed)? Or is this not deterministic enough to put in a test?
Comment 13 Devin Rousso 2017-11-09 11:27:03 PST
Comment on attachment 326378 [details]
Patch

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

>> Source/JavaScriptCore/inspector/protocol/Canvas.json:191
>> +                { "name": "bufferUsed", "type": "integer" }
> 
> This needs explanation. What is the unit of measure? is it the amount used by the reported frames, the amount used by the backend before/after releasing the frames, ??

This represents the total number of bytes used by the recording so far.  We default this to 100MB, but the protocol does allow the user to specify a smaller amount.

>> Source/WebCore/inspector/InspectorCanvas.h:65
>> +    RefPtr<Inspector::Protocol::Recording::InitialState>&& releaseInitialState() WARN_UNUSED_RETURN;
> 
> What does this do?

It throws a compile error if callers of this function don't save the return value =D

>> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:124
>> +            if (context.getImageData)
> 
> Pardon the ignorance but are these different cases intended for 2d vs webgl? Would it be clearer to switch on the context type? And then what is the last fallback for.

I thought about switching on the context type, but I felt that this was somewhat "safer".  I can switch it.

The last case is for if we have a WebGPU canvas, or if/when we support Bitmap contexts, so that we won't have to change this much.  It should have an assertion though, so I'll add it.

>> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.css:139
>> +    height: calc(var(--preview-height) - 1em - (2 * var(--progress-padding)));
> 
> Wow. Yo might want to translate this into English.

I wanted to keep the overall size of the CanvasContentView the same after adding the progress info so that the flexbox still looks nice.  This just subtracts from the previously set height (280px) the height of the text (1em) and the padding of the progress element (2 * --progress-padding, for top and bottom).  I'll add a comment inline.

>> LayoutTests/inspector/canvas/resources/recording-utilities.js:109
>> +        let frameCount = 0;
> 
> Shouldn't we be checking the size of the frames somewhere (bufferUsed)? Or is this not deterministic enough to put in a test?

It should be consistent, but I think it's more important that we confirm that it increases with each event rather than check the exact value.  I'll add something to the tests.
Comment 14 Devin Rousso 2017-11-09 18:07:14 PST
Created attachment 326523 [details]
Patch
Comment 15 Brian Burg 2017-11-10 09:12:39 PST
Comment on attachment 326523 [details]
Patch

r=me. We can move forward with this. I think we might want to revisit or remove the recording size limit, since we don't actually retain everything in the backend. Let's get some user feedback, if people want longer recordings I think that it would be reasonable to remove the restriction.
Comment 16 WebKit Commit Bot 2017-11-11 01:22:11 PST
Comment on attachment 326523 [details]
Patch

Clearing flags on attachment: 326523

Committed r224726: <https://trac.webkit.org/changeset/224726>
Comment 17 WebKit Commit Bot 2017-11-11 01:22:13 PST
All reviewed patches have been landed.  Closing bug.