Environment: -------------------------------------------------- Gaia bbba09c732c35d9434ecb04d5e2e41d05511f4d9 Gecko https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/65b4c58a7da9 BuildID 20140410164004 Version 28.1 ro.build.version.incremental=eng.cltbld.20140410.210129 ro.build.date=Thu Apr 10 21:01:38 EDT 2014 Steps to reproduce: --------------------------------------------------- 1. Copy specific pictures to sd card 2. Launch Gallery 3. Wait for gallery to load those pictures Actual result: --------------------------------------------------- Fail to load all the pictures. After a while, gallery exits.

Download the pictures here: https://www.dropbox.com/s/e4djgh3z5r1mbbd/298517-pic_for_testing.zip Unzip the file and copy all the pictures to sd card.

Marking as OOM, as there is a low memory error message; having said that there's also some invalid metadata errors in the log. Need to see if this happens in 1.3 using the images given.

Hi Blake, I find out that there is a specific picture to make Gallery exit. Could you please help to investigate this media parser issue? Thank you. file: "gif_8M.gif" size: 8.9MB

Attach the specific picture for debug.

The issue is easy to be reproduced with the specific picture. Also attach console log and dmesg for debugging. ====== Build version ====== Gaia 44ff6248c28ff83b9ad1161847a176399f93d3bb Gecko https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/72055108f470 BuildID 20140414164002 Version 28.1 ro.build.version.incremental=eng.dannyliang.20140412.101028 ro.build.date=六 4月 12 10:16:11 CST 2014

So it's another OOM issue but I think we could do something from Gallery app to prevent the crash from happening. Currently Gallery app would not load any image larger than 5 megapixels. In this case, however, the attached GIF file contains much less than that. Therefore the image was loaded and OOM occurred. I would suggest that maybe we could handle GIF separately, do something like pixel + file size check to ensure that we won't even try to load an oversized image and cause OOM. Just my 2 cents.

triage: 1.3T+ per discussion with Product to add some limitation to file loading. when loading file, will check resolution AND file size

This patch add two new config: CONFIG_MAX_IMAGE_FILE_SIZE and CONFIG_MAX_UNKNOWN_IMAGE_FILE_SIZE. The CONFIG_MAX_IMAGE_FILE_SIZE is for known file format and CONFIG_MAX_UNKNOWN_IMAGE_FILE_SIZE is for unknown file format. The default value of CONFIG_MAX_IMAGE_FILE_SIZE case: I had tested with the 8M gif case in master code with Inari device. It works smoothly and plays the animation correctly. So, I use 10MB as its default value. For 1.3t case, I had done few cases: 1. 3MP image with 1.9MB file size whose jpeg compression ratio is low => failed 2. 2MP image with 450KB file size which is captured by tarako => success So, I use 500KB as its default value. The default value of CONFIG_MAX_UNKNOWN_IMAGE_FILE_SIZE: I just use the original default value: 500KB for it.

