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

Mobile(iOS safari) crashes with multiple upload and ngf-resize enabled #1307

Open
shawnzam opened this issue Jan 27, 2016 · 40 comments
Open

Comments

@shawnzam
Copy link

I am working with the latest version of ng-file-upload.

I have a button with the following attributes.

      <button 
            ngf-resize="{width: 500, quality: .8}" 
            ngf-fix-orientation="true" 
            ngf-select="uploadFiles($files)" 
            ngf-before-model-change="startResizing($files)"
            multiple
            accept="image/*">
            Select Files
        </button>

And a controller like so:

$scope.uploadFiles = function (files) {
            $scope.files = files;
            if (files && files.length) {
                $scope.uploading = true;
                Upload.upload({
                    url: '/myurl',
                    data: {
                        files: files
                    }
                }).then(function (response) {
                    $timeout(function () {
                        $scope.result = response.data;
                        $scope.uploading = false;
                        $scope.files = null;
                    });
                }, function (response) {
                    if (response.status > 0) {
                        $scope.errorMsg = response.status + ': ' + response.data;
                    }
                }, function (evt) {
                    $scope.progress = Math.min(100, parseInt(100.0 * evt.loaded / evt.total));
                });
            }
        };

This seems to only work if less than 6 photos are selected from an iPhone 6. If I select more, mobile safari either crashes or reloads the page displaying a warning that something went wrong. Any idea how to solve this issue?

@danialfarid
Copy link
Owner

You can do the resizing and fixing orientation 5 at a time with delays. Remove the directives and on ngf-change split the files into batch of fives and use Upload.resize() and Upload.applyExifRotation() to resize and fix orientation. It is probably running out of memory during all those transformations.

@shawnzam
Copy link
Author

Thanks for the tips. I am still seeing crashing iphones with the following setup

<button ng-hide="uploading" accept="image/*" class="button" ngf-select ngf-change="filesUpdated($files)" ngf-multiple="true">Select Files</button>
        $scope.filesUpdated = function(files) {
            if (files && files.length) {
                $scope.counts = files.length;
                $scope.files = files;
                while ($scope.files.length > 0) {
                    var f = $scope.files.pop();
                    Upload.resize(f, 500).then(function(f) {
                        return f;
                    }).then(function(f) {
                        Upload.applyExifRotation(f).then(function(f) {
                            console.log(f);
                            $scope.uploadFile(f);
                        })
                    })
                }
            }
        }

If you could offer a suggestion that would be very helpful. I am starting to look at the Safari iphone web inspector, so that should help as well.

@danialfarid
Copy link
Owner

You missed the part to split the files into chunks of 5 and do the processing sequentially.
Something like:

// break files into array of array of files. each chunk has at most 5 files.
fileChinks = ...;

function uploadChink(i) {
    var uploaded = 0;
    angular.forEach(fileChunks[i], function(f) {
         Upload.resize(f, 500).then(function(f) {
                        return f;
                    }).then(function(f) {
                        Upload.applyExifRotation(f).then(function(f) {
                            console.log(f);
                            uploaded++;                            
                            $scope.uploadFile(f);
                            if (uploaded == 5) {
                                // upload next chunk after the first 5 are completed.
                                uploadChunk(i + 1);
                            }
                        })
                    });
    });
}

@shawnzam
Copy link
Author

Thanks, I got this working now.

@shawnzam
Copy link
Author

This is what I have, and it is working pretty well. However I am still seeing crashes when > 70 images are selected. I am hoping to handle upwards of 250 images at a time. Seems like the browser is still running out of memory. Any advice is much appreciated.

Here is my entire controller

var _ = lodash;
        $scope.info = false;
        $scope.uploading = false;
        $scope.remaining = 0;
        $scope.total = 0;
        var CHUNKSIZE = 5;
        $scope.doit = function(files) {
            $scope.remaining = files.length;
            $scope.fileChunks = _.chunk(files, CHUNKSIZE);
            $scope.uploadChunk(0);
        }

        $scope.uploadChunk = function(i) {
            var uploaded = 0;
            angular.forEach($scope.fileChunks[i], function(f) {
                Upload.resize(f, 1000).then(function(f) {
                    return f;
                }).then(function(f) {
                    Upload.applyExifRotation(f).then(function(f) {
                        uploaded++;
                        $scope.uploadFile(f);
                        if (uploaded == CHUNKSIZE) {
                            $scope.uploadChunk(i + 1);
                        }
                    })
                });
            });
        }
        $scope.uploadFile = function(file) {
            if (file) {
                $scope.uploading = true;
                Upload.upload({
                    url: '/api/studies/' + $stateParams.study_id + '/participants/' + $stateParams.participant_id + '/albums/' + $stateParams.album_id + '/photos/upload',
                    data: {
                        file: file
                    }
                }).then(function(res) {
                    $timeout(function() {
                        $scope.result = res.data;
                        $scope.total++;
                        console.log($scope.total);
                        $scope.info_msg = $scope.total + " of " + $scope.remaining + " photos uploaded successfully";
                        if ($scope.remaining <= $scope.total) {
                            $scope.uploading = false;
                        }
                    });
                }, function(err) {
                    if (err.status > 0) {
                        $scope.errorMsg = err.status + ': ' + err.data;
                    }
                }, function(evt) {
                    $scope.progress = Math.min(100, parseInt(100.0 * evt.loaded / evt.total));
                });

            }
        }

