[B2G] Add double buffering for 2D canvas

RESOLVED WONTFIX

Status

()

RESOLVED WONTFIX
6 years ago
4 years ago

People

(Reporter: pchang, Assigned: pchang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
Replace DrawTarget of 2D canvas to graphic buffer type directly to save the "UpdateSurface" call in BasicCanvasLayer.cpp.
(Assignee)

Comment 1

6 years ago
working on patch and will provide benchmark result
Assignee: nobody → pchang
(Assignee)

Comment 2

6 years ago
Created attachment 723782 [details] [diff] [review]
Use backbuffer as drawtarget

wip patch

With this patch, b2g can get about 10% improvement from below benchmarks.
[Result]
guimark2        36->41 fps
cut the rope    49->54 fps

Got one artifact issue in "cut the rope" because of inconsistent content inside two backbuffers. The reason was that this app only painted once when selected next level. 

Now I'm checking the timing to trigger BasicShadowableCanvasLayer::Paint.
(Assignee)

Comment 3

6 years ago
Created attachment 724247 [details] [diff] [review]
Use backbuffer as drawtarget v2

Create patch v2 which fixed the artifact issue.
Attachment #723782 - Attachment is obsolete: true
(Assignee)

Comment 4

6 years ago
Created attachment 724735 [details] [diff] [review]
Use backbuffer as drawtarget v3

WIP patch v3.

Fixed the side effect when launch crystalskull/cubevid.
Attachment #724247 - Attachment is obsolete: true
Attachment #724735 - Flags: review?(vladimir)
Attachment #724735 - Flags: review?(bjacob)
Sorry about the delay. I was on vacation an came back yesterday (as indicated in my bugzilla name).

We are the wrong people to review this patch. Try Bas Schouten or Nick Cameron instead.

However, we are in the process of replacing gfx/layers by new code, and are almost finished. Work is going on in this branch:

  http://hg.mozilla.org/projects/graphics

If this work is only meant to land on mozilla-central, then you should probably work off the graphics branch. If you want this relatively small patch to be backported to B2G 1.0 or 1.1 then I understand that you would want to carry on with your current approach. In any case, I'd suggest having this conversation with people who really know about that stuff: Bas Schouten or Nick Cameron or Nicolas Silva, I suppose.
Attachment #724735 - Flags: review?(vladimir)
Attachment #724735 - Flags: review?(bjacob)
(Assignee)

Comment 7

6 years ago
Thanks for comments.

For new graphic branch, I'm checking the modifcation scope.
For b2g18, I would prefer to land this patch first because it could improve 2D canvas performance about 10%.

[Result]
guimark2        36->41 fps
cut the rope    49->54 fps
(Assignee)

Comment 8

6 years ago
Created attachment 727518 [details] [diff] [review]
Use backbuffer as drawtarget for b2g18

Create patch for b2g18 branch
Attachment #724735 - Attachment is obsolete: true
Attachment #727518 - Flags: review?(ncameron)

Comment 9

6 years ago
Comment on attachment 727518 [details] [diff] [review]
Use backbuffer as drawtarget for b2g18

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

Detailed comments below, BUT, I am not sure that this is the right approach. Is it possible to do this without nsCanvasRenderingContext2DAzure.cpp knowing we are double buffered? It would be nice if the double buffering were entirely handled by the CanvasLayer.

Its hard to tell from the diffs, but is it possible to add the changes in Layers.h to just ShadowableCanvasLayer, rather than for all canvas layers? (OR all layers) I can't imagine we would need this stuff unless we are a shadowable canvas layer.

::: content/canvas/src/nsCanvasRenderingContext2DAzure.h
@@ +704,4 @@
>    // accessing it. In the event of an error it will be equal to
>    // sErrorTarget.
>    mozilla::RefPtr<mozilla::gfx::DrawTarget> mTarget;
> +  mozilla::RefPtr<mozilla::gfx::DrawTarget> mLastTarget;

mFrontTarget maybe - it is the front buffer draw target right?

::: gfx/layers/Layers.h
@@ +1527,5 @@
>    }
>  
>    /**
> +   * Returns true if the canvas surface contents register drawtargetswap
> +   * callback.

I cannot parse this comment

@@ +1534,5 @@
> +  {
> +    if (mDrawTargetSwapCallback) {
> +      return true;
> +    }
> +    return false;

return !!mDrawTargetSwapCallback;

But why not just test mDrawTargetSwapCallback where you need to rather than having this method at all

@@ +1550,5 @@
>  
>    /**
> +   * Register a callback to be called during back buffer swap.
> +   */
> +  typedef void (* DrawTargetSwapCallback)(void* aClosureData, void* data);

Please comment on what the args are for. Also s/data/aData

::: gfx/layers/basic/BasicCanvasLayer.cpp
@@ +309,4 @@
>  public:
>    BasicShadowableCanvasLayer(BasicShadowLayerManager* aManager) :
>      BasicCanvasLayer(aManager),
> +    mBufferIsOpaque(false), mBackTarget(nullptr)

two lines please