(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #12) > Created attachment 8407452 [details] > patch for master > > This patch add two new config: CONFIG_MAX_IMAGE_FILE_SIZE and > CONFIG_MAX_UNKNOWN_IMAGE_FILE_SIZE. The CONFIG_MAX_IMAGE_FILE_SIZE is for > known file format and CONFIG_MAX_UNKNOWN_IMAGE_FILE_SIZE is for unknown file > format. > > The default value of CONFIG_MAX_IMAGE_FILE_SIZE case: > I had tested with the 8M gif case in master code with Inari device. It works > smoothly and plays the animation correctly. So, I use 10MB as its default > value. > > For 1.3t case, I had done few cases: > 1. 3MP image with 1.9MB file size whose jpeg compression ratio is low => > failed > 2. 2MP image with 450KB file size which is captured by tarako => success > > So, I use 500KB as its default value. > > The default value of CONFIG_MAX_UNKNOWN_IMAGE_FILE_SIZE: > I just use the original default value: 500KB for it. I think file size 500KB is not enough. I can use tarako to take a picture larger than 800KB. So if we set this value to 500KB, the pictures user took might not be able to be loaded in Gallery. This is not acceptable. To take a large pic, you just need to use sth with rich color.

What I complain in comment 13 is setting CONFIG_MAX_IMAGE_FILE_SIZE to 500KB.

(In reply to pcheng from comment #14) > What I complain in comment 13 is setting CONFIG_MAX_IMAGE_FILE_SIZE to 500KB. Thanks for that. I had tried multiple cases of it. I found the value is wrong. Currently, peipei can take a picture about 800KB. But the largest picture I can take is 787KB. So, I will try to find a 2MP image with largest file size. It is welcome if you guys can provide some.

I found the worst case of JPEG compression is fractal noise from google. The largest image I can found is this one: 2MP with 1.6MB file size. And this image can be used at tarako correctly.

Comment on attachment 8407452 [details] patch for master I had done more tests about the image size: JPG: all cases are passed with 2MP. The largest one is 1.6 MB GIF: (resolution is near 2MP 1600x1200) 8MB => thumbnail cannot be generated 4MB => thumbnail can be generated, unable to zoom, unable to crop and pick 3MB => thumbnail can be generated, unable to zoom, unable to crop and pick 2MB => thumbnail can be generated, unable to zoom, can be cropped and picked. 1.2MB => thumbnail can be generated, can zoom (but unstable), can be cropped and picked. 1MB => thumbnail can be generated, can zoom, can be cropped and picked. 900K => thumbnail can be generated, can zoom, can be cropped and picked. I wound like to ask others opinion to 'only' block the size of GIF. I will continue to test PNG cases. clear the review flag to wait for the final result.

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #3) > Marking as OOM, as there is a low memory error message; having said that > there's also some invalid metadata errors in the log. Need to see if this > happens in 1.3 using the images given. The issue of not all the images loading *does* reproduce on the 04/17/14 1.3 build on a Buri. 16 out of the 35 images loaded, one of them being an animated GIF that did animate. The Gallery did not crash with OOM though. Device: Buri v1.3 MOZ BuildID: 20140417004003 Gaia: 11d027ec28d8e8f09c76b35661222499e124abc8 Gecko: 69851ef3849c Version: 28.0 Firmware Version: v1.2-device.cfg

(In reply to Matthew Vaughan from comment #18) > (In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from > comment #3) > > Marking as OOM, as there is a low memory error message; having said that > > there's also some invalid metadata errors in the log. Need to see if this > > happens in 1.3 using the images given. > > The issue of not all the images loading *does* reproduce on the 04/17/14 1.3 > build on a Buri. 16 out of the 35 images loaded, one of them being an > animated GIF that did animate. The Gallery did not crash with OOM though. > Yes. In my experience, the issue here is unable to reproduce on v1.3/Unagi and master/Inari in my impressions.

Comment on attachment 8407437 [details] patch for 1.3t Per comment 19, I don't think we need to limit GIF image size in master. So, I will obsolete the master patch. I had tested lots gif files in Inari from 20MB to 1MB. And it works fine in all of those cases. According to the finding at comment 17, I limit the GIF image size to 1MB.

John will submit a patch to set the 1MB limit on v1.3T branch directly.

Comment on attachment 8407437 [details] patch for 1.3t The patch is to add a configuration to limit the gif file size and put 1MB as its default. This patch will only land to v1.3T.

Can we land this patch before 4.15 -- spreadtrum PTR2 test cycle.

If we limit the image size, can we prompt end user?

Comment on attachment 8407437 [details] patch for 1.3t This looks okay to me. I'm not convinced that creating a new gallery.json file makes sense. I think we should either split camera.json and gallery.json completely, or keep a single file for both. For bug 989290, I'm likely to add build time configuration options that are specific to camera, and those variables will end up in the gallery config file Also, for patches that are going directly to the device-specfic 1.3 branch, I wonder if we should just create gaia/distribution/camera.json and edit it directly with our patches to set default values for Tarako.

(In reply to David Flanagan [:djf] from comment #25) > This looks okay to me. Thanks for the reviewing. > > I'm not convinced that creating a new gallery.json file makes sense. I think > we should either split camera.json and gallery.json completely, or keep a > single file for both. > > For bug 989290, I'm likely to add build time configuration options that are > specific to camera, and those variables will end up in the gallery config > file OK, got it. I can put it back to camera.json. I only create one config variable. BTW, we already have two different customization files for each one[1] in master branch. [1] https://github.com/mozilla-b2g/gaia/blob/f0463704888881b8ed1619e8d4b0d851b0e0311b/apps/gallery/build/build.js#L45

merged to v1.3t: https://github.com/mozilla-b2g/gaia/commit/e87f8a69a2bad4a1361529b180a4e015fc78fbf1

Fixed on todays 1.3T build. 1.3T Environmental Variables: Device: Tarako 1.3T MOZ BuildID: 20140428014001 Gaia: 8895b180ed636069473703d0e7b73086989601ce Gecko: 7caf4b5abfce Version: 28.1 Firmware Version: sp6821 Gif of this size no longer show as available files in the gallery.

John, will you be getting this change into master?

Russ, On master, we probably want it to be build-time configurable so that only low-memory devices have this restriction. We could add a new build time config variable, or maybe just check for non-zero CONFIG_MAX_PICK_PIXEL_SIZE.

Russ, I had put a PR for master at comment 17. But according comment 19, it is not reproduced at master. So, I obsoleted it. If you want to add a new build time config, you may check that PR.

uplift to v1.4 https://github.com/mozilla-b2g/gaia/pull/22356 wait for tbpl passing.

(In reply to GaryChen [:GaryChen][:PYChen][:陳柏宇] from comment #32) > uplift to v1.4 > https://github.com/mozilla-b2g/gaia/pull/22356 > > wait for tbpl passing. hi gchen: we have made a limitation for the file size before the patch is added.the code is : in MetadataParser.js: >if (metadata.width * metadata.height > imagesizelimit || > file.size > filesizelimit) { > metadataError('Ignoring high-resolution image ' + file.name); > return; > > } but I see another specify limitation for the file size in your patch: >if (file.size > CONFIG_MAX_IMAGE_FILE_SIZE) { > metadataError('Ignoring acceptable resolution but large file ' + > file.name); > return; > } Could you help to explain why should we limit the file size at two different places? Thank you very much!

(In reply to jingmei.zhang from comment #33) > (In reply to GaryChen [:GaryChen][:PYChen][:陳柏宇] from comment #32) > > uplift to v1.4 > > https://github.com/mozilla-b2g/gaia/pull/22356 > > > > wait for tbpl passing. > > hi gchen: > > we have made a limitation for the file size before the patch is added.the > code is : > > in MetadataParser.js: > > >if (metadata.width * metadata.height > imagesizelimit || > > file.size > filesizelimit) { > > metadataError('Ignoring high-resolution image ' + file.name); > > return; > > > > } > > but I see another specify limitation for the file size in your patch: > |filesizelimit| comes from |CONFIG_MAX_IMAGE_PIXEL_SIZE| [1], it is a different from |CONFIG_MAX_IMAGE_FILE_SIZE|. Please refer to bug 997051 to find out the main reason why we use |CONFIG_MAX_IMAGE_FILE_SIZE|. [1] https://github.com/mozilla-b2g/gaia/blob/v1.4/apps/gallery/js/MetadataParser.js#L234 Thanks. > >if (file.size > CONFIG_MAX_IMAGE_FILE_SIZE) { > > metadataError('Ignoring acceptable resolution but large file ' + > > file.name); > > return; > > } > > Could you help to explain why should we limit the file size at two > different places? > > Thank you very much!