169498 – CanvasCapture should not generate a frame per each canvas draw command

youenn fablet

Reported 2017-03-10 18:19:13 PST

CanvasCapture is generating one frame per draw command.

youenn fablet

Comment 1 2017-03-10 18:20:26 PST

Created attachment 304111 [details] Patch

youenn fablet

Comment 2 2017-03-13 11:49:17 PDT

Created attachment 304279 [details] Patch

Eric Carlson

Comment 3 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>?

youenn fablet

Comment 4 2017-03-13 13:44:36 PDT

Created attachment 304292 [details] Patch

youenn fablet

Comment 5 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!

youenn fablet

Comment 6 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?

Eric Carlson

Comment 7 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.

youenn fablet

Comment 8 2017-03-13 20:15:41 PDT

Created attachment 304340 [details] Patch for landing

WebKit Commit Bot

Comment 9 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

WebKit Commit Bot

Comment 10 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

Simon Fraser (smfr)

Comment 11 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.

Simon Fraser (smfr)

Comment 12 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?

youenn fablet

Comment 13 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.

Simon Fraser (smfr)

Comment 14 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?

youenn fablet

Comment 15 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.

Simon Fraser (smfr)

Comment 16 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.

youenn fablet

Comment 17 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.

youenn fablet

Comment 18 2017-03-14 16:59:22 PDT

Created attachment 304445 [details] Patch

youenn fablet

Comment 19 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...

youenn fablet

Comment 20 2017-03-14 21:39:01 PDT

Created attachment 304469 [details] Patch for landing

WebKit Commit Bot

Comment 21 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>

WebKit Commit Bot

Comment 22 2017-03-14 22:21:16 PDT

All reviewed patches have been landed. Closing bug.

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
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.