1046167 - [Gallery][Buri] crashes when a not-too-big file is on the sdcard

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

Attachment: 31120037-5mb.jpg

[Blocking Requested - why for this release]:

When the attached file is in the sdcard, the gallery is crashing on Buri.

I tried with slightly larger files and they are correctly ignored. But this file is maybe just below the limit.

This is on a v2.0 build, but it is some days old, so it might be a good idea to test on a more recent build and on master.

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: config.js file on the device

Comment 2

[David Flanagan [:djf]]

When I click on the attached image, I see it displayed in two passes, so I suspect that means it is a progressive jpeg file of some sort. I wonder if #-moz-samplesize does not work correctly with files of this sort.  (I remember some previous bug involving a pjpeg file and it seemed like it worked okay, but maybe I didn't test whether #-moz-samplesize was actually working.)

So it will be interesting to see if I can save this file as a regular jpeg and see if the bug still reproduces. If not, then I'll need a way to distinguish pjpeg from regular jpegs so I can set a smaller image size limit for those.

But if the bug does reproduce for a jpeg with the same file size and same # of pixels, then I'll want to reconsider my rough heuristic for limiting the file size in mb to 2x the maximum image size in mp.  That was just a guess, and Julien's test case may be telling us that the guess was bad.

Also, the config.js file Julien has attached is showing a 5mp max image decode size, which would mean that we would decode this 15+mp image at half width and half height, resulting in an ~4mp image  So that is 16mb of memory for the decoded image plus 5mb of memory for the image file.

On the other hand, we've landed a patch (that should be in 2.0) to check the amount of memory, and on a 256mb buri, the image size should be limited to 3mp. If that code is running correctly in Julien's build, then the image should have been decoded at 3/8ths width and height for a total image size of 2.5mp.

If Julien's build was a little old and didn't have this runtime check for memory then maybe that is the cause of this bug.

Comment 3

[Hema Koka [:hema]]

adding qawanted to confirm the bug in latest 2.0 with Buri (256MB)

Comment 4

[Hema Koka [:hema]]

NI Julien also to check if he used the latest 2.0 build for reproducing this issue

Comment 5

[Julien Wajsberg [:julienw]]

Just flashed latest 2.0 build, and still crashing.

Comment 6

[Jason Smith [:jsmith]]