@shawnzam shawnzam reopened this Jan 29, 2016
@shawnzam
Copy link
Author

Also it seem like images are not rotated correctly. Am I missing anything with my implementation of applyExifRotation?

@danialfarid
Copy link
Owner

You can try to make the chunk size smaller or uploading the next chunk in a $timeout to give it more time. The rotation code seems to be fine.

@shawnzam
Copy link
Author

Thanks again for working with me on this. It seems that chunking is not reducing the memory footprint. Running the code in this fiddle uses upwards of 1.1gb of ram for 171 images totaling 694.3 MB. It appears the promises are causing the memory to spike. Is there a way to iterate each file opening it for resizing and uploading and closing it before moving to the next file.

Fiddle: http://jsfiddle.net/nwy08oak/11/ (If you select a folder full of images and monitor chrome's Task Manager, you can see the memory use. Something seems to be leaking.)

Thanks

@danialfarid
Copy link
Owner

The memory usage goes down for me once the garbage collector kicks in. Have you tried the timeout approach I mentioned? http://jsfiddle.net/nwy08oak/12/
this works for me for large amount of images.

@shawnzam
Copy link
Author

shawnzam commented Feb 5, 2016

I am still struggling with this. Do you have any additional suggestions? Safari on my Iphone 6 crases at about 85 images in when visiting http://jsfiddle.net/nwy08oak/12/. I think the promises are not resolving properly and garbage collection is missing them.

Thanks again for all your help,
S

@danialfarid
Copy link
Owner

Does it work on chrome Iphone6?

@shawnzam
Copy link
Author

shawnzam commented Feb 5, 2016

Nope. Crashes tab. Memory uses are near 1.2gb

On Friday, February 5, 2016, Danial Farid notifications@github.com wrote:

Does it work on chrome Iphone6?


Reply to this email directly or view it on GitHub
#1307 (comment)
.

@danielbonnell
Copy link

I've encountered this same issue on Chrome for OS X this morning. If I select 10-15 images at 4MB each it works fine, but if I select more (e.g. 30 or 50) it crashes. I wonder if it would help solve this issue if you set the directive to immediately start uploading images once they've been processed. It seems like currently it processes all the selected images and waits to start uploading until that task has completed. Maybe that would solve the memory issue, which is what this looks like.

Update

I also tested this on Chrome (latest) for Windows and it crashed with just 10 photos selected (5MB each).

@danialfarid
Copy link
Owner

@danielbonnell what's your code? Does the jsfiddle mentioned above crashes too on windows?

@danielbonnell
Copy link

@danialfarid I didn't test the JSFiddle on Windows, just my code. In my controller I have the following upload function: I can test the JSFiddle on Windows if it's helpful. I don't have access to that machine at the moment.

function upload(files) {
    var uploadedCount = files.length;
    function cycle(files) {
        $scope.totalUploadedCount = files.length;
        $scope.uploading = true;
        var promises = [];
        for (var i = 0; i < files.length; i++) {
            var file = files[i];
            var promise = Upload.upload({
                url: '/photos.json',
                file: file
            }).success(function(data, status, headers, config){
                $scope.uploadedCount += 1;
                $scope.uploadedPercent = Math.round(($scope.uploadedCount / $scope.totalUploadedCount) * 100);
            });
            promises.push(promise);
        };
        return $q.all(promises);
    };

    cycle(files).then(function(result) {
        if(uploadedCount > 1) {
            $scope.lastAction = 'uploaded ' + uploadedCount + ' photos';
        } else {
            $scope.lastAction = 'uploaded ' + uploadedCount + ' photo';
        };
        successModal();

        $scope.totalUploadedCount = 0;
        $scope.uploadedCount = 0;
        $scope.uploadedPercent = 0;
        $scope.uploading = false;
        $scope.failedUploads = [];
        $scope.newPhotos = {
            token: $scope.token,
            files: undefined
        };
        photoUploadBtn.removeClass('disabled');
    });
    if(typeof(Intercom) === 'function') Intercom('trackEvent', 'uploaded-photos', {'count': uploadedCount});
};

In my view I have the following button:

<div class="btn btn-default" style="float:right;margin-right:10px;" ng-model="editPhotos.files" ngf-select="submit($files, $event);" ngf-multiple="true" ngf-resize="{width: 1280, height: 1280}" name="files" ngf-pattern="image/*" accept="image/*" ngf-max-size="10MB" required>
    Upload Images
    <i class="fa fa-upload"></i>
</div>

I just tested the JSFiddle on my Mac and I had no problem "uploading" 250+ photos. Should I be resizing the photos in my upload function instead of using ngf-resize on my button?

@danielbonnell
Copy link

I refactored my upload function a bit and tried throwing 250+ photos at it. It didn't crash, but it's using an absurd amount of memory, which seems strange since it's uploading them as it processes them, which it was not doing before. I also removed ngf-resize from my button.

Here is the function:

function upload(files) {
    var count = files.length;
    $scope.totalUploadedCount = files.length;
    $scope.uploading = true;
    files.forEach(function(file, index) {
        Upload.resize(files[index], 1280).then(function(result){
            Upload.upload({
                url: '/photos.json',
                file: result
            }).success(function(data, status, headers, config){
                $scope.uploadedCount += 1;
                $scope.uploadedPercent = Math.round(($scope.uploadedCount / $scope.totalUploadedCount) * 100);

                if(count === index + 1) {
                    if(count > 1) {
                        $scope.lastAction = 'uploaded ' + count + ' photos';
                    } else {
                        $scope.lastAction = 'uploaded ' + count + ' photo';
                    };
                    successModal();

                    $scope.uploading = false;
                    $scope.newPhotos.files = undefined;
                    photoUploadBtn.removeClass('disabled');
                    if(typeof(Intercom) === 'function') Intercom('trackEvent', 'uploaded-photos', {'count': count});
                }
            });
        })
    });
};

When I check the Chrome Task Manager I can see that at its peak, the tab was using 3.0GB of memory, vs about 146MB base usage. Not a problem on my system, but would likely crash lesser computers.

@danielbonnell
Copy link

@danialfarid I just tested that JSFiddle on Windows (Windows 7, 4GB RAM) with 42 images (5MB each) and it did NOT crash.

I also tested my last code snippet on the same Windows and it crashed with 42 pictures (5MB each). On my Mac with 8GB of RAM it didn't crash, even when I threw 260+ 5MB images at it at once.

@shawnzam
Copy link
Author

shawnzam commented Feb 8, 2016

@danielbonnell Thanks for providing some additional examples. I am hoping to use this library on a mobile application where RAM is much more sparse. My hunch is that the promises are never collected properly.

@danialfarid Thanks for all your work thus far on this. Kindly let me know if this is something you think you can tackle. If not, I am happy to poke under the hood and create a pull request.

@shawnzam shawnzam closed this as completed Feb 8, 2016
@shawnzam shawnzam reopened this Feb 8, 2016
@shawnzam
Copy link
Author

shawnzam commented Feb 8, 2016

It appears that for each resize() call within resize.js you create

    var canvasElement = document.createElement('canvas');
    var imageElement = document.createElement('img');

These are added to the document and hence never cleared during garbage collection. I am a little shaky with promises still, so how do you suggest modifying the resize function to remove these elements from the dom?

@danialfarid
Copy link
Owner

@shawnzam they are not added to the document they are just created in a local var and will be garbage collected. I am pretty sure there is no memory leak but there might be some unnecessary data being stored for the purpose of caching and faster response which could be optimized like $ngfDataUrl. I will try to find those and reduce the memory consumption for the later releases.

@shawnzam
Copy link
Author

shawnzam commented Feb 8, 2016

Thank a lot. For now I will add a cumbersome 3 second timeout for each chunk, with a chunk size for 10. This allows for ~80 images safely from my testing with mobile devices. I will limit the upload to 80 images and ask users to repeat the process for larger sets. Kindly let me know how you make out.

@danielbonnell
Copy link

@danialfarid I still suspect it may be memory related. I ran a test this morning and I noticed something peculiar. When I first load up my app I can see that it's consuming 280MB of memory. I then select 50 files to upload. As expected, the memory usages spikes up to 1.2GB. But then once the process completes, it only drops back down to 946MB, which doesn't make sense. I tried @shawnzam idea of setting a delay, which definitely seems to have improved the performance, but memory is still an issue.

@danialfarid
Copy link
Owner

@danielbonnell it won't drop back to where it started, garbage collection is done on demand when extra memory is needed so as long as it doesn't crash if you keep this process going, like for 1000 files there is no memory leak, however holding 1000 files data in memory might consume all the available memory and make it crash. You can try repeating the process that bumped thee memory usage to 1GB several times to see if it would eventually crash.

@danialfarid
Copy link
Owner

At version 12.0.1 I have made the plugin to use less memory for storing data urls and blob urls.
Let me know if this helps with the crashing issue.
You can also play with these two defaults to lower the memory consumption.

