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

Convert url to blob for large file size download #1455

Merged
merged 5 commits into from
Mar 11, 2021

Conversation

ataata107
Copy link

Fixes #1429 (<=== Replace 0000 with the Issue Number)

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with npm test
  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • ask @publiclab/is-reviewers for help, in a comment below
  • Insert-step functionality is working correct as expected.

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software
Please make sure to get at least two reviews before asking for merging the PR as that would make the PR more reliable on our part
Thanks!

@codecov
Copy link

codecov bot commented Jan 11, 2020

Codecov Report

Merging #1455 (c044512) into main (853e719) will decrease coverage by 1.94%.
The diff coverage is 12.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1455      +/-   ##
==========================================
- Coverage   66.67%   64.73%   -1.95%     
==========================================
  Files         130      127       -3     
  Lines        2686     2603      -83     
  Branches      433      418      -15     
==========================================
- Hits         1791     1685     -106     
- Misses        895      918      +23     
Impacted Files Coverage Δ
examples/lib/defaultHtmlStepUi.js 11.41% <12.50%> (+0.06%) ⬆️
src/modules/ImportImage/Module.js 12.50% <0.00%> (-75.00%) ⬇️
src/modules/NoiseReduction/NoiseReduction.js 65.21% <0.00%> (-34.79%) ⬇️
src/modules/FlipImage/flipImage.js 80.00% <0.00%> (-20.00%) ⬇️
src/modules/Dither/Dither.js 58.13% <0.00%> (-18.61%) ⬇️
src/util/getStep.js 10.71% <0.00%> (-14.29%) ⬇️
src/modules/ColorTemperature/Module.js 84.21% <0.00%> (-10.53%) ⬇️
src/ui/LoadImage.js 50.00% <0.00%> (-9.10%) ⬇️
src/modules/ReplaceColor/ReplaceColor.js 92.59% <0.00%> (-7.41%) ⬇️
src/modules/Channel/Module.js 87.50% <0.00%> (-6.25%) ⬇️
... and 20 more

@ataata107
Copy link
Author

@publiclab/is-reviewers

@ataata107
Copy link
Author

Previously files around 1 mb or greater were not possible but this PR makes it possible to download large files

@LeoDog896
Copy link

Works fine! Since this project is in ES6, why don't you use let instead of var inside the function scope?

@ataata107
Copy link
Author

@LeoDog896 Done

@harshkhandeparkar
Copy link
Member

This one is outdated. Closing for now, please feel free to rebase and reopen :)

@jywarren
Copy link
Member

Hi @harshkhandeparkar i wonder if this would help performance on larger images, and it looks like a relatively complete PR. Should we test it out and consider merging it?

@jywarren jywarren reopened this Mar 10, 2021
element.setAttribute('download', step.name + '.' + fileExtension(step.imgElement.src));
element.style.display = 'none';
document.body.appendChild(element);
var blob = dataURLtoBlob(step.output);
var objurl = URL.createObjectURL(blob);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between this URL and dataURL?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just encountered this in https://github.com/jywarren/bookletize.js and data urls have a length limit in browsers, so they fail on large files, and also blob is more efficient because they don't need to be reencoded.

I think this is probably worth it!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we internally use blobs too, then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's possible. We had very very long ago considered different formats for between-module data handoff, including possibly buffers...? i forget, but it was in our very first issue: #1 (comment)

We could open a new issue with this idea -- basically that default is data-urls, because strings are completely compatible, but if two modules can negotiate that both are compatible with blobs, that's OK too. I think the data-urls worked especially well because they had to be displayed anyways. But now that I think of it I think it's possible to display a blob as well in the browser. Please feel free to open an issue to discuss this!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this would just work, but it depends on the run environment, I imagine:

var i = drawarray[pos].i;
var input = ref.steps[i - 1].output;
var step = ref.steps[i];
step.getStep = function getStep(offset) {
if (i + offset >= ref.steps.length) return { options: { name: undefined } };
else return ref.steps.slice(i + offset)[0];
};
step.getIndex = function getIndex() {
return i;
};
for (var util in getStepUtils) {
if (getStepUtils.hasOwnProperty(util)) {
step[util] = getStepUtils[util];
}
}
// Tell UI that a step is being drawn.
step.UI.onDraw(step.options.step);
// provides a set of standard tools for each step
var inputForNextStep = require('./RunToolkit')(ref.copy(input));
step.draw(
inputForNextStep,
function onEachStep() {
// This output is accessible by UI
ref.steps[i].options.step.output = ref.steps[i].output.src;
ref.steps[i].options.step.wasmSuccess = ref.steps[i].output.wasmSuccess || false;
ref.steps[i].options.step.useWasm = ref.steps[i].output.useWasm || false;
// Tell UI that step has been drawn.
ref.steps[i].UI.onComplete(ref.steps[i].options.step);
drawStep(drawarray, ++pos);

We could add a parameter to drawStep somewhere that evaluates whether the neighboring steps can both handle blobs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I broke this into an issue at #1847 !!

@jywarren jywarren requested a review from a team as a code owner March 11, 2021 16:19
@gitpod-io
Copy link

gitpod-io bot commented Mar 11, 2021

@jywarren
Copy link
Member

Merging this! 🎉 at last, thank youuuuuuu!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't Download Images with higher dimensions
4 participants