| Summary: | CanvasCapture should not generate a frame per each canvas draw command | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||||||||
| Component: | WebCore Misc. | Assignee: | youenn fablet <youennf> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | commit-queue, dino, eric.carlson, ryanhaddad, simon.fraser | ||||||||||||||
| Priority: | P2 | ||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
youenn fablet
2017-03-10 18:19:13 PST
Created attachment 304111 [details]
Patch
Created attachment 304279 [details]
Patch
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>? Created attachment 304292 [details]
Patch
(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! (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 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. Created attachment 304340 [details]
Patch for landing
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 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 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 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? (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. (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? (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. 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. (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. Created attachment 304445 [details]
Patch
(In reply to comment #18) > Created attachment 304445 [details] > Patch Moving the async logic to CanvasCapture. Another review needed then... Created attachment 304469 [details]
Patch for landing
Comment on attachment 304469 [details] Patch for landing Clearing flags on attachment: 304469 Committed r213972: <http://trac.webkit.org/changeset/213972> All reviewed patches have been landed. Closing bug. |