Implement background copying for windowless async plugins

RESOLVED FIXED in mozilla2.0b10

Status

()

Core
Plug-ins
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: roc, Assigned: cjones)

Tracking

Trunk
mozilla2.0b10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [hardblocker])

Attachments

(32 attachments, 26 obsolete attachments)

211.42 KB, application/java-archive
Details
4.68 KB, text/html
Details
206.71 KB, application/java-archive
Details
1.75 KB, patch
bas
: review+
Details | Diff | Splinter Review
19.10 KB, patch
bas
: review+
cjones
: superreview+
Details | Diff | Splinter Review
24.40 KB, patch
cjones
: review+
Details | Diff | Splinter Review
19.06 KB, patch
cjones
: review-
Details | Diff | Splinter Review
38.24 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
14.14 KB, patch
bas
: review+
Details | Diff | Splinter Review
6.66 KB, patch
Details | Diff | Splinter Review
1.34 KB, patch
Details | Diff | Splinter Review
23.13 KB, patch
bsmedberg
: review+
Details | Diff | Splinter Review
41.94 KB, patch
bsmedberg
: review+
cjones
: superreview+
Details | Diff | Splinter Review
8.49 KB, patch
karlt
: review+
Details | Diff | Splinter Review
16.13 KB, patch
Details | Diff | Splinter Review
20.61 KB, patch
cjones
: review+
Details | Diff | Splinter Review
26.68 KB, patch
cjones
: review+
Details | Diff | Splinter Review
2.66 KB, patch
Details | Diff | Splinter Review
1.69 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.60 KB, patch
karlt
: review+
Details | Diff | Splinter Review
3.99 KB, patch
karlt
: review+
Details | Diff | Splinter Review
8.91 KB, patch
karlt
: review+
Details | Diff | Splinter Review
2.62 KB, patch
roc
: review+
Details | Diff | Splinter Review
7.39 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
2.91 KB, patch
bas
: review+
Details | Diff | Splinter Review
26.60 KB, patch
Details | Diff | Splinter Review
274.84 KB, patch
Details | Diff | Splinter Review
2.21 KB, patch
karlt
: review+
Details | Diff | Splinter Review
7.91 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.04 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.69 KB, patch
roc
: review+
Details | Diff | Splinter Review
14.46 KB, patch
karlt
: review+
Details | Diff | Splinter Review
Here's how I think we could do it:

Create a new layer type ReadbackLayer. This layer is configured with a callback that receives the contents of the backbuffer under the layer. This callback can be called on the main thread at any time. nsObjectFrame creates a ReadbackLayer underneath the ImageLayer for an async-rendered windowless plugin. When the callback fires, it sends the buffer to the plugin process.

When the plugin process has a background buffer, the plugin is drawn over the background and the resulting opaque data is handed back as the plugin image. When the plugin process has no background buffer, we use alpha recovery instead.

It might simplify the implementation if we allow the callback to be called with no data, indicating "background unknown". When that happens the plugin process would fall back to alpha recovery.

A key problem is to efficiently detect when the plugin background has changed in a way that affects the plugin rendering, and avoid re-rendering the plugin if the background hasn't changed. For example we don't want to be repainting the plugin as we scroll.

I'd like to analyze some sites but it seems to me that in most of the cases we care about, the background of the plugin is a single opaque ThebesLayer or ColorLayer. Maybe we can implement ReadbackLayer to handle only those cases. Those cases are particularly easy to handle because with BasicLayers, D3D9, and GL we don't have to read back from the GPU to get a surface we can use for the plugin background --- ThebesLayers have the data in the format the plugin needs. Also, if we stick to those cases we can monitor layer invalidation and layer tree changes to detect when the background has changed.

With D3D10 we would need to use asynchronous readback to get data from D2D/D3D surfaces into system memory. Bas thinks we can do this without hurting performance.
Is this bug just for Windows? Are we going to implement something similar for Linux/Mac? Or on mac with CALayer do we consistently always get a correct alpha surface and don't have to worry about it?
I believe the Mac Core Animation and CoreGraphics drawing models can always give us an alpha surface. The other model, Quickdraw, doesn't work cross-process anyway as I understand it.

