Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(238)

Issue 2653933003: Make stream captures work on canvases that are not in the DOM. (Closed)

Created:
3 years, 11 months ago by Justin Novosad
Modified:
3 years, 10 months ago
CC:
chromium-reviews, blink-reviews-platform-graphics_chromium.org, dshwang, kinuko+watch, emircan+watch+capturefromdom_chromium.org, mcasas+watch+capturefromdom_chromium.org, krit, drott+blinkwatch_chromium.org, blink-reviews-html_chromium.org, jam, Justin Novosad, dglazkov+blink, Rik, darin-cc_chromium.org, blink-reviews, rwlbuis, ajuma+watch_chromium.org, mlamouri+watch-content_chromium.org, jbroman, pdr+graphicswatchlist_chromium.org, haraken, piman+watch_chromium.org, Stephen Chennney, ajuma+watch-canvas_chromium.org, f(malita), danakj+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Make stream captures work on canvases that are not in the DOM. This CL refactors part of the internal workings of 2D canvas in order to decouple the notions animation frame boundaries and paint invalidations. The two used to be considered as one, which caused frame boundaries to stop being processed on object not attached to the DOM due to the lack of paint invalidations. TBR=kbr@chromium.org, esprehn@chromium.org BUG=653548 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2653933003 Cr-Commit-Position: refs/heads/master@{#449845} Committed: https://chromium.googlesource.com/chromium/src/+/2a5eabadc87a335563364fd77061a5d46e05d38a

Patch Set 1 #

Total comments: 10

Patch Set 2 : fix test + review comments #

Total comments: 12

Patch Set 3 : xlai feedback #

Total comments: 1

Patch Set 4 : rebase #

Patch Set 5 : rebase #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+238 lines, -331 lines) Patch
M content/renderer/gpu/render_widget_compositor.cc View 1 2 3 1 chunk +1 line, -4 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-capture-out-of-DOM-element.html View 1 2 1 chunk +39 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/html/HTMLCanvasElement.h View 1 2 2 chunks +6 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp View 1 2 3 2 chunks +40 lines, -43 lines 0 comments Download
M third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h View 1 2 3 5 chunks +9 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp View 1 2 3 3 chunks +26 lines, -1 line 0 comments Download
M third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.h View 1 2 3 5 chunks +5 lines, -9 lines 0 comments Download
M third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp View 1 2 3 4 5 chunks +13 lines, -30 lines 0 comments Download
M third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.h View 1 2 1 chunk +3 lines, -1 line 0 comments Download
M third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp View 1 2 3 4 1 chunk +5 lines, -8 lines 2 comments Download
M third_party/WebKit/Source/platform/graphics/Canvas2DImageBufferSurface.h View 1 2 3 2 chunks +3 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.h View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp View 1 2 3 4 chunks +19 lines, -13 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridgeTest.cpp View 1 2 3 4 4 chunks +5 lines, -9 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/ImageBuffer.h View 1 2 3 4 1 chunk +2 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp View 1 2 3 1 chunk +4 lines, -10 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/ImageBufferClient.h View 1 1 chunk +0 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/ImageBufferSurface.h View 1 2 3 4 1 chunk +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.h View 1 2 3 4 1 chunk +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.cpp View 1 2 3 4 4 chunks +11 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurfaceTest.cpp View 1 2 3 4 3 chunks +39 lines, -175 lines 0 comments Download

Messages

Total messages: 39 (23 generated)
Justin Novosad
PTAL
3 years, 11 months ago (2017-01-25 15:35:47 UTC) #5
xlai (Olivia)
The patch breaks existing media capture layout tests. Please check it up. https://codereview.chromium.org/2653933003/diff/1/third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp ...
3 years, 10 months ago (2017-01-27 22:23:25 UTC) #8
Justin Novosad
New patch https://codereview.chromium.org/2653933003/diff/1/third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp (right): https://codereview.chromium.org/2653933003/diff/1/third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp#newcode120 third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp:120: // an error when CanvasRenderingContext2D::didProcessTask() is invoked ...
3 years, 10 months ago (2017-02-07 21:52:29 UTC) #10
xlai (Olivia)
lgtm with nits. But you still need review from webgl and content/renderer. https://codereview.chromium.org/2653933003/diff/20001/third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-capture-out-of-DOM-element.html File third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-capture-out-of-DOM-element.html ...
3 years, 10 months ago (2017-02-08 17:58:40 UTC) #14
Justin Novosad
https://codereview.chromium.org/2653933003/diff/20001/third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-capture-out-of-DOM-element.html File third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-capture-out-of-DOM-element.html (right): https://codereview.chromium.org/2653933003/diff/20001/third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-capture-out-of-DOM-element.html#newcode15 third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-capture-out-of-DOM-element.html:15: recorder.start(0); On 2017/02/08 17:58:40, xlai (Olivia) wrote: > no ...
3 years, 10 months ago (2017-02-08 18:43:12 UTC) #15
Justin Novosad
zmo or kbr -> modules/WebGL esprehn -> content/renderer
3 years, 10 months ago (2017-02-08 18:47:26 UTC) #17
Justin Novosad
3 years, 10 months ago (2017-02-08 18:47:53 UTC) #19
Justin Novosad
https://codereview.chromium.org/2653933003/diff/40001/content/renderer/gpu/render_widget_compositor.cc File content/renderer/gpu/render_widget_compositor.cc (left): https://codereview.chromium.org/2653933003/diff/40001/content/renderer/gpu/render_widget_compositor.cc#oldcode1116 content/renderer/gpu/render_widget_compositor.cc:1116: NOTREACHED(); FYI: A unit test that fires synthetic GPU ...
3 years, 10 months ago (2017-02-08 18:49:32 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2653933003/40001
3 years, 10 months ago (2017-02-09 19:51:56 UTC) #24
commit-bot: I haz the power
Failed to apply patch for third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp: While running git apply --index -p1; error: patch failed: ...
3 years, 10 months ago (2017-02-09 21:31:33 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2653933003/60001
3 years, 10 months ago (2017-02-11 02:11:26 UTC) #29
commit-bot: I haz the power
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/151742) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, ...
3 years, 10 months ago (2017-02-11 02:14:00 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2653933003/80001
3 years, 10 months ago (2017-02-11 03:01:04 UTC) #34
commit-bot: I haz the power
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/2a5eabadc87a335563364fd77061a5d46e05d38a
3 years, 10 months ago (2017-02-11 05:07:33 UTC) #37
Ken Russell (switch to Gerrit)
Justin, apologies for not getting to this review sooner, but I do have a concern ...
3 years, 10 months ago (2017-02-14 01:31:38 UTC) #38
Justin Novosad
3 years, 10 months ago (2017-02-14 18:36:25 UTC) #39
Message was sent while issue was closed.
https://codereview.chromium.org/2653933003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2653933003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1407:
didDraw(SkIRect::MakeXYWH(0, 0, canvasSize.width(), canvasSize.height()));
On 2017/02/14 01:31:38, Ken Russell wrote:
> This is going to be triggered all the time now, and it registers a "finalize
> frame" task observer in CanvasRenderingContext::didDraw. Task observers are
not
> cheap. I'm concerned about the performance impact of this change in the common
> case, namely that stream capturing is not enabled. What can you say about this
> part of this change?

This does not get called more than once per frame due to the check on 
m_markedCanvasDirty. I added trace instrumentation in a local build and I found
that the cost of CanvasRenderingContext::didDraw is 20us to 30us. Of that cost,
5us to 10us is for registering the task observer.  And the execution of the task
observer handler (didProcessTask), including the cost of de-registering itself
as an observer is 5us.  So in all the additional overhead is 10 to 15
microseconds per frame. I am not sure how reliable this data is since the values
are pretty close to the precision limit of the timer. For reference,
TRACE_EVENT0 inside an empty function is 5us.  So the overhead is pretty much
negligible IMO.

Powered by Google App Engine
This is Rietveld 408576698