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)
Tracking
(blocking-b2g:1.4+, 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.
Reporter | ||
Updated•10 years ago
|
blocking-b2g: --- → 1.4?
Comment 1•10 years ago
|
||
QA Wanted to get an about:memory report on 1.4.
Updated•10 years ago
|
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.
Comment 3•10 years ago
|
||
David, Can you please take a look at this? Thanks Hema
Flags: needinfo?(dflanagan)
Comment 4•10 years ago
|
||
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)
Comment 5•10 years ago
|
||
(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)
Updated•10 years ago
|
Assignee: nobody → dflanagan
Comment 6•10 years ago
|
||
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)
Comment 7•10 years ago
|
||
Tapas, please provide info requested in comment 4 so we can proceed further on this bug
Reporter | ||
Comment 8•10 years ago
|
||
(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)
Comment 9•10 years ago
|
||
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
Comment 10•10 years ago
|
||
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.
Updated•10 years ago
|
Flags: needinfo?(tkundu)
Updated•10 years ago
|
Flags: needinfo?(jmitchell)
Comment 11•10 years ago
|
||
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?
Comment 12•10 years ago
|
||
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)
Comment 13•10 years ago
|
||
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.
Comment 14•10 years ago
|
||
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)
Comment 15•10 years ago
|
||
(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)
Comment 16•10 years ago
|
||
(In reply to David Flanagan [:djf] from comment #14) > Created attachment 8440211 [details] > 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)
Comment 17•10 years ago
|
||
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)
Comment 18•10 years ago
|
||
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.
Comment 19•10 years ago
|
||
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 20•10 years ago
|
||
Comment on attachment 8440211 [details]
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)
Comment 21•10 years ago
|
||
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)
Updated•10 years ago
|
blocking-b2g: 1.4? → 1.4+
Comment 22•10 years ago
|
||
Please make sure to verify this once it lands on 1.4 as we were unable to repro in MOzilla.
Comment 23•10 years ago
|
||
Requesting UX input here per comment 21
Flags: needinfo?(firefoxos-ux-bugzilla)
Comment 24•10 years ago
|
||
Please clarify what the UX feedback in comment #21 is needed on. Sounds like a performance issue but let us know.
Comment 25•10 years ago
|
||
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.
Updated•10 years ago
|
Flags: needinfo?(tshakespeare)
Flags: needinfo?(firefoxos-ux-bugzilla)
Flags: needinfo?(dflanagan)
Comment 26•10 years ago
|
||
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)
Comment 27•10 years ago
|
||
vikram -- need your feedback too on the performance impact.
Flags: needinfo?(mvikram)
Comment 28•10 years ago
|
||
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)
Comment 29•10 years ago
|
||
I don't believe the reusable spinner with the variable delays is in v1.4 though.
Flags: needinfo?(jdarcangelo)
Assignee | ||
Comment 30•10 years ago
|
||
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?
Comment 31•10 years ago
|
||
(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)
Comment 32•10 years ago
|
||
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!
Comment 33•10 years ago
|
||
David -- let me know if you want me to try out a new patch.
Comment 34•10 years ago
|
||
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)
Assignee | ||
Comment 35•10 years ago
|
||
I can take this
Flags: needinfo?(wilsonpage)
Flags: needinfo?(mvikram)
Flags: needinfo?(jdarcangelo)
Flags: needinfo?(dmarcos)
Assignee | ||
Updated•10 years ago
|
Assignee: dflanagan → dmarcos
Comment 36•10 years ago
|
||
> 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. >
Comment 37•10 years ago
|
||
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.
Assignee | ||
Comment 38•10 years ago
|
||
Comment 39•10 years ago
|
||
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?
Updated•10 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [caf priority: p1][CR 673864][MemShrink] → [c=memory p= s= u=1.4] [caf priority: p1][CR 673864][MemShrink]
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+][lead-review+]
Comment 40•10 years ago
|
||
(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.
Assignee | ||
Comment 41•10 years ago
|
||
Comment on attachment 8441789 [details]
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)
Updated•10 years ago
|
Flags: needinfo?(ying.xu)
Flags: needinfo?(sam.hua)
Flags: needinfo?(Xiaohong.Fan)
Flags: needinfo?(Dafeng.Xu)
Comment 42•10 years ago
|
||
Sam, you should check this issue on dolphin.
Updated•10 years ago
|
Component: Gaia::Gallery → Gaia::Camera
Comment 43•10 years ago
|
||
Comment on attachment 8441789 [details]
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)
Comment 44•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8441789 -
Flags: feedback?(jdarcangelo) → feedback+
Assignee | ||
Comment 45•10 years ago
|
||
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
Assignee | ||
Comment 46•10 years ago
|
||
Updated the PR to address review comments. Waiting for green
Updated•10 years ago
|
Flags: needinfo?(Xiaohong.Fan)
Assignee | ||
Comment 47•10 years ago
|
||
Landed in v1.4: https://github.com/mozilla-b2g/gaia/commit/127730f3a45f1b35416af294c74902a51ea0a44d
Assignee | ||
Comment 48•10 years ago
|
||
I'll work now on porting this to master/v2.0
Updated•10 years ago
|
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]
Assignee | ||
Comment 49•10 years ago
|
||
This has landed in v1.4. Should we remove the 1.4+ flag?
Flags: needinfo?(hkoka)
Comment 50•10 years ago
|
||
(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.
Comment 51•10 years ago
|
||
(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)
Assignee | ||
Updated•10 years ago
|
blocking-b2g: 1.4+ → ---
Comment 52•10 years ago
|
||
comment 50 & comment 51 already clarified why the flag shouldn't be removed here, so re-adding the blocking flag.
blocking-b2g: --- → 1.4+
Assignee | ||
Updated•10 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 53•10 years ago
|
||
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
status-b2g-v1.4:
--- → fixed
status-b2g-v2.0:
--- → affected
status-b2g-v2.1:
--- → affected
Resolution: FIXED → ---
Comment 54•10 years ago
|
||
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
Comment 55•10 years ago
|
||
Bhavana/Hema - what's the status of the fix for 2.0?
Blocks: CAF-v2.0-FC-metabug
Comment 56•10 years ago
|
||
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)
Assignee | ||
Comment 57•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8448383 -
Flags: review?(jdarcangelo)
Attachment #8448383 -
Flags: feedback?(ikumar)
Comment 58•10 years ago
|
||
Comment on attachment 8448383 [details]
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+
Comment 59•10 years ago
|
||
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)
Comment 60•10 years ago
|
||
(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.
Comment 61•10 years ago
|
||
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)
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(tkundu)
Assignee | ||
Comment 62•10 years ago
|
||
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)
Comment 63•10 years ago
|
||
Thanks Diego. Tapas -- can you please give it a try.
Flags: needinfo?(ikumar) → needinfo?(tkundu)
Reporter | ||
Comment 64•10 years ago
|
||
(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)
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(dmarcos)
Assignee | ||
Comment 65•10 years ago
|
||
I just rebased. I'll land as soon as I get green on Travis.
Flags: needinfo?(dmarcos)
Assignee | ||
Comment 66•10 years ago
|
||
Landed on master: https://github.com/mozilla-b2g/gaia/commit/abf2a23f7e7fed7e5290549743f5a170ef1f7edf
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Comment 67•10 years ago
|
||
(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.
Updated•10 years ago
|
Target Milestone: --- → 2.0 S6 (18july)
Comment 68•10 years ago
|
||
> 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)
Comment 69•10 years ago
|
||
This caused regression bug 1037462.
Updated•10 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 70•10 years ago
|
||
Why is bug reopened? We should be backing out the patch here if we're reopening.
Flags: needinfo?(hkoka)
Comment 71•10 years ago
|
||
(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.
Comment 72•10 years ago
|
||
(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.
Comment 73•10 years ago
|
||
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 ago → 10 years ago
Flags: needinfo?(hkoka)
Resolution: --- → FIXED
Comment 74•10 years ago
|
||
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
Comment 75•10 years ago
|
||
(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)
Comment 76•10 years ago
|
||
v2.0: https://github.com/mozilla-b2g/gaia/commit/65ba1cbd2660b770263e6053ff70648843ff7b17
Flags: needinfo?(ryanvm)
Assignee | ||
Comment 77•10 years ago
|
||
Comment 78•10 years ago
|
||
Why aren't we doing a runtime detection using navigator.getFeature() ?
Updated•10 years ago
|
Flags: in-moztrap?(bzumwalt)
Comment 79•10 years ago
|
||
New test case needs to be written to address bug.
QA Whiteboard: [QAnalyst-Triage+][lead-review+] → [QAnalyst-Triage?][lead-review+]
Flags: needinfo?(ktucker)
Comment 80•10 years ago
|
||
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+
Comment 81•10 years ago
|
||
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.
Updated•10 years ago
|
Flags: needinfo?(dflanagan)
You need to log in
before you can comment on or make changes to this bug.
Description
•