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.

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.

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.

(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!

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

(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.

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.

Rollup of roc's patches and my patch, in case anyone wants to noodle.

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.)

(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".)

(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.

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.

Add ReadbackManager files.

Oops, left out a patch in the other one. Thanks Bas.

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.

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.

This is based on the communitymx wmode=transparent example. I didn't check copyrights, please don't use this page for anything.

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.

Adds logic to fall back on alpha recovery on windows.

+ 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.

Updated benchmark to include a mozPaintCount counter, and allow modes to be specified via a query parameter, e.g. ?opaque. Default is transparent.

Right file

(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?

trivial

Not sure who should review this really

Perhaps you won't like this, Timothy :-)

Note that there's no actual readback here. We just ensure that the ThebesLayer draws enough.

This is pure backout, and should land after the other patches in this bug have landed successfully.

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.

Just the goop to get plugin backgrounds into dom/plugins.

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.

(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)

(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.

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.

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.

(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.

>+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();

Lots of updates, reposting. Carrying over r+ and sr+.

(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.

(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.

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.)

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.

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.

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.

(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);

(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.

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

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.

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?

Yes!

In case anyone wants it. Based on m-c c618b7b138fb.

(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.

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?

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.

(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.

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.

Addressed review comments.

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.

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.

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.

(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?)

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.

Pretty sure this is OK, but want a second look.

Changed to do this only for NS_FREE_PERMANENT_DATA as we discussed.

(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.

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.

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.

Includes all the fixes above.

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.

Added a note and fixed the backgrounddestroyer bug.

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?

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())|.

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

Adjusted to use a single 'DeviceAttachments' struct as suggested in review by cjones.

(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.

(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.

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.

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.

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().

To be folded into part 2.

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.

(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?

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).

This is the simple fix we discussed. OK on tryserver.