1061988 - Get rid of the notion of "demoting" and replace it with "switch rendering mode"

Status: NEW

(Core :: Graphics: Canvas2D, defect)

Description

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

Related to bug 1042291 - we need to be able to go both ways if necessary (ie, from sw->gpu and gpu->sw). The word "demote" is really terrible anyway as it's meaningless in the context of switching the rendering backend.

Let's do away with the "demote" junk and replace it with a rendering mode that can be switched on the fly.

Comment 1

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

Attachment: 0001-Bug-1061988-Get-rid-of-the-notion-of-demoting-and-re.patch

How does this look? I'm a little concerned about the enum naming, and the fact that we have to keep the Demote() API alive (for now at least), but the rest I think makes sense.

Comment 2

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

Comment on attachment 8483116 [details] [diff] [review]
0001-Bug-1061988-Get-rid-of-the-notion-of-demoting-and-re.patch

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

Looks good if you fix the one comment

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +791,2 @@
>  
> +  mRenderingMode = aRenderingMode;

Set this after you create the target. That way if it fails, this is still correct.

Comment 3

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

Attachment: 0001-Bug-1061988-Get-rid-of-the-notion-of-demoting-and-re.patch

Comment 4

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

Comment on attachment 8483603 [details] [diff] [review]
0001-Bug-1061988-Get-rid-of-the-notion-of-demoting-and-re.patch

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

