FileWriter writes data in chunks, and converts ArrayBuffers to Base64. Fixes Issue #364

[LightMind]

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

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

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))
• I've updated the documentation if necessary
• 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)

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.

[LightMind]

The failing tests do not seem to be related to my changes at all :/

[timbru31]

No the CI is currently broken for plugins, no need to worry about that. Thanks a lot for your PR! 💯

[pietos]

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?

[LightMind]

@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.

[ataylor32]

In my testing, this pull request's code appears to work fine on Android, but not iOS. Here's an example image:

https://en.wikipedia.org/wiki/File:ESM_Australia_ribbon.png

When I write the above PNG on iOS, the file's contents looks like this:

iVBORw0KGgoAAAANSUhEUgAAAGQAAAAeBAMAAAAodabAAAAALHRFWHRDcmVhdGlvbiBUaW1lAE1vbiAyMyBBcHIgMjAwNyAyMDoxMzoxMyArMTAwMOPXhbkAAAAHdElNRQfXBBcKDSKW+PWuAAAACXBIWXMAAAsSAAALEgHS3X78AAAABGdBTUEAALGPC/xhBQAAABJQTFRFCAhC3tbWxhAIY2Nz////3mtzHGNMBgAAADlJREFUeNpjMAaBUChwgQJcfLBiBjBQggJBKMDFh6getWXUlsFqCyHTYXwkWwiZDuOP2jJqy2C1BQCv4Q0C6Rns2QAAAABJRU5ErkJggg==

As you can see, it's the base64 encoded version of the image. So when I try to use the file as an image, it doesn't work. I would expect the file to be binary.

[ataylor32]

I performed a quick and dirty fix and haven't tested it thoroughly, but it seems to work, so far. Currently, this is what the convertCurrentChunkToBase64AndWriteToDisk function does:

turnArrayBufferIntoBase64EncodedString(
    writeConvertedChunk,
    errorCallback,
    arrayBuffer.slice(startOfChunk, endOfChunk)
);

I replaced the above with this:

writeConvertedChunk(arrayBuffer.slice(startOfChunk, endOfChunk));

With this quick and dirty fix in place, it looks like the files being written are binary on both Android and iOS, as I hoped. I'm not sure why this pull request converts to base64 first. Is there a good reason for that? Is there something I'm completely overlooking?

[LightMind]

Hi @ataylor32 , thank you for looking into this 👍

The original problem was, that the bridge between JS and the Android Native part doesn't support passing binary data.
The default was, that if you pass binary data ( ArrayBuffer, Blob ) to cordova.js exec method, cordova would encode the binary data as base64 with a memory intensive algorithm, crashing our apps.

By changing writeBase64EncodedStringInChunks so that it runs

writeConvertedChunk(arrayBuffer.slice(startOfChunk, endOfChunk));

instead, you are letting cordova.js do the conversion from binary to base64 for you, which caused the original problem.

That might work OK, because the chunks are small enough. I can not remember what filesizes were causing issues.

What is very worrying about your finding is, that the change affects iOS at all. On iOS, the chunked writing should not happen, I wrote a guard:

if (supportsBinary && (data instanceof ArrayBuffer) && cordova.platformId === 'android') {
        writeBase64EncodedStringInChunks.call(
            me,

So I am not sure what is going on with that :/

[ataylor32]

@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
2. Run cordova plugin add https://github.com/LightMind/cordova-plugin-file#issue/364-writing-large-files
3. Replace the contents of www/index.html with this:

<!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.

[LightMind]

@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?

[LightMind]

@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:

apache/cordova-js#242

[ataylor32]

@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 apache/cordova-js#242 to my attention. I will have to test with a future version of Cordova and see if that fixes it.