The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 16

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Mikel, Assigned: Ehsan)

Tracking

({regression})

Trunk
mozilla16
x86_64
Windows 7
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox16+ fixed)

Details

(URL)

Attachments

(3 attachments)

(Reporter)

Description

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

Updated

5 years ago
Component: Untriaged → General

Comment 1

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

5 years ago
Created attachment 633926 [details]
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
(Reporter)

Comment 3

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

Comment 4

5 years ago
Pushlog in mozilla-inbound tinderbox.
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f5a441d6929f&tochange=df6702c41ddd
Triggered by Bug 157681
Blocks: 157681
Status: UNCONFIRMED → NEW
Component: General → Layout
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
QA Contact: untriaged → layout
tracking-firefox16: --- → ?
(Assignee)

Comment 5

5 years ago
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?
(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.)
There could also be something with layers that I'm not accounting for.
(Assignee)

Comment 8

5 years ago
(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.
If it is something to do with layers and InvalidateOverflowRect isn't enough you could try InvalidateFrameSubtree to see if that is the problem.
(Assignee)

Comment 10

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

Comment 12

5 years ago
Created attachment 634574 [details] [diff] [review]
Patch (v1)

Yeah this seems to fix the bug.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #634574 - Flags: review?(dbaron)
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
Attachment #634574 - Flags: review?(dbaron) → review+
(Assignee)

Comment 14

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

Comment 15

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cf39d1867ea
status-firefox16: --- → fixed
tracking-firefox16: ? → +
Target Milestone: --- → mozilla16
(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.
(Assignee)

Updated

5 years ago
Blocks: 766116
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.
https://hg.mozilla.org/mozilla-central/rev/1cf39d1867ea
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 19

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

5 years ago
Created attachment 634953 [details]
snapshot FF16 after the fix

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

Comment 21

5 years ago
The fix will be in tonight's Nightly.
(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.
You need to log in before you can comment on or make changes to this bug.