Add an API for reading window contents using the OS, rather than the compositor backbuffer

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mchang, Assigned: dvander)

Tracking

({dev-doc-complete})

29 Branch
mozilla41
x86
macOS
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

Bug 1156135 reads back the source surface from the canvas to verify that the pixels that are painted are what we expect. One of the TODOs listed is to use OS APIs to get a screenshot of the area and check that pixels are what we expected. Do that on OS X and Windows in this bug.

As it stands today, I'm working on it on OSX and :dvander is working on it on Windows.
Posted patch api, WIP (obsolete) — Splinter Review
this just adds API to nsIWidget/PCompositor to take snapshots. also an untested windows implementation. still have to hook this up to DrawWindow (probably just a new flag makes sense).
There's two ways to approach the actual widget implementation: (1) snapshot the entire desktop, and attempt to fish out the window in question, or (2) ask the OS for only pixels in the window. For Windows, that'd be the difference between GetDC(GetDesktopWindow()) and GetDC(mWnd).

I've heard that for (2), there are black-screen scenarios we still wouldn't catch due to seeing pixels before DWM composites them versus after. I'd like to understand what those are and if they're testable. (1) at the moment feels very risky since any sort of external application running could cause us to read back something unintended.
Flags: needinfo?(bas)
Posted patch api, WIP (obsolete) — Splinter Review
w/ a new flag to canvas.drawWindow()
Attachment #8609199 - Attachment is obsolete: true
Posted patch api, WIP (obsolete) — Splinter Review
Attachment #8609415 - Attachment is obsolete: true
(Reporter)

Comment 5

