430450 - vertical stripes in windowless plugins

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[Benjamin Otte]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en; rv:1.9b4) Gecko Epiphany/2.22
Build Identifier: 

This is a problem that only happens for plugins that use windowless mode, which is why I can only tell how to reproduce it with swfdec-mozilla. I tried reproducing it with diamondx, but I didn't manage to do make it appear on my simple HTML test page.

Reproducible: Always

Steps to Reproduce:
1. Use Firefox on Linux (other Gecko browsers should work fine, too)
2. install swfdec-mozilla plugin from Swfdec git. 
2. Go to www.youtube.com
3. If the Flash doesn't start playing immediately, click the play button.
4. If the bug isn't visible resize the browser window.
Actual Results:  
drawn areas show a 1-pixel wide horizontal black stripe at the left side. (Sometimes at the right side, too?).
You can see this best for windows that repaint random areas (as the youtube home page does), because then the stripes become more. Otherwise it's just a 1-pixel wide line at the left. The lines seems to be drawn in the color used as background for the current area, so positioning the EMBED in a "background-color: green" DIV might help for test cases.

Expected Results:  
No visual glitches.

I did this testing on Ubuntu Hardy.

Comment 1

[Larry Ewing]

We're seeing this on ff3b5 with moonlight

Comment 2

[Larry Ewing]

from a quick look at the code it looks a lot like nsObjectFrame might have some of the rectangle rounding problems that roc blogged about a while ago such that in some circumstances the rounding comes out wrong.

Comment 3

[Larry Ewing]

Attachment: partial patch

This fixes the specific problem for me but there are several other suspicious rounding errors in nsObjectFrame.cs

Comment 4

[Mike Melanson]

I'm pretty sure this is the same thing I am seeing in the official Flash Player while developing windowless mode support. The same problem does not occur under Opera.

Comment 5

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

Opera has windowless support on X? Cool.

Comment 6

[Mike Melanson]

Yep, Opera (9.5 beta, at least) has windowless support on X. Transparency, too.

Comment 7

[Karl Tomlinson (:karlt)]

I can reproduce with the unixprinting sample plugin patched with attachment 270128 [details] [diff] [review] on the page in attachment 169652 [details].

The effect is visible after resetting page zoom, then (non-text-only) zooming two steps, then dragging another window horizontally or vertically across the plugin area.

It's as if the outermost pixels that the plug draws are applied with
an alpha < 1.

Comment 8

[Karl Tomlinson (:karlt)]

The origin of the frame is not necessarily aligned to the browser's device
pixels.  What seems to be happening is that gfxXlibNativeRenderer gives the
plugin a drawable with pixels aligned to the origin of the frame, and then
smudges the result into the browser's device pixels.

(In reply to comment #3)
> Created an attachment (id=319501) [details]
> partial patch

I'm not confident rounding out to browser pixels is the right thing to do for
pluginDirtyRect because that is used for plugin pixels:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsObjectFrame.cpp&rev=1.653&mark=4143-4146#4143

The offset between browser and plugin pixels would need to be taken into
account.

--

But really I suspect the right thing to do is to align plugin pixels with
browser pixels.  Maybe this is a good place to do that:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsObjectFrame.cpp&rev=1.653&mark=1093,1095#1090

I'll do some experimenting.

--

As a related issue, window width and height advertised to the plugin are
obtained by rounding frame width and height, without considering offsets in
the origin.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsObjectFrame.cpp&rev=1.653&mark=894-895#889

This is inconsistent with the sizing of widgets for windowed plugins which
round the rectangle bounds:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/view/src/nsView.cpp&rev=3.260&mark=385#360

I suspect something like nsRect::ScaleRoundPreservingCentersInverse would be
better for nsObjectFrame::FixupWindow too.

Comment 9

[Karl Tomlinson (:karlt)]

(In reply to comment #8)
> (In reply to comment #3)
> > Created an attachment (id=319501) [details] [details]
> > partial patch
> 
> I'm not confident rounding out to browser pixels is the right thing to do for
> pluginDirtyRect because that is used for plugin pixels:

Oh, sorry.  That is _plugin_ pixels that you are rounding to, so that seems (part of) the right thing to do for X11.  I'll think about Windows/Mac.

> But really I suspect the right thing to do is to align plugin pixels with
> browser pixels.

I think we should do this too.  It would be more efficient and look better within the plugin.  There may be some issues round the edges of the plugin rect, but no worse than with windowed plugins.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Marking wanted, feel free to adjust the priority if you disagree.

Comment 11

[Karl Tomlinson (:karlt)]

Attachment: snap to pixels, round dirty rect out, draw only dirty

There a 3 changes here:

1) When the only graphics transform is a translation, snap the plugin rect to
   browser device pixels.  (This is similar to what happens with images.)

   This was enough to solve the problem in the situations that I tested
   because the dirty rect is aligned to browser device pixels.  But perhaps
   this is not always the case - consider an expose event after an unaligned
   partially-covering frame (e.g. menu).

   This also save creating temporary xlib surfaces and means that plugin need
   only draw once instead of twice (to get alpha info).

2) For general transforms and (if there are) non-aligned dirty rects in
   translation-only transforms, round out the dirty rect to plugin pixels.

3) Renderer::Draw need only draw the dirty rect.  Previously temporary xlib
   surfaces were the size of the plugin rect.

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: snap to pixels, round dirty rect out, draw only dirty v1.1

Corrected use of width for height.  Added some comments.

Comment 13

[Karl Tomlinson (:karlt)]

Tryserver build with attachment 324914 [details] [diff] [review] and attachment 324752 [details] [diff] [review]:

https://build.mozilla.org/tryserver-builds/2008-06-12_19:[e-mail]-lines-null-wsinfo-2/

Comment 14

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

+  nsPresContext* presContext = mOwner->PresContext();
+  gfxRect dirtyGfxRect(presContext->AppUnitsToGfxUnits(aDirtyRect.x),
+                       presContext->AppUnitsToGfxUnits(aDirtyRect.y),
+                       presContext->AppUnitsToGfxUnits(aDirtyRect.width),
+                       presContext->AppUnitsToGfxUnits(aDirtyRect.height));

Mind adding an nsPresContext method gfxRect AppUnitsToGfxUnits(const nsRect&)? We have this pattern in a few places already. You don't have to convert existing users though (although it would be great if you did in a followup patch).

I think you really want to use gfxContext::UserToDevicePixelSnapped here.

Mind breaking item #3 out into a separate patch?

Comment 15

[Karl Tomlinson (:karlt)]

Comment on attachment 324914 [details] [diff] [review]
snap to pixels, round dirty rect out, draw only dirty v1.1

(In reply to comment #14)
> +  nsPresContext* presContext = mOwner->PresContext();
> +  gfxRect dirtyGfxRect(presContext->AppUnitsToGfxUnits(aDirtyRect.x),
> +                       presContext->AppUnitsToGfxUnits(aDirtyRect.y),
> +                       presContext->AppUnitsToGfxUnits(aDirtyRect.width),
> +                       presContext->AppUnitsToGfxUnits(aDirtyRect.height));
> 
> Mind adding an nsPresContext method gfxRect AppUnitsToGfxUnits(const nsRect&)?
> We have this pattern in a few places already. You don't have to convert
> existing users though (although it would be great if you did in a followup
> patch).

That sounds a good idea.
Do we need it on the nsIDeviceContext as well or instead?

> I think you really want to use gfxContext::UserToDevicePixelSnapped here.

It didn't really seem to do what we need.

We need to RoundOut() the dirty rect to plugin pixels.

If aDirtyRect is ever not aligned to browser pixels then perhaps we should
first RoundOut() to browser pixels, but that doesn't really seem to be the
role of nsPluginInstanceOwner::Paint, which should only need to paint what it
is told to paint. i.e. it should be told to paint full browser pixels and that
seems to be what is happening.

UserToDevicePixelsSnapped rounds to the nearest pixel, whereas dirty rects
should be rounded out.

We should clip to the dirty rect before it is rounded out to plugin pixels, so
that when the plugin over-draws, the NativeRenderer doesn't half draw the
surrrounding pixels.  But the clip rect already seems to be set appropriately.
Do other frames need to worry about clipping their drawing to the dirty rect?

> Mind breaking item #3 out into a separate patch?

Thought you might ask that ;).
I don't think that will be too much trouble.

Comment 16

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

