The code is very broken, because we try to paint onto an image surface, but that requires a dc. This is a s-s because we end up making invalid static casts to SharedDIBSurface* from vanilla image surface, then passing the HDC to the plugin, e.g. here 2525 if (curSurface) { 2526 NS_ASSERTION(SharedDIBSurface::IsSharedDIBSurface(curSurface), 2527 "Expected (SharedDIB) image surface."); 2528 2529 SharedDIBSurface* dibsurf = static_cast<SharedDIBSurface*>(curSurface.get()); 2530 dc = dibsurf->GetHDC(); I don't know if this is exploitable, just crashy, or windows sanitizes these, but better safe than sorry etc.

This needs to block for being broken and possibly exploitable or crashy.

Not s-s, this code can't be hit in the wild.

I'm not quite sure under what conditions we would enable alpha extraction on windows. If drawing-on-background fails, in which cases we would try to use the current set of hacks and in which cases would we fall back on alpha extraction? Is the idea to remove the current hacks and always fall back on alpha extraction, since we're betting on it being an uncommon case?

Thanks Jim, I cannibalized your patch from bug 611698. I'm not sure whether this bug should block bug 626602 or not, and whether there's code we want to get rid of in favor of the normal path in 626602 and the fallback here.

Aha, bug 626202#c10 confirms.

We definitely need this. The "current set of hacks" --- i.e., painting the plugin to a DIB and hoping we get the right alpha values --- fails in unpredictable ways, so we just can't do that.

Comment on attachment 508050 [details] [diff] [review] Fix alpha recovery fallback on windows I should probably review this since jimm wrote part of it... + // Paint the plugin directly onto the target, with a white + // background and copy the result + PaintRectToSurface(aRect, aSurface, gfxRGBA(1.0, 1.0, 1.0)); + { + gfxRect copyRect(gfxPoint(0, 0), targetRect.size); + nsRefPtr<gfxContext> ctx = new gfxContext(whiteImage); + ctx->SetOperator(gfxContext::OPERATOR_SOURCE); + ctx->SetSource(aSurface, deviceOffset); + ctx->Rectangle(copyRect); + ctx->Fill(); Isn't this copy avoidable? Can't whiteImage be a temporary DIB surface, that we draw into and then pass a gfxImageSurface wrapper around the same bits to RecoverAlpha?

Also, while we're changing the temp surface allocation, we should do something to ensure that the buffers have the same alignment relative to 16-byte boundaries, to ensure the SSE2 RecoverAlpha code is used.

(In reply to comment #7) > Isn't this copy avoidable? Can't whiteImage be a temporary DIB surface, that we > draw into and then pass a gfxImageSurface wrapper around the same bits to > RecoverAlpha? Yes. (In reply to comment #8) > Also, while we're changing the temp surface allocation, we should do something > to ensure that the buffers have the same alignment relative to 16-byte > boundaries, to ensure the SSE2 RecoverAlpha code is used. That's kinda tricky unless we always want to create a temporary blackImage.

I don't see why. Actually, in my testing a SharedDIBSurface's bits are always aligned to a page or something like that. So all we need to do is ensure that the temporary whiteImage bits are allocated at an aligned offset.

The strides need to match up properly as well. Maybe this is easier than I think, I'll take a look after finishing some other stuff.

Jim, what all needs to be set up for the plugin to paint to a temporary gfxWindowsSurface smaller than its frame? I tried setting this up by adding a gfxASurface param to UpdateWindowAttributes() and using its DC if non-null, but I got messed-up painting from flash with that, I suspect because other window attributes didn't match up with the DC. In the meantime, I'll try to get a new patch up that aligns things properly for SSE2 alpha recovery.

(In reply to comment #12) > Jim, what all needs to be set up for the plugin to paint to a temporary > gfxWindowsSurface smaller than its frame? I tried setting this up by adding a > gfxASurface param to UpdateWindowAttributes() and using its DC if non-null, but > I got messed-up painting from flash with that, I suspect because other window > attributes didn't match up with the DC. > > In the meantime, I'll try to get a new patch up that aligns things properly for > SSE2 alpha recovery. Plugins require a number of setup calls if you're switching out surfaces, all of which happen in UpdateWindowAttributes and PaintRectToPlatformSurface. All of these rely on data in mWindow, is that getting updated correctly?

I'd suggest overhauling the code in those two routines so they don't use mWindow at all. Everything should get passed in so we have better control over what gets rendered to. We only call UpdateWindowAttributes from two places, DoAsyncSetWindow and PaintRectToPlatformSurface.

I looked a bit more last night and I'm not sure how using a temporary surface is going to work. It seems like with the code that's there, I would need to change the window size, but that seems wrong (don't want flash "reflowing"). And experience shows flash doesn't deal with a DC for the smaller surface. The X code does the same thing too, only hands out surfaces that are the size of the frame. I think this might just be a limitation we'll have to live with for the time being, unless I'm missing something.

I have the nagging feeling that I'm missing a cleverer solution for the rect-nudging algorithm, but what's there should be OK.

This uses the copy-back-to-temporary approach because I don't know how to give flash a temporary windows surface. In totally unscientific tests of the GUIMark wmode=transparent benchmark from bug 626602, I saw fps go from ~13fps to ~15fps.

Crap, I realized that a simplification I made to this earlier today was wrong. Fixed.

(In reply to comment #14) > I'd suggest overhauling the code in those two routines so they don't use > mWindow at all. Everything should get passed in so we have better control over > what gets rendered to. We only call UpdateWindowAttributes from two places, > DoAsyncSetWindow and PaintRectToPlatformSurface. FWIW, I totally agree with this, but I think we should wholesale rewrite PluginInstanceChild/Parent. The current code is a disgusting rat's nest. I have some ideas for how we can use the remote layers system to clean things up and make updates more efficient / less glitchy too. Post-4.0 though! :) (Although if we want async rendering on mac for 4.0, we may need this.)

Comment on attachment 508200 [details] [diff] [review] part 1: Add some helpers for aligning surfaces for alpha recovery, v2 + static PRUint32 GoodAlignmentlg2() { return 4; /* for SSE2 */ } GoodAlignmentLog2 + return (aX + aStride * aY) & ((1 << aTolg2) - 1); "Tolg" is not a word :-) + // Thebes only supports ARGB32 CONTENT_COLOR_ALPHA surfaces, so + // assume we have one here. Add an assertion on aSurface->GetFormat().

(In reply to comment #22) > Comment on attachment 508200 [details] [diff] [review] > part 1: Add some helpers for aligning surfaces for alpha recovery, v2 > > + static PRUint32 GoodAlignmentlg2() { return 4; /* for SSE2 */ } > > GoodAlignmentLog2 Oops, fixed. I'm dumb. > + return (aX + aStride * aY) & ((1 << aTolg2) - 1); > > "Tolg" is not a word :-) > aAlignToLog2? > + // Thebes only supports ARGB32 CONTENT_COLOR_ALPHA surfaces, so > + // assume we have one here. > > Add an assertion on aSurface->GetFormat(). Done.

(In reply to comment #23) > aAlignToLog2? Sure.

http://hg.mozilla.org/mozilla-central/rev/10677c174653 http://hg.mozilla.org/mozilla-central/rev/e9279a68304d http://hg.mozilla.org/mozilla-central/rev/66459fa063f3 http://hg.mozilla.org/mozilla-central/rev/0903dd13cb78

backed out due to permaorange in linux reftests http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1297267690.1297268369.1934.gz reftest/tests/modules/plugin/test/reftest/plugin-alpha-zindex.html | image comparison (==) http://hg.mozilla.org/mozilla-central/rev/548c6fb45f53

FTR, this failure is due to a pre-existing bug that was incidentally fixed in part 9 of bug 626602. But we'll re-land all this stuff together so it doesn't really matter.

The algorithm keeps the bottommost pixel fixed always, and keeps the rightmost fixed wrt finding a new x coordinate, but needs to check that the original rightmost pixel + dw is within the surface. I was confusing right/top/w/h in my head. (Ignore the *NULL = 5, just for debugging.)

Comment on attachment 511533 [details] [diff] [review] part 1.1: Fix thinko + for (dw = 0; (dw < pixPerAlign) && (r + dw <= sw); ++dw) { Shouldn't this actually compare "(r + dw)*bpp <= surfaceSize.stride"? I guess this is conservative. I think this would be easier to understand if you named dw 'dr' instead and added some comments explaining that the d* values mean.

(In reply to comment #29) > Comment on attachment 511533 [details] [diff] [review] > part 1.1: Fix thinko > > + for (dw = 0; (dw < pixPerAlign) && (r + dw <= sw); ++dw) { > > Shouldn't this actually compare "(r + dw)*bpp <= surfaceSize.stride"? I guess > this is conservative. We already bail early if |bpp * surfaceSize.width != aSurface->Stride()|, so we know that |r + dw <= sw => (r + dw)*bpp <= surfaceSize.stride". > I think this would be easier to understand if you named dw 'dr' instead and > added some comments explaining that the d* values mean. Yes, good ideas.

http://hg.mozilla.org/mozilla-central/rev/22a8b2770f45 http://hg.mozilla.org/mozilla-central/rev/2be3da6a6531 http://hg.mozilla.org/mozilla-central/rev/7e8fb5a646da http://hg.mozilla.org/mozilla-central/rev/663d27e9098c