Bug 169498 - CanvasCapture should not generate a frame per each canvas draw command
Summary: CanvasCapture should not generate a frame per each canvas draw command
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-10 18:19 PST by youenn fablet
Modified: 2017-03-14 22:21 PDT (History)
5 users (show)

See Also:


Attachments
Patch (10.44 KB, patch)
2017-03-10 18:20 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (10.83 KB, patch)
2017-03-13 11:49 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (11.27 KB, patch)
2017-03-13 13:44 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (11.36 KB, patch)
2017-03-13 20:15 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (12.67 KB, patch)
2017-03-14 16:59 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (12.72 KB, patch)
2017-03-14 21:39 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2017-03-10 18:19:13 PST
CanvasCapture is generating one frame per draw command.
Comment 1 youenn fablet 2017-03-10 18:20:26 PST
Created attachment 304111 [details]
Patch
Comment 2 youenn fablet 2017-03-13 11:49:17 PDT
Created attachment 304279 [details]
Patch
Comment 3 Eric Carlson 2017-03-13 12:15:54 PDT
Comment on attachment 304279 [details]
Patch

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

> LayoutTests/ChangeLog:9
> +        * fast/mediastream/captureStream/canvas2d-heavy-drawing-expected.txt: Added.

Looks like you forgot to include the results.

> LayoutTests/fast/mediastream/captureStream/canvas2d-heavy-drawing.html:6
> +    <head>
> +        <canvas id="canvas1" width=100px height=100px></canvas>
> +        <video id="video" autoplay width=100px height=100px></video>
> +        <canvas id="canvas2" width=100px height=100px></canvas>

