Mason Chang [Inactive] [:mchang] Reporter
	Description • 9 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 1 • 9 years ago

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).

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 2 • 9 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 3 • 9 years ago

w/ a new flag to canvas.drawWindow()

	Mason Chang [Inactive] [:mchang] Reporter
	Comment 5 • 9 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

	Mason Chang [Inactive] [:mchang] Reporter
	Comment 6 • 9 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.

	Mason Chang [Inactive] [:mchang] Reporter
	Comment 7 • 9 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?

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 8 • 9 years ago

(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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 9 • 9 years ago

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.

	Matt Woodrow (:mattwoodrow)
	Comment 10 • 9 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 11 • 9 years ago

w/ new API

	Matt Woodrow (:mattwoodrow)
	Comment 12 • 9 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 13 • 9 years ago

(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.

	Matt Woodrow (:mattwoodrow)
	Comment 14 • 9 years ago

(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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 15 • 9 years ago

Comment on attachment 8612641 [details] [diff] [review]
windows patch

r? from a DOM peer for a chrome-only API addition to CanvasRenderingContext2D.webidl

	Pulsebot
	Comment 16 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/31fb1f0e59c1

	Carsten Book [:Tomcat]
	Comment 17 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/31fb1f0e59c1

	Florian Scholz (Open Web Docs)
	Comment 18 • 9 years ago

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

	Florian Scholz (Open Web Docs)
	Comment 19 • 9 years ago

Added https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/drawWidgetAsOnScreen