995661 - when canvas and content graphics backends don't match, drawWindow calls that don't cover the complete canvas or have transparent background blank the rest of it

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

One of the problems I ran into trying to get the reftest harness to use partial drawWindow calls again in bug 995410 was that when the content and canvas graphics backends don't match, all drawWindow calls blank the rest of the canvas.  This means that a drawWindow call that doesn't cover the canvas is broken, and I believe one that involves transparency is broken as well.

The fix is a one-line change (use OP_OVER rather than OP_SOURCE), although I have some preparatory cleanup and a mochitest for drawWindow that tests partial draws (including at more interesting offsets than what the reftest harness does, where every drawWindow draws a subregion exactly matching the current translation) and transparency.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1 - Remove dead code resulting from removal of gfxPlatform::SupportsAzureContent()

This should have been removed in bug 985049.

The changes here can be understood as a simplification based on drawSurf
always being null.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2 - Use OP_OVER rather than OP_SOURCE to handle canvas drawWindow calls that don't cover the canvas or (I believe) that contain transparency

This is needed to allow us to return to using partial-canvas invalidates
in the reftest harness, though only for the platforms that go through
this codepath.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3 - Add mochitest for canvas drawWindow

This test covers some basic cases, such as drawing the complete window
to a canvas of the same size, drawing a part of the window to a
different offset within the canvas, drawing parts of the window against
different backgrounds and against a transparent background, and doing
multiple such drawWindow calls on the same canvas.

It runs its tests with a few different combinations of drawWindow flags.

However, it's entirely possible that it might not be exercising various
drawWindow scenarios as well as it should be because of the way the
iframe being drawn is set up, since this test has so far only shown one
of the two bugs that was a problem in the reftest harness (the use of
OP_SOURCE rather than OP_OVER).

Comment 4

[Timothy Nikkel (:tnikkel)]

Widget layers will get ignored if you are drawWindow'ing an iframe. To test it you'll need to drawWindow a displayroot, ie the toplevel document in an window (in the OS window sense).

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3 - Add mochitest for canvas drawWindow

This test covers some basic cases, such as drawing the complete window
to a canvas of the same size, drawing a part of the window to a
different offset within the canvas, drawing parts of the window against
different backgrounds and against a transparent background, and doing
multiple such drawWindow calls on the same canvas.

It runs its tests both with an iframe (whose background is transparent)
and content in a new window (which has a white backdrop).

Bug 995721 will add further tests (in mochitest-chrome) that exercise
drawWindow from a toplevel chrome window, which exercises the
DRAWWINDOW_USE_WIDGET_LAYERS codepath, which these tests do not.

The final 4 tests in each set of 7 fail without patch 2 in this bug.

Comment 6

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8405802 [details] [diff] [review]
patch 2 - Use OP_OVER rather than OP_SOURCE to handle canvas drawWindow calls that don't cover the canvas or (I believe) that contain transparency

Review of attachment 8405802 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +3674,5 @@
>      mgfx::Rect sourceRect(0, 0, sw, sh);
>      mTarget->DrawSurface(source, destRect, sourceRect,
>                           DrawSurfaceOptions(mgfx::Filter::POINT),
> +                         DrawOptions(1.0f, CompositionOp::OP_OVER,
> +                                     AntialiasMode::NONE));

I think we'd be better off using PushClipRect(destRect); DrawSurface(); PopClip(); here instead of using OP_OVER.

Using OVER would be incorrect if our window isn't fully opaque, and we haven't provided an opaque background color. I don't think that ever actually happens with reftests, but it seems possible in theory.

OP_SOURCE is rather confusing in that the effects of it aren't bound by the size of the mask (the source image in this case). So it draws something into every pixel of the destination that isn't clipped out, and if that location doesn't map to a pixel within the source then it uses the clamped value of the nearest edge.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8405802 [details] [diff] [review]
patch 2 - Use OP_OVER rather than OP_SOURCE to handle canvas drawWindow calls that don't cover the canvas or (I believe) that contain transparency

