Closed Bug 738413 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla15

People

(Reporter: lh.bennett, Assigned: bas.schouten)

References

Details

Attachments

(3 files, 1 obsolete file)

Part 1: Add OpaqueRect API to DrawTargets.
854 bytes, patch
roc
: review+
Details | Diff | Splinter Review
Part 3: Fix up PushGroupAndCopyBackground to behave consistent with non-Azure.
4.87 KB, patch
roc
: review+
Details | Diff | Splinter Review
Part 2: Manage OpaqueRect in BasicLayers for Azure-Thebes. v2
6.76 KB, patch
roc
: review+
Details | Diff | Splinter Review 
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.
Blocks: 715768
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.
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.
(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
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 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?
(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.
(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?
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.
(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.
(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?
(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"?
(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.
(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+
(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+

        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+

    
  
No longer blocks: 715768

    
  
Blocks: 715768

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: