629799 - Alpha extraction in plugin subprocesses is broken on windows

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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?

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Fix alpha recovery fallback on windows

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.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Aha, bug 626202#c10 confirms.

Comment 6

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

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 7

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

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?

Comment 8

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

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.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 10

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

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.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 13

[Jim Mathies [:jimm]]

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

Comment 14

[Jim Mathies [:jimm]]

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.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Add some helpers for aligning surfaces for alpha recovery

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

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Ensure that gfxImageSurfaces are aligned well for alpha recovery

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 3: Ensure that SharedDIBWins are aligned well for alpha recovery

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Fix alpha-recovery fallback on windows and try to use the SIMD path when possible

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.

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Add some helpers for aligning surfaces for alpha recovery, v2

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

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

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

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

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 24

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

(In reply to comment #23)
> aAlignToLog2?

Sure.

Comment 25

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 26

[Marco Bonardo [:mak]]

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

Comment 27

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 28

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1.1: Fix thinko

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 29

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

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.

Comment 30

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 31

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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