556052 - Flash objects draws random vertical lines when animation changes

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[d.a.]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a4pre) Gecko/20100330 Minefield/3.7a4pre
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a4pre) Gecko/20100330 Minefield/3.7a4pre

When viewing any Flash content (such as YouTube) vertical lines appear whenever the Flash object is animated (such as the YouTube player seek bar). These lines 1px wide and may disappear whenever the Flash animation changes again. 

If viewed in fullscreen no lines appear at any time. Only when viewed within a Firefox window the lines will appear.

Screenshot: http://i39.tinypic.com/2621e2c.png

Reproducible: Always

Steps to Reproduce:
1. Go to any site with Flash contents.
2. Make the object animate (such as playing a YouTube video).
Actual Results:  
Vertical lines appear.

Expected Results:  
No vertical lines should appear.

Flash version tested: MAC 10,0,45,2
Starting Firefox/Minefield in safe-mode makes no difference, the lines still appear.
Started happening with trunk builds around early to mid March 2010.

Comment 1

[Justin Dolske [:Dolske]]

I've been seeing this too on trunk OS X builds. I thought it was just something broken in the YouTube player, but I've seen it happen on a few other sites too.

Most often, for YouTube, the problem manifests as black or white vertical lines around the throbber, or along the video scrubber when moving left and right. Almost looks like there's an off-by-one error with the size/position of a clip mask or something like that.

Comment 2

[Josh Aas]

I have seen this too.

Comment 3

[d.a.]

Tested some older builds:

2010/03/14 works, changeset: bc17358bf0ad

2010/03/15 error appears, changeset: 17a3e86d5ec5

May be bug 546071 - Loading Youtube in overflow:hidden causes 1-pixel jog

Comment 4

[Justin Dolske [:Dolske]]

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bc17358bf0ad&tochange=17a3e86d5ec5

Yeah, that or the other view-related changeset from roc seem like the most likely candidates in the range.

Comment 5

[Johnny Stenback (:jst)]

Blocking since this is a regression.

Comment 6

[Justin Dolske [:Dolske]]

Attachment: White lines

Just another example...

I see this on http://www.crateandbarrel.com/family.aspx?c=14425&f=36088, which might be helpful since it's a static picture. (although it's apparently redrawing something; the tiny white vertical line moves left-to-right every second or so). I also get lots of white lines when moving the pointer over the image... It's like the (Flash-customized) pointer has a 1px white border on the right which leaves turds when moving the cursor.

Comment 7

[Benoit Girard (:BenWa)]