@@ +338,5 @@
>    virtual void SetBackBuffer(const SurfaceDescriptor& aBuffer)
>    {
>      mBackBuffer = aBuffer;
> +
> +#ifdef MOZ_WIDGET_GONK

Why is this ifdef'ed and nothing else is?

@@ +355,5 @@
>  
>    void DestroyBackBuffer()
>    {
> +    if (mBackTarget &&
> +        mBackBuffer.type() == SurfaceDescriptor::TSurfaceDescriptorGralloc) {

I would prefer that we don't test the SurfaceDescriptor type here, instead either enforce that we will have that type if mBackTarget is non-null, or test in FireDrawTargetSwapCallback

@@ +388,4 @@
>      return static_cast<BasicShadowLayerManager*>(mManager);
>    }
>  
> +  virtual DrawTarget* GetDrawTargetFromBackBuffer(const SurfaceDescriptor& backbuffer) {

brace on new line please

@@ +388,5 @@
>      return static_cast<BasicShadowLayerManager*>(mManager);
>    }
>  
> +  virtual DrawTarget* GetDrawTargetFromBackBuffer(const SurfaceDescriptor& backbuffer) {
> +    nsRefPtr<gfxASurface> surf = ShadowLayerForwarder::OpenDescriptor(OpenMode::OPEN_READ_WRITE, backbuffer);

Please add a comment about where you close this descriptor.

Is it certainly safe and *necessary* to leave the descriptor open like this? If so, please justify in the comment.

::: gfx/layers/ipc/ShadowLayers.h
@@ +322,5 @@
> +  static already_AddRefed<gfxASurface>
> +  OpenDescriptor(OpenMode aMode, const SurfaceDescriptor& aSurface);
> +
> +  /** Make this descriptor unusable for gfxASurface clients.  A
> +   * private interface with AutoOpenSurface. */

new line for |*/|

The comment doesn't seem to match the code though.

More fundamentally, given this comment, how do you justify making this previously deliberately private interface public? Could you use AutoOpenSurface instead?
Attachment #727518 - Flags: review?(ncameron)

Updated

6 years ago
Summary: [B2G] Replace 2D canvas DrawTarget to graphic buffer to save redundant rendering cost → [B2G] Add double buffering for 2D canvas

Comment 10

6 years ago
Bas - thoughts on this?
Flags: needinfo?(bas)
Doesn't this require canvas to be repainting the entire buffer contents with opaque pixels every frame?

If the canvas isn't doing that (and a lot of them don't), then we'd need to copy back pixels from the front buffer.
(Assignee)

Comment 12

6 years ago
It is not easy to hide the double buffer knowledge in nsCanvasRenderingContext2DAzure.cpp. Because the drawtarget created in EnsureTarget was used before GetCanvasLayer call. 

Therefore, I tried to invoke the callback in CanvasLayer to replace drawtarget used in nsCanvasRenderingContext2D. Maybe we can hack the drawtarget replacement in existed callback – FireDidTransactionCallback but it requires the sequence changes (move FireDidTransactionCallback to setbackbuffer).

(In reply to Nick Cameron [:nrc] from comment #9)
> Comment on attachment 727518 [details] [diff] [review]
> Use backbuffer as drawtarget for b2g18
> 
> Review of attachment 727518 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Detailed comments below, BUT, I am not sure that this is the right approach.
> Is it possible to do this without nsCanvasRenderingContext2DAzure.cpp
> knowing we are double buffered? It would be nice if the double buffering
> were entirely handled by the CanvasLayer.
>
(Assignee)

Comment 13

6 years ago
When paint is called first time, canvas content will be copied to backbuffer by UpdateSurface because mBackTarget is still nullptr.

After first paint, all draw calls are issued to backbuffer directly.

BasicShadowableCanvasLayer::Paint(gfxContext* aContext, Layer* aMaskLayer)
  if (mBackTarget) {
    Painted();
    mBackTarget->Flush();
    ShadowLayerForwarder::CloseDescriptor(mBackBuffer);
    mBackTarget = nullptr;
  } else {
    AutoOpenSurface autoBackSurface(OPEN_READ_WRITE, mBackBuffer);
    UpdateSurface(autoBackSurface.Get(), nullptr);
  }


(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> Doesn't this require canvas to be repainting the entire buffer contents with
> opaque pixels every frame?
> 
> If the canvas isn't doing that (and a lot of them don't), then we'd need to
> copy back pixels from the front buffer.
I'm not sure how that fixes anything.

Consider the case where both the front and back buffers are clear.

We then paint full red into the entire (back) buffer, and swap buffers. Now the front buffer is red, and the back buffer is clear.

We then paint 50% opacity blue into the buffer, expecting it to be blended on top of red, but it's instead blended on top of transparent pixels.

What we do for content drawing (since it has the same issue), is copy back the red pixels from front buffer -> back buffer when we swap. Given that canvas will often be copying back the entire frame, I doubt this will actually perform any better.
(Assignee)

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
Flags: needinfo?(bas)
You need to log in before you can comment on or make changes to this bug.