I think we can do better with the software fallback logic here, so let's do one more pass.

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +777,3 @@
>  {
> +  if (!IsTargetValid() || mRenderingMode == aRenderingMode)
> +    return false;

Please add braces

@@ +780,3 @@
>  
> +  if (aRenderingMode == RenderingMode::SoftwareBackendMode &&
> +      !mStream) {

So in this case, mRenderingMode == OpenGLBackendMode (or whatever), but it failed to create an OpenGL context, so we have no SurfaceStream, and have fallen back to a software backend. I think we should just set mRenderingMode to SoftwareBackend in that case and you can remove this check.

@@ +791,3 @@
>  
> +  // Recreate target using the new rendering mode
> +  EnsureTarget(aRenderingMode);

EnsureTarget could return the mode it actually ended up using. So if creating the GL backend fails for whatever reason, this would return SoftwareBackend. You would set mRenderingMode to the result of this.

@@ +794,1 @@
>    if (!IsTargetValid())

Though only after this passes, annoyingly. I guess you could have EnsureTarget return some "null" (None? Invalid?) mode when it fails.

Comment 5

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

Attachment: 0001-Bug-1061988-Get-rid-of-the-notion-of-demoting-and-re.patch

Comment 6

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

Comment on attachment 8484256 [details] [diff] [review]
0001-Bug-1061988-Get-rid-of-the-notion-of-demoting-and-re.patch

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

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +932,5 @@
>      return;
>    }
>  
> +  // This would make no sense, so make sure we don't get ourselves in a mess
> +  MOZ_ASSERT(mRenderingMode != RenderingMode::DefaultBackendMode);

This assertion should be true in general, not just as a prerequisite to calling EnsureTarget(), right?  Wonder if it'd be helpful to assert this more often and document it.

@@ +1136,5 @@
>  
>    if (Preferences::GetBool("gfx.canvas.willReadFrequently.enable", false)) {
>      // Use software when there is going to be a lot of readback
> +    if (attributes.mWillReadFrequently) {
> +      mRenderingMode = RenderingMode::SoftwareBackendMode;

This code (same as before this change) assumes something about the state of the canvas, otherwise we couldn't just do this, and we'd have to have called Demote(), or now the equivalent SwitchRenderingMode(...Software...), right?

What are those assumptions?  That there is no valid target?  I understand this method may only be called when those assumptions are correct, but since we're directly setting mRenderingMode, it seems like it'd be worth documenting and/or asserting what those assumptions are?

Comment 7

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

(In reply to Milan Sreckovic [:milan] from comment #6)
> > +  // This would make no sense, so make sure we don't get ourselves in a mess
> > +  MOZ_ASSERT(mRenderingMode != RenderingMode::DefaultBackendMode);
> 
> This assertion should be true in general, not just as a prerequisite to
> calling EnsureTarget(), right?  Wonder if it'd be helpful to assert this
> more often and document it.

I think EnsureTarget is about as often as you can assert it, as every draw call calls EnsureTarget to make sure there's a target to draw to. As only EnsureTarget actually uses mRenderingMode as a value to change its behaviour (rather than elsewhere, where it's just set), I think this is the right place for it.

> @@ +1136,5 @@
> >  
> >    if (Preferences::GetBool("gfx.canvas.willReadFrequently.enable", false)) {
> >      // Use software when there is going to be a lot of readback
> > +    if (attributes.mWillReadFrequently) {
> > +      mRenderingMode = RenderingMode::SoftwareBackendMode;
> 
> This code (same as before this change) assumes something about the state of
> the canvas, otherwise we couldn't just do this, and we'd have to have called
> Demote(), or now the equivalent SwitchRenderingMode(...Software...), right?
> 
> What are those assumptions?  That there is no valid target?  I understand
> this method may only be called when those assumptions are correct, but since
> we're directly setting mRenderingMode, it seems like it'd be worth
> documenting and/or asserting what those assumptions are?

My understanding is that SetContextOptions is called before any drawing is done, and so we know that there is no current target for it. Calling SwitchRenderingMode or EnsureTarget here would be bad, I suspect, as it would cause us to prematurely create a DrawTarget which may never be drawn to.

Comment 8

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

That being said, I agree that we should add something like MOZ_ASSERT(!mTarget). What do you think, snorp?

Comment 9

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

Comment on attachment 8484256 [details] [diff] [review]
0001-Bug-1061988-Get-rid-of-the-notion-of-demoting-and-re.patch

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

You need to set mRenderingMode = Software in a a couple fallback cases in EnsureTarget

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +926,3 @@
>  }
>  
>  void

Pretty sure this doesn't build? Make sure you return the right thing for the various software fallback cases here too.

Comment 10

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

Hah, oops, I forgot to add the returns to EnsureTarget.

Comment 11

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

Attachment: 0001-Bug-1061988-Get-rid-of-the-notion-of-demoting-and-re.patch

OK, I think this should cater for the fallbacks. The !mStream case was removed entirely from the first version of this patch because aRenderingMode is already set to Software in that case, so it'll fall through and try to EnsureTarget with that.

I also added the assert in SetContextOptions as per Milan's concerns.

Comment 12

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

Comment on attachment 8484476 [details] [diff] [review]
0001-Bug-1061988-Get-rid-of-the-notion-of-demoting-and-re.patch

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

This is tricky, but we're getting closer.

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +930,3 @@
>  {
>    if (mTarget) {
> +    return mRenderingMode;

This is fine, but I wonder if we should put the switching logic here instead? Right now if you call EnsureTarget() and get back a mode other than the one you passed in, you don't know if it was due to a failure or just an existing DT with that mode.

@@ +934,5 @@
>  
> +  // This would make no sense, so make sure we don't get ourselves in a mess
> +  MOZ_ASSERT(mRenderingMode != RenderingMode::DefaultBackendMode);
> +
> +  RenderingMode mode = (aRenderingMode == RenderingMode::DefaultBackendMode) ? mRenderingMode : aRenderingMode;

Should we disallow OpenGL mode if !UseAcceleratedSkiaCanvas()?

@@ +956,3 @@
>  
>       if (layerManager) {
>        if (gfxPlatform::GetPlatform()->UseAcceleratedSkiaCanvas() &&

Maybe the initial rendering mode should be set according to UseAcceleratedSkiaCanvas(), then you could remove this from the condition.

@@ +971,4 @@
>              AddDemotableContext(this);
>            } else {
>              printf_stderr("Failed to create a SkiaGL DrawTarget, falling back to software\n");
> +            mode = RenderingMode::SoftwareBackendMode;

I think you need to do this for when the CheckSizeForSkiaGL() condition fails too? A few lines down. This shitty review system won't let me comment on stuff not in the patch, apparently.

Comment 13

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

Attachment: 0001-Bug-1061988-Get-rid-of-the-notion-of-demoting-and-re.patch

I don't think the switching logic should be moved into EnsureTarget(); it's already messy enough in there without adding more complexity. Refactoring EnsureTarget to be more simple is outside of the scope of this bug, imho, and we should definitely file another bug to simplify it.

That being said, I've added a further check for the early return to ensure that we forge on and create a new target if the requested mode isn't the same as our current mode. This should solidify the implication that getting back a mode that doesn't reflect the mode passed in means failure.

I also think that we need to reset mode to software mode if we fallback to CreateOffscreenCanvasDrawTarget() (ie - if there's no layerManager obj). Yep, this code is a mess.

I'll run it all through try.

Comment 14

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

Comment on attachment 8485120 [details] [diff] [review]
0001-Bug-1061988-Get-rid-of-the-notion-of-demoting-and-re.patch

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

Alright, good enough. I guess.

Comment 15

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

https://tbpl.mozilla.org/?tree=Try&rev=42027034ef34

Comment 16

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

https://tbpl.mozilla.org/?tree=Try&rev=ab76fb7b9548 should work this time.

Comment 17

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ab0aed9d824