Reduce Thebes dependencies in plugin architecture

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bob_twinkles, Assigned: bob_twinkles)

Tracking

Trunk
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 8 obsolete attachments)

8.56 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.72 KB, patch
mattwoodrow
: review+
jwatt
: checkin+
Details | Diff | Splinter Review
4.27 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
Assignee

Description

6 years ago
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.
Assignee

Updated

6 years ago
Attachment #8368230 - Flags: review?(matt.woodrow)
Assignee: nobody → srkoser+mozilla
Blocks: 933019
Attachment #8368135 - Attachment is obsolete: true
Attachment #8368135 - Attachment is patch: true
Attachment #8368230 - Attachment is patch: true
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.
Assignee

Comment 3

6 years ago
Posted patch PluginInstanceChildPatch v3 (obsolete) — Splinter Review
(hopefully) fix issues
Attachment #8368230 - Attachment is obsolete: true
Attachment #8368230 - Flags: review?(matt.woodrow)
Attachment #8368370 - Flags: review?(matt.woodrow)
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 '(').
Assignee

Comment 5

6 years ago
Posted patch PluginInstanceChildPatch v4 (obsolete) — Splinter Review
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
Attachment #8368370 - Attachment is obsolete: true
Attachment #8368370 - Flags: review?(matt.woodrow)
Attachment #8369027 - Flags: review?(matt.woodrow)
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.
Attachment #8369027 - Flags: review?(matt.woodrow) → review+
Assignee

Comment 7

6 years ago
Posted patch PluginInstanceChildPatch v5 (obsolete) — Splinter Review
Attachment #8369027 - Attachment is obsolete: true
Attachment #8369274 - Flags: review?(matt.woodrow)
Comment on attachment 8369274 [details] [diff] [review]
PluginInstanceChildPatch v5

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

https://tbpl.mozilla.org/?tree=Try&rev=2219dfc4d35a
Attachment #8369274 - Flags: review?(matt.woodrow) → review+
Assignee

Comment 9

6 years ago
Posted patch PluginInstanceChildPatch2 v1 (obsolete) — Splinter Review
Removes the last two gfxContext calls in PluginInstanceChild.cpp
Attachment #8369762 - Flags: review?(matt.woodrow)
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.
Attachment #8369762 - Flags: review?(matt.woodrow) → review-
Assignee

Comment 11

6 years ago
Posted patch azurePluginInstanceChild2 (obsolete) — Splinter Review
Well that was really silly. I think I need to figure out how to extend the mochitest suits...
Attachment #8369762 - Attachment is obsolete: true
Attachment #8369776 - Flags: review?(matt.woodrow)
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()
Assignee

Comment 13

6 years ago
Posted patch PluginInstanceChildPatch2 v2 (obsolete) — Splinter Review
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()?)
Attachment #8369776 - Attachment is obsolete: true
Attachment #8369776 - Flags: review?(matt.woodrow)
Attachment #8372041 - Flags: review?(matt.woodrow)
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.
Reed, are you still working on this?
Flags: needinfo?(srkoser+mozilla)
Assignee

Comment 16

5 years ago
Not actively, other things have come up and I haven't had time to dive into firefox work - feel free to reassign it.
Flags: needinfo?(srkoser+mozilla)
Reed, which email address do you want used for the checkin author field?
Flags: needinfo?(srkoser+mozilla)
Attachment #8369274 - Attachment is obsolete: true
Attachment #8435387 - Flags: review?(matt.woodrow)
Assignee

Comment 19

5 years ago
srkoser@gmail.com should be fine.
Flags: needinfo?(srkoser+mozilla)
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.
Attachment #8435387 - Flags: review?(matt.woodrow) → review+
Attachment #8372041 - Flags: review?(matt.woodrow)
(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.
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
https://hg.mozilla.org/mozilla-central/rev/ca725c15bdeb
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
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
Looks like OMTC is causing this problem.  Turning OMTC off fixes the problem.
Doesn't seem like Flash on all sites is affected.  One that is www.weather.com.
(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.

Updated

5 years ago
Blocks: 1022140
Looking into this.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: FIXED → ---
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).
Matt, any idea why the change in comment 29 would interact badly with OMTC?
Flags: needinfo?(matt.woodrow)
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.
Flags: needinfo?(matt.woodrow)
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.)
Attachment #8447748 - Flags: review?(matt.woodrow) → review+
Keywords: leave-open
Comment on attachment 8447748 [details] [diff] [review]
Make PluginInstanceChild::PaintRectToSurface paint using Moz2D

https://hg.mozilla.org/integration/mozilla-inbound/rev/0303b2fe5f90
Attachment #8447748 - Flags: checkin+
Attachment #8449676 - Flags: review?(matt.woodrow) → review+
OS: Linux → All
Hardware: x86_64 → All
https://hg.mozilla.org/mozilla-central/rev/2d2ff2b8db94
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.