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}
4 years, 3 months ago
(2016-09-01 21:21:10 UTC)
#1
Patchset #3 (id:40001) has been deleted
xlai (Olivia)
Description was changed from ========== Submit Compositor Frame from OffscreenCanvas [not for commit yet] COMMIT=false ...
4 years, 3 months ago
(2016-09-02 17:48:11 UTC)
#2
Description was changed from
==========
Submit Compositor Frame from OffscreenCanvas
[not for commit yet]
COMMIT=false
BUG=563852
==========
to
==========
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.
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
==========
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
Sending this CL to junov@ and danakj@ first before involving other reviewers.
junov@: The implementation follows the design
(https://www.lucidchart.com/documents/view/346a55cc-0ab1-48e8-9d34-90f59d36094e)
here. Please take a look at the overall structure. Still have some troubles on
the layout test (green color can be seen on screen but calling getImageData on
canvas will not get anything).
danakj@: Please pay special attention to the block of code about creating a
Green color compositorFrame in
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp.
Are they right?
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
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
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
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
(right):
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:27:
void OffscreenCanvasFrameDispatcherImpl::uploadImage(int width, int height)
On 2016/09/02 19:12:43, danakj wrote:
> I thought width and height never change, why arent these part of construction?
This is just temporary. Once we start pushing actual frames the args to this
method will probably be SkImage or some other vehicle that carries size info, so
no need to store width and height in this class IMHO.
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
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
(right):
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:27:
void OffscreenCanvasFrameDispatcherImpl::uploadImage(int width, int height)
On 2016/09/02 19:20:08, Justin Novosad wrote:
> On 2016/09/02 19:12:43, danakj wrote:
> > I thought width and height never change, why arent these part of
construction?
>
> This is just temporary. Once we start pushing actual frames the args to this
> method will probably be SkImage or some other vehicle that carries size info,
so
> no need to store width and height in this class IMHO.
Do sizes change? Because then we have to deal with making new surface ids and
stuff.
4 years, 3 months ago
(2016-09-02 19:21:45 UTC)
#9
On 2016/09/02 19:20:59, danakj wrote:
>
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Sou...
> File
>
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
> (right):
>
>
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:27:
> void OffscreenCanvasFrameDispatcherImpl::uploadImage(int width, int height)
> On 2016/09/02 19:20:08, Justin Novosad wrote:
> > On 2016/09/02 19:12:43, danakj wrote:
> > > I thought width and height never change, why arent these part of
> construction?
> >
> > This is just temporary. Once we start pushing actual frames the args to this
> > method will probably be SkImage or some other vehicle that carries size
info,
> so
> > no need to store width and height in this class IMHO.
>
> Do sizes change? Because then we have to deal with making new surface ids and
> stuff.
Nope, the API does not allow it.
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
On Fri, Sep 2, 2016 at 12:21 PM, <junov@chromium.org> wrote:
> On 2016/09/02 19:20:59, danakj wrote:
> >
> https://codereview.chromium.org/2294963002/diff/100001/
> third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcher
> Impl.cpp
> > File
> >
> third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcher
> Impl.cpp
> > (right):
> >
> >
> https://codereview.chromium.org/2294963002/diff/100001/
> third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcher
> Impl.cpp#newcode27
> >
> third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcher
> Impl.cpp:27:
> > void OffscreenCanvasFrameDispatcherImpl::uploadImage(int width, int
> height)
> > On 2016/09/02 19:20:08, Justin Novosad wrote:
> > > On 2016/09/02 19:12:43, danakj wrote:
> > > > I thought width and height never change, why arent these part of
> > construction?
> > >
> > > This is just temporary. Once we start pushing actual frames the args
> to this
> > > method will probably be SkImage or some other vehicle that carries size
> info,
> > so
> > > no need to store width and height in this class IMHO.
> >
> > Do sizes change? Because then we have to deal with making new surface
> ids and
> > stuff.
>
> Nope, the API does not allow it.
>
Cool. Then my pref would be to give size at construction, store them as
const on the class, and DCHECK that SkImages given to it match the size.
--
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.
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
On Fri, Sep 2, 2016 at 12:21 PM, <junov@chromium.org> wrote:
> On 2016/09/02 19:20:59, danakj wrote:
> >
> https://codereview.chromium.org/2294963002/diff/100001/
> third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcher
> Impl.cpp
> > File
> >
> third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcher
> Impl.cpp
> > (right):
> >
> >
> https://codereview.chromium.org/2294963002/diff/100001/
> third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcher
> Impl.cpp#newcode27
> >
> third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcher
> Impl.cpp:27:
> > void OffscreenCanvasFrameDispatcherImpl::uploadImage(int width, int
> height)
> > On 2016/09/02 19:20:08, Justin Novosad wrote:
> > > On 2016/09/02 19:12:43, danakj wrote:
> > > > I thought width and height never change, why arent these part of
> > construction?
> > >
> > > This is just temporary. Once we start pushing actual frames the args
> to this
> > > method will probably be SkImage or some other vehicle that carries size
> info,
> > so
> > > no need to store width and height in this class IMHO.
> >
> > Do sizes change? Because then we have to deal with making new surface
> ids and
> > stuff.
>
> Nope, the API does not allow it.
>
Cool. Then my pref would be to give size at construction, store them as
const on the class, and DCHECK that SkImages given to it match the size.
--
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
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
On 2016/09/02 19:21:45, Justin Novosad wrote:
> On 2016/09/02 19:20:59, danakj wrote:
> >
>
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Sou...
> > File
> >
>
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
> > (right):
> >
> >
>
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Sou...
> >
>
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:27:
> > void OffscreenCanvasFrameDispatcherImpl::uploadImage(int width, int height)
> > On 2016/09/02 19:20:08, Justin Novosad wrote:
> > > On 2016/09/02 19:12:43, danakj wrote:
> > > > I thought width and height never change, why arent these part of
> > construction?
> > >
> > > This is just temporary. Once we start pushing actual frames the args to
this
> > > method will probably be SkImage or some other vehicle that carries size
> info,
> > so
> > > no need to store width and height in this class IMHO.
> >
> > Do sizes change? Because then we have to deal with making new surface ids
and
> > stuff.
>
> Nope, the API does not allow it.
Hmmm.... but I see your point. Might be worth it to store size in this class
after all. That will allow us to validate, .i.e CHECK(), the size of the frame
in the future.
Justin Novosad
LOL. I read your mind :-)
4 years, 3 months ago
(2016-09-02 19:25:08 UTC)
#13
LOL. I read your mind :-)
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
On Fri, Sep 2, 2016 at 12:25 PM, <junov@chromium.org> wrote:
> LOL. I read your mind :-)
>
:D
--
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.
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
On Fri, Sep 2, 2016 at 12:25 PM, <junov@chromium.org> wrote:
> LOL. I read your mind :-)
>
:D
--
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
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
https://codereview.chromium.org/2294963002/diff/100001/content/browser/render...
File content/browser/renderer_host/offscreen_canvas_frame_receiver_impl.cc
(right):
https://codereview.chromium.org/2294963002/diff/100001/content/browser/render...
content/browser/renderer_host/offscreen_canvas_frame_receiver_impl.cc:33: if
(!!surface) {
On 2016/09/02 19:12:42, danakj wrote:
> you dont need !! for this
Done.
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp (right):
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Sou...
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, danakj wrote:
> why isn't this created when OffscreenCanvas is constructed? then the ordering
> requirements would be more clear
The FrameDispatcher is needed only when user intends to do commit(); after
transferring control from HTMLCanvasElement to OffscreenCanvas. There are use
cases when OffscreenCanvas is constructed but users do not intend to call
commit().
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcher.h
(right):
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcher.h:17:
virtual void uploadImage(int width, int height);
On 2016/09/02 19:11:15, Justin Novosad wrote:
> I think "dispatchFrame" would be more consistent a name.
On 2016/09/02 19:12:42, danakj wrote:
> = 0
All done.
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
(right):
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:27:
void OffscreenCanvasFrameDispatcherImpl::uploadImage(int width, int height)
On 2016/09/02 19:20:59, danakj wrote:
> On 2016/09/02 19:20:08, Justin Novosad wrote:
> > On 2016/09/02 19:12:43, danakj wrote:
> > > I thought width and height never change, why arent these part of
> construction?
> >
> > This is just temporary. Once we start pushing actual frames the args to this
> > method will probably be SkImage or some other vehicle that carries size
info,
> so
> > no need to store width and height in this class IMHO.
>
> Do sizes change? Because then we have to deal with making new surface ids and
> stuff.
Made width and height to be const member in OffscreenCanvasFrameDispatcherImpl.
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:33:
frame.delegated_frame_data.reset(new cc::DelegatedFrameData());
On 2016/09/02 19:12:42, danakj wrote:
> nit: no () needed
Done.
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:34:
frame.delegated_frame_data->resource_list.resize(0u);
On 2016/09/02 19:12:42, danakj wrote:
> it already starts empty so i dont think u need this
Done.
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:39:
pass->SetAll(renderPassId, bounds, bounds, gfx::Transform(), true);
On 2016/09/02 19:12:42, danakj wrote:
> background is normally not transparent for the root render pass, is true here
> intentional? i thought canvases are opaque
Yah you're right; I think it's more logical if canvas is opaque.
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:42:
sqs->SetAll(gfx::Transform(), bounds.size(), bounds, bounds, false, 1.f,
SkXfermode::kSrc_Mode, 0);
On 2016/09/02 19:12:42, danakj wrote:
> pass an empty clip rect since is_clipped is false
>
> SrcOver mode is more common, can you use that?
Done.
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:48:
quad->SetAll(sqs, bounds, opaqueRect, bounds, needsBlending, SK_ColorGREEN,
forceAntialiasingOff);
On 2016/09/02 19:12:42, danakj wrote:
> you can use SetNew here it's helpful and decides the opaquerect.
>
> needsblending should be false. you don't need to force blending:
> https://cs.chromium.org/chromium/src/cc/quads/draw_quad.h?rcl=1472831237&l=69
Done. The solidcolorQuad::setNew() will set needsblending to be false by
default.
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:52:
m_service->SubmitCompositorFrame(m_surfaceId, std::move(frame));
On 2016/09/02 19:12:42, danakj wrote:
> The frame doesn't look totally wrong or anything to me. You could throw it in
a
> direct renderer and see what comes out if you want (see renderer_pixeltest.cc)
I see. That's a useful way to debug for me. Right now I've already added a
layout test in this CL so I can see it comes out to be a green rectangle inside
the canvas.
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.h
(right):
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.h:9:
#include "cc/output/compositor_frame.h"
On 2016/09/02 19:11:15, Justin Novosad wrote:
> I think this include is only needed in the .cpp
Done and also cleaned up other unnecessary include.
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.h:22:
const cc::SurfaceId& getSurfaceId() const { return m_surfaceId; }
On 2016/09/02 19:12:43, danakj wrote:
> group this separate from overrides
Hmmm...I realize that I don't need this trivial getter at this moment. I delete
it now and will add it only when I find it useful.
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.h:23:
void uploadImage(int width, int height) override;
On 2016/09/02 19:12:43, danakj wrote:
> Group overrides together and put a comment on them: //
> OffscreenCanvasFrameDispatcher implementation.
Done.
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom
(right):
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom:25:
// TODO(563858): Make commit() work for worker.
On 2016/09/02 19:12:43, danakj wrote:
> Assuming this is a bug number, the syntax is
>
> TODO(crbug.com/XYZ): Stuff.
Done and elsewhere.
+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
+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 your feedback.
Thanks!
Tom Sepez
mojom lgtm
4 years, 3 months ago
(2016-09-02 21:00:03 UTC)
#19
mojom lgtm
xlai (Olivia)
The CQ bit was checked by xlai@chromium.org to run a CQ dry run
4 years, 3 months ago
(2016-09-02 21:02:37 UTC)
#20
Description was changed from ========== Submit Compositor Frame from OffscreenCanvas on main CompositorFrame is to ...
4 years, 3 months ago
(2016-09-02 21:06:25 UTC)
#22
Description was changed from
==========
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.
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
==========
to
==========
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
==========
4 years, 3 months ago
(2016-09-02 23:41:28 UTC)
#26
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp (right):
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Sou...
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, danakj wrote:
> > Oops I mean to change this comment but it got lost. What about creating this
> > when the surface ids are set instead? That happens when transferring
control.
>
> Though also from
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...
> ? Which I don't know when that happens.
The code link you've given happens when OffscreenCanvas is transferred from main
thread
to worker thread, which may or may not happen depending on user's JavaScript
code.
Also, after users call HTMLCanvasElement.transferControlToOffscreen() in
JavaScript, they may
or may not call OffscreenCanvas2D.commit(). If they don't call commit(), then we
are doing an
eager instantiation of OffscreenCanvasFrameDispatcher here.
I'm just feeling that a lazy instantiation of frameDispatcher is better--that I
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.
https://codereview.chromium.org/2294963002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp (right):
https://codereview.chromium.org/2294963002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:31:
m_size.setWidth(clampTo<int>(width));
You're right. I also feel that it's illogical to have setWidth when
OffscreenCanvas is transferred control
from html canvas; in fact, its size is purely determined by the size of HTML
canvas.
I add an if statement here to make calling this function have no effect in this
particular case.
Now this function is reserved only for the use case when OffscreenCanvas is
constructed on its own without transfering control.
On 2016/09/02 21:53:50, danakj wrote:
> While I'm reading this code, can you explain why there is setWidth/setHeight
if
> they dont change?
https://codereview.chromium.org/2294963002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp
(right):
https://codereview.chromium.org/2294963002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp:50:
// TODO(563858/xlai): implement commit() on worker thread; currently, do
On 2016/09/02 21:51:00, danakj wrote:
> Sorry it should be
>
> TODO(xlai): Stuff to do etc, see crbug.com/563858.
>
> or
>
> TODO(crbug.com/563858): Stuff to do etc.
Done.
https://codereview.chromium.org/2294963002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.h
(right):
https://codereview.chromium.org/2294963002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.h:24:
cc::SurfaceId m_surfaceId;
On 2016/09/02 21:51:00, danakj wrote:
> const also?
Done.
4 years, 3 months ago
(2016-09-03 00:40:43 UTC)
#27
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp (right):
https://codereview.chromium.org/2294963002/diff/100001/third_party/WebKit/Sou...
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, xlai (Olivia) wrote:
> On 2016/09/02 21:53:50, danakj wrote:
> > > Oops I mean to change this comment but it got lost. What about creating
this
> > > when the surface ids are set instead? That happens when transferring
> control.
> >
> > Though also from
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...
> > ? Which I don't know when that happens.
>
> The code link you've given happens when OffscreenCanvas is transferred from
main
> thread
> to worker thread, which may or may not happen depending on user's JavaScript
> code.
>
> Also, after users call HTMLCanvasElement.transferControlToOffscreen() in
> JavaScript, they may
> or may not call OffscreenCanvas2D.commit(). If they don't call commit(), then
we
> are doing an
> eager instantiation of OffscreenCanvasFrameDispatcher here.
>
> I'm just feeling that a lazy instantiation of frameDispatcher is better--that
I
> 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 :)
xlai (Olivia)
The CQ bit was checked by xlai@chromium.org to run a CQ dry run
4 years, 3 months ago
(2016-09-03 01:19:41 UTC)
#28
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
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).
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
4 years, 3 months ago
(2016-09-03 03:11:23 UTC)
#31
Description was changed from ========== Submit Compositor Frame from OffscreenCanvas on main CompositorFrame is to ...
4 years, 3 months ago
(2016-09-03 03:17:44 UTC)
#36
Message was sent while issue was closed.
Description was changed from
==========
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
==========
to
==========
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
==========
commit-bot: I haz the power
Committed patchset #10 (id:200001)
4 years, 3 months ago
(2016-09-03 03:17:45 UTC)
#37
Message was sent while issue was closed.
Committed patchset #10 (id:200001)
commit-bot: I haz the power
Description was changed from ========== Submit Compositor Frame from OffscreenCanvas on main CompositorFrame is to ...
4 years, 3 months ago
(2016-09-03 03:21:37 UTC)
#38
Message was sent while issue was closed.
Description was changed from
==========
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
==========
to
==========
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}
==========
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
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
Message was sent while issue was closed.
On 2016/09/05 04:14:13, Matt Giuca wrote:
> A revert of this CL (patchset #10 id:200001) has been created in
> https://codereview.chromium.org/2313533002/ by mailto:mgiuca@chromium.org.
>
> The reason for reverting is: The new OffscreenCanvas-commit-main tests are
> broken on MSAN on Linux:
>
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20MSAN/b...
>
> Reason: use-of-uninitialized-value
>
> unexpected_failures:
> fast/canvas/OffscreenCanvas-commit-main.html
> virtual/gpu/fast/canvas/OffscreenCanvas-commit-main.html
> virtual/display_list_2d_canvas/fast/canvas/OffscreenCanvas-commit-main.html.
The MSAN stdio logs (from
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20MSAN/b...):
21:51:38.503 3993 worker/2
virtual/gpu/fast/canvas/OffscreenCanvas-commit-main.html crashed, (stderr
lines):
21:51:38.503 3993
[4003:4003:0902/215137:3947701411:WARNING:audio_manager.cc(317)] Multiple
instances of AudioManager detected
21:51:38.503 3993
[4003:4003:0902/215137:3947701561:WARNING:audio_manager.cc(278)] Multiple
instances of AudioManager detected
21:51:38.503 3993 Xlib: extension "RANDR" missing on display ":9".
21:51:38.503 3993 Xlib: extension "RANDR" missing on display ":9".
21:51:38.503 3993 Uninitialized bytes in __interceptor_send at offset 228
inside [0x718000002800, 976)
21:51:38.503 3993 ==4==WARNING: MemorySanitizer: use-of-uninitialized-value
21:51:38.503 3993 #0 0xa464dbd in WriteNoLock
mojo/edk/system/channel_posix.cc:398:18
21:51:38.503 3993 #1 0xa45c7b4 in Write
mojo/edk/system/channel_posix.cc:126:14
21:51:38.503 3993 #2 0xa470fb3 in WriteChannelMessage
mojo/edk/system/node_channel.cc:900:15
21:51:38.503 3993 #3 0xa470fb3 in PortsMessage
mojo/edk/system/node_channel.cc:352:0
21:51:38.503 3993 #4 0xa403efe in SendPeerMessage
mojo/edk/system/node_controller.cc:649:11
21:51:38.503 3993 #5 0xa40563c in ForwardMessage
mojo/edk/system/node_controller.cc:782:5
21:51:38.503 3993 #6 0xe30450 in ForwardMessages_Locked
mojo/edk/system/ports/node.cc:1187:16
21:51:38.503 3993 #7 0xe27a50 in OnUserMessage
mojo/edk/system/ports/node.cc:474:18
21:51:38.503 3993 #8 0xe26e3b in AcceptMessage
mojo/edk/system/ports/node.cc:335:14
21:51:38.503 3993 #9 0xe26786 in SendMessageInternal
mojo/edk/system/ports/node.cc:852:12
21:51:38.503 3993 #10 0xe25d7c in SendMessage
mojo/edk/system/ports/node.cc:314:12
21:51:38.503 3993 #11 0xa3fddf3 in SendMessage
mojo/edk/system/node_controller.cc:285:19
21:51:38.503 3993 #12 0xa3eeca8 in WriteMessage
mojo/edk/system/message_pipe_dispatcher.cc:142:30
21:51:38.503 3993 #13 0xa3c6844 in WriteMessageNew
mojo/edk/system/core.cc:696:22
21:51:38.503 3993 #14 0x708c535 in WriteMessageNew
mojo/public/cpp/system/message_pipe.h:97:10
21:51:38.503 3993 #15 0x708c535 in Accept
mojo/public/cpp/bindings/lib/connector.cc:158:0
21:51:38.503 3993 #16 0x3e6fa9d in SubmitCompositorFrame
./out/Release/gen/third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom-blink.cc:642:24
21:51:38.503 3993 #17 0x133498fc in dispatchFrame
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:52:16
21:51:38.503 3993 #18 0x1240215 in Call v8/src/api-arguments.cc:21:3
21:51:38.503 3993 #19 0x14adaef in HandleApiCallHelper<false>
v8/src/builtins/builtins-api.cc:106:36
21:51:38.503 3993 #20 0x14a9ca4 in Builtin_Impl_HandleApiCall
v8/src/builtins/builtins-api.cc:135:5
21:51:38.503 3993 #21 0x398c94a in DoRuntimeCall
v8/src/arm64/simulator-arm64.cc:610:27
21:51:38.503 3993 #22 0x39898e0 in ExecuteInstruction
v8/src/arm64/simulator-arm64.h:315:5
21:51:38.503 3993 #23 0x39898e0 in Run
v8/src/arm64/simulator-arm64.cc:446:0
21:51:38.503 3993 #24 0x39898e0 in CheckPCSComplianceAndRun
v8/src/arm64/simulator-arm64.cc:252:0
21:51:38.503 3993 #25 0x39898e0 in CallVoid
v8/src/arm64/simulator-arm64.cc:162:0
21:51:38.503 3993 #26 0x398a26c in CallInt64
v8/src/arm64/simulator-arm64.cc:169:3
21:51:38.503 3993 #27 0x398a26c in CallJS
v8/src/arm64/simulator-arm64.cc:194:0
21:51:38.503 3993 #28 0x246c82c in Invoke v8/src/execution.cc:141:13
21:51:38.503 3993 #29 0x246b62e in Call v8/src/execution.cc:178:10
21:51:38.503 3993 #30 0x1270164 in Run v8/src/api.cc:1846:23
21:51:38.503 3993 #31 0xc15cced in runCompiledScript
third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:417:26
21:51:38.503 3993 #32 0xc09c094 in executeScriptAndReturnValue
third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:149:21
21:51:38.503 3993 #33 0xc0a2651 in evaluateScriptInMainWorld
third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:395:35
21:51:38.503 3993 #34 0xc0a2cbf in executeScriptInMainWorld
third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:373:5
21:51:38.503 3993 #35 0xf9e32ca in executeScript
third_party/WebKit/Source/core/dom/ScriptLoader.cpp:438:21
21:51:38.504 3993 #36 0xf9d8a87 in prepareScript
third_party/WebKit/Source/core/dom/ScriptLoader.cpp:276:14
21:51:38.504 3993 #37 0x143471bb in runScript
third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:427:23
21:51:38.504 3993 #38 0x1434626a in execute
third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:280:5
21:51:38.504 3993 #39 0x142f745d in runScriptsForPausedTreeBuilder
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:273:25
21:51:38.504 3993 #40 0x142f745d in
processTokenizedChunkFromBackgroundParser
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:518:0
21:51:38.504 3993 #41 0x142ee42e in pumpPendingSpeculations
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:575:36
21:51:38.504 3993 #42 0xbb16a07 in
Invoke<std::__1::unique_ptr<blink::WebTaskRunner::Task,
std::__1::default_delete<blink::WebTaskRunner::Task> > >
base/bind_internal.h:164:12
21:51:38.504 3993 #43 0xbb16a07 in MakeItSo<void (*const
&)(std::__1::unique_ptr<blink::WebTaskRunner::Task,
std::__1::default_delete<blink::WebTaskRunner::Task> >),
std::__1::unique_ptr<blink::WebTaskRunner::Task,
std::__1::default_delete<blink::WebTaskRunner::Task> > >
base/bind_internal.h:284:0
21:51:38.504 3993 #44 0xbb16a07 in RunImpl<void (*const
&)(std::__1::unique_ptr<blink::WebTaskRunner::Task,
std::__1::default_delete<blink::WebTaskRunner::Task> >), const
std::__1::tuple<base::internal::PassedWrapper<std::__1::unique_ptr<blink::WebTaskRunner::Task,
std::__1::default_delete<blink::WebTaskRunner::Task> > > > &, 0>
base/bind_internal.h:347:0
21:51:38.504 3993 #45 0xbb16a07 in Run base/bind_internal.h:325:0
21:51:38.504 3993 #46 0x69a9cc6 in Run base/callback.h:61:12
21:51:38.504 3993 #47 0x69a9cc6 in RunTask
base/debug/task_annotator.cc:54:0
21:51:38.504 3993 #48 0xbaeeb31 in ProcessTaskFromWorkQueue
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:320:19
21:51:38.504 3993 #49 0xbae7932 in DoWork
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:221:13
21:51:38.504 3993 #50 0x69a9cc6 in Run base/callback.h:61:12
21:51:38.504 3993 #51 0x69a9cc6 in RunTask
base/debug/task_annotator.cc:54:0
21:51:38.504 3993 #52 0x67b8b1b in RunTask
base/message_loop/message_loop.cc:488:19
21:51:38.504 3993 #53 0x67ba657 in DeferOrRunPendingTask
base/message_loop/message_loop.cc:497:5
21:51:38.504 3993 #54 0x67bccb7 in DoDelayedWork
base/message_loop/message_loop.cc:660:10
21:51:38.504 3993 #55 0x67c826e in Run
base/message_loop/message_pump_default.cc:39:27
21:51:38.504 3993 #56 0x683ef6c in Run base/run_loop.cc:35:10
21:51:38.504 3993 #57 0xae5e3c4 in RendererMain
content/renderer/renderer_main.cc:198:23
21:51:38.504 3993 #58 0x4d30382 in RunZygote
content/app/content_main_runner.cc:343:14
21:51:38.504 3993 #59 0x4d333dc in RunNamedProcessTypeMain
content/app/content_main_runner.cc:426:12
21:51:38.504 3993 #60 0x4d368f1 in Run
content/app/content_main_runner.cc:786:12
21:51:38.504 3993 #61 0x4d14d80 in ContentMain
content/app/content_main.cc:20:28
21:51:38.504 3993 #62 0x4ad510 in main
content/shell/app/shell_main.cc:48:10
21:51:38.505 3993 #63 0x7fda7676176c in __libc_start_main
/build/eglibc-rrybNj/eglibc-2.15/csu/libc-start.c:226:0
21:51:38.505 3993 #64 0x442de8 in _start ??:0
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
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
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
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: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.
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
Reviewers: Justin Novosad, danakj, Tom Sepez, no sievers
Base URL:
Comments: 41