1290008 - Add dirty region for canvas to avoid copying whole surface

Status: RESOLVED INACTIVE

(Core :: Graphics: Canvas2D, defect)

Description

[Ethan Lin[:ethlin]]

Currently we always copy whole canvas surface to TextureClient each frame. I think we can try to add dirty region/rect for canvas. There are front and back TextureClient. I think if we try to create dirty region for each canvas operation and union current dirty region with last dirty region, then we just need to copy that region to TextureClient.

Comment 1

[Ethan Lin[:ethlin]]

Attachment: wip

I implemented a very rough version for FillRect. I collect dirty rect in FillRect and copy surface partially according to the dirty rect. I tried the testcase in bug 1161818. The result looked fine and I got an 60fps with the wip from the original 30fps with 8000x8000 canvas on windows.

Comment 2

[Nicolas Silva [:nical]]

Comment on attachment 8775472 [details] [diff] [review]
wip

Review of attachment 8775472 [details] [diff] [review]:
-----------------------------------------------------------------

The idea is good, but bad timing unfortunately, since the copy you are optimizing is getting removed (see comment below).

::: gfx/layers/CopyableCanvasLayer.cpp
@@ +84,5 @@
>    return mGLContext == aData.mGLContext;
>  }
>  
>  void
> +CopyableCanvasLayer::UpdateTarget(DrawTarget* aDestTarget, const gfx::Rect& aDirty)

There isn't a lot of value in optimizing this code path because it will become used rarely as I re-enable PersistentBufferProviderShared on each configuration. I would prefer to leave it as is and even try to remove it completely whenever we can.

I am currenlty in the process of re-enabling PersistentBufferProviderShared, so I propose that we wait a few weeks to see how much I managed to remove it and then reevaluate whether it's worth adding valid region tracking here.

::: gfx/layers/PersistentBufferProvider.cpp
@@ +70,5 @@
> +  return mDirtyRect;
> +}
> +
> +void
> +PersistentBufferProviderBasic::SetDirtyRect(const gfx::Rect& aRect)

What you are adding helps with optimizing the copy from the canvas to a TextureClient, but with PersistentBufferProviderShared we remove this copy that you are optimizing entirely (canvas draws directly in a TextureClient, and do a copy-on-write at the beginning of the next frame if needed).

I don't think it is worth it for you to spend time and effort optimizing this copy since it will hopefully be removed entirely soon in most cases.

Comment 3

[Nicolas Silva [:nical]]

If you want to help with optimizing canvas copies, you can enable the pref "layers.shared-buffer-provider.enabled", and help me find the eventual bugs and inefficiencies.

For example there are configurations with which PersistentBufferProvuiderShared will be disabled by default at first and that need investigation:
 * when using the basic compositor
 * when skiagl is enabled

We should fix those, ping me on irc if you are interested, and I'll give you an overview of the missing pieces.

Comment 4

[Ethan Lin[:ethlin]]

Per discussion with nical, I should integrate dirty region to PersistentBufferProvuiderShared if it's really helpful. I will try to do that and do the performance test on windows.

Comment 5

[Ethan Lin[:ethlin]]

Attachment: calculate dirty rect

I add dirty rect for PersistentBufferProviderShared. I tried the test case in bug 1161818, dirty rect calculation can keep the fps to 60fps on windows while the dimension is 8000x8000. Though I'm not sure how many web site use incremental update. And setting dirty rect for each canvas api is a little bit annoying.

Comment 6

[Ethan Lin[:ethlin]]

Attachment: testcase

Testcase from bug 1161818. I fix the size to 8000x8000.

Comment 7

[Nicolas Silva [:nical]]

Comment on attachment 8779641 [details] [diff] [review]
calculate dirty rect

Review of attachment 8779641 [details] [diff] [review]:
-----------------------------------------------------------------

This is quite complicated for something that we don't know for sure to be useful in practice (I mean it helps in your test case, but we don't know how often canvases update incrementally in real sites, and when they do, if the canvases are big enough for this to make a difference). Considering the number of invalidation bugs we run into with painted layers, I want to be sure before adding this kind of complexity. Invalidation bugs are usually a lot harder to debug than an average crash.

