809782 - [Gallery] gallery crashes if too many images are available on sdcard

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Gallery, defect, P1)

Description

[FxOS Deutsche Telekom]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:16.0) Gecko/20100101 Firefox/16.0
Build ID: 20121024073032

Steps to reproduce:

1. putting 450+ images on the sdcard, putting it into the device an starting it
2. opening gallery from homescreen
3. wait until all images have been shown in preview --> here the error occurs


Actual results:

When opening the gallery and wait for a while, the gallery closes with the message "Gallery had a problem and closed" - happens also if trying to change the background of the home screen with a picture from the gallery.



Expected results:

The gallery should load without any error and if there is a problem with loading 450+ images, it should be communicated to the user with the gallery further working as expected.

Comment 1

[David Flanagan [:djf]]

Can you tell me more about the 450 images you tested with?  What was their resolution?  If they were taken with a higher-resolution digital camera, we may be running out of memory just trying to process them and produce thumbnails.

My point is that I don't think I could reproduce this just with a large number of photos, I'd guess that the images have to be big ones.  Can you confirm.

I suspect this is related to https://bugzilla.mozilla.org/show_bug.cgi?id=805114
I'm planning to try addressing it with some of the steps described in https://bugzilla.mozilla.org/show_bug.cgi?id=805114#c45

Comment 2

[FxOS Deutsche Telekom]

Most of these images were taken with a DSLR, but downsized to 1630x1080. Don't know what the experience is with regards to memory size.

Comment 3

[David Flanagan [:djf]]

There are a three major things I want to do to decrease the memory usage and improve the performance of the Gallery app, and I'm going to use this bug as the vehicle to do them:

1) Modify the JPEG metadata parser so that if the image includes an embedded preview, it uses that for generating the thumbnail instead of generating it from the full-size image. That should make scanning for new files faster and much more memory-efficient.

2) When the embedded preview is at least as big as the screen size (as it is in the images produced by the camera app), I'm going to use that preview instead of the full size image.  If the user zooms in, I'll switch to the full size image, but for quickly flicking through the images, I'll stick with the preview.  This should substantially reduce the memory used while browsing for images.

3) The gallery currently creates DOM elements to display thumbnails for all photos it knows about. Gecko apparently decodes all the thumbnails, including offscreen ones, so I need to convert the thumbnail list to something that adds and removes thumbnails dynamically as the user scrolls. This should improve the memory performance of very long lists of thumbnails.

The crash described in this bug should be fixed by fixing #1 and/or #3 above. #2 is more directly related to https://bugzilla.mozilla.org/show_bug.cgi?id=810105, but that bug has been taken over by gecko hackers.

Here are some related bugs: 

  https://bugzilla.mozilla.org/show_bug.cgi?id=805114
  https://bugzilla.mozilla.org/show_bug.cgi?id=810105
  https://bugzilla.mozilla.org/show_bug.cgi?id=796611
  https://bugzilla.mozilla.org/show_bug.cgi?id=796473

Comment 4

[David Flanagan [:djf]]

Attachment: part 1: fix metadata parsing

Comment 5

[Alex Keybl [:akeybl]]

Marking for C2, given this meets the criteria of known P1/P2 blocking-basecamp+ bugs at the end of C1.

Comment 6

[Dominic Kuo [:dkuo]]

Comment on attachment 683906 [details]
part 1: fix metadata parsing

r+, and please also see the github comments.

I have tested with 900 DSLR pictures (3776 * 2520, total 3GB), gallery does not crash again and finish loading all my pictures. Also gallery shows only the pictures that are smaller then the target size. So this PR should solve the OOM problem in first step.

Comment 7

[David Flanagan [:djf]]

Attachment: part 2: when possible use preview image instead of full-size image

Dominic,

This pull request implements the second part of my memory use reduction strategy.  Whenever possible, it makes the Gallery app use the embedded preview image from the JPEG file rather than displaying the full size image.

Comment 8

[Dominic Kuo [:dkuo]]

Comment on attachment 684225 [details]
part 2: when possible use preview image instead of full-size image

r+, looks good to me.

There are two thing that I would like to mention:

1) I have tested the zooming and found out that sometimes the screen still flash, but not sure how to reproduce it. I am sure the MozAfterPaint event does fire every time when the new image is painted.

2) When we 1. thumbnailList > 2. fullscreenView > 3. thumbnailList > 4. fullscreenView, at initial of step 4, we will see the previous image in step 2, maybe we can reset it before leaving the fullscreenView?

Comment 9

[David Flanagan [:djf]]

For part 3, see also the discussion here: https://bugzilla.mozilla.org/show_bug.cgi?id=805114

Comment 10

[David Flanagan [:djf]]

I haven't forgotten about this bug. I got the most important two parts done, and then have been working on the more critical video rotation bug.

My plan is to come back to this one and work on part three, which will be to modify the list of thumbnails so that it never displays all the thumbnails at once. I'm not sure how long this will take but I'll guess 2 days, and I think I'll have this done by the C2 deadline.

I'm flagging this qawanted because there is no qa contact assigned and I wanted to raise these QA issues:

1) this isn't still a smoketest blocker, is it?

2) if anyone in qa has time, I'd be very interested to know how many thumbnails the gallery can display without running out of memory.  I think it is quite efficient now about scanning and previewing the images, so it shouldn't OOM during those steps, but it might still OOM when the raw number of photos gets too high.  That is the thing I want to prevent with the "part 3" work that still needs to be done.  So if someone could test now, that would tell us how urgent part 3 is, and would be a test we can use to verify that part 3 actually works, when I'm done with it.

I'd guess that gallery can handle 100 photos, but can it handle 500?  Note that you don't have to take 500 pictures to determine this. You can use a single photo (from the Gaia camera) and just copy it to the sd card with 500 different names (or in 500 different directories).

