738413 - [Azure] <div> backgrounds are sometimes corrupted or not updated

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Leman Bennett [Omega]]

This is probably a dupe of Bug 715658, but I'll file just to be sure.

<div> backgrounds are sometimes garbled or not updated on scroll. Its hard to describe and examples are sparsely random. Usually images that have a transparent background contained in a <div> or backgrounds of <div> elements are either dark or leave an after image of what was scrolled through. In some rare cases, the <div> background images were corrupted. That last part sounds similar to the other progressive jpeg bug that was fixed not too long ago.

An example can be found using Google Reader. Here's a video from Fanolian who also noticed the bug: https://www.youtube.com/watch?v=xPBugPaWGoQ
Keep your eye on the gray shortcut marker next to the Title of the feed entry.

I'll add more examples once I find more consistent sources.

Comment 1

[Michael]

I can give a reliable example from the background of this page...
This one the background won't paint at all. Fine if azure is off though.

http://support.amd.com/us/Pages/Catalyst-Hotfixes.aspx

If you click on the links from the page above (any link that takes you to a scrollable page that is) scroll it up and down rapidly and you will see parts of the background simply disappear and reappear while scrolling.

This is also on on nightly windows  7 and windows 8 with HW Acceleration on.

Comment 2

[Leman Bennett [Omega]]

Here's another example:

1. Visit http://penny-arcade.com/
2. Scroll down
3. Scroll back to the top

Notice that the orange tabs bleed into the <li> background element.

Comment 3

[Bas Schouten (:bas.schouten)]

(In reply to Leman Bennett (Omega X) from comment #2)
> Here's another example:
> 
> 1. Visit http://penny-arcade.com/
> 2. Scroll down
> 3. Scroll back to the top
> 
> Notice that the orange tabs bleed into the <li> background element.

I can see this, this is very fascinating, I have no idea at this point which causes this.

Comment 4

[Bas Schouten (:bas.schouten)]

I believe this bug has to do with incorrect functioning of PushGroupAndCopyBackground. Mainly due to some oversights in implementing features there. I'm working on confirming that diagnosis.

Comment 5

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Add OpaqueRect API to DrawTargets.

Comment 6

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Manage OpaqueRect in BasicLayers for Azure-Thebes.

Comment 7

[Bas Schouten (:bas.schouten)]

Attachment: Part 3: Fix up PushGroupAndCopyBackground to behave consistent with non-Azure.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 616134 [details] [diff] [review]
Part 1: Add OpaqueRect API to DrawTargets.

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

::: gfx/2d/2D.h
@@ +785,5 @@
> +  }
> +
> +  const IntRect &GetOpaqueRect() const {
> +    return mOpaqueRect;
> +  }

This desperately needs some (precise) documentation!

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe it would make more sense to just have a userdata API for DrawTargets and attach the opaque rect with that API?

Comment 10