(In reply to Hema Koka [:hema] from comment #3)
> adding qawanted to confirm the bug in latest 2.0 with Buri (256MB)

Also - include info for what happens on Flame 319 MB.

Comment 7

[Pi Wei Cheng [:piwei] (inactive)]

Crash occurs on Buri 2.0, Buri 2.1, and Buri 1.4.

Device: Buri
Build ID: 20140730080807
Gaia: e8425788e0372fbbcc40387b799f0636c041fc14
Gecko: 20c472fc0ee3
Version: 32.0 (2.0)
Firmware Version: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Device: Buri
Build ID: 20140730141509
Gaia: b67ddd7d40b52e65199478b8d6631c2c28fdf41d
Gecko: 005424a764da
Version: 34.0a1 (Master)
Firmware Version: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Device: Buri 1.4
Build ID: 20140731064109
Gaia: 3feb37ee2ed2319c9e556728723a5517dc1663ea
Gecko: 77119c4a721f
Version: 30.0 (1.4)
Firmware Version: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0

-----------------

On Flame 2.0 319MB mem it does not crash, but Gallery app runs very slow when attempting to display the thumbnail of the said image. When attempting to view the image by tapping on its thumbnail, Gallery is unable to display the picture and shows a gray screen; Also it breaks all UI buttons' functionality (tapping on them does nothing), but Home button still works so user can get out of this by tapping on Home.

Device: Flame (319MB)
Build ID: 20140730063205
Gaia: 3f9323d3b3ae8541fd84125a3b3a136bd018dedc
Gecko: 3fe40fbaee6d
Version: 32.0 (2.0)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 8

[Preeti Raghunath(:Preeti)]

Wayne,

Will this be an issue for Madai devices, given it uses a larger camera? Please check and comment.

Comment 9

[Hema Koka [:hema]]

Crash is happening with a particular file on Buri per comment 7 and unpleasant experience on Flame

David, can you check what the issue is and if this issue may lead to a larger user impact?

Thanks
Hema

Comment 10

[Wayne Chang [:wchang]]

(In reply to Preeti Raghunath(:Preeti) from comment #8)
> Wayne,
> 
> Will this be an issue for Madai devices, given it uses a larger camera?
> Please check and comment.

We still don't have devices yet but given the comments I believe this isn't specifically camera related but images in a particular filesize/dimension range. My vote is for 2.0+ too here as we have devices that will expect better image capabilities.

Comment 11

[Andrew Overholt [:overholt]]

(In reply to David Flanagan [:djf] from comment #2)
> So it will be interesting to see if I can save this file as a regular jpeg
> and see if the bug still reproduces.

Jason, can this be tested by QA?

Comment 12

[Jason Smith [:jsmith]]

Sure.

Comment 13

[Julien Wajsberg [:julienw]]

(In reply to Andrew Overholt [:overholt] from comment #11)
> (In reply to David Flanagan [:djf] from comment #2)
> > So it will be interesting to see if I can save this file as a regular jpeg
> > and see if the bug still reproduces.
> 
> Jason, can this be tested by QA?

Note that we'd need a file that has the same size (both data size (around) and pixel size) than this jpeg, not necessary the same one.

Comment 14

[Chris Kreinbring [:CKreinbring]]

The bug does not repro on Buri 2.1, Buri 2.0 and Buri 1.4
Actual result: Using the attached jpeg image, the gallery loads all videos and pictures with no errors.

Buri 2.1
BuildID: 20140806090025
Gaia: 5e6ef81cb9e917657ce050f598229dfc83c58b8f
Gecko: f41a267983c1
Platform Version: 34.0a1
Firmware Version: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

Buri 2.0
BuildID: 20140806084615
Gaia: 47fa0ba8197e71cc7034943ff037642e7f35cdfe
Gecko: 4feed2803746
Platform Version: 32.0
Firmware Version: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Buri 1.4
BuildID: 20140806081546
Gaia: e9dce1f60f729e228810f751417681b5ff937b6b
Gecko: d281a3bdfea6
Platform Version: 30.0
Firmware Version: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0

Comment 15

[Chris Kreinbring [:CKreinbring]]

Attachment: Jpeg picture used for second check

Comment 16

[Dietrich Ayala (:dietrich)]

This does not appear to be a regression, and there's no overwhelming use-case for why progressive jpg for this size is something that should hold the release back. Triage group recommends to not block.

Please explain if these conclusions are incorrect, and renominate.

Comment 17

[David Flanagan [:djf]]

In order to know what is really going on here we should know about gecko's support for progressive jpegs including whether #-moz-samplesize works for them.

Jeff: do you know whether libjpeg (and therefore #-moz-samplesize) supports downsampling while decoding of progressive jpegs?

(And if not, do you happen to know anything about the pjpeg file format that I could use to distinguish them from regular jpegs?)

Comment 18

[Jeff Muizelaar [:jrmuizel]]

(In reply to David Flanagan [:djf] from comment #17)
> In order to know what is really going on here we should know about gecko's
> support for progressive jpegs including whether #-moz-samplesize works for
> them.
> 
> Jeff: do you know whether libjpeg (and therefore #-moz-samplesize) supports
> downsampling while decoding of progressive jpegs?

My intuition is no. But I'll check.

> (And if not, do you happen to know anything about the pjpeg file format that
> I could use to distinguish them from regular jpegs?)

Take a look here:
https://github.com/notmasteryet/jpgjs/blob/master/jpg.js#L682

Comment 19

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #18)
> (In reply to David Flanagan [:djf] from comment #17)
> > In order to know what is really going on here we should know about gecko's
> > support for progressive jpegs including whether #-moz-samplesize works for
> > them.
> > 
> > Jeff: do you know whether libjpeg (and therefore #-moz-samplesize) supports
> > downsampling while decoding of progressive jpegs?
> 
> My intuition is no. But I'll check.

Actually, I was thinking of something else. My intuition is that libjpeg should support downsampling while decoding progressive jpegs.

Comment 20

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #19)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #18)
> > (In reply to David Flanagan [:djf] from comment #17)
> > > In order to know what is really going on here we should know about gecko's
> > > support for progressive jpegs including whether #-moz-samplesize works for
> > > them.
> > > 
> > > Jeff: do you know whether libjpeg (and therefore #-moz-samplesize) supports
> > > downsampling while decoding of progressive jpegs?
> > 
> > My intuition is no. But I'll check.
> 
> Actually, I was thinking of something else. My intuition is that libjpeg
> should support downsampling while decoding progressive jpegs.

djpeg seems to support downscaling progressive so I'd expect it should work with moz-samplesize

Comment 21

[David Flanagan [:djf]]

I've experimented with the attached pjpeg image.  On a 512mb Flame, I don't get a crash, but I do see that it takes about 8 seconds to display the image, and using b2g-ps I see massive (> 100mb) memory spikes while decoding the image and when zooming in to it.  

Thinking about this, I realize that a progressive jpeg really can't be downsampled while being decoded (at least not in a streaming fashion) because adjacent scanlines will be spread far apart in the file. In order to downsample the image, we first have to decode just about all of it.

It does appear that #-moz-samplesize is producing a downsampled image. But it isn't doing so with any efficiency.

To fix the memory crash in Gaia, I'll have to modify my jpeg metadata parser to distinguish pjpegs from regular jpegs and treat pjpegs that are too large like pngs: files that are subject to a maximum resolution limit and that do not use #-moz-samplesize

Comment 22

[David Flanagan [:djf]]

I found an informative comment on this blog post: http://blog.patrickmeenan.com/2013/06/progressive-jpegs-ftw.html

Quote:

Frédéric KayserJune 28, 2013 at 10:59 AM

I would also use large progressive JPEGs with caution on memory limited devices (entry level smartphones).
To reconstruct a progressive JPEG a browser has to either:
- keep huge matrices in memory (6 bytes per pixel in case of a color JPEG, since each "data units" holds 14-bits values per cell).
- keep all the scans compressed in memory and perform decompression in parallel on those (you loose the benefits of progressive display in this case).

Sequential JPEGs based on interleaved MCU holding the 3 components (YCbCr) do not face this memory hog problem, since the memory used by an MCU (or a lign of MCUs) could be released right after it has been uncompressed back to RGB pixels.

Comment 23

[David Flanagan [:djf]]

Work in progress here: https://github.com/davidflanagan/gaia/compare/bug1046167

Comment 24

[David Flanagan [:djf]]

I had hoped to land a fix for this before the 2.1 FL date, but other blockers have taken my time up and I have not been able to get back to my WIP patch. I will be OOO until September 2nd and will be able to resume work after that. Since this is a bug fix and not a feature, perhaps we can land it in 2.1 after the deadline.

Comment 25

[David Flanagan [:djf]]

Attachment: link to patch on github

On second thought, my work in progress was nearly done, so I went ahead and created a pull request for it.

Punam, would you review and land this please? (Or if you're too busy with gallery edit mode changes, maybe ask Russ to review and land.)

With this patch, you should see that the first sample image attached to this bug is rejected just like a really large PNG image would be.

Comment 26

[Punam Dahiya [:pdahiya]]

Comment on attachment 8477599 [details] [review]
link to patch on github

I am neck deep into edit mode updates, passing it to Russ to help out with the review and land. Thanks

Comment 27

[Russ Nicoletti [:russn]]

Comment on attachment 8477599 [details] [review]
link to patch on github

Looks good.

Comment 28

[Punam Dahiya [:pdahiya]]

Thanks Russ for review. Patch landed on master

https://github.com/mozilla-b2g/gaia/commit/0952f21286deca9901748ce708d632dbcebc1a19