4 years ago
Comment on attachment 8609479 [details] [diff] [review]
api, WIP

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +801,5 @@
>  bool
> +CompositorParent::RecvMakeWidgetSnapshot(const SurfaceDescriptor& aInSnapshot)
> +{
> +  RefPtr<DrawTarget> target = GetDrawTargetForDescriptor(aInSnapshot, gfx::BackendType::CAIRO);
> +  if (!mWidget) {

The widget usually doesn't exist because we set it to nullptr when we allocate the PLayerTransactionParent.

https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp?from=CompositorParent.cpp&case=true#1433
(Reporter)

Comment 6

4 years ago
Based on patches in attachment 8609479 [details] [diff] [review] and attachment 8608496 [details] [diff] [review] from Bug 1156135. The screenshot doesn't actually show the red tile / playing movie from Matt's addon. Investigating.
(Reporter)

Comment 7

4 years ago
I'm thinking that this API should be async rather than sync. We should composite and wait for the host OS to finish desktop window compositing. On Windows this would be DWMFlush(), not sure about Windows XP. On Mac, there isn't any clean way to synchronously wait for a screen refresh. Once we get the notification that the host OS has composited, we take a screenshot. Otherwise, we might screenshot before the contents can be rendered on the screen. What do you think David?
Flags: needinfo?(dvander)
(In reply to Mason Chang [:mchang] from comment #7)
> I'm thinking that this API should be async rather than sync. We should
> composite and wait for the host OS to finish desktop window compositing. On
> Windows this would be DWMFlush(), not sure about Windows XP. On Mac, there
> isn't any clean way to synchronously wait for a screen refresh. Once we get
> the notification that the host OS has composited, we take a screenshot.
> Otherwise, we might screenshot before the contents can be rendered on the
> screen. What do you think David?

Async seems fine. I'll take a stab at it tomorrow morning.
Flags: needinfo?(dvander)
Posted patch windows patch (obsolete) — Splinter Review
This is the synchronous API w/ the flaws pointed out by Mason fixed. We're still not sure yet whether the OS X API will work so I kept the synchronous API for now.

Appears to work with Matt's addon on my machine but probably needs more testing.
Attachment #8609479 - Attachment is obsolete: true
Attachment #8610954 - Flags: review?(matt.woodrow)
Comment on attachment 8610954 [details] [diff] [review]
windows patch

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

Impl all looks good, just a little worried about the drawWindow API.

::: dom/interfaces/canvas/nsIDOMCanvasRenderingContext2D.idl
@@ +31,5 @@
>    // Don't synchronously decode images - draw what we have
>    const unsigned long DRAWWINDOW_ASYNC_DECODE_IMAGES = 0x10;
> +  // Rather than use the compositor's snapshot, read from the operating
> +  // system's view of the window instead.
> +  const unsigned long DRAWWINDOW_USE_SCREEN_IMAGE = 0x20;

This API is getting a bit confusing.

DRAWWINDOW_USE_WIDGET_LAYERS has the weird contract that it's only actually respected if the other flags you pass are compatible with it and the window size is <= the screen size. If it doesn't get used, then you get no notifications or errors, it just renders using a temporary (software) layer manager instead.

DRAWWINDOW_USE_SCREEN_IMAGE now has basically the opposite, in that it always succeeds and ignores the other flags if they're not compatible. If the window is bigger than the screen, then the resulting snapshot will just be missing the overhanging bits.

Neither of these seem particularly nice to work with.

It's an internal only API so not a huge deal, but it's probably best to just add a new function call for snapshotting the screen image and then we can not worry about what it might mean to ask for a drawWindow with USE_SCREEN_IMAGE but not DRAW_CARET and so on.
Posted patch windows patchSplinter Review
w/ new API
Attachment #8610954 - Attachment is obsolete: true
Attachment #8610954 - Flags: review?(matt.woodrow)
Attachment #8612641 - Flags: review?(matt.woodrow)
Comment on attachment 8612641 [details] [diff] [review]
windows patch

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

::: browser/app/profile/extensions/sanity-gfx@mozilla.org/bootstrap.js
@@ +92,5 @@
>    // TODO: When Bug 1151611 lands we can test hardware video decoding status
>    // directly instead of checking the pref.
>    if (!testVideoRendering(ctx)) {
>      reportResult(TEST_FAILED_VIDEO);
> +    if (Preferences.get("media.hardware-video-decoding.enabled", false)) {

Oops.

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +4847,5 @@
> +    error.Throw(NS_ERROR_FAILURE);
> +    return;
> +  }
> +
> +  RefPtr<DataSourceSurface> data = snapshot->GetDataSurface();

Why do we need to convert to a data surface and then back into a source surface? mTarget->DrawSurface(snapshot) should just work.
Attachment #8612641 - Flags: review?(matt.woodrow) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #12)
> ::: dom/canvas/CanvasRenderingContext2D.cpp
> @@ +4847,5 @@
> > +    error.Throw(NS_ERROR_FAILURE);
> > +    return;
> > +  }
> > +
> > +  RefPtr<DataSourceSurface> data = snapshot->GetDataSurface();
> 
> Why do we need to convert to a data surface and then back into a source
> surface? mTarget->DrawSurface(snapshot) should just work.

Yup you're right, that does work. Fixed locally.

So this patch is successfully capturing stuff off the screen, the problem is by the time we've composited, the window isn't actually painted by the OS yet. It looks like the simplest thing to do is to fire an OS-specific event/notification (unless one is already available - I couldn't see anything obvious), and have the addon only do its tests once that completes. That'll mean it tests a little later, but it won't block startup while waiting.
(In reply to David Anderson [:dvander] from comment #13)
> 
> So this patch is successfully capturing stuff off the screen, the problem is
> by the time we've composited, the window isn't actually painted by the OS
> yet. It looks like the simplest thing to do is to fire an OS-specific
> event/notification (unless one is already available - I couldn't see
> anything obvious), and have the addon only do its tests once that completes.
> That'll mean it tests a little later, but it won't block startup while
> waiting.

That sounds like the right solution.
Assignee: mchang → dvander
Status: NEW → ASSIGNED
Summary: Use OS API snapshotting to verify graphics runtime features rather than reading back the source surface → Add an API for reading window contents using the OS, rather than the compositor backbuffer
Comment on attachment 8612641 [details] [diff] [review]
windows patch

r? from a DOM peer for a chrome-only API addition to CanvasRenderingContext2D.webidl
Attachment #8612641 - Flags: review?(khuey)
https://hg.mozilla.org/mozilla-central/rev/31fb1f0e59c1
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
No longer depends on: 1156135
ddn info:
Chrome-only API addition to CanvasRenderingContext2D

/**
 * Render the root widget of a window into the canvas. Unlike drawWindow,
 * this uses the operating system to snapshot the widget on-screen, rather
 * than reading from our own compositor.
 *
 * Currently, this is only supported on Windows, and only on widgets that
 * use OMTC, and only from within the chrome process.
 */
[Throws, ChromeOnly]
void drawWidgetAsOnScreen(Window window);
TBD:
https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/drawWidgetAsOnScreen
See also 
https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/drawWindow
Keywords: dev-doc-needed
Flags: needinfo?(bas)
You need to log in before you can comment on or make changes to this bug.