621156 - Painting problems in www.mtv3.fi when hardware acceleration and Flash are enabled

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Only some parts of the page is painted until mouse is moved over it.

Mobility Radeon HG 5650 / 8.713

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

If I disable Flash, the page is painted properly.

B8 seems to work ok.

Comment 2

[Alice0775 White]

Graphics:      
Adapter Description : ATI Radeon HD 4300/4500 Series
Vendor ID : 1002
Device ID : 954f
Adapter RAM : 512
Adapter Drivers : aticfx64 aticfx64 aticfx32 aticfx32 atiumd64 atidxx64
atiumdag atidxx32 atiumdva atiumd6a atitmm64
Driver Version : 8.791.0.0
Driver Date : 
Direct2D Enabled : true
DirectWrite Enabled : true
GPU Accelerated Windows : 1/1 Direct3D 10


Regression window(cached hourly):
Works:
http://hg.mozilla.org/mozilla-central/rev/76f51caf3a16
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20101215 Firefox/4.0b9pre ID:20101215122517
Fails:
http://hg.mozilla.org/mozilla-central/rev/965edccda61c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20101215 Firefox/4.0b9pre ID:20101215124105
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=76f51caf3a16&tochange=965edccda61c

Comment 3

[Bas Schouten (:bas.schouten)]

So Roc might understand this better than I do. For some reason, this is all broken when CairoImageD3D10 returns an invalid D2D surface (the surface created from our texture doesn't actually work). I'm fixing this to return a working surface, however I wonder why the page is fine if it just returns 0.

As a matter of fact, I don't understand why it's drawing the CairoImage with basic layers at all! The responsible stack trace is:

 	xul.dll!mozilla::layers::CairoImageD3D10::GetAsSurface()  Line 458	C++
 	xul.dll!mozilla::layers::ImageContainerD3D10::GetCurrentAsSurface(gfxIntSize * aSize=0x0b46256c)  Line 154 + 0x25 bytes	C++
 	xul.dll!mozilla::layers::BasicImageLayer::GetAndPaintCurrentImage(gfxContext * aContext=0x0b008e30, float aOpacity=1.0000000)  Line 640 + 0x2b bytes	C++
 	xul.dll!mozilla::layers::BasicImageLayer::Paint(gfxContext * aContext=0x0b008e30, void (mozilla::layers::ThebesLayer *, gfxContext *, const nsIntRegion &, const nsIntRegion &, void *)* aCallback=0x0ffbdad0, void * aCallbackData=0x003ebcc8)  Line 630 + 0x28 bytes	C++
 	xul.dll!mozilla::layers::BasicLayerManager::PaintLayer(mozilla::layers::Layer * aLayer=0x0b462468, void (mozilla::layers::ThebesLayer *, gfxContext *, const nsIntRegion &, const nsIntRegion &, void *)* aCallback=0x0ffbdad0, void * aCallbackData=0x003ebcc8)  Line 1287	C++
 	xul.dll!mozilla::layers::BasicLayerManager::EndTransaction(void (mozilla::layers::ThebesLayer *, gfxContext *, const nsIntRegion &, const nsIntRegion &, void *)* aCallback=0x0ffbdad0, void * aCallbackData=0x003ebcc8)  Line 1204	C++
>	xul.dll!mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer * aLayer=0x0b075958, gfxContext * aContext=0x0b008e30, const nsIntRegion & aRegionToDraw={...}, const nsIntRegion & aRegionToInvalidate={...}, void * aCallbackData=0x003ebcc8)  Line 1693	C++
 	xul.dll!mozilla::layers::ThebesLayerD3D10::DrawRegion(const nsIntRegion & aRegion={...})  Line 294 + 0x27 bytes	C++
 	xul.dll!mozilla::layers::ThebesLayerD3D10::Validate()  Line 230	C++
 	xul.dll!mozilla::layers::ContainerLayerD3D10::Validate()  Line 340	C++
 	xul.dll!mozilla::layers::ContainerLayerD3D10::Validate()  Line 340	C++
 	xul.dll!mozilla::layers::LayerManagerD3D10::Render()  Line 452	C++

