Closed Bug 1379920 Opened 7 years ago Closed 7 years ago

Support canvas for layers-free mode

Categories

(Core :: Graphics: WebRender, enhancement)

Product:
Core
Component:
Graphics: WebRender
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: mtseng, Assigned: mtseng)

References

Details

Attachments

(7 files)

Bug 1379920 - Introduce WebRenderCanvasData.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1379920 - Introduce mLastCanvasDatas.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1379920 - Add aAsync param to AddPipelineIdForAsyncCompositable and rename it to AddPipelineIdForCompositable.
59 bytes, text/x-review-board-request
sotaro
: review+
Details
Bug 1379920 - Fix unified build error.
59 bytes, text/x-review-board-request
nical
: review+
Details
Bug 1379920 - Introduce CanvasRenderer and its derived classes.
59 bytes, text/x-review-board-request
nical
: review+
jrmuizel
: review+
jgilbert
: review+
Details
Bug 1379920 - Support canvas in layers free mode.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1379920 - Rename Copy/ShareableCanvasLayer to Copy/ShareableCanvasRenderer.
59 bytes, text/x-review-board-request
jrmuizel
: review+
nical
: review+
Details 
      No description provided.
BTW, current iframe position is wrong with prebuilt display list. See https://github.com/servo/webrender/pull/1466
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 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)
I agree with you. Clear review flag.
Attachment #8885181 - Flags: review?(bugmail)
Attachment #8885182 - Flags: review?(bugmail)
Attachment #8885184 - Flags: review?(bugmail)
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.
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 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 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 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 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.
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 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.
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 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+
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.
> 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 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.
(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 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 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 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+
Jeff, review ping.
Flags: needinfo?(jmuizelaar)
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 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+
Thanks for review!
Flags: needinfo?(jmuizelaar)
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
Depends on: 1387994
Depends on: 1388520
For the future, all changes to dom/canvas/WebGL* should request review from me.
Attachment #8887402 - Flags: review?(jgilbert)
Depends on: 1391960
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+
Depends on: 1435546
No longer depends on: 1435546
Depends on: 1436466
Regressions: 1599653
Regressions: 1656987
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: