Prefill newly created buffers with old content

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: stechz, Assigned: stechz)

Tracking

({regression})

Trunk
regression
Points:
---

Firefox Tracking Flags

(blocking2.0 final+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
Bug 603885 removes prefilling new buffers with old content, but it turns out that we still need to do that.
(Assignee)

Comment 1

7 years ago
Created attachment 490195 [details] [diff] [review]
Prefill newly created buffers with old content
Ah ... I gave bad advice on IRC.  This backout isn't actually the best solution.  The bug here is that after the new buffer is created and old pixels are copied into it, mRegionToPaint is actually a subset of what's out of sync between back/front.  We always need to read back the entire front buffer into the back after the first swap after an re-allocation.

Nice debugging work, thanks!
One relatively simple solution to this would be, in BasicShadowableThebesLayer::PaintBuffer after a buffer realloc, just pass the layer's entire visible region to BasicManager()->PaintedThebesBuffer(), instead of aRegionToDraw.
(Assignee)

Comment 4

7 years ago
This would be slower though, yes?
As the backout?  No, it would be the same, except the copy would be off the critical path to Update()ing chrome, so it would be better.
(Assignee)

Updated

7 years ago
Attachment #490195 - Attachment is obsolete: true
(Assignee)

Comment 6

7 years ago
Created attachment 490217 [details] [diff] [review]
Prefill newly created buffers with old content
(Assignee)

Updated

7 years ago
Attachment #490217 - Flags: review?(jones.chris.g)
Comment on attachment 490217 [details] [diff] [review]
Prefill newly created buffers with old content

>   NS_OVERRIDE virtual already_AddRefed<gfxASurface>
>   CreateBuffer(Buffer::ContentType aType, const nsIntSize& aSize);
> 
>   // This describes the gfxASurface we hand to mBuffer.  We keep a
>   // copy of the descriptor here so that we can call
>   // DestroySharedSurface() on the descriptor.
>   SurfaceDescriptor mBackBuffer;
>+
>+  bool mIsNewBuffer;
> };

PRBool for roc's sake.  Ditch the newline.

> 
> void
> BasicShadowableThebesLayer::SetBackBufferAndAttrs(const ThebesBuffer& aBuffer,
>                                                   const nsIntRegion& aValidRegion,
>                                                   float aXResolution,
>                                                   float aYResolution,
>                                                   const OptionalThebesBuffer& aReadOnlyFrontBuffer,
>@@ -1524,20 +1527,29 @@ BasicShadowableThebesLayer::PaintBuffer(
>                                         void* aCallbackData)
> {
>   Base::PaintBuffer(aContext, aRegionToDraw, aRegionToInvalidate,
>                     aCallback, aCallbackData);
>   if (!HasShadow()) {
>     return;
>   }
> 
>+  nsIntRegion visibleRegion;
>+  if (mIsNewBuffer) {
>+    visibleRegion = mVisibleRegion;
>+    mIsNewBuffer = false;
>+  } else {
>+    visibleRegion = aRegionToDraw;
>+  }

Call this |updatedRegion|, and add a comment describing why we ensure it's the entire visible region after a new buffer has been allocated.
Attachment #490217 - Flags: review?(jones.chris.g) → review+
Assignee: nobody → ben
blocking2.0: --- → final+
Keywords: regression
(Assignee)

Comment 8

7 years ago
Pushed http://hg.mozilla.org/mozilla-central/rev/b7e6e0d314b6
Status: NEW → RESOLVED
blocking2.0: final+ → ---
Last Resolved: 7 years ago
Resolution: --- → FIXED
why i oughta
blocking2.0: --- → final+
You need to log in before you can comment on or make changes to this bug.