Comment 4

[Bas Schouten (:bas.schouten)]

Attachment: Return usable surface from GetCurrentAsSurface

So, the surface we return currently is created from the IMMUTABLE texture that we create initially from our cairo source surface. D2D can't actually deal with that type of texture though so everything breaks. This copies the texture data to a usable texture when GetCurrentAsSurface is used. This has some small performance penalty but this should be ok since this is a rare fallback path.

We need to understand why this path is being called though! It seems bad for the BasicLayerManager to be drawing this when the plugin is already being drawn by the window layer manager. Roc, are we not ignoring drawing image layers which exist at the same position in the window layer manager (i.e. presumably the window layer manager will draw them anyway on top of the ThebesLayer that is now drawing them?) And will this not interact badly with partially transparent pixels? (they get drawn once with the BasicLayerManager to the ThebesLayer and once more by the window layer manager and composited on top of the ThebesLayer)

In the past when we just returned NULL the whole problem was avoided because it wouldn't get drawn by the BasicLayerManager. Now that we've fixed that though...

Comment 5

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

set MOZ_DUMP_PAINT_LIST=1 and get a layer tree/display list dump. That'll show us why fallback is being called.

> It seems bad for
> the BasicLayerManager to be drawing this when the plugin is already being drawn
> by the window layer manager.

Do you know for sure that the plugin is being painted twice?

Comment 6

[Bas Schouten (:bas.schouten)]

(In reply to comment #5)
> set MOZ_DUMP_PAINT_LIST=1 and get a layer tree/display list dump. That'll show
> us why fallback is being called.
> 
> > It seems bad for
> > the BasicLayerManager to be drawing this when the plugin is already being drawn
> > by the window layer manager.
> 
> Do you know for sure that the plugin is being painted twice?

See the above mentioned stack trace in comment 3. I don't see how that code would not be drawing it if it's calling GetCurrentAsSurface through that code. I can add a paint list dump as well if you like though? Although I don't think it's a D2D specific issue so it should be happening on this site in any configuration. (although not leading to problems if GetCurrentAsSurface returns nsnull or a valid surface)

Comment 7

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

That shows us painting the plugin via a BasicImageLayer. It doesn't show us that the plugin is being painted through an ImageLayerD3D10. Why do you think that stack shows us the plugin being painted twice?

Comment 8

[Bas Schouten (:bas.schouten)]

(In reply to comment #7)
> That shows us painting the plugin via a BasicImageLayer. It doesn't show us
> that the plugin is being painted through an ImageLayerD3D10. Why do you think
> that stack shows us the plugin being painted twice?

Oh, that's a good point actually. So we were getting an ImageLayerD3D10::RenderLayer being called, but indeed I didn't check if it was the same image. (Although considering the circumstances it looked like it!) But still that seems like a problem, the GetCurrentAsSurface call is considered a fallback. Compositing the plugins should be left to the window layer manager and should, regardless of position, etc. never occur on the ThebesLayers as far as I can see. Otherwise we'll need to optimize for GetCurrentAsSurface as a common case.

Comment 9

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

We have fallback painting paths for cases the layer manager can't handle: clipping to complex regions (e.g. border-radius combined with overflow:hidden), SVG filters and masks.

I don't know which one this page is hitting. But we can't fix all those paths for FF4. They're not common though.

Comment 10

[Bas Schouten (:bas.schouten)]

Comment on attachment 499517 [details] [diff] [review]
Return usable surface from GetCurrentAsSurface

Jeff is back from holiday.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 499517 [details] [diff] [review]
Return usable surface from GetCurrentAsSurface

Please add a comment about why we need to make a copy of the surface.

Comment 12

[Bas Schouten (:bas.schouten)]

http://hg.mozilla.org/mozilla-central/rev/9330470d6a38