1294812 - No need for accelerated canvas when the layers are not accelerated

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

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

Don't want SkiaGL canvas, when using basic layers.

Comment 1

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

I imagine this has been the case since 42, but I only tried (and verified) in 47 and later.

Comment 2

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

Attachment: Bug 1294812: Clean up SkiaGL canvas logic, making sure we only take it for OpenGL layers. Rename a function for clarity.

Review commit: https://reviewboard.mozilla.org/r/71338/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71338/

Comment 3

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

This is a minimal patch, appropriate for uplift, if we decide to.  The real one (follow up bug?) probably includes using gfxConfig.

Comment 4

[Benoit Girard (:BenWa)]

Comment on attachment 8780681 [details]
Bug 1294812: Clean up SkiaGL canvas logic, making sure we only take it for OpenGL layers. Rename a function for clarity.

https://reviewboard.mozilla.org/r/71338/#review68886

::: gfx/thebes/gfxPlatform.cpp:1252
(Diff revision 1)
>  bool gfxPlatform::UseAcceleratedCanvas()
>  {
>    // Allow acceleration on Skia if the preference is set, unless it's blocked
>    if (mPreferredCanvasBackend == BackendType::SKIA && gfxPrefs::CanvasAzureAccelerated()) {
> +
> +    if (!gfxConfig::GetFeature(Feature::HW_COMPOSITING).IsEnabled()) {

This doesn't check if the context has HW compositing to the screen. For instance we can have a basic pop-up panels with a canvas (add-ons like to create those panels). Perhaps this is something we'd prefer. We'd have to check the LayerManager when creating the context.

It looks like HW_COMPOSITING just checks if any compositor would be allowed (you didn't flip layers.accelerated.disable) and could even be true  but I'm not 100% sure. So this doesn't mean that we have an accelerated compositor.

A safemode check here could be pretty simple if that's all we want to protect against.

Comment 5

[Benoit Girard (:BenWa)]

(In reply to Milan Sreckovic [:milan] from comment #3)
> This is a minimal patch, appropriate for uplift

I missed this part. If it works, particularly in safe mode, I wouldn't object to taking it as-is.

Comment 6

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

(In reply to Benoit Girard (:BenWa) from comment #5)
> (In reply to Milan Sreckovic [:milan] from comment #3)
> > This is a minimal patch, appropriate for uplift
> 
> I missed this part. If it works, particularly in safe mode, I wouldn't
> object to taking it as-is.

Safe mode is part of HW_COMPOSITING - https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatform.cpp#2180

Otherwise, you're right - this only checks the preferences, the safe mode and the possible startup crash.  Let me see if there is something better.

Comment 7

[Benoit Girard (:BenWa)]

Comment on attachment 8780681 [details]
Bug 1294812: Clean up SkiaGL canvas logic, making sure we only take it for OpenGL layers. Rename a function for clarity.

https://reviewboard.mozilla.org/r/71338/#review71260

I'll give this a r+ in case we decide to land this as-is given what was identified here.

Comment 8

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

I'll land the simple patch, that at least covers majority of the cases.  Things are better, though not perfect.

Comment 9

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

Comment on attachment 8780681 [details]
Bug 1294812: Clean up SkiaGL canvas logic, making sure we only take it for OpenGL layers. Rename a function for clarity.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71338/diff/1-2/

Comment 10

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

(In reply to Milan Sreckovic [:milan] from comment #9)
> Comment on attachment 8780681 [details]
> Bug 1294812: Canvas shouldn't be accelerated if composition isn't.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/71338/diff/1-2/

Use the layer manager type instead.

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8780681 [details]
Bug 1294812: Clean up SkiaGL canvas logic, making sure we only take it for OpenGL layers. Rename a function for clarity.

https://reviewboard.mozilla.org/r/71338/#review80924

::: gfx/thebes/gfxPlatform.cpp:1261
(Diff revision 2)
>    // Allow acceleration on Skia if the preference is set, unless it's blocked
> -  if (mPreferredCanvasBackend == BackendType::SKIA && gfxPrefs::CanvasAzureAccelerated()) {
> +  // as long as we have the accelerated layers
> +  if (mPreferredCanvasBackend == BackendType::SKIA &&
> +      gfxPrefs::CanvasAzureAccelerated() &&
> +      (mCompositorBackend != LayersBackend::LAYERS_NONE &&
> +       mCompositorBackend != LayersBackend::LAYERS_BASIC)) {

We should be querying the canvas backend based on a specific compositor, not the assumed compositor type. [1] is how we do this for content.

That said, this is strictly more accurate than what we have now, so r=me.

nit: Rather than have an empty line for readability, can you move the { down to a new line? The style guide does not have any rule for this case, but super-complex conditions aren't that uncommon. SpiderMonkey adopted that exception, I've been using it in gfx where appropriate. No one has called me on it yet (I hope).

[1] http://searchfox.org/mozilla-central/rev/d1276b5b84e6cf7991c8e640b5e0ffffd54575a6/gfx/thebes/gfxPlatform.h#296

Comment 12

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

Comment on attachment 8780681 [details]
Bug 1294812: Clean up SkiaGL canvas logic, making sure we only take it for OpenGL layers. Rename a function for clarity.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71338/diff/2-3/

Comment 13

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

(In reply to Milan Sreckovic [:milan] from comment #12)
> Comment on attachment 8780681 [details]
> Bug 1294812: Clean up SkiaGL canvas logic, making sure we only take it for
> OpenGL layers. Rename a function for clarity.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/71338/diff/2-3/

This has changed enough that I'd rather not carry r+, but I'm not sure how to re-ask for it in reviewboard, so, David, when you have a chance, please take a look at it (again.)

Comment 14

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8780681 [details]
Bug 1294812: Clean up SkiaGL canvas logic, making sure we only take it for OpenGL layers. Rename a function for clarity.

https://reviewboard.mozilla.org/r/71338/#review81046

The previous patch required that we were using an accelerated compositor. This one requires that we have an OPENGL compositor (i.e. we're not Windows), and that skia content is enabled. Is that intended?

Comment 15

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

(In reply to David Anderson [:dvander] from comment #14)
> Comment on attachment 8780681 [details]
> Bug 1294812: Clean up SkiaGL canvas logic, making sure we only take it for
> OpenGL layers. Rename a function for clarity.
> 
> https://reviewboard.mozilla.org/r/71338/#review81046
> 
> The previous patch required that we were using an accelerated compositor.
> This one requires that we have an OPENGL compositor (i.e. we're not
> Windows), and that skia content is enabled. Is that intended?

Right now, yes.  Currently we only have and want SkiaGL with the OpenGL compositor.  I think I'd rather make an explicit decision if that ever changes.

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

Ok, thanks for explaining - looks good.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f9dfdb9971c4
Clean up SkiaGL canvas logic, making sure we only take it for OpenGL layers. Rename a function for clarity. r=BenWa,dvander

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f9dfdb9971c4

Comment 19

[Andrei Vaida [:avaida]]

Milan, should manual QA be looking at this?

Comment 20

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

It may be worth looking at OS X, with hardware acceleration disabled, and how the canvas behaves before and after this change.

Comment 21

[Andrei Vaida [:avaida]]

(In reply to Milan Sreckovic [:milan] from comment #20)
> It may be worth looking at OS X, with hardware acceleration disabled, and
> how the canvas behaves before and after this change.

I see, then I think we can treat this as regular bug work and just plan a testing session on OS X for it. Are there any demo pages we could use to make sure we're testing the right type of content?

Comment 22

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

Google sheets use canvas, that's probably the easiest test.

Comment 23

[Gerry Chang [:gchang]]

Mark 51 won't fix as this is a new feature to 51.

Comment 24

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

It's actually complicated :)

Technically, this affects 51 - we are setting the acceleration when we shouldn't be.  However, because of bug 1309913, even when acceleration is set, we don't actually accelerate, which means that this is not actually a problem in 51.

Once we fix 1309913, which we should do in 51, this particular bug will get fixed in 51 as well.

Comment 25

[Mihai Boldan, Desktop QA [:mboldan]]

I managed to perform some tests on Firefox Latest Nightly under Mac OS X 10.11.6.
The tests were performed whit the Hardware acceleration Enabled (AzureCanvasAccelerated-1) and Disabled (AzureCanvasAccelerated-0), using google sheets and a couple of canvas demo pages.
No differences were spotted during testing. 

Please let me know if there are any other tests that need to be covered for this fix.

Also, I just wanted to be sure that this fix affected only the Mac OS X OSs and it's expected that on Windows and Ubuntu OSs that the value of AzureCanvasAccelerated not to modify when enabling/disabling the hardware acceleration option.

Comment 26

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

This is OS X only, but until bug 1309913 is fixed, the canvas acceleration setting is ignored - we never get accelerated canvas, so we're not really testing what needs to be tested.

Comment 27

[Mihai Boldan, Desktop QA [:mboldan]]

Hello Milan,

I noticed that Bug 1309913 was fixed, so I assume that is safe to retest this bug.
Can you please confirm this?

Thanks,
Mihai

Comment 28

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

Yes, it is safe to retest, thanks for the reminder.

Comment 29

[Paul Silaghi, QA [:pauly]]

(In reply to Milan Sreckovic [:milan] from comment #20)
> It may be worth looking at OS X, with hardware acceleration disabled, and
> how the canvas behaves before and after this change.
I haven't seen any differences on google sheets between 52.0a1 (2016-10-05) and 52.0a1 (2016-11-04), OS X 10.11 and 10.12.

I noticed that unchecking the "Use hardware acceleration when available" pref in Firefox Preferences leaves the 'gfx.canvas.azure.accelerated' still on TRUE in about:config. Is that correct? Shouldn't set gfx.canvas.azure.accelerated=FALSE?

Comment 30

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

Right - we left those preferences independent - the value of gfx.canvas.azure.accelerated is one way to turn off acceleration in canvas, and setting it to true is a request only - overall acceleration being off, or sometimes a run time decision based on the canvas commands that are being detected are some of the reasons we will turn it off at run time.

Comment 31

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 52.0a1 (2016-11-04) based on comments 29, 30.