terikon / cordova-plugin-photo-library

Maintainer needed. Please contact if you're using this library in your project
MIT License
149 stars 295 forks source link

The code takes ~15 seconds to run #34

Closed kartagis closed 7 years ago

kartagis commented 7 years ago
var doUIChanges = function(library) {
  library.forEach(function(libraryItem) {
      $("#gallery").css("display", "none");
      image = "<img height='100px' width='100px' src='"+libraryItem.thumbnailURL+"' style='margin: 5px' />";
      $(image).appendTo(document.body);
  });
};
$("#gallery").on("click", function() {
  cordova.plugins.photoLibrary.getLibrary().then(doUIChanges);
});

Hello, I found this plugin, hoping to create my own grid with checkboxes on selected images and so on, but why would it take this long?

viskin commented 7 years ago

What platform are you using? Android or iOS? Can you please tell how many results getLibrary returns in your case?

kartagis commented 7 years ago

I'm using Android, and I have quite a few images (I don't know exactly how many).

viskin commented 7 years ago

I want to address this issue this week. Returning data in chunks can be a solution. But some data will really help me. Can you please print out the library.length on your device? And it will be really great if you can tell me from Photos app approximately how many albums do you have?

kartagis commented 7 years ago

I've got 6 photo albums, and 1 video album. Also, .length gave me 2265 after ~3 seconds.

viskin commented 7 years ago

Ok, thanks. So getLibrary runs for 3 seconds (which is still too long for good user experience). And adding the images with jQuery, calling thumbnailURL, takes another 12 seconds. Do I get it right? 2200 is not very huge number, but I don't think adding all the photos at once to document.body is a good solution. I'd use some sort of virtualized list, like infiniteScroll. You call thumbnailURL only for photos that are currently visible, so user doesn't wait for photos to load.

viskin commented 7 years ago

There's also MegaList for jQuery.

kartagis commented 7 years ago

Let me do it separately. I'll get back to you in a minute.

On 20 January 2017 at 10:45, viskin notifications@github.com wrote:

Ok, thanks. So getLibrary runs for 3 seconds (which is still too long for good user experience). And adding the images with jQuery, calling thumbnailURL, takes another 12 seconds. Do I get it right? 2200 is not very huge number, but I don't think adding all the photos at once to document.body is a good solution. I'd use some sort of virtualized list, like infiniteScroll http://www.infinite-scroll.com/infinite-scroll-jquery-plugin. You call thumbnailURL only for photos that are currently visible, so user doesn't wait for photos to load.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/terikon/cordova-plugin-photo-library/issues/34#issuecomment-273998882, or mute the thread https://github.com/notifications/unsubscribe-auth/ABiHXvGVKAdijSSl_B4bCzrDgbo6vfB1ks5rUGYjgaJpZM4Lo5JO .

--

kartagis commented 7 years ago

Getting all the images took 7466.497ms and adding them to DOM 0.901ms. However, they might have been cached.

On 20 January 2017 at 10:49, Tolga Ozses tolga@ozses.net wrote:

Let me do it separately. I'll get back to you in a minute.

On 20 January 2017 at 10:45, viskin notifications@github.com wrote:

Ok, thanks. So getLibrary runs for 3 seconds (which is still too long for good user experience). And adding the images with jQuery, calling thumbnailURL, takes another 12 seconds. Do I get it right? 2200 is not very huge number, but I don't think adding all the photos at once to document.body is a good solution. I'd use some sort of virtualized list, like infiniteScroll http://www.infinite-scroll.com/infinite-scroll-jquery-plugin. You call thumbnailURL only for photos that are currently visible, so user doesn't wait for photos to load.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/terikon/cordova-plugin-photo-library/issues/34#issuecomment-273998882, or mute the thread https://github.com/notifications/unsubscribe-auth/ABiHXvGVKAdijSSl_B4bCzrDgbo6vfB1ks5rUGYjgaJpZM4Lo5JO .

--

--

viskin commented 7 years ago

Yeah, what takes time is preparing thumbnail when using thumbnailURL. And yes the images are cached by browser. What about time it takes... 7 sec for 2200 photos seems OK to me, it's 3.4ms per photo. If virtualized list used, this should not be a problem. I'll try to address 3 seconds for getLibrary, this should be chunked. The idea behind photo-library is to make photos and thumbnails accessible by url, so it will be possible to use plenty of web libraries for displaying them. Just assume that getting data by thumbnailURL is not so different from retrieving data from server, and solve the performance problem same way you'd solve it for accessing photos from remote urls. Like retrieving only needed photos this case.