Why are these in the <head>?
Comment 4 youenn fablet 2017-03-13 13:44:36 PDT
Created attachment 304292 [details]
Patch
Comment 5 youenn fablet 2017-03-13 13:44:47 PDT
(In reply to comment #3)
> Comment on attachment 304279 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=304279&action=review
> 
> > LayoutTests/ChangeLog:9
> > +        * fast/mediastream/captureStream/canvas2d-heavy-drawing-expected.txt: Added.
> 
> Looks like you forgot to include the results.
> 
> > LayoutTests/fast/mediastream/captureStream/canvas2d-heavy-drawing.html:6
> > +    <head>
> > +        <canvas id="canvas1" width=100px height=100px></canvas>
> > +        <video id="video" autoplay width=100px height=100px></video>
> > +        <canvas id="canvas2" width=100px height=100px></canvas>
> 
> Why are these in the <head>?

Thanks, done!
Comment 6 youenn fablet 2017-03-13 13:45:12 PDT
(In reply to comment #3)
> Comment on attachment 304279 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=304279&action=review
> 
> > LayoutTests/ChangeLog:9
> > +        * fast/mediastream/captureStream/canvas2d-heavy-drawing-expected.txt: Added.

But, why is it that the EWS bots did not catch this?
Comment 7 Eric Carlson 2017-03-13 16:36:34 PDT
Comment on attachment 304292 [details]
Patch

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

> LayoutTests/fast/mediastream/captureStream/canvas2d-heavy-drawing.html:24
> +    	if (window.internals) {
> +             assert_true((internals.canvasChangedCount - previousNumberOfCanvasChanged) <= 1);
> +             previousNumberOfCanvasChanged = internals.canvasChangedCount;
> +        }
> +        canvas2.getContext("2d").drawImage(video, 0 ,0);
> +        assert_array_equals(canvas2.getContext("2d").getImageData(0 ,0, 100, 100), canvas2.getContext("2d").getImageData(0, 0, 100, 100));

Nit: indentation.

> LayoutTests/fast/mediastream/captureStream/canvas2d-heavy-drawing.html:37
> +    	    previousNumberOfCanvasChanged = internals.canvasChangedCount;

Ditto.

> LayoutTests/fast/mediastream/captureStream/canvas2d-heavy-drawing.html:53
> +    for (var i = 0 ; i < 100; ++i)
> +	context.fillRect(0, 0, 100, 100);

Ditto.
Comment 8 youenn fablet 2017-03-13 20:15:41 PDT
Created attachment 304340 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2017-03-13 20:38:57 PDT
Comment on attachment 304340 [details]
Patch for landing

Rejecting attachment 304340 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 304340, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
4f1 M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
fatal: unable to connect to git.webkit.org:
git.webkit.org[0: 54.190.50.177]: errno=Operation timed out

Current branch master is up to date.

Full output: http://webkit-queues.webkit.org/results/3314280
Comment 10 WebKit Commit Bot 2017-03-13 21:08:11 PDT
Comment on attachment 304340 [details]
Patch for landing

Rejecting attachment 304340 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 304340, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
4f1 M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
fatal: unable to connect to git.webkit.org:
git.webkit.org[0: 54.190.50.177]: errno=Operation timed out

Current branch master is up to date.

Full output: http://webkit-queues.webkit.org/results/3314968
Comment 11 Simon Fraser (smfr) 2017-03-13 21:34:35 PDT
Comment on attachment 304340 [details]
Patch for landing

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

> Source/WebCore/html/HTMLCanvasElement.cpp:370
> +void HTMLCanvasElement::notifyObserversCanvasChanged(const FloatRect& rect)
> +{
> +    if (!m_observers.size())
> +        return;
> +
> +    m_changedRect = unionRect(m_changedRect, rect);
> +    if (m_canvasChangedTimer.isActive())
> +        return;
> +    m_canvasChangedTimer.startOneShot(0);

Doesn't this make the canvasChanged API now asynchronous for all observers? That looks like it may affect the behavior of CSSCanvasValue::canvasChanged(), which could be web-observable via -webkit-canvas.
Comment 12 Simon Fraser (smfr) 2017-03-13 21:35:17 PDT
Comment on attachment 304340 [details]
Patch for landing

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

> Source/WebCore/ChangeLog:3
> +        CanvasCapture is generating too many frames

This is too vague. Too many per second? More than one per screen update?
Comment 13 youenn fablet 2017-03-13 22:07:09 PDT
(In reply to comment #11)
> Comment on attachment 304340 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=304340&action=review
> 
> > Source/WebCore/html/HTMLCanvasElement.cpp:370
> > +void HTMLCanvasElement::notifyObserversCanvasChanged(const FloatRect& rect)
> > +{
> > +    if (!m_observers.size())
> > +        return;
> > +
> > +    m_changedRect = unionRect(m_changedRect, rect);
> > +    if (m_canvasChangedTimer.isActive())
> > +        return;
> > +    m_canvasChangedTimer.startOneShot(0);
> 
> Doesn't this make the canvasChanged API now asynchronous for all observers?
> That looks like it may affect the behavior of
> CSSCanvasValue::canvasChanged(), which could be web-observable via
> -webkit-canvas.

Right, the idea is to be closer to what is happening on the screen.
Ideally, we would like to call observers whenever the canvas compositing happens.

The patch seems good for -webkit-canvas too.
I am not sure how I can add a test for it though.

(In reply to comment #12)
> Comment on attachment 304340 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=304340&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        CanvasCapture is generating too many frames
> 
> This is too vague. Too many per second? More than one per screen update?

I'll beef up the change log.
Comment 14 Simon Fraser (smfr) 2017-03-13 22:17:21 PDT
(In reply to comment #13)
> (In reply to comment #11)
> > Comment on attachment 304340 [details]
> > Patch for landing
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=304340&action=review
> > 
> > > Source/WebCore/html/HTMLCanvasElement.cpp:370
> > > +void HTMLCanvasElement::notifyObserversCanvasChanged(const FloatRect& rect)
> > > +{
> > > +    if (!m_observers.size())
> > > +        return;
> > > +
> > > +    m_changedRect = unionRect(m_changedRect, rect);
> > > +    if (m_canvasChangedTimer.isActive())
> > > +        return;
> > > +    m_canvasChangedTimer.startOneShot(0);
> > 
> > Doesn't this make the canvasChanged API now asynchronous for all observers?
> > That looks like it may affect the behavior of
> > CSSCanvasValue::canvasChanged(), which could be web-observable via
> > -webkit-canvas.
> 
> Right, the idea is to be closer to what is happening on the screen.
> Ideally, we would like to call observers whenever the canvas compositing
> happens.
> 
> The patch seems good for -webkit-canvas too.
> I am not sure how I can add a test for it though.

I'm concerned that changes made to a -webkit-canvas (see https://webkit.org/blog/176/css-canvas-drawing/) will be one frame behind after this changes.

Can you do away with the one-shot timer in HTMLCanvasElement, and just do the work when painting happens?
Comment 15 youenn fablet 2017-03-13 22:38:47 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #11)
> > > Comment on attachment 304340 [details]
> > > Patch for landing
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=304340&action=review
> > > 
> > > > Source/WebCore/html/HTMLCanvasElement.cpp:370
> > > > +void HTMLCanvasElement::notifyObserversCanvasChanged(const FloatRect& rect)
> > > > +{
> > > > +    if (!m_observers.size())
> > > > +        return;
> > > > +
> > > > +    m_changedRect = unionRect(m_changedRect, rect);
> > > > +    if (m_canvasChangedTimer.isActive())
> > > > +        return;
> > > > +    m_canvasChangedTimer.startOneShot(0);
> > > 
> > > Doesn't this make the canvasChanged API now asynchronous for all observers?
> > > That looks like it may affect the behavior of
> > > CSSCanvasValue::canvasChanged(), which could be web-observable via
> > > -webkit-canvas.
> > 
> > Right, the idea is to be closer to what is happening on the screen.
> > Ideally, we would like to call observers whenever the canvas compositing
> > happens.
> > 
> > The patch seems good for -webkit-canvas too.
> > I am not sure how I can add a test for it though.
> 
> I'm concerned that changes made to a -webkit-canvas (see
> https://webkit.org/blog/176/css-canvas-drawing/) will be one frame behind
> after this changes.

I see.
If the risk to break web sites is important, the simplest approach is to put the timer logic in CanvasCapture and not touch --webkit-canvas behavior.

> Can you do away with the one-shot timer in HTMLCanvasElement, and just do
> the work when painting happens?

We only want to call observers when canvas change, not for other repainting reasons.
I am not very familiar with the rendering code. From looking at the code, it was not super straightforward to do that. Not sure I will have time to precisely study that, although that would probably be a perf improvement for --webkit-canvas.
Comment 16 Simon Fraser (smfr) 2017-03-14 11:59:48 PDT
Another consideration here. I don't think you should ever encode a frame for WebRTC that the sender cannot see locally. In other words, you should never send frames that are not displayed on the sender's screen, so that suggests linking capture to display. 

If WebRTC can use offscreen canvases then we'll have to come up with something else.
Comment 17 youenn fablet 2017-03-14 12:43:18 PDT
(In reply to comment #16)
> Another consideration here. I don't think you should ever encode a frame for
> WebRTC that the sender cannot see locally. In other words, you should never
> send frames that are not displayed on the sender's screen, so that suggests
> linking capture to display. 
> 
> If WebRTC can use offscreen canvases then we'll have to come up with
> something else.

captureStream can be applied to offscreen canvases.
Comment 18 youenn fablet 2017-03-14 16:59:22 PDT
Created attachment 304445 [details]
Patch
Comment 19 youenn fablet 2017-03-14 17:00:25 PDT
(In reply to comment #18)
> Created attachment 304445 [details]
> Patch

Moving the async logic to CanvasCapture. Another review needed then...
Comment 20 youenn fablet 2017-03-14 21:39:01 PDT
Created attachment 304469 [details]
Patch for landing
Comment 21 WebKit Commit Bot 2017-03-14 22:21:09 PDT
Comment on attachment 304469 [details]
Patch for landing

Clearing flags on attachment: 304469

Committed r213972: <http://trac.webkit.org/changeset/213972>
Comment 22 WebKit Commit Bot 2017-03-14 22:21:16 PDT
All reviewed patches have been landed.  Closing bug.