Closed Bug 1024692 Opened 10 years ago Closed 10 years ago

camera app is killed during zoom in/zoom out (out of memory) on 256 MB target

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:1.4+, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

RESOLVED FIXED
2.0 S6 (18july)
blocking-b2g 1.4+
Tracking Status
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: tkundu, Assigned: dmarcos, NeedInfo)

References

()

Details

(Keywords: memory-footprint, perf, Whiteboard: [c=memory p= s= u=1.4] [caf priority: p1][CR 673864][MemShrink:P2])

Attachments

(5 files)

STR:

Go to Camera->take a photo-> tap on shortcut to open the photo
try Zoom out/Zoom in and observe
Expected result:
device should be Zoom in and Zoom out
Actual result:
camera app is killed

we are seeing following thing in dmesg:

<6>[  151.338733] send sigkill to 1847 ((Preallocated a), adj 667, size 1215
<6>[  151.636678] send sigkill to 1615 (Homescreen), adj 534, size 1090
<6>[  151.782002] send sigkill to 1697 (Camera), adj 134, size 1445
<4>[  151.782204] Camera used greatest stack depth: 4444 bytes left


This indicated that we are running out of memory and LMK is killing apps in proper order as expected .


Device used : MSM8610 256MB.
Issue is present in both FFOS v1.3 and FFOS v1.4 running on same 256MB target. If we increase memory to 512MB then issue does not reproduce for same build.

This is observed with fix from 
[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1002593#c46
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1014955#c39


Please let me know if you want to see DMD report or not.
blocking-b2g: --- → 1.4?
QA Wanted to get an about:memory report on 1.4.
Keywords: footprint, perf, qawanted
Whiteboard: [CR 673864] → [CR 673864][MemShrink]
Whiteboard: [CR 673864][MemShrink] → [caf priority: p1][CR 673864][MemShrink]
For reference,
As per comment: https://bugzilla.mozilla.org/show_bug.cgi?id=949748#c9, patches from bug 1002593 and bug 1014955 was supposed to solve this issue but it didn't.

https://bugzilla.mozilla.org/show_bug.cgi?id=990292 is probably related.
David, 

Can you please take a look at this? 

Thanks
Hema
Flags: needinfo?(dflanagan)
Tapas,

1) Did you apply the patches in bug 1014955 yourself, or just use the nightly build from today? Note that 1014955 landed yesterday and was then backed out before the nightly build was created.  It has now landed again and will hopefully stick.  So if you are not certain that bug 1014955 has been applied, you may want to check this again on Friday.

2) Do you know if this reproduces on the Flame or any non-QRD devices?

3) What is the screen size (in device pixels) of the device you are testing on?

4) Please attach a sample photo, so I can see what the photo resolution and the EXIF preview resolution are.
Flags: needinfo?(dflanagan) → needinfo?(tkundu)
(In reply to Inder from comment #2)
> For reference,
> As per comment: https://bugzilla.mozilla.org/show_bug.cgi?id=949748#c9,
> patches from bug 1002593 and bug 1014955 was supposed to solve this issue
> but it didn't.
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=990292 is probably related.

Inder: why do you say that this is related? Is this bug happening while the gallery app is scanning?  If so, that should be clearly stated in the STR because it makes a huge difference.  

Tapas: does this bug occur when the app is done scanning, or only while it is processing images?
Flags: needinfo?(ikumar)
Assignee: nobody → dflanagan
Nevermind. Clearing the needinfo request for Inder. This bug is about camera. It is completely unrelated to 990292 because that bug is very specific to Gallery.
Flags: needinfo?(ikumar)
Tapas, please provide info requested in comment 4 so we can proceed further on this bug
Attached image IMG_0002.jpg
(In reply to David Flanagan [:djf] from comment #4)
> Tapas,
> 
> 1) Did you apply the patches in bug 1014955 yourself, or just use the
> nightly build from today? Note that 1014955 landed yesterday and was then
> backed out before the nightly build was created.  It has now landed again
> and will hopefully stick.  So if you are not certain that bug 1014955 has
> been applied, you may want to check this again on Friday.
> 

I am quite sure that fix from 1014955 was present on that build which i tested. I double checked it on your request.

> 2) Do you know if this reproduces on the Flame or any non-QRD devices?
> 
We didn't try it with flame device But you can put flame device in 256MB configuration and try to reproduce this issue.

