Closed
Bug 1379920
Opened 7 years ago
Closed 7 years ago
Support canvas for layers-free mode
Categories
(Core :: Graphics: WebRender, enhancement)
Core
Graphics: WebRender
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: mtseng, Assigned: mtseng)
References
Details
Attachments
(7 files)
(deleted),
text/x-review-board-request
|
kats
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
kats
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
sotaro
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
nical
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
nical
:
review+
jrmuizel
:
review+
jgilbert
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
kats
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jrmuizel
:
review+
nical
:
review+
|
Details |
No description provided.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
BTW, current iframe position is wrong with prebuilt display list. See https://github.com/servo/webrender/pull/1466
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8885184 [details]
Bug 1379920 - Fix unified build error.
https://reviewboard.mozilla.org/r/156072/#review161246
So I don't really like the way this is architected, in that you are still creating a canvas layer to do the paint, and making the WebRenderCanvasData a thin wrapper around the canvas layer.
What I would prefer is that you move all the fields from WebRenderCanvasLayer that you need (basically anything used in WebRenderCanvasLayer::RenderLayerAsync) into the WebRenderCanvasData class. Then, you don't need to keep a pointer to the layer in WebRenderCanvasData, or ever create the layer in "layers-free" mode. You can invert the relationship instead, so that WebRenderCanvasLayer has an instance of the WebRenderCanvasData and uses that for its old RenderLayer implementation, to avoid duplication.
That way once we switch over completely to the layers-free mode, we can ditch WebRenderCanvasLayer along with the other layer classes without too much trouble.
Does that sound reasonable?
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8885183 [details]
Bug 1379920 - Add aAsync param to AddPipelineIdForAsyncCompositable and rename it to AddPipelineIdForCompositable.
https://reviewboard.mozilla.org/r/156070/#review161250
Somebody else should probably review this patch - I'm not too familiar with this code. I'm not sure if this will change much with the new design I suggested so I'm clearing the flag for now, but maybe sotaro or nical would be a better reviewer.
Attachment #8885183 -
Flags: review?(bugmail)
Assignee | ||
Comment 8•7 years ago
|
||
I agree with you. Clear review flag.
Assignee | ||
Updated•7 years ago
|
Attachment #8885181 -
Flags: review?(bugmail)
Assignee | ||
Updated•7 years ago
|
Attachment #8885182 -
Flags: review?(bugmail)
Assignee | ||
Updated•7 years ago
|
Attachment #8885184 -
Flags: review?(bugmail)
Assignee | ||
Comment 9•7 years ago
|
||
Status update: I has extracted common interfaces to several classes which are used by both with and without layers mode. I'll push to try to see any try fail and organized the patches for review.
Assignee | ||
Comment 10•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b7756351d3cc7f5d2647dbab84a6c687dd8aedf&selectedJob=115107888
try looks ok besides the unified build error on windows. Will fix later.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8885184 [details]
Bug 1379920 - Fix unified build error.
https://reviewboard.mozilla.org/r/156072/#review163432
Attachment #8885184 -
Flags: review?(nical.bugzilla) → review+
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8885181 [details]
Bug 1379920 - Introduce WebRenderCanvasData.
https://reviewboard.mozilla.org/r/156066/#review163612
::: gfx/layers/wr/WebRenderUserData.h:118
(Diff revision 2)
> + CanvasRenderer* GetCanvasRenderer();
> +
> +protected:
> + UniquePtr<CanvasRenderer> mCanvasRenderer;
Considering that in both of the next two patches you do a static_cast on the return value of GetCanvasRenderer(), I think it makes sense to change the return type of GetCanvasRenderer and mCanvasRenderer to be more specific - you can use WebRenderCanvasRenderer* and UniquePtr<WebRenderCanvasRenderer> instead. That way you can eliminate the static_cast in one patch at least. If you change it to WebRenderCanvasRendererAsync* then you can eliminate the other static_cast too but that might be too specific, I'm not sure.
Attachment #8885181 -
Flags: review?(bugmail) → review+
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8885182 [details]
Bug 1379920 - Introduce mLastCanvasDatas.
https://reviewboard.mozilla.org/r/156068/#review163618
::: gfx/layers/wr/WebRenderLayerManager.h:270
(Diff revision 2)
> uint32_t mPaintSequenceNumber;
> // See equivalent field in ClientLayerManager
> APZTestData mApzTestData;
> +
> + typedef nsTHashtable<nsRefPtrHashKey<WebRenderCanvasData>> CanvasDataSet;
> + CanvasDataSet mLastCanvasDatas;
Add a comment on this field ("Store of WebRenderCanvasData objects for use in empty transactions" or something)
::: gfx/layers/wr/WebRenderLayerManager.cpp:489
(Diff revision 2)
> CreateWebRenderCommandsFromDisplayList(aDisplayList, aDisplayListBuilder, sc, builder);
> builder.Finalize(contentSize, mBuiltDisplayList);
> + } else {
> + for (auto iter = mLastCanvasDatas.Iter(); !iter.Done(); iter.Next()) {
> + RefPtr<WebRenderCanvasData> canvasData = iter.Get()->GetKey();
> + WebRenderCanvasRenderer* canvas = static_cast<WebRenderCanvasRenderer*>(canvasData->GetCanvasRenderer());
Drop this static_cast with the change I suggested in the previous patch
Attachment #8885182 -
Flags: review?(bugmail) → review+
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8887403 [details]
Bug 1379920 - Support canvas in layers free mode.
https://reviewboard.mozilla.org/r/158268/#review163626
::: dom/html/HTMLCanvasElement.cpp:1066
(Diff revision 1)
> if (!frame)
> return;
>
> ActiveLayerTracker::NotifyContentChange(frame);
>
> + // CanvasRenderer might store in frame, try to get it.
This comment isn't clear. I'd prefer something like:
When using layers-free WebRender, we cannot invalidate the layer (because there isn't one). Instead, we mark the CanvasRenderer dirty and scheduling an empty transaction which is effectively equivalent.
::: layout/generic/nsHTMLCanvasFrame.cpp:193
(Diff revision 1)
> + case CanvasContextType::ImageBitmap:
> + {
> + // TODO: Support ImageBitmap
> + break;
> + }
> + default:
> + break;
I feel like these unhandled cases should return false instead of doing "break;" and returning true. Also since the enum only has 5 possible values, I'd prefer getting rid of "default" and using "case NoContext" explicitly. That way if new types are added to the enum the compiler should warn us if it's not handled here.
Attachment #8887403 -
Flags: review?(bugmail) → review+
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8887402 [details]
Bug 1379920 - Introduce CanvasRenderer and its derived classes.
https://reviewboard.mozilla.org/r/158266/#review163440
Looks good overall modulo a few nits. I'd would feel better with an extra pair of eyes reviewing this, as the patch is large and I haven't been involved with the specifics of layer free yet.
::: gfx/layers/CanvasRenderer.h:72
(Diff revision 1)
> + // Whether the canvas front buffer is already being rendered somewhere else.
> + // When true, do not swap buffers or Morph() to another factory on mGLContext
> + bool mIsMirror;
> +};
> +
> +class CanvasRenderer
Please add a comment explaining the purpose of this class.
::: gfx/layers/client/ClientCanvasLayer.h:75
(Diff revision 1)
> virtual ShadowableLayer* AsShadowableLayer() override { return this; }
>
> virtual void Disconnect() override
> {
> - if (mBufferProvider) {
> - mBufferProvider->ClearCachedResources();
> + ClientCanvasRenderer* canvasRenderer =
> + static_cast<ClientCanvasRenderer*>(mCanvasRenderer.get());
I'd rather avoid static_cast whenever possible.
::: gfx/layers/client/ClientCanvasLayer.cpp:26
(Diff revision 1)
> {
> AUTO_PROFILER_LABEL("ClientCanvasLayer::RenderLayer", GRAPHICS);
>
> RenderMaskLayers(this);
>
> - UpdateCompositableClient();
> + ShareableCanvasRenderer* canvasRenderer = static_cast<ShareableCanvasRenderer*>(mCanvasRenderer.get());
I'd rather not use static_casts for this. You can add a collection of virtual down-cast methods like `AsShareableCanvasRenderer()` and then check that the result is not null. We don't need to have a fallback code path, it can just be a fatal assertion but at least we know we can't have weird memory corruptions out of that.
Comment 22•7 years ago
|
||
Jeff, I (tried to) add you in the review requests for this patch in mozreview, not sure it did what I intended. could you have a look at it?
Flags: needinfo?(jmuizelaar)
Comment 23•7 years ago
|
||
mozreview-review |
Comment on attachment 8887403 [details]
Bug 1379920 - Support canvas in layers free mode.
https://reviewboard.mozilla.org/r/158268/#review163614
::: layout/generic/nsHTMLCanvasFrame.cpp:170
(Diff revision 1)
> + // We don't push a stacking context for this async image pipeline here.
> + // Instead, we do it inside the iframe that hosts the image. As a result,
> + // a bunch of the calculations normally done as part of that stacking
> + // context need to be done manually and pushed over to the parent side,
> + // where it will be done when we build the display list for the iframe.
> + // That happens in WebRenderCompositableHolder.
How do we ensure that canvas updates are applied/visible at the same time as layout updates from the same spin of the JS event loop? It's an annoying and tricky thing to support but we currently go through great lengths to do it.
Assignee | ||
Comment 24•7 years ago
|
||
mozreview-review |
Comment on attachment 8887403 [details]
Bug 1379920 - Support canvas in layers free mode.
https://reviewboard.mozilla.org/r/158268/#review163948
::: layout/generic/nsHTMLCanvasFrame.cpp:170
(Diff revision 1)
> + // We don't push a stacking context for this async image pipeline here.
> + // Instead, we do it inside the iframe that hosts the image. As a result,
> + // a bunch of the calculations normally done as part of that stacking
> + // context need to be done manually and pushed over to the parent side,
> + // where it will be done when we build the display list for the iframe.
> + // That happens in WebRenderCompositableHolder.
In layers-free mode, canvas update triggers a compositable update and also empty transaction. Hence, we send display list to parent and parent update the content with latest compositable. But this frame won't be visible until WebRender's render thread build and render it. I think frame delay issue also happened in with layers mode.
Comment 25•7 years ago
|
||
mozreview-review |
Comment on attachment 8885183 [details]
Bug 1379920 - Add aAsync param to AddPipelineIdForAsyncCompositable and rename it to AddPipelineIdForCompositable.
https://reviewboard.mozilla.org/r/156070/#review163986
Attachment #8885183 -
Flags: review?(sotaro.ikeda.g) → review+
Assignee | ||
Comment 26•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8887402 [details]
Bug 1379920 - Introduce CanvasRenderer and its derived classes.
https://reviewboard.mozilla.org/r/158266/#review163440
> I'd rather not use static_casts for this. You can add a collection of virtual down-cast methods like `AsShareableCanvasRenderer()` and then check that the result is not null. We don't need to have a fallback code path, it can just be a fatal assertion but at least we know we can't have weird memory corruptions out of that.
Thanks for the comment. I remove all static_cast in next patch.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 31•7 years ago
|
||
> In layers-free mode, canvas update triggers a compositable update and also empty transaction. Hence, we send display list to parent and parent update the content with latest compositable. But this frame won't be visible until WebRender's render thread build and render it. I think frame delay issue also happened in with layers mode.
We don't have this frame delay behavior with the non-webrender code paths. I personally hope we can get rid of this constraint but we may not be able to.
I don't think it would be too much work to move this out of ImageBridge (which is for things that don't require the frame synchronization), but it's something we should be careful about. I also checked that chrome synchronizes layout and canvas updates.
Comment 32•7 years ago
|
||
mozreview-review |
Comment on attachment 8887402 [details]
Bug 1379920 - Introduce CanvasRenderer and its derived classes.
https://reviewboard.mozilla.org/r/158266/#review164180
::: commit-message-f33e7:1
(Diff revision 2)
> +Bug 1379920 - Introduce CanvasRenderer and it's derived classes. r=nical,jrmuizel
and its derived classes.
::: commit-message-f33e7:15
(Diff revision 2)
> +
> +Also added InitializeCanvasRenderer in the canvas related classes to
> +initialize CanvasRenderer without involved layer.
> +
> +MozReview-Commit-ID: 5hqQ19W169r
> +
Using hg mv for ShareableCanvasLayer.cpp -> ShareableCanvasRenderer.cpp seems like it would make this easier to review and would preserve the history better.
Also, since the patch is so big it would be to include a more detailed commit message.
Comment 33•7 years ago
|
||
(In reply to Nicolas Silva [:nical] from comment #31)
> We don't have this frame delay behavior with the non-webrender code paths. I
> personally hope we can get rid of this constraint but we may not be able to.
> I don't think it would be too much work to move this out of ImageBridge
> (which is for things that don't require the frame synchronization), but it's
> something we should be careful about. I also checked that chrome
> synchronizes layout and canvas updates.
Yeah, canvas and layout need to be synchronized.
Flags: needinfo?(jmuizelaar)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 41•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8887402 [details]
Bug 1379920 - Introduce CanvasRenderer and its derived classes.
https://reviewboard.mozilla.org/r/158266/#review164180
> Using hg mv for ShareableCanvasLayer.cpp -> ShareableCanvasRenderer.cpp seems like it would make this easier to review and would preserve the history better.
>
> Also, since the patch is so big it would be to include a more detailed commit message.
Due to limitation of git-cinnabar. I move renamed patch into separate commit.
Comment 42•7 years ago
|
||
mozreview-review |
Comment on attachment 8887402 [details]
Bug 1379920 - Introduce CanvasRenderer and its derived classes.
https://reviewboard.mozilla.org/r/158266/#review165152
Attachment #8887402 -
Flags: review?(nical.bugzilla) → review+
Comment 43•7 years ago
|
||
mozreview-review |
Comment on attachment 8888221 [details]
Bug 1379920 - Rename Copy/ShareableCanvasLayer to Copy/ShareableCanvasRenderer.
https://reviewboard.mozilla.org/r/159170/#review165156
Attachment #8888221 -
Flags: review?(nical.bugzilla) → review+
Comment 45•7 years ago
|
||
mozreview-review |
Comment on attachment 8887402 [details]
Bug 1379920 - Introduce CanvasRenderer and its derived classes.
https://reviewboard.mozilla.org/r/158266/#review164982
::: gfx/layers/CanvasRenderer.h:76
(Diff revision 3)
> + // When true, do not swap buffers or Morph() to another factory on mGLContext
> + bool mIsMirror;
> +};
> +
> +// Based class which used for canvas rendering. There are many derived classes for
> +// different purposes. such as:
Base instead of Based
Attachment #8887402 -
Flags: review?(jmuizelaar) → review+
Comment 46•7 years ago
|
||
mozreview-review |
Comment on attachment 8888221 [details]
Bug 1379920 - Rename Copy/ShareableCanvasLayer to Copy/ShareableCanvasRenderer.
https://reviewboard.mozilla.org/r/159170/#review166580
Attachment #8888221 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 48•7 years ago
|
||
Comment 49•7 years ago
|
||
Pushed by mtseng@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/813018d583be
Fix unified build error. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0bdffa0ee1a
Add aAsync param to AddPipelineIdForAsyncCompositable and rename it to AddPipelineIdForCompositable. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/e83b23441f6d
Rename Copy/ShareableCanvasLayer to Copy/ShareableCanvasRenderer. r=nical,jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5d815d3ac0b
Introduce CanvasRenderer and its derived classes. r=nical,jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ecd7180c6a7
Introduce WebRenderCanvasData. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9317e74f705
Introduce mLastCanvasDatas. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4e2c3d4f17c
Support canvas in layers free mode. r=kats
Comment 50•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/813018d583be
https://hg.mozilla.org/mozilla-central/rev/c0bdffa0ee1a
https://hg.mozilla.org/mozilla-central/rev/e83b23441f6d
https://hg.mozilla.org/mozilla-central/rev/d5d815d3ac0b
https://hg.mozilla.org/mozilla-central/rev/3ecd7180c6a7
https://hg.mozilla.org/mozilla-central/rev/b9317e74f705
https://hg.mozilla.org/mozilla-central/rev/c4e2c3d4f17c
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 51•7 years ago
|
||
For the future, all changes to dom/canvas/WebGL* should request review from me.
Updated•7 years ago
|
Attachment #8887402 -
Flags: review?(jgilbert)
Comment 52•7 years ago
|
||
mozreview-review |
Comment on attachment 8887402 [details]
Bug 1379920 - Introduce CanvasRenderer and its derived classes.
https://reviewboard.mozilla.org/r/158266/#review176688
I really would have prefered to take this opportunity to coalesce some of the CanvasLayer (now more or less CanvasRenderer) code to simplify things. As we add more classes, this code only gets harder to understand and maintain.
::: gfx/layers/CanvasRenderer.h:31
(Diff revision 3)
> +class CopyableCanvasRenderer;
> +class PersistentBufferProvider;
> +class WebRenderCanvasRendererSync;
> +class WebRenderCanvasRendererAsync;
> +
> +struct CanvasInitializeData {
I'm not sure why this didn't become CanvasRenderer::Data?
::: gfx/layers/CopyableCanvasRenderer.h:50
(Diff revision 3)
> + void Destroy() override;
>
> - virtual bool IsDataValid(const Data& aData) override;
> + CopyableCanvasRenderer* AsCopyableCanvasRenderer() override { return this; }
>
> - bool IsGLLayer() { return !!mGLContext; }
> + bool NeedsYFlip() const { return mOriginPos == gl::OriginPos::BottomLeft; }
> + bool HasGLContext() const { return !!mGLContext; }
Prefer bool(foo) to !!foo.
::: gfx/layers/CopyableCanvasRenderer.h:51
(Diff revision 3)
>
> - virtual bool IsDataValid(const Data& aData) override;
> + CopyableCanvasRenderer* AsCopyableCanvasRenderer() override { return this; }
>
> - bool IsGLLayer() { return !!mGLContext; }
> + bool NeedsYFlip() const { return mOriginPos == gl::OriginPos::BottomLeft; }
> + bool HasGLContext() const { return !!mGLContext; }
> + bool IsOpaque() const { return mOpaque; }
Elsewhere we talk about has-alpha, and that should have been continued here.
Later, at the caller, this would result in the ideally-obvious:
foo = (HasAlpha() : COLOR_ALPHA : COLOR)
::: gfx/layers/CopyableCanvasRenderer.cpp:37
(Diff revision 3)
> using namespace mozilla::gfx;
> using namespace mozilla::gl;
>
> -CopyableCanvasLayer::CopyableCanvasLayer(LayerManager* aLayerManager, void *aImplData) :
> - CanvasLayer(aLayerManager, aImplData)
> +CopyableCanvasRenderer::CopyableCanvasRenderer()
> + : mGLContext(nullptr)
> + , mBufferProvider(nullptr)
Why are you manually initializing smart-pointers? They default-construct to null.
::: gfx/layers/CopyableCanvasRenderer.cpp:105
(Diff revision 3)
> + if (mBufferProvider) {
> + mBufferProvider->ClearCachedResources();
> + }
> +
> + mCachedTempSurface = nullptr;
This does the same thing as ClearCachedResources. Shouldn't it just call that function?
::: gfx/layers/CopyableCanvasRenderer.cpp:161
(Diff revision 3)
> + if (needsPremult) {
> + gfxUtils::PremultiplyDataSurface(resultSurf, resultSurf);
> + }
> + MOZ_ASSERT(resultSurf);
> +
> + FireDidTransactionCallback();
IIRC a bug was already filed about this not being executed if we take an early-out, but it's worth checking.
::: gfx/layers/ShareableCanvasRenderer.cpp:150
(Diff revision 3)
> return false;
> }
>
> IntSize readSize(frontbuffer->mSize);
> - SurfaceFormat format = (GetContentFlags() & CONTENT_OPAQUE)
> - ? SurfaceFormat::B8G8R8X8
> + SurfaceFormat format =
> + mOpaque ? SurfaceFormat::B8G8R8X8 : SurfaceFormat::B8G8R8A8;
The formatting was better before, in that it made it easier to read that one was BGRX and the other was BGRA.
Attachment #8887402 -
Flags: review?(jgilbert) → review+
You need to log in
before you can comment on or make changes to this bug.
Description
•