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

Issue 2294963002: Submit Compositor Frame from OffscreenCanvas on main (Closed)

Created:
4 years, 3 months ago by xlai (Olivia)
Modified:
4 years, 3 months ago
CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, blink-reviews-platform-graphics_chromium.org, dshwang, yzshen+watch_chromium.org, Stephen Chennney, rwlbuis, krit, drott+blinkwatch_chromium.org, jam, abarth-chromium, dglazkov+blink, Rik, darin-cc_chromium.org, blink-reviews, ajuma+watch_chromium.org, blink-reviews-api_chromium.org, jbroman, pdr+graphicswatchlist_chromium.org, haraken, danakj+watch_chromium.org, Aaron Boodman, f(malita), darin (slow to review)
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Submit Compositor Frame from OffscreenCanvas on main CompositorFrame is to be submitted from OffscreenCanvas from either main or worker thread (but not both at the same time); therefore, another interface "OffscreenCanvasFrameReceiver" is created only when commit() is invoked. This CL follows the class design structure in this diagram: www.lucidchart.com/documents/view/346a55cc-0ab1-48e8-9d34-90f59d36094e Currently, we only submit a Green color frame for simplicity. More CLs will follow to fill in image data to the compositor frame. BUG=563852 Committed: https://crrev.com/94a3a26405be3996569d9006b29d0460bb9e8dca Cr-Commit-Position: refs/heads/master@{#416436}

Patch Set 1 #

Patch Set 2 : Rebase #

Patch Set 3 : Register services #

Patch Set 4 : Rebase + Implement submitCompositorFrame #

Patch Set 5 : Using Ref test fixes layout test problem #

Total comments: 35

Patch Set 6 : Fix #

Patch Set 7 : Further cleanup + modify Owners #

Patch Set 8 : update global interface listing #

Total comments: 6

Patch Set 9 : Fix #

Patch Set 10 : a test to ensure resizing on transferred offscreencanvas has no effect #

Unified diffs Side-by-side diffs Delta from patch set Stats (+285 lines, -47 lines) Patch
M content/browser/renderer_host/OWNERS View 1 2 3 4 5 6 1 chunk +2 lines, -2 lines 0 comments Download
A content/browser/renderer_host/offscreen_canvas_frame_receiver_impl.h View 1 2 3 4 5 6 1 chunk +36 lines, -0 lines 0 comments Download
A content/browser/renderer_host/offscreen_canvas_frame_receiver_impl.cc View 1 2 3 4 5 1 chunk +37 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_process_host_impl.cc View 1 2 2 chunks +3 lines, -0 lines 0 comments Download
M content/content_browser.gypi View 1 1 chunk +2 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-commit-main.html View 1 2 3 4 5 6 7 8 9 1 chunk +11 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-commit-main-expected.html View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferControlToOffscreen.html View 1 2 3 4 5 6 7 8 9 2 chunks +35 lines, -29 lines 0 comments Download
D third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferControlToOffscreen-expected.txt View 1 2 3 4 5 6 7 8 9 1 chunk +0 lines, -13 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-dedicated-worker-expected.txt View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-shared-worker-expected.txt View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.h View 1 3 chunks +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp View 1 2 3 4 5 6 7 8 3 chunks +15 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.h View 1 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp View 1 2 3 4 5 6 7 8 1 chunk +10 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.idl View 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/BUILD.gn View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcher.h View 1 2 3 4 5 1 chunk +22 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.h View 1 2 3 4 5 6 7 8 1 chunk +32 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp View 1 2 3 4 5 1 chunk +55 lines, -0 lines 0 comments Download
M third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom View 1 2 3 4 5 6 7 8 1 chunk +8 lines, -3 lines 0 comments Download

Messages

