You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@cordova.apache.org by GitBox <gi...@apache.org> on 2021/02/17 18:36:50 UTC
[GitHub] [cordova-plugin-file] LightMind opened a new pull request #461: FileWriter writes data in chunks, and converts ArrayBuffers to Base64. Fixes Issue #364
LightMind opened a new pull request #461:
URL: https://github.com/apache/cordova-plugin-file/pull/461
### Platforms affected
All
### Motivation and Context
Writing large files with FileWriter consumes very large amounts of memory, because the underlying cordova.js function converts ArrayBuffers to base64 encoded strings by using String concatonation and using JSON.encode on all the data.
[Link to issue](https://github.com/apache/cordova-plugin-file/issues/364)
### Description
This PR aims to improve the conversion of ArrayBuffers to base64, by using FileReader.readAsDataUrl, which is much faster and more memory efficient.
The PR also writes the data in chunks of 1MiB, because the cordova code calls JSON.encode, which again has problems with large strings.
First, I refactored the code, to have more named and private functions, to make all the asynchronous calls more easy to follow. Then, in FileWriter.write, I check that we received an ArrayBuffer that should be written to disk. If so, we branch away from the existing code.
Then chunk the ArrayBuffer, convert chunks to Blob's with `octet` encoding, turn them into Base64 encoded strings, and then write them one at a time.
### Testing
I have used my plugin in our existing cordova Application, and have written both real and synthetic files up 200MiB, and checked that the written files have the correct content. I could open written PDFs and images just fine. For synthetic data, I checked that the output of FileReader.readAsArrayBuffer matches the input to FileWriter.write exactly.
I measured writing speed and memory requirements with chrome's debugging tools. Write speeds have improved to a consistent 4-5 MiB/s on my old Nexus 5x. Memory requirements look much more reasonable and are linear, instead of having extreme spikes with the old version.
See my analysis in the comments of [the linked issue](https://github.com/apache/cordova-plugin-file/issues/364)
I have not tested on a real iOS device yet, but will be able to do so very soon.
### Checklist
- [ ] I've run the tests to see all new and existing tests pass
- [ ] I added automated test coverage as appropriate for this change
- [ ] Commit is prefixed with `(platform)` if this change only applies to one platform (e.g. `(android)`)
- [ ] If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct [keyword to close issues using keywords](https://help.github.com/articles/closing-issues-using-keywords/))
- [ ] I've updated the documentation if necessary
### Questions
I hope it's ok to use the PR for further discussion.
I am not sure, how the FileWriter should behave, if it encounters an error while writing chunks. I see two possibilities:
* Try to revert everything that has been written for this invocation of .write(), i.e reset the position and delete the contents of the filer after that position, or
* Leave the file as-is, and passing an error that tells the caller how many bytes were written up until the error.
Introducing the chunked writing also makes we wonder; Should there be a parameter on FileWriter, that lets callers change the chunksize, or turn off chunking. Mostly for the case where a write should either succeed or fail completely.
This PR should not be merged, until we figure out what to do here and I can update the tests and documentation accordingly.
Also, there is a `tests` folder. Are there any instruction on how to run the tests in the browser / on a real device?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-plugin-file] ataylor32 edited a comment on pull request #461: FileWriter writes data in chunks, and converts ArrayBuffers to Base64. Fixes Issue #364
Posted by GitBox <gi...@apache.org>.
ataylor32 edited a comment on pull request #461:
URL: https://github.com/apache/cordova-plugin-file/pull/461#issuecomment-947017857
@LightMind Regarding this quote from my previous comment:
> So I switched to the code from https://github.com/LightMind/cordova-plugin-file/blob/master/www/FileWriter.js
That wasn't a typo. I used the code from your fork's `master` branch. But what I ultimately ended up doing instead of my quick and dirty fix is I updated the `convertCurrentChunkToBase64AndWriteToDisk` function to do this:
```javascript
if (cordova.platformId === 'android') {
turnArrayBufferIntoBase64EncodedString(
writeConvertedChunk,
errorCallback,
arrayBuffer.slice(startOfChunk, endOfChunk)
);
} else {
writeConvertedChunk(arrayBuffer.slice(startOfChunk, endOfChunk));
}
```
So both platforms write the data in chunks, but on Android it's converted to base64 first.
Thanks for bringing https://github.com/apache/cordova-js/pull/242 to my attention. I will have to test with a future version of Cordova and see if that fixes it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-plugin-file] LightMind commented on pull request #461: FileWriter writes data in chunks, and converts ArrayBuffers to Base64. Fixes Issue #364
Posted by GitBox <gi...@apache.org>.
LightMind commented on pull request #461:
URL: https://github.com/apache/cordova-plugin-file/pull/461#issuecomment-946424638
@ataylor32 Thanks for the example app, I will use it for testing :)
> So I switched to the code from https://github.com/LightMind/cordova-plugin-file/blob/master/www/FileWriter.js ,
Do you mean you switched to the `issue-364` branch?
This is interesting, maybe something changed on the iOS side in either Cordova or Safari? When changing the code, I tried to not touch the iOS code path, so this pull request should not break it.
Do I understand correctly: Your version does chunked writing for Android and iOS, but instead of turning it into base64 encoded strings, you just pass the chunked binary data to cordova's `exec`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-plugin-file] LightMind commented on pull request #461: FileWriter writes data in chunks, and converts ArrayBuffers to Base64. Fixes Issue #364
Posted by GitBox <gi...@apache.org>.
LightMind commented on pull request #461:
URL: https://github.com/apache/cordova-plugin-file/pull/461#issuecomment-784000049
The failing tests do not seem to be related to my changes at all :/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-plugin-file] pietos commented on pull request #461: FileWriter writes data in chunks, and converts ArrayBuffers to Base64. Fixes Issue #364
Posted by GitBox <gi...@apache.org>.
pietos commented on pull request #461:
URL: https://github.com/apache/cordova-plugin-file/pull/461#issuecomment-855368376
Your commits solved my issue. I had a lot of memory issues in an app where we would download and store video files of around 50 mb. But on an average sync the memory heaps were between 10 and 600 mb, while the files were never bigger then 50mb.
After I inserted your commits, the memory load stayed stable between 10 and 50 mb, which solved the issue of apps crashing because the device was out of memory.
**Before your commit**
**After your commit**
Can this be merged with the master branch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-plugin-file] ataylor32 commented on pull request #461: FileWriter writes data in chunks, and converts ArrayBuffers to Base64. Fixes Issue #364
Posted by GitBox <gi...@apache.org>.
ataylor32 commented on pull request #461:
URL: https://github.com/apache/cordova-plugin-file/pull/461#issuecomment-947017857
@LightMind Regarding this quote from my previous comment:
> So I switched to the code from https://github.com/LightMind/cordova-plugin-file/blob/master/www/FileWriter.js
That wasn't a typo. I used the code from your fork's `master` branch. But what I ultimately ended up doing instead of my quick and dirty fix is I updated the `convertCurrentChunkToBase64AndWriteToDisk` function to do this:
```
if (cordova.platformId === 'android') {
turnArrayBufferIntoBase64EncodedString(
writeConvertedChunk,
errorCallback,
arrayBuffer.slice(startOfChunk, endOfChunk)
);
} else {
writeConvertedChunk(arrayBuffer.slice(startOfChunk, endOfChunk));
}
```
So both platforms write the data in chunks, but on Android it's converted to base64 first.
Thanks for bringing https://github.com/apache/cordova-js/pull/242 to my attention. I will have to test with a future version of Cordova and see if that fixes it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-plugin-file] LightMind commented on pull request #461: FileWriter writes data in chunks, and converts ArrayBuffers to Base64. Fixes Issue #364
Posted by GitBox <gi...@apache.org>.
LightMind commented on pull request #461:
URL: https://github.com/apache/cordova-plugin-file/pull/461#issuecomment-946427039
@ataylor32 please have a look at this merge request for cordova.js, it looks like they are improving the conversion to base64 on their side, which might make this issue obsolete at some point:
https://github.com/apache/cordova-js/pull/242
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-plugin-file] ataylor32 commented on pull request #461: FileWriter writes data in chunks, and converts ArrayBuffers to Base64. Fixes Issue #364
Posted by GitBox <gi...@apache.org>.
ataylor32 commented on pull request #461:
URL: https://github.com/apache/cordova-plugin-file/pull/461#issuecomment-944422863
@LightMind Thanks for the reply. It turns out I accidentally used the code from https://github.com/LightMind/cordova-plugin-file/blob/master/www/FileWriter.js instead of https://github.com/LightMind/cordova-plugin-file/blob/issue/364-writing-large-files/www/FileWriter.js . I switched to the correct code and iOS still crashes with large files. This is what I did to test:
1. Create a new Cordova project
1. Run `cordova plugin add https://github.com/LightMind/cordova-plugin-file#issue/364-writing-large-files`
1. Replace the contents of www/index.html with this:
```html
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8">
<meta name="viewport" content="initial-scale=1, width=device-width">
<title>Testing cordova-plugin-file</title>
<style>
#loaded_content {
display: none;
}
</style>
</head>
<body>
<div id="loading_content">
Loading...
</div>
<div id="loaded_content">
<p>
Status:
<span id="status">Waiting for you to choose a file</span>
</p>
<form id="form">
<input type="file" id="file_input">
</form>
</div>
<script src="cordova.js"></script>
<script>
document.addEventListener('deviceready', onDeviceReady, false);
function onDeviceReady() {
const status = document.getElementById('status');
const form = document.getElementById('form');
const fileInput = document.getElementById('file_input');
function resetForm() {
form.reset();
fileInput.disabled = false;
}
fileInput.addEventListener('change', (e) => {
fileInput.disabled = true;
const file = e.target.files[0];
window.requestFileSystem(window.LocalFileSystem.PERSISTENT, 0, (fs) => {
status.textContent = `Storing ${file.name}`;
fs.root.getFile(
file.name,
{ create: true, exclusive: false },
(fileEntry) => {
fileEntry.createWriter(async (fileWriter) => {
fileWriter.onwriteend = function () {
status.textContent = `Successfully stored ${file.name}. You may choose another file if you want.`;
resetForm();
};
fileWriter.onerror = function () {
status.textContent = 'Error';
resetForm();
};
fileWriter.write(file);
});
},
);
});
});
document.getElementById('loading_content').style.display = 'none';
document.getElementById('loaded_content').style.display = 'block';
}
</script>
</body>
</html>
```
After following the above steps, I ran this test project on an actual iPhone (I used an iPhone XS running iOS 14.7.1). The file I tried storing was a video that's 3 minutes and 30 seconds long, which I recorded using the Camera app. The original video is an HEVC video that's 1920 × 1080 and 209.9 MB (200.1 MiB), but when I choose it, iOS will first automatically compress it before trying to store it. The compressed version is an H.264 video that's 1280 × 720 and 70 MB (66.8 MB). I've tried it many times and it has never successfully stored the file. What happens is the app restarts itself and the "Status" text once again says "Waiting for you to choose a file". I removed the ` && cordova.platformId === 'android'` portion of your `if` statement and it stored base64 into the content of the file. So I switched to the code from https://github.com/LightMind/cordova-plugin-file/blob/master/www/FileWriter.js , applied my quick and dirty fix, and that worked.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-plugin-file] timbru31 commented on pull request #461: FileWriter writes data in chunks, and converts ArrayBuffers to Base64. Fixes Issue #364
Posted by GitBox <gi...@apache.org>.
timbru31 commented on pull request #461:
URL: https://github.com/apache/cordova-plugin-file/pull/461#issuecomment-784067882
No the CI is currently broken for plugins, no need to worry about that. Thanks a lot for your PR! 💯
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-plugin-file] LightMind edited a comment on pull request #461: FileWriter writes data in chunks, and converts ArrayBuffers to Base64. Fixes Issue #364
Posted by GitBox <gi...@apache.org>.
LightMind edited a comment on pull request #461:
URL: https://github.com/apache/cordova-plugin-file/pull/461#issuecomment-859593648
@pietos I am happy that these changes helped you, thanks for checking it out :)
Sorry that I have not been active on this issue. I think it's stable enough to be merged, but there are some points I feel I need to adres:
* Users of the plugin might want to disable this behavior or control the chunk size
* Documentation needs to explain that the writes are chunked, and if writes fail, the files can be in an inconsistent state.
* Needs a unit-test that writes a file with size bigger than the chunk size, to check that the file is written consistently. Probably needs to check file sizes are that are
- A multiple of the chunk size
- A multiple of the chunk size + 1 / -1
In general, its not clear what should be done, if an error shows up, after we have written the first chunk, but before the last chunk is done writing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-plugin-file] LightMind commented on pull request #461: FileWriter writes data in chunks, and converts ArrayBuffers to Base64. Fixes Issue #364
Posted by GitBox <gi...@apache.org>.
LightMind commented on pull request #461:
URL: https://github.com/apache/cordova-plugin-file/pull/461#issuecomment-859593648
@pietos I am happy that these changes helped you.
Sorry that I have not been active on this issue. I think it's stable enough to be merged, but there are some points I feel I need to adres:
* Users of the plugin might want to disable this behavior or control the chunk size
* Documentation needs to explain that the writes are chunked, and if writes fail, the files can be in an inconsistent state.
* Needs a unit-test that writes a file with size bigger than the chunk size, to check that the file is written consistently. Probably needs to check file sizes are that are
- A multiple of the chunk size
- A multiple of the chunk size + 1 / -1
In general, its not clear what should be done, if an error shows up, after we have written the first chunk, but before the last chunk is done writing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org