(In reply to comment #15)
> (From update of attachment 324914 [details] [diff] [review])
> (In reply to comment #14)
> > +  nsPresContext* presContext = mOwner->PresContext();
> > +  gfxRect dirtyGfxRect(presContext->AppUnitsToGfxUnits(aDirtyRect.x),
> > +                       presContext->AppUnitsToGfxUnits(aDirtyRect.y),
> > +                       presContext->AppUnitsToGfxUnits(aDirtyRect.width),
> > +                       presContext->AppUnitsToGfxUnits(aDirtyRect.height));
> > 
> > Mind adding an nsPresContext method gfxRect AppUnitsToGfxUnits(const nsRect&)?
> > We have this pattern in a few places already. You don't have to convert
> > existing users though (although it would be great if you did in a followup
> > patch).
> 
> That sounds a good idea.
> Do we need it on the nsIDeviceContext as well or instead?

I think it should be fine on the prescontext. Few places have only a devicecontext.

> > I think you really want to use gfxContext::UserToDevicePixelSnapped here.
> 
> It didn't really seem to do what we need.
> 
> We need to RoundOut() the dirty rect to plugin pixels.
> 
> If aDirtyRect is ever not aligned to browser pixels then perhaps we should
> first RoundOut() to browser pixels, but that doesn't really seem to be the
> role of nsPluginInstanceOwner::Paint, which should only need to paint what it
> is told to paint. i.e. it should be told to paint full browser pixels and that
> seems to be what is happening.
> 
> UserToDevicePixelsSnapped rounds to the nearest pixel, whereas dirty rects
> should be rounded out.

OK, here's what I think it should do. First make the dirty rect relative to the plugin rect and round out to device pixel boundaries, unconditionally, no matter what the current matrix is. Then we should be snapping the plugin rect using UserToDevicePixelsSnapped. (We may need to do that at invalidation time too so that we invalidate the right area.) Then we should be in good shape.

> We should clip to the dirty rect before it is rounded out to plugin pixels, so
> that when the plugin over-draws, the NativeRenderer doesn't half draw the
> surrrounding pixels.  But the clip rect already seems to be set appropriately.
> Do other frames need to worry about clipping their drawing to the dirty rect?

No.

Comment 17

[Karl Tomlinson (:karlt)]

(In reply to comment #16)
Maybe I'm missing something here, but I can't see how to make this work:

> OK, here's what I think it should do. First make the dirty rect relative to
> the plugin rect and round out to device pixel boundaries, unconditionally,
> no matter what the current matrix is.

To transform the dirty rect from frame to plugin coords we need to know the
position of the plugin, which depends on the snapping:

> Then we should be snapping the plugin rect using
> UserToDevicePixelsSnapped.

And part of the reason for operating directly on the transformation matrix
instead of calculating an offset then translating like

  snappedx = round(oldx + x0) - x0;
  x0 += snappedx - oldx;

is that any rounding errors would cause a temporary surface to be created in
the native renderer.
Perhaps we can be sure that x0 becomes an integer here because we can use
oldx = 0?
But it seems clearer if x0 is rounded:

  x0 = round(x0);

> (We may need to do that at invalidation time too so that we invalidate the
> right area.)

Good point.  I'll check that out.

Comment 18

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

Hmm.

We should probably avoid adding a translation to the rendering context before calling nsPluginInstanceOwner::Paint. Elsewhere in layout we've basically stopped doing that, to make it easier to snap things to device pixels.

Like images, let's use UserToDevicePixelsSnapped to figure out the screen rectangle where the plugin should be rendered.

Then you can take the dirty rect and scale it to devpixel units, rounding out. Then we have the plugin rect and the dirty rect in the same coordinate system. In the common case, when there is only a translation in the matrix, both will be pixel-aligned. And we've done it without looking directly at the matrix. How does that sound.

Comment 19

[Karl Tomlinson (:karlt)]

Attachment: nsPresContext::AppUnitsToGfxUnits(const nsRect&)

(In reply to comment #14)
> Mind adding an nsPresContext method gfxRect AppUnitsToGfxUnits(const nsRect&)?

Comment 20

[Karl Tomlinson (:karlt)]

Attachment: snap to pixels, round dirty rect out

There are 2 changes here:

1) When the only graphics transform is a translation, snap the plugin rect to
   browser device pixels.

2) For general transforms, round out the dirty rect to plugin pixels.

(In reply to comment #18)
> We should probably avoid adding a translation to the rendering context before
> calling nsPluginInstanceOwner::Paint. Elsewhere in layout we've basically
> stopped doing that, to make it easier to snap things to device pixels.
> 
> Like images, let's use UserToDevicePixelsSnapped to figure out the screen
> rectangle where the plugin should be rendered.
> 
> Then you can take the dirty rect and scale it to devpixel units, rounding out.
> Then we have the plugin rect and the dirty rect in the same coordinate system.

> In the common case, when there is only a translation in the matrix, both
> will be pixel-aligned. And we've done it without looking directly at the
> matrix. How does that sound.

Done.
I was concerned about how that would work with this translation,

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/view/src/nsViewManager.cpp&rev=3.485&mark=491-496#491

but Rob has assured me there is no need to deal with cases when these offsets
are not integer.

Comment 21

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

Comment on attachment 325703 [details] [diff] [review]
snap to pixels, round dirty rect out

nits:

+  // With translation-only transmation matrices, frameRect is already

typo

+  if (PRInt32(mWindow->width) != mPluginSize.width ||
+      PRInt32(mWindow->height) != mPluginSize.height) {

nsIntSize(mWindow->width, mWindow->height) != mPluginSize

Looks good otherwise.

Comment 22

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

We should consider this for 3.0.1. The risk for non-X11 and non-windowless plugins is extremely low. And we don't want to cause trouble for any upcoming releases of windowless X11 plugins.

Comment 23

[Karl Tomlinson (:karlt)]

Attachment: snap to pixels, round dirty rect out v2.1

(In reply to comment #21)
> (From update of attachment 325703 [details] [diff] [review])
> nits

Comment 24

[Karl Tomlinson (:karlt)]

(In reply to comment #16)
> ... we should be snapping the plugin rect
> using UserToDevicePixelsSnapped. (We may need to do that at invalidation time
> too so that we invalidate the right area.)

nsRect::ScaleRoundOut should take care of that here:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/view/src/nsViewManager.cpp&rev=3.485&mark=2052#2034

Comment 25

[Benjamin Otte]

(In reply to comment #22)
> We should consider this for 3.0.1. The risk for non-X11 and non-windowless
> plugins is extremely low. And we don't want to cause trouble for any upcoming
> releases of windowless X11 plugins.
> 
I'm strongly in favour of this, as I want to enable windowless mode in Swfdec with the next GNOME release, and don't want to resort to user agent sniffing.

Comment 26

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

Hmm, Karl, can you land this?

Comment 27

[Karl Tomlinson (:karlt)]

pushed to mozilla-central:
http://hg.mozilla.org/index.cgi/mozilla-central/rev/43296bfd03e6
http://hg.mozilla.org/index.cgi/mozilla-central/rev/953a299d8c32
http://hg.mozilla.org/index.cgi/mozilla-central/rev/5a86109c609b
http://hg.mozilla.org/index.cgi/mozilla-central/rev/4a02f4e3366d

(We should get this in for 1.9.0.2 when this and bug 435764 have had some bake time on central.)

Comment 28

[Karl Tomlinson (:karlt)]

Requesting blocking 1.9.0.2 because both swfdec and Adobe now have windowless-capable Flash plugins available (though Adobe's at least is still in beta), and moonlight also has a windowless plugin.

As well as the visible lines, there is a significant and noticeable performance impact here.

Comment 29

[Karl Tomlinson (:karlt)]

(In reply to comment #11)
> 3) Renderer::Draw need only draw the dirty rect.  Previously temporary xlib
>    surfaces were the size of the plugin rect.

(In reply to comment #14)
> Mind breaking item #3 out into a separate patch?

Filed bug 444498.

Comment 30

[Samuel Sidler (old account; do not CC)]

Karl, can you attach a safe 1.9.0.x patch for this? Flash 10 is now at RC. We might want to take this...

Comment 31

[Karl Tomlinson (:karlt)]

Attachment: patch for 1.9.0

This is the same code that landed on trunk.  The only difference is in
surrounding context due to other patches landing in a different order.  The
code has been well tested on trunk, so making any changes would only add risk.

This should be taken with the patch for bug 445707.

Comment 32

[Samuel Sidler (old account; do not CC)]

Comment on attachment 333872 [details] [diff] [review]
patch for 1.9.0

Approved for 1.9.0.2. Please land in CVS when the tree is green. a=ss

Comment 33

[Samuel Sidler (old account; do not CC)]

Oh, and can we get a test for this or no, because it uses plugins?

Comment 34

[Karl Tomlinson (:karlt)]

It's plugins, unfortunately.  Bug 386144 would help with this.

Comment 35

[Samuel Sidler (old account; do not CC)]

fwiw, we won't block on this, but the patch is approved.

Comment 36

[Karl Tomlinson (:karlt)]

Checked-in to CVS:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1218776154&maxdate=1218776280&who=karlt%2B%25karlt.net

Comment 37

[Bernd]

this patch causes  4 compiler warnings C4244 for roughly every file under layout as it includes nsPresContext.h

Comment 38

[Karl Tomlinson (:karlt)]

(In reply to comment #37)
> this patch causes  4 compiler warnings C4244 for roughly every file under
> layout as it includes nsPresContext.h

Which patch?
Is it the inclusion of gfxRect.h?

Can you paste an error message, please?
I'm having trouble finding one on the tinderbox logs.