Review of attachment 8405802 [details] [diff] [review]:
-----------------------------------------------------------------

I retract my previous comments after looking at the docs for drawWindow(). OVER makes more sense.

We're also supposed to take the global alpha into account, but clearly nobody cares about this. It's easy enough to fix for this path (just replace the 1.0f with state.globalAlpha), but not really for the direct-rendering path. Maybe we should just change CanvasRenderingContext2D.webidl to say that we don't do this.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8405926 [details] [diff] [review]
patch 3 - Add mochitest for canvas drawWindow

Review of attachment 8405926 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/test/test_drawWindow.html
@@ +28,5 @@
> +    if (event.target != sourceWindow.document) {
> +      return;
> +    }
> +
> +    var cxInterfaceWrap = SpecialPowers.wrap(CanvasRenderingContext2D);

What is this used for?

@@ +32,5 @@
> +    var cxInterfaceWrap = SpecialPowers.wrap(CanvasRenderingContext2D);
> +
> +    runDrawWindowTests(document.getElementById("source").contentWindow,
> +                       0, true);
> +    runDrawWindowTests(sourceWindow, 0, false);

Can we run this second test with the flag 'DRAWWINDOW_USE_WIDGET_LAYERS' (maybe as an extra, not instead). This asks us to render using an existing layer tree for this window, and is what will cause us to hit the code in ClientLayerManager.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> > +    var cxInterfaceWrap = SpecialPowers.wrap(CanvasRenderingContext2D);
> 
> What is this used for?

Oops, that ended up used in the test in bug 995721 but not here.

> Can we run this second test with the flag 'DRAWWINDOW_USE_WIDGET_LAYERS'
> (maybe as an extra, not instead). This asks us to render using an existing
> layer tree for this window, and is what will cause us to hit the code in
> ClientLayerManager.

That's in the test in bug 995721.

(It wouldn't work here, since in this case it's a window with chrome around it, not the actual toplevel window.)

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, it turns out the revised version of the mochitest failed on B2G because my expectation that the window that was the result of window.open has an opaque background is wrong on b2g; I think I'm just going to add a B2G-specific flip in that expectation.  (It's actually rather a pain to even just verify what that window looks like...)

Comment 11

[Timothy Nikkel (:tnikkel)]

Documents that would be opaque on desktop aren't always on b2g because http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#5190 IsCrossProcessRootContentDocument returns false for some documents that are root content documents, this is because TabChild has it's own notion of root content document which is just |!HasAppOwnerApp()|. Ironically the comment there indicates that this is done so that remote iframes are opaque for remote reftests.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/febd5ad98adb
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e0abf6eeec3
https://hg.mozilla.org/integration/mozilla-inbound/rev/33bfd3ceee1f

Comment 13

[Kathleen :Brade]

Does this fix also fix bug #985563?

Comment 14

[Mark Smith [:mcs]]

(In reply to Kathleen Brade from comment #13)
> Does this fix also fix bug #985563?

I just tested with https://tbpl.mozilla.org/?tree=Try&rev=62175d515bb4 and indeed #985563 is fixed by the OP_OVER patch (https://hg.mozilla.org/integration/mozilla-inbound/rev/2e0abf6eeec3)

It would be nice to pull that fix into a release earlier than FF 31 since (1) it is a regression that has been present since FF 24 and (2) the fix is simple.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And a followup to disable the mochitest for now on b2g desktop:
https://hg.mozilla.org/integration/mozilla-inbound/rev/16f349eeb65c

(I have vague memories of window.open not working in b2g desktop mochitests...)

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/febd5ad98adb
https://hg.mozilla.org/mozilla-central/rev/2e0abf6eeec3
https://hg.mozilla.org/mozilla-central/rev/33bfd3ceee1f
https://hg.mozilla.org/mozilla-central/rev/16f349eeb65c