Closed Bug 1036637 Opened 5 years ago Closed 5 years ago

[B2G][Camera]Taking a picture with HDR enabled causes the camera app to crash

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.0+, firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v1.4 affected, b2g-v2.0 fixed, b2g-v2.1 fixed)

RESOLVED FIXED
2.0 S6 (18july)
blocking-b2g 2.0+
Tracking Status
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)

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
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
Attached file logcat (obsolete) —
Attaching logcat
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
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)
QA Whiteboard: [QAnalyst-Triage-]
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)
Thanks for clarifying, adding qawanted to do flame branch checks with memory set to 273MB.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
Keywords: qawanted
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?]
Flags: needinfo?(jmitchell)
Keywords: qawanted
QA Contact: croesch
Nomming for 2.0, crash in a major feature (camera)
blocking-b2g: --- → 2.0?
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
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
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]
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.
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)
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
Attached file Output of |adb shell top -m 10 -d 1| (obsolete) —
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)
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.
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)
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 17 addressed the needinfo to me.
Flags: needinfo?(jsmith)
What we should probably do in this bug then is getting HDR turned off if memory < 512 MB.
Keywords: footprint, perf
(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: nobody → mhabicher
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)
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)
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)
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 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+
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+
blocking-b2g: 2.0? → 2.0+
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+
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][lead-review+]
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 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+
https://hg.mozilla.org/mozilla-central/rev/88a2ebd864b0
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Needs rebasing for Aurora uplift.
Flags: needinfo?(mhabicher)
Target Milestone: --- → 2.0 S6 (18july)
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.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #34)

> Yes, 2.0 is targeting 256 MB devices.

...that support HDR?
(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.
(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)
(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)
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.
Flags: needinfo?(jgong) → needinfo?
Flags: needinfo?
You need to log in before you can comment on or make changes to this bug.