Add support for context globalAlpha for gradients and patterns #1064

asturur · 2017-12-21T09:52:30Z

So this is another discrepancy i found out inspecting differences between browser canvas and node-canvas.

Pattern and gradients are always solid.

Now i added those tests here:
Pattern fil and stroke, with and without opacity

Gradients with rgba, gradients and globalAlpha and gradients and trasnforms

The code is a bit ... meh ...
The point of the fix for patterns i think is ok, i pick up the chosen pattern, i create a new surface that is as big as the pattern surface, i paint it with alpha, i create a new pattern from that surface.
This is the only thing that was working for me.

For Gradients i do same, create a new gradient, multiply alpha and move on,

I just created 2 functions in the the code, i did not make them class method, i m not a C++ or node-gyp dev, so this is what i could do, i m open to suggestion to improve it.

I also think i have to destroy some resources after this process, any hint how/where?

V1.x update

zbjornson

Another nice find. Unfortunate that the fix for this is creating another whole surface... I'm not familiar enough with Cairo's gradients and patterns to know if there's a better way though. Patterns are probably not that big in general at least.

zbjornson · 2017-12-21T23:21:57Z

src/CanvasRenderingContext2d.cc

+  cairo_pattern_t *newGradient;
+  cairo_pattern_type_t type = cairo_pattern_get_type(source);
+  cairo_pattern_get_color_stop_count(source, &count);
+  if (type == 2) {


can you use the cairo_pattern_type_t constants instead? https://developer.gnome.org/cairo/stable/cairo-cairo-pattern-t.html#cairo-pattern-type-t

zbjornson · 2017-12-21T23:23:03Z

src/CanvasRenderingContext2d.cc

+  cairo_pattern_type_t type = cairo_pattern_get_type(source);
+  cairo_pattern_get_color_stop_count(source, &count);
+  if (type == 2) {
+    cairo_pattern_get_linear_points (source, &x0, &y0, &x1, &y1);


nit: no space between fn name and parentheses
same a few lines down (322)

zbjornson · 2017-12-21T23:33:48Z

src/CanvasRenderingContext2d.cc

  } else {
    setSourceRGBA(state->fill);
  }
-


I can't comment far down enough, but after the shadow/fill calls a few lines down, you need to free the new_pattern or new_gradient cairo_pattern_ts with https://developer.gnome.org/cairo/stable/cairo-cairo-pattern-t.html#cairo-pattern-destroy.

zbjornson · 2017-12-21T23:34:42Z

src/CanvasRenderingContext2d.cc

+    } else {
+      cairo_pattern_set_filter(state->strokeGradient, state->patternQuality);
+      cairo_set_source(_context, state->strokeGradient);
+    }


Ditto about freeing the cairo_pattern_ts at the end of this function.

zbjornson · 2017-12-21T23:41:22Z

src/CanvasRenderingContext2d.cc

+  cairo_pattern_get_surface(source, &surface);
+  int width = cairo_image_surface_get_width(surface);
+  int height = cairo_image_surface_get_height(surface);
+  cairo_surface_t *mask_surface = cairo_image_surface_create(


*mask_surface needs to be freed at some point. I think it's safe to call cairo_surface_destroy just after your cairo_pattern_create_for_surface(mask_surface) call, so you can do memory management within this function.

I still don't see this being freed... calling cairo_surface_destroy decreases the ref count, but doesn't destroy it unless the ref count is 0. cairo_pattern_create_for_surface increases the ref count on the surface. There's an example of this exact lifecycle pattern in the Cairo source code here. Make sense?

i thought that since the pattern is alive and to be used you have to keep the surface.
I get this feeling from the sentence 'the pattern owns the surface' but maybe i m wrong. I'll try to delete everything, if it still draws is ok.

zbjornson · 2017-12-21T23:42:25Z

src/CanvasRenderingContext2d.cc

+    CAIRO_FORMAT_ARGB32,
+    width,
+    height);
+  cairo_t *mask_context = cairo_create(mask_surface);


*mask_context needs to be freed. Looks like you don't need it after cairo_paint_with_alpha so that's easy.

You should also check cairo_status(mask_context) since this could fail to allocate.

asturur · 2017-12-22T20:23:26Z

I think i solved all the comments @zbjornson

V1.x

zbjornson

Cool, almost there.

zbjornson · 2017-12-24T19:03:03Z

src/CanvasRenderingContext2d.cc

+  if (type == CAIRO_PATTERN_TYPE_RADIAL) {
+    cairo_pattern_get_radial_circles(source, &x0, &y0, &r0, &x1, &y1, &r1);
+    newGradient = cairo_pattern_create_radial(x0, y0, r0, x1, y1, r1);
+  }


