Allow blocklisting of Canvas 2D acceleration

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: milan, Assigned: milan)

Tracking

Trunk
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(4 attachments, 3 obsolete attachments)

We don't have a way of doing that.
Assignee: nobody → milan
OS: Unspecified → All
Hardware: Unspecified → All
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.
Attachment #8721501 - Flags: review?(bgirard) → review+
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+
Attachment #8721505 - Flags: review?(snorp) → review+
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?
Attachment #8721503 - Flags: review?(gwright) → review+
Attachment #8721504 - Flags: review?(mchang) → review+
(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.
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
Flags: needinfo?(milan)
Sorry, missed the red in the first try run from comment 11.
Flags: needinfo?(milan)
The third patch blocked all of 10.6 for Canvas 2D, and the test happened to be setting 10.6 as the platform.
Attachment #8723134 - Attachment is patch: true
Attachment #8723134 - Flags: review+
Attachment #8721504 - Attachment is obsolete: true
Attachment #8723134 - Attachment description: Bug 1249640: Part 3. Update Mac to use the new blocking. r=mchang → Part 3. Update Mac to use the new blocking. Carry r=mchang
Resolved problems from comment 13, see the try run in comment 19.
Keywords: checkin-needed
Depends on: 1251887
You need to log in before you can comment on or make changes to this bug.