765597 - Pictures on the picture slideshow on www.overclock.net become corrupted

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mikel]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0a1
Build ID: 20120617030532

Steps to reproduce:

Trying to move to the next or the previous picture in the picture slideshow on www.overclock.net results in washed pictures.


Actual results:

The result is that the picture on the slide that is being changed from becoming visibly corrupted (washed effect). 

Also important to note that the problem persist on the latest nightly version (16.0a1), but NOT on the stable version (13.0.1) and in other browsers.


Expected results:

The pictures should be displayed properly while moving pictures in the slideshow.

Comment 1

[Loic]

Can you take 2 screeshots (FF13 and FF16) to compare?
On my side, on FF16, the slideshow is not so smooth than FF13 but colors are not washed-out.

Comment 2

[Loic]

Attachment: snapshot FF13/FF16

Anyway I succeeded to find a regression range:

mc
good=2012-06-06
bad=2012-06-07
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6338a8988917&tochange=7e4c2abb9fc9

mi
good=2012-06-05
bad=2012-06-06
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f56e2197d9cd&tochange=ec7c7be7c70d

Comment 3

[Mikel]

Your snapshot is exactly what I am referring to in this bug, thanks.

Comment 4

[Alice0775 White]

Pushlog in mozilla-inbound tinderbox.
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f5a441d6929f&tochange=df6702c41ddd
Triggered by Bug 157681

Comment 5

[(no longer active)]

Hmm, this is a relatively positioned ol element inside a relatively positioned div.  This looks like an invalidation bug, but I thought that InvalidateOverflowRect here is enough...  Is that not the case?

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Ehsan Akhgari [:ehsan] from comment #5)
> Hmm, this is a relatively positioned ol element inside a relatively
> positioned div.  This looks like an invalidation bug, but I thought that
> InvalidateOverflowRect here is enough...  Is that not the case?

I think it should be enough if the overflow rect is correct.

(I don't see the bug, so it's hard to have much of an opinion here, though.)

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

There could also be something with layers that I'm not accounting for.

Comment 8

[(no longer active)]

(In reply to David Baron [:dbaron] from comment #6)
> (I don't see the bug, so it's hard to have much of an opinion here, though.)

Clicking rapidly on the two buttons helps me see this much more often.

(In reply to David Baron [:dbaron] from comment #7)
> There could also be something with layers that I'm not accounting for.

Do you know who's familiar with the layers stuff that might be involved here.

Comment 9

[Timothy Nikkel (:tnikkel)]

If it is something to do with layers and InvalidateOverflowRect isn't enough you could try InvalidateFrameSubtree to see if that is the problem.

Comment 10

[(no longer active)]

(In reply to Timothy Nikkel (:tn) from comment #9)
> If it is something to do with layers and InvalidateOverflowRect isn't enough
> you could try InvalidateFrameSubtree to see if that is the problem.

Which frame should that be called on?  The root frame, or the frame that has just moved?  Or some other frame?

Comment 11

[Timothy Nikkel (:tnikkel)]

(In reply to Ehsan Akhgari [:ehsan] from comment #10)
> Which frame should that be called on?  The root frame, or the frame that has
> just moved?  Or some other frame?

It sounded like you have a specific InvalidateOverflowRect call in mind. But you could try the frame that moved.

Comment 12

[(no longer active)]

Attachment: Patch (v1)

Yeah this seems to fix the bug.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 634574 [details] [diff] [review]
Patch (v1)

r=dbaron, though the commit message isn't quite right, since the difference between the methods is this:

void
nsIFrame::InvalidateFrameSubtree()
{
  Invalidate(GetVisualOverflowRectRelativeToSelf());
  FrameLayerBuilder::InvalidateThebesLayersInSubtree(this);
}

void
nsIFrame::InvalidateOverflowRect()
{
  Invalidate(GetVisualOverflowRectRelativeToSelf());
}

so what you're doing "as well" is invalidating thebes layers

Comment 14

[(no longer active)]

(In reply to David Baron [:dbaron] from comment #13)
> Comment on attachment 634574 [details] [diff] [review]
> Patch (v1)
> 
> r=dbaron, though the commit message isn't quite right, since the difference
> between the methods is this:
> 
> void
> nsIFrame::InvalidateFrameSubtree()
> {
>   Invalidate(GetVisualOverflowRectRelativeToSelf());
>   FrameLayerBuilder::InvalidateThebesLayersInSubtree(this);
> }
> 
> void
> nsIFrame::InvalidateOverflowRect()
> {
>   Invalidate(GetVisualOverflowRectRelativeToSelf());
> }
> 
> so what you're doing "as well" is invalidating thebes layers

Oh, in this case, I'll remove the existing calls to InvalidateOverflowRect.

Also, this code is sort of stupid, I filed bug 766298 to improve it.

I'll fix the commit message when landing.

Comment 15

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1cf39d1867ea

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Ehsan Akhgari [:ehsan] from comment #14)
> Oh, in this case, I'll remove the existing calls to InvalidateOverflowRect.

Er, right.  I saw minus signs in the patch where there weren't any.

Comment 17

[Timothy Nikkel (:tnikkel)]

Is that really what we want to do? I was suggesting that as a way to determine what the problem was. It does decrease performance in that we repaint more content, but if it is necessary then it's necessary.

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1cf39d1867ea

Comment 19

[(no longer active)]

(In reply to Timothy Nikkel (:tn) from comment #17)
> Is that really what we want to do? I was suggesting that as a way to
> determine what the problem was. It does decrease performance in that we
> repaint more content, but if it is necessary then it's necessary.

Well, it does indeed fix the problem, which tells me we were painting too little before, right?

Comment 20

[Loic]

Attachment: snapshot FF16 after the fix

With the latest Nightly, the "washed effect" is still visible.

Comment 21

[(no longer active)]

The fix will be in tonight's Nightly.

Comment 22

[Timothy Nikkel (:tnikkel)]

(In reply to Ehsan Akhgari [:ehsan] from comment #19)
> (In reply to Timothy Nikkel (:tn) from comment #17)
> > Is that really what we want to do? I was suggesting that as a way to
> > determine what the problem was. It does decrease performance in that we
> > repaint more content, but if it is necessary then it's necessary.
> 
> Well, it does indeed fix the problem, which tells me we were painting too
> little before, right?

Yeah, but we didn't really understand exactly what was going on, which means why might be painting more than we really need.