1437949 - wake_up implementation should poke the renderer to check for messages

Status: RESOLVED FIXED

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

Description

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

The WR capturing mechanism relies on the wake_up callback [1] to process the DebugOutput message in the renderer's queue, which writes external images to files. Right now the wake_up callback is implemented as a no-op [2] when really what it should do is trigger a call to wr_renderer_update on the Renderer thread.

[1] https://searchfox.org/mozilla-central/rev/9d47b5fb14152865ad1375eb3ee2e571d81ecdb9/gfx/webrender/src/render_backend.rs#644
[2] https://searchfox.org/mozilla-central/rev/9d47b5fb14152865ad1375eb3ee2e571d81ecdb9/gfx/webrender_bindings/RenderThread.cpp#468

Comment 1

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

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

Comment 2

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

Attachment: Bug 1437949 - Make the wake_up WR notification poke the renderer.

Review commit: https://reviewboard.mozilla.org/r/219916/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219916/

Comment 3

[Dzmitry Malyshau [:kvark]]

Comment on attachment 8950662 [details]
Bug 1437949 - Make the wake_up WR notification poke the renderer.

https://reviewboard.mozilla.org/r/219916/#review225726

::: gfx/webrender_bindings/RenderThread.cpp:187
(Diff revision 1)
> +    return;
> +  }
> +
> +  if (!IsInRenderThread()) {
> +    Loop()->PostTask(
> +      NewRunnableMethod<wr::WindowId>("wr::RenderThread::WaekUp",

typo "WaekUp"

Comment 4

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

(In reply to Dzmitry Malyshau [:kvark] from comment #3)
> typo "WaekUp"

Good catch! Fixed.

Comment 5

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

Comment on attachment 8950662 [details]
Bug 1437949 - Make the wake_up WR notification poke the renderer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219916/diff/1-2/

Comment 6

[Sotaro Ikeda [:sotaro]]

Don't we need to call MakeCurrent() in RendererOGL::Update(), do we?

Comment 7

[Dzmitry Malyshau [:kvark]]

Good point, Sotaro. WebRender may need to upload some texture data, so GL context is needed to be valid.

Comment 8

[Sotaro Ikeda [:sotaro]]

Yea, and GL context needs to be current if there are multiple windows(multiple gl contexts).

Comment 9

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

Good point. I already triggered autolanding of this patch (and there's no way to stop it) but after it lands I'll push a follow-up to add the MakeCurrent call.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/37935fa7ffc2
Make the wake_up WR notification poke the renderer. r=kvark

Comment 11

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/37935fa7ffc2

Comment 12

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

Attachment: Follow-up to do MakeCurrent

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/16fab079284b
Follow-up to make the GL context current before poking the WR renderer. r=sotaro

Comment 14

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/16fab079284b