The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla15

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Leman Bennett [Omega], Assigned: bas)

Tracking

Trunk
mozilla15
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Blocks: 715768

Comment 1

5 years ago
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.
(Reporter)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
(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.
Assignee: nobody → bas.schouten
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
Created attachment 616134 [details] [diff] [review]
Part 1: Add OpaqueRect API to DrawTargets.
Attachment #616134 - Flags: review?(roc)
(Assignee)

Comment 6

5 years ago
Created attachment 616136 [details] [diff] [review]
Part 2: Manage OpaqueRect in BasicLayers for Azure-Thebes.
Attachment #616136 - Flags: review?(roc)
(Assignee)

Comment 7

5 years ago
Created attachment 616138 [details] [diff] [review]
Part 3: Fix up PushGroupAndCopyBackground to behave consistent with non-Azure.
Attachment #616138 - Flags: review?(roc)
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!
Maybe it would make more sense to just have a userdata API for DrawTargets and attach the opaque rect with that API?
(Assignee)

Comment 10

5 years ago
(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.)
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.
(Assignee)

Comment 12

5 years ago
(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.
(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

5 years ago
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.
(Reporter)

Comment 15

5 years ago
(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.
(Assignee)

Comment 16

5 years ago
(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

5 years ago
(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 :)
(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"?
(Assignee)

Comment 19

5 years ago
(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.'
'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.
(Assignee)

Comment 21

5 years ago
(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.
(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 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"?
Attachment #616134 - Flags: review?(roc) → review+
(Assignee)

Comment 24

5 years ago
(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 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 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
Attachment #616138 - Flags: review?(roc) → review+
(Assignee)

Comment 27

5 years ago
Created attachment 620084 [details] [diff] [review]
Part 2: Manage OpaqueRect in BasicLayers for Azure-Thebes. v2
Attachment #616136 - Attachment is obsolete: true
Attachment #616136 - Flags: review?(roc)
Attachment #620084 - Flags: review?(roc)
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);
Attachment #620084 - Flags: review?(roc) → review+
(Assignee)

Comment 29

5 years ago
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
(Assignee)

Comment 30

5 years ago
Followup to address comment 28:

https://hg.mozilla.org/integration/mozilla-inbound/rev/43b2b050af51
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
(Assignee)

Updated

5 years ago
No longer blocks: 715768
(Assignee)

Updated

5 years ago
Blocks: 715768
You need to log in before you can comment on or make changes to this bug.