kartagis commented 7 years ago

Virtualised list?

On 20 January 2017 at 11:26, viskin notifications@github.com wrote:

Yeah, what takes time is preparing thumbnail when using thumbnailURL. And yes the images are cached by browser. What about time it takes... 7 sec for 2200 photos seems ok to me, it's 3.4ms per photo. If virtualized list used, this should not be a problem. I'll try to address 3 seconds for getLibrary, this should be chunked. The idea behind photo-library is to make photos and thumbnails accessible by url, so it will be possible to use plenty of web libraries for displaying them. Just assume that getting data by thumbnailURL is not so different from retrieving data from server, and solve the performance problem same way you'd solve it for accessing photos from remote urls. Retrieving only photos needed in this case.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/terikon/cordova-plugin-photo-library/issues/34#issuecomment-274007701, or mute the thread https://github.com/notifications/unsubscribe-auth/ABiHXgj1VC-DuTChBp6lpmykqtsvMxECks5rUG_HgaJpZM4Lo5JO .

--

viskin commented 7 years ago

Loading thouthands of photos cannot take less than couple of seconds, while user wants to see the results immediately. Please try infiniteScroll or MegaList if you use jQuery. Use results of getLibrary as data source, and let one of them load thumbnails by url for you when needed. This should work. So you should get 3 seconds before user sees something. These 3 seconds can be addressed later with chunking, when it will be implemented for Android.

viskin commented 7 years ago

And you also can give a try to PhotoSwipe library, if it suits your needs.

kartagis commented 7 years ago

Hello, Any luck addressing the issue?

kartagis commented 7 years ago

MegaList with a few CSS changes worked very well for me, thanks a lot for recommending that 👍

viskin commented 7 years ago

You welcome. Planning to address partial results this or next week.

kartagis commented 7 years ago

Following up from #35:

Have you been able to address this yet?

viskin commented 7 years ago

I think the solution will be like this: getLibrary will get partialCallback parameter (just as it does now), and partial callback will be called each 0.5 sec. In contrast to current implementation, it will receive chunk of library array, that was added from previous time. So even when loading all the photos will take some time, you always have some data. Finally, usual result callback will be called with full data. Current implementation returns chunks of library [0...up to now], I will change it to [last time...up to now] Do you think this solution is appropriate?

kartagis commented 7 years ago

As long as the minimum amount of time is < 2 seconds, it should work. On Tue, 31 Jan 2017 at 17:00, viskin notifications@github.com wrote:

I think the solution will be like this: getLibrary will get partialCallback parameter (just as it does now), and partial callback will be called each 0.5 sec. In contrast to current implementation, it will receive chunk of library array, that was added from previous time. So even when loading all the photos will take some time, you always have some data. Finally, usual result callback will be called with full data. Current implementation returns chunks of library [0...up to now], I will change it to [last time...up to now] Do you think this solution is appropriate?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/terikon/cordova-plugin-photo-library/issues/34#issuecomment-276369841, or mute the thread https://github.com/notifications/unsubscribe-auth/ABiHXhWHcV4tudcsnN8JLCmG0uAMSXO1ks5rXz5xgaJpZM4Lo5JO .

--

ChadKoder commented 7 years ago

@viskin I am using this plugin with ios and would love to see your solution implemented. I have tried getting the library in chunks, but getting 0.. up to now, is not ideal. It causes just as much lag as loading the full library in my case. I currently have a little over 1300 images in my library and it takes 20+ seconds to load to full library for some reason.

viskin commented 7 years ago

@ChadKoder I will try to simulate 1300 images on my device and will try to improve raw performance.

ChadKoder commented 7 years ago

1,381 photos on my device to be exact. I'm running on an iPhone 5s version 10.2. This plugin is a much better approach than using input type='file', by the way. I'm glad to have found it, but the slowness I see is almost as bad as when I was waiting for the results to return from the file input... I look forward to your update.

viskin commented 7 years ago

I successfully reproduced this problem on my iPhone device. I have 780 photos, and it takes 26 sec for getLibrary to run. I will try to improve this.

ChadKoder commented 7 years ago

I also wanted to mention you may need to update the readme section that mentions how to use this plugin with angular. I was getting the content security policy issue until I switched the whitelist sanitization to $compileProvider.imgSrcSanitizationWhitelist(/^\s*(https?|cdvphotolibrary):/); instead of using aHrefSanitizationWhitelist. I did a few tests and it seems to only need this, it wouldn't work at all with aHrefSanitizationWhitelist.

