1255703 - Investigate using an image surfaces instead of win32 surfaces for basic compositor

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Once we're using a dibsection for the backbuffer I don't think we have anything to gain from using win32 surfaces to do the actual composition. In all of the cases where we actually use gdi, I expect pixman is actually faster.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Use image surfaces for basic compositor

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Use image surfaces for basic compositor

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Use image surfaces for basic compositor

Update nits.

Comment 4

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Use image surfaces for basic compositor

Fix assert.

Comment 5

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=a8991a5d5450&newProject=try&newRevision=d692ed4a02ec&framework=1&showOnlyImportant=0

compare performance with/without patch applied with BasicCompositor. It seemed to improve "tscrollx opt e10s" and  "tresize opt" a lot. But there are some tests that decreased performance a little bit.

Comment 6

[Sotaro Ikeda [:sotaro]]

:jrmuizel, can you comment to comment 5?

Comment 7

[Jeff Muizelaar [:jrmuizel]]

I'm quite surprised by tscrollx opt e10s improvements. We should not be using basic compositor in the win7 and win8 cases.

Comment 8

[Sotaro Ikeda [:sotaro]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> I'm quite surprised by tscrollx opt e10s improvements. We should not be
> using basic compositor in the win7 and win8 cases.

The Tryserver push just enabled BasicCompositor in all cases to check BasicCompositor's performance.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> > I'm quite surprised by tscrollx opt e10s improvements. We should not be
> > using basic compositor in the win7 and win8 cases.
> 
> The Tryserver push just enabled BasicCompositor in all cases to check
> BasicCompositor's performance.

Ah. That talos numbers look good then.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8738902 [details] [diff] [review]
patch - Use image surfaces for basic compositor

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

This feels a bit awkward to me.

Can we instead just have windows/nsWindow override nsBaseWidget::GetBackBufferDrawTarget, call the nsBaseWidget version and then add the CreateDrawTargetForData code to the end?

That would mean the ClearRect call would still go through GDI (which I doubt matters), but if we really care then we could factor the nsBaseWidget version out into a few helpers and call those from nsWindow instead.

::: widget/nsIWidget.h
@@ +1326,5 @@
> +    virtual already_AddRefed<mozilla::gfx::DrawTarget> GetBackBufferDrawTarget(mozilla::gfx::DrawTarget* aScreenTarget,
> +                                                                               const LayoutDeviceIntRect& aRect,
> +                                                                               const LayoutDeviceIntRect& aClearRect) = 0;
> +
> +    virtual mozilla::gfx::DrawTarget* EndRemoteBackBufferDrawing() = 0;

Might as well return a SourceSurface from this, since that's what the callers want.

Drop the word 'Remote' from this, or add it to GetBackBufferDrawTarget so that it's clearer that they are a pair and need to be matched (a comment would be good too).

Comment 11

[Sotaro Ikeda [:sotaro]]

Thanks for the advice. I agree the patch is a bit awkward. I rethink about it related to bug 1263527.

Comment 12

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Use image surfaces for basic compositor on Windows

Apply the comment.

Comment 13

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Use image surfaces for basic compositor on Windows

Comment 14

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8751185 [details] [diff] [review]
patch - Use image surfaces for basic compositor on Windows

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

::: gfx/layers/basic/BasicCompositor.cpp
@@ +173,5 @@
>    RefPtr<BasicCompositingRenderTarget> rt;
>    IntRect rect = aRect.ToUnknownRect();
>  
>    if (aBufferMode != BufferMode::BUFFER_NONE) {
> +    RefPtr<DrawTarget> target = mWidget->GetBackBufferDrawTarget(mDrawTarget, aRect, aClearRect);

Can you please either assert that target != mDrawTarget, or add a 'mNeedsEndBackBufferDrawing' var, so that it's obvious that we always call EndBackBufferDrawing if we called GetBackBufferDrawTarget.

The current code relies on the implementation of GetBackBufferDrawTarget to not return aDrawTarget to be correct. I know that should always be true, but it requires looking up that function to know.

::: widget/windows/WinCompositorWidgetProxy.cpp
@@ +107,5 @@
>  
>  void
>  WinCompositorWidgetProxy::EndRemoteDrawing()
>  {
> +  if (mLockedBackBufferData) {

Should we just make BasicCompositor call EndBackBufferDrawing from AbortFrame (if required), and then just assert that this never happens?

Comment 15

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Use image surfaces for basic compositor on Windows

Apply the comment. Carry "r=mattwoodrow".

Comment 16

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=88f3d07e9d32

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e945d8c30dd

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8e945d8c30dd

Comment 19

[Joel Maher ( :jmaher ) (UTC -8)]

some wins on winxp talos:
https://treeherder.mozilla.org/perf.html#/alerts?id=1177

Comment 20

[Jeff Muizelaar [:jrmuizel]]

This brings general performance improvements when not using hardware acceleration on Windows.