611799 - Prefill newly created buffers with old content

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benjamin Stover (:stechz)]

Bug 603885 removes prefilling new buffers with old content, but it turns out that we still need to do that.

Comment 1

[Benjamin Stover (:stechz)]

Attachment: Prefill newly created buffers with old content

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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!

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 4

[Benjamin Stover (:stechz)]

This would be slower though, yes?

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 6

[Benjamin Stover (:stechz)]

Attachment: Prefill newly created buffers with old content

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 8

[Benjamin Stover (:stechz)]

Pushed http://hg.mozilla.org/mozilla-central/rev/b7e6e0d314b6

Comment 9

[Joe Drew (not getting mail)]

why i oughta