1249640 - Allow blocklisting of Canvas 2D acceleration

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

We don't have a way of doing that.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Since this is a new feature for blocklisting, and only releases after 41 can deal with features they don't understand, we should make sure this does not get used in the downloadable blocklist until 38 is off the books.  Which is around 47, so we should resist the temptation to uplift this.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 1. A way to block canvas 2D acceleration from blocklists. r=benwa

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 2. gfxPlatform simplification when it comes to accelerated canvas, using the new blocking. r=gw280

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Bug 1249640: Part 3. Update Mac to use the new blocking. r=mchang

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Bug1249640: Part 4. Android to use new blocking. r=snorp

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8721505 [details] [diff] [review]
Bug1249640: Part 4. Android to use new blocking. r=snorp

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

::: widget/android/GfxInfo.cpp
@@ +188,3 @@
>  
>    // the HARDWARE field isn't available on Android SDK < 8
> +  if (mSDKVersion >= 8 && mozilla::AndroidBridge::Bridge()->GetStaticStringField("android/os/Build", "HARDWARE", mHardware)) {

You can remove the SDK version check, we only run on 9+ these days, and soon (like this week?) it will be 15+

Comment 7

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 8721503 [details] [diff] [review]
Part 2. gfxPlatform simplification when it comes to accelerated canvas, using the new blocking. r=gw280

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

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +1205,2 @@
>  {
>    if (!IsTargetValid() || mRenderingMode == aRenderingMode) {

We can probably remove the check for the current rendering mode now. In any case, I don't think it makes sense to return false if we are already set to the right rendering mode.

::: gfx/thebes/gfxPlatform.h
@@ +268,5 @@
>      static bool AsyncPanZoomEnabled();
>  
>      virtual void GetAzureBackendInfo(mozilla::widget::InfoObject &aObj) {
>        aObj.DefineProperty("AzureCanvasBackend", GetBackendName(mPreferredCanvasBackend));
> +      aObj.DefineProperty("AzureSkiaAccelerated", UseAcceleratedCanvas());

Should we remove "Skia" here?

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to George Wright (:gw280) (:gwright) from comment #7)
> Comment on attachment 8721503 [details] [diff] [review]
> Part 2. gfxPlatform simplification when it comes to accelerated canvas,
> using the new blocking. r=gw280
> 
> Review of attachment 8721503 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/CanvasRenderingContext2D.cpp
> @@ +1205,2 @@
> >  {
> >    if (!IsTargetValid() || mRenderingMode == aRenderingMode) {
> 
> We can probably remove the check for the current rendering mode now. In any
> case, I don't think it makes sense to return false if we are already set to
> the right rendering mode.

Right - having it return false when we're already set to the right rendering mode, so that Demote*() methods don't end up calling RemoveDemotableContext().  And then that had me check before calling in this place, so that we don't emit gfxDebug() because we wouldn't actually be failing.  So, the return value means "did the back end change", rather than "is the back end match the argument after return".
> 
> ::: gfx/thebes/gfxPlatform.h
> @@ +268,5 @@
> >      static bool AsyncPanZoomEnabled();
> >  
> >      virtual void GetAzureBackendInfo(mozilla::widget::InfoObject &aObj) {
> >        aObj.DefineProperty("AzureCanvasBackend", GetBackendName(mPreferredCanvasBackend));
> > +      aObj.DefineProperty("AzureSkiaAccelerated", UseAcceleratedCanvas());
> 
> Should we remove "Skia" here?

Good question.  I'm not sure if we're using this as telemetry data somewhere, I'll check.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 2. gfxPlatform simplification when it comes to accelerated canvas, using the new blocking. Carry r=gw280

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 4. Android to use new blocking. Carry r=snorp

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=815c44428610
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2037ced62148

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ca40a403e9d
https://hg.mozilla.org/integration/mozilla-inbound/rev/303b759e886e
https://hg.mozilla.org/integration/mozilla-inbound/rev/8151f4102646
https://hg.mozilla.org/integration/mozilla-inbound/rev/21222476d9d9

Comment 13

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

Had to back these out for XPCshell failures on OS X:

Backout:

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=21222476d9d9
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=22279987&repo=mozilla-inbound

07:42:44     INFO -  TEST-START | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_Device.js
07:42:45  WARNING -  TEST-UNEXPECTED-FAIL | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_Device.js | xpcshell return code: 0

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

Sorry, missed the red in the first try run from comment 11.

Comment 15

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

> Backout:
That's https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=80baf2babce0

Comment 16

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf721b828cce
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c971e22e81c
https://hg.mozilla.org/integration/mozilla-inbound/rev/901a3b751970

Comment 17

[Milan Sreckovic [:milan] (needinfo for best results)]

The third patch blocked all of 10.6 for Canvas 2D, and the test happened to be setting 10.6 as the platform.

Comment 18

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 3. Update Mac to use the new blocking. Carry r=mchang

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

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

Comment 20

[Milan Sreckovic [:milan] (needinfo for best results)]

Resolved problems from comment 13, see the try run in comment 19.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/67e2660b7478
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bfbed61f70c
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd3df45a85da
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e882a4bf894

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/67e2660b7478
https://hg.mozilla.org/mozilla-central/rev/7bfbed61f70c
https://hg.mozilla.org/mozilla-central/rev/dd3df45a85da
https://hg.mozilla.org/mozilla-central/rev/1e882a4bf894