viskin commented 7 years ago

@ChadKoder It would be great if you'd update the readme and provide PR. I don't use angular, so I just added aHrefSanitizationWhitelist as one of library users explained.

viskin commented 7 years ago

Latest version (still not published to npm) improved performance on iOS significantly. getLibrary for my 780 photos now run 4.5 sec instead of 26. To achieve this, few fields were removed from libraryItem (I don't think they were so useful, like mimeType and nativeURL). Next step is to improve chunked output, so result will start to appear immediately to user, if needed.

kartagis commented 7 years ago

Great news :) Should we expect less times or can I install now?

On 9 February 2017 at 21:57, viskin notifications@github.com wrote:

Latest version (still not published to npm) improved performance on iOS significantly. getLibrary for my 780 photos now run 4.5 sec instead of 26. To achieve this, few fields were removed from libraryItem (I don't think they were so useful, like mimeType and nativeURL). Next step is to improve chunked output, so result will start to appear immediately to user, if needed.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/terikon/cordova-plugin-photo-library/issues/34#issuecomment-278738043, or mute the thread https://github.com/notifications/unsubscribe-auth/ABiHXmrlSHLCbXgg0UOR-ZhENh9fS8Pdks5ra2GrgaJpZM4Lo5JO .

--

viskin commented 7 years ago

@kartagis You can try it from latest git.

ChadKoder commented 7 years ago

@viskin I just tried the latest, I now have over 1400 images and it took around 4.5 seconds as well. Great job! I'll enter a PR when I have an extra minute for the angular sanitization update in the readme.

viskin commented 7 years ago

@ChadKoder Nice to hear.

viskin commented 7 years ago

I added possibility for chunked output (itemsInChunk and chunkTimeSec parameters). So now an user shouldn't wait at all for first items to appear.

kartagis commented 7 years ago

So no need to use MegaList or such?

On Sat, 11 Feb 2017 at 19:09, viskin notifications@github.com wrote:

I added possibility for chunked output (itemsInChunk and chunkTimeSec parameters). So now an user shouldn't wait at all for first items to appear.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/terikon/cordova-plugin-photo-library/issues/34#issuecomment-279155990, or mute the thread https://github.com/notifications/unsubscribe-auth/ABiHXrtscSuXGQONupK0Rcx1wUcQ5dtIks5rbd1UgaJpZM4Lo5JO .

--

viskin commented 7 years ago

@kartagis You still need it. Using something like MegaList enables displaying photos immediately after getLibrary finishes. Still, calling getLibrary takes some time if there're many photos (likely 4.5 sec for 1400 photos). Such delay is not acceptable for user that navigates to photos page in your application. To make photos appear immediately for your user, you can use new chunked functionality. You can ask getLibrary to return first result after say 0.3 seconds. You display this first chunk of library to user. Meanwhile, getLibrary will continue to provide new chunks, which you add to list of items that you display. Retrieving whole library still will take 4.5 seconds, but your user will not care, as it will see the photos after 0.3 seconds (kind-of immediately for him).

kartagis commented 7 years ago

Sadly, 1229 photos take 20~ sec to load (on iOS) and this is my code:

function readLibrary() {
  var st=new Date().getTime();
  cordova.plugins.photoLibrary.getLibrary(function(library, isLastChunk) {
    var et=new Date().getTime();
    var diff=et-st;
    alert(library.length+' photos took '+diff+' ms to load');
    ActivityIndicator.hide();
    $("#images").megalist();
    library.forEach(function(libraryItem) {
      $("#gallery").css("display", "none");
      image = "<div class='imgHolder'><img height='100px' class='img' width='100px' src='"+libraryItem.thumbnailURL+"' style='margin: 5px' /></div>";
      $(image).appendTo($("#images"));
    });
    $(".imgHolder").on("click",function(){
      //$(this).toggleClass("checked");
      console.log($(this).thumbnailURL);
      //images.push($(this).thumbnailURL);
    })
  }, function(err) {
    console.log(err);
  }, { 
    thumbnailHeight:384,
    thumbnailWidth:512,
    quality:1,
    itemsInChunk:100,
    chunkTimeSec:.5,
    useOriginalFileNames:false,
  });
}
viskin commented 7 years ago

How do you include the library in your config.xml?

On יום ב׳, 13 בפבר׳ 2017 at 8:29 Tolga Ozses notifications@github.com wrote:

Sadly, 1229 photos take 20~ sec to load (on iOS) and this is my code:

function readLibrary() {

var st=new Date().getTime();

cordova.plugins.photoLibrary.getLibrary(function(library, isLastChunk) {

var et=new Date().getTime();

var diff=et-st;

alert(library.length+' photos took '+diff+' ms to load');

ActivityIndicator.hide();

$("#images").megalist();

library.forEach(function(libraryItem) {

  $("#gallery").css("display", "none");

  image = "<div class='imgHolder'><img height='100px' class='img' width='100px' src='"+libraryItem.thumbnailURL+"' style='margin: 5px' /></div>";

  $(image).appendTo($("#images"));

});

$(".imgHolder").on("click",function(){

  //$(this).toggleClass("checked");

  console.log($(this).thumbnailURL);

  //images.push($(this).thumbnailURL);

})

}, function(err) {

console.log(err);

}, {

thumbnailHeight:384,

thumbnailWidth:512,

quality:1,

itemsInChunk:100,

chunkTimeSec:.5,

useOriginalFileNames:false,

});

}

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/terikon/cordova-plugin-photo-library/issues/34#issuecomment-279304604, or mute the thread https://github.com/notifications/unsubscribe-auth/AKYX9KXSq1TtGbdmOVz0g4NdAGiMttg5ks5rb_g4gaJpZM4Lo5JO .

kartagis commented 7 years ago

On 13 February 2017 at 13:36, viskin notifications@github.com wrote:

How do you include the library in your config.xml?

On יום ב׳, 13 בפבר׳ 2017 at 8:29 Tolga Ozses notifications@github.com wrote:

Sadly, 1229 photos take 20~ sec to load (on iOS) and this is my code:

function readLibrary() {

var st=new Date().getTime();

cordova.plugins.photoLibrary.getLibrary(function(library, isLastChunk) {

var et=new Date().getTime();

var diff=et-st;

alert(library.length+' photos took '+diff+' ms to load');

ActivityIndicator.hide();

$("#images").megalist();

library.forEach(function(libraryItem) {

$("#gallery").css("display", "none");

image = "

<img height='100px' class='img' width='100px' src='"+libraryItem.thumbnailURL+"' style='margin: 5px' />
";

$(image).appendTo($("#images"));

});

$(".imgHolder").on("click",function(){

//$(this).toggleClass("checked");

console.log($(this).thumbnailURL);

//images.push($(this).thumbnailURL);

})

}, function(err) {

console.log(err);

}, {

thumbnailHeight:384,

thumbnailWidth:512,

quality:1,

itemsInChunk:100,

chunkTimeSec:.5,

useOriginalFileNames:false,

});

}

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/terikon/cordova-plugin-photo-library/ issues/34#issuecomment-279304604, or mute the thread https://github.com/notifications/unsubscribe-auth/ AKYX9KXSq1TtGbdmOVz0g4NdAGiMttg5ks5rb_g4gaJpZM4Lo5JO

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/terikon/cordova-plugin-photo-library/issues/34#issuecomment-279350134, or mute the thread https://github.com/notifications/unsubscribe-auth/ABiHXhMQHuwnSpN5WeJVd5jFnKnbCauCks5rcDIogaJpZM4Lo5JO .

--

kartagis commented 7 years ago

After adding --variable, I was able to get the library in chunks, but how to add to DOM in chunks also?

viskin commented 7 years ago

@kartagis Maybe something like this:

function readLibrary() {
  var firstChunk = true;
  cordova.plugins.photoLibrary.getLibrary(function(library, isLastChunk) {
    if (firstChunk) {
      $("#images").megalist();
      firstChunk = false;
    }
    library.forEach(function(libraryItem) {
      $("#gallery").css("display", "none");
      image = "<div class='imgHolder'><img height='100px' class='img' width='100px' src='"+libraryItem.thumbnailURL+"' style='margin: 5px' /></div>";
      // attach onClick handler to image here
      $(image).appendTo($("#images"));
    });
  }, function(err) {
    console.log(err);
  }, { 
    thumbnailHeight:384,
    thumbnailWidth:512,
    quality:1,
    chunkTimeSec:0.3,
  });
}
kartagis commented 7 years ago

I've implemented the code above, and now it adds only 56 photos.

viskin commented 7 years ago

Hmm. And how many times the callback function is called? It should be called multiple times. Anyway, let me try to reproduce this sometime today.

kartagis commented 7 years ago

As far as I can see, it's called multiple times (minus the add-to-DOM).

On 14 February 2017 at 12:12, viskin notifications@github.com wrote:

Hmm. And how many times the callback function is called? It should be called multiple times. Anyway, let me try to reproduce this sometime today.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/terikon/cordova-plugin-photo-library/issues/34#issuecomment-279649692, or mute the thread https://github.com/notifications/unsubscribe-auth/ABiHXjyIJVKcMBIanI6ATd-SSq1VrqVbks5rcW_lgaJpZM4Lo5JO .

--

viskin commented 7 years ago

And the total length of all library arrays returned is 56?

kartagis commented 7 years ago

alert(library.length) just after getLibrary gives me 4 100s and 1 31, which makes it 431; however I have over 1000 photos.

On 14 February 2017 at 12:47, viskin notifications@github.com wrote:

And the total length of all library arrays returned is 56?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/terikon/cordova-plugin-photo-library/issues/34#issuecomment-279658963, or mute the thread https://github.com/notifications/unsubscribe-auth/ABiHXr7O-5VexdHPOSKe89OhYqUZX5Bsks5rcXgngaJpZM4Lo5JO .

--

viskin commented 7 years ago

Ok, I will try this later. That's the reason why the library is still not published - fixing these issues. Thank you.

kartagis commented 7 years ago

Happy to help :)