[Bas Schouten (:bas.schouten)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (offline April 22-25) from comment #9)
> Maybe it would make more sense to just have a userdata API for DrawTargets
> and attach the opaque rect with that API?

Ugh, I missed your review comments. Well, we could, but this information could potentially be useful for DrawTargets to, for example, know when they can do subpixel AA, or other kinds of optimizations. (i.e. I was created as an alpha surface, but my opaque rect now covers all of my surface, which can happen in PNG decoding for example. I can now memcpy rather than blend, etc.)

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, then you need to update the patch to document the precise semantics of what this means, e.g. does it mean the current pixels are already opaque alpha values, or we will set them to opaque alpha values, etc.

Comment 12

[Bas Schouten (:bas.schouten)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> OK, then you need to update the patch to document the precise semantics of
> what this means, e.g. does it mean the current pixels are already opaque
> alpha values, or we will set them to opaque alpha values, etc.

Agreed, my current thought is as follows:

'Within this rectangle all pixels will be opaque by the time the result of this DrawTarget is first used as a source for drawing.'

This would be sufficient, as any pixels that are subpixel AA'ed would, within this definition, either be overwritten by an opaque pixel or be on top of an opaque pixel once rendering is finished.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Bas Schouten (:bas) from comment #12)
> 'Within this rectangle all pixels will be opaque by the time the result of
> this DrawTarget is first used as a source for drawing.'

Can you define that a bit more precisely?

Comment 14

[Michael]

FWIW this patch has fixed all of the remaining issues for me using gfx.content.azure.enabled true here locally. Haven't hit any bad backgrounds or corrupted images for almost a week.

Comment 15

[Leman Bennett [Omega]]

(In reply to Michael from comment #14)
> FWIW this patch has fixed all of the remaining issues for me using
> gfx.content.azure.enabled true here locally. Haven't hit any bad backgrounds
> or corrupted images for almost a week.

The patches in this bug has not landed. Also, I can still reproduce the issues reported.

Comment 16

[Bas Schouten (:bas.schouten)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> (In reply to Bas Schouten (:bas) from comment #12)
> > 'Within this rectangle all pixels will be opaque by the time the result of
> > this DrawTarget is first used as a source for drawing.'
> 
> Can you define that a bit more precisely?

Hrm, I can add my explanation of why that description is sufficient? Do you have an idea of how you would you like to propose expanding the definition?

Comment 17

[Michael]

(In reply to Leman Bennett (Omega X) from comment #15)
> (In reply to Michael from comment #14)
> > FWIW this patch has fixed all of the remaining issues for me using
> > gfx.content.azure.enabled true here locally. Haven't hit any bad backgrounds
> > or corrupted images for almost a week.
> 
> The patches in this bug has not landed. Also, I can still reproduce the
> issues reported.

I am using a private build that includes this patch :)

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Bas Schouten (:bas) from comment #16)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> > (In reply to Bas Schouten (:bas) from comment #12)
> > > 'Within this rectangle all pixels will be opaque by the time the result of
> > > this DrawTarget is first used as a source for drawing.'
> > 
> > Can you define that a bit more precisely?
> 
> Hrm, I can add my explanation of why that description is sufficient? Do you
> have an idea of how you would you like to propose expanding the definition?

What exactly does it mean to "be used as a source for drawing"?

Comment 19

[Bas Schouten (:bas.schouten)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18)
> (In reply to Bas Schouten (:bas) from comment #16)
> > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> > > (In reply to Bas Schouten (:bas) from comment #12)
> > > > 'Within this rectangle all pixels will be opaque by the time the result of
> > > > this DrawTarget is first used as a source for drawing.'
> > > 
> > > Can you define that a bit more precisely?
> > 
> > Hrm, I can add my explanation of why that description is sufficient? Do you
> > have an idea of how you would you like to propose expanding the definition?
> 
> What exactly does it mean to "be used as a source for drawing"?

How about:

'Within this rectangle all pixels will be opaque by the time the result of this DrawTarget is first used for drawing. Either by the backing texture being used as the input, or a snapshot being taken.'

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

'Within this rectangle all pixels will be opaque by the time the result of this DrawTarget is first used for drawing. Either by the backing texture being used as the input, or a snapshot being taken.'

What does "the backing texture being used as the input" mean, from the point of view of a DrawTarget user who doesn't know how all the DrawTarget backends are implemented?

"A snapshot being taken" means a call to Snapshot() right? Just say Snapshot() instead.

Comment 21

[Bas Schouten (:bas.schouten)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> 'Within this rectangle all pixels will be opaque by the time the result of
> this DrawTarget is first used for drawing. Either by the backing texture
> being used as the input, or a snapshot being taken.'
> 
> What does "the backing texture being used as the input" mean, from the point
> of view of a DrawTarget user who doesn't know how all the DrawTarget
> backends are implemented?
> 
> "A snapshot being taken" means a call to Snapshot() right? Just say
> Snapshot() instead.

New attempt :)

'Within this rectangle all pixels will be opaque by the time the result of
this DrawTarget is first used for drawing. Either by the underlying surface
being used as an input to external drawing, or Snapshot() being called.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Bas Schouten (:bas) from comment #21)
> 'Within this rectangle all pixels will be opaque by the time the result of
> this DrawTarget is first used for drawing. Either by the underlying surface
> being used as an input to external drawing, or Snapshot() being called.

I'm still not completely happy with that --- what does it mean to be used as an input for external drawing? --- but I'll move on...

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 616134 [details] [diff] [review]
Part 1: Add OpaqueRect API to DrawTargets.

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

r+ with the comment we agreed on. Also, I assume it's in "device space"?

Comment 24

[Bas Schouten (:bas.schouten)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #23)
> Comment on attachment 616134 [details] [diff] [review]
> Part 1: Add OpaqueRect API to DrawTargets.
> 
> Review of attachment 616134 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with the comment we agreed on. Also, I assume it's in "device space"?

Indeed, will add this to the documentation.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 616136 [details] [diff] [review]
Part 2: Manage OpaqueRect in BasicLayers for Azure-Thebes.

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

::: gfx/layers/basic/BasicLayers.cpp
@@ +1930,4 @@
>    const nsIntRect& bounds = visibleRegion.GetBounds();
>    
>    if (aTarget->IsCairo()) {
>      const gfxRect& targetOpaqueRect = currentSurface->GetOpaqueRect();

Actually, I think this entire "if" statement needs to be inside an "if (is2D)"! Please fix!

@@ +1954,5 @@
> +        Rect(bounds.x, bounds.y, bounds.width, bounds.height));
> +      opaqueRect.RoundIn();
> +      aTarget->GetDrawTarget()->SetOpaqueRect(
> +        IntRect(int32_t(opaqueRect.x), int32_t(opaqueRect.y),
> +                int32_t(opaqueRect.width), int32_t(opaqueRect.height)));

I think something needs to be done here to avoid overflow. Something like gfxUtils::GfxRectToIntRect.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 616138 [details] [diff] [review]
Part 3: Fix up PushGroupAndCopyBackground to behave consistent with non-Azure.

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

::: gfx/thebes/gfxContext.cpp
@@ +1564,5 @@
> +      gfxRect clipRect = GetRoundOutDeviceClipExtents(this);
> +      clipExtents = IntRect(clipRect.x, clipRect.y, clipRect.width, clipRect.height);
> +    }
> +    if (mDT->GetFormat() == FORMAT_B8G8R8X8 ||
> +      mDT->GetOpaqueRect().Contains(clipExtents)) {

Fix indent

Comment 27

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Manage OpaqueRect in BasicLayers for Azure-Thebes. v2

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 620084 [details] [diff] [review]
Part 2: Manage OpaqueRect in BasicLayers for Azure-Thebes. v2

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

r+ with that

::: gfx/thebes/gfxUtils.h
@@ +129,5 @@
> +     * If aIn can be represented exactly using an nsIntRect (i.e.
> +     * integer-aligned edges and coordinates in the PRInt32 range) then we
> +     * set aOut to that rectangle, otherwise return failure.
> +    */
> +    static bool RectToIntRect(const mozilla::gfx::Rect& aIn, mozilla::gfx::IntRect* aOut);

Actually, just make this a method of Rect:
  bool ToIntRect(IntRect* aOut);

Comment 29

[Bas Schouten (:bas.schouten)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d32d6823f0af
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9a0af04be0b
https://hg.mozilla.org/integration/mozilla-inbound/rev/0187b4f0c50f

Comment 30

[Bas Schouten (:bas.schouten)]

Followup to address comment 28:

https://hg.mozilla.org/integration/mozilla-inbound/rev/43b2b050af51

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0187b4f0c50f
https://hg.mozilla.org/mozilla-central/rev/f9a0af04be0b
https://hg.mozilla.org/mozilla-central/rev/d32d6823f0af
https://hg.mozilla.org/mozilla-central/rev/43b2b050af51