Closed
Bug 1036637
Opened 10 years ago
Closed 10 years ago
[B2G][Camera]Taking a picture with HDR enabled causes the camera app to crash
Categories
(Firefox OS Graveyard :: Gaia::Camera, defect)
Tracking
(blocking-b2g:2.0+, firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v1.4 affected, b2g-v2.0 fixed, b2g-v2.1 fixed)
People
(Reporter: astole, Assigned: mikeh)
References
Details
(Keywords: memory-footprint, perf, Whiteboard: [273MB-Flame-Support][MemShrink])
Attachments
(2 files, 6 obsolete files)
(deleted),
patch
|
dhylands
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mikeh
:
review+
|
Details | Diff | Splinter Review |
When taking a picture with HD for the first time, the device crashes. With HD enabled, every time a picture is taken after the device crash, the camera app crashes. After the crash, there is no crash report captured. Repro Steps: 1) Update a Flame to BuildID: 20140709000201 2) Open the camera app 3) Enable HD option 4) Take a picture Actual: Device crashes. Expected: The device does not crash and the picture is taken. 2.0 Environmental Variables: Device: Flame 2.0 BuildID: 20140709000201 Gaia: 1dd043857399c713e3b509c0ed31bdf20326f08b Gecko: 0b5f757da75a Version: 32.0a2 Firmware Version: v122 Repro frequency: 3/3, 100% See attached: logcat
Reporter | ||
Updated•10 years ago
|
Reporter | ||
Updated•10 years ago
|
Summary: [B2G][Camera]Taking a picture with HD enabled causes the device to crash → [B2G][Camera]Taking a picture with HD enabled causes the camera app to crash
Comment 2•10 years ago
|
||
In your actual results you state the device crashed, did the OS restart or did the app force close?
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga) → needinfo?(astole)
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage-]
Reporter | ||
Comment 3•10 years ago
|
||
The first time a picture was taken the device crashed for 3 out of the 4 times it was tested (there was no crash log). After the initial device crash, every time a picture was taken with HD enabled, the app would crash, not the actual device.
QA Whiteboard: [QAnalyst-Triage-] → [QAnalyst-Triage?]
Flags: needinfo?(astole) → needinfo?(pbylenga)
Comment 4•10 years ago
|
||
Thanks for clarifying, adding qawanted to do flame branch checks with memory set to 273MB.
Comment 5•10 years ago
|
||
This bug repro's on: Flame 2.1 Master, Flame 2.0 and Flame 1.4 Actual Results: With 273mb, Taking a picture in the Camera app with HDR ON will crash the Camera app. Environmental Variables: Device: Flame Master Build ID: 20140709131330 Gaia: 4e4e579b4b1e35f863ed43ef6ba840f49bfd761c Gecko: fc35681b0a87 Version: 33.0a1 (Master) Firmware Version: v122 ----------------------------------------------- Environmental Variables: Device: Flame 2.0 Build ID: 20140709133231 Gaia: 1cd5a8546959df287ec56fd619f68877148cd1f9 Gecko: 6a7a6089f214 Version: 32.0a2 (2.0) Firmware Version: v122 ----------------------------------------------- Environmental Variables: Device: Flame 1.4 Build ID: 20140709075131 Gaia: b0e9b4bdb39c5eb93a6783a34624ffc84f62b126 Gecko: ccabaf8826a4 Version: 30.0 (1.4) Firmware Version: v122
QA Whiteboard: [QAnalyst-Triage?]
status-b2g-v1.4:
--- → affected
status-b2g-v2.1:
--- → affected
Flags: needinfo?(jmitchell)
Keywords: qawanted
QA Contact: croesch
Comment 6•10 years ago
|
||
Nomming for 2.0, crash in a major feature (camera)
blocking-b2g: --- → 2.0?
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
Assignee | ||
Comment 7•10 years ago
|
||
Much like video in bug 1027592, HDR puts a lot of memory pressure on the device. I'm not sure how many photos HDR stitches together into the high-dynamic range image, but it's more than one. And 273 MiB is already pushing it.
Summary: [B2G][Camera]Taking a picture with HD enabled causes the camera app to crash → [B2G][Camera]Taking a picture with HDR enabled causes the camera app to crash
Assignee | ||
Comment 8•10 years ago
|
||
Andrew, you can confirm the memory pressure by running the following command after the crash: # adb shell dmesg | grep sigkill You'll see lines like: <6>[ 503.698433] send sigkill to 1119 (Camera), adj 134, size 2914 ...which is the low-memory killer nuking the Camera app (and likely others).
Flags: needinfo?(astole)
We should investigate, but I'm not convinced that expecting to support HDR in 256 MB of memory is reasonable.
Whiteboard: [273MB-Flame-Support] → [273MB-Flame-Support][MemShrink]
Assignee | ||
Comment 10•10 years ago
|
||
Running the following build on a flame with mem limit set to 273MiB: Gonk v122 Gaia c329a67bafe60032820ab64b52c07c22eeff114e Gecko https://hg.mozilla.org/integration/b2g-inbound/rev/f17ba2374df6 BuildID 20140710065755 Version 33.0a1 ro.build.version.incremental=108 ro.build.date=Tue Jun 10 19:40:40 CST 2014 HDR on: <6>[ 39.345560] send sigkill to 976 (Built-in Keyboa), adj 667, size 3164 <6>[ 84.594314] send sigkill to 1061 (Homescreen), adj 534, size 1983 <6>[ 85.316141] send sigkill to 1062 (Camera), adj 134, size 3173 HDR off: No processes killed. With the mem limit set to 319MiB (from bug 1027592 comment 15), HDR on: <6>[ 53.941894] send sigkill to 975 (Built-in Keyboa), adj 667, size 3111 ...Camera app doesn't crash.
Reporter | ||
Comment 11•10 years ago
|
||
Ran the command when the camera app crashed on yesterday's build HDR on. This is the output I received if you still needed it. <6>[ 38.063115] send sigkill to 957 (Communications), adj 734, size 3265 <6>[ 40.766702] send sigkill to 1161 (Built-in Keyboa), adj 667, size 3004 <6>[ 204.276870] send sigkill to 1196 (Settings), adj 667, size 4759 <6>[ 224.538928] send sigkill to 1143 (Homescreen), adj 534, size 2600 <6>[ 226.744554] send sigkill to 1671 (Camera), adj 134, size 1031
Flags: needinfo?(astole)
Assignee | ||
Comment 12•10 years ago
|
||
Bisection of the build in comment 10 shows that with a mem limit of 347MiB, no other processes get killed when HDR is on; but with a mem limit of 346MiB, processes (usually the keyboard) start getting killed: <6>[ 50.330251] send sigkill to 1068 (Built-in Keyboa), adj 667, size 3351
Assignee | ||
Comment 13•10 years ago
|
||
grepping for "qcamera" shows that whenever an HDR picture is taken, the VSS of the mm-qcamera-daemon process temporarily jumps from ~44MiB to ~93MiB, an increase of almost 49MiB. Similarly, the RSS of that process jumps from ~9MiB to ~37MiB, an increase of 28MiB. Once the HDR processing is complete, the VSS/RSS figures return to their previous values.
I claim that taking a picture in HD is out of scope for a 256 MB device. Does any other device on the market support HD pictures with only 256 MB of memory? This is a product call at this point, I think. Jason, who should I ask to weigh in here?
Flags: needinfo?(jsmith)
Assignee | ||
Comment 15•10 years ago
|
||
Further to comment 13, when HDR is off, the mm-qcamera-daemon process' VSS and RSS values stay fairly constant at 44 and 9MiB, respectively, when taking a picture.
Comment 16•10 years ago
|
||
NI Sri and Wayne Based on the testing/investigation by Mike, Flame needs to be configured at 347MB at least for taking HDR pictures. See Mike's earlier comments. How important is to support HDR on a 256 MB device? We can configure to turn HDR off for 256 MB devices. Please provide input from the product side. Thanks Hema
Flags: needinfo?(wchang)
Flags: needinfo?(skasetti)
Comment 17•10 years ago
|
||
We can turn off HDR for low memory devices and indicate that 512 MB as the minimum memory requirement to support this feature..
Flags: needinfo?(skasetti)
Comment 19•10 years ago
|
||
What we should probably do in this bug then is getting HDR turned off if memory < 512 MB.
Updated•10 years ago
|
(In reply to Jason Smith [:jsmith] from comment #19) > What we should probably do in this bug then is getting HDR turned off if > memory < 512 MB. Agreed.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → mhabicher
Comment 21•10 years ago
|
||
I have no concerns if we need to disable HDR for lower spec'ed devices. Sri, can you update the roadmap/features wiki to indicate this requirement in case partners refer to them?
Flags: needinfo?(wchang) → needinfo?(skasetti)
Comment 22•10 years ago
|
||
Adding Jean since she manages the wiki. Jean, please update the roadmap wiki (bug 966835) with the following "HDR feature is supported for devices with RAM of 512 MB and above"
Flags: needinfo?(skasetti) → needinfo?(jgong)
Assignee | ||
Comment 23•10 years ago
|
||
try-server push: https://tbpl.mozilla.org/?tree=Try&rev=7ce6ecaba2ba This patch removes 'hdr' from the list of supported scene modes on low-memory devices, which we have decided is any device with less than 512MiB of RAM. (On flame, empirically, this means the MemTotal field in /proc/meminfo is less than 414268 kB.)
Attachment #8453339 -
Attachment is obsolete: true
Attachment #8453846 -
Attachment is obsolete: true
Attachment #8454489 -
Flags: review?(dhylands)
Assignee | ||
Comment 24•10 years ago
|
||
try-server (re)push: https://tbpl.mozilla.org/?tree=Try&rev=bd55074387d0
Attachment #8454489 -
Attachment is obsolete: true
Attachment #8454489 -
Flags: review?(dhylands)
Attachment #8454614 -
Flags: review?(dhylands)
Comment 25•10 years ago
|
||
Comment on attachment 8454614 [details] [diff] [review] Disable the HDR scene mode on low-memory devices, v1.1 Review of attachment 8454614 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, although I think that hard-coding the 404Mb is bad, and I think it should be a preference. I also think that you're missing a test (test that hdr stays present when low memory is not set) r=me with those 2 issues addressed. ::: dom/camera/GonkCameraParameters.cpp @@ +28,5 @@ > +GonkCameraParameters::IsLowMemoryPlatform() > +{ > + // Empirically, this is the value returned by hal::GetTotalSystemMemory() > + // when Flame's memory is limited to 512MiB. > + const uint32_t kMinimumTotalMemoryForHDRSupport = (404 * 1024 * 1024); MemTotal reported in /proc/meminfo is essentially the total amount of physical memory minus the amount needed for the kernel code and static data. MemTotal will reduce/increase by reconfiguring the kernel or moving to a new kernel, and may vary from device type A to device type B. I think that this should be a preference to allow vendors to tune the number. @@ +288,5 @@ > + while (mSceneModes.RemoveElement(NS_LITERAL_STRING("hdr"))) { > + hdrRemoved = true; > + } > + if (hdrRemoved) { > + DOM_CAMERA_LOGI("Disabling HDR support\n"); nit: perhaps this should say "Disabling HDR support due to low memory\n" ::: dom/camera/test/test_camera_fake_parameters.html @@ +124,5 @@ > + run(); > + }); > + }); > + }, > + test: function testFakeLowMemoryPlatform(cam, cap) { shouldn't we also test that hdr is present when low memory is not set?
Attachment #8454614 -
Flags: review?(dhylands) → review+
Assignee | ||
Comment 26•10 years ago
|
||
Incorporating review feedback; carrying r+ forward. try-server push: https://tbpl.mozilla.org/?tree=Try&rev=7a993c630fb7
Attachment #8454614 -
Attachment is obsolete: true
Attachment #8454660 -
Flags: review+
Updated•10 years ago
|
blocking-b2g: 2.0? → 2.0+
Assignee | ||
Comment 27•10 years ago
|
||
Carrying r+ forward: this version is required to deal with old(er) compilers where nsresult is simply a typedef of uint32_t, causing issues with method overloading. It also fixes an incorrect #ifndef around the low-memory threshold pref in all.js. try-server push: https://tbpl.mozilla.org/?tree=Try&rev=cccb7b4aba4a
Attachment #8454660 -
Attachment is obsolete: true
Attachment #8454717 -
Flags: review+
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][lead-review+]
Assignee | ||
Comment 28•10 years ago
|
||
Would you mind giving this one more look over, Dave? This passes try and local builds/testing for flame and the emulator; but in incorporating your feedback, I needed to make a few not-insignificant changes.
Attachment #8454717 -
Attachment is obsolete: true
Attachment #8454947 -
Flags: review?(dhylands)
Comment 29•10 years ago
|
||
Comment on attachment 8454947 [details] [diff] [review] Disable the HDR scene mode on low-memory devices, v1.4 Review of attachment 8454947 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me - I actually looked over the changes when you submitted the try run.
Attachment #8454947 -
Flags: review?(dhylands) → review+
Assignee | ||
Comment 30•10 years ago
|
||
Thanks, Dave! https://hg.mozilla.org/integration/b2g-inbound/rev/88a2ebd864b0
Comment 31•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/88a2ebd864b0
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 32•10 years ago
|
||
Needs rebasing for Aurora uplift.
Assignee | ||
Comment 33•10 years ago
|
||
Hema, 2.0 is affected by this problem, but is it worth fixing? Are we targeting 2.0 for any low-memory devices? (Rebasing the patch is non-trivial.)
Flags: needinfo?(mhabicher) → needinfo?(hkoka)
Yes, 2.0 is targeting 256 MB devices.
Assignee | ||
Comment 35•10 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #34) > Yes, 2.0 is targeting 256 MB devices. ...that support HDR?
Comment 36•10 years ago
|
||
(In reply to Mike Habicher [:mikeh] from comment #35) > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #34) > > > Yes, 2.0 is targeting 256 MB devices. > > ...that support HDR? Isn't the patch here disabling HDR on low memory devices? 256 MB devices will get 2.0 but if certain features can't run on it we may have to pref them off as necessary. From CAF's perspective they do not need support for HDR on 256 MB for 2.0.
Assignee | ||
Updated•10 years ago
|
Blocks: camera-memory
Comment 37•10 years ago
|
||
(In reply to Mike Habicher [:mikeh] from comment #33) > Hema, 2.0 is affected by this problem, but is it worth fixing? Are we > targeting 2.0 for any low-memory devices? (Rebasing the patch is > non-trivial.) Yes. Kyle already answered it. HDR can be disabled for low-memory devices. Can you please look into rebasing the patch for aurora? CAF can get this onto their builds and run memory reports. Thanks Hema
Flags: needinfo?(hkoka)
Comment 39•10 years ago
|
||
(In reply to Mike Habicher [:mikeh] from comment #38) > Created attachment 8456316 [details] [diff] [review] > Disable the HDR scene mode on low-memory devices, v1.4 [aurora] r=dhylands > a=2.0+ > > Backport to 2.0. NI :ryan to help land this.
Flags: needinfo?(ryanvm)
Comment 40•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/48868fc69e45
status-firefox31:
--- → wontfix
status-firefox32:
--- → fixed
status-firefox33:
--- → fixed
Flags: needinfo?(ryanvm)
Keywords: branch-patch-needed
Comment 41•10 years ago
|
||
Verifying while running an Open 2 device using: Gaia b9d19011123487009c80d1200937652d58c434a0 SourceStamp 00f4b3a7046f BuildID 20140722000200 Version 32.0 HDR is not shown as an option on this device.
Updated•10 years ago
|
Flags: needinfo?(jgong) → needinfo?
Updated•10 years ago
|
Flags: needinfo?
You need to log in
before you can comment on or make changes to this bug.
Description
•