Last Comment Bug 765597 - Pictures on the picture slideshow on www.overclock.net become corrupted
: Pictures on the picture slideshow on www.overclock.net become corrupted
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla16
Assigned To: :Ehsan Akhgari
:
: Jet Villegas (:jet)
Mentors:
http://www.overclock.net/
Depends on:
Blocks: 157681 766116
  Show dependency treegraph
 
Reported: 2012-06-17 11:20 PDT by Mikel
Modified: 2012-06-20 22:47 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
snapshot FF13/FF16 (855.35 KB, image/png)
2012-06-17 15:08 PDT, Loic
no flags Details
Patch (v1) (1.13 KB, patch)
2012-06-19 13:47 PDT, :Ehsan Akhgari
dbaron: review+
Details | Diff | Splinter Review
snapshot FF16 after the fix (145.36 KB, image/jpeg)
2012-06-20 09:45 PDT, Loic
no flags Details

Description Mikel 2012-06-17 11:20:24 PDT
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 2012-06-17 13:57:40 PDT
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 2012-06-17 15:08:15 PDT
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
Comment 3 Mikel 2012-06-18 01:36:03 PDT
Your snapshot is exactly what I am referring to in this bug, thanks.
Comment 4 Alice0775 White 2012-06-18 02:36:20 PDT
Pushlog in mozilla-inbound tinderbox.
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f5a441d6929f&tochange=df6702c41ddd
Triggered by Bug 157681
Comment 5 :Ehsan Akhgari 2012-06-18 10:29:41 PDT
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-10 (vacation, returning December 19) 2012-06-18 11:03:07 PDT
(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-10 (vacation, returning December 19) 2012-06-18 11:03:29 PDT
There could also be something with layers that I'm not accounting for.
Comment 8 :Ehsan Akhgari 2012-06-18 11:05:39 PDT
(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) 2012-06-18 14:52:44 PDT
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 :Ehsan Akhgari 2012-06-18 20:43:10 PDT
(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) 2012-06-18 22:18:27 PDT
(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 :Ehsan Akhgari 2012-06-19 13:47:57 PDT
Created attachment 634574 [details] [diff] [review]
Patch (v1)

Yeah this seems to fix the bug.
Comment 13 David Baron :dbaron: ⌚️UTC-10 (vacation, returning December 19) 2012-06-19 13:58:25 PDT
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 :Ehsan Akhgari 2012-06-19 14:07:08 PDT
(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 16 David Baron :dbaron: ⌚️UTC-10 (vacation, returning December 19) 2012-06-19 14:12:39 PDT
(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) 2012-06-19 21:46:00 PDT
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] 2012-06-20 02:19:54 PDT
https://hg.mozilla.org/mozilla-central/rev/1cf39d1867ea
Comment 19 :Ehsan Akhgari 2012-06-20 09:32:39 PDT
(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 2012-06-20 09:45:26 PDT
Created attachment 634953 [details]
snapshot FF16 after the fix

With the latest Nightly, the "washed effect" is still visible.
Comment 21 :Ehsan Akhgari 2012-06-20 10:26:15 PDT
The fix will be in tonight's Nightly.
Comment 22 Timothy Nikkel (:tnikkel) 2012-06-20 22:47:29 PDT
(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.

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