1298345 - Simplify Canvas2D's surface allocation code

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Nicolas Silva [:nical]]

EnsureDrawTarget is pretty hairy, it's not always clear how it interacts with GetBufferProvider (depending on the order they are called), and the error state is not very well defined when it comes to maintaining the clip stack etc. Let's simplify all of this.

Comment 1

[Nicolas Silva [:nical]]

Attachment: Refactor the allocation code.

This patch splits EnsureTarget into smaller functions that are easy to understand and only apply the results if allocation succeed. It also makes sure that the error state (mTarget = sErrorTarget) is better defined (we clear the clip stack along with the rest of the drawing state to avoid getting into a situation where we manage to re-create a surface after being in the error state for whatever reason and try to pop the content of the clip stack on a target doesn't have these clips applied.
This patch also removes GetBufferProvider which was the second place we were potentially allocating the buffer provider. Now it has to happen in Ensure target where we are in a better position to make choose provider backend and check that it is able to allocate the surface.

Hopefully this will remove the remaining clip management bugs (if any left) but I haven't managed to reproduce them so it is hard to tell.

Comment 2

[Bas Schouten (:bas.schouten)]

Comment on attachment 8785268 [details] [diff] [review]
Refactor the allocation code.

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

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +1553,5 @@
>  
> +  // Check that the dimensions are sane
> +  if (mWidth > gfxPrefs::MaxCanvasSize() ||
> +      mHeight > gfxPrefs::MaxCanvasSize() ||
> +      mWidth < 0 || mHeight < 0) {

nit: Why are these signed integers anyway? We don't seem to be using any magical negative numbers.

@@ +1581,5 @@
>    }
>  
>    ScheduleStableStateCallback();
>  
> +  auto persistedRect = canDiscardContent ? IntRect()

nit: Please don't use auto in these types of cases, it's an IntRect. (I realize you just moved this code, let's just fix it)

@@ +1688,5 @@
> +
> +  gCanvasAzureMemoryUsed += mWidth * mHeight * 4;
> +  JSContext* context = nsContentUtils::GetCurrentJSContext();
> +  if (context) {
> +    JS_updateMallocCounter(context, mWidth * mHeight * 4);

I can't seem to find out where we decrement this? But for the record this is true for the current state of affairs as well.

Comment 3

[Nicolas Silva [:nical]]

(In reply to Bas Schouten (:bas.schouten) from comment #2)
> nit: Why are these signed integers anyway? We don't seem to be using any
> magical negative numbers.

There are two school of thoughts represented in gecko's code about this, one being that we should only ever use unsigned integers where we don't expect the value to be negative, the other being that using signed integers makes it easier to catch bugs where the integer becomes negative. The second one seems to be the most represented in gecko, although not overwhelmingly so. I also think we tend to inherit signed-ness from our widespread use of IntSize which is signed. Just using signed integers everywhere makes it easier to write stuff without having to check the type and cast every time we compare numbers (-Werror, etc.).
I don't have a strong opinion, but think that the current state of things (signed all-the-things) is fine.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bbc7d520ccb
Refactor CanvasRenderingContext2D's texture allocation code. r=Bas

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9bbc7d520ccb

Comment 6

[Nicolas Silva [:nical]]

Comment on attachment 8785268 [details] [diff] [review]
Refactor the allocation code.

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Canvas crashes related to clip management and some correctness issues affecting badly pdf.js which could be fixed by uplifting several other patches instead of this one.

There has been a series of patches fixing various canvas issues mostly concentrated around CanvasRenderingContext2D::EnsureTarget. most of them fix issues that are also affecting aurora. This patch reorganizes and consolidates all of that work into something simpler and more robust. It is a lot easier to uplift this than hand pick the other fixes independently and resolve merge conflicts for each of them (and less risky).

[Describe test coverage new/current, TreeHerder]: None.
[Risks and why]: Less risky than uplifting the handful of canvas fixes that affect crashes present on aurora.
[String/UUID change made/needed]: None.

Uplifting this also means that if we find another issue and need to uplift it, it will be a lot easier because the canvas code currently look very different on central and aurora due to this reorganization.

Comment 7

[Nicolas Silva [:nical]]

Comment on attachment 8785268 [details] [diff] [review]
Refactor the allocation code.

I changed my mind: I'll try to disable copy-on-write canvas on aurora and only uplift the non-copy-on-write fixes.

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 8785268 [details] [diff] [review]
Refactor the allocation code.

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

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +1741,5 @@
> +                                                      size, format);
> +  if (!aOutDT) {
> +    return false;
> +    gfxCriticalNote << "Failed to create a SkiaGL DrawTarget, falling back to software\n";
> +  }

Coverity identified that the gfxCriticalNote call is dead code. Want to do a follow-up patch, nical?

Comment 9

[Nicolas Silva [:nical]]

Attachment: Warning fix

Oops!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ef9025fbcd3
Dead code warning fix. r=njn

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8ef9025fbcd3