(I haven't fixed the "scanning make the thumbnails jumpy" bug yet, so starting up gallery after putting 500 new photos on the card will be ugly, but that's easy to solve.

Comment 11

[Dylan Oliver [:doliver]]

I added 600 images ranging in size from 750k to 2.5mb. It took a while to create the thumbnails of them all (3+ minutes) but had no trouble generating all of them. Scroll speed through the thumbnails was good, and flicking through previews was also quick & responsive. 

It was not as happy when I deleted those images out from underneath it (via USB Storage) and restarted. The thumbnails were still there but clicking on them had unpredictable results -- flashing image and/or app crash.

Comment 12

[David Flanagan [:djf]]

Dylan,

Thanks for testing! That's a better result than I expected.  Given what you've found, maybe this doesn't need to be blocking. Though I suppose I should actually do some memory measurements and find out how much memory each thumbnail is costing us...

The scanning process does or should (eventually) discover that deleted files have been deleted and removes the thumbnails. But there isn't any code in the gallery to gracefully handle the error that arises when the user clicks on a thumbnail whose underlying image has been deleted.... I should handle that.  (Maybe in a separate bug). I wonder if this will require a new user-visible string that will need l10n, or if I can just make the gallery do something non-broken without actually alerting the user.

Comment 13

[David Flanagan [:djf]]

I've done some followup on Dylan's testing. Dylan can run with 1000 images. I can run with 800. But I can't run with 1600.  Every 100 thumbnails takes up something like 6mb of memory. So a user with 1000 photos in their gallery is using something like 50mb more than they need to, and launching gallery with that many images is likely to force other apps to exit.

So I really do need to do part 3, and this part should directly address the problem listed in the description of this bug.

My original thought was that I needed an LazyList abstraction that would maintain an array of all my thumbnails and insert them and remove them from an underlying HTML list element as the user scrolls.  That would be great to have for other apps, too.

But, because of time constraints, I'm going to settle for something simpler.  My <ul> element will still contain one <li> for every thumbnail. But I'll only set the background image of the li for those thumbnails that are visible or are near to being visible, and I'll set and unset background images as the user scrolls. (If the user scrolls really fast, they may see blank squares, but the thumbnails should pop into place quickly when they stop scrolling.)  The size of the decoded images is much much bigger than the overhead of all the HTML elements, so this should be almost as good as a full-blown infinite scroll type solution.

Comment 14

[Dylan Oliver [:doliver]]

At this point, I think we can take this one off the smoketest list. Supporting at a minimum several hundred 5-megapixel images should clear the bar.

Also removing qawanted -- looks like David has what he needs for this one.

Comment 15

[David Flanagan [:djf]]

I've got a WIP that solves the thumbnail issue. The app can display 1600 thumbnails at a time, without using a ton of image memory.

Still a few kinks to work out, but I should have something soon.

Comment 16

[David Flanagan [:djf]]

Attachment: part 3: don't set background-image for offscreen thumbnails

This is my final fix for gallery memory issues.  I'm not sure who should review it.

Comment 17

[Dale Harvey (:daleharvey)]

Comment on attachment 689876 [details]
part 3: don't set background-image for offscreen thumbnails

Merged in: https://github.com/mozilla-b2g/gaia/commit/6cd734d45745695072f88ebcaf2f2a517b6ea962

Comment 18

[Dale Harvey (:daleharvey)]

This looks to be the last part of this issue so closing

There was one problem I found, full screen images flash while loading, but that issue existed without this patch so it wasnt a regression, closing here and have filed a new bug on the flashing image

https://bugzilla.mozilla.org/show_bug.cgi?id=819815

Comment 19

[David Flanagan [:djf]]

Sadly, I'm reopening this. 

I've found some problems with that part 3 patch that need to be fixed:

1) If you scroll down and then go from the thumbnail list to select mode you get a bunch of gray squares (in part because the thumbnails scroll back to the top, which is unrelated but shouldn't happen)

2) In pick mode, if you scroll down, pick a photo and then go back to pick mode, there are gray squares.

I suspect that this is related to using display:none instead of visibilty:hidden, or it is related to the fact that I move the thumbnail list between containers when switching modes. Maybe visibility_monitor should use mutation events to monitor its own tree status and recompute the visible range when added to a new parent?

Comment 20

[David Flanagan [:djf]]

Attachment: followup fix for part 3

My 3rd patch for this bug had a couple of bugs in it that would sometimes cause thumbnails to not be displayed properly.  This is a followup to fix those bugs.

One bug was in visibility_monitor.js

The other bug has to do with how the gallery was managing its list of thumbnails when switching views. So this patch includes a minor refactoring of how the thumbnails list is managed. As part of that I had to chase down a problem with fullscreen mode, so it turned out to be a larger patch than I anticipated, but it gave me the opportunity to do a little CSS cleanup, as well.

Comment 21

[David Flanagan [:djf]]

Michael: I'm not authorized to view that one.

What is it you're seeing in that bug? All of the memory performance issues that I know about are now resolved. The only reason that this bug is still open (with a patch awaiting review) is that there are now some thumbnail rendring bugs: you sometimes see gray squares instead of thumbnails.

I don't plan to do any other work on this bug and will be closing it, so if you still have something blocking 802677, you'll have to give me access to that bug or tell me what symptom you're seeing so I can fix it.

Comment 22

[Michael Vines [:m1] [:evilmachines]]

opps, my bad.  sorry! :)

Comment 23

[Dominic Kuo [:dkuo]]

Comment on attachment 691064 [details]
followup fix for part 3

r+, looks good to me.

David, I have tested this patch and tried to reproduce the issues that you mentioned in the previous comments, and they should be all gone with this one.

Comment 24

[David Flanagan [:djf]]

Last bit just landed in github pr #6961. Closing.