On 14 February 2017 at 13:12, viskin notifications@github.com wrote:

Ok, I will try this later. That's the reason why the library is still not published - fixing these issues. Thank you.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/terikon/cordova-plugin-photo-library/issues/34#issuecomment-279664912, or mute the thread https://github.com/notifications/unsubscribe-auth/ABiHXmoF8tUzsEJe9t-FhCOABKocR5jjks5rcX4agaJpZM4Lo5JO .

--

viskin commented 7 years ago

Found the problem. In some point at time, chunks are being returned in wrong order. I'll fix it today I hope.

kartagis commented 7 years ago

I also noticed some odd behaviour on iOS. The chunks keep coming, but only added to DOM after all are finished. On Fri, 17 Feb 2017 at 19:38, viskin notifications@github.com wrote:

Found the problem. In some point at time, chunks are being returned in wrong order. I'll fix it today I hope.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/terikon/cordova-plugin-photo-library/issues/34#issuecomment-280700111, or mute the thread https://github.com/notifications/unsubscribe-auth/ABiHXtvU-gC53GjPN2Iz0dxWzWyalJ7kks5rdcz1gaJpZM4Lo5JO .

--

viskin commented 7 years ago

Hope it was fixed. At least, all the tests pass. Please try latest version (from github).

kartagis commented 7 years ago

