Process data uri images synchronously #4105

jonmmease · 2019-08-03T17:34:49Z

This PR is the result of some time spent debugging an odd behavior I was seeing when using plotly.js layout images to display interactive results generated by Datashader.

The basic idea is that every time the viewport of a plot changes (pan or zoom), a new image is generated by datashader to be displayed for that viewport. By encoding these images a data uris, I've been able to display them pretty successfully using layout.image objects.

But one problem has been that sometimes the images appear to jump right before they are updated. Here's what that looks like.

After playing with this for a while, I realized that what's happening is that the image position (with the old image) is getting updated a moment before the new image is displayed. Here's a pure JavaScript example that shows the problem a little more clearly. (gist: https://gist.github.com/jonmmease/d2793251fdb1f370e483d54ca6561690). In this snippet, each time the viewport changes, Plotly.react is used to update the image with a new source and a new position, where the position is always the bottom right corner of the viewport.

Notice how the old image is moved to the bottom right corner before the new image is displayed.

After digging into plotly.js a bit, I realized that the reason this is happening is that the displayed image is updated in an img.onload callback that is executed after the image has loaded. This callback then builds and saves the data uri form of the image using a canvas element.

This PR makes a small adjustment to bypass the onload callback for images that are specified as data uri's already. In this case we can use the layout.image.source string directly. With this change, images specified as data uris will update at the same time that their position is updated. This change is in commit 536c90e. Commit 0a397c4 contains the indentation that I omitted from 536c90e in order to make a cleaner diff of the logic changes.

Here is what the example above looks like in this PR:

Let me know if there are any specific tests you'd like to see for this change. I did notice that there is already a data uri image being tested by the layout_image mock.

This causes the image source to be updated at the same time as the image location.

This commit is the indentation that was omitted from previous commit for clarity

jonmmease · 2019-08-04T22:45:42Z

I'm not sure what's going on with the tests.

    ✗ @gl should update the scene camera - orthographic case (3494ms)
	Error: Timeout - Async callback was not invoked within 3000ms (set by jasmine.DEFAULT_TIMEOUT_INTERVAL)

And another timeout. I already tried restarting them once with no change. Any ideas?

etpinard · 2019-08-05T14:25:43Z

And another timeout. I already tried restarting them once with no change. Any ideas?

Tests are passing now.

etpinard · 2019-08-05T15:32:49Z

src/components/images/draw.js

-                var canvas = document.createElement('canvas');
-                canvas.width = this.width;
-                canvas.height = this.height;
+            if(d.source && d.source.slice(0, 5) === 'data:') {


d.source should always be a string at this stage, so you could use:

if(d.source.indexOf('data:') === 0) { // ... }

instead.

d.source can be large though, isn't slice(0, 5) going to be a lot faster than indexOf?

That's a good point @alexcjohnson. @etpinard, anything you don't like about slice here?

It's not obvious to me why slice would be faster than indexOf() === 0. Has anyone tested that out on jsperf before?

My thinking was that If the string starts with 'data:', then indexOf() would terminate right away. But if not, indexOf() would search the full string and not bail out until it reaches the end, whereas slice never needs to look past the first 5 characters. If the string doesn't start with data:, then I guess it should be a URL which hopefully isn't terribly long. So it might not matter either way.

I guess in as far as the two options we care about are a short string starting http
and a long string starting data:, indexOf should be fine. The case to worry about would be a long string that doesn't start data:... but we don't expect that and wouldn't care if it's slow.

jsperf shows a win for indexOf https://jsperf.com/slice-vs-indexof-acj but I don't trust it, there's no way it could actually do indexOf in 2 clock cycles, compiler must have turned it into a constant. This isn't happening in a loop so either option is fast enough to be not worth worrying about.

src/components/images/draw.js

etpinard · 2019-08-05T16:01:28Z

Thanks @jonmmease - this works!

Before merging this, it would be cool to add one test checking that layout images with data uri as source do not call canvas.getContext and/or canvas.toDataURL. Here's one way we currently use to spy on canvas methods:

plotly.js/test/jasmine/tests/heatmap_test.js

Lines 735 to 741 in ffa6e34

    
           spyOn(document, 'createElement').and.callFake(function(elementType) { 
        
               var element = originalCreateElement.call(document, elementType); 
        
               if(elementType === 'canvas') { 
        
                   spyOn(element, 'getContext').and.returnValue(getContextStub); 
        
               } 
        
               return element; 
        
           });

etpinard · 2019-08-05T16:03:37Z

Oh and @jonmmease , I put this under the 1.50.0 milestone. This could be released as part of a patch if urgent, but I would prefer releasing this in a minor. I hope you don't mind.

jonmmease · 2019-08-05T16:07:41Z

Thanks @etpinard, yeah that test approach make sense. I'll give it a go!

Not urgent, 1.50.0 is fine. Thanks for the quick review!

- Stash prior input image src string as the this._imgSrc property - Pull datauti code outside of the promise, so that we skip creating the promise and skip creating an Image() object, and skip running the onload function.

jonmmease · 2019-08-07T12:26:34Z

Ok, in c310c92 I pulled the data uri image handling outside of the promise. So in this case we don't create the promise, construct an Image, or run onload. And I added a this._imgSrc property to save off the input image string for later comparison.

In dd585b2, I added a test to make sure that we don't construct any Image elements for the data uri case, but we do construct an image and call toDataURL for the image url case.

Note: I tried to avoid the setTimeout call in the test, but I couldn't figure out how to get Jasmine to check that a variable remains undefined, without timing out the test. expect(newCanvasElement).not.eventually.toBeDefined(), expect(newCanvasElement).eventually.not.toBeDefined(), and expect(newCanvasElement).eventually.toBeUndefined() all timed out.

test/jasmine/tests/layout_images_test.js

etpinard · 2019-08-07T14:56:09Z

Very strong PR @jonmmease ! Thanks very much for your help!

We haven't started merging things for 1.50.0, so I won't give you a : dancer : just yet. We'll most likely release 1 or 2 more patch releases in the 1.49.x series before.

jonmmease · 2019-08-07T15:04:04Z

Thanks for the reviews @etpinard, nice to dip my toe into the code base again!

etpinard · 2019-08-28T21:05:44Z

We've started merging things for 1.50.0, so @jonmmease merge away 💃 💃

jonmmease added 4 commits August 3, 2019 12:47

If image source is already in data uri form, process synchronously

536c90e

This causes the image source to be updated at the same time as the image location.

Apply indentation

0a397c4

This commit is the indentation that was omitted from previous commit for clarity

lint fix

611064d

Restore "data:" string

86a3fc0

etpinard added status: in progress labels Aug 5, 2019

etpinard reviewed Aug 5, 2019

View reviewed changes

etpinard added this to the v1.50.0 milestone Aug 5, 2019

etpinard reviewed Aug 5, 2019

View reviewed changes

src/components/images/draw.js Outdated Show resolved Hide resolved

etpinard added the bug something broken label Aug 5, 2019

jonmmease added 2 commits August 7, 2019 06:14

Don't create promise or invoke onload for data uri images

c310c92

- Stash prior input image src string as the this._imgSrc property - Pull datauti code outside of the promise, so that we skip creating the promise and skip creating an Image() object, and skip running the onload function.

Add test that a canvas element is not created when image is a data uri

dd585b2

etpinard reviewed Aug 7, 2019

View reviewed changes

test/jasmine/tests/layout_images_test.js Outdated Show resolved Hide resolved

Restructure test to avoid setTimeout

4a1afbd

etpinard added status: reviewable and removed status: in progress labels Aug 7, 2019

jonmmease merged commit 37e80cb into master Aug 28, 2019

etpinard deleted the datauri_images branch September 23, 2019 20:39

snyk-bot mentioned this pull request Feb 4, 2020

[Snyk] Upgrade plotly.js from 1.45.3 to 1.52.0 beauremus/ftp-viewer#1

Merged

snyk-bot mentioned this pull request Feb 22, 2020

[Snyk] Upgrade plotly.js from 1.35.2 to 1.52.1 raindigi/eShopOnContainersAI#1

Open

snyk-bot mentioned this pull request Apr 17, 2020

[Snyk] Upgrade plotly.js from 1.41.3 to 1.52.3 dunzoit/redash#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Process data uri images synchronously #4105

Process data uri images synchronously #4105

jonmmease commented Aug 3, 2019

jonmmease commented Aug 4, 2019

etpinard commented Aug 5, 2019

etpinard Aug 5, 2019

jonmmease Aug 5, 2019

alexcjohnson Aug 5, 2019

jonmmease Aug 7, 2019

etpinard Aug 7, 2019

jonmmease Aug 7, 2019

alexcjohnson Aug 7, 2019

etpinard commented Aug 5, 2019

etpinard commented Aug 5, 2019

jonmmease commented Aug 5, 2019

jonmmease commented Aug 7, 2019

etpinard commented Aug 7, 2019

jonmmease commented Aug 7, 2019

etpinard commented Aug 28, 2019

Process data uri images synchronously #4105

Process data uri images synchronously #4105

Conversation

jonmmease commented Aug 3, 2019

jonmmease commented Aug 4, 2019

etpinard commented Aug 5, 2019

etpinard Aug 5, 2019

Choose a reason for hiding this comment

jonmmease Aug 5, 2019

Choose a reason for hiding this comment

alexcjohnson Aug 5, 2019

Choose a reason for hiding this comment

jonmmease Aug 7, 2019

Choose a reason for hiding this comment

etpinard Aug 7, 2019

Choose a reason for hiding this comment

jonmmease Aug 7, 2019

Choose a reason for hiding this comment

alexcjohnson Aug 7, 2019

Choose a reason for hiding this comment

etpinard commented Aug 5, 2019

etpinard commented Aug 5, 2019

jonmmease commented Aug 5, 2019

jonmmease commented Aug 7, 2019

etpinard commented Aug 7, 2019

jonmmease commented Aug 7, 2019

etpinard commented Aug 28, 2019