Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

graph: fix “Download PNG” for large images #1610

Merged
merged 1 commit into from Nov 14, 2018

Conversation

Projects
None yet
3 participants
@wchargin
Copy link
Member

wchargin commented Nov 14, 2018

Summary:
When viewing large graphs in Chrome, clicking the “Download PNG” button
could the error message, “Failed - Network error”. This is because
Chrome does not support downloading large data-URLs. Chrome does,
however, support downloading large object URLs, so this patch makes the
appropriate change to use that API instead.

Test Plan:
Generate the audio_demo data and open the wave:01;sine_wave graph in
TensorBoard in Chrome. Transitively expand all node groups. Then, click
“Download PNG”. Before this patch, this consistently failed to download.
After this patch, it downloads and image with the correct contents.

Also tested in Firefox.

wchargin-branch: graph-download-png-blob

@nfelt

nfelt approved these changes Nov 14, 2018

Copy link
Collaborator

nfelt left a comment

cc @stephanwlee

If this change is worth doing here perhaps we should do it for the scalar chart download as SVG feature too? Those charts are going to be smaller in typical cases but for a really enormous number of runs they could also get fairly large. It would also seem to more "properly" address #1601.

this.download.href = this.downloadCanvas.toDataURL('image/png');
// Revoke the old URL, if any. Then, generate a new URL.
URL.revokeObjectURL(this.download.href);
// We can't use the `HTMLCanvasElement.toBlob` API because it does

This comment has been minimized.

Copy link
@nfelt

nfelt Nov 14, 2018

Collaborator

FWIW, for the scalar plot download buttons, we use a drop-down menu as the trigger to generate the URL so that it's a two-click process. If we did something similar for the graph it would be possible to use toBlob() and just disable the actual download click target until the synchronous call completes.

Maybe it's not worth it, but I have to imagine toBlob() might be faster than iterating over each character in the blob by hand.

return;
}
const data = atob(dataUrl.slice(dataUrl.indexOf(",") + 1));
const bytes = new Uint8Array(Array.from(data).map(c => c.charCodeAt(0)));

This comment has been minimized.

Copy link
@nfelt

nfelt Nov 14, 2018

Collaborator

Instead of creating a single-chart string for each element of data, can we just change the index to charCodeAt()? E.g. what this does: https://stackoverflow.com/a/21797381/1179226

It seems almost guaranteed to be cheaper to simply index the string at different points versus sub-slicing the string at then indexing into the sub-slice.

This comment has been minimized.

Copy link
@wchargin

wchargin Nov 14, 2018

Author Member

Sure, can do.

This comment has been minimized.

Copy link
@wchargin

wchargin Nov 14, 2018

Author Member

Good call—this cuts out almost all of the overhead. (Now looking at
~520ms with this code vs. ~500ms for toBlob.) Nice to have even if the
code isn’t performance-critical.

This comment has been minimized.

Copy link
@nfelt

nfelt Nov 14, 2018

Collaborator

Great. Nice to have those numbers since then I guess toBlob isn't much advantage anyway.

@wchargin wchargin force-pushed the wchargin-graph-download-png-blob branch from 4c7d749 to 0f94aff Nov 14, 2018

// URL.
const dataUrl = this.downloadCanvas.toDataURL("image/png");
const prefix = dataUrl.slice(0, dataUrl.indexOf(","));
if (!prefix.endsWith(":base64") && !prefix.endsWith(";base64")) {

This comment has been minimized.

Copy link
@stephanwlee

stephanwlee Nov 14, 2018

Collaborator

in which case does the prefix end with ":base64"? maybe the mdn doc is incomplete: https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/Data_URIs#Syntax

This comment has been minimized.

Copy link
@wchargin

wchargin Nov 14, 2018

Author Member

I seem to recall having seen data URIs that do not specify a media type
and are of the form data:base64,<data>, even though they really should
be data:;base64,<data>. But on further thought I suppose that those
are technically valid data URIs whose media type is base64 and whose
encoding is textual, not base64, so it would be better to fail here
(which is the more conservative action, anyway). Will patch.

graph: fix “Download PNG” for large images
Summary:
When viewing large graphs in Chrome, clicking the “Download PNG” button
could the error message, “Failed - Network error”. This is because
Chrome does not support downloading large data-URLs. Chrome does,
however, support downloading large object URLs, so this patch makes the
appropriate change to use that API instead.

Test Plan:
Generate the `audio_demo` data and open the `wave:01;sine_wave` graph in
TensorBoard in Chrome. Transitively expand all node groups. Then, click
“Download PNG”. Before this patch, this consistently failed to download.
After this patch, it downloads and image with the correct contents.

Also tested in Firefox.

wchargin-branch: graph-download-png-blob

@wchargin wchargin force-pushed the wchargin-graph-download-png-blob branch from 0f94aff to 4ab62c7 Nov 14, 2018

@wchargin wchargin merged commit 08a157f into master Nov 14, 2018

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wchargin wchargin deleted the wchargin-graph-download-png-blob branch Nov 20, 2018

stephanwlee added a commit to stephanwlee/tensorboard that referenced this pull request Dec 13, 2018

graph: fix “Download PNG” for large images (tensorflow#1610)
Summary:
When viewing large graphs in Chrome, clicking the “Download PNG” button
could the error message, “Failed - Network error”. This is because
Chrome does not support downloading large data-URLs. Chrome does,
however, support downloading large object URLs, so this patch makes the
appropriate change to use that API instead.

Test Plan:
Generate the `audio_demo` data and open the `wave:01;sine_wave` graph in
TensorBoard in Chrome. Transitively expand all node groups. Then, click
“Download PNG”. Before this patch, this consistently failed to download.
After this patch, it downloads and image with the correct contents.

Also tested in Firefox.

wchargin-branch: graph-download-png-blob

stephanwlee added a commit that referenced this pull request Dec 14, 2018

graph: fix “Download PNG” for large images (#1610)
Summary:
When viewing large graphs in Chrome, clicking the “Download PNG” button
could the error message, “Failed - Network error”. This is because
Chrome does not support downloading large data-URLs. Chrome does,
however, support downloading large object URLs, so this patch makes the
appropriate change to use that API instead.

Test Plan:
Generate the `audio_demo` data and open the `wave:01;sine_wave` graph in
TensorBoard in Chrome. Transitively expand all node groups. Then, click
“Download PNG”. Before this patch, this consistently failed to download.
After this patch, it downloads and image with the correct contents.

Also tested in Firefox.

wchargin-branch: graph-download-png-blob

stephanwlee added a commit that referenced this pull request Mar 8, 2019

scalar: use ObjectURL for SVG link for durability (#1979)
Data uri in the link for download has given us some trouble with
encoding and length limitations. We want to use the ObjectURL like
PR #1610 that is more durable to aforementioned limitations.

Related to #1601.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.