To get rid of the maybe-uninitialized warning here, can you make this if (linear) {} else if (raidal) {} else { do something reasonable like throwing an error } please?

i did not get any warning, but definitely i can guard against it.

zbjornson · 2017-12-24T19:09:28Z

src/CanvasRenderingContext2d.cc

+  cairo_pattern_get_surface(source, &surface);
+  int width = cairo_image_surface_get_width(surface);
+  int height = cairo_image_surface_get_height(surface);
+  cairo_surface_t *mask_surface = cairo_image_surface_create(


I still don't see this being freed... calling cairo_surface_destroy decreases the ref count, but doesn't destroy it unless the ref count is 0. cairo_pattern_create_for_surface increases the ref count on the surface. There's an example of this exact lifecycle pattern in the Cairo source code here. Make sense?

zbjornson · 2017-12-24T19:12:36Z

src/CanvasRenderingContext2d.cc

@@ -330,6 +402,9 @@ Context2d::fill(bool preserve) {
      ? shadow(cairo_fill)
      : cairo_fill(_context);
  }
+  if (new_pattern != state->fillPattern && new_pattern != NULL) {


I think the new_pattern != state->fillPattern part of this makes the cairo_pattern_destroy unreachable in cases of global alpha < 1. Here again the destroy fn actually just decreases ref count; it should be safe to call it whenever new_pattern != NULL.

Actually, you can make the destroy call after line 388 I think, right after cairo_set_source(_context, new_pattern). That'll let you shrink the scope of new_pattern as well.

i added this check because the new_pattern can be NULL globalAlpha = 1 or can be fillPattern if the creation of the new pattern failed ( because we return the same pattern from the other function ).
Why you think is unreachable?

zbjornson · 2017-12-24T19:12:49Z

src/CanvasRenderingContext2d.cc

@@ -357,6 +444,9 @@ Context2d::stroke(bool preserve) {
      ? shadow(cairo_stroke)
      : cairo_stroke(_context);
  }
+  if (new_pattern != state->strokePattern && new_pattern != NULL) {


asturur · 2017-12-24T22:15:39Z

I now essentially moved the destroy call right after pattern usage.
I m also now trowing errors as soon as something goes wrong, i will not return the original pattern if creating the new one fails, and i will not check for the equality later.
Seems to render everything fine anyway.

LinusU · 2017-12-26T10:33:33Z

src/CanvasRenderingContext2d.cc

+    cairo_pattern_get_radial_circles(source, &x0, &y0, &r0, &x1, &y1, &r1);
+    newGradient = cairo_pattern_create_radial(x0, y0, r0, x1, y1, r1);
+  } else {
+    Nan::ThrowError("Unexpected gradient type");


I don't remember exactly how this works but it feels like there should be a return statement here:

Nan::ThrowError("Unexpected gradient type"); return nullptr;

Hmm, actually, possibly that needs to be guarded at the call-site as well? I don't see how Nan::ThrowError can alter the normal flow of a c++ function, but maybe I'm missing something 🤔

if i only knew c++... i have C knowledge coming from embedded platforms with proprietary api. I stop there.

How can i test it? i can force a gradient type comparision and see what happens?

i can force a gradient type comparision and see what happens?

that sounds good 👍

oh by the way using nullptr kills nodejs < 4.

asturur · 2017-12-26T14:51:04Z

this is the best error managment i can get.
The server do not crash, it continues staying alive, it spit an error in the console. Rendering do not happen:

i triggered the error in this way:

adding a number to the enums so they do not match.

asturur · 2017-12-26T14:52:49Z

this is what i can get for patterns.

zbjornson · 2017-12-27T06:22:05Z

src/CanvasRenderingContext2d.cc

+      cairo_set_source(_context, new_pattern);
+      if (new_pattern != state->fillPattern) {
+        cairo_pattern_destroy(new_pattern);
+      }


Looks great! Last comment... do I have this right: new_pattern == state->fillPattern only if create_transparent_pattern fails? If so, then I'd consider changing this (and the equivalent other blocks of code) to:

new_pattern = create_transparent_pattern(state->fillPattern, state->globalAlpha); if (new_pattern == state->fillPattern) { // failed to allocate; Nan::ThrowError has already been called, so return from this fn. return; } cairo_set_source(_context, new_pattern); cairo_pattern_destroy(new_pattern);

I think that clarifies the flow a bit, and explicitly stops execution if we've reached an error state.

and what about gradient? gradient has a similiar case, a slower chance to fail since i have no idea how you can create gradients type that are not linear or radial, but if this happen,still we are in a sort of error state, or at least rendering something different from what is expected.
Do we want to return also in that case?

I think you can do the same thing...

new_pattern = create_transparent_gradient(state->fillGradient, state->globalAlpha); if (new_pattern == state->fillGradient) return; cairo_pattern_set_filter(new_pattern, state->patternQuality); cairo_set_source(_context, new_pattern);

Also, since you're now bailing out in case of error, you might as well return NULL from create_transparent_gradient/create_transparent_pattern in case of error, and then just check if new_pattern == NULL.

but can i return null from a function that expect a pointer to cairo_pattern_t?

asturur · 2017-12-27T22:20:32Z

Ok, i used NULL the compiler does not complain.

Fixes #1061

zbjornson · 2017-12-28T01:27:40Z

Ported to master/v2.x in d17e9ce. Thanks!

…d alpha Automattic/node-canvas#1061 Automattic/node-canvas#1064 Thanks!

asturur added 5 commits December 21, 2017 00:35

sofar so good

5875be3

Merge pull request #1 from Automattic/v1.x

b5f6545

V1.x update

Merge branch 'v1.x' into gradients-opacity

74e9250

this is a draft

e00aa8b

different fix

009c632

zbjornson requested changes Dec 21, 2017

View reviewed changes

asturur added 2 commits December 22, 2017 11:30

partial update

e3cdd85

should be doone

931c0d3

asturur added 3 commits December 22, 2017 21:29

Merge pull request #2 from Automattic/v1.x

ac4dd37

V1.x

merged conflicts

5e9b4e6

fix breks

93dab0c

zbjornson requested changes Dec 24, 2017

View reviewed changes

asturur added 2 commits December 24, 2017 23:12

different handling

a5c2e74

removed non more valid comments;

b2be100

LinusU reviewed Dec 26, 2017

View reviewed changes

working error management

5e5614a

zbjornson reviewed Dec 27, 2017

View reviewed changes

asturur added 3 commits December 27, 2017 10:15

return from fill/stroke if new pattern failed to allocate

c894c8b

Merge branch 'v1.x' into gradients-opacity

e679840

merged master and used NULL

803a24e

zbjornson approved these changes Dec 28, 2017

View reviewed changes

zbjornson merged commit a815257 into Automattic:v1.x Dec 28, 2017

zbjornson pushed a commit that referenced this pull request Dec 28, 2017

Add support for context globalAlpha for gradients and patterns (#1064)

d17e9ce

Fixes #1061

jacquev6 added a commit to jacquev6/JsOfOCairo that referenced this pull request Apr 25, 2018

Acknowledge that node-canvas.1.6.10 implements paint with gradient an…

9f9a587

…d alpha Automattic/node-canvas#1061 Automattic/node-canvas#1064 Thanks!

asturur mentioned this pull request Sep 3, 2018

Bug with alpha of gradient fill style #417

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for context globalAlpha for gradients and patterns #1064

Add support for context globalAlpha for gradients and patterns #1064

asturur commented Dec 21, 2017 •

edited

Loading

zbjornson left a comment

zbjornson Dec 21, 2017

zbjornson Dec 21, 2017

zbjornson Dec 21, 2017

zbjornson Dec 21, 2017

zbjornson Dec 21, 2017

zbjornson Dec 24, 2017

asturur Dec 24, 2017

zbjornson Dec 21, 2017

asturur commented Dec 22, 2017

zbjornson left a comment

zbjornson Dec 24, 2017

asturur Dec 24, 2017

zbjornson Dec 24, 2017

zbjornson Dec 24, 2017

asturur Dec 24, 2017

zbjornson Dec 24, 2017

asturur commented Dec 24, 2017

LinusU Dec 26, 2017

asturur Dec 26, 2017

LinusU Dec 26, 2017

asturur Dec 26, 2017

asturur commented Dec 26, 2017

asturur commented Dec 26, 2017

zbjornson Dec 27, 2017

asturur Dec 27, 2017

zbjornson Dec 27, 2017

asturur Dec 27, 2017

zbjornson Dec 27, 2017

asturur commented Dec 27, 2017

zbjornson commented Dec 28, 2017

Add support for context globalAlpha for gradients and patterns #1064

Add support for context globalAlpha for gradients and patterns #1064

Conversation

asturur commented Dec 21, 2017 • edited Loading

zbjornson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur commented Dec 22, 2017

zbjornson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur commented Dec 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur commented Dec 26, 2017

asturur commented Dec 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur commented Dec 27, 2017

zbjornson commented Dec 28, 2017

asturur commented Dec 21, 2017 •

edited

Loading