Improve `Plotly.toImage` #1939

etpinard · 2017-08-08T19:14:50Z

The bulk of this PR is in commit 8866525 (where several improvement to Plotly.toImage are made) and dd003ce (where the setBackground config option is now a little more portable)

With 8866525, Plotly.toImage can now accept data/layout/config (plain) object as in input. Previously, only existing graph divs were supported. This is important for image generation performance.

In details, to generate an image using only official API methods (i.e. none of the Plotly.Snapshot methods), currently one must first call Plotly.newPlot and then pass the gd to Plotly.toImage. To ensure that toImage does not mutate the existing graph div, toImage clones it (i.e. extends the graph's data/layout) and calls Plotly.plot on an off-screen <div>. That is, Plotly.plot is called twice for each generated image. Needless to say, we can do better.

So, now calling toImage as:

Plotly.toImage({
  data: [/* */],
  layout: {/* */},
  config: {/* */}
})
.then(imgData => {})

assumes that the data/layout/config containers are new (i.e. aren't linked to an existing graph) meaning that the containers aren't extended and Plotly.plot is only called once per generated image 🐎

- which should fail ;)

- i.e. not crash when called multiple times

- instead of in `plot_config.js` declaration

- which is currently used in the image server

- + add 'webp' test case

- accept data/layout/config figure (plain) object as input, along side existing graph div or (string) id of existing graph div - use Lib.validate & Lib.coerce to sanatize options - bypass Snapshot.cloneplot (Lib.extendDeep is really all we need) - handle 'setBackground` (same as config option) - add 'imageDataOnly' option to strip 'data:image/' prefix

etpinard · 2017-08-08T19:16:08Z

src/plot_api/plot_api.js

@@ -404,6 +404,11 @@ function opaqueSetBackground(gd, bgColor) {
    setBackground(gd, bgColor);
 }

+function blendSetBackground(gd, bgColor) {
+    var blend = Color.combine(bgColor, 'white');


Implements this (old) image server logic in plotly.js

doesn't this end up visually equivalent to opaqueSetBackground? And if so can we remove opaqueSetBackground?

RE 'opaque' vs 'blend', they don't generate the same PNG (gm compare generates a diff) but I can't find a case where the diff is detectable to my 👀

So, I guess a can make the new image server use 'opaque' instead of 'blend' and 🔪 blendSetBackground.

done in 22a598b

etpinard · 2017-08-08T19:36:13Z

src/plot_api/to_image.js

-            reject(new Error('Height and width should be pixel values.'));
-        }
+            if(format === 'svg' && imageDataOnly) {
+                return resolve(svg);


Fast pass for svg with imageDataOnly where Snapshot.toSVG gives the correct result.

Note that PDF and EPS images should first be generated as svg and then converted to pdf or eps using another library. In-house PDF and EPS image generation is outside the scope of plotly.js.

is it worth handling imageDataOnly=false here too - and just adding the prefix? Or is it more complicated than that?

Yeah, it shouldn't be too hard. I'll add that in.

done in 4ec3e0f

alexcjohnson · 2017-08-11T17:28:50Z

src/plot_api/to_image.js

+
+    if(!isBadlySet('format')) {
+        throw new Error('Image format is not jpeg, png, svg or webp.');
+    }


Nice job using coerce for this! But isBadlySet is confusing me... it's true if you either haven't set the attribute or if it has a valid input? Wouldn't that be isValidOrImplied or something? Anyway I don't see how you can use the same logic for format (which has a default) as for width and height (which don't).

Dimensions you could just look at fullOpts after coerce to see if you have something that's not undefined (the implicit dflt)...

Oh I see, it is OK to omit dimensions, then they default to what's in the layout... might be clearer if this was just used as the default width = coerce('width', layout.width)?

might be clearer if this was just used as the default width = coerce('width', layout.width)?

right, but layout.width might not exist at this stage.

OK, but if it's not in opts and it's not in layout then what width do we get?

We get the dflt from plots/layout_attributes.js

Alright, I don't want to beat a dead horse and it's not a big deal, but I believe it would still work just fine if you coerce with a dflt which might be undefined at this point, then you coerce with the real dflt later on, overriding that undefined.

Anyway my initial comment stands, about isBadlySet being misnamed and confusing logic.

isBadlySet -> isImpliedOrValid in 3c42efb

alexcjohnson · 2017-08-11T17:32:13Z

src/plot_api/to_image.js

+    // extend config for static plot
+    var configImage = Lib.extendFlat({}, config, {
+        staticPlot: true,
+        plotGlPixelRatio: 2,


is plotGlPixelRatio something people might want to override? Would it work if they could?

Yeah, potentially. Good 👀

done in 7565d0b

alexcjohnson · 2017-08-11T17:52:10Z

test/jasmine/tests/toimage_test.js

+        })
+        .then(function() {
+            expect(errors.length).toBe(1);
+            expect(errors[0]).toBe('Image format is not jpeg, png, svg or webp.');


You're not a fan of expect(function() { ... }).toThrow(...)?

done in 43a6150

- and make 'opaque' use Color.combine instead of hard-setting _paperdiv.

- 🔪 config assignments to we already get for free via staticPlot: true

- make Plotly.toImage bypass svgToImg even when imageDataOnly is false.

etpinard · 2017-08-14T20:25:42Z

src/plot_api/to_image.js

+                if(imageDataOnly) {
+                    return resolve(svg);
+                } else {
+                    return resolve('data:image/svg+xml,' + encodeURIComponent(svg));


as on

plotly.js/src/snapshot/svgtoimg.js

Line 64 in f428026

var url = 'data:image/svg+xml,' + encodeURIComponent(svg);

This broke a lot of tests. It made the toImage promise resolved before removing the clone graph div.

Fixed in 816df16

alexcjohnson · 2017-08-14T20:36:26Z

Love it. 💃

- so that even 'svg' formats (which bypass svgToImg) clear the clone graph div - no need to remove *all* graph div in toImage test anymore 🎉

etpinard added 8 commits August 7, 2017 11:03

add isPlainObject test case for HTML <div>

f758c35

- which should fail ;)

make gl3d, gl2d and mapbox .destroy() idempotent

3d19676

- i.e. not crash when called multiple times

set setBackground config option in setPlotContext

dd003ce

- instead of in `plot_config.js` declaration

add 'blend' setBackground value

9128e13

- which is currently used in the image server

robustify toImage tests

5d2b635

- + add 'webp' test case

add 'webp' to list of available formats in svgToImg error msg

b3fac37

add tolerance for jpeg image length test

32f46c5

etpinard added status: reviewable feature something new labels Aug 8, 2017

etpinard commented Aug 8, 2017

View reviewed changes

etpinard mentioned this pull request Aug 10, 2017

Identify canvases for the specific gd being exported #1947

Merged

alexcjohnson reviewed Aug 11, 2017

View reviewed changes

etpinard added 6 commits August 14, 2017 11:23

Merge branch 'master' into improve-toimage

e5e35c0

rm 'blend' special setBackground value

22a598b

- and make 'opaque' use Color.combine instead of hard-setting _paperdiv.

lint (use toThrow, instead of error msg comparison)

43a6150

allow plotGlPixelRatio to be overrode by user config

7565d0b

- 🔪 config assignments to we already get for free via staticPlot: true

rename isBadlySet -> isImpliedOrValid

3c42efb

move IE-specific SVG string tweaks to toSVG

4ec3e0f

- make Plotly.toImage bypass svgToImg even when imageDataOnly is false.

etpinard commented Aug 14, 2017

View reviewed changes

purge cloneGd before resolving/rejecting convert promise

816df16

- so that even 'svg' formats (which bypass svgToImg) clear the clone graph div - no need to remove *all* graph div in toImage test anymore 🎉

etpinard added this to the v1.30.0 milestone Aug 14, 2017

lint

640503a

etpinard merged commit cb2d3d1 into master Aug 14, 2017

etpinard deleted the improve-toimage branch August 14, 2017 21:57

etpinard mentioned this pull request Aug 15, 2017

Plotting a 3D graph with config option staticPlot turned on results in blank graph #457

Open

etpinard mentioned this pull request Aug 16, 2017

Dockerize plotly/orca#1

Merged

etpinard mentioned this pull request Oct 18, 2017

Ability to style download image for chart #2098

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `Plotly.toImage` #1939

Improve `Plotly.toImage` #1939

etpinard commented Aug 8, 2017

etpinard Aug 8, 2017

alexcjohnson Aug 11, 2017

etpinard Aug 11, 2017

etpinard Aug 14, 2017

etpinard Aug 14, 2017

etpinard Aug 8, 2017 •

edited

Loading

alexcjohnson Aug 11, 2017

etpinard Aug 11, 2017

etpinard Aug 14, 2017

alexcjohnson Aug 11, 2017

alexcjohnson Aug 11, 2017

etpinard Aug 11, 2017

alexcjohnson Aug 14, 2017

etpinard Aug 14, 2017

alexcjohnson Aug 14, 2017

etpinard Aug 14, 2017 •

edited

Loading

alexcjohnson Aug 11, 2017

etpinard Aug 11, 2017

etpinard Aug 14, 2017

alexcjohnson Aug 11, 2017

etpinard Aug 14, 2017

etpinard Aug 14, 2017

etpinard Aug 14, 2017

alexcjohnson commented Aug 14, 2017

Improve Plotly.toImage #1939

Improve Plotly.toImage #1939

Conversation

etpinard commented Aug 8, 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

etpinard Aug 8, 2017 • edited Loading

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

Choose a reason for hiding this comment

etpinard Aug 14, 2017 • edited Loading

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

alexcjohnson commented Aug 14, 2017

Improve `Plotly.toImage` #1939

Improve `Plotly.toImage` #1939

etpinard Aug 8, 2017 •

edited

Loading

etpinard Aug 14, 2017 •

edited

Loading