I have found the problem part off bug 558350. On OSX with OOPP I only get the lines when the plug-ins X position is sufficiently large. I believe the problem is that NSAppUnitsToFloatPixels(x, float(aAppUnitsPerPixel)) is not sufficiently accurate for large values of x, causing nsRect::ToOutsidePixels (http://mxr.mozilla.org/mozilla-central/source/gfx/public/nsRect.h#341) to be off by -1. The plug-in repaint region is then off by -1 causing vertical lines where no repainting occurs.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

which call to ToOutsidePixels is this?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Also I'm not quite sure what you're getting at. Can you give some example values that are causing a problem there?

Comment 10

[Benoit Girard (:BenWa)]

All of this is happening within nsObjectFrame.cpp. nsDisplayPlugin::Paint calls nsObjectFrame::PaintPlugin using mVisibleRect as the dirty rect and GetBound(aBuilder) as the plugin/content rect.

If you follow along with http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsObjectFrame.cpp#1566 the values are:

aPluginRect (5850, 7200, 38400, 23100)
aDirtyRect (5850, 9900, 30, 16200)
contextPixels (98, 120, 640, 385)
dirtyPixels (97, 165, 1, 270)
nativeClipRect (98, 165, 0, 0)

Note that dirtyPixels 97+1 ends where contextPixels begins thus leading nativeClipRect to have width/height of 0. dirtyPixels.x should be 98 instead of 97 I think.

I'm not 100% sure if the problem has to do with the specific conversion factor but the problem happens far worse when browser window is larger and the plug-ins' X position is larger.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

ToOutsidePixels is doing the right thing here. Here, aDirtyRect doesn't contain any pixel centers so in principle we shouldn't be drawing anything, and I guess that's we get. Can you dig into how we get a 30-appunit-wide dirty rect? That seems odd (60 appunits per pixel here).

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Setting gDumpPaintList to 1 and observing the display list dump when a problematic paint occurs would probably be useful.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

and BTW since this is my regression, I very much appreciate you looking into this, and if you don't want to deal with it and it's blocking your work, I'll fix it myself ASAP

Comment 14

[Benoit Girard (:BenWa)]

I'd like to look into it actually, it's a good chance to look at the layout code a bit. I wouldn't mind a brief explanation of the relation is between pixels and appunit. Is it resolution dependant?

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

there are always 60 appunits per CSS pixel. The number of appunits per device pixel is zoom-dependent. See http://weblogs.mozillazine.org/roc/archives/2008/01/subpixel_layout.html

Comment 17

[Benoit Girard (:BenWa)]

Here is the output I get. The width starts off as 60 appunits: 

Painting --- before optimization (dirty 13920,9900,60,16200):
SolidColor 0x4f15eb0(Viewport(-1)) (0,0,86400,37320) opaque uniform
Clip 0x0() (0,0,85500,37320)
    CanvasBackground 0x578b6e0(Canvas(html)(-1)) (0,0,85500,93270) uniform
Clip 0x0() (0,0,85500,37320)
    Background 0x5785cc8(Block(body)(1)) (0,0,85500,37320) uniform
Clip 0x579e3d0(Block(div)(9)) (0,0,85500,37320)
    WrapList 0x579e3d0(Block(div)(9)) (0,7200,85500,23100)
        Background 0x57d6c98(Block(div)(7)) (0,7200,85500,23100) opaque uniform
Clip 0x57d6e00(Block(div)(1)) (0,0,85500,37320)
    WrapList 0x57d6e00(Block(div)(1)) (0,0,0,0)
Clip 0x57d6fc0(Block(div)(3)) (0,0,85500,37320)
    WrapList 0x57d6fc0(Block(div)(3)) (0,0,0,0)
Clip 0x57d7398(ObjectFrame(embed)(2)) (0,0,85500,37320)
    WrapList 0x57d7398(ObjectFrame(embed)(2)) (13950,7200,38400,23100)
        Plugin 0x57d7398(ObjectFrame(embed)(2)) (13950,7200,38400,23100)
Painting --- after optimization:
Clip 0x57d7398(ObjectFrame(embed)(2)) (0,0,85500,37320)
    Background 0x57d6c98(Block(div)(7)) (0,7200,85500,23100) opaque uniform
    Plugin 0x57d7398(ObjectFrame(embed)(2)) (13950,7200,38400,23100)
Paint plugin
aPluginRect (13950, 7200, 38400, 23100)
aDirtyRect (13950, 9900, 30, 16200)
contextPixels (233, 120, 640, 385)
dirtyPixels (232, 165, 1, 270)
nativeClipRect (233, 165, 0, 0)
CocoaEventDraw (0.000000, 45.000000, 0.000000, 0.000000)

So we have a dirtyrect at 13920 with width 60 but the ObjectFrame is at 13950 so the width of the dirtyrect within the ObjectFrame is correctly 30. I think that the real problem is that the ObjectFrame is on a sub pixel at 232.5. The dirtyrect from 232.5 to 233. In ObjectFrame.cpp:1570 we do aPluginRect.ToNearestPixels and aDirtyRect.ToOusidePixels. This results in 232.5 snapping to 233 for the contentPixels and 232 for dirtyRect. When this snapping occurs the width is off by 1.

I think the fix we should make here is in "nsRect::ToOutsidePixels". If we snap x/y to be 0.5 pixel back, then the width/height should increase by 0.5 pixel (or any fractional value). If we agree with this fix I will post a patch.

Comment 18

[Benoit Girard (:BenWa)]

My last paragraph was not accurate, the calculations performed by ToOutsidePixels are in fact correct. The real problem is that contentPixels takes the ceiling of the start position of the ObjectFrame and dirtyPixels floors it causing the values to be off by one. This seems to only happen if the ObjectFrame start position is on a subpixel.

I have tested my theory with a patch that will increase the dirtyrect's size if |aPluginRect.ToNearestPixels| rounds up the position. It fixes both this bug and bug 558350.

Comment 19

[Benoit Girard (:BenWa)]

Attachment: Fix subpixel rounding

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This patch doesn't look right to me, we shouldn't need to do something like that.

Our goal is that a pixel should be painted by the topmost element containing the center of the pixel. So here we're interested in the pixel center at x=232.5. The question is whether a rectangle whose left edge is at 232.5 contains the point 232.5. According to nsRect::ToNearestPixels it does not, because the edge at 232.5 gets snapped to 233. This might not be the best decision, but I don't see how it would cause a bug. If the plugin starts at 232.5 we should be OK painting the background at that pixel. Why would it cause these random vertical lines?

Comment 21

[Benoit Girard (:BenWa)]

Attachment: Test case showing subpixel behavior

This problem does not happen randomly, it is very specific. I have attached a testcase to demonstrate this. The testcases has two flash objects, one which is placed at 232.5px and the other which is placed at 233px. This bug causes the rendering to stop if |dom.ipc.plugins.enabled.flash player.plugin| is set so unsetting it makes it easier to see the vertical lines.

When mousing over the play icon/top left title the vertical lines only show up for the object that is played on a subpixel.

Comment 22

[Benoit Girard (:BenWa)]

I don't think the problem has to do with painting the background. I believe the problem is that nativeClipRect is a pixel smaller then it should be. In the example above we have (Replaced irrelevant numbers with _):
aPluginRect (13950, _, _, _)
aDirtyRect (13950, _, 30, _)

At this point we assume that we will be drawing at least 0.5 pixel, so 1 pixel really. I believe everything is correct at this point. The key point at this stage is that from the input of the function we expect to redraw at least one pixel WITHIN the plug-in!

contextPixels (233, _, 640, _) <-- Nearest Rounding
dirtyPixels (232, _, 1, _)     <-- Outside Rounding

So Nearest/Outside both returned the expected value. The nearest pixel to 13950 is 233 and the outside is 232. The width of contextPixel is still 640 as expected and the width of dirtyPixels was rounded up to 1 as expected. But at this stage we are no longer drawing 1 pixel WITHIN the plug-in. If we agree that based on the input aPluginRect/aDirtyRect that we expect to redraw one pixel then this step is faulty.

FYI: Chromes just rounded down the position of the object frame to 232, even if I set the position to be 232.9. I don't believe we want to do that, nearest rounding is a better solution.

Comment 23

[Benoit Girard (:BenWa)]

Attachment: Horizontal line test case

According to what I discussed above, there was no reason for the subpixel positioning error to only happen on the X axis so I tried the same test case with the Y position to be on a sub pixel and the problem reproduces. I also noticed that the object positioned at top 10.5px shows up at 12 pixels from the top.

I also tried having both the X/Y position on a subpixel. On this case we get vertical lines along the right and bottom edge of the dirtyrect.

Comment 24

[Benoit Girard (:BenWa)]

I had a lengthy discussion with Jeff about this bug. I believe that we need to clarify the behavior of the following numbers:
aPluginRect (13950, _, _, _)
aDirtyRect (13950, _, 30, _)

We can handle these numbers the following ways:
(0) We round the plug-in to start at 232px. We don't want to that I assume.
(1) We round the plug-in to start at 233px. We draw zero pixels since the dirtyrect is outside 233px. This incorrect since the layout expect at least half/one pixel to be touched.
(2) We round the plug-in to start at 233px. We update pixel 233. This is incorrect since the pixel 233 has not been invalidated before we draw over it.

Comment 25

[Josh Aas]

This is a pretty major regression and it is making it hard to test and work on Mac OS X OOPP. Can we get a fix quickly or identify a changeset to back out?

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: testcase

This testcase shows that things are pretty messed up...

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Okay this is a bit nasty...

First of all, the fix for bug 549630 was wrong. We were calculating mViewToWidgetOffset the right way around, although the calculation was slightly wrong for the case where mDimBounds.TopLeft() != (mPosX, mPosY). So I'm reverting that and adding a comment to explain why this is correct.

The real bug here, and probably the underlying bug in bug 549630, is that during the layers work revision c85d57ea1d37 removed code that translates the rendering context by ViewToWidgetOffset. We need to add that back in somewhere, so I'm exposing ViewToWidgetOffset and using it in PresShell::Paint when we paint child widgets. This seems to fix things nicely.

Comment 28

[Mats Palmgren (inactive)]

Comment on attachment 439498 [details] [diff] [review]
fix

> First of all, the fix for bug 549630 was wrong.

Sorry, I really should have seen that.

> revision c85d57ea1d37 removed code that translates the
> rendering context by ViewToWidgetOffset.

Right, and this patch adds back the translation that 
nsViewManager::Refresh used to do.  Looks good.

I didn't find any other nsIViewObservers (which would need
the same translation).

r=mats

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #28)
> (From update of attachment 439498 [details] [diff] [review])
> > First of all, the fix for bug 549630 was wrong.
> 
> Sorry, I really should have seen that.

Well, so should I, since I wrote it!

Thanks.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/4bdf5ca22519

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is rather difficult to write a test for because it only affects rendering inside a plugin's widget.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: better testcase

There should be no gray lines outside the black border of any of the plugins. Also, the borders of the plugin should all have the same thickness; every instance should look exactly the same.

Comment 33

[Timothy Nikkel (:tnikkel)]

Could somebody who could reproduce this problem try the try server build at http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-e4621282bedd/ to see if the problem is present or not in that build?