This approach is intended for Windows and X. It could also work for CoreGraphics if there was a benefit to doing so --- e.g. we wanted subpixel AA in plugins to work --- but since the Core Animation model is "the future" and doesn't support subpixel AA AFAIK, we probably shouldn't care.
(In reply to comment #0)
> Those cases are particularly easy to handle because with BasicLayers, D3D9, and
> GL we don't have to read back from the GPU to get a surface we can use for the
> plugin background --- ThebesLayers have the data in the format the plugin
> needs.

To clarify: the *changed* area of a ThebesLayer is available in system memory (or an X pixmap). And that's all we need here.
Assignee: nobody → roc
blocking2.0: --- → ?
Target Milestone: --- → mozilla2.0b10
Rob, are your current patches available in a public mq anywhere?
http://hg.mozilla.org/users/rocallahan_mozilla.com/main

Patches bug626602 and bug626602-basic. They build, but don't work yet (as far as getting data to nsPluginInstanceOwner::BeginUpdateBackground). But you can see what I'm doing.
Patch currently in bug626602 there creates ReadbackLayers for BasicLayers and gets to the point of delivering data to nsPluginInstanceOwner::BeginBackgroundUpdate.
Here's a preliminary analysis of the effectiveness of this approach. I looked at all the problematic sites in bug 611698, plus hulu.com since I know it uses a windowless Flash object. Any other suggestions for sites that use windowless Flash to add to this list appreciated.

http://www.foreignaffairs.com/issues/2010/89/6
http://megaupload.com/
http://www.homeicecream.com/
http://www.disneyinternational.com/disneyxd_eu/
http://www.punypng.com/
http://www.telusmobility.com/en/BC/plans/
http://www.support.philips.com/support/sms_page.jsp?userLanguage=en&userCountry=my
http://www.logitech.com/en-us/gaming/controllers/devices/288
http://tinypic.com/view.php?pic=8zfzar&s=5&hid=1&tag=flower+icon
http://www.skihood.com/
http://www.hulu.com/ (Front page only; can't stream shows from NZ)
No problem, background for all Flash instances.

http://superpokepets.com/spp
http://oscar-raul.superpokepets.com/
No problem after you actually get into the game.
During the "Loading" stage, the biggest Flash instance doesn't get a background, because it
overlaps another Flash instance.

http://www.inimacopiilor.ro/campanie/ajuta_promoveaza.php
Basically fails, because of the page structure; the layer underneath is
treated as transparent. If we stored opaque regions for each ThebesLayer, we'd probably be able
to rescue this.
Based on this data, I think it's worth going ahead with the current approach.
For the hulu player itself, with the patch applied, I get two readback layers in the layer tree, so I assume conditions were right.
  PRPackedBool mMayHaveReadbackChild;

in ContainerLayer isn't being initialized.
Thanks. Fixed in patch queue. Also, I've added the patch bug626602-d3d9, which adds support for ReadbackLayers to D3D9. We don't actually need to do VRAM readback with D3D9:
  // Because updates to D3D9 ThebesLayers are rendered with the CPU, we don't
  // have to do readback from D3D9 surfaces. Instead we make sure that any area
  // needed for readback is included in the drawRegion we ask layout to render.
  // Then the readback areas we need can be copied out of the temporary
  // destinationSurface in DrawRegion.
Bas, we're probably ready for you to take the D3D9 code and make a D3D10 version that actually does async readback.

Managing lifetimes of stuff is going to be a little tricky. I suggest you put your state in ReadbackLayerD3D10 ... it'll need to be an array of outstanding readback requests.
I've got all code in place for X, but nspluginwrapper is choking on X errors (argh!).  Shouldn't be too bad to sort out.  After that, there is a great deal of refactoring :( and the windows impl.
If you've got the IPC done to get the buffer over to PluginInstanceChild, post it. I'd be more than happy to finish the Windows implementation.
Blocks: 615025
Karl points out http://www.marcodilauro.com/portfolio/africa/#num=content-40&id=album-6, in which we're not getting background rendering.  Here's the layer tree:

1589537088[10aaa60]:     BasicContainerLayer (0x231b4a0) [clip=(x=0, y=120, w=1920, h=935)] [visible=< (x=0, y=120, w=1920, h=935); >] [opaqueContent]
1589537088[10aaa60]:       BasicThebesLayer (0x230d840) [clip=(x=0, y=0, w=0, h=0)] [transform=[ 1 0; 0 1; 0 120; ]]
1589537088[10aaa60]:       BasicColorLayer (0x230f550) [clip=(x=0, y=120, w=1920, h=917)] [transform=[ 1 0; 0 1; 0 120; ]] [visible=< (x=0, y=0, w=1920, h=917); >] [opaqueContent] [color=rgba(0, 0, 0, 1)]
1589537088[10aaa60]:       BasicReadbackLayer (0x230df60) [clip=(x=0, y=120, w=1920, h=935)] [transform=[ 1 0; 0 1; 0 120; ]] [visible=< (x=0, y=0, w=1920, h=917); >] [size=(w=1920, h=935)] [nobackground]
1589537088[10aaa60]:       BasicImageLayer (0x230e550) [clip=(x=0, y=120, w=1920, h=935)] [transform=[ 1 0; 0 1; 0 120; ]] [visible=< (x=0, y=0, w=1920, h=917); >]
1589537088[10aaa60]:       BasicThebesLayer (0x230ed50) [visible=< (x=0, y=155, w=1920, h=900); >] [componentAlpha]

Rob, is this a bug in your patches?  We should be getting a color layer background, if I read things correctly.  Does the top thebes layer mess things up?  It appears to be translucent locally.
Yeah, I think it's just a bug with the ColorLayer code. On it.
Oh, no it isn't. The problem here is the black-background footer DIV makes the ColorLayer underneath take a visible region that's less than the full height of the page. Ergo, the visible region of the ColorLayer doesn't contain the background of the plugin.

A slightly crazy but probably OK thing to do would be for nsDisplayPluginReadback to *add* its area to the visible region passed to things below it.
I have pushed an updated patch queue that does that. Seems to work on Karl's page. There's a navigation menu that pops up *under* the Flash object sometimes which breaks things, but I think it's a bug in the page really because you can never see it :-). Most of the time it's not there.
Oops, no, that approach is broken. Need to rethink.
Yeah, apparently the menu is ruining things most of the time on my machine.  I don't get background drawing except after clicking on one of the thumbnails, while the full-size image draws on the left.

Updated

7 years ago
blocking2.0: ? → betaN+
(The reason that approach is broken is because we can extend the visible region OK, but the desired ThebesLayer content might not be painted in that region because it'll be clipped out.)
http://hg.mozilla.org/users/cjones_mozilla.com/mcmq/file/2f6b1fad2cc7/626602-goop gets the background pixels to the plugin subprocess, on X.  Probably doesn't compile on windows.  I'm going to fork off the X stuff because it's a big mess, and shouldn't block the windows work.  I'll rebase on whatever changes are made for windows.

There's an invalidation bug but I think it's X only.  There's also a bug in the readback logic, because it doesn't take background changes into account.
Rob, I can hack on the windows stuff in the plugin subprocess if you're working on the readback layer logic.
Yes, I'm looking at the readback logic right now. Keep going! :-)
(Just to be clear, 626602-goop doesn't work on X platforms either, because I split all the gross hacks into a separate patch.)
OK, I've got a fix for the visibility stuff, but something very weird is happening that I need to debug...
http://www.marcodilauro.com/portfolio/africa/ is actually broken for me on trunk with BasicLayers ...
... with BasicLayers and D2D. OK. I'm going to ignore that for now.
OK, the latest version of 626602-goop works on windows.  Not quite review-ready but fine for testing.  I'm not going to touch that patch for a while.
OK, I've pushed my patch queue again. The 626602* patches give us readback on the marcodilauro.com page, except when the menu is fading out (which seems to take a while, for some reason).

I'm going home now but later I'll try to integrate your patch and do some testing.
Rebasing on 3b4f201929d4 killed my patch.  Cleaning it up.
/home/cjones/mozilla/mozilla-central/layout/generic/nsObjectFrame.cpp:1375: error: no matching function for call to ‘nsDisplayPluginReadback::ComputeVisibility(nsDisplayListBuilder*&, nsRegion*&)’
/home/cjones/mozilla/mozilla-central/layout/generic/../base/nsDisplayList.h:713: note: candidates are: virtual PRBool nsDisplayItem::ComputeVisibility(nsDisplayListBuilder*, nsRegion*, PRBool&)
Oh, I didn't have all patches applied.  Word of warning, though :).
Wow!  With the latest visibility patch, it's night and day on http://www.marcodilauro.com/portfolio/africa/ on X.  There's an invalidation bug or 10, but we're about as smooth as chrome 9 on the thumbnail animations, and noticeably smoother on filling the large images.  There's room for optimization left too.

Patch seems to work fine on windows, don't notice a perf difference on that page though.
I pushed changes to 626602-goop.  Should work pretty well on windows and X.  There are occasional transient rendering artifacts on both systems.  Patch pretty close to ready, mostly an issue of how much to refactor the existing code.
A few notes:

Alpha recovery is currently broken on Windows. It needs to be fixed and enabled. The main problems are that PaintRectToPlatformSurface doesn't know how to paint to arbitrary aSurface on Windows, and PaintRectWithAlphaExtraction tries to paint to an image surface, which currently doesn't work. It would be best for PaintRectWithAlphaExtraction to create a gfxWindowsSurface on Windows so we don't have to do an unnecessary copy.

I discovered that currently our SSE2 alpha recovery code is not being invoked because nothing tries to ensure that the white and black buffers have the same 16-byte alignment.

I grabbed a Flash benchmark "GUIMark3", tweaked it a little bit, and ran it in a variety of configurations. Here are the FPS results:

== Windowed mode ==

Firefox 3.6.13: 55
Firefox trunk opt build, Basic: 55
Firefox trunk opt build, D3D9: 56
Firefox trunk opt build, D3D10: 56
Chrome 10.0.648.6: 45
Opera 11: 56

The Firefox results sometimes drop to 44 if you switch tabs and then switch back again. It's weird.

== Windowless opaque mode ==

Firefox trunk opt build, Basic: 50
Firefox trunk opt build, D3D9: 42
Firefox trunk opt build, D3D10: 52
Chrome 10.0.648.6: 59
Opera 11: 60 (jerky)

== Windowless transparent mode ==

Firefox 3.6.13: 60
Firefox trunk opt build, Basic: 36
Firefox trunk opt build, D3D9: 32
Firefox trunk opt build, D3D10: 38
Chrome 10.0.648.6: 55
Opera 11: 60 (jerky)

Notice how D3D9 lags behind in both windowless modes. I wonder why that is.

I wonder why transparent mode is currently slower than opaque mode. It should be about the same right now.

If I enable the current (broken) alpha extraction code:
Firefox trunk opt build, Basic, alpha extraction: 18
Not good. We definitely need this bug!
Created attachment 507830 [details]
benchmark
jar:https://bug626602.bugzilla.mozilla.org/attachment.cgi?id=507830!/GUIMark_Flex3.html

This was derived from http://www.craftymind.com/factory/guimark/GUIMark_Flex3.html

The benchmark is in transparent mode. To try the other modes, change the wmode attribute on the <embed> in the HTML.
The source for the SWF is here:
http://www.craftymind.com/factory/guimark/source/GUIMark_Flex3_source.zip

Updated

7 years ago
Blocks: 629679

Comment 40

7 years ago
(In reply to comment #36)
> A few notes:
> 
> Alpha recovery is currently broken on Windows. It needs to be fixed and
> enabled. The main problems are that PaintRectToPlatformSurface doesn't know how
> to paint to arbitrary aSurface on Windows, and PaintRectWithAlphaExtraction
> tries to paint to an image surface, which currently doesn't work. It would be
> best for PaintRectWithAlphaExtraction to create a gfxWindowsSurface on Windows
> so we don't have to do an unnecessary copy.

There's a patch in bug 611698 that fixes alpha recovery in plugininstancechild, if you need it.

Updated

7 years ago
Blocks: 626206
applying roc-patches/bug626602-d3d9
gfx/layers/d3d9/ContainerLayerD3D9.cpp : No such file or directory

which is odd, because the patch seems to apply correctly.

ReadbackLayerD3D9.h has \r's.
Created attachment 507914 [details] [diff] [review]
WIP rollup patch

Rollup of roc's patches and my patch, in case anyone wants to noodle.
Blocks: 626202
This benchmark tries to resize the window, which chrome and opera prevent, so I ran it full-screen in all.

Here's wmode=transparent perf on my Ubuntu 10.10 x86-64 desktop, with flash 10.3 d162: http://pastebin.mozilla.org/996495 .

I *really* don't trust these fps numbers (see below and raw data links in pastebin), but here they are

3.6 (distro): 37.5
Nightly (2011/01/28): 12.2
Local + patches: 29.27
Chrome 10: 24.8
Opera 11: 57.48

The important bits are, first the nightly is clearly the worst of the bunch.  I found it pretty hard to distinguish chrome and opera, but chrome might have been slightly smoother.  Regardless, 3.6 and my local build are without a doubt smoother than the others.  I couldn't visually distinguish 3.6 and my local build.

I don't know how these numbers are being computed, but they do not at all reflect what my eyes see!
(In reply to comment #43)
> This benchmark tries to resize the window, which chrome and opera prevent, so I
> ran it full-screen in all.

Yeah, I manually resized the windows.

> 3.6 (distro): 37.5
> Nightly (2011/01/28): 12.2
> Local + patches: 29.27
> Chrome 10: 24.8
> Opera 11: 57.48
> 
> The important bits are, first the nightly is clearly the worst of the bunch.  I
> found it pretty hard to distinguish chrome and opera, but chrome might have
> been slightly smoother.  Regardless, 3.6 and my local build are without a doubt
> smoother than the others.  I couldn't visually distinguish 3.6 and my local
> build.
> 
> I don't know how these numbers are being computed, but they do not at all
> reflect what my eyes see!

You can look at the SWF source. It registers an event listener for an "ENTER_FRAME" event, and basically uses the count of those events as the frame count. It updates the Flash object state in that event listener too, so I think you definitely can't see more frame updates than are reported by this number: it's an upper bound on the true frame rate. So I think that's useful.

However, there's no guarantee that all the frames generated by the plugin will actually be rendered to the screen, so this FPS number can be higher than the true FPS. I don't know of any way to measure the true FPS across browsers, but we should add a mozPaintCount-based monitor for Gecko. We'll need the patch in bug 625248 though. (mozPaintCount doesn't give the true FPS either, due to window manager buffering, but it's the best we can do right now.)
Depends on: 625248
(I thought opaque and window would be boring, but they weren't!)

Full data for all tests: http://pastebin.mozilla.org/996595

tl;dr

wmode=opaque:
  3.6 (distro): 43.88
  Nightly: 34.27
  Local + patches: 49.22 (??)
  Chrome 10: 26.26
  Opera 11: 59.91

To my eyes, the firefox builds were smoother than the others.  Again the perf numbers are suspect.  I don't know why my local build would be different than a nightly, will investigate.

wmode=window
  3.6 (distro): 53.9 (severe tearing)
  Nightly: 53.88 (severe tearing)
  Local + patches: 51.94 (severe tearing)
  Chrome 10: 54.22 (severe tearing)
  Opera 11: 32.11 (no tearing!)

That data mostly makes sense, except for opera.  Not sure what it's doing there.  The tearing was godawful in the firefoxen+chrome.
(In reply to comment #44)
> However, there's no guarantee that all the frames generated by the plugin will
> actually be rendered to the screen, so this FPS number can be higher than the
> true FPS.

Yeah, the firefoxen are probably being friendlier to X.

> I don't know of any way to measure the true FPS across browsers

I do! :).  But it's not ready yet.
(In reply to comment #36)
> I wonder why transparent mode is currently slower than opaque mode. It should
> be about the same right now.

Yeah, good question.  I wonder if we're hitting some problematic fallback path in the PluginBackgroundSink.  The gfxContext target (for the background) is an RGB24 image surface on windows, if that rings any bells.
(In reply to comment #45)
> wmode=opaque:
>   Nightly: 34.27
>   Local + patches: 49.22 (??)

Hmm, I can't reproduce this difference anymore.  The nightly gets about the same fps generally as my local build does, ~50.
(In reply to comment #36)
> Alpha recovery is currently broken on Windows.
>

I filed a separate bug on this.
 
> I discovered that currently our SSE2 alpha recovery code is not being invoked
> because nothing tries to ensure that the white and black buffers have the same
> 16-byte alignment.

This might be a little trickier if we want skip creating an unnecessary temporary surface.  Maybe it's not worth bothering with for 4.0 though, since it might be a hard trade-off to measure.
(In reply to comment #49)
> Maybe it's not worth bothering with for 4.0 though, since
> it might be a hard trade-off to measure.

(... implicit in that was supposed to be, "and it's the fallback anyway".)

Updated

7 years ago
Duplicate of this bug: 629679

Updated

7 years ago
Whiteboard: [hardblocker]
(In reply to comment #36)
> I wonder why transparent mode is currently slower than opaque mode. It should
> be about the same right now.

I see a similar effect in my windows VM (yes yes, VM, sorry).  I get ~28fps with wmode=transparent and ~40fps with wmode=opaque.

The slowdown seems to be in flash, for whatever reason.  I timed the two relevant PaintRectToSurface()s in ShowPluginFrame() and grabbed a random sample of the timings.  (I have the raw data, if anyone wants it.)

wmode=opaque:
Count 40 total 650 average 16.25 [Ed: milliseconds]

wmode=transparent:
Count 40 total 1233 average 30.825 [Ed: milliseconds]

This was in a Windows VM though so I suspect the timing resolution.  The individual frame timings weren't either uniformly 0, 15 or 30, but distributed around the means, so I'm not sure the timer precision is an issue.  Dunno though.  (We can haz high-precision TimeStamp on Windows?)

I also applied a patch to only ask flash to repaint <x=0,y=0, w=100,h=100> in the plugin frame (enough to cover the fps counter), but have our code pretend like the entire frame was invalidated on each paint, i.e. force us to do as much work as possible.  This gave 60fps, which is our cap (I believe that still applies to empty transactions ...).

Note that in the numbers above, there's a similar effect on desktop linux: ~30fps for wmode=transparent and ~50fps for wmode=opaque.

The numbers seem to agree, so I'm comfortable blaming code running in flash.

Might there be something in our Windows hooks that could be slowing down flash in wmode=transparent?
It's entirely possible --- indeed, probable --- that Flash uses a different compositing path for wmode=opaque, one where it doesn't first BitBlt from our HDC to an internal buffer.
Sorry about the \rs :-(. I've pushed updated patches.

I have also added a patch to my queue that backs out the SPI hooks from bug 611689, now that they should no longer be needed. Indeed, with this bug fixed subpixel-AA in Flash should work again whenever we can find the background.
15ms is the default user mode thread granulity on some configurations in Windows (supposed to be for multi-core configurations, but I'm on a quad core and get 10ms). You could try running a little program in the background that calls timeBeginPeriod(1) to increase the granulity to 1ms time slices, which should be system-wide. This call should be coupled with a timeEndPeriod(1) call when you're done, or the timing granulity won't go back to what it was until a reboot.

Of course, it might be that whatever timers Firefox uses won't benefit from this - it's just something that should be quick and simple to try.
I filed bug 629866 to analyze the wmode="opaque" case and try to optimize that better. That should help wmode="transparent" too once this bug is done.
With your updated patches on wmode=transparent GUIMark3, I'm seeing backgrounds larger than the plugin frame.  The plugin ended up with an 1100x750 surface with an 1100x1100 background.  Not sure if that was intended.
I've got patches split up and pretty much ready for review.
(In reply to comment #57)
> With your updated patches on wmode=transparent GUIMark3, I'm seeing backgrounds
> larger than the plugin frame.  The plugin ended up with an 1100x750 surface
> with an 1100x1100 background.  Not sure if that was intended.

Hmm, that must be a bug.
Created attachment 508396 [details] [diff] [review]
Implement asynchronous D3D10 readback

This patch implements D3D10 readback, it seems to work fine but I've got some questions about general functionality which I'll discuss on IRC.

This patch implements the following approach:

- A 'ReadbackManager' is created on a per-D3D10 device basis (we have one actively used device per session).
- This ReadbackManager maintains a single thread that essentially synchronizes with the pipeline of the D3D10 device.

When a layer is changed, a ReadbackTask is posted to the ReadbackManager, the ReadbackManager will execute readback tasks in the order they're posted, maintaining synchronisation with the device pipeline. It will do this by attempting a blocking map on the texture, once the map succeeds we will know the synchronisation point for readback has been reached, and it will immediately unmap, and post an event to the main thread which will map the texture again, and update the sink on the ReadbackLayer.
Attachment #508396 - Flags: feedback?(roc)
Created attachment 508398 [details] [diff] [review]
Implement asynchronous D3D10 readback v2

Add ReadbackManager files.
Attachment #508396 - Attachment is obsolete: true
Attachment #508398 - Flags: feedback?(roc)
Attachment #508396 - Flags: feedback?(roc)
Attachment #508398 - Flags: feedback?(jones.chris.g)
Created attachment 508461 [details] [diff] [review]
WIP rollup, v2
Attachment #507914 - Attachment is obsolete: true
Created attachment 508487 [details] [diff] [review]
WIP rollup, v3

Oops, left out a patch in the other one.  Thanks Bas.
Attachment #508461 - Attachment is obsolete: true
Comment on attachment 508398 [details] [diff] [review]
Implement asynchronous D3D10 readback v2

ReadbackManagerD3D10 could be implemented using nsThreadPool with less
code, but this needs to be pretty close the metal so I don't mind the
duplication.

You're using several bare pointers here where an nsRefPtr or nsAutoPtr
would work just as well, and lead to simpler resource management.

ReadbackProcessor::Update doesn't hold a ref to its underlying layer,
and nothing in the ReadbackTask does either.  This is unsafe.  We need
some way to cancel these.

feedback- for the last point above.
Attachment #508398 - Flags: feedback?(jones.chris.g) → feedback-
Created attachment 508542 [details]
benchmark, with separate files for the different wmode's

Didn't change anything in roc's code.
The last patch in my mq enables alpha recovery on windows when we don't have a background available, so we need the non-broken non-sg:critical impl.
Depends on: 629799
Created attachment 508570 [details]
test page with wmode=transparent

This is based on the communitymx wmode=transparent example.  I didn't check copyrights, please don't use this page for anything.
Created attachment 508663 [details] [diff] [review]
Implement asynchronous D3D10 readback v3

This patch appears to work well. Without this patch we seem to be about 0.8 fps faster on the GUIMark transparent test but I'm not sure that difference is significant or some fluke.
Attachment #508398 - Attachment is obsolete: true
Attachment #508663 - Flags: review?(roc)
Attachment #508398 - Flags: feedback?(roc)
Created attachment 508665 [details] [diff] [review]
WIP rollup, v4

Adds logic to fall back on alpha recovery on windows.
Attachment #508487 - Attachment is obsolete: true
+      ctx->SetColor(gfxRGBA(0, 1, 0));

I hope you didn't mean to include this :-)

Can you say something about the threading rules in comments? E.g. in ReadbackTask:
+struct ReadbackTask {
+  // The texture that we copied the contents of the thebeslayer to.
+  nsRefPtr<ID3D10Texture2D> mReadbackTexture;
+  // This exists purely to keep the ReadbackLayer alive for the lifetime of
+  // mUpdate.
+  nsRefPtr<ReadbackLayer> mLayer;
+  nsAutoPtr<ReadbackProcessor::Update> mUpdate;
+  // The origin in ThebesLayer coordinates of mReadbackTexture.
+  gfxPoint mOrigin;

Which fields are accessed on which threads? Looks like mReadbackTexture is read on the readback thread (but only while the ReadbackManager owns the ReadbackTask), and all other fields are entirely main-thread only?

And what are the invariants governed by mTaskSemaphore?

Instead of making mUpdate a pointer, you can probably just make it a direct member.

You will need to do something to deal with mUpdate's mLayer possibly dying. Probably just addref it while you're holding it --- but only addref/release on the main thread please!

+  size = sizeof(ReadbackManagerD3D10*);
+  if (FAILED(mDevice->GetPrivateData(sReadbackManager, &size, mReadbackManager.StartAssignment()))) {
+    mReadbackManager = new ReadbackManagerD3D10();
+    mDevice->SetPrivateDataInterface(sReadbackManager, mReadbackManager);
+  }

Can we create the ReadbackManager lazily? That would avoid any startup impact --- and overhead, if the user happens to not encounter a transparent windowless plugin.

+    // We want to block here until the texture contents is available, the

"are available"

Better comment somewhere that ownership of the ReadbackTask is passed from mPendingReadbackTasks to the ReadbackResultWriter.

+      nsRefPtr<ID3D10Texture2D> readbackTexture;
+      device()->CreateTexture2D(&desc, NULL, getter_AddRefs(readbackTexture));
+      device()->CopyResource(readbackTexture, mTexture);

How much of an issue is it that you copy the entire texture? Seems like it would save a bit of time and VRAM if we copied only the rectangle(s) needed by the plugin.
Created attachment 508712 [details]
GUIMark.zip

Updated benchmark to include a mozPaintCount counter, and allow modes to be specified via a query parameter, e.g. ?opaque. Default is transparent.
Attachment #507830 - Attachment is obsolete: true
Created attachment 508714 [details]
actual benchmark

Right file
Attachment #508712 - Attachment is obsolete: true
(In reply to comment #57)
> With your updated patches on wmode=transparent GUIMark3, I'm seeing backgrounds
> larger than the plugin frame.  The plugin ended up with an 1100x750 surface
> with an 1100x1100 background.  Not sure if that was intended.

I'm not seeing this. I see 1100x750 in nsPluginInstanceOwner::BeginUpdateBackground. Maybe something got flipped after that?
Created attachment 508722 [details] [diff] [review]
Part 0: Fix header guards

trivial
Attachment #508722 - Flags: review?(bas.schouten)
Created attachment 508723 [details] [diff] [review]
Part 1: ReadbackLayer API
Attachment #508723 - Flags: superreview?(jones.chris.g)
Attachment #508723 - Flags: review?(bas.schouten)
Created attachment 508724 [details] [diff] [review]
Part 2: BasicLayers implementation
Attachment #508724 - Flags: review?(jones.chris.g)
Created attachment 508726 [details] [diff] [review]
Part 3: Hook up to nsObjectFrame

Not sure who should review this really
Attachment #508726 - Flags: review?(jones.chris.g)
Created attachment 508727 [details] [diff] [review]
Part 4: Make display items behind the plugin as visible as possible

Perhaps you won't like this, Timothy :-)
Attachment #508727 - Flags: review?(tnikkel)
Created attachment 508728 [details] [diff] [review]
Part 5: D3D9 support

Note that there's no actual readback here. We just ensure that the ThebesLayer draws enough.
Attachment #508728 - Flags: review?(bas.schouten)
Created attachment 508729 [details] [diff] [review]
Part X: backout SPI hook

This is pure backout, and should land after the other patches in this bug have landed successfully.
Attachment #508722 - Flags: review?(bas.schouten) → review+
Comment on attachment 508723 [details] [diff] [review]
Part 1: ReadbackLayer API

Well, the whole API doesn't fill me with warm fuzzy feelings, but it does the job, and we need it :-).

>From: Robert O'Callahan <robert@ocallahan.org>
>Bug 626602. Part 1: Create ReadbackLayer API to enable collection of background pixels in a layer tree. r=bas,sr=cjones
>diff --git a/gfx/layers/opengl/ContainerLayerOGL.cpp b/gfx/layers/opengl/ContainerLayerOGL.cpp
>--- a/gfx/layers/opengl/ContainerLayerOGL.cpp
>+++ b/gfx/layers/opengl/ContainerLayerOGL.cpp
>@@ -40,43 +40,45 @@
> 
> namespace mozilla {
> namespace layers {
> 
> template<class Container>
> static void
> ContainerInsertAfter(Container* aContainer, Layer* aChild, Layer* aAfter)
> {
>+  NS_ADDREF(aChild);
>+

I don't think we can do this, we'll leak a reference if aAfter is not found I think, which is not great behavior in my opinion. This goes for all the container layers.
Attachment #508723 - Flags: review?(bas.schouten) → review+
Attachment #508728 - Flags: review?(bas.schouten) → review+
Created attachment 508826 [details] [diff] [review]
part 7: Copy the impl of < and <= from nsIntSize to gfxIntSize. Sigh.
Attachment #508826 - Flags: superreview?(roc)
Created attachment 508827 [details] [diff] [review]
part 8: Dig a tunnel from nsObjectFrame to PluginInstanceParent for background copying

Just the goop to get plugin backgrounds into dom/plugins.
Attachment #508827 - Flags: superreview?(roc)
Attachment #508827 - Flags: review?(joshmoz)
Created attachment 508828 [details] [diff] [review]
Bug 626602, part 9: When possible, copy from a background to an opaque surface and have transparent plugins draw directly on the copied background, instead of doing alpha recovery

This patch was mostly fairly straightforward.  The one subtlety that arose was in destroying plugin backgrounds.  I added a long comment to PPluginBackgroundDestroyer.ipdl that should hopefully clarify things.
Attachment #508828 - Flags: superreview?(roc)
Attachment #508828 - Flags: review?(karlt)
Attachment #508828 - Flags: review?(jmathies)
Attachment #508827 - Flags: review?(joshmoz) → review?(benjamin)
Attachment #508828 - Flags: review?(jmathies) → review?(benjamin)
(In reply to comment #81)
> I don't think we can do this, we'll leak a reference if aAfter is not found I
> think, which is not great behavior in my opinion. This goes for all the
> container layers.

Good point. Fixed in patch queue.
Ben, comment 0 is still an accurate description of the plan.  Down in dom/plugins, things basically happen as follows

 - layout calls BeginBackgroundUpdate() when it gets ready to hand a plugin instance a background
 - on EndBackgroundUpdate(), the background is sent to the plugin process
 - plugin instances hand out ARGB front buffers to layout if they're transparent and don't have a background (X is messy, but let's pretend that's true).  They hand out RGB24 surfaces otherwise.
 - if a plugin has an ARGB back buffer, it will do alpha recovery
 - layout can "take away" an instance's background at any time.  That happens through SetBackgroundUnknown().  Destroying the background was the slightly tricky thing mentioned by comment 84.
 - (this only happens on windows and X platforms)
Attachment #508826 - Flags: superreview?(roc) → superreview+
Attachment #508827 - Flags: superreview?(roc) → superreview+
(In reply to comment #75)
> Created attachment 508723 [details] [diff] [review]
> Part 1: ReadbackLayer API

Wasn't this patch supposed to add a few files?
I'll fold the SPI backout into my last patch so that we make a clean switch.
Comment on attachment 508828 [details] [diff] [review]
Bug 626602, part 9: When possible, copy from a background to an opaque surface and have transparent plugins draw directly on the copied background, instead of doing alpha recovery

+    if (mIsTransparent && !CanPaintOnBackground()) {

How about just checking the format of renderSurface here?

+    bool haveTransparentPixels = mIsTransparent && !CanPaintOnBackground();

Same here.

We don't need to store mBackgroundUpdateCtx in PluginInstanceParent.
Attachment #508828 - Flags: superreview?(roc) → superreview+
Comment on attachment 508723 [details] [diff] [review]
Part 1: ReadbackLayer API

diff --git a/gfx/layers/Layers.h b/gfx/layers/Layers.h
--- a/gfx/layers/Layers.h
+++ b/gfx/layers/Layers.h
 public:
+  // Keep these in alphabetical order

Thanks, I share this anal gene.

@@ -947,16 +960,17 @@ protected:
   // re-painted regions at all.  It only affects how scalable thebes
   // content is rasterized to device pixels.
   //
   // Setting the resolution isn't part of the public ThebesLayer API
   // because it's backend-specific, and it doesn't necessarily make
   // sense for all backends to fully support it.
   float mXResolution;
   float mYResolution;
+  bool mUsedForReadback;

I wouldn't mind a note here clarifying that this means "also
participates in readback", not "only used for readback".

diff --git a/gfx/layers/ReadbackLayer.h b/gfx/layers/ReadbackLayer.h
new file mode 100644
--- /dev/null
+++ b/gfx/layers/ReadbackLayer.h
+class THEBES_API ReadbackSink {
+public:
+  ReadbackSink() {}
+  virtual ~ReadbackSink() {}
+
+  /**
+   * Sends an update to indicate that the background is currently unknown.
+   */
+  virtual void SetUnknown(PRUint64 aSequenceNumber) = 0;
+  /**
+   * Called by the layer system to indicate that the contents of part of
+   * the readback area are changing.
+   * @param aRect is the rectangle of content that is being updated,
+   * in the coordinate system of the ReadbackLayer.
+   * @param aSequenceNumber updates issued out of order should be ignored.
+   * Only use updates whose sequence counter is greater than all other updates
+   * seen so far. Return null when a non-fresh sequence value is given.
+   * @return a context into which the update should be drawn. This should be
+   * set up to clip to aRect. Zero should never be passed as a sequence number.
+   * If this returns null, EndUpdate should NOT be called.

Also, "If this returns nonnull, EndUpdate MUST be called."  The
dom/plugins code relies on that.

+/**
+ * A ReadbackLayer never renders anything. It enables clients to extract
+ * the rendered contents of the layer tree below the ReadbackLayer.
+ * The rendered contents are delivered asynchronously via calls to a
+ * ReadbackSink object supplied by the client.
+ *
+ * This is a "best effort" API; it is possible for the layer system to tell
+ * the ReadbackSink that the contents of the readback area are unknown.

Would you add a comment noting that this code exists to work around
broken plugins?  I'm not sure what the conditions are for dropping
this code, but if you know them, please add them here too.  I'm
concerned about enterprising but misguided folks attempting to use
this for drawWindow or -moz-element.

+ */
+class THEBES_API ReadbackLayer : public Layer {
+  /**
+   * CONSTRUCTION PHASE ONLY
+   * Set the callback object to which readback updates will be delivered.
+   * This also resets the "needed rectangle" so that on the next layer tree
+   * transaction we will try to deliver the full contents of the readback
+   * area to the sink.
+   * This layer takes ownership of the sink. It will be deleted when the
+   * layer is destroyed or when a new sink is set.

This makes me a bit nervous but works in the current impl.  I wish
there were a way to assert |aSink|'s concrete class isn't refcounted
but I can't think of one.

+  // When the alpha value of mBackgroundColor is 255, this is the color

"mBackgroundColor is opaque"?  It would actually be 1.0, but that's
not an important detail here.

diff --git a/gfx/layers/ReadbackProcessor.h b/gfx/layers/ReadbackProcessor.h
new file mode 100644
--- /dev/null
+++ b/gfx/layers/ReadbackProcessor.h
@@ -0,0 +1,105 @@
+#ifndef GFX_READBACKPROCESSOR_H
+#define GFX_READBACKPROCESSOR_H
+
+#include "ReadbackLayer.h"
+#include "ThebesLayerBuffer.h"
+
+namespace mozilla {
+namespace layers {
+
+class ReadbackProcessor {
+public:
+  /**
+   * Called by the container before processing any child layers. Call this
+   * if any child layer might have changed in any way (other than content-only
+   * changes to layers other than ColorLayers and ThebesLayers).
+   *
+   * This method recomputes the relationship between ReadbackLayers and
+   * sibling layers, and dispatches changes to ReadbackLayers. Except that
+   * if a ThebesLayer needs its contents sent to some ReadbackLayer, we'll
+   * just record that internally and later the ThebesLayer should call
+   * GetThebesLayerUpdates when it paints, to find out which rectangle needs
+   * to be sent, and the ReadbackLayer it needs to be sent to.
+   */
+  void BuildUpdates(ContainerLayer* aContainer);
+
+  struct Update {
+    /**
+     * The layer a ThebesLayer should send its contents to.
+     */
+    ReadbackLayer* mLayer;
+    /**
+     * The rectangle of content that it should send, in the ThebesLayer's
+     * coordinate system. This rectangle is guaranteed to be in the ThebesLayer's
+     * visible region. Translate it to mLayer's coordinate system
+     * by adding mLayer->GetBackgroundLayerOffset().
+     */
+    nsIntRect      mUpdateRect;
+    /**
+     * The sequence counter value to use when calling DoUpdate
+     */
+    PRUint64       mSequenceCounter;

As things stand, updates are always delivered to the sink in sequence
even with D3D10 so we'll never reject updates, but there's no reason
to make that assumption so this is fine.

Too bad we couldn't spend this time and code on, say, implementing fast
-moz-element, but gotta do what ya gotta do.
Attachment #508723 - Flags: superreview?(jones.chris.g) → superreview+
Attachment #508724 - Flags: review?(jones.chris.g) → review+
Comment on attachment 508726 [details] [diff] [review]
Part 3: Hook up to nsObjectFrame

>diff --git a/layout/generic/nsObjectFrame.cpp b/layout/generic/nsObjectFrame.cpp
>--- a/layout/generic/nsObjectFrame.cpp
>+++ b/layout/generic/nsObjectFrame.cpp
> // special class for handeling DOM context menu events because for
> // some reason it starves other mouse events if implemented on the
> // same class
> class nsPluginDOMContextMenuListener : public nsIDOMContextMenuListener
> {
> public:
>@@ -459,26 +462,32 @@ public:
>                                      nsIDOMClientRect* position,
>                                      nsIDOMClientRect* clip);
> #endif
> 
>   void NotifyPaintWaiter(nsDisplayListBuilder* aBuilder);
>   // Return true if we set image with valid surface
>   PRBool SetCurrentImage(ImageContainer* aContainer);
> 
>+  // Methods to update the background image we send to async plugins
>+  void SetBackgroundUnknown() {}
>+  already_AddRefed<gfxContext> BeginUpdateBackground(const nsIntRect& aRect) { return nsnull; }
>+  void EndUpdateBackground(gfxContext* aContext, const nsIntRect& aRect) {}

For reader sanity, these should mention their eventual destination in
PluginInstanceParent, but I'll change that in my patch.

>+class PluginBackgroundSink : public ReadbackSink {
>+public:
>+  PluginBackgroundSink(nsObjectFrame* aFrame)
>+    : mLastSequenceNumber(0), mFrame(aFrame) {}
>+  ~PluginBackgroundSink()
>+  {
>+    if (mFrame) {
>+      mFrame->mBackgroundSink = nsnull;
>+    }
>+  }
>+
>+  virtual void SetUnknown(PRUint64 aSequenceNumber)
>+  {
>+    if (aSequenceNumber <= mLastSequenceNumber || !mFrame ||
>+        !mFrame->mInstanceOwner)

I'd share this logic, up to you though.

>+      return;
>+    mLastSequenceNumber = aSequenceNumber;
>+    mFrame->mInstanceOwner->SetBackgroundUnknown();
>+  }
>+
>+  virtual already_AddRefed<gfxContext>
>+      BeginUpdate(const nsIntRect& aRect, PRUint64 aSequenceNumber)
>+  {
>+    if (aSequenceNumber <= mLastSequenceNumber || !mFrame ||
>+        !mFrame->mInstanceOwner)
>+      return nsnull;
>+    mLastSequenceNumber = aSequenceNumber;
>+    return mFrame->mInstanceOwner->BeginUpdateBackground(aRect);

One problem with this impl I came across in PluginInstanceParent is
that if it's unable to allocate a background for the first
BeginUpdate, say from low mem, then there's no guarantee that the next
BeginUpdate will be for the entire readback rect.  The code that
exists deals with this though (i.e. doesn't crash) and it's enough of
a corner case I don't care yet.

>+PRBool
>+nsDisplayPluginReadback::ComputeVisibility(nsDisplayListBuilder* aBuilder,
>+                                           nsRegion* aVisibleRegion)
>+{
>+  if (!nsDisplayItem::ComputeVisibility(aBuilder, aVisibleRegion))
>+    return PR_FALSE;
>+  // *Add* our bounds to the visible region so that stuff underneath us is
>+  // likely to be made visible, so we can use it for a background! This is
>+  // a bit crazy but should be OK.

The craziness is that you're doing the opposite of what we would
normally do to optimize when there's occlusion?

>-mozilla::LayerState
>-nsObjectFrame::GetLayerState(nsDisplayListBuilder* aBuilder,
>-                             LayerManager* aManager)
>+LayerState
>+nsObjectFrame::GetLayerState(nsDisplayListBuilder* aBuilder)
> {
>   if (!mInstanceOwner || !mInstanceOwner->UseLayers())
>-    return mozilla::LAYER_NONE;
>-
>-  return mozilla::LAYER_ACTIVE;
>+    return LAYER_NONE;
>+
>+  return LAYER_ACTIVE;

Or

return (mInstanceOwner && mInstanceOwner->UseLayers()) ? LAYER_ACTIVE : LAYER_NONE;

>+    NS_ASSERTION(aItem->GetType() == nsDisplayItem::TYPE_PLUGIN_READBACK,
>+                 "Unknown item type");
>+    if (!layer) {
>+      layer = aManager->CreateReadbackLayer();
>+      if (!layer)
>+        return nsnull;
>+    }
>+    NS_ASSERTION(layer->GetType() == Layer::TYPE_READBACK, "Bad layer type");
>+
>+    ReadbackLayer* readback = static_cast<ReadbackLayer*>(layer.get());
>+    if (readback->GetSize() != nsIntSize(size.width, size.height)) {
>+      // This will destroy any old background sink and notify us that the
>+      // background is now unknown
>+      readback->SetSink(nsnull);
>+

assert !mBackgroundSink after this.

>diff --git a/layout/generic/nsObjectFrame.h b/layout/generic/nsObjectFrame.h
>--- a/layout/generic/nsObjectFrame.h
>+++ b/layout/generic/nsObjectFrame.h
>   private:
>     nsString mEventType;
>   };
>   
>   nsRefPtr<nsPluginInstanceOwner> mInstanceOwner;
>   nsIView*                        mInnerView;
>   nsCOMPtr<nsIWidget>             mWidget;
>   nsIntRect                       mWindowlessRect;
>+  PluginBackgroundSink*           mBackgroundSink;

Note what owns this.
Attachment #508726 - Flags: review?(jones.chris.g) → review+
(In reply to comment #89)
> Comment on attachment 508828 [details] [diff] [review]
> Bug 626602, part 9: When possible, copy from a background to an opaque surface
> and have transparent plugins draw directly on the copied background, instead of
> doing alpha recovery
> 
> +    if (mIsTransparent && !CanPaintOnBackground()) {
> 
> How about just checking the format of renderSurface here?
> 

Sure.

> +    bool haveTransparentPixels = mIsTransparent && !CanPaintOnBackground();
> 
> Same here.
> 

OK.

> We don't need to store mBackgroundUpdateCtx in PluginInstanceParent.

I use it to check state.  I can remove those assertions if you'd prefer.
Just remove the assertion or maybe replace it with a debug-only boolean flag.
(In reply to comment #90)
> +     * The sequence counter value to use when calling DoUpdate
> +     */
> +    PRUint64       mSequenceCounter;
> 
> As things stand, updates are always delivered to the sink in sequence
> even with D3D10 so we'll never reject updates, but there's no reason
> to make that assumption so this is fine.
> 

Actually we could get a SetUnknown() with a pending readback, so this isn't true and we need the counter currently.
(In reply to comment #57)
> With your updated patches on wmode=transparent GUIMark3, I'm seeing backgrounds
> larger than the plugin frame.  The plugin ended up with an 1100x750 surface
> with an 1100x1100 background.  Not sure if that was intended.

I'm still seeing this with latest patches.  Digging.
Silly typo on my part.  Fixed.  Thanks, Karl.
(In reply to comment #93)
> Just remove the assertion or maybe replace it with a debug-only boolean flag.

Removed.
Comment on attachment 508828 [details] [diff] [review]
Bug 626602, part 9: When possible, copy from a background to an opaque surface and have transparent plugins draw directly on the copied background, instead of doing alpha recovery

>@@ -2673,17 +2680,17 @@ PluginInstanceChild::PaintRectToSurface(
>         // Don't use mHelperSurface if surface is image and mMaemoImageRendering is TRUE
>         if (!mMaemoImageRendering ||
>             renderSurface->GetType() != gfxASurface::SurfaceTypeImage)
> #endif
>         renderSurface = mHelperSurface;
>     }
> #endif
> 
>-    if (mIsTransparent) {
>+    if (mIsTransparent && !CanPaintOnBackground()) {

I wondered whether it might be tidier to check whether aColor.a is non-zero,
if you like.

>+    return (mBackground &&
>+            mCurrentSurface &&
>+            mCurrentSurface->GetSize() <= mBackground->GetSize());

An equals check would be better than <=, because something is out of sync if
they are not equal.  (It looks like we are not sending partial backgrounds,
because there is no offset.)  I wonder whether the size check could just be an
assertion.

>+    *aCtx = nsRefPtr<gfxContext>(mBackgroundUpdateCtx).forget().get();

That's quite a mouthful.  What do you think about using NS_ADDREF()?
(I wonder whether this could even return the already_AddRefed instead of
 the nsresult.)

>+    // We're not in a good position to do anything if this fails.  Let
>+    // normal cleanup kick in.
>+    if (mNewBackground) {
>+        // Have to XSync here to avoid the plugin trying to draw with
>+        // this surface racing with its creation in the X server.
>+        // (NB: we don't XSync after each redraw, although technically
>+        // we should to avoid some classes of artifacts.)
>+        XSync(DefaultXDisplay(), False);

I think we probably should sync on each redraw, because there is no
notification to repaint when the draw has completed.
(Maybe one day we'll send a message via the server to the plugin so only the
plugin needs to wait.) 

>+    bool CreateBackground(const nsIntSize& aSize);
>+    void DestroyBackground();
>+    SurfaceDescriptor BackgroundDescriptor() /*const*/;
>+
>+    NS_OVERRIDE
>+    virtual PPluginBackgroundDestroyerParent*
>+    AllocPPluginBackgroundDestroyer();
>+
>+    NS_OVERRIDE
>+    virtual bool
>+    DeallocPPluginBackgroundDestroyer(PPluginBackgroundDestroyerParent* aActor);

I prefer to have methods declared above together with the other methods, so that the variables are together and their order can be more easily seen.  The "#if defined(OS_WIN)" section has already ruined that, but can we avoid going
further in that direction?

>+    // shared to the plugin subprocess.  This exists to work around
>+    // problems with the windows SharedDIB.

If you could be a bit more specific about the "problems" here, that might be
helpful.
Attachment #508828 - Flags: review?(karlt) → review+
>+PluginInstanceChild::RecvPPluginBackgroundDestroyerConstructor(
>+    PPluginBackgroundDestroyerChild* aActor)
>+{
>+    mBackground = nsnull;
>+    ClearCurrentSurface();
>+    return PPluginBackgroundDestroyerChild::Send__delete__(aActor);
>+}

Should this do an immediate Invalidate/ShowPlugin like RecvUpdateBackground?
> >@@ -2673,17 +2680,17 @@ PluginInstanceChild::PaintRectToSurface(
> >         // Don't use mHelperSurface if surface is image and mMaemoImageRendering is TRUE
> >         if (!mMaemoImageRendering ||
> >             renderSurface->GetType() != gfxASurface::SurfaceTypeImage)
> > #endif
> >         renderSurface = mHelperSurface;
> >     }
> > #endif
> > 
> >-    if (mIsTransparent) {
> >+    if (mIsTransparent && !CanPaintOnBackground()) {
> 
> I wondered whether it might be tidier to check whether aColor.a is non-zero,
> if you like.
> 

I'm checking the renderSurface content type now.

> >+    return (mBackground &&
> >+            mCurrentSurface &&
> >+            mCurrentSurface->GetSize() <= mBackground->GetSize());
> 
> An equals check would be better than <=, because something is out of sync if
> they are not equal.

Yep, fixed.

> (It looks like we are not sending partial backgrounds,
> because there is no offset.)  I wonder whether the size check could just be an
> assertion.

I think it's possible for the plugin window to resize and the plugin to repaint before the background catches up.  That's not a big problem, it'll just cause a temporary rendering glitch, to which this scheme is vulnerable in general.

> 
> >+    *aCtx = nsRefPtr<gfxContext>(mBackgroundUpdateCtx).forget().get();
> 
> That's quite a mouthful.  What do you think about using NS_ADDREF()?
> (I wonder whether this could even return the already_AddRefed instead of
>  the nsresult.)
> 
> >+    // We're not in a good position to do anything if this fails.  Let
> >+    // normal cleanup kick in.
> >+    if (mNewBackground) {
> >+        // Have to XSync here to avoid the plugin trying to draw with
> >+        // this surface racing with its creation in the X server.
> >+        // (NB: we don't XSync after each redraw, although technically
> >+        // we should to avoid some classes of artifacts.)
> >+        XSync(DefaultXDisplay(), False);
> 
> I think we probably should sync on each redraw, because there is no
> notification to repaint when the draw has completed.
> (Maybe one day we'll send a message via the server to the plugin so only the
> plugin needs to wait.) 

That's still not going to prevent glitches in general because after the XSync, the browser can keep drawing to the surface before the plugin re-renders.  How is the XSync going to help?

> 
> >+    bool CreateBackground(const nsIntSize& aSize);
> >+    void DestroyBackground();
> >+    SurfaceDescriptor BackgroundDescriptor() /*const*/;
> >+
> >+    NS_OVERRIDE
> >+    virtual PPluginBackgroundDestroyerParent*
> >+    AllocPPluginBackgroundDestroyer();
> >+
> >+    NS_OVERRIDE
> >+    virtual bool
> >+    DeallocPPluginBackgroundDestroyer(PPluginBackgroundDestroyerParent* aActor);
> 
> I prefer to have methods declared above together with the other methods, so
> that the variables are together and their order can be more easily seen.  The
> "#if defined(OS_WIN)" section has already ruined that, but can we avoid going
> further in that direction?

Sure.

> 
> >+    // shared to the plugin subprocess.  This exists to work around
> >+    // problems with the windows SharedDIB.
> 
> If you could be a bit more specific about the "problems" here, that might be
> helpful.

This comment is actually obsolete now, I can remove it.  The issue is that SharedDIBWin doesn't integrate into the IPC system so it needs special code to avoid "re-sharing" it back and forth between processes each time it's used.  This isn't itself a problem, except windows xp apparently has buggy HANDLE management, so we got bug 613915 (see the fix in bug 625425).  But, it turned out we can use gfxSharedImageSurface for the background on windows, which doesn't have these problems.
(In reply to comment #99)
> >+PluginInstanceChild::RecvPPluginBackgroundDestroyerConstructor(
> >+    PPluginBackgroundDestroyerChild* aActor)
> >+{
> >+    mBackground = nsnull;
> >+    ClearCurrentSurface();
> >+    return PPluginBackgroundDestroyerChild::Send__delete__(aActor);
> >+}
> 
> Should this do an immediate Invalidate/ShowPlugin like RecvUpdateBackground?

Hmmmmmm.  The cases are,

 - destroy background before new background.  In that case, an immediate re-render would be counterproductive.
 - destroy background because we can't use background-copying anymore.  Rerendering soon would be useful because the browser has a stale front surface until the plugin redraws.  I think, however, that a SetWindow should be on the way that will trigger redraw.

Is it worth doing a "soon" ShowPlugin for the second case?  I sort of lean towards "let experience tell" unless someone has good info otherwise.
(In reply to comment #101)
>  - destroy background because we can't use background-copying anymore. 
> Rerendering soon would be useful because the browser has a stale front surface
> until the plugin redraws.  I think, however, that a SetWindow should be on the
> way that will trigger redraw.

Yes, that's the case I'm worried about.  I'm not sure a SetWindow will necessarily arrive and I don't know whether that necessarily invalidates.

> 
> Is it worth doing a "soon" ShowPlugin for the second case?  I sort of lean
> towards "let experience tell" unless someone has good info otherwise.

How about just an invalidate then.  Essentially we're being told that the background has changed, so we need to record that a redraw is necessary.
OK, I think invalidating is a good compromise.  Added that.
(In reply to comment #98)
> >+    *aCtx = nsRefPtr<gfxContext>(mBackgroundUpdateCtx).forget().get();
> 
> That's quite a mouthful.  What do you think about using NS_ADDREF()?
> (I wonder whether this could even return the already_AddRefed instead of
>  the nsresult.)

Oops sorry, missed this on first glance.  I can use the ADDREF() macro but I don't like it.  I'm trying to start a new idiom :).  This method has an nsresult return to match style in PluginLibrary.
Actually that code went away with roc's suggested changes anyway.  It's now

    nsRefPtr<gfxContext> ctx = new gfxContext(mBackground);
    *aCtx = ctx.forget().get();
Created attachment 508948 [details] [diff] [review]
part 9: When possible, copy from a background to an opaque surface and have transparent plugins draw directly on the copied background, instead of doing alpha recovery or hoping plugins give us alpha

Lots of updates, reposting.  Carrying over r+ and sr+.
Attachment #508828 - Attachment is obsolete: true
Attachment #508948 - Flags: superreview+
Attachment #508948 - Flags: review?(benjamin)
Attachment #508828 - Flags: review?(benjamin)
(In reply to comment #105)
>     nsRefPtr<gfxContext> ctx = new gfxContext(mBackground);
>     *aCtx = ctx.forget().get();

I'm happier with that, for some reason.
Perhaps because having an object exist with refcount == 0 is a bit crazy and nsRefPtr is usually the way to ensure that doesn't happen (for long).
(In reply to comment #90)
> Would you add a comment noting that this code exists to work around
> broken plugins?  I'm not sure what the conditions are for dropping
> this code, but if you know them, please add them here too.  I'm
> concerned about enterprising but misguided folks attempting to use
> this for drawWindow or -moz-element.

It's not really "broken plugins", but "unfortunate plugin rendering APIs". I wrote:
 * This API exists to work around the limitations of transparent windowless
 * plugin rendering APIs. It should not be used for anything else.

> As things stand, updates are always delivered to the sink in sequence
> even with D3D10 so we'll never reject updates, but there's no reason
> to make that assumption so this is fine.
> 
> Too bad we couldn't spend this time and code on, say, implementing fast
> -moz-element, but gotta do what ya gotta do.

Yes we do!

(In reply to comment #91)
> I'd share this logic, up to you though.

Done.

> The craziness is that you're doing the opposite of what we would
> normally do to optimize when there's occlusion?

Yes. Updated the comment.

> return (mInstanceOwner && mInstanceOwner->UseLayers()) ? LAYER_ACTIVE :
> LAYER_NONE;

Done.

All other comments addressed; updated patches in queue.
Comment on attachment 508726 [details] [diff] [review]
Part 3: Hook up to nsObjectFrame

I haven't fully read the next patch, or understand the details of this bug, but here are some comments on this patch.

-nsRect
-nsDisplayPlugin::GetBounds(nsDisplayListBuilder* aBuilder)
-{
-  nsRect r = mFrame->GetContentRect() - mFrame->GetPosition() +
-    ToReferenceFrame();
-  if (aBuilder->IsForPluginGeometry()) {
-    // Return the geometry we want, not the geometry we have (which is based
-    // on the surface the plugin last gave us)
-    return r;
-  }

Did this comment cease to be relevant anymore?

+  // *Add* our bounds to the visible region so that stuff underneath us is
+  // likely to be made visible, so we can use it for a background! This is
+  // a bit crazy but should be OK.
+  aVisibleRegion->Or(*aVisibleRegion, GetBounds(aBuilder));
+  return PR_TRUE;

Feel like this bit of crazyness should have gotten review from someone in layout, so they are at least aware of the change.

So this could expand the visible region to extend outside what the visible region was to start with (outside of the whole window)?. We want that so we get the background behind the entirity of the plugin? After visibility analysis in nsLayoutUtils::PaintFrame we pass the final visible region to SetFinalTransparentRegion and UpdateTransparentRegion. I'm worried this excess visible area could cause problems with that.

In bug 622328 wasn't the point of ensuring we removed the root content document background from the visible region so that all plugins were contained in a known opaque part of the window? This does the exact opposite.

Updated

7 years ago
Whiteboard: [hardblocker] → [hardblocker][has patch]
(In reply to comment #109)
> +  // *Add* our bounds to the visible region so that stuff underneath us is
> +  // likely to be made visible, so we can use it for a background! This is
> +  // a bit crazy but should be OK.
> +  aVisibleRegion->Or(*aVisibleRegion, GetBounds(aBuilder));
> +  return PR_TRUE;
> 
> Feel like this bit of crazyness should have gotten review from someone in
> layout, so they are at least aware of the change.

Sorry, that was meant to be in part 4, and the someone in layout is you :-)

> So this could expand the visible region to extend outside what the visible
> region was to start with (outside of the whole window)?. We want that so we get
> the background behind the entirity of the plugin? After visibility analysis in
> nsLayoutUtils::PaintFrame we pass the final visible region to
> SetFinalTransparentRegion and UpdateTransparentRegion. I'm worried this excess
> visible area could cause problems with that.

Part 4 controls that, sorry. Part 4 ensures that we never extend the visible region outside the clip region for the display item.
Created attachment 509031 [details] [diff] [review]
part fold-into-9: Fix reftests on non-maemo X11

There were two problems causing reftests to fail
 - When using alpha recovery on a plugin that hasn't yet been painted, we temporarily "make it visible" by fudging its clip rect then resetting it when done.  When we're doing alpha extraction though, we end up calling the clip-fudging code twice.  The second time, it didn't deliver SetWindow to the plugin because it didn't think anything important had changed (normally it's forced, by a browser message).  I hoisted this to a higher-up level on karl's advice.
 - the second was to switch to checking for non-transparent colors in PaintRectToSurface to see whether or not to clear.  I changed this to check for CONTENT_COLOR_ALPHA but messed up and used the wrong surface.  So, maybe checking the color is better :).

(I'll need to fold this into part 9 before landing so as not to break bisection.)
Attachment #509031 - Flags: review?(karlt)
These patches are failing a bunch of tests with d3d10+d2d for what looks a pretty dumb offset issue: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1296616295.1296620214.12075.gz .  This is *without* Bas's patch applied, so pure alpha recovery.  Probably doesn't make a difference though.

I don't have a d3d10+d2d environment set up because my driver is being blocked.  I'll try upgrading, but in the meantime if anyone else wants to take a look at these, please do.
Comment on attachment 509031 [details] [diff] [review]
part fold-into-9: Fix reftests on non-maemo X11

>+
>+    UpdateWindowAttributes();

I don't think this is necessary here in ShowPluginFrame() as PaintRectToPlatformSurface() will do this.

There is a small change in logic with the intersectino of the clip rect with the expose rect now not making the expose rect of the bogus paint empty.  But on the this first bogus paint, I assume mBackSurface is NULL and so ReadbackDifferenceRect() will fail and rect was already enlarged to the whole plugin anyway.  i.e. in the end, there is no real change to the rect.
Attachment #509031 - Flags: review?(karlt) → review+
Comment on attachment 508827 [details] [diff] [review]
part 8: Dig a tunnel from nsObjectFrame to PluginInstanceParent for background copying


> nsresult
> PluginModuleParent::AsyncSetWindow(NPP instance, NPWindow* window)
> {
>     PluginInstanceParent* i = InstCast(instance);
>-    if (!i)
>-        return NS_ERROR_FAILURE;
>-
>-    return i->AsyncSetWindow(window);
>+    return !i ? NS_ERROR_FAILURE : i->AsyncSetWindow(window);
> }

I know this is functionally equivalent, but I prefer the "if" version of this, so please keep that version and use it below instead of the trigraph.

>diff --git a/layout/generic/nsObjectFrame.cpp b/layout/generic/nsObjectFrame.cpp

>+  already_AddRefed<nsIPluginInstance_MOZILLA_2_0_BRANCH>
>+  GetInstance()
>+  {
>+    nsCOMPtr<nsIPluginInstance_MOZILLA_2_0_BRANCH> inst = do_QueryInterface(mInstance);
>+    return inst.forget();
>+  }

nsIPluginInstance_MOZILLA_2_0_BRANCH should inherit from nsIPluginInstance. Then make mInstance a nsIPluginInstance_MOZILLA_2_0_BRANCH. That way you only have to QI once in nsPluginInstanceOwner::SetInstance, and you get simpler code lower down.

>diff --git a/modules/plugin/base/src/PluginPRLibrary.cpp b/modules/plugin/base/src/PluginPRLibrary.cpp

>+nsresult
>+PluginPRLibrary::SetBackgroundUnknown(NPP instance)
>+{
>+  nsNPAPIPluginInstance* inst = (nsNPAPIPluginInstance*)instance->ndata;
>+  NS_ENSURE_TRUE(inst, NS_ERROR_NULL_POINTER);
>+  return NS_ERROR_NOT_IMPLEMENTED;

assert here and below, this should never be called (yet) on in-process plugins.

>diff --git a/modules/plugin/base/src/nsNPAPIPlugin.cpp b/modules/plugin/base/src/nsNPAPIPlugin.cpp

>   nsNPAPIPluginInstance *inst = (nsNPAPIPluginInstance *) npp->ndata;
>+  nsIPluginInstance *iinst = inst;

With the inherited interface you don't need this local.

>diff --git a/modules/plugin/base/src/nsNPAPIPluginInstance.cpp b/modules/plugin/base/src/nsNPAPIPluginInstance.cpp

>+class NS_STACK_CLASS AutoPluginLibraryCall {

nit, opening brace goes in column 0 of the next line

> NS_IMETHODIMP
> nsNPAPIPluginInstance::AsyncSetWindow(NPWindow* window)
> {
>   if (RUNNING != mRunning)
>     return NS_OK;
> 
>-  PluginDestructionGuard guard(this);
>-
>-  if (!mPlugin)
>-    return NS_ERROR_FAILURE;
>-
>-  PluginLibrary* library = mPlugin->GetLibrary();
>-  if (!library)
>-    return NS_ERROR_FAILURE;
>-
>-  return library->AsyncSetWindow(&mNPP, window);
>+  AutoPluginLibraryCall library(this);
>+  return !library ? NS_ERROR_FAILURE : library->AsyncSetWindow(&mNPP, window);
> }

Again, please avoid the trigraph here and below.

>diff --git a/modules/plugin/base/src/nsNPAPIPluginInstance.h b/modules/plugin/base/src/nsNPAPIPluginInstance.h

>-class nsNPAPIPluginInstance : public nsIPluginInstance
>+class nsNPAPIPluginInstance : public nsIPluginInstance,
>+                              public nsIPluginInstance_MOZILLA_2_0_BRANCH

With the inherited interface this can just list the _BRANCH interface.

>diff --git a/modules/plugin/base/src/nsPluginHost.cpp b/modules/plugin/base/src/nsPluginHost.cpp
>--- a/modules/plugin/base/src/nsPluginHost.cpp
>+++ b/modules/plugin/base/src/nsPluginHost.cpp
>@@ -623,17 +623,18 @@ nsresult nsPluginHost::GetPrompt(nsIPlug
> NS_IMETHODIMP nsPluginHost::GetURL(nsISupports* pluginInst,
>                                    const char* url,
>                                    const char* target,
>                                    nsIPluginStreamListener* streamListener,
>                                    const char* altHost,
>                                    const char* referrer,
>                                    PRBool forceJSEnabled)
> {
>-  return GetURLWithHeaders(static_cast<nsNPAPIPluginInstance*>(pluginInst),
>+  nsIPluginInstance *inst = static_cast<nsIPluginInstance*>(pluginInst);
>+  return GetURLWithHeaders(static_cast<nsNPAPIPluginInstance*>(inst),

and this cast will no longer be necessary.

>-  nsNPAPIPluginInstance* instance = static_cast<nsNPAPIPluginInstance*>(pluginInst);
>+  nsIPluginInstance *iinstance = static_cast<nsIPluginInstance*>(pluginInst);
>+  nsNPAPIPluginInstance* instance = static_cast<nsNPAPIPluginInstance*>(iinstance);

Nor these.

r=me with those changes. Note that bug 591687 will want to tack on to the branch version of nsIPluginInstance.
Attachment #508827 - Flags: review?(benjamin) → review+
Comment on attachment 508948 [details] [diff] [review]
part 9: When possible, copy from a background to an opaque surface and have transparent plugins draw directly on the copied background, instead of doing alpha recovery or hoping plugins give us alpha

I really think we could use a few more reftests for opaque plugins, specifically these cases:

* the plugin moves over the background but never invalidates itself
* force the plugin to do alpha recovery by making the background very complex, to test the alpha-recovery codepath
* The plugin invalidates part of the screen simultaneously as the background is being updated.
Attachment #508948 - Flags: review?(benjamin) → review+
(In reply to comment #113)
> Comment on attachment 509031 [details] [diff] [review]
> part fold-into-9: Fix reftests on non-maemo X11
> 
> >+
> >+    UpdateWindowAttributes();
> 
> I don't think this is necessary here in ShowPluginFrame() as
> PaintRectToPlatformSurface() will do this.
> 

Yes, that's right.  Fixed.
> > nsresult
> > PluginModuleParent::AsyncSetWindow(NPP instance, NPWindow* window)
> > {
> >     PluginInstanceParent* i = InstCast(instance);
> >-    if (!i)
> >-        return NS_ERROR_FAILURE;
> >-
> >-    return i->AsyncSetWindow(window);
> >+    return !i ? NS_ERROR_FAILURE : i->AsyncSetWindow(window);
> > }
> 
> I know this is functionally equivalent, but I prefer the "if" version of this,
> so please keep that version and use it below instead of the trigraph.
> 

OK.  Changed others.

> >diff --git a/layout/generic/nsObjectFrame.cpp b/layout/generic/nsObjectFrame.cpp
> 
> >+  already_AddRefed<nsIPluginInstance_MOZILLA_2_0_BRANCH>
> >+  GetInstance()
> >+  {
> >+    nsCOMPtr<nsIPluginInstance_MOZILLA_2_0_BRANCH> inst = do_QueryInterface(mInstance);
> >+    return inst.forget();
> >+  }
> 
> nsIPluginInstance_MOZILLA_2_0_BRANCH should inherit from nsIPluginInstance.
> Then make mInstance a nsIPluginInstance_MOZILLA_2_0_BRANCH. That way you only
> have to QI once in nsPluginInstanceOwner::SetInstance, and you get simpler code
> lower down.

Fixed.  Much better.

> 
> >diff --git a/modules/plugin/base/src/PluginPRLibrary.cpp b/modules/plugin/base/src/PluginPRLibrary.cpp
> 
> >+nsresult
> >+PluginPRLibrary::SetBackgroundUnknown(NPP instance)
> >+{
> >+  nsNPAPIPluginInstance* inst = (nsNPAPIPluginInstance*)instance->ndata;
> >+  NS_ENSURE_TRUE(inst, NS_ERROR_NULL_POINTER);
> >+  return NS_ERROR_NOT_IMPLEMENTED;
> 
> assert here and below, this should never be called (yet) on in-process plugins.
> 

OK.

> >diff --git a/modules/plugin/base/src/nsNPAPIPluginInstance.cpp b/modules/plugin/base/src/nsNPAPIPluginInstance.cpp
> 
> >+class NS_STACK_CLASS AutoPluginLibraryCall {
> 
> nit, opening brace goes in column 0 of the next line
> 

K.
I can repro the windows reftest failures on my machine capable of d3d10+d2d, although it can't drive a high enough screen resolution to USE_WIDGET_LAYERS.  Anyway, the problem looks to be

0[3ac658]:       BasicImageLayer (0x5e58ca0) [clip=(x=0, y=0, w=800, h=1000)] [transform=[ 1 0; 0 1; 200 200; ]] [visible=< (x=-200, y=-200, w=400, h=400); >]

which is wrong.  The positioning is correct on my linux desktop.  This looks suspiciously like the fallback code to position the plugin image in the middle of the enclosing frame when frame/image size mismatch.  This also might be the cause of that T-Mobile bug.
Comment on attachment 508726 [details] [diff] [review]
Part 3: Hook up to nsObjectFrame

> already_AddRefed<Layer>
> nsObjectFrame::BuildLayer(nsDisplayListBuilder* aBuilder,
>                           LayerManager* aManager,
>                           nsDisplayItem* aItem)
> {
>+  // Create image
>+  nsRefPtr<ImageContainer> container = GetImageContainer(aManager);
>+  if (!container)
>+    return nsnull;
>+  gfxIntSize size = container->GetCurrentSize();

I missed a problem here on first review, sorry: if this is the first time processing a display item for the object frame, and there's no readback layer, this call will end up creating a new container but not completely initializing it.  So, the size ends up <0,0> here even though the plugin's surface and its frame area are not <0,0>.  This breaks the centering code below.

>+  r.pos.x += (r.Width() - size.width) / 2;
>+  r.pos.y += (r.Height() - size.height) / 2;
>   transform.Translate(r.pos);
Attachment #508726 - Flags: review+ → review-
Blocks: 612113
(In reply to comment #118)
> I can repro the windows reftest failures on my machine capable of d3d10+d2d,
> although it can't drive a high enough screen resolution to USE_WIDGET_LAYERS. 
> Anyway, the problem looks to be
> 
> 0[3ac658]:       BasicImageLayer (0x5e58ca0) [clip=(x=0, y=0, w=800, h=1000)]
> [transform=[ 1 0; 0 1; 200 200; ]] [visible=< (x=-200, y=-200, w=400, h=400);
> >]
> 
> which is wrong.  The positioning is correct on my linux desktop.  This looks
> suspiciously like the fallback code to position the plugin image in the middle
> of the enclosing frame when frame/image size mismatch.  This also might be the
> cause of that T-Mobile bug.

Maybe. That positioning code also causes problems for plugins that change their height. I think we should just position the plugin image at the top-left of the frame. Can you try that?
Sure, that would also make the reftests pass.  I think we should still fix BuildLayer though, too.
What do you mean? That positioning code is in BuildLayer.
I meant fix BuildLayer's use of the plugin's image container size before initializing the container.
Created attachment 509338 [details] [diff] [review]
Some reftests and a crashtest

This is kind of a tricky feature to test :S.  I added the following
 - check that we don't crash when switching background<->alpha-recovery, in a few ways
 - check that we get the right pixels with a few different backgrounds
 - check that while resizing and moving the plugin around, we don't observe illegal pixel values

Not sure I need review on these, but feedback of course appreciated.
I think we're missing
 (1) properly initialize the plugin image container in part 3.  Or just stop trying to center it in the frame, so as to pass reftests.  Or both.
 (2) review on part 4
 (3) updated part 6
 (4a) another reftest run on try
 (4b) figure out the shouldWaitForPendingPaints() weirdness in the win XP reftest
Created attachment 509362 [details] [diff] [review]
Part 3 v2

This should have all comments addressed.

I've dealt with the ImageContainer initialization issue by moving the initializer up out of the if-block. I've also arranged things so we only call it once per display list construction.
Attachment #509362 - Flags: review?(jones.chris.g)
I also had to refresh part 4, you can pull that from my queue if you like.
Comment on attachment 509362 [details] [diff] [review]
Part 3 v2

Should we have another bug for changing/removing the image centering?
Attachment #509362 - Flags: review?(jones.chris.g) → review+
Yes!
Created attachment 509617 [details] [diff] [review]
rollup, v5

In case anyone wants it.  Based on m-c c618b7b138fb.
Attachment #508665 - Attachment is obsolete: true
(In reply to comment #70)
> +      ctx->SetColor(gfxRGBA(0, 1, 0));
> 
> I hope you didn't mean to include this :-)
Heh... no, I didn't :)

> Can we create the ReadbackManager lazily? That would avoid any startup impact
> --- and overhead, if the user happens to not encounter a transparent windowless
> plugin.

It could be done fairly easily (i.e. readbackManager() could just call EnsureReadbackManager() first), although I'm not sure it's worth the hassle? The creation really doesn't do a lot of work, initializing a semaphore, a critical section an event and creating a thread should be fairly cheap. Do you think these transparent windowless flash instances are rare enough?

> +      nsRefPtr<ID3D10Texture2D> readbackTexture;
> +      device()->CreateTexture2D(&desc, NULL, getter_AddRefs(readbackTexture));
> +      device()->CopyResource(readbackTexture, mTexture);
> 
> How much of an issue is it that you copy the entire texture? Seems like it
> would save a bit of time and VRAM if we copied only the rectangle(s) needed by
> the plugin.

It only saves us RAM, and even then only briefly, since the readback texture exists in RAM and only for the lifetime of the readback task. As for bus bandwidth I think the impact is not significant, this seems the most reliable (and least bug prone) method.
Blocks: 630690
Created attachment 509783 [details] [diff] [review]
Implement asynchronous D3D10 readback v4
Attachment #508663 - Attachment is obsolete: true
Attachment #509783 - Flags: review?(jones.chris.g)
Attachment #508663 - Flags: review?(roc)
Comment on attachment 509783 [details] [diff] [review]
Implement asynchronous D3D10 readback v4

>+  size = sizeof(ReadbackManagerD3D10*);
>+  if (FAILED(mDevice->GetPrivateData(sReadbackManager, &size, mReadbackManager.StartAssignment()))) {
>+    mReadbackManager = new ReadbackManagerD3D10();
>+    mDevice->SetPrivateDataInterface(sReadbackManager, mReadbackManager);
>+  }
>+

I agree with Rob that creating this lazily would be better.  It
wouldn't require any more lines of code than this and eliminates the
risk of a Ts regression from spawning the thread.  I would expect many
browsing sessions to start and finish without creating a transparent
plugin too, no need to waste a thread per window.

>+// Structure that contains the information required to execute a readback task,
>+// the only member accessed off the main thread here is mReadbackTexture.
>+struct ReadbackTask {
>+  // The texture that we copied the contents of the thebeslayer to.
>+  nsRefPtr<ID3D10Texture2D> mReadbackTexture;
>+  // This exists purely to keep the ReadbackLayer alive for the lifetime of
>+  // mUpdate.
>+  nsRefPtr<ReadbackLayer> mLayer;

Add a note here that mLayer can only be addref'd and released on the
main thread, so users should be very careful with ReadbackTasks.

>+
>+class ReadbackResultWriter : public nsIRunnable

I would add a comment describing how this interacts with the readback
thread, especially that this must be destroyed on the main thread.

>+ReadbackManagerD3D10::~ReadbackManagerD3D10()
>+{
>+  ::SetEvent(mShutdownEvent);
>+
>+  // This shouldn't take longer than 5 seconds, if it does we're going to choose
>+  // to leak the thread and its synchronisation in favor of crashing or freezing
>+  DWORD result = ::WaitForSingleObject(mTaskThread, 5000);

This sucks, but bleh.  We only /really/ have to do this during XPCOM
shutdown, so if need be we could add that complexity later.

>+  void PostTask(ID3D10Texture2D *aTexture, void *aUpdate, const gfxPoint &aOrigin);

Just include ReadbackProcessor.h and use |ReadbackProcessor::Update*|.
You're already including windows.h, it can't get any worse ;).

>+  // These objects provide a very simple solution to the Producer-Consumer
>+  // problem. The producer will signal mTaskSemaphore at each placement, and
>+  // the consumer will take a task from the queue for each completed wait.
>+  // Releasing the consumer and taking the task does not happen atomically,
>+  // and similarly the producer placing a task and signalling the semaphore does
>+  // not happen atomically.
>+  // mTaskGuard is used here to synchronise access to mPendingReadbackTasks.
>+  // This works fine since the only invariant we wish to guard is:
>+  //
>+  // #waits <= postedTasks
>+  //
>+  // As well as an end-state where
>+  //
>+  // #waits = postedTasks
>+  //
>+  // Both conditions are satisfied here.

I think all you need to say here is something like,

  // The invariant maintained by |mTaskSemaphore| is that the readback thread
  // will awaken from WaitForMultipleObjects() at least once per readback 
  // task enqueued by the main thread.  Since the readback thread processes
  // exactly one task per wakeup (with one exception), no tasks are lost.  The
  // exception is when the readback thread is shut down, which orphans the
  // remaining tasks, on purpose.

>+  CRITICAL_SECTION mTaskGuard;

Convention is "mTaskMutex". 

>+  HANDLE mTaskSemaphore;

So, what you really want here is a condition variable, but I
understand that they're rocket science on windows so meh.

>+  // FiFo list of readback tasks that are to be executed.
>+  nsTArray<nsAutoPtr<ReadbackTask>> mPendingReadbackTasks;

Note here that access to the task queue must be serialized by
mTaskMutex.  I would put mTaskMutex next to mPendingReadbackTasks in
this list to make their relationship clearer.

>diff --git a/gfx/layers/d3d9/ThebesLayerD3D9.cpp b/gfx/layers/d3d9/ThebesLayerD3D9.cpp
>--- a/gfx/layers/d3d9/ThebesLayerD3D9.cpp
>+++ b/gfx/layers/d3d9/ThebesLayerD3D9.cpp
>@@ -472,16 +472,17 @@ ThebesLayerD3D9::DrawRegion(const nsIntR
>     const ReadbackProcessor::Update& update = aReadbackUpdates[i];
>     nsIntPoint offset = update.mLayer->GetBackgroundLayerOffset();
>     nsRefPtr<gfxContext> ctx =
>         update.mLayer->GetSink()->BeginUpdate(update.mUpdateRect + offset,
>                                               update.mSequenceCounter);
>     if (ctx) {
>       ctx->Translate(gfxPoint(offset.x, offset.y));
>       ctx->SetSource(destinationSurface, gfxPoint(bounds.x, bounds.y));
>+      ctx->SetColor(gfxRGBA(0, 1, 0));

What's this?
Attachment #509783 - Flags: review?(jones.chris.g)
I see some problems running the d3d10 patch on the reftests: http://pastebin.mozilla.org/1024126

The worst appear to be

WARNING: Failed to create texture for image surface Error code: 0: file c:/Users/cjones/mozilla/ff-dbg/gfx/layers/../../../mozilla-central/gfx/layers/d3d10/LayerManagerD3D10.cpp, line 601

(which I think is causing the reftest == failure) and

###!!! ABORT: Update outside of background area: 'nsIntRect(0, 0, sz.width, sz.height).Contains(aRect)', file c:/Users/cjones/mozilla/ff-dbg/dom/plugins/../../../mozilla-central/dom/plugins/PluginInstanceParent.cpp, line 622
Comment on attachment 508727 [details] [diff] [review]
Part 4: Make display items behind the plugin as visible as possible

   virtual PRBool ComputeVisibility(nsDisplayListBuilder* aBuilder,
                                    nsRegion* aVisibleRegion,
+                                   const nsRect& aAllowVisibleRegionExpansion,
                                    PRBool& aContainsRootContentDocBG)

You should document this new parameter.

@@ -738,17 +739,17 @@ PRBool nsDisplayItem::RecomputeVisibilit
-  if (!ComputeVisibility(aBuilder, aVisibleRegion, notUsed))
+  if (!ComputeVisibility(aBuilder, aVisibleRegion, nsRect(), notUsed))

Might want a comment here explaning that we only need the expansion of visible region for plugins with layers, so the plugin won't be in the thebes layer and we don't need to expand the visibe region within layers.


There are two other things that I think are subtle, but correct. We always clip subdocuments, and we clip the visible region expansion rect so that the visible region can never make anything in chrome transparent that wasn't before. And adding plugin bounds to the visible region could mean that the root content doc area is no longer considered opaque, but the root content doc background is always behind plugins, so we always end up removing it from the visible region.
Attachment #508727 - Flags: review?(tnikkel) → review+
(In reply to comment #135)
> Comment on attachment 508727 [details] [diff] [review]
> Part 4: Make display items behind the plugin as visible as possible
> 
>    virtual PRBool ComputeVisibility(nsDisplayListBuilder* aBuilder,
>                                     nsRegion* aVisibleRegion,
> +                                   const nsRect& aAllowVisibleRegionExpansion,
>                                     PRBool& aContainsRootContentDocBG)
> 
> You should document this new parameter.

"aAllowVisibleRegionExpansion is a rect where we are allowed to expand the visible region and is only used for making sure the background behind a plugin is visible."

> @@ -738,17 +739,17 @@ PRBool nsDisplayItem::RecomputeVisibilit
> -  if (!ComputeVisibility(aBuilder, aVisibleRegion, notUsed))
> +  if (!ComputeVisibility(aBuilder, aVisibleRegion, nsRect(), notUsed))
> 
> Might want a comment here explaning that we only need the expansion of visible
> region for plugins with layers, so the plugin won't be in the thebes layer and
> we don't need to expand the visibe region within layers.

"When we recompute visibility within layers we don't need to expand the visible region for content behind plugins (the plugin is not in the layer)."

roc can of course decide something better when he is back.
Comment on attachment 509783 [details] [diff] [review]
Implement asynchronous D3D10 readback v4

>+    // We can only send the update to the sink on the main thread, so post an
>+    // event there to do so. Ownership of the task is passed from
>+    // mPendingReadbackTasks to ReadbackResultWriter here.
>+    nsCOMPtr<nsIThread> thread = do_GetMainThread();
>+    nsCOMPtr<nsIRunnable> ev = new ReadbackResultWriter(nextReadbackTask);
>+    thread->Dispatch(ev, nsIEventTarget::DISPATCH_NORMAL);

Sigh, there's a subtle threading bug here: you have to let
thread->Dispatch() have the first ref to this task or it's possible
for it to be deleted here, on the readback thread, if the stars align
right.  That unrefs the layer off-main-thread, bad bad.

I think you need

    thread->Dispatch(new ReadbackResultWriter(nextReadbackTask), nsIEventTarget::DISPATCH_NORMAL);

Yay threads.
Comment on attachment 509783 [details] [diff] [review]
Implement asynchronous D3D10 readback v4

Can't land as-is.
Attachment #509783 - Flags: review-
Here's what appears to be triggering the abort:

  UPDATING <x=0,y=0, w=199,h=199> for <w=199,h=199>
WARNING: Failed to create texture for image surface Error code: 0: file c:/Users/cjones/mozilla/ff-dbg/gfx/layers/../../../mozilla-central/gfx/layers/d3d10/LayerManagerD3D10.cpp, line 601
WARNING: Failed to create texture for image surface Error code: 0: file c:/Users/cjones/mozilla/ff-dbg/gfx/layers/../../../mozilla-central/gfx/layers/d3d10/LayerManagerD3D10.cpp, line 601
WARNING: Failed to create texture for image surface Error code: 0: file c:/Users/cjones/mozilla/ff-dbg/gfx/layers/../../../mozilla-central/gfx/layers/d3d10/LayerManagerD3D10.cpp, line 601
WARNING: Failed to create texture for image surface Error code: 0: file c:/Users/cjones/mozilla/ff-dbg/gfx/layers/../../../mozilla-central/gfx/layers/d3d10/LayerManagerD3D10.cpp, line 601
WARNING: Failed to create texture for image surface Error code: 0: file c:/Users/cjones/mozilla/ff-dbg/gfx/layers/../../../mozilla-central/gfx/layers/d3d10/LayerManagerD3D10.cpp, line 601
  UPDATING <x=0,y=0, w=200,h=200> for <w=200,h=200>
WARNING: Failed to create texture for image surface Error code: 0: file c:/Users/cjones/mozilla/ff-dbg/gfx/layers/../../../mozilla-central/gfx/layers/d3d10/LayerManagerD3D10.cpp, line 601
WARNING: Failed to create texture for image surface Error code: 0: file c:/Users/cjones/mozilla/ff-dbg/gfx/layers/../../../mozilla-central/gfx/layers/d3d10/LayerManagerD3D10.cpp, line 601
  UPDATING <x=0,y=0, w=201,h=201> for <w=200,h=200>
###!!! ABORT: Update outside of background area: 'nsIntRect(0, 0, sz.width, sz.height).Contains(aRect)', file c:/Users/cjones/mozilla/ff-dbg/dom/plugins/../../../mozilla-central/dom/plugins/PluginInstanceParent.cpp, line 627

We get an update for area 199x199, then 200x200 (the test gradually grows the plugin's size).  Since there wasn't an abort in between, there must have been an intervening SetUnknown().  Then we get an update for 201x201, and boom.  There must not have been a SetUnknown() before it.
Tryserver has shown leaking TArrays (on all platforms) and failures in both existing and the new reftests.  Looking into the leak.  Not sure what to do about the tests yet, will poke.
(In reply to comment #133)
> Comment on attachment 509783 [details] [diff] [review]
> Implement asynchronous D3D10 readback v4
> 
> >+  size = sizeof(ReadbackManagerD3D10*);
> >+  if (FAILED(mDevice->GetPrivateData(sReadbackManager, &size, mReadbackManager.StartAssignment()))) {
> >+    mReadbackManager = new ReadbackManagerD3D10();
> >+    mDevice->SetPrivateDataInterface(sReadbackManager, mReadbackManager);
> >+  }
> >+
> 
> I agree with Rob that creating this lazily would be better.  It
> wouldn't require any more lines of code than this and eliminates the
> risk of a Ts regression from spawning the thread.  I would expect many
> browsing sessions to start and finish without creating a transparent
> plugin too, no need to waste a thread per window.

We don't waste a thread per window, only per session/device loss (there's 1 readbackmanager per device). But I think your point is fair enough.
Created attachment 509982 [details] [diff] [review]
Implement asynchronous D3D10 readback v5

Addressed review comments.
Attachment #509783 - Attachment is obsolete: true
Attachment #509982 - Flags: review?(jones.chris.g)
Attachment #509982 - Flags: review?(jones.chris.g) → review+
Bas, are the

TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsTArray_base with size 4 bytes

during mochitests, e.g. http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1296865219.1296867616.27485.gz , caused by the leaking readback manager?  If so we'll need to fix that too before landing.
Created attachment 509990 [details] [diff] [review]
rollup v6

Includes all patches from here, bug 629799, and bug 631388.

The "hard blockers" IMHO for landing this bug are
 - nsTArray leak from plugin process.  I'm working on that right now.
 - nsTArray leak with d3d10 from reference cycle.  Bas might need to do that, or I can look later.
 - crashes on d3d10 from (apparent?) missed SetBackgroundUnknown

I can't repro the reftest failures locally, so I'd say they "soft block", but would be good to understand since the known-bad corner cases shouldn't arise in those tests.
Created attachment 509993 [details] [diff] [review]
part 8.1: Close the XDisplay to free the gfxXlibSurface DisplayTable

Tryserver reported nsTArray_base leaks from the plugin subprocess on the linux boxes.  Valgrind's --show-reachable led right to the problem

==6852== 40 bytes in 1 blocks are still reachable in loss record 1,170 of 2,403
==6852==    at 0x4C2815C: malloc (vg_replace_malloc.c:236)
==6852==    by 0x749C7CD: malloc (nsTraceMalloc.c:1178)
==6852==    by 0x5250EFD: moz_xmalloc (mozalloc.cpp:98)
==6852==    by 0x5B8ED63: nsTArrayInfallibleAllocator::Malloc(unsigned long) (nsTArray.h:84)
==6852==    by 0x5B92AA0: nsTArray_base<nsTArrayDefaultAllocator>::EnsureCapacity(unsigned int, unsigned int) (nsTArray-inl.h:77)
==6852==    by 0x75AE76F: nsTArray<DisplayTable::ColormapEntry, nsTArrayDefaultAllocator>::AppendElements(unsigned int) (nsTArray.h:794)
==6852==    by 0x75AE53E: nsTArray<DisplayTable::ColormapEntry, nsTArrayDefaultAllocator>::AppendElement() (nsTArray.h:809)
==6852==    by 0x75ADC04: DisplayTable::GetColormapAndVisual(Screen*, XRenderPictFormat*, Visual*, unsigned long*, Visual**) (gfxXlibSurface.cpp:366)
==6852==    by 0x75ADD79: gfxXlibSurface::GetColormapAndVisual(unsigned long*, Visual**) (gfxXlibSurface.cpp:402)
==6852==    by 0x71CD474: mozilla::plugins::PluginInstanceChild::MaybeCreatePlatformHelperSurface() (PluginInstanceChild.cpp:2421)
==6852==    by 0x71CD6E6: mozilla::plugins::PluginInstanceChild::EnsureCurrentBuffer() (PluginInstanceChild.cpp:2485)
==6852==    by 0x71CE487: mozilla::plugins::PluginInstanceChild::ShowPluginFrame() (PluginInstanceChild.cpp:2855)

The DisplayTable is freed off an XDisplay close hook, but we were never closing the XDisplay in the plugin subprocess.  This patch has us do that, and fixes the leak for me.  The code was taken from nsAppRunner.

Note that this patch is careful to close the XDisplay after unloading the plugin dso.  The XCloseDisplay() docs say it does an internal XSync(), so this patch doesn't do an explicit one.
Attachment #509993 - Flags: review?(karlt)
(Also I'm not sure why this hasn't popped up before; maybe because we previously never created Xlib surfaces with non-default visuals on configurations tested on tinderbox?)
Created attachment 509995 [details] [diff] [review]
Cleanup D3D10 resources upon last layer manager destruction

This cleans up several D3D10 resources when the last layer manager belonging to a device is destroyed. This takes care of deleting the ReadbackManager.
I'm now able to reproduce a failure on plugin-background-1-step.html fairly reliably, with some new logging code turned on.  I'm out of gas to fix the bug, but what's happening is

1740130144[1f85a60]: [InstanceChild][2b206c002db0] Painting with alpha recovery
307918688[60aa60]: [InstanceParent][2b3318721d70] RecvShow for <x=0,y=0, w=199,h=199>
307918688[60aa60]:    (RecvShow invalidated for surface 2b331856f3c0)
307918688[60aa60]: [InstanceParent][2b3318721d70] BeginUpdateBackground for <x=0,y=0, w=199,h=199>
307918688[60aa60]: [InstanceParent][2b3318721d70] EndUpdateBackground for <x=0,y=0, w=199,h=199>
1740130144[1f85a60]: [InstanceChild][2b206c002db0] Painting on background
307918688[60aa60]: [InstanceParent][2b3318721d70] RecvShow for <x=0,y=0, w=0,h=0>
307918688[60aa60]:    (RecvShow invalidated for surface 2b3318736af0)
1740130144[1f85a60]: [InstanceChild][2b206c002db0] Painting on background
307918688[60aa60]: [InstanceParent][2b3318721d70] RecvShow for <x=0,y=0, w=199,h=199>
307918688[60aa60]:    (RecvShow invalidated for surface 2b3318736be0)
1740130144[1f85a60]: [InstanceChild][2b206c002db0] Painting onto opaque surface
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                           THIS IS BAD
307918688[60aa60]: [InstanceParent][2b3318721d70] BeginUpdateBackground for <x=0,y=0, w=199,h=199>
307918688[60aa60]: [InstanceParent][2b3318721d70] EndUpdateBackground for <x=0,y=0, w=199,h=199>
307918688[60aa60]: [InstanceParent][2b3318721d70] RecvShow for <x=0,y=0, w=200,h=200>
307918688[60aa60]:    (RecvShow invalidated for surface 2b3318743ed0)
1740130144[1f85a60]: [InstanceChild][2b206c002db0] Painting onto opaque surface
307918688[60aa60]: [InstanceParent][2b3318721d70] RecvShow for <x=0,y=0, w=199,h=199>
307918688[60aa60]:    (RecvShow invalidated for surface 2b33187410f0)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                           THIS IS BAD
307918688[60aa60]: [InstanceParent][2b3318721d70] SetBackgroundUnknown
1740130144[1f85a60]: [InstanceChild][2b206c002db0] Painting with alpha recovery
307918688[60aa60]: [InstanceParent][2b3318721d70] BeginUpdateBackground for <x=0,y=0, w=200,h=200>
307918688[60aa60]: [InstanceParent][2b3318721d70] EndUpdateBackground for <x=0,y=0, w=200,h=200>

This test ends up just resizing the plugin frame from 199x199 to 200x200 and then quitting.  There are two bad things happening here: the plugin might be getting a bad SetWindow, since it appears to size itself 199x199->200x200->199x199.  Second, we're falling back on the last-ditch have-background-but-doesn't-match-surface-size path that we shouldn't be hitting on these tests.  This might be related to SetWindow.

More tomorrow.
(In reply to comment #144)
>  - crashes on d3d10 from (apparent?) missed SetBackgroundUnknown

I'm working on this now.
Comment on attachment 509995 [details] [diff] [review]
Cleanup D3D10 resources upon last layer manager destruction

Stylistically, I think I'd prefer to have one struct containing all the things we stick on the device, say DeviceAttachments, so that it's clearer what needs to be done to add/remove items (i.e., add/remove them from DeviceAttachments).  Should make managing the attached items' lifetimes a little easier too.
(In reply to comment #144)
>  - crashes on d3d10 from (apparent?) missed SetBackgroundUnknown

There are two problems here: first is that the PluginBackgroundSink needs to be created with the current seqno of its ReadbackLayer, or it will accept updates it's supposed to reject.  That was an easy fix, but not the problem here.

The second problem is that the d3d10 ReadbackTask ends up holding the last reference to a ReadbackLayer that's been removed from the layer tree, but the dying layer still has a nonnull sink.  The update is delivered to the sink, which is OK in this case, but then when the ReadbackLayer is destroyed after the task is processed, it takes out the PluginBackgroundSink with it, but there's no SetBackgroundUnknown() when it's destroyed.  I think the right fix here is to SetUnknown() on ReadbackLayers when they're removed from the layer tree.
Created attachment 510068 [details] [diff] [review]
part 1.1: When readback layers are removed from the tree, have them SetUnknown() and drop their sink

Pretty sure this is OK, but want a second look.
Attachment #510068 - Flags: review?(roc)
Created attachment 510069 [details] [diff] [review]
part 8.1: Close the XDisplay to free the gfxXlibSurface DisplayTable, for the sake of leak checkers, v2

Changed to do this only for NS_FREE_PERMANENT_DATA as we discussed.
Attachment #509993 - Attachment is obsolete: true
Attachment #510069 - Flags: review?(karlt)
Attachment #509993 - Flags: review?(karlt)
(In reply to comment #136)
> (In reply to comment #135)
> > Comment on attachment 508727 [details] [diff] [review]
> > Part 4: Make display items behind the plugin as visible as possible

I've got all roc's patches imported into my mq now, and I added the comments tn suggested.
Attachment #510069 - Flags: review?(karlt) → review+
I'm about 90% sure I have a fix for the reftest failures, but I no longer can reproduce locally without the patch :/.  Fired off a try run.

Looks like the last dragon to slay is this issue on d3d10 platforms

###!!! ABORT: Expecting rect for whole frame: 'aRect.TopLeft() == nsIntPoint(0, 0)', file e:/builds/moz2_slave/try-w32-dbg/build/dom/plugins/PluginInstanceParent.cpp, line 622

Needless to say I didn't see this locally.  This assertion failing means either that an offset calculation was wrong or that we got an update for a subset of the background area, out of order.
Created attachment 510153 [details] [diff] [review]
part 9.0001: Need to drop the background on SetWindow changes (to be folded into part 9)

This fixes a case where we get SetWindow but not a background update before repainting.  I looked a little closer at the reftest window during runs, and this patch actually fixes visible artifacts.  There were other reftest failures on tryserver that I don't understand yet, but I can reproduce one in a VM set to use a single core with binary translation and logging and tracemalloc.  Sigh.

Still no idea about the failing assertion with d3d10.
Attachment #510153 - Flags: review?(karlt)
Created attachment 510159 [details] [diff] [review]
rollup v7

Includes all the fixes above.
Attachment #509617 - Attachment is obsolete: true
Attachment #509990 - Attachment is obsolete: true
Comment on attachment 510153 [details] [diff] [review]
part 9.0001: Need to drop the background on SetWindow changes (to be folded into part 9)

This assumes that SetWindow always arrives before the UpdateBackground.
I guess that's hard to write an assertion for, but can you make sure that is clearly documented somewhere, please?

> bool
> PluginInstanceChild::RecvPPluginBackgroundDestroyerConstructor(
>     PPluginBackgroundDestroyerChild* aActor)
> {
>+    if (!mBackground)
>+        return true;
>+

What does it mean to skip "PPluginBackgroundDestroyerChild::Send__delete__(aActor)" here?
(In reply to comment #158)
> Comment on attachment 510153 [details] [diff] [review]
> This assumes that SetWindow always arrives before the UpdateBackground.
> I guess that's hard to write an assertion for, but can you make sure that is
> clearly documented somewhere, please?

Sorta kinda.  We can always soldier on without a background.  I'll add a note though.

> > bool
> > PluginInstanceChild::RecvPPluginBackgroundDestroyerConstructor(
> >     PPluginBackgroundDestroyerChild* aActor)
> > {
> >+    if (!mBackground)
> >+        return true;
> >+
> 
> What does it mean to skip
> "PPluginBackgroundDestroyerChild::Send__delete__(aActor)" here?

Bad things :).  (Specifically, hold on to the surface until the PluginInstance is destroyed.)  Nice catch.
Created attachment 510172 [details] [diff] [review]
part 9.0001: Need to drop the background on SetWindow changes (to be folded into part 9), v2

Added a note and fixed the backgrounddestroyer bug.
Attachment #510153 - Attachment is obsolete: true
Attachment #510172 - Flags: review?(karlt)
Attachment #510153 - Flags: review?(karlt)
Attachment #510172 - Flags: review?(karlt) → review+
Attachment #510068 - Flags: review?(roc) → review+
Maybe we could push a patch to try that logs the background update rects sent by the browser, to try to unravel the problem in comment #155?
Created attachment 510216 [details] [diff] [review]
part 9.002: Don't attempt readback after an alpha-recovery->background switch, and fix the readback code for 'invisible' plugins, for reftests' sake (to be folded into part 9)

This patch makes me very sad, but it passes the reftests.  On my artificially resource-limited VM, what was happening on the failing tests was
 - paint once with alpha recovery, temporarily setting clipRect to window size
 - get a background, invalidate window
 - read back from front buffer to back buffer
 - /attempt/ to paint back buffer using background copying

The last step didn't paint any pixels because the plugin's clipRect was null, after being reset from the fudging for the first paint.  The readback here is just a waste of time, but not a serious bug.  I fixed that anyway.  (Oops, I forgot to address my own comment 22.)  The "real" bug was the null clipRect.  I'm not ashamed to admit that I don't understand what's going on with the clipRect here.  These changes are I think correct on their own, but really workarounds for the reftests.

This should have been failing on windows too, except that the windows nptest.dll code (AFAICT) doesn't honor the clipRect.

Most of this patch is added or modified logging code.  The "real" changes are at |if (!ReadbackDifferenceRect(rect)) {| and |if (mCurrentSurface->GetContentType() != mBackSurface->GetContentType())|.
Attachment #510216 - Flags: review?(karlt)
Created attachment 510217 [details] [diff] [review]
rollup v8

Based on 16cc18d74a8c.
Reftests are clean on tryserver.  Failing assertion is the last issue.

Bas, we also need to get a D3D10 cleanup patch reviewed and ready to go.
Comment on attachment 510216 [details] [diff] [review]
part 9.002: Don't attempt readback after an alpha-recovery->background switch, and fix the readback code for 'invisible' plugins, for reftests' sake (to be folded into part 9)

>+    NPRect realClipRect = mWindow.clipRect;
>+    bool usingTemporaryClipRect = !IsVisible() && !mHasPainted;
>+    if (usingTemporaryClipRect) {
>+        // Since we haven't painted yet, we can rely on the eventual
>+        // UpdateWindowAttributes() to deliver this fudged clipRect to
>+        // the plugin
>         mWindow.clipRect.right = mWindow.width;
>         mWindow.clipRect.bottom = mWindow.height;
>     }
> 
>     // Make expose rect not bigger than clip rect
>     nsIntRect clip(mWindow.clipRect.left, mWindow.clipRect.top,
>                    mWindow.clipRect.right - mWindow.clipRect.left,
>                    mWindow.clipRect.bottom - mWindow.clipRect.top);
>     mAccumulatedInvalidRect.IntersectRect(mAccumulatedInvalidRect, clip);
> 
>     // Clear accRect here to be able to pass
>     // test_invalidate_during_plugin_paint  test
>     nsIntRect rect = mAccumulatedInvalidRect;
>     mAccumulatedInvalidRect.Empty();
> 
>     if (!ReadbackDifferenceRect(rect)) {
>-        // Just repaint whole plugin, because we cannot read back from Shmem which is owned by another process
>+        // We couldn't read back the pixels that differ between the
>+        // current surface and last, so we have to invalidate the
>+        // entire window.
>         rect.SetRect(0, 0, mWindow.width, mWindow.height);
>+        usingTemporaryClipRect = true;
>+        mWindow.clipRect.right = mWindow.width;
>+        mWindow.clipRect.bottom = mWindow.height;
>+        // If we've already painted, we can't rely on an eventualy
>+        // implicit SetWindow() on the plugin to change the clipRect
>+        if (mHasPainted) {
>+            UpdateWindowAttributes(true);
>+        }

I'm not fully understanding the need to ShowPluginFrame when !IsVisible && mHasPainted.
If there is no need then returning early might make more sense.
Perhaps it is useful though so that the browser has a reasonable copy to paint immediately, but I don't know how many retained buffers we want to keep for invisible plugins.

If we need valid buffer contents for invisible plugins, then there is already a pre-existing bug here when ReadbackDifferenceRect succeeds because it is copying invalid pixels.  It seems that the temporarilyMakeVisible test should simply be "!IsVisible()" (independent of mHasPainted).  The clip rect intersection code should then be unnecessary.

>+    if (mCurrentSurface->GetContentType() != mBackSurface->GetContentType())
>+        return false;

Good catch here.
The mHasPainted check is necessary to solve the problem of plugins which never become visible: Flash requires that we ask them to paint once regardless, see bug 601064
Created attachment 510380 [details] [diff] [review]
Cleanup D3D10 resources upon last layer manager destruction v2

Adjusted to use a single 'DeviceAttachments' struct as suggested in review by cjones.
Attachment #510380 - Flags: review?(jmuizelaar)
(In reply to comment #166)
I follow that bug 601064 seems to require a paint (even if not a ShowPluginFrame) when !IsVisible && !mHasPainted.
But attachment 510216 [details] [diff] [review] is dealing with the !IsVisible && mHasPainted situation.
Reassigning per request.
Assignee: roc → jones.chris.g
(In reply to comment #165)
> I'm not fully understanding the need to ShowPluginFrame when !IsVisible &&
> mHasPainted.

We fail reftests otherwise ;).

> If we need valid buffer contents for invisible plugins, then there is already a
> pre-existing bug here when ReadbackDifferenceRect succeeds because it is
> copying invalid pixels.

Can you give an example in which this would lead to problems?  I can't think of one.

>  It seems that the temporarilyMakeVisible test should
> simply be "!IsVisible()" (independent of mHasPainted).  The clip rect
> intersection code should then be unnecessary.

That seems like an OK workaround for whatever the underlying bug is here.  We still need the intersection code for the IsVisible() case where there's a clip rect smaller than the window, because the test plugin asserts that the expose rectangle is within the clip.  (Unless I misunderstand something.)
(In reply to comment #170)
> We fail reftests otherwise ;).

Which reftests?  Are they checking paint count or what is painted?
(Is what they are testing what we want to test?)

> 
> > If we need valid buffer contents for invisible plugins, then there is already a
> > pre-existing bug here when ReadbackDifferenceRect succeeds because it is
> > copying invalid pixels.
> 
> Can you give an example in which this would lead to problems?  I can't think of
> one.

It would be only temporarily invalid as DoAsyncSetWindow invalidates on clipRect change.  That doesn't seem a big problem, but I don't see what is substantially different between the ReadbackDifferenceRect success and failure cases.

> We
> still need the intersection code for the IsVisible() case where there's a clip
> rect smaller than the window, because the test plugin asserts that the expose
> rectangle is within the clip.  (Unless I misunderstand something.)

AFAIK we don't manage a valid region in the buffer sent from ShowPluginFrame.
That suggests that we should keep the whole buffer valid.
If the browser is sending clip rects smaller than the window size through AsyncSetWindow, that serves no purpose and PluginInstanceChild should ignore the size, only taking information from whether it is empty or not.
(In reply to comment #171)
> (In reply to comment #170)
> > We fail reftests otherwise ;).
> 
> Which reftests?  Are they checking paint count or what is painted?
> (Is what they are testing what we want to test?)

Some of the new ones added for this bug, and plugin-alpha-opacity and plugin-alpha-zindex.  They are testing what is being painted.  The tests look just fine to my eyes.

The real problem here is that we apparently can go a long time and several plugin paints after loading an instance while the instance still has a empty clipRect, i.e. the browser has never set it to non-empty.  This I don't understand at all.

> > We
> > still need the intersection code for the IsVisible() case where there's a clip
> > rect smaller than the window, because the test plugin asserts that the expose
> > rectangle is within the clip.  (Unless I misunderstand something.)
> 
> AFAIK we don't manage a valid region in the buffer sent from ShowPluginFrame.
> That suggests that we should keep the whole buffer valid.

That's what the code is attempting to do, modulo clipping.

> If the browser is sending clip rects smaller than the window size through
> AsyncSetWindow, that serves no purpose and PluginInstanceChild should ignore
> the size, only taking information from whether it is empty or not.

In these failing tests, the browser is never sending a nonempty clip rect.
Created attachment 510487 [details] [diff] [review]
part 9.002: Don't attempt readback after an alpha-recovery->background switch, and skip painting plugins if we were just going to ask them to repaint an empty area, v2

Karl and I hypothesized that this forced first-paint was messing with the reftests' usual timing, namely starting painting while the browser thinks the plugin is "invisible", and we only happened to pass in these tests because only one paint was required.  The background copying patches were forcing more paints while still "invisible" and these were failing because of the empty clip.  This patch works on my constrained VM, so I think that we were correct.

Again, the changes here are probably OK on their own, but are still really workarounds for whatever bug is causing us to consider plugins "invisible" after reflowing them and painting them several times.
Attachment #510159 - Attachment is obsolete: true
Attachment #510216 - Attachment is obsolete: true
Attachment #510487 - Flags: review?(karlt)
Attachment #510216 - Flags: review?(karlt)
Comment on attachment 510487 [details] [diff] [review]
part 9.002: Don't attempt readback after an alpha-recovery->background switch, and skip painting plugins if we were just going to ask them to repaint an empty area, v2

Thanks cjones.  These changes look like real improvements worth making.
Attachment #510487 - Flags: review?(karlt) → review+
It's a bug in the offset calculation, as we hypothesized.

[PBS][0C477D18] Created with seqno=1
[Readback][0D34E228] Posting readback task for layer=090E9970, sink=0C477D18, offset=<-1,-1>
[Readback][0D34E228] Posting readback task for layer=090E9970, sink=0C477D18, offset=<0,0>
[Readback][09AACDF8] Finished for layer=090E9970, sink=0C477D18
  delivering BeginUpdate with offset=<0,0>, rect=<x=1,y=1, w=199,h=199>
[PBS][0C477D18] Accepted update seqno_=1->2
[InstanceParent][0EAF1820] BeginUpdateBackground for <x=1,y=1, w=199,h=199>
  (new background)
###!!! ABORT: Expecting rect for whole frame: 'aRect.TopLeft() == nsIntPoint(0, 0)', file e:/builds/moz2_slave/try-w32-dbg/build/dom/plugins/PluginInstanceParent.cpp, line 642

The update rect should have been offset by <-1,-1>, or not at all.  Pondering the best/simplest fix.
I pushed off a try build with a patch to save away the background offset when ReadbackTasks are created.  I'm seeing occasional local failures on plugin-background-2-step, but they appear to be caused by the plugin not painting at all.  That's likely a bug in the reftest harness, => random-if().
Created attachment 510529 [details] [diff] [review]
part 2.001: Don't shadow ReadbackLayers

To be folded into part 2.
Attachment #510529 - Flags: review?(roc)
Attachment #510529 - Flags: review?(roc) → review+
Windows debug reftest didn't crash, but had two failures in the new tests.  One was from the plugin not painting and the other I'm not sure about.  But, random-if(/^Windows\x20NT\x206\.1/.test(http.oscpu)) and let's land this tomorrow.
Blocks: 627469
(In reply to comment #178)
> Windows debug reftest didn't crash, but had two failures in the new tests.  One
> was from the plugin not painting and the other I'm not sure about.  But,
> random-if(/^Windows\x20NT\x206\.1/.test(http.oscpu)) and let's land this
> tomorrow.

If random-if(d2d) going to be enough maybe?
Comment on attachment 510380 [details] [diff] [review]
Cleanup D3D10 resources upon last layer manager destruction v2

> 
>+struct DeviceAttachments
>+{
>+  nsRefPtr<ID3D10Effect> mEffect;
>+  nsRefPtr<ID3D10InputLayout> mInputLayout;
>+  nsRefPtr<ID3D10Buffer> mVertexBuffer;
>+  nsRefPtr<ReadbackManagerD3D10> mReadbackManager;
>+};
>+
> LayerManagerD3D10::~LayerManagerD3D10()
> {
>+  if (mDevice) {
>+    UINT size = sizeof(int);
>+    int count = 0;

This is a reference count right? I think it's worth renaming the variable reference count. It also might be worth adding a bit of documentation about how the device attachments stuff is intended to work. I'd suggest putting this near the DeviceAttachments structure.

>+    HRESULT hr = mDevice->GetPrivateData(sLayerManagerCount, &size, &count);

You get the error result but don't check it. I think you should either check the return value or ignore it.



>     mNv3DVUtils->SetDeviceInfo(devUnknown);
>   }
> 
>-  UINT size = sizeof(ID3D10Effect*);
>-  if (FAILED(mDevice->GetPrivateData(sEffect, &size, mEffect.StartAssignment()))) {
>+  UINT size = sizeof(int);
>+  int count = 0;

How about swapping these.

int referenceCount;
UINT size = sizeof(referenceCount);

>+  // If this isn't there yet it'll fail, count will remain 0, which is correct.
>+  mDevice->GetPrivateData(sLayerManagerCount, &size, &count);
>+  count++;
>+  size = sizeof(int);
>+  mDevice->SetPrivateData(sLayerManagerCount, size, &count);

mDevice->SetPrivateData(sLayerManagerCount, sizeof(count), &count);

>+
>+  DeviceAttachments *attachments;
>+  size = sizeof(DeviceAttachments*);

sizeof(attachements); etc.


>   }
> 
>-  UINT size = sizeof(ReadbackManagerD3D10*);
>-  if (FAILED(mDevice->GetPrivateData(sReadbackManager, &size, mReadbackManager.StartAssignment()))) {
>+  DeviceAttachments *attachments;
>+  UINT size = sizeof(DeviceAttachments*);
>+  if (FAILED(mDevice->GetPrivateData(sDeviceAttachments, &size, &attachments))) {
>+    // Strange! This shouldn't happen ... return a readback manager for this
>+    // layer manager only.

Perhaps use gfx::LogFailure?
Attachment #510380 - Flags: review?(jmuizelaar) → review-
Created attachment 510664 [details] [diff] [review]
Cleanup D3D10 resources upon last layer manager destruction v3

Updated to address review comments.

I wasn't able to compile test this due to the state my patch queue is currently in :) (i.e. it doesn't apply on top of the last rollup but it doesn't compile without a bunch of the async plugin stuff).
Attachment #510380 - Attachment is obsolete: true
Attachment #510664 - Flags: review?(jmuizelaar)
Attachment #510664 - Flags: review?(jmuizelaar) → review+
Created attachment 510684 [details] [diff] [review]
part 6.00001: Remember the background layer's offset when creating a readback task

This is the simple fix we discussed.  OK on tryserver.
Attachment #510684 - Flags: review?
Attachment #510684 - Flags: review? → review?(bas.schouten)
Attachment #510684 - Flags: review?(bas.schouten) → review+
http://hg.mozilla.org/mozilla-central/rev/0fc46487e027
http://hg.mozilla.org/mozilla-central/rev/c7f068259a8a
http://hg.mozilla.org/mozilla-central/rev/5300b2ae26ad
http://hg.mozilla.org/mozilla-central/rev/f47bc2d59e52
http://hg.mozilla.org/mozilla-central/rev/a4cc7836fc3c
http://hg.mozilla.org/mozilla-central/rev/d9d905c76a3e
http://hg.mozilla.org/mozilla-central/rev/9bb82d1b62a8
http://hg.mozilla.org/mozilla-central/rev/580f6ff0b824
http://hg.mozilla.org/mozilla-central/rev/fd046a4bbb19
http://hg.mozilla.org/mozilla-central/rev/9d3141affbe5
http://hg.mozilla.org/mozilla-central/rev/a96c75dbc151
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Oops, also

http://hg.mozilla.org/mozilla-central/rev/a75cef6e752a

Comment 185

7 years ago
Failed to build with --disable-ipc.

nsObjectFrame.cpp line 242: Error: plugins is not a member of mozilla.

Either nsNPAPIPlugin.h should be included or "using namespace mozilla::plugins;" should be removed or quoted with #ifdef MOZ_IPC
Depends on: 632696

Comment 186

7 years ago
I can see the same compile error on thunderbird.

8 are burning now.
http://tinderbox.mozilla.org/showbuilds.cgi?tree=Thunderbird
(In reply to comment #186)
> I can see the same compile error on thunderbird.
> 
> 8 are burning now.
> http://tinderbox.mozilla.org/showbuilds.cgi?tree=Thunderbird

I think this was bug 593372, which was backed out.

Comment 188

7 years ago
(In reply to comment #187)
> I think this was bug 593372, which was backed out.

Thank you for letting me know. However They are still burning. :-(

Linux x86-64 comm-central build %
rev:7bda34f6086b7296819e0709398fde0dd0292cee
moz:c8d984f57dbd
(In reply to comment #188)
> (In reply to comment #187)
> > I think this was bug 593372, which was backed out.
> 
> Thank you for letting me know. However They are still burning. :-(
> 
> Linux x86-64 comm-central build %
> rev:7bda34f6086b7296819e0709398fde0dd0292cee
> moz:c8d984f57dbd

http://build.mozillamessaging.com/tinderboxpushlog/

It's a mixture, I've stared the builds with known failures.
I'm not going to be around long enough for the next windows tp4 runs, but if they're still OOM-y we need to back this out and investigate more.
I just landed a non-IPC bustage fix:

http://hg.mozilla.org/mozilla-central/rev/283fe70e1eb5

The using namespace mozilla::plugins statement wasn't actually required. I suspect that IPC builds somehow include plugin header files into that file.
(In reply to comment #191)
> I just landed a non-IPC bustage fix:
> 
> http://hg.mozilla.org/mozilla-central/rev/283fe70e1eb5
> 
> The using namespace mozilla::plugins statement wasn't actually required. I
> suspect that IPC builds somehow include plugin header files into that file.

And because I messed up, it was actually required on Windows just my Mac build didn't show it, so fixed with an ifdef instead:

http://hg.mozilla.org/mozilla-central/rev/280e978fc6fb
Created attachment 511025 [details] [diff] [review]
backout of 5300b2ae26ad

The changeset 5300b2ae26ad has broke fennec (see the video on bug 632783). The attachment is a backout of 5300b2ae26ad in order to have a working fennec (but panning seems very slow now)

Sadly the Firefox tree is closed right now so I can't land that.
Just to add some details: Remote (e10s) web page rendering doesn't seem to happen in Fennec anymore. The thumbnails of the remote pages _are_ rendered, but the main content area is not.

Could be an issue with ShadowLayers? The thumbnails just use asyncDrawXULElement
Backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Duplicate of this bug: 632696
Depends on: 632783

Updated

7 years ago
Whiteboard: [hardblocker][has patch] → [hardblocker]
Bug 632871 looks like an instance of the nightly build crashing in the wild, in the same way as the Windows Tp4 failure here.
Yes.
I backed out all the changesets that were pushed in this bug included the non-ipc ones.
Version: unspecified → Trunk
Thanks Marco.

The tp4 problems are a real head-scratcher.  These patches have us use more memory for plugins in every situation on windows, but in the usual case, 1.5x more per instance.  There are edge cases where we could send a few new background updates in succession and use more than 1.5x for a short period of time.

Another possibility is that we're leaking in a way our leak checkers aren't catching, but I haven't seen this while monitoring memory consumption.

I also looked back over some of the later stacks from 632696 and some didn't look quite so OOM-y.  There's a good chance there are other bugs lurking here.

(BTW, the process failure in landing this bug was me not requesting talos runs on these patches before pushing.)
To summarize comment 200, there are two questions we need to answer locally
 - are we leaking backgrounds or back/front buffers?
 - are we crashing from non-OOM bugs?

I haven't been able to observe leaking.  If someone else can look into that, that'd be great.  I'll try finding some ways to push this code harder to expose non-OOM bugs.  It wouldn't surprise me if they're lurking.

With those two issues resolved, we need to fire off more tp4 runs and see if they're still crashy, and go on from there.
Depends on: 632871
Created attachment 511147 [details] [diff] [review]
rollup v9

Includes followup patch to check allocation of the whiteImage in the alpha recovery code.
Attachment #510217 - Attachment is obsolete: true
How often are we allocating these images? At least on Windows XP we know of problems where constantly allocating or attaching DIBSections at video framerates will fragment virtual memory (or use handles, or something) and make the browser unusable. Are we reallocating the various buffers (background and white image) on every repaint, or only when the size changes? Note how I had to fix bug 625425.
No, these patches don't do anything like that (I kept that bug in mind! ;) ).  If we're forced to use alpha recovery, we allocate a temporary whiteImage on every repaint, but those are gfxImageSurfaces, which are malloc()'d.
I'm able to reproduce a persistent rendering artifact on https://bugzilla.mozilla.org/attachment.cgi?id=508570 by holding down Ctrl-+ and then Ctrl-- alternately, then holding down Ctrl-- until the page is zoomed out as far as it will go.  This looks somewhat like the plugin's ImageLayer having a surface larger than layout/layers expects, then drawing outside its bounds but the surrounding thebes content not invalidating.  That is, "bleeding out" into the surrounding thebes content.

(I can also force transient rendering artifacts that are expected due to not double buffering of the background, not an issue.)
I'm pretty sure I've got the problems sorted out, but my XP VM decided to do a reconfigure, so I have some free time to note the problems.  I reproduced all of these pretty easily with the site in bug 632871.
 - we were keeping too big of an invalidated region when the frame shrank.  There were several bugs in computing the invalidated region, but I just added code to intersect the window rect with the invalidated rect.
 - there was a stupid typo in the code that nudges rects for alpha-recovery
 - we were calling SetWindow() on flash in ShowPluginFrame(), but flash was making RPCs to the parent process, which then allowed background-update messages to re-enter.  On resizes those cleared the current back buffer, and things went boom when we tried to use it.

The good news is that I have all this code in my head now (I think).  The bad news is that the fix for the third problem above spreads complicated invariants around the code even more.  I know how to rewrite the code now, but I don't want to add any risk to this bug.  I might try after another thing I have to do.

Comment 207

7 years ago
Here's another crash [@ _moz_pixman_image_set_transform ] probably caused by the patch:

https://crash-stats.mozilla.com/report/index/bp-3d24ce9e-f397-48a9-b0e7-2fdf02110210
I've got the windows issues sorted out (other than the glitch listed in comment 205).  But, on first run on my linux desktop, "!!! ABORT: X_ShmPutImage: BadMatch".  Sigh.
Blocks: 633387
I'm going to call

-      if (aLayer->GetType() == Layer::TYPE_THEBES) {
+      if (aLayer->AsThebesLayer()) {

a typo fix and not request review.  That fixes shadow layers.  (And, the wheels to getting reftest-ipc on tryserver are now in motion.)
Created attachment 511612 [details] [diff] [review]
part 9.003: Invalidations can accumulate for old window sizes, so just intersect them with the current window size before painting
Attachment #511612 - Flags: review?(karlt)
Created attachment 511617 [details] [diff] [review]
part 9.004: Add some hacks to avoid blowing up on re-entry and/or using the wrong surface type

This patch makes me sad.

The fundamental problem that cropped up was ShowPluginFrame() calling PaintRect*() getting into PaintPlatformRect() getting into UpdateWindowAttributes() and calling SetWindow() on the plugin.  When flash decided to RPC back into the browser off of that, sometimes we had new background/background destroy re-enter.  These sometimes need to clear the current surface to force a resize or choice of new content type.  But, we can't do that from the nested context, or we destroy the surface we're painting to.  Oops!  So, the hack to fix that is to use AsyncSetWindow() to hopefully clear the surface eventually, from a safe context.  I didn't want to try to make AsyncSetWindow() responsible for clearing the surface on background changes (not exactly sure how that would work), so I just added code to do that before we paint on the background.  It's a mess.  I file bug 633387 on cleaning it up.
Attachment #511617 - Flags: review?(karlt)
Rob, layout/base/tests/test_flush_on_paint.html is failing on tryserver (on linux), e.g. http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1297399606.1297404170.22770.gz .  Are these failures something urgent or should we temporarily disable the test?

Given that the test was passing before part 9.004, I think the problem is in that patch's preferring to use an existing background's size for the plugin's surface size, instead of the most recent SetWindow() size.  If that's so, I think these failures are evidence of the background lagging behind SetWindow().  (Or something.)  The reason the patch does that is painting to the background is always faster than alpha recovery, so it's better to wait for background SetUnknown()s than to waste intermediate paints doing alpha recovery.  That could be changed though.

I'll take a look in the meantime.
(If it matters, that would be a lot easier to change/fix with bug 633387.)
On my desktop, I can 100%-reliably reproduce the mochitest failure with part 9.004 and bug 631388 applied.  I can't reproduce the failure with just part 9.004 and not bug 631388.  I *also* can't reproduce the failure with just bug 631388 and not part 9.004.

I have no idea why this is, but that's a problem for bug 631388! ;)
Attachment #511612 - Flags: review?(karlt) → review+
Depends on: 633463

Updated

7 years ago
Depends on: 633507

Updated

7 years ago
No longer depends on: 633463
Here's the hurt from talos
 - ~10% regression in talos's working set measurement (tp4_memset).  Not terribly surprising since we have to touch more memory when painting.
 - possible ~3-10% hit on tp4 on windows.

The tp4 regression hurts, but I don't really know of anything we can "fix" in these patches.  I'll probably need to get another run, will see how noisy those numbers are.
I'm very surprised that this affects Tp4 in any measurable way... is it simply the readback, even though it is async?
I don't know.  The readback thread might be competing with the main thread for GPU time.
It's probably also worth noting that on the first landing, I saw crashes due to OOM-looking conditions, which is pretty ridiculous on the 2GB slaves.  It's quite possible we've got GC params etc. tuned to the breaking point, and creating these backgrounds is the straw breaking physical memory's back.  Somewhat awkward to test though.
It's also entirely possible that the tp4 regression is from us now using alpha recovery on the forced first-paint of invisible plugins.  That's fairly easy to hack around.
Yeah, we should avoid that. How about if the plugin is going to be visible, we don't paint it until we receive the first background. If it's not going to be visible, paint it but don't save the data or do any alpha recovery.
You have been watching #gfx! ;)

AFAIK there's no way in the current code to know if the plugin is going to visible or not, otherwise we could use that knowledge to skip the phony forced paint for eventually-to-be-visible plugins.
Hrm... we should just say "the browser must send an asyncsetwindow soon after startup". I'm sure the *browser* knows whether the plugin will be visible.
Yes, I agree.  I think that's worth fixing in another bug.
Actually, don't we do that already? We should just not do the first-paint until we receive the first AsyncSetWindow, and then we'll know.
(Don't want to sound too fatalistic or set off any alarm bells, but it wouldn't be a bad idea for someone to check how well mozilla.plugins.use_layers=false is working on trunk, in parallel.)
Another potential culprit for the tp4 regression is reading back the entire thebeslayer texture instead of just the subrect behind the plugin.  That might explain why the hit was higher on windows 7 than on XP.  This is an easy-to-test hypothesis.

Bas, how hard would it be to just read back the part of texture we need?  (If we need to do it.)
Created attachment 511896 [details] [diff] [review]
part 10: Don't send the results of the force-paint to the browser, and don't use alpha recovery for it

This fixes the first candidate for the tp4 hit.  I think we should take this anyway though.

After dinner, I'm going to fire off a tp4 run with readback disabled for d3d10 and see what the effect is, the second candidate.  It's also worth seeing how often we need to use alpha recovery in the tp4 pages, the third candidate.
Attachment #511896 - Flags: review?(roc)
Attachment #511896 - Flags: review?(roc) → review+
Comment on attachment 511617 [details] [diff] [review]
part 9.004: Add some hacks to avoid blowing up on re-entry and/or using the wrong surface type

>     if (mWindow.width != aWindow.width || mWindow.height != aWindow.height) {
>-        // We weakly assume here that the SetWindow arrives before the
>-        // next UpdateBackground, for the new window size, if we were
>-        // going to get one.
>-        mBackground = nsnull;
>+        if (mBackground) {
>+            gfxIntSize bgSize = mBackground->GetSize();
>+            if (bgSize.width != aWindow.width || bgSize.height != aWindow.height) {
>+                // We weakly assume here that the SetWindow arrives
>+                // before the next UpdateBackground, for the new
>+                // window size, if we were going to get one.
>+                mBackground = nsnull;

Does making this change remove the assumption described here?

>+        window.x = mWindow.x;
>+        window.y = mWindow.y;
>+        // Prefer the background size to whatever SetWindow last told
>+        // us.  They should catch up eventually.
>+        window.width = bgSize.width;
>+        window.height = bgSize.height;
>+        window.clipRect = mWindow.clipRect;

I think preferring bgSize is OK because it uses the most recent indicator of
the window size, and so we never overwrite the good size.  If it were to use
the current mWindow it would revert a scheduled size from RecvAsyncSetWindow.

However, I think the other values need to be set from those in the
mCurrentAsyncSetWindowTask, if it is scheduled (or use some other means so as
not to revert them).

>+        mAccumulatedInvalidRect = aRect;

I gather this is assuming aRect is the full window, but would it be clearer to
use bgSize?
Attachment #511617 - Flags: review?(karlt) → review-
I started instrumenting talos to record background paints vs. alpha recovery, but ran into this bug on several pages

###!!! ABORT: Only transparent plugins use backgrounds: 'mIsTransparent', file /home/cjones/mozilla/mozilla-central/dom/plugins/PluginInstanceChild.cpp, line 3151

We're sending a background to opaque plugins!  Whups, that's going to hurt tp4.  With the patch below,

diff --git a/layout/generic/nsObjectFrame.cpp b/layout/generic/nsObjectFrame.cpp
--- a/layout/generic/nsObjectFrame.cpp
+++ b/layout/generic/nsObjectFrame.cpp
@@ -2083,16 +2083,18 @@ nsObjectFrame::BuildLayer(nsDisplayListB
     ImageLayer* imglayer = static_cast<ImageLayer*>(layer.get());
     imglayer->SetContainer(container);
     imglayer->SetFilter(nsLayoutUtils::GetGraphicsFilterForFrame(this));
 
     layer->SetContentFlags(IsOpaque() ? Layer::CONTENT_OPAQUE : 0);
   } else {
     NS_ASSERTION(aItem->GetType() == nsDisplayItem::TYPE_PLUGIN_READBACK,
                  "Unknown item type");
+    NS_ABORT_IF_FALSE(!IsOpaque(), "Opaque plugins don't use backgrounds");
+
     if (!layer) {
       layer = aManager->CreateReadbackLayer();
       if (!layer)
         return nsnull;
     }
     NS_ASSERTION(layer->GetType() == Layer::TYPE_READBACK, "Bad layer type");
 
     ReadbackLayer* readback = static_cast<ReadbackLayer*>(layer.get());

the assertion fails a little closer to home at

###!!! ABORT: Opaque plugins don't use backgrounds: '!IsOpaque()', file /home/cjones/mozilla/mozilla-central/layout/generic/nsObjectFrame.cpp, line 2091

In the "before optimization" display list before a failed assertion, I see

                                PluginReadback 0x33272d0(ObjectFrame(embed)(0)) (600,11100,33000,6900)(0,0,0,0)
                                Plugin 0x33272d0(ObjectFrame(embed)(0)) (600,11100,33000,6900)(0,0,0,0) opaque

in which it appears to me that the PluginReadback ought to be culled.  I don't know why that's not happening.

I'm going ahead with the instrumentation with these assertions turned off, since this code is pretty far outside of my wheelhouse.  If someone wants to grab this in the meantime, please do.
Good news (finally!): with the latest patches, on tp4 we're back within noise on win7, and down to a 4.7% regression on XP.  The fix for sending a background when we don't need to should hopefully make that go away.
The tp4 results from a separate push that disables actual readback (instead drawing a solid color) shows a ~2.3% decrease in tp4 as compared to actually reading back.  That's approximately the same increase for reading back compared to the m-c rev I based the push on, and within noise for the last few revs before that.  So I think we're good readback-wise.
Here's what happens during tp4 on my desktop (the results are nondeterministic)

  Plugin instance painting stats:
      Painted transparent plugin on background 202 times
      Painted transparent plugin using alpha recovery 17 times
      Painted opaque plugin 159 times
      Phony-painted 93 times
With a bit more instrumentation

  Plugin instance painting stats:
      Painted transparent plugin on background 240 times
        sent 346 background updates, wasted 116 of them
      Painted transparent plugin using alpha recovery 20 times
      Painted opaque plugin 155 times
      Phony-painted 95 times

Whups!  ("Wasted" background updates are the ones going to opaque instances.)
Created attachment 511928 [details] [diff] [review]
part 3.001: Don't rely on display list optimization to cull plugin readback items for opaque plugins

This is the dumb fix.  Not sure whether you'd prefer to fix the display list optimization code, or if that's even possible.
Attachment #511928 - Flags: review?(roc)
(In reply to comment #228)
> Comment on attachment 511617 [details] [diff] [review]
> part 9.004: Add some hacks to avoid blowing up on re-entry and/or using the
> wrong surface type
> 
> >     if (mWindow.width != aWindow.width || mWindow.height != aWindow.height) {
> >-        // We weakly assume here that the SetWindow arrives before the
> >-        // next UpdateBackground, for the new window size, if we were
> >-        // going to get one.
> >-        mBackground = nsnull;
> >+        if (mBackground) {
> >+            gfxIntSize bgSize = mBackground->GetSize();
> >+            if (bgSize.width != aWindow.width || bgSize.height != aWindow.height) {
> >+                // We weakly assume here that the SetWindow arrives
> >+                // before the next UpdateBackground, for the new
> >+                // window size, if we were going to get one.
> >+                mBackground = nsnull;
> 
> Does making this change remove the assumption described here?

Not really.  We can still accidentally throw out the background, say if we get AsyncSetWindow([smaller window]), then AsyncSetWindow([original window]).  The intermediate [smaller window] state might never be visible to the code that forwards the background.

> >+        window.x = mWindow.x;
> >+        window.y = mWindow.y;
> >+        // Prefer the background size to whatever SetWindow last told
> >+        // us.  They should catch up eventually.
> >+        window.width = bgSize.width;
> >+        window.height = bgSize.height;
> >+        window.clipRect = mWindow.clipRect;
> 
> I think preferring bgSize is OK because it uses the most recent indicator of
> the window size, and so we never overwrite the good size.  If it were to use
> the current mWindow it would revert a scheduled size from RecvAsyncSetWindow.
> 
> However, I think the other values need to be set from those in the
> mCurrentAsyncSetWindowTask, if it is scheduled (or use some other means so as
> not to revert them).

We could pile another hack on top of this hack and set to -1 the values we want to be overridden by whatever is in the current window when the DoAsyncSetWindow() is processed.

> 
> >+        mAccumulatedInvalidRect = aRect;
> 
> I gather this is assuming aRect is the full window, but would it be clearer to
> use bgSize?

It's assuming is that aRect is exactly <0, 0, bgSize.width, bgSize.height>, because that's an invariant maintained by PluginInstanceParent.  I could add another assertion here and use the bgSize.  I see why that would make for easier reading.
(In reply to comment #235)
> We could pile another hack on top of this hack and set to -1 the values we want
> to be overridden by whatever is in the current window when the
> DoAsyncSetWindow() is processed.

That's simpler than my suggestion, so ... yes, why not.

Thanks for the explanation on the other questions too.
Created attachment 511942 [details] [diff] [review]
part 9.004: Add some hacks to avoid blowing up on re-entry and/or using the wrong surface type, v2

Hopefully addressed review comments.
Attachment #511617 - Attachment is obsolete: true
Attachment #511942 - Flags: review?(karlt)
Comment on attachment 511928 [details] [diff] [review]
part 3.001: Don't rely on display list optimization to cull plugin readback items for opaque plugins

This is definitely right; creating the display item and then culling it is a waste of time, even if it worked.
Attachment #511928 - Flags: review?(roc) → review+
Talos showed a worse tp4 regression on win7 with the patch to *not* send a background to opaque plugins (i.e., 1/3 of the times we were doing that previously), as compared to sending it.  That makes no sense.  XP was better, and tp4_memset did reduce by about 1/3.  I fired off another run with part 9.004 v2 to double check.

If that result on win7 is nonsense, then it appears that we're looking at
 - ~2-3% regression on win7 from readback
 - ~3% regression on XP

We have an increase in working set size that seems to correlate with the amount of readback we do.  That's not unexpected.

It looks like we might be trading that for fixing flash rendering glitches on windows and getting a huge perf boost on X11 systems.  I can live with that.
The last talos run showed a ~1% regression on XP, which might be within noise. \o/

But, it also showed a ~14% regression on win7, the highest yet, which is just insane.  Less readback regresses compared to more readback.  I suspect there's something fishy or subtle going on, like hitting messy GPU or process scheduling effects.  There's nothing left to be optimized in the plugin or layout code.

I'll try one more win7 run to see how noisy these numbers are.

In the meantime though, Bas or Rob, can one of you try profiling a tp4 run with d3d10?
Created attachment 512004 [details] [diff] [review]
part 9.004: Add some hacks to avoid blowing up on re-entry and/or using the wrong surface type, v3

I realized that v2 could result in us throwing out a background in some cases that we wouldn't in v1, from using a cliprect that mismatches the background.  Changed that in this patch.

Fired off another talos run with that tweak.
Attachment #511942 - Attachment is obsolete: true
Attachment #512004 - Flags: review?(karlt)
Attachment #511942 - Flags: review?(karlt)
Comment on attachment 512004 [details] [diff] [review]
part 9.004: Add some hacks to avoid blowing up on re-entry and/or using the wrong surface type, v3

>+        window.clipRect.right = bgSize.width;
>+        window.clipRect.bottom = bgSize.height;
>+        window.type = mWindow.type;
>+
>+        nsIntRect bgRect(0, 0, bgSize.width, bgSize.height);
>+        NS_ASSERTION(bgRect == aRect,
>+                     "PluginInstanceParent maintains this invariant");
>+        mAccumulatedInvalidRect.UnionRect(bgRect, mAccumulatedInvalidRect);
>+        AsyncSetWindow(mSurfaceType, window,
>+                       // Fill in the top-left with whatever was most
>+                       // recently set when this task is processed.
>+                       PLACEHOLDER_XY);

The assumption here is that we only get an UpdateBackground when the plugin is
visible.  Is that reasonable?
Attachment #512004 - Flags: review?(karlt) → review+
(In reply to comment #242)
> The assumption here is that we only get an UpdateBackground when the plugin is
> visible.  Is that reasonable?

Well, depends on who's idea of visibility you're asking about.  We definitely only get background updates for plugins that layout thinks are visible.  In fact, with the current system, we can *only* get backgrounds for fully-visible plugins.  But, layout's idea of visibility can lag behind the plugin code's idea.

You're probably thinking of the following problem this code is vulnerable to in theory

  (0) (Plugin is visible and has a background)
  (1) RecvAsyncSetWindow([invisible window]) - enqueue SetWindow() for invisible window
  (2) RecvUpdateBackground() - enqueue SetWindow() for background size
  (3) Process first SetWindow(), set window to invisible
  (4) Process second SetWindow(), make window visible again

The later DestroyBackground() message that must follow would use PLACEHOLDER_CLIPRECT and so wouldn't be able to make the window invisible again.

Nothing "bad" would happen in that case, we would just allow off-screen plugins to continue painting themselves until visibility state changed again.  I said "in theory" above because I'm not sure if it could happen with the current code.  But even if it can, I think it's enough of an edge case that it's not worth worrying about.  It's also basically impossible to fix without bug 633387.
Comment on attachment 512004 [details] [diff] [review]
part 9.004: Add some hacks to avoid blowing up on re-entry and/or using the wrong surface type, v3

Sorry, I should have realized this before making comment 236, but there is still the problem that some AsyncSetWindow values can get lost when mCurrentAsyncSetWindowTask is cancelled.

(In reply to comment #243)
>   (0) (Plugin is visible and has a background)
>   (1) RecvAsyncSetWindow([invisible window]) - enqueue SetWindow() for
> invisible window
>   (2) RecvUpdateBackground() - enqueue SetWindow() for background size

Yes.  It would be nice if the browser cancelled the UpdateBackground when the plugin becomes invisible, but I don't know whether that happens.

>   (3) Process first SetWindow(), set window to invisible
>   (4) Process second SetWindow(), make window visible again

If the RecvUpdateBackground() used PLACEHOLDER_CLIPRECT then this wouldn't be a problem.
However, I didn't follow comment 241 so don't know the reason for removing that.

Really all the browser is telling us with the clipRect is the visibility, and we could safely use the full window size (irrespective of the latest size received) if the plugin is visible.
Attachment #512004 - Flags: review+ → review-
I saw a crash in tp4 on win7 in the first push to talos, but it didn't give me a stack and I can't reproduce locally.  Not sure what to do that one atm.

But, on the second run with the latest patches, we have a ~1% regression on XP, and a ~3% *improvement* on win7.  \o/  I say we land.  Anyone disagree?
(That is, land once the 9.004 issues are sorted out.)  I'm not around today, so that's going to have to happen tomorrow.
(In reply to comment #245)
> But, on the second run with the latest patches, we have a ~1% regression on XP,
> and a ~3% *improvement* on win7.  \o/  I say we land.  Anyone disagree?

Sounds good to me!
I realized there's a bug in nsObjectFrame, in that it uses the plugin's most recent image to determine display item size.  We don't want to do that because if the plugin temporarily painted to an old background for perf reasons (knowing that the background was old), then if layout uses that image size instead of whatever layout calculated, it can accidentally reinforce that temporary size and possibly never fix it.  That's a small patch but might have perf implications :/.

(In reply to comment #244)
> However, I didn't follow comment 241 so don't know the reason for removing
> that.
> 
> Really all the browser is telling us with the clipRect is the visibility, and
> we could safely use the full window size (irrespective of the latest size
> received) if the plugin is visible.

We can fix the case of accidentally stomping the cliprect invisible->visible by adding another placeholder flag, say PLACEHOLDER_VISIBILITY.  Then UpdateBackground() would set the phony cliprect size regardless but use PLACEHOLDER_VISIBILITY, but in DoAsyncSetWindow(), if flags & PLACEHOLDER_VISIBILITY, the code would check whether setting the cliprect would stomp the visibility setting and skip if so.  How does that sound?

(Sadly, I already have this fixed in bug 633387.  That work is hitting a bit of a speedbump in the form of maemo cruft.)
Created attachment 512262 [details] [diff] [review]
part 3.001: Don't use the plugin's image to determine visible rect
Attachment #512262 - Flags: review?(roc)
Created attachment 512263 [details] [diff] [review]
part 9.004: Add some hacks to avoid blowing up on re-entry and/or using the wrong surface type, v4

Adds PLACEHOLDER_VISIBILITY per discussion.
Attachment #512004 - Attachment is obsolete: true
Attachment #512263 - Flags: review?(karlt)
(In reply to comment #248)
> I realized there's a bug in nsObjectFrame, in that it uses the plugin's most
> recent image to determine display item size.  We don't want to do that because
> if the plugin temporarily painted to an old background for perf reasons
> (knowing that the background was old), then if layout uses that image size
> instead of whatever layout calculated, it can accidentally reinforce that
> temporary size and possibly never fix it.  That's a small patch but might have
> perf implications :/.

Seems to me we should size the nsDisplayPluginReadback item based on the true desired size, and the nsDisplayPlugin item based on the surface we got from the plugin.
Comment on attachment 512262 [details] [diff] [review]
part 3.001: Don't use the plugin's image to determine visible rect

If we do this, we shouldn't ever mark the display item as opaque, since it might not paint its entire bounds. I recommend using the image size in nsDisplayPlugin but not nsDisplayPluginReadback.
If I understand things correctly, the existing code is already broken, since it may not paint its bounds either.  Sigh.

How about this solution --- add IsWmodeTransparent(), and change IsOpaque() to IsOpaqueFor(const nsRect& aRect), which returns true if !IsWmodeTransparent() and the current image covers aRect.  (Except on mac, which has a special case there I don't understand.)
(Oops, also needs to return true for wmode=transparent but have a background.)
Yes, that sounds right.
There's not a way to implement that, unfortunately --- we can't ask CairoImages for their content type, and we can't otherwise determine if the current image was painted on a background.  Not quite sure what to do here.  Will scratch my head for a bit and fix up the other patch.
Created attachment 512405 [details] [diff] [review]
part 9.004: Add some hacks to avoid blowing up on re-entry and/or using the wrong surface type, v5

Updated to not attempt to cancel SetWindows.  Also moved to using a "task factory" that cancels pending tasks when the factory goes out of scope, instead of tracking them manually.
Attachment #512263 - Attachment is obsolete: true
Attachment #512405 - Flags: review?(karlt)
Attachment #512263 - Flags: review?(karlt)
I'm confused, why do you need to ask a CairoImage for its content type?
It's not important anymore, I've got another fix for the visibility problem.  Testing now.
Created attachment 512411 [details] [diff] [review]
part 3.002: Use the computed frame size instead of image size when we need to update plugin geometry
Attachment #512262 - Attachment is obsolete: true
Attachment #512411 - Flags: review?(roc)
Attachment #512262 - Flags: review?(roc)
I should have noted that the IsWmodeTransparent() change isn't strictly needed to fix a bug, just semantically wmode=transparent and opaqueness are different.
Comment on attachment 512411 [details] [diff] [review]
part 3.002: Use the computed frame size instead of image size when we need to update plugin geometry

IsWmodeTransparent should be called IsTransparentMode. "Wmode" is a Flash invention.

GetComputedDisplayItemBounds should be GetDesiredDisplayItemBounds ... everything's computed around here :-).

GetRealDisplayItemBounds should be GetDrawnDisplayItemBounds ... everything's real around here too :-).
Attachment #512411 - Flags: review?(roc) → review+
The latest numbers compared to http://hg.mozilla.org/rev/9b4c5cfb3fc6 are still showing ~1% tp4 hit on XP.  That's been pretty stable.  The hit was ~8% on win7, which has varied from 3% win to 14% regression.  Looks like we'll have a real regression here, but it's hard to say how much from these sporadic runs.  I think landing this is still the best of the options we have.  (There's also still the not unexpected tp4_memset increase.)
(In reply to comment #262)
> Comment on attachment 512411 [details] [diff] [review]
> part 3.002: Use the computed frame size instead of image size when we need to
> update plugin geometry
> 
> IsWmodeTransparent should be called IsTransparentMode. "Wmode" is a Flash
> invention.
> 
> GetComputedDisplayItemBounds should be GetDesiredDisplayItemBounds ...
> everything's computed around here :-).
> 
> GetRealDisplayItemBounds should be GetDrawnDisplayItemBounds ... everything's
> real around here too :-).

Heh, sure.  0/3, new record ;).
I did another push of the latest patches with d3d10 readback disabled, and the 8% loss on tp4 on win7 turned into a 3% win.  (The hack patch didn't affect XP, and it was still at a ~1% loss.)

So in all probability, readback on win7 is making the tp4 numbers noisier, but there's a real regression in there.  We can look at optimizing d3d10 readback in a followup, and such a followup wouldn't block betaN IMHO.
Created attachment 512706 [details] [diff] [review]
part 9.004: Add some hacks to avoid blowing up on re-entry and/or using the wrong surface type

Fixed the following issues we discussed on IRC
 - missed ClearCurrentSurface() for background->!background transition without an accompanying window-size change
 - removed unused isAsync argument
 - pointless mInvalidatePending check
 - fixed inaccurate comment
 - added a "surface type" placeholder to avoid accidentally stomping that
Attachment #512405 - Attachment is obsolete: true
Attachment #512706 - Flags: review?(karlt)
Attachment #512405 - Flags: review?(karlt)
Created attachment 512733 [details] [diff] [review]
part 9.004: Avoid blowing up on re-entry and/or using the wrong surface type

Main change is to move the ClearCurrentSurface() responsibility to EnsureCurrentBuffer() to simplify the methods that might be called in below ShowPluginFrame() on reentrancy.  (The exception is clearing the surface upon becoming invisible.)  This also adds a couple of guards that we discussed on IRC.

The remainder of the changes were sending the current-background descriptor on every update since it's free for Xlib and shmem surfaces.  This can help regain a background if we have to toss it out from an ill-timed setwindow.
Attachment #512706 - Attachment is obsolete: true
Attachment #512733 - Flags: review?(karlt)
Attachment #512706 - Flags: review?(karlt)
Comment on attachment 512733 [details] [diff] [review]
part 9.004: Avoid blowing up on re-entry and/or using the wrong surface type

This looks much tidier, thanks.

>+    // If we were going to use layers rendering but it's not set up
>+    // yet, and the plugin happens to call this first, we'll
>+    // harmlessly forward the invalidation to the browser.

For some definition of "harmless".  It may make the browser paint unnecessarily.

>-    if (!mWindow.width || !mWindow.height) {
>-        return false;
>-    }
>-

Are you sure ShowPluginFrame() will never be called on an empty window?

>     // The browser is limping along with a stale copy of our pixels.
>-    // Try to repaint ASAP.
>+    // Try to repaint ASAP.  This will ClearCurrentBackground() if we
>+    // needed it.

I assume you mean "ClearCurrentSurface()".  But here we already had a
background; just we now know that some pixels have been updated.  I can't think
why ClearCurrentBackground() would be needed here.

Perhaps you meant this comment for the AsyncShowPluginFrame() on new
background above.
Attachment #512733 - Flags: review?(karlt) → review+
(In reply to comment #268)
> >+    // If we were going to use layers rendering but it's not set up
> >+    // yet, and the plugin happens to call this first, we'll
> >+    // harmlessly forward the invalidation to the browser.
> 
> For some definition of "harmless".  It may make the browser paint
> unnecessarily.

If the browser wasn't planning on sending a setwindow anyway, or it was and the plugin was invisible, I assume (but didn't check) that the invalidate would be dropped.

> 
> >-    if (!mWindow.width || !mWindow.height) {
> >-        return false;
> >-    }
> >-
> 
> Are you sure ShowPluginFrame() will never be called on an empty window?

No.

> >     // The browser is limping along with a stale copy of our pixels.
> >-    // Try to repaint ASAP.
> >+    // Try to repaint ASAP.  This will ClearCurrentBackground() if we
> >+    // needed it.
> 
> I assume you mean "ClearCurrentSurface()". 

Yes, sorry.

> Perhaps you meant this comment for the AsyncShowPluginFrame() on new
> background above.

The same applies there, for both the current background and surface.  We might have to clear the background and/or the surface if paint state requires it.
http://hg.mozilla.org/mozilla-central/rev/aa7b2f04d225
http://hg.mozilla.org/mozilla-central/rev/f66564b1510d
http://hg.mozilla.org/mozilla-central/rev/993a5de16346
http://hg.mozilla.org/mozilla-central/rev/bafe55cc0292
http://hg.mozilla.org/mozilla-central/rev/892a2750be5e
http://hg.mozilla.org/mozilla-central/rev/9ea24ee20206
http://hg.mozilla.org/mozilla-central/rev/1a257991dd13
http://hg.mozilla.org/mozilla-central/rev/e3a73d79b6cd
http://hg.mozilla.org/mozilla-central/rev/53aad08fcb3b
http://hg.mozilla.org/mozilla-central/rev/f0404ddb90ed
http://hg.mozilla.org/mozilla-central/rev/dae302e05a8b
http://hg.mozilla.org/mozilla-central/rev/a1090cf786b9
http://hg.mozilla.org/mozilla-central/rev/0f777e59d48c
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
looks like you missed the --disable-ipc follow-ups in comment 191 and comment 192 (that merged are a mere #ifdef MOZ_IPC around using namespace mozilla::plugins;)
http://hg.mozilla.org/mozilla-central/rev/3cc3be69720d
Depends on: 635091
Depends on: 636585
Attachment #508542 - Attachment mime type: application/zip → application/java-archive
Depends on: 636114
Depends on: 637278
Depends on: 646150
Depends on: 657874
Depends on: 682625
Depends on: 685082
You need to log in before you can comment on or make changes to this bug.