Closed Bug 1682026 Opened 4 years ago Closed 4 years ago

Switch SanityTest.jsm takeWindowSnapshot from drawWindow to drawSnapshot for Fission

Categories

(Core :: Graphics, task)

task

Tracking

()

RESOLVED WONTFIX
Fission Milestone M7

People

(Reporter: smacleod, Unassigned)

References

(Blocks 1 open bug)

Details

CanvasRenderingContext2D.drawWindow() is synchronous and won't work with fission, so SanityTest.jsm[1] should be updated to use the asynchronous drawSnapshot[2]

  1. https://searchfox.org/mozilla-central/rev/0bcf81557b89e7757c44e25bb4bc7f4cb8619dc9/toolkit/components/gfx/SanityTest.jsm#112
  2. https://searchfox.org/mozilla-central/rev/9b282b34b5/dom/chrome-webidl/WindowGlobalActors.webidl#81-98

Tracking for Fission Beta milestone (M7).

Fission Milestone: --- → M7

SanityTest.jsm is used for checking Compositor capability. DrawSnapshot() seems not getting a result of Compositor compositing. With WebRender, .drawWindow() is not called, since RenderCompositorANGLE::MaybeReadback() is too slow.

:mattwoodrow, can you comment to this bug?

Flags: needinfo?(matt.woodrow)

Function call is like the following
->CanvasRenderingContext2D::DrawWindow()
->PresShell::RenderDocument()
->nsLayoutUtils::PaintFrame()
->nsDisplayList::PaintRoot()
->ClientLayerManager::BeginTransactionWithTarget()
->>>>>
->ClientLayerManager::EndTransaction()
->ClientLayerManager::MakeSnapshotIfRequired()
->CompositorBridgeChild::SendMakeSnapshot()
->CompositorBridgeParent::RecvMakeSnapshot()
->CompositorBridgeParent::ForceComposeToTarget()

TL;DR: I think we should keep using drawWindow for SanityTest.

drawWindow has two different 'modes' (along with a lot of other options and flags).

If the caller is in the parent process, the window is top-level, requested caret mode is default, and the USE_WIDGET_LAYERS flag is passed:
      Send a message to the compositor, and do a readback of the pixels that we're presenting to the OS which includes the document and all subdocs (OOP or not), but only includes pixels that are actually visible.
Otherwise:
      Create a custom rendering of the document and all in-process subdocuments, using a temporary software renderer.

drawSnapshot is basically the same as the latter of these, except that it's async, and includes OOP subdocuments.

Code using the latter mode of drawWindow generally should be switched to drawSnapshot, so that it can include OOP subdocs and be correct with fission.

SanityTest is using the former mode, where we really want the readback mode, so we can verify that the pixels presented to the OS are correct. Switching to drawSnapshot will create an image using a software renderer and won't test WebRender or any of the OS integration.

Creating a new API that more explicitly handles the former case of drawWindow would probably be a good idea (and could be async). Then we can truly remove drawWindow once all callers have been moved to drawSnapshot or the new readback API.

Also, given that we just skip SanityTest with WebRender, we should consider fixing that, or just removing the whole thing (since WR is >75% of our users now).

Flags: needinfo?(matt.woodrow)

(In reply to Sotaro Ikeda [:sotaro] from comment #2)

since RenderCompositorANGLE::MaybeReadback() is too slow.

IIRC, there might be a plan to make Snapshot of os compositor on Windows fast.

:gw, is there a plan to do it?

Flags: needinfo?(gwatson)

There are some vague ideas to make it fast - the idea is that we implement the Draw compositor in terms of the native compositor trait (this is useful for other reasons, including simplifying WR code), and could then use that when screenshots are enabled, rather than drawing to the native surfaces.

However, there may be some unknowns that complicate this plan, and it's not currently on anyone's roadmap to implement, as far as I know.

Flags: needinfo?(gwatson)

For the purposes of both SanityTest, and the reftest harness, this plan doesn't sound great, since it excludes the whole native compositing layer from being testable.

For screenshots for the profiler (where performance matters more than catching rendering bugs), it sounds great!

Severity: -- → S3

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)

TL;DR: I think we should keep using drawWindow for SanityTest.

Should we WONTFIX this bug then?

Flags: needinfo?(matt.woodrow)
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: needinfo?(matt.woodrow)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.