Total messages: 43 (15 generated)
xlai (Olivia)
Sending this CL to junov@ and danakj@ first before involving other reviewers. junov@: The implementation ...
4 years, 3 months ago (2016-09-02 17:52:38 UTC) #4
Justin Novosad
This is looking good. Good use of *Impl to isolate the dependencies. Lgtm with nits ...
4 years, 3 months ago (2016-09-02 19:11:15 UTC) #5
danakj
https://codereview.chromium.org/2294963002/diff/100001/content/browser/renderer_host/offscreen_canvas_frame_receiver_impl.cc File content/browser/renderer_host/offscreen_canvas_frame_receiver_impl.cc (right): https://codereview.chromium.org/2294963002/diff/100001/content/browser/renderer_host/offscreen_canvas_frame_receiver_impl.cc#newcode33 content/browser/renderer_host/offscreen_canvas_frame_receiver_impl.cc:33: if (!!surface) { you dont need !! for this ...
4 years, 3 months ago (2016-09-02 19:12:43 UTC) #6
Justin Novosad
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp#newcode27 third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:27: void OffscreenCanvasFrameDispatcherImpl::uploadImage(int width, int height) On 2016/09/02 19:12:43, danakj ...
4 years, 3 months ago (2016-09-02 19:20:08 UTC) #7
danakj
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp#newcode27 third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:27: void OffscreenCanvasFrameDispatcherImpl::uploadImage(int width, int height) On 2016/09/02 19:20:08, Justin ...
4 years, 3 months ago (2016-09-02 19:20:59 UTC) #8
Justin Novosad
On 2016/09/02 19:20:59, danakj wrote: > https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp > File > third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp > (right): > > ...
4 years, 3 months ago (2016-09-02 19:21:45 UTC) #9
danakj
On Fri, Sep 2, 2016 at 12:21 PM, <junov@chromium.org> wrote: > On 2016/09/02 19:20:59, danakj ...
4 years, 3 months ago (2016-09-02 19:23:42 UTC) #10
danakj
On Fri, Sep 2, 2016 at 12:21 PM, <junov@chromium.org> wrote: > On 2016/09/02 19:20:59, danakj ...
4 years, 3 months ago (2016-09-02 19:23:45 UTC) #11
Justin Novosad
On 2016/09/02 19:21:45, Justin Novosad wrote: > On 2016/09/02 19:20:59, danakj wrote: > > > ...
4 years, 3 months ago (2016-09-02 19:24:24 UTC) #12
Justin Novosad
LOL. I read your mind :-)
4 years, 3 months ago (2016-09-02 19:25:08 UTC) #13
danakj
On Fri, Sep 2, 2016 at 12:25 PM, <junov@chromium.org> wrote: > LOL. I read your ...
4 years, 3 months ago (2016-09-02 19:26:31 UTC) #14
danakj
On Fri, Sep 2, 2016 at 12:25 PM, <junov@chromium.org> wrote: > LOL. I read your ...
4 years, 3 months ago (2016-09-02 19:32:07 UTC) #15
xlai (Olivia)
https://codereview.chromium.org/2294963002/diff/100001/content/browser/renderer_host/offscreen_canvas_frame_receiver_impl.cc File content/browser/renderer_host/offscreen_canvas_frame_receiver_impl.cc (right): https://codereview.chromium.org/2294963002/diff/100001/content/browser/renderer_host/offscreen_canvas_frame_receiver_impl.cc#newcode33 content/browser/renderer_host/offscreen_canvas_frame_receiver_impl.cc:33: if (!!surface) { On 2016/09/02 19:12:42, danakj wrote: > ...
4 years, 3 months ago (2016-09-02 20:10:07 UTC) #16
xlai (Olivia)
+tsepez@chromium.org: Please review offscreen_canvas_surface.mojom +sievers@chromium.org: Please review content/browser/renderer_host/ danakj@: I've edited the code based on ...
4 years, 3 months ago (2016-09-02 20:56:48 UTC) #18
Tom Sepez
mojom lgtm
4 years, 3 months ago (2016-09-02 21:00:03 UTC) #19
no sievers
On 2016/09/02 20:56:48, xlai (Olivia) wrote: > mailto:+tsepez@chromium.org: Please review offscreen_canvas_surface.mojom > > mailto:+sievers@chromium.org: Please ...
4 years, 3 months ago (2016-09-02 21:22:38 UTC) #23
danakj
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp File third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp (right): https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp#newcode142 third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:142: m_frameDispatcher = wrapUnique(new OffscreenCanvasFrameDispatcherImpl(m_clientId, m_localId, m_nonce)); On 2016/09/02 19:12:42, ...
4 years, 3 months ago (2016-09-02 21:51:01 UTC) #24
danakj
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp File third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp (right): https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp#newcode142 third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:142: m_frameDispatcher = wrapUnique(new OffscreenCanvasFrameDispatcherImpl(m_clientId, m_localId, m_nonce)); On 2016/09/02 21:51:00, ...
4 years, 3 months ago (2016-09-02 21:53:51 UTC) #25
xlai (Olivia)
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp File third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp (right): https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp#newcode142 third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:142: m_frameDispatcher = wrapUnique(new OffscreenCanvasFrameDispatcherImpl(m_clientId, m_localId, m_nonce)); On 2016/09/02 21:53:50, ...
4 years, 3 months ago (2016-09-02 23:41:28 UTC) #26
danakj
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp File third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp (right): https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp#newcode142 third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:142: m_frameDispatcher = wrapUnique(new OffscreenCanvasFrameDispatcherImpl(m_clientId, m_localId, m_nonce)); On 2016/09/02 23:41:28, ...
4 years, 3 months ago (2016-09-03 00:40:43 UTC) #27
xlai (Olivia)
On 2016/09/03 00:40:43, danakj wrote: > > I'm just feeling that a lazy instantiation of ...
4 years, 3 months ago (2016-09-03 01:31:07 UTC) #30
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/2294963002/200001
4 years, 3 months ago (2016-09-03 03:12:15 UTC) #35
commit-bot: I haz the power
Committed patchset #10 (id:200001)
4 years, 3 months ago (2016-09-03 03:17:45 UTC) #37
commit-bot: I haz the power
Patchset 10 (id:??) landed as https://crrev.com/94a3a26405be3996569d9006b29d0460bb9e8dca Cr-Commit-Position: refs/heads/master@{#416436}
4 years, 3 months ago (2016-09-03 03:21:39 UTC) #39
Matt Giuca
A revert of this CL (patchset #10 id:200001) has been created in https://codereview.chromium.org/2313533002/ by mgiuca@chromium.org. ...
4 years, 3 months ago (2016-09-05 04:14:13 UTC) #40
Matt Giuca
On 2016/09/05 04:14:13, Matt Giuca wrote: > A revert of this CL (patchset #10 id:200001) ...
4 years, 3 months ago (2016-09-05 04:22:45 UTC) #41
danakj
On Fri, Sep 2, 2016 at 6:31 PM, <xlai@chromium.org> wrote: > On 2016/09/03 00:40:43, danakj ...
4 years, 3 months ago (2016-09-06 19:08:51 UTC) #42
danakj
4 years, 3 months ago (2016-09-06 19:16:52 UTC) #43
Message was sent while issue was closed.
On Fri, Sep 2, 2016 at 6:31 PM, <xlai@chromium.org> wrote:

> On 2016/09/03 00:40:43, danakj wrote:
> > > I'm just feeling that a lazy instantiation of frameDispatcher is
> better--that
> > > only create new object
> > > when I am sure that I'm gonna use it.
> > >
> > > P.S. the sequence of JavaScript code that users write
> > > 1. var offscreenCanvas = htmlCanvas.transferControlToOffscreen();
> > > 2. worker.postMessage(offscreenCanvas, [offscreenCanvas]);
> > > 3. var ctx2d = offscreenCanvas.getContext("2d");
> > > 4. // do a lot of drawing actions on ctx2d...
> > > 5. ctx2d.commit(); <<-- Here is the point of time when
> m_frameDispatcher
> is
> > > needed.
> >
> > I see. I guess I mostly find it a bit confusing that this class has like
> 3
> > states:
> > - Constructed
> > - Constructed with surface id
> > - Constructed with surface id and dispatcher
> >
> > I was poking at getting rid of the third one by merging it with the 2rd.
> Maybe
> > the 2nd could merge with the 3rd instead, or with the 1st. Anyhow..
> something
> > for another day, this LGTM :)
>
> Thanks for the stamp. Just for post-stamp clarification, for the 3 states
> you've
> mentioned:
> - Constructed:
> yes, OffscreenCanvas can be constructed on its own; in this scenario, we
> don't need surfaceId and dispatcher.
> - Constructed with surface id:
> When users construct OffscreenCanvas by transferring Control from html
> canvas (this step can only happen on
> main thread), it signals something (maybe a commit() is coming soon?). But
> we don't know whether the commit()
> happens in worker thread or main thread, we must grab this last opportunity
> that we are on the main thread to
> get a valid SurfaceId and use it for the SurfaceLayer that's attached to
> html canvas. So now, we got surfaceId.
> - Constructed with surface id and dispatcher
> When commit() happens, we confirm to know whether OffscreenCanvas lives on
> main thread or worker thread.
> Now it is the right time to create FrameDispatcher because we want to
> create
> a mojo channel between browser
> and renderer/worker OR renderer/main. Before this point, we don't know
> that.
>
> Whilst I'm writing, I realize that besides lazy instantiation, a stronger
> reason
> to explain why I create frameDispatcher
> at commit() is that only at this point of time, I know exactly what thread
> my
> new mojo message channel is connected
> to (i.e. whether it is main thread or worker thread).
>

Cool thanks, that makes a lot of sense. Maybe the code could explain this
in method names in some way if you can find a way.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

Powered by Google App Engine
This is Rietveld 408576698