1405481 - Push canvas updates to the compositor on empty transactions

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P1)

Description

[Kartikaya Gupta (email:[e-mail])]

We have some code that looks like it's supposed to send canvas updates to the compositor on empty transactions [1]. However it runs inside the layers-free block of EndTransactionInternal which only gets invoked accidentally. In bug 1403915 I'm moving the code around a bit, and commenting it out because it causes crashes if I run it where I want to run it. Somebody who knows what this code is doing should use this bug to fix it up and uncomment it.

[1] http://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/gfx/layers/wr/WebRenderLayerManager.cpp#817-824

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Push canvas updates to the compositor on empty transactions

Comment 2

[Sotaro Ikeda [:sotaro]]

Bug 1405594 is related to this bug.

Comment 3

[Sotaro Ikeda [:sotaro]]

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

Comment 4

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #3)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=d6d3f0560a0069efd6be9f4f1b380266a3cd7bb4

Some tests failed. Main reason of "application timed out after 330 seconds with no output" seemed to be caused by a thing that DidComposite did not triggered by EndEmptyTransaction().

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Push canvas updates to the compositor on empty transactions

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Push canvas updates to the compositor on empty transactions

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Push canvas updates to the compositor on empty transactions

Comment 8

[Kartikaya Gupta (email:[e-mail])]

(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> (In reply to Sotaro Ikeda [:sotaro] from comment #3)
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=d6d3f0560a0069efd6be9f4f1b380266a3cd7bb4
> 
> Some tests failed. Main reason of "application timed out after 330 seconds
> with no output" seemed to be caused by a thing that DidComposite did not
> triggered by EndEmptyTransaction().

I think I made a mistake when doing the new EndEmptyTransaction, I forgot to check for the EndTransactionFlags and do a composite if the flags don't contain END_NO_COMPOSITE. That fix should probably be a separate patch from the canvas updates thing.

Comment 9

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e6cf2a568e66f2da5aea15281be44eb01fd28e8

Comment 10

[Sotaro Ikeda [:sotaro]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> (In reply to Sotaro Ikeda [:sotaro] from comment #4)
> > (In reply to Sotaro Ikeda [:sotaro] from comment #3)
> > > https://treeherder.mozilla.org/#/
> > > jobs?repo=try&revision=d6d3f0560a0069efd6be9f4f1b380266a3cd7bb4
> > 
> > Some tests failed. Main reason of "application timed out after 330 seconds
> > with no output" seemed to be caused by a thing that DidComposite did not
> > triggered by EndEmptyTransaction().
> 
> I think I made a mistake when doing the new EndEmptyTransaction, I forgot to
> check for the EndTransactionFlags and do a composite if the flags don't
> contain END_NO_COMPOSITE. That fix should probably be a separate patch from
> the canvas updates thing.

Thanks! I am going to add a patch to skip composition if END_NO_COMPOSITE is set.

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Push canvas updates to the compositor on empty transactions

Rebased.

Comment 12

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Push canvas updates to the compositor on empty transactions

Comment 13

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Suppress to composite when END_NO_COMPOSITE is set

Comment 14

[Sotaro Ikeda [:sotaro]]

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

Comment 15

[Sotaro Ikeda [:sotaro]]

attachment 8915839 [details] [diff] [review] does not work well with "WebGL Lesson 3 – a bit of movement" sample.
   http://learningwebgl.com/lessons/lesson03/index.html

On current nightly, the sample update is only 2-3 fps. attachment 8915819 [details] [diff] [review] addressed the low fps problem. But attachment 8915839 [details] [diff] [review] reverts the situation.

From it, it seems that even when END_NO_COMPOSITE is set, if canvases have updates, WebRenderLayerManager needs to trigger composite.

Comment 16

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Suppress to composite when END_NO_COMPOSITE is set

Comment 17

[Sotaro Ikeda [:sotaro]]

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

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8915819 [details] [diff] [review]
patch part 1 - Push canvas updates to the compositor on empty transactions

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

There's a bunch of stuff in here that I'm not too familiar with, so bouncing to Jeff.

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8915861 [details] [diff] [review]
patch part 2 - Suppress to composite when END_NO_COMPOSITE is set

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

::: gfx/layers/wr/WebRenderLayerManager.cpp
@@ +188,4 @@
>  
> +  bool updated = mWebRenderCommandBuilder.EmptyTransaction();
> +
> +  if (aFlags & EndTransactionFlags::END_NO_COMPOSITE && !updated) {

Do we need to call mWebRenderCommandBuilder.EmptyTransaction() after BeginTransaction() ? Because it would be more efficient to call mWebRenderCommandBuilder.EmptyTransaction() first, and then if it returns false (and we don't need a composite), we just call SendSetFocusTarget and return. That way we don't unnecessarily call BeginTransaction and CancelTransaction in the case where there are no canvases.

Comment 20

[Sotaro Ikeda [:sotaro]]

Thanks for the advice :) I am going to update patches.

Comment 21

[Sotaro Ikeda [:sotaro]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #19)
> 
> Do we need to call mWebRenderCommandBuilder.EmptyTransaction() after
> BeginTransaction() ?

Yes, it is necessary to mWebRenderCommandBuilder.EmptyTransaction() after WebRenderBridgeChild::BeginTransaction(). It is necessary for handling Compositable transaction related stuffs.

Comment 22

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Push canvas updates to the compositor on empty transactions

Comment 23

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Suppress to composite when END_NO_COMPOSITE is set

Comment 24

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8917225 [details] [diff] [review]
patch part 1 - Push canvas updates to the compositor on empty transactions

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

::: gfx/layers/wr/WebRenderCommandBuilder.cpp
@@ +37,5 @@
> +    RefPtr<WebRenderCanvasData> canvasData = iter.Get()->GetKey();
> +    WebRenderCanvasRendererAsync* canvas = canvasData->GetCanvasRenderer();
> +    canvas->UpdateCompositableClient();
> +  }
> +}

I wonder if this code should move out of the CommandBuilder

Comment 25

[Sotaro Ikeda [:sotaro]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #24)
> Comment on attachment 8917225 [details] [diff] [review]
> patch part 1 - Push canvas updates to the compositor on empty transactions
> 
> Review of attachment 8917225 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/wr/WebRenderCommandBuilder.cpp
> @@ +37,5 @@
> > +    RefPtr<WebRenderCanvasData> canvasData = iter.Get()->GetKey();
> > +    WebRenderCanvasRendererAsync* canvas = canvasData->GetCanvasRenderer();
> > +    canvas->UpdateCompositableClient();
> > +  }
> > +}
> 
> I wonder if this code should move out of the CommandBuilder

This could be done in a different bug.

Comment 26

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #25)
> > 
> > Review of attachment 8917225 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/layers/wr/WebRenderCommandBuilder.cpp
> > @@ +37,5 @@
> > > +    RefPtr<WebRenderCanvasData> canvasData = iter.Get()->GetKey();
> > > +    WebRenderCanvasRendererAsync* canvas = canvasData->GetCanvasRenderer();
> > > +    canvas->UpdateCompositableClient();
> > > +  }
> > > +}
> > 
> > I wonder if this code should move out of the CommandBuilder
> 
> This could be done in a different bug.

I implemented the above in CommandBuilder just since CanvasDataSet and mLastCanvasDatas is defined in CommandBuilder.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/86c487f84974
Push canvas updates to the compositor on empty transactions r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/077b6ec36d84
Suppress to composite when END_NO_COMPOSITE is set r=jrmuizel

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/86c487f84974
https://hg.mozilla.org/mozilla-central/rev/077b6ec36d84