I will try when I get home. Thank you :) On Fri, 17 Feb 2017 at 21:20, viskin notifications@github.com wrote:

Hope it was fixed. At least, all tests are pass. Please try latest version (from github).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/terikon/cordova-plugin-photo-library/issues/34#issuecomment-280726837, or mute the thread https://github.com/notifications/unsubscribe-auth/ABiHXl7CcTxzgLXggULYQWyaSzuTtZ8kks5rdeT5gaJpZM4Lo5JO .

--

arberkryeziu commented 7 years ago

i did give it a try in my iPhone 5s (iOS 10.2) . ... I have 3924 photos on my library. it took around 16sec till the results begin to come in, and it did return results for only 3802 photos ....

viskin commented 7 years ago

@arberK Thanks. Just a small check, what version of the plugin do you use? The one from npm or the one from github? I reduced everything to just a loop over photos. Bellow is the code that runs in case of default parameters. I don't see anything that can be optimized further. fetchResult.enumerateObjects is just slow. The way to handle the slowness is to use chunks, so the first result will appear practically immediately. It's good for scenarios where the library is presented to user.

fetchResult.enumerateObjects({ (asset: PHAsset, index, stop) in

    let libraryItem = NSMutableDictionary()

    libraryItem["id"] = asset.localIdentifier
    libraryItem["fileName"] = asset.fileName
    libraryItem["width"] = asset.pixelWidth
    libraryItem["height"] = asset.pixelHeight
    libraryItem["creationDate"] = self.dateFormatter.string(from: asset.creationDate!)
    if let location = asset.location {
        libraryItem["latitude"] = location.coordinate.latitude
        libraryItem["longitude"] = location.coordinate.longitude
    }

    chunk.append(libraryItem)

    if index == fetchResult.count - 1 {
        completion(chunk, true)
    } 

})