When we talked about dirty rects earlier I thought you meant to replace the persisted rect parameter we currently use with a dirty (or covered) rect to be able to copy only parts of the canvas (currently we copy either everything or nothing), I didn't think you meant to track invalid regions across frames.

I think that we should focus on simpler optimizations first and collect some data to see how often the top sites would benefit from something like this, before taking the risk to introduce valid region tracking across frames.

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +1544,5 @@
> +  if (mBufferProvider) {
> +    Matrix transform = mTarget ? mTarget->GetTransform() : CurrentState().transform;
> +    gfx::Rect dirtyRect = aDirtyRect ? transform.TransformBounds(*aDirtyRect) :
> +                                       gfx::Rect(0, 0, mWidth, mHeight);
> +    IntRect dirty(IntPoint::Floor(dirtyRect.x, dirtyRect.y), IntSize::Ceil(dirtyRect.width, dirtyRect.height));

nit: you can use IntRect dirty = RoundedOut(dirtyRect);

@@ +1611,5 @@
>            mTarget = Factory::CreateDrawTargetSkiaWithGrContext(glue->GetGrContext(), size, format);
>            if (mTarget) {
>              AddDemotableContext(this);
> +            //mBufferProvider = new PersistentBufferProviderBasic(mTarget);
> +            mBufferProvider = layerManager->CreatePersistentBufferProvider(size, format);

I think that this change belongs to a separate patch (a separate bug, even).

Comment 8

[Nicolas Silva [:nical]]

Adding Bas to the discussion. He probably has an opinion on this. Bas, if you think it is worth tracking invalid regions across all frames in CanvasRenderingContext2D and PersistentBuffer(kinda like we do with tiles, except we may have more than a back and a front buffer in PersistentBufferProviderShared), I'll revise my feedback on the patch.

Comment 9

[Ethan Lin[:ethlin]]

(In reply to Nicolas Silva [:nical] from comment #7)
> Comment on attachment 8779641 [details] [diff] [review]
> calculate dirty rect
> 
> Review of attachment 8779641 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is quite complicated for something that we don't know for sure to be
> useful in practice (I mean it helps in your test case, but we don't know how
> often canvases update incrementally in real sites, and when they do, if the
> canvases are big enough for this to make a difference). Considering the
> number of invalidation bugs we run into with painted layers, I want to be
> sure before adding this kind of complexity. Invalidation bugs are usually a
> lot harder to debug than an average crash.
> 
> When we talked about dirty rects earlier I thought you meant to replace the
> persisted rect parameter we currently use with a dirty (or covered) rect to
> be able to copy only parts of the canvas (currently we copy either
> everything or nothing), I didn't think you meant to track invalid regions
> across frames.
> 
> I think that we should focus on simpler optimizations first and collect some
> data to see how often the top sites would benefit from something like this,
> before taking the risk to introduce valid region tracking across frames.

Right, it's quite complicated to calculate the dirty region. Maybe we should do this after there is a real use case.

Comment 10

[Peter Chang[:pchang]]

(In reply to Nicolas Silva [:nical] from comment #8)
> Adding Bas to the discussion. He probably has an opinion on this. Bas, if
> you think it is worth tracking invalid regions across all frames in
> CanvasRenderingContext2D and PersistentBuffer(kinda like we do with tiles,
> except we may have more than a back and a front buffer in
> PersistentBufferProviderShared), I'll revise my feedback on the patch.

How about we land this with a perf off by default? Or we need to simply current solution.

Comment 11

[Nicolas Silva [:nical]]

(In reply to Peter Chang[:pchang] from comment #10)
> How about we land this with a perf off by default? Or we need to simply
> current solution.

:ethlin is currently working on lower hanging fruits, I would prefer that we hold off on adding complex code like this until we run out of simpler improvements and more importantly until we have gathered a bit of data showing that it's actually worth it. Preff'ed off code still comes with a tax on maintainability.

That said, I'll be happy to r+ the patch if we get some numbers showing that the specific case it improves happens often enough on real websites.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:lsalzman, could you have a look please?
For more information, please visit auto_nag documentation.