965967 - Reduce Thebes dependencies in plugin architecture

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Reed Koser (:bob_twinkles)]

Attachment: Patch to PluginInstanceChild.cpp to implement Moz2D code paths

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20140108212829

Steps to reproduce:

Bug 933019 requested work on reducing the number of calls to gfxContext(gfxASurface*) and I did some work on this inside the plugin architecture. I would like someone to review the work I have done so far to make sure I am actually on the right track.

Comment 1

[Reed Koser (:bob_twinkles)]

Attachment: Fix bug with code that was compiled out because I'm not on XP

Comment 2

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8368230 [details] [diff] [review]
Fix bug with code that was compiled out because I'm not on XP

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

I think you need to test this, can't guarantee I've got them all right.

::: dom/plugins/ipc/PluginInstanceChild.cpp
@@ +3179,5 @@
>  #endif
>  
>      if (mIsTransparent && !CanPaintOnBackground()) {
>         // Clear surface content for transparent rendering
> +       if (gfxPlatform::GetPlatform()->SupportsAzureContent()) {

This should always be true now, so no need to check it or have the else branch. Same with the other instances in this file.

@@ +3182,5 @@
>         // Clear surface content for transparent rendering
> +       if (gfxPlatform::GetPlatform()->SupportsAzureContent()) {
> +          ColorPattern color(ToColor(aColor));
> +          RefPtr<DrawTarget> dt = gfxPlatform::GetPlatform()->CreateDrawTargetForSurface(renderSurface, IntSize(aRect.XMost(), aRect.YMost()));
> +          dt->FillRect(Rect(0, 0, aRect.XMost(), aRect.YMost()), color);

I think both of these lines should be using aRect.Width()/Height() not XMost()/YMost(). XMost is x+width

You need to pass DrawOptions(1.0f, OP_SOURCE) as a third parameter to get correct rendering (in the case where aColor isn't fully opaque).

@@ +3197,5 @@
>  
>      if (renderSurface != aSurface) {
>          // Copy helper surface content to target
> +        if (gfxPlatform::GetPlatform()->SupportsAzureContent()) {
> +            RefPtr<DrawTarget> dt = gfxPlatform::GetPlatform()->CreateDrawTargetForSurface(renderSurface, IntSize(aRect.XMost(), aRect.YMost()));

Shouldn't this be aSurface? and Width()/Height() again.

@@ +3200,5 @@
> +        if (gfxPlatform::GetPlatform()->SupportsAzureContent()) {
> +            RefPtr<DrawTarget> dt = gfxPlatform::GetPlatform()->CreateDrawTargetForSurface(renderSurface, IntSize(aRect.XMost(), aRect.YMost()));
> +            RefPtr<SourceSurface> source = gfxPlatform::GetPlatform()->GetSourceSurfaceForSurface(dt, renderSurface);
> +
> +            dt->CopySurface(source, ToIntRect(aRect), IntPoint());

aRect.TopLeft() instead of IntPoint(), we want to copy it to the same place in the destination.

@@ +3265,5 @@
> +
> +    if (gfxPlatform::GetPlatoform()->SupportsAzureContent()) {
> +        RefPtr<DrawTarget> dt = gfxPlatform::GetPlatform()->CreateDrawTargetForSurface(whiteImage, IntSize(rect.width, rect.height);
> +        RefPtr<SourceSurface> source = gfxPlatform::GetPlatform()->GetSourceSurfaceForSurface(dt, aSurface);
> +        IntRect sourceRect(rect.x + deviceOffset.x, rect.y + deviceOffset.y, rect.width + deviceOffset.x, rect.height + deviceOffset.y);

I think this one is just rect, no need to add deviceOffset. I think the deviceOffset is positioning the rect.x/y at 0,0 on the destination, which IntPoint() below manages.

@@ +3320,5 @@
> +            IntSize s(targetRect.width, targetRect.height);
> +            RefPtr<DrawTarget> dt = gfxPlatform::GetPlatform()->CreateDrawTargetForSurface(aSurface, s);
> +            RefPtr<SourceSurface> source = gfxPlatform::GetPlatform()->GetSourceSurfaceForSurface(dt, blackImage);
> +
> +            dt->CopySurface(source, IntRect(0, 0, targetRect.width, targetRect.height), IntPoint());

Both parameters need to take targetRect.x/y into account.

Comment 3

[Reed Koser (:bob_twinkles)]

Attachment: PluginInstanceChildPatch v3

(hopefully) fix issues

Comment 4

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8368370 [details] [diff] [review]
PluginInstanceChildPatch v3

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

Looks pretty good!

Have you run the plugin tests locally and done a tryserver run?

::: dom/plugins/ipc/PluginInstanceChild.cpp
@@ +3250,5 @@
> +        RefPtr<DrawTarget> dt = gfxPlatform::GetPlatform()->CreateDrawTargetForSurface(whiteImage, IntSize(rect.Width(), rect.Height());
> +        RefPtr<SourceSurface> source = gfxPlatform::GetPlatform()->GetSourceSurfaceForSurface(dt, aSurface);
> +        IntRect sourceRect(rect.x, rect.y, rect.XMost(), rect.YMost());
> +
> +        dt->CopySuface(source, sourceRect, rect.TopLeft());

I think this one was supposed to stay as IntPoint() and the previous one needs the TopLeft

@@ +3296,5 @@
> +        RefPtr<DrawTarget> dt = gfxPlatform::GetPlatform()->CreateDrawTargetForSurface(aSurface, s);
> +        RefPtr<SourceSurface> source = gfxPlatform::GetPlatform()->GetSourceSurfaceForSurface(dt, blackImage);
> +
> +        dt->CopySurface(source, IntRect(targetRect.x, targetRect.y, targetRect.XMost(), targetRect.YMost())
> +                       , IntPoint(targetRect.x, targetRect.y));

comma goes on the previous line, no leading whitespace here (and align under the 's' not the '(').

Comment 5

[Reed Koser (:bob_twinkles)]

Attachment: PluginInstanceChildPatch v4

I revisited the XP section, I'm not entirerly sure it is correct because I don't have an XP machine to test with.

I don't have access to the tryserver, but the mochitest-ipcplugins test suite passes.

I added a ToIntPoint function to gfx2DGlue.h because I needed it and thought the functionality might be usefull to others in the future

Comment 6

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8369027 [details] [diff] [review]
PluginInstanceChildPatch v4

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

::: dom/plugins/ipc/PluginInstanceChild.cpp
@@ +3249,5 @@
>      PaintRectToSurface(rect, aSurface, gfxRGBA(1.0, 1.0, 1.0));
>      {
> +        RefPtr<DrawTarget> dt = gfxPlatform::GetPlatform()->CreateDrawTargetForSurface(whiteImage, ToIntSize(targetSize));
> +        RefPtr<SourceSurface> source = gfxPlatform::GetPlatform()->GetSourceSurfaceForSurface(dt, aSurface);
> +        IntRect sourceRect(rect.x + deviceOffset.x, rect.y + deviceOffset.y, rect.XMost() + deviceOffset.x, rect.YMost() + deviceOffset.y);

I think we want ToIntRect(rect) here, no deviceOffset.

Comment 7

[Reed Koser (:bob_twinkles)]

Attachment: PluginInstanceChildPatch v5

Comment 8

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8369274 [details] [diff] [review]
PluginInstanceChildPatch v5

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

https://tbpl.mozilla.org/?tree=Try&rev=2219dfc4d35a

Comment 9

[Reed Koser (:bob_twinkles)]

Attachment: PluginInstanceChildPatch2 v1

Removes the last two gfxContext calls in PluginInstanceChild.cpp

Comment 10

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8369762 [details] [diff] [review]
PluginInstanceChildPatch2 v1

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

CopyRect copies an area of the destination onto itself, you're not using the intended source at all.

Comment 11

[Reed Koser (:bob_twinkles)]

Attachment: azurePluginInstanceChild2

Well that was really silly. I think I need to figure out how to extend the mochitest suits...

Comment 12

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8369776 [details] [diff] [review]
azurePluginInstanceChild2

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

Let me know if you want to do more of these patches and I can try explain the thebes API to make the conversions easier :)

::: dom/plugins/ipc/PluginInstanceChild.cpp
@@ +3431,5 @@
> +            nsRefPtr<gfxASurface> surface = mHelperSurface ? mHelperSurface : mCurrentSurface;
> +            RefPtr<DrawTarget> dt = gfxPlatform::GetPlatform()->CreateDrawTargetForSurface(surface, ToIntSize(surface->GetSize()));
> +            RefPtr<SourceSurface> source = gfxPlatform::GetPlatform()->GetSourceSurfaceForSurface(dt, mBackground);
> +
> +            dt->CopySurface(source, ToIntRect(rect), IntPoint());

ToIntPoint(rect.TopLeft()) for the third param

@@ +3563,5 @@
>      result.Sub(mSurfaceDifferenceRect, nsIntRegion(rect));
>      nsIntRegionRectIterator iter(result);
>      const nsIntRect* r;
>      while ((r = iter.Next()) != nullptr) {
> +        dt->CopySurface(source, ToIntRect(*r), IntPoint());

Same here with the ToIntPoint()

Comment 13

[Reed Koser (:bob_twinkles)]

Attachment: PluginInstanceChildPatch2 v2

I also fixed the compilation issues on XP and MacOSX for the other patch, should I upload that version?

While reviewing the testserver logs I noticed that the test_invalidate_during_plugin_paint test fails with an error that looks to me like corupted state; I haven't had time to look into it yet. Do you have any recomendations for where I should start? (e.g. is there some lock I need to aquire before using gfxPlatform::GetPlatform()?)

Comment 14

[Matt Woodrow (:mattwoodrow)]

Oh that's a pain. Looks like we can't use the pref service from the child (plugin) process, and accessing gfxPlatform does exactly that.

It looks like CreateDrawTargetForSurface can be made static (or just extract it to a local helper). For GetSourceSurfaceForSurface you'll need to extract the BackendType::CAIRO section into a local helper.

Comment 15

[Jonathan Watt [:jwatt]]

Reed, are you still working on this?

Comment 16

[Reed Koser (:bob_twinkles)]

Not actively, other things have come up and I haven't had time to dive into firefox work - feel free to reassign it.

Comment 17

[Jonathan Watt [:jwatt]]

Reed, which email address do you want used for the checkin author field?

Comment 18

[Jonathan Watt [:jwatt]]

Attachment: PluginInstanceChildPatch v6

Comment 19

[Reed Koser (:bob_twinkles)]

[e-mail] should be fine.

Comment 20

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8435387 [details] [diff] [review]
PluginInstanceChildPatch v6

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

::: dom/plugins/ipc/PluginInstanceChild.cpp
@@ +3317,5 @@
> +        RefPtr<DrawTarget> dt = CreateDrawTargetForSurface(aSurface);
> +        RefPtr<SourceSurface> surface =
> +            gfxPlatform::GetSourceSurfaceForSurface(dt, blackImage);
> +        dt->CopySurface(surface,
> +                        IntRect(0, 0, rect.width, rect.height),

We have to use all of rect here, not just the size don't we? This seems identical to the 2nd one.

Comment 21

[Jonathan Watt [:jwatt]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #20)
> We have to use all of rect here, not just the size don't we? This seems
> identical to the 2nd one.

I don't think so. |blackImage| is only the size of aRect (or rect), and we're copying the entire thing into aSurface (where we want it offset to to the position of aRect.

Comment 22

[Jonathan Watt [:jwatt]]

It passes Try including the plugin invalidation reftests, and seems to work with youtube (including overlay advert dismissal) so I went ahead and landed part 1:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca725c15bdeb

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ca725c15bdeb

Comment 24

[Gary [:streetwolf52]]

Based on regression testing I've found that this patch causes visual distortion of Flash videos.  When I start a video I usually see a small black box before the video starts.  Other blackness might occur.  Once the video is playing, hovering over the video causes it to flash (no pun intended).

Good: https://hg.mozilla.org/integration/mozilla-inbound/rev/7e7061043b85

Bad : https://hg.mozilla.org/integration/mozilla-inbound/rev/ca725c15bdeb


Graphics
--------

Adapter Description: AMD Radeon HD 7900 Series
Adapter Drivers: aticfx64 aticfx64 aticfx64 aticfx32 aticfx32 aticfx32 atiumd64 atidxx64 atidxx64 atiumdag atidxx32 atidxx32 atiumdva atiumd6a atitmm64
Adapter RAM: 3072
ClearType Parameters: D [ Gamma: 2200 Pixel Structure: R ClearType Level: 100 Enhanced Contrast: 50 ] D [ Gamma: 2200 Pixel Structure: R ClearType Level: 100 Enhanced Contrast: 50 ]
Device ID: 0x6798
Direct2D Enabled: true
DirectWrite Enabled: true (6.3.9600.16384)
Driver Date: 5-22-2014
Driver Version: 14.200.0.0
GPU #2 Active: false
GPU Accelerated Windows: 1/1 Direct3D 11 (OMTC)
Vendor ID: 0x1002
WebGL Renderer: Google Inc. -- ANGLE (AMD Radeon HD 7900 Series Direct3D9Ex vs_3_0 ps_3_0)
windowLayerManagerRemote: true
AzureCanvasBackend: direct2d
AzureContentBackend: direct2d
AzureFallbackCanvasBackend: cairo
AzureSkiaAccelerated: 0

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0

Comment 25

[Gary [:streetwolf52]]

Looks like OMTC is causing this problem.  Turning OMTC off fixes the problem.

Comment 26

[Gary [:streetwolf52]]

Doesn't seem like Flash on all sites is affected.  One that is www.weather.com.

Comment 27

[[e-mail]]

(In reply to Gary [:streetwolf] from comment #26)
> Doesn't seem like Flash on all sites is affected.  One that is
> www.weather.com.

I ran into this on several websites.

Comment 28

[Jonathan Watt [:jwatt]]

Looking into this.

Comment 29

[Jonathan Watt [:jwatt]]

Curious. This is due to the section:

@@ -3166,33 +3194,33 @@ PluginInstanceChild::PaintRectToSurface(
     }
     if (mHelperSurface) {
         // On X11 we can paint to non Xlib surface only with HelperSurface
         renderSurface = mHelperSurface;
     }
 #endif
 
     if (mIsTransparent && !CanPaintOnBackground()) {
-       // Clear surface content for transparent rendering
-       nsRefPtr<gfxContext> ctx = new gfxContext(renderSurface);
-       ctx->SetDeviceColor(aColor);
-       ctx->SetOperator(gfxContext::OPERATOR_SOURCE);
-       ctx->Rectangle(GfxFromNsRect(plPaintRect));
-       ctx->Fill();
+        // Clear surface content for transparent rendering
+        ColorPattern color(ToColor(aColor));
+        RefPtr<DrawTarget> dt = CreateDrawTargetForSurface(renderSurface);
+        dt->FillRect(gfx::Rect(plPaintRect.x, plPaintRect.y,
+                               plPaintRect.width, plPaintRect.height),
+                     color,
+                     DrawOptions(1.f, CompositionOp::OP_SOURCE));
     }
 
     PaintRectToPlatformSurface(plPaintRect, renderSurface);
 
     if (renderSurface != aSurface) {

I'll revert that part as soon as the tree reopens (and assume r=mattwoodrow).

Comment 30

[Jonathan Watt [:jwatt]]

Partial backout:

https://hg.mozilla.org/integration/mozilla-inbound/rev/45eeaf4173cd

Comment 31

[Tim Nguyen :ntim]

Merge of backout : https://hg.mozilla.org/mozilla-central/rev/45eeaf4173cd

Comment 32

[Jonathan Watt [:jwatt]]

Matt, any idea why the change in comment 29 would interact badly with OMTC?

Comment 33

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8435387 [details] [diff] [review]
PluginInstanceChildPatch v6

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

::: dom/plugins/ipc/PluginInstanceChild.cpp
@@ +3200,5 @@
>  
>      if (mIsTransparent && !CanPaintOnBackground()) {
> +        // Clear surface content for transparent rendering
> +        ColorPattern color(ToColor(aColor));
> +        RefPtr<DrawTarget> dt = CreateDrawTargetForSurface(renderSurface);

Check if renderSurface has a deviceOffset set. Moz2D doesn't take these into account, but thebes will.

Comment 34

[Jonathan Watt [:jwatt]]

The failure caused by the code backed out in comment 29 is because Moz2D doesn't treat filling as a bounded operation whet you use OP_SOURCE. (See bug 1031713.)

(The fact that failure didn't show up on non-Windows would seem to indicate that the Moz2D behavior is not consistent, but that's a separate issue.)

Comment 35

[Jonathan Watt [:jwatt]]

Attachment: Make PluginInstanceChild::PaintRectToSurface paint using Moz2D

Comment 36

[Jonathan Watt [:jwatt]]

Comment on attachment 8447748 [details] [diff] [review]
Make PluginInstanceChild::PaintRectToSurface paint using Moz2D

https://hg.mozilla.org/integration/mozilla-inbound/rev/0303b2fe5f90

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0303b2fe5f90

Comment 38

[Jonathan Watt [:jwatt]]

Attachment: Get rid of the remaining Thebes backed gfxContexts in PluginInstanceChild.cpp

Comment 39

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d2ff2b8db94

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2d2ff2b8db94