Upload.defaults.blobUrlsMaxMemory = 268435456 // default max total size of files stored in blob urls.
Upload.defaults.blobUrlsMaxQueueSize = 200 // default max number of blob urls stored by this application.

@shawnzam
Copy link
Author

Thanks. I'll give it a try tomorrow. Do you still suggest we chunk the files list?

@danialfarid
Copy link
Owner

Maybe, just try it without chunking first, and lower the queue size if it crashes to see if that helps.

@shawnzam
Copy link
Author

@danialfarid Thanks for the update. Can you confirm that this JSFiddle uses the preferred method for optimizing memory. http://jsfiddle.net/swqdyvs2/3/

At first glance I am not seeing any improvement in memory use. Even when modifying blobUrlsMaxMemory or blobUrlsMaxQueueSize.

@danialfarid
Copy link
Owner

Use setDefaults instead Upload.setDefaults({blobUrlsMaxQueueSize: 2}).
I think you still need to do it in chunks since otherwise it tries to resize all the files at the same time which would be very heavy and might cause crash.

@shawnzam
Copy link
Author

I am sorry to report that this release may have made the issue worse on mobile devices. At least on an iPhone. Here is my working fiddle:

http://jsfiddle.net/n5Lf4o84/2/

I am using https://cdnjs.cloudflare.com/ajax/libs/danialfarid-angular-file-upload/12.0.1/ng-file-upload-all.min.js

No matter what I setDefaults to I seem to crash the browser with < 40 images.

I also tried extending the timeout from 100ms, to 1000ms, but this did not have any effect.

@danialfarid
Copy link
Owner

So if you switch back to 11.x.x with the same fiddle it would let you upload more files until it crashes?

@danialfarid
Copy link
Owner

Also in the fiddle you are not setting the defaults correctly. see the code I posted, setDefaults is a function not an object. Try to set the queue size to 1 like I specified above.

@shawnzam
Copy link
Author

I am sorry, I sent you the wrong fiddle. That was an out of date one!

Even setting my blobUrlsMaxQueueSize to 2 results in a crashing phone and over 1.2 gb of memory.
http://jsfiddle.net/n5Lf4o84/8/

@danialfarid
Copy link
Owner

Is it any better than the older version? Does it handle more files before it crashes?

@shawnzam
Copy link
Author

ng-file-upload v(12.01)

  1. http://jsfiddle.net/n5Lf4o84/9/ | 28 images | Chrome Version 48.0.2564.109 (64-bit) | ng-file-upload v(12.01) | max memory (900mb) | 97 seconds
  2. http://jsfiddle.net/n5Lf4o84/9/ | 26 images | i-os safari | ng-file-upload v(12.01) | Does not crash | 36 seconds
  3. http://jsfiddle.net/n5Lf4o84/9/ | 72 images | ios safari | ng-file-upload v(12.01) | crases after 42 images | 102 seconds

ng-file-upload v(11.2.3)

  1. http://jsfiddle.net/n5Lf4o84/10/ | 28 images | Chrome Version 48.0.2564.109 (64-bit) | ng-file-upload v(11.2.3) | max memory (1200mb) | 86 seconds
  2. http://jsfiddle.net/n5Lf4o84/10/ | 26 images | i-os safari | ng-file-upload v(11.2.3) | Does not crash | 56 seconds
  3. http://jsfiddle.net/n5Lf4o84/10/ | 72 images | ios safari | ng-file-upload v(11.2.3) | crases after 46 images | 138 seconds

@danialfarid
Copy link
Owner

ok so it is about the same in both versions. Try to make the process slower, more delay and less files at a time. You can also try to splice the files that are already resized out of the files array and upload them to release the reference to those files.

1 similar comment
@danialfarid
Copy link
Owner

ok so it is about the same in both versions. Try to make the process slower, more delay and less files at a time. You can also try to splice the files that are already resized out of the files array and upload them to release the reference to those files.

@danialfarid danialfarid reopened this Feb 22, 2016
@shawnzam
Copy link
Author

Thanks for sticking with me on this. Do you mean slice from the fileChunks array? Can you kindly elaborate and/or post a snippet.

@danialfarid
Copy link
Owner

Something like this: http://jsfiddle.net/n5Lf4o84/12/

@shawnzam
Copy link
Author

shawnzam commented Mar 3, 2016

Thanks, Just to fill you in that this solution does not seem to affect memory usage. I appreciate all your work, but I am not able to invest anymore time in solving this issue. If you are able to make progress, kindly update this issue, as I'll try to keep an eye on it.

thx

@zspitzer
Copy link

here is the Safari bug, if you are affected, add yourself to the CC list
https://bugs.webkit.org/show_bug.cgi?id=172533

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

No branches or pull requests

4 participants