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

Status: RESOLVED FIXED

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

Description

[Andrew Stole [:astole]]

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

Comment 1

[Andrew Stole [:astole]]

Attachment: logcat

Attaching logcat

Comment 2

[Peter Bylenga [:PBylenga]]

In your actual results you state the device crashed, did the OS restart or did the app force close?

Comment 3

[Andrew Stole [:astole]]

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.

Comment 4

[Peter Bylenga [:PBylenga]]

Thanks for clarifying, adding qawanted to do flame branch checks with memory set to 273MB.

Comment 5

[Cody Roesch [:croesch]]

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

Comment 6

[Joshua Mitchell (Inactive)]

Nomming for 2.0, crash in a major feature (camera)

Comment 7

[Mike Habicher [:mikeh] (high bugzilla latency)]

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.

Comment 8

[Mike Habicher [:mikeh] (high bugzilla latency)]

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).

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We should investigate, but I'm not convinced that expecting to support HDR in 256 MB of memory is reasonable.

Comment 10

[Mike Habicher [:mikeh] (high bugzilla latency)]

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.

Comment 11

[Andrew Stole [:astole]]

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

Comment 12

[Mike Habicher [:mikeh] (high bugzilla latency)]

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

Comment 13

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Output of |adb shell top -m 10 -d 1|

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.

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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?

Comment 15

[Mike Habicher [:mikeh] (high bugzilla latency)]

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

[Hema Koka [:hema]]

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

Comment 17

[Sri Kasetti]

We can turn off HDR for low memory devices and indicate that 512 MB as the minimum memory requirement to support this feature..

Comment 18

[Jason Smith [:jsmith]]

comment 17 addressed the needinfo to me.

Comment 19

[Jason Smith [:jsmith]]

What we should probably do in this bug then is getting HDR turned off if memory < 512 MB.

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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.

Comment 21

[Wayne Chang [:wchang]]

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?

Comment 22

[Sri Kasetti]

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"

Comment 23

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Disable the HDR scene mode on low-memory devices, v1

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.)

Comment 24

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Disable the HDR scene mode on low-memory devices, v1.1

try-server (re)push: https://tbpl.mozilla.org/?tree=Try&rev=bd55074387d0

Comment 25

[Dave Hylands [: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?

Comment 26

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Disable the HDR scene mode on low-memory devices, v1.2 r=dhylands

Incorporating review feedback; carrying r+ forward.

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=7a993c630fb7

Comment 27

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Disable the HDR scene mode on low-memory devices, v1.3 r=dhylands

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

Comment 28

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Disable the HDR scene mode on low-memory devices, v1.4

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.

Comment 29

[Dave Hylands [: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.

Comment 30

[Mike Habicher [:mikeh] (high bugzilla latency)]

Thanks, Dave!

https://hg.mozilla.org/integration/b2g-inbound/rev/88a2ebd864b0

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/88a2ebd864b0

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Aurora uplift.

Comment 33

[Mike Habicher [:mikeh] (high bugzilla latency)]

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.)

Comment 34

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yes, 2.0 is targeting 256 MB devices.

Comment 35

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #34)

> Yes, 2.0 is targeting 256 MB devices.

...that support HDR?

Comment 36

[bhavana bajaj [:bajaj]]

(In reply to Mike Habicher [:mikeh] from comment #35)
> (In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 37

[Hema Koka [:hema]]

(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

Comment 38

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Disable the HDR scene mode on low-memory devices, v1.4 [aurora] r=dhylands a=2.0+

Backport to 2.0.

Comment 39

[bhavana bajaj [:bajaj]]

(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.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/48868fc69e45

Comment 41

[Marcia Knous [:marcia]]

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.