> 3) What is the screen size (in device pixels) of the device you are testing
> on?
> 
WVGA

> 4) Please attach a sample photo, so I can see what the photo resolution and
> the EXIF preview resolution are.

Image attached.
Flags: needinfo?(tkundu)
Issue is NOT reproducible using Buri on 1.4 and 1.3.
Preview of camera correctly reacts to zooming in and out. Repro rate: 0 out of 10 attempts on 1.4, and 0 out of 10 attempts on 1.3.

Note: We do not have a device that uses MSM8610. The closest one we have is Buri which uses MSM7227A & 256MB of RAM.

Tested on:
(no repro)
Device: Buri
Build ID: 20140613000202
Gaia: 1dae62556e642b0b2e08689e35e24e56daa8c79b
Gecko: 30224c7f5e58
Version: 30.0
Firmware Version: v1.2device.cfg

(no repro)
Device: Buri
Build ID: 20140613024000
Gaia: 8d6bd6c484557c5322bf14798a4273d2a8f4300f
Gecko: d0c6d2ebfe65
Version: 28.0
Firmware Version: v1.2device.cfg
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Keywords: qawanted
Tapas: Did this bug also occur before 1014955 landed? 

So the QRD camera is taking 5mp pictures with a tiny 320x240 EXIF preview. Since this is much too small to fill the screen, the camera app has to decode the full-size image in order to show the photo to the user.
Before that patch we decoded the photo at full size for every preview, so the pinch gesture to zoom in would not be allocating any more memory.

After 1014955, we first downsample while decoding the photo to produce a preview. Then on the pinch-to-zoom, we decode again at full size, and in this case, the pinch causes a memory allocation.

So if this OOM is occuring in the same way both before and after bug 1014955 then I'm pretty confused about it and suspect there is something else going on.

It is a large image with a small preview on a low memory device with a big screen, so that is just about the worst combination of factors.  I'll investigate to see if the small EXIF preview is causing an issue.

I can't reproduce this on Dolphin, which has a similar configuration, I think. But that is a different chipset.  Not able to test on my Hamachi because it has an old base image and everything crashes on it all the time.
Flags: needinfo?(tkundu)
Flags: needinfo?(jmitchell)
Tapas:

A couple more questions:

