Last Comment Bug 738413 - [Azure] <div> backgrounds are sometimes corrupted or not updated
: [Azure] <div> backgrounds are sometimes corrupted or not updated
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla15
Assigned To: Bas Schouten (:bas.schouten)
:
Mentors:
Depends on:
Blocks: 715768
  Show dependency treegraph
 
Reported: 2012-03-22 12:43 PDT by Leman Bennett [Omega]
Modified: 2012-05-20 13:41 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Add OpaqueRect API to DrawTargets. (854 bytes, patch)
2012-04-18 07:54 PDT, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Splinter Review
Part 2: Manage OpaqueRect in BasicLayers for Azure-Thebes. (4.06 KB, patch)
2012-04-18 07:55 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 3: Fix up PushGroupAndCopyBackground to behave consistent with non-Azure. (4.87 KB, patch)
2012-04-18 07:56 PDT, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Splinter Review
Part 2: Manage OpaqueRect in BasicLayers for Azure-Thebes. v2 (6.76 KB, patch)
2012-05-01 15:08 PDT, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Splinter Review

Description Leman Bennett [Omega] 2012-03-22 12:43:04 PDT
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 2012-03-29 15:17:31 PDT
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] 2012-04-16 07:38:13 PDT
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) 2012-04-18 06:42:57 PDT
(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) 2012-04-18 07:09:37 PDT
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) 2012-04-18 07:54:13 PDT
Created attachment 616134 [details] [diff] [review]
Part 1: Add OpaqueRect API to DrawTargets.
Comment 6 Bas Schouten (:bas.schouten) 2012-04-18 07:55:11 PDT
Created attachment 616136 [details] [diff] [review]
Part 2: Manage OpaqueRect in BasicLayers for Azure-Thebes.
Comment 7 Bas Schouten (:bas.schouten) 2012-04-18 07:56:25 PDT
Created attachment 616138 [details] [diff] [review]
Part 3: Fix up PushGroupAndCopyBackground to behave consistent with non-Azure.
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-18 17:55:38 PDT
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) 2012-04-18 18:56:10 PDT
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) 2012-04-25 08:10:23 PDT
(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) 2012-04-25 19:47:20 PDT
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) 2012-04-25 19:55:02 PDT
(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) 2012-04-25 20:19:12 PDT
(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 2012-04-28 11:32:02 PDT
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] 2012-04-28 11:40:09 PDT
(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) 2012-04-28 11:45:38 PDT
(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 2012-04-28 14:40:51 PDT
(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) 2012-04-28 22:43:56 PDT
(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) 2012-04-29 09:13:35 PDT
(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) 2012-04-29 15:24:26 PDT
'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) 2012-04-29 20:20:30 PDT
(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) 2012-04-29 20:24:13 PDT
(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) 2012-04-29 20:24:45 PDT
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) 2012-04-29 20:25:18 PDT
(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) 2012-04-29 20:28:51 PDT
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) 2012-04-29 20:32:08 PDT
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) 2012-05-01 15:08:06 PDT
Created attachment 620084 [details] [diff] [review]
Part 2: Manage OpaqueRect in BasicLayers for Azure-Thebes. v2
Comment 28 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-01 15:43:01 PDT
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 30 Bas Schouten (:bas.schouten) 2012-05-03 14:42:47 PDT
Followup to address comment 28:

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

Note You need to log in before you can comment on or make changes to this bug.