Does this OOM occur when you zoom in on the same photos in the Gallery app?  If so, then I'll investigate the image display and zooming code.  If not, then I'd want to investigate whether the camera is not being shut down enough when we switch to preview mode.  (Can you tell from the logcat output whether your camera driver is continuing to display a viewfinder preview stream while you're previewing the photo?)

Does this OOM occur for you on master as well as on 1.4?
Our Flame device does not work with 256mb (see bug 1008050). But I've found that if I just add 32mb more, and configure it at 288mb, I can get it to boot.

With 288mb of memory, I can crash the camera when zooming in on a photo and panning it around.  If I just pinch to zoom in, it didn't crash right away, but the image started flickering in and out indicating that gecko was releasing image memory, so obviously we were very low on memory.  Then when I tried to pan a bit in the zoomed-image, the Camera app did die with a OOM. (This was on master, not 1.4)
With 1.4 nightly on a Flame configured with 288mb of memory, I can reproduce the OOM easily when zooming in on an image in Gallery.  And can usually OOM in Camera if I zoom in and then pan around.

The images captured by the flame are 5mp and have 640x480 previews.

5mp images are big and I think 1.4 is the first release where we've tried to do them by default on a 256mb device. (My hamachi takes 3mp images).  A 5mp image requires 20mb of memory to decode, and perhaps that is just too big for these 256mb devices.

Perhaps we should reduce the size of the photos, or reduce the size that they are decoded at. Thanks to the 1.3T work, we have the ability to set config variables that would prevent us from decoding any image at larger than 3mb (for example).  So we could take 5mp images but never let the user see all the pixels: they could zoom in only part-way.

What I'm suggesting is that we may not have a bug here. It could just be that 5mp photos are just too big for 256mb devices.
Tapas,

Would you try applying this patch to your 1.4 branch and then rebuilding the Camera and Gallery apps like this:

  GAIA_MEMORY_PROFILE=256 APP=camera make install-gaia
  GAIA_MEMORY_PROFILE=256 APP=gallery make install-gaia

Doing that should limit both apps to downsample the 5mp images at a maximum size of 3mp and should prevent the OOM.

Please let me know if this works for you.

Alternatively if you're using a distribution directory for creating your builds, you can put camera.json and gallery.json files in that directory to and use them to customize the maxImagePixelSize variable.
Attachment #8440211 - Flags: feedback?(tkundu)
(In reply to David Flanagan [:djf] from comment #11)
> Tapas:
> 
> A couple more questions:
> 
> Does this OOM occur when you zoom in on the same photos in the Gallery app? 
> If so, then I'll investigate the image display and zooming code.  If not,
> then I'd want to investigate whether the camera is not being shut down
> enough when we switch to preview mode.


David, here are some data points:
Without downsample:
Camera : OOM right away with pinch and zoom
Gallery: Sometimes OOM with multiple tries

With downsample:
Camera : OOM right away with pinch and zoom
Gallery: Didn't OOM after multiple tries

> Can you tell from the logcat output whether your camera driver is continuing to display a viewfinder preview stream while you're previewing the photo?

Yes, I do see the preview stream in logcat while previewing the image 

> Does this OOM occur for you on master as well as on 1.4?

Yes, OOM occurs on master as well

------
I will try out your suggestion in comment 14 and update here..
Flags: needinfo?(tkundu) → needinfo?(ikumar)
(In reply to David Flanagan [:djf] from comment #14)
> Created attachment 8440211 [details] [review]
> trial patch for the 1.4 branch
> 
> Tapas,
> 
> Would you try applying this patch to your 1.4 branch and then rebuilding the
> Camera and Gallery apps like this:
> 
>   GAIA_MEMORY_PROFILE=256 APP=camera make install-gaia
>   GAIA_MEMORY_PROFILE=256 APP=gallery make install-gaia
> 
> Doing that should limit both apps to downsample the 5mp images at a maximum
> size of 3mp and should prevent the OOM.
> 
> Please let me know if this works for you.

David -- I don't see OOM in Gallery but I still see it in case of Camera. Earlier I could see the OOM right away in Camera after single pinch-zoom when previewing the image, now it takes couple of tries but it still happens.
maybe you want to look into why camera preview is not shutting down when previewing the image?
Flags: needinfo?(ikumar) → needinfo?(dflanagan)
Justin,

Now that we've uplifted the ability to downsample images in MediaFrame it looks like we may have to start using that ability for low-end 1.4 devices. Would you take a look at the camera changes in this patch and see if they look okay to you?  Also, I'd like to remove the limitMaxPreviewSize setting completely rather than defaulting it to true and just use CONFIG_MAX_IMAGE_PREVIEW_SIZE !== 0 as the condition. Is that okay with you?
Flags: needinfo?(dflanagan) → needinfo?(jdarcangelo)
Justin,

I just noticed that the patch above will reduce the photo size from 5mp to 3mp because config/settings.js incorrectly uses CONFIG_MAX_IMAGE_PIXEL_SIZE instead of CONFIG_MAX_SNAPSHOT_PIXEL_SIZE to set the photo resoluton.  So that is something else I'll want to change in the patch.
I've updated the patch to release the camera hardware while previewing images. This reduces memory usage a little bit and significantly improves performance. The downside is that it takes a little longer for the user to get back into camera mode and be able to take a picture. Using GAIA_MEMORY_PROFILE=256 to select 3mp previews of the 5mp images will still be needed to fix this though. Also, I discovered that the pinch-to-zoom gesture on the photo being previewed was also being treated as a zoom gesture on the viewfinder, so at the same time that we were allocating memory to zoom in on the photo we were also asking the camera driver to zoom in and I suspect that contributed to the OOM error.

Inder: please test this updated version.

Justin: Please take a look at this patch. I suspect that there is a better place to put the code that releases the camera hardware than where I've put it.  Your advice is welcome here. Or perhaps you could just take this bug and finish it up.
Flags: needinfo?(ikumar)
Comment on attachment 8440211 [details] [review]
trial patch for the 1.4 branch

David: This patch looks good, with minimal impact, for v1.4. I'm working on another bug now for handling multiple device "profiles" for configuration that will ultimately solve many of the same issues (hopefully). But, since that work will be fairly extensive, it wouldn't make sense to bring it into v1.4 at this point. The only other comment I have is that we should probably pull the start/stop viewfinder code into v2.0 and master. I haven't pulled your patch down to test it firsthand, but is there any noticeable delay now when switching between the preview gallery and the viewfinder? I would imagine that would bring a fair amount of memory savings and if there's not much latency when switching between preview and viewfinder then we should definitely patch v2.0 and master as well.
Flags: needinfo?(jdarcangelo)
Attachment #8440211 - Flags: feedback?(tkundu) → feedback+
Flags: needinfo?(ikumar)
David -- here are my findings:
I am not able to reproduce the issue after I tried your patch, which I noticed has following changes, along with the config changes to force 3mp in comment 14:
Applying: Bug 1024692: limit image decode size to 3mp on 256mb devices
Applying: pick picture size based on the correct config variable
Applying: release camera hardware when previewing photos. Also do not zoom camera when zooming previews.

And, to find where the issue is, I also tried your patch *without* the config to force 3mp and I am not able to reproduce the issue in this case too!
So, not sure if we need to force 3mp change.

However, I do see a noticeable delay in starting up the camera preview when returning to camera after reviewing the image but that's something we had suspected.
We may need to get feedback from performance and UX team here?
Flags: needinfo?(dflanagan)
blocking-b2g: 1.4? → 1.4+
Please make sure to verify this once it lands on 1.4 as we were unable to repro in MOzilla.
Requesting UX input here per comment 21
Flags: needinfo?(firefoxos-ux-bugzilla)
Please clarify what the UX feedback in comment #21 is needed on. Sounds like a performance issue but let us know.
Tif is the UX designer for camera, so let's ask her about this directly.

Tif: low-end 1.4 devices are having OOMs when the user previews an image and zooms in. One of the steps I had to take to fix it was to release the camera hardware when the user goes to preview a photo. This means that when they are done previewing the camera app has to request the hardware again. So it takes a little longer to go from the preview screen back to the viewfinder and take another picture. Are you okay with that?  (I don't know that you actually have any choice in the matter...)  Once I have a final patch I'll ask you to give it a UX review.
Flags: needinfo?(tshakespeare)
Flags: needinfo?(firefoxos-ux-bugzilla)
Flags: needinfo?(dflanagan)
I discussed this with Mike on IRC. Apparently before 1.4, it was possible to pause the viewfinder stream when the user was previewing photos. In 1.4 and later, the API has changed and that is not possible anymore. So the camera hardware remains fully active, using memory and battery while the user is viewing photos.

For 1.4 and 2.0 I think we need to release the hardware so we don't have that problem.  Per discussion with Tif, maybe we will want to display a spinner when re-initializing the camera.

Mike says he might be able to add pausePreview()/resumePreview() methods to the camera API to idle the camera without releasing it.  If we had those methods on master (or in 2.2) then we could get rid of the spinner.

(Needinfo Mike becasue I'm quoting him so much here.)
Flags: needinfo?(mhabicher)
vikram -- need your feedback too on the performance impact.
Flags: needinfo?(mvikram)
Discussed with djf over IRC - seems like it's very similar to another bug we had regarding startup. I would also use the spinner in this instance with the same number of ms before calling it.
https://bugzilla.mozilla.org/show_bug.cgi?id=999171

If I recall, justindarc mentioned the spinner has been abstracted since we're also using it for HDR bug. 
https://bugzilla.mozilla.org/show_bug.cgi?id=1016492
Flags: needinfo?(tshakespeare) → needinfo?(jdarcangelo)
I don't believe the reusable spinner with the variable delays is in v1.4 though.
Flags: needinfo?(jdarcangelo)
I left some comments in the PR about the code structure.

The patch fixes the issue but introduces a penalty when opening back camera from the preview/gallery. It stops and restarts the camera when opening and closing preview. Booting the camera hardware is expensive and it can take up to 2 seconds in the case of hamachi. Is there a way to work around this issue in 2.0+ without the having to release the camera?
(In reply to David Flanagan [:djf] from comment #26)
> 
> Mike says he might be able to add pausePreview()/resumePreview() methods to
> the camera API to idle the camera without releasing it.  If we had those
> methods on master (or in 2.2) then we could get rid of the spinner.

Pausing the viewfinder may save some CPU cycles and some gralloc memory, but I don't think it's likely to make much space for apps.
Flags: needinfo?(mhabicher)
I discovered a serious problem with my trial patch... When we return to the homescreen we close the preview gallery even when it is not open and this causes an even that restarts the camera hardware even though the camera is in the background!
David -- let me know if you want me to try out a new patch.
To summarize the discussion in this bug: the OOM issue here is primarily simply a matter that 5 megapixel images are too big to display full-size on a 256mb device. Secondarily, camera API changes in gekco in 1.4 mean that the camera is no longer shuttng down while we're previewing images and this seems to be making us more likely to OOM.

To fix this bug we need:

     - a change to the gallery build script to limit image previews to 3mp on 256mb devices

     - a similar change to the camera build script

     - a change to the camera so that it uses CONFIG_MAX_SNAPSHOT_PIXEL_SIZE to compute picture size rather than CONFIG_MAX_IMAGE_PIXEL_SIZE

     - a change to camera so that it releases the camera hardware when previewing photos. (Even when it is not a low-memory device this may make a notice able difference in performance to not have the camera running while the user is zooming and panning a photo.)

     - a change to display a spinner when it reloads the camera hardwhere when the user is done previewing photos. 

     - a fix for the issue where a pinch to zoom the preview image also tries to zoom the camera hardware.

We should make these changes on master and then uplift to 2.0 and 1.4.

I can take a crack at this if no one else is available but there are so many moving parts in the new camera app that I worry I'd screw something up.

Wilson, Diego, Justin: is one of you able to take this bug from me?
Flags: needinfo?(wilsonpage)
Flags: needinfo?(jdarcangelo)
Flags: needinfo?(dmarcos)
I can take this
Flags: needinfo?(wilsonpage)
Flags: needinfo?(mvikram)
Flags: needinfo?(jdarcangelo)
Flags: needinfo?(dmarcos)
Assignee: dflanagan → dmarcos
> To fix this bug we need:
> 
>      - a change to the gallery build script to limit image previews to 3mp
> on 256mb devices
> 
>      - a similar change to the camera build script
> 
>      - a change to the camera so that it uses CONFIG_MAX_SNAPSHOT_PIXEL_SIZE
> to compute picture size rather than CONFIG_MAX_IMAGE_PIXEL_SIZE
> 

David -- i am not sure if we need to make the above 3 changes to limit image size to 3mp. As per my comment 21, I don't see any OOM without the config change. If you want I can try out a patch with just the following changes again.


>      - a change to camera so that it releases the camera hardware when
> previewing photos. (Even when it is not a low-memory device this may make a
> notice able difference in performance to not have the camera running while
> the user is zooming and panning a photo.)
> 
>      - a change to display a spinner when it reloads the camera hardwhere
> when the user is done previewing photos. 
> 
>      - a fix for the issue where a pinch to zoom the preview image also
> tries to zoom the camera hardware.
>
Inder,

I think your device is the first 256mb device with a 5mp camera we have worked with... I didn't realize that any such devices existed.  In my testing on a 288mb Flame, I do need to limit the decode size to 3mp or I OOM.  So I think it is safer to include the downsampling restriction.  Even if you don't see an OOM in Camera and Gallery when decoding at full 5mp, background apps are probably dying.  So I think imposing the 3mp limit is just a good defensive step and an acceptable compromise for these devices.  Being able to zoom in at 3mp still allows camera users to view most of the detail in their images.
Diego: is you plan to fix this on 1.4 and then do a patch for master and 2.0?  Or can you do one patch for master that will uplift to 2.0 and 1.4?
Status: NEW → ASSIGNED
Whiteboard: [caf priority: p1][CR 673864][MemShrink] → [c=memory p= s= u=1.4] [caf priority: p1][CR 673864][MemShrink]
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+][lead-review+]
(In reply to David Flanagan [:djf] from comment #37)
> So I think imposing the 3mp limit is
> just a good defensive step and an acceptable compromise for these devices. 
> Being able to zoom in at 3mp still allows camera users to view most of the
> detail in their images.

David -- sounds good. thanks.
Comment on attachment 8441789 [details] [review]
Alternative Pull Request v1.4

I updated the PR:

- Now we disable events over preview when settings or gallery are opened
- I display the loading spinner when returning to camera from gallery/preview
- I fixed unit tests

Once I get r+ I will on the solution for master/2.0
Attachment #8441789 - Flags: review?(dflanagan)
Flags: needinfo?(ying.xu)
Flags: needinfo?(sam.hua)
Flags: needinfo?(Xiaohong.Fan)
Flags: needinfo?(Dafeng.Xu)
Sam, you should check this issue on dolphin.
Component: Gaia::Gallery → Gaia::Camera
Comment on attachment 8441789 [details] [review]
Alternative Pull Request v1.4

I've left various comments on github (on the individual commits not the PR itself).

Overall, this looks fine to me.

Since Justin says he's working on build-time configurability in another bug, I'd like his feedback on the GAIA_MEMORY_PROFILE=256 thing before we land this, because if we can do something here that matches what he is doing in his bug that would probably be better.
Attachment #8441789 - Flags: review?(dflanagan)
Attachment #8441789 - Flags: review+
Attachment #8441789 - Flags: feedback?(jdarcangelo)
(In reply to David Flanagan [:djf] from comment #43)
> Since Justin says he's working on build-time configurability in another bug,
> I'd like his feedback on the GAIA_MEMORY_PROFILE=256 thing before we land
> this, because if we can do something here that matches what he is doing in
> his bug that would probably be better.

This should be fine for v1.4 since the device profiles stuff I'm working on will likely not make its way there.
Attachment #8441789 - Flags: feedback?(jdarcangelo) → feedback+
Justin. What the status of the device profiles? I have to port this patch to master too. It would be great to use your stuff
Updated the PR to address review comments. Waiting for green
Flags: needinfo?(Xiaohong.Fan)
I'll work now on porting this to master/v2.0
Whiteboard: [c=memory p= s= u=1.4] [caf priority: p1][CR 673864][MemShrink] → [c=memory p= s= u=1.4] [caf priority: p1][CR 673864][MemShrink:P2]
This has landed in v1.4. Should we remove the 1.4+ flag?
Flags: needinfo?(hkoka)
(In reply to Diego Marcos [:dmarcos] from comment #49)
> This has landed in v1.4. Should we remove the 1.4+ flag?

Landing a patch does not imply a blocking flag gets removed. They are completely independent entities.
(In reply to Diego Marcos [:dmarcos] from comment #49)
> This has landed in v1.4. Should we remove the 1.4+ flag?

Diego, 

If a bug is marked with 1.4+ flag, then it means it is a blocking bug for that release. Once it lands on 1.4, you can change the bug status to resolved-fixed and QA will then verify and mark it resolved-verified. We don't need to remove the 1.4+ flag.  

Some more landing info: https://wiki.mozilla.org/Release_Management/B2G_Landing

Thanks
Hema
Flags: needinfo?(hkoka)
blocking-b2g: 1.4+ → ---
comment 50 & comment 51 already clarified why the flag shouldn't be removed here, so re-adding the blocking flag.
blocking-b2g: --- → 1.4+
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Per our usual practices, we shouldn't mark this bug as fixed until it's landed on master unless the bug *only* affects a given branch. Given comment 41, it seems this bug does affect all branches.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I tried dolphin(256M RAM, 480x800 resolution) , which takes photos with 5MP pixels
I didn't find this bug.

apusr@yingxuubt:/home/ffos/yingxu/6821$ adb shell b2g-info
                  |      megabytes     |
  NAME   PID PPID  CPU(s) NICE  USS  PSS  RSS VSIZE OOM_ADJ USER     
   b2g   846    1 15751.3    0 44.6 48.8 53.4 243.0       0 root     
(Nuwa)   895  846    29.8    0  0.3  0.5  1.0  50.9       0 root     
Camera 15736  895    61.8    1 35.5 39.8 44.5 123.5       2 u0_a15736

System memory info:

            Total 215.8 MB
     Used - cache 165.7 MB
  B2G procs (PSS)  89.1 MB
    Non-B2G procs  76.6 MB
     Free + cache  50.1 MB
             Free  14.5 MB
            Cache  35.6 MB
Flags: needinfo?(ying.xu)
Bhavana/Hema - what's the status of the fix for 2.0?
Flags: needinfo?(hkoka)
Inder, 

Diego is working on a 2.0 patch. Could you please help test the patch that he is planning to add to this bug soon and see if that fixes it.

Thanks
Hema
Flags: needinfo?(hkoka)
This patch makes improvements to save memory:

1. Zoom is disabled when gallery and settings are opened.
2. The camera hardware is released when going to gallery/preview and requested when going back to camera mode.

To prevent out of memory issues on devices with 256 MB we recommend limiting the picture resolution to 3MP. This can be done by changing the configuration profile:

https://github.com/mozilla-b2g/gaia/tree/master/apps/camera#camera-configuration

Below the values that have to be modified:

// It limits the resolution to 3MP
CONFIG_MAX_IMAGE_PIXEL_SIZE: 3145728
CONFIG_MAX_SNAPSHOT_PIXEL_SIZE: 3145728

// It makes the camera use the values above to limit picture size
limitMaxPreviewSize: true
Flags: needinfo?(ikumar)
Attachment #8448383 - Flags: review?(jdarcangelo)
Attachment #8448383 - Flags: feedback?(ikumar)
Comment on attachment 8448383 [details] [review]
Pull Request for Master

Conditional R+.. See comment in GitHub PR (Syntax mistake.. I don't think the tests will pass) -- Otherwise, looks good!
Attachment #8448383 - Flags: review?(jdarcangelo) → review+
Inder/Tapas: Diego is unable to reproduce this issue, we really need your help to test and provide feedback on this patch on your device before we can land the work. Please help!

Thanks
Hema
Flags: needinfo?(tkundu)
(In reply to Diego Marcos [:dmarcos] from comment #57)

> 
> To prevent out of memory issues on devices with 256 MB we recommend limiting
> the picture resolution to 3MP. This can be done by changing the
> configuration profile:
> 
> https://github.com/mozilla-b2g/gaia/tree/master/apps/camera#camera-
> configuration
> 
> Below the values that have to be modified:
> 
> // It limits the resolution to 3MP
> CONFIG_MAX_IMAGE_PIXEL_SIZE: 3145728
> CONFIG_MAX_SNAPSHOT_PIXEL_SIZE: 3145728
> 
> // It makes the camera use the values above to limit picture size
> limitMaxPreviewSize: true

I think the 1.4 patch allowed 5mp photos, but downsampled them when previewing so they would never be opened at more than 3mp. Can't we do that here for this patch? Allow CONFIG_MAX_SNAPSHOT_PIXEL_SIZE to be 5mp but set CONFIG_MAX_IMAGE_PIXEL_SIZE to 3mp to avoid the OOMs?  Also, I don't think there is any need to have limitMaxPreviewSize. I suggest you get get rid of that entirely.
I like David's recommendation. Adding the ni on Diego.
We will help test the patch once David's recommendation is addressed.
Flags: needinfo?(ikumar) → needinfo?(dmarcos)
Flags: needinfo?(tkundu)
I updated the patch to set CONFIG_MAX_IMAGE_PIXEL_SIZE to 3MP. I tested with my flame with memory set to 288 MB and I don't see OOM issues. Can you please verify?
Flags: needinfo?(dmarcos) → needinfo?(ikumar)
Thanks Diego.
Tapas -- can you please give it a try.
Flags: needinfo?(ikumar) → needinfo?(tkundu)
(In reply to Diego Marcos [:dmarcos] from comment #62)
> I updated the patch to set CONFIG_MAX_IMAGE_PIXEL_SIZE to 3MP. I tested with
> my flame with memory set to 288 MB and I don't see OOM issues. Can you
> please verify?

Sorry for delayed reply. I tried your patch from #comment 58 and it is works fine for me 256MB msm8610 device running FFOS 2.0.

You may want to land it now. :) 
Thanks a lot for your efforts.
Flags: needinfo?(tkundu)
Flags: needinfo?(dmarcos)
I just rebased. I'll land as soon as I get green on Travis.
Flags: needinfo?(dmarcos)
Landed on master:

https://github.com/mozilla-b2g/gaia/commit/abf2a23f7e7fed7e5290549743f5a170ef1f7edf
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
(In reply to Diego Marcos [:dmarcos] from comment #62)
> I updated the patch to set CONFIG_MAX_IMAGE_PIXEL_SIZE to 3MP.

Can we make this config 256MB specific?  Flame at 1GB ram for example need not be restricted to a 3MP image.
Target Milestone: --- → 2.0 S6 (18july)
> Can we make this config 256MB specific?  Flame at 1GB ram for example need
> not be restricted to a 3MP image.

Totally agree with Michael. David can you please also take a look too.
Flags: needinfo?(dflanagan)
Attachment #8448383 - Flags: feedback?(ikumar)
This caused regression bug 1037462.
Depends on: 1037462
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
No longer depends on: 1037462
Why is bug reopened? We should be backing out the patch here if we're reopening.
Flags: needinfo?(hkoka)
(In reply to Jason Smith [:jsmith] from comment #70)
> Why is bug reopened? We should be backing out the patch here if we're
> reopening.

This shouldn't be backed out. Bug 1036312 is R+ -- waiting on Travis/Gaia-Try, then landing.
(In reply to Justin D'Arcangelo [:justindarc] from comment #71)
> (In reply to Jason Smith [:jsmith] from comment #70)
> > Why is bug reopened? We should be backing out the patch here if we're
> > reopening.
> 
> This shouldn't be backed out. Bug 1036312 is R+ -- waiting on
> Travis/Gaia-Try, then landing.

Then why is this bug reopened? Bugs should *not* be reopened if there's a patch already landed for the bug.
I reopened the bug to look into addressing comment 68. But perhaps it is better to create a follow up bug on that instead.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Flags: needinfo?(hkoka)
Resolution: --- → FIXED
Blocks: 1037638
No longer blocks: 1037638
Based on offline IRC discussions on this with Inder, Diego, we will uplift this patch into 2.0 to unblock CAF testing and address comment 67 as part of follow up bug that we opened. 

Thanks
Hema
(In reply to Hema Koka [:hema] from comment #74)
> Based on offline IRC discussions on this with Inder, Diego, we will uplift
> this patch into 2.0 to unblock CAF testing and address comment 67 as part of
> follow up bug that we opened. 

OK, Let me NI ryan to get this uplifted asap on 2.0.
> 
> Thanks
> Hema
Flags: needinfo?(ryanvm)
Attached file Pull request for 2.0
Flags: needinfo?(sam.hua)
Why aren't we doing a runtime detection using navigator.getFeature() ?
Flags: in-moztrap?(bzumwalt)
New test case needs to be written to address bug.
QA Whiteboard: [QAnalyst-Triage+][lead-review+] → [QAnalyst-Triage?][lead-review+]
Flags: needinfo?(ktucker)
There are test cases in moztrap to cover this issue:

https://moztrap.mozilla.org/manage/case/1856/

https://moztrap.mozilla.org/manage/case/1857/
QA Whiteboard: [QAnalyst-Triage?][lead-review+] → [QAnalyst-Triage+][lead-review+]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(bzumwalt)
Flags: in-moztrap+
Bulk edit to clear old and out of date needinfo requests that I never responded to. I'm assuming that these are no longer relevant. If you are still waiting for an answer from me, please set needinfo? again.
Flags: needinfo?(dflanagan)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: