990761 - [Camera][Madai][QRD Only] Camera preview freezes after taking 1st picture when HDR is ON

Status: RESOLVED FIXED

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

Description

[Hema Koka [:hema]]

Issue: Camera preview freezes after taking 1st picture when HDR is ON.

Device: QRD (Qualcomm)

Build: Gecko rev: 38bc15c969fb1b5d435fa943f488bb4bd8918397

Gaia rev: eee964fc7e8dc519218dee6b72595ad0f7dd8231

Comment 1

[Hema Koka [:hema]]

Youngjun, please have someone from your team take this up and see if you can reproduce on nexus4.

Comment 2

[Youngjun Kim]

Dear Ashish,

Please check it whether it's reproduced on Nexus4.

Comment 3

[singhashish1887]

Hi Hema,

This bug is not reproducible on Nexus-4 Devices.
We need your help to reproduce this issue on Nexus Device .

Comment 4

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

I can also confirm that this does not reproduce on a nexus-4, running:
- gecko: b2g-inbound:275feb8db98a
- gaia: camera-new-features:bb7e44e428640f15e1c07b0ab89c712e797fb9ea

Comment 5

[Hema Koka [:hema]]

Mike - need your help reproducing this on the QRD that you have (new build info from QC is here: https://bugzilla.mozilla.org/show_bug.cgi?id=961246)

Comment 6

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

I can certainly reproduce this one:
- gecko: b2g_autogen_ephemeral_branch:8e91fa5a127e5cbfa92a99896f4501ede717d347
- gaia: master:43255a9fac46da49937eb145aeeee0389d62ef07

I don't know what 'b2g-autogen_ephemeral_branch" is, but that seems to be what the QRD build script leaves $B2G/gecko in.

Comment 7

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

Attachment: logcat of issue reproducing

In this logcat, there seems to be a single call to takePicture():

04-10 13:05:32.370   199  1507 E QCamera2HWI: [KPI Perf] static int qcamera::QCamera2HardwareInterface::take_picture(camera_device*): E PROFILE_TAKE_PICTURE
04-10 13:05:32.370   199  1572 D QCamera2HWI: int qcamera::QCamera2HardwareInterface::takePicture(): E
   ...
04-10 13:05:32.680   199  1572 D QCamera2HWI: int qcamera::QCamera2HardwareInterface::takePicture(): X
04-10 13:05:32.680   199  1507 D QCamera2HWI: [KPI Perf] static int qcamera::QCamera2HardwareInterface::take_picture(camera_device*): X

BUT there are _two_ callbacks invoked, 100ms apart:

04-10 13:05:35.890  1114  1651 I Gecko   : Got picture, type 'image/jpeg', 1121299 bytes
04-10 13:05:35.890  1114  1651 I Gecko   : nsGonkCameraControl::OnTakePictureComplete() done

and:

04-10 13:05:35.960  1114  1122 I Gecko   : Got picture, type 'image/jpeg', 645673 bytes
04-10 13:05:35.960  1114  1122 I Gecko   : nsGonkCameraControl::OnTakePictureComplete() done

Comment 8

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

The logcat also shows, after the first callback invocation:

04-10 13:05:35.890  1114  1651 I Gecko   : Got picture, type 'image/jpeg', 1121299 bytes
04-10 13:05:35.890  1114  1651 I Gecko   : nsGonkCameraControl::OnTakePictureComplete() done
04-10 13:05:35.890  1114  1548 I Gecko   : Starting preview (this=0xb2ce6640)
04-10 13:05:35.890  1114  1548 I Gecko   : virtual int android::GonkCameraHardware::StartPreview():326 : this=0xb2a034c0
04-10 13:05:35.890   199  1572 E QCameraStateMachine: Cannot set preview window when preview is running
04-10 13:05:35.890   199   199 E QCamera2HWI: [KPI Perf] static int qcamera::QCamera2HardwareInterface::start_preview(camera_device*): E PROFILE_START_PREVIEW
04-10 13:05:35.890   199  1572 E QCameraStateMachine: int32_t qcamera::QCameraStateMachine::procEvtPicTakingState(qcamera::qcamera_sm_evt_enum_t, void*): cannot handle evt(9) in state(4)
04-10 13:05:35.890   199   199 D QCamera2HWI: [KPI Perf] static int qcamera::QCamera2HardwareInterface::start_preview(camera_device*): X
04-10 13:05:35.890  1114  1548 I Gecko   : Failed to start camera preview
04-10 13:05:35.890  1114  1548 I Gecko   : Camera control API failed at 7 with 0x80004005
04-10 13:05:35.890  1114  1114 I Gecko   : DOM OnError context=7, error='FAILURE'

Except the preview was stopped earlier:

04-10 13:05:22.990   199  1572 D QCamera2HWI: int qcamera::QCamera2HardwareInterface::stopPreview(): E
   ...
04-10 13:05:23.080   199  1572 D QCamera2HWI: int qcamera::QCamera2HardwareInterface::stopPreview(): X

Comment 9

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

Bhargav, Inder: is the snippet in comment 8 a bug in your CameraStateMachine?

Comment 10

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

Also, in comment 7, I assume we're expecting two returned photos? I'm guessing the first one is just a normal photo, and the second one is the HDR version?

Comment 11

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

Actually, looking through the code, I can see that the logcat error message "Cannot set preview window when preview is running" appears in three places, and is descriptively inaccurate for two of them, including procEvtPicTakingState().

Comment 12

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

So, the high-level question is: since we currently expect to be able to restart the preview after getting a picture, but HDR returns multiple pictures and while the process is taking place, we can't restart the preview, is there a way to explicitly determine how many pictures will be returned? Or can we just assume that with HDR ON, we will always get 2?

Comment 13

[Inder]

Mike -- let me grok the code and get back.

> I don't know what 'b2g-autogen_ephemeral_branch" is

Our build scripts has a way to apply any local patches we may have for gecko/gaia to try some changes and creates this temporary branch.

Comment 14

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

I'll 'take' this so that someone is working on it, but am waiting for info from CAF.

Comment 15

[bhargavg1]

(In reply to Mike Habicher [:mikeh] from comment #10)
> Also, in comment 7, I assume we're expecting two returned photos? I'm
> guessing the first one is just a normal photo, and the second one is the HDR
> version?

How many pictures are returned when HDR is ON is decided by key, "KEY_QC_HDR_NEED_1X" if set to TRUE would be 2 else 1

Comment 16

[Inder]

MikeH -- please let us know if you need anything else.

Comment 17

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

Bhargav, is the second photo the HDR one? Always? If so, we'll probably just discard the first one.

Comment 18

[Inder]

mikeh -- are you planning to hardcode the logic to always discard the 2nd image? I don't think that would be a good considering this param can be altered. Maybe you should check the param value first?

Comment 19

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

(In reply to Inder from comment #18)
>
> mikeh -- are you planning to hardcode the logic to always discard the 2nd
> image? I don't think that would be a good considering this param can be
> altered. Maybe you should check the param value first?

Hey Inder, I was planning to check CameraParameters::KEY_QC_HDR_NEED_1X, and if it's "true" (or whatever the 'true' value is), then I was just going to discard the first image result and only accept/return the second one.

Do you have any plans to extend this to cover more than one photo?

Comment 20

[bhargavg1]

(In reply to Mike Habicher [:mikeh] from comment #19)
> (In reply to Inder from comment #18)
> >
> > mikeh -- are you planning to hardcode the logic to always discard the 2nd
> > image? I don't think that would be a good considering this param can be
> > altered. Maybe you should check the param value first?
> 
> Hey Inder, I was planning to check CameraParameters::KEY_QC_HDR_NEED_1X, and
> if it's "true" (or whatever the 'true' value is), then I was just going to
> discard the first image result and only accept/return the second one.
> 
> Do you have any plans to extend this to cover more than one photo?

Or may be set it to false and only image returned would be HDR

Comment 21

[bhargavg1]

(In reply to bhargavg1 from comment #20)
> (In reply to Mike Habicher [:mikeh] from comment #19)
> > (In reply to Inder from comment #18)
> > >
> > > mikeh -- are you planning to hardcode the logic to always discard the 2nd
> > > image? I don't think that would be a good considering this param can be
> > > altered. Maybe you should check the param value first?
> > 
> > Hey Inder, I was planning to check CameraParameters::KEY_QC_HDR_NEED_1X, and
> > if it's "true" (or whatever the 'true' value is), then I was just going to
> > discard the first image result and only accept/return the second one.
> > 
> > Do you have any plans to extend this to cover more than one photo?
> 
> Or may be set it to false and only image returned would be HDR

Just to clarify, set CameraParameters::KEY_QC_HDR_NEED_1X to false and only one image that of HDR would be returned

Comment 22

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

(In reply to bhargavg1 from comment #21)
> 
> Just to clarify, set CameraParameters::KEY_QC_HDR_NEED_1X to false and only
> one image that of HDR would be returned

Thanks, that's much clearer.

Comment 23

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

Attachment: Disable normal photos in HDR scene mode, v1

Comment 24

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

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

Comment 25

[Dave Hylands [:dhylands]]

Comment on attachment 8407901 [details] [diff] [review]
Disable normal photos in HDR scene mode, v1

Review of attachment 8407901 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/camera/GonkCameraParameters.cpp
@@ +690,5 @@
>  }
>  
> +// Handle bools
> +nsresult
> +GonkCameraParameters::SetTranslated(uint32_t aKey, const bool& aValue)

Passing a const bool ref seems odd to me (unless you're using this as part of a template somewhere).

Why not just pass a bool ?

Comment 26

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

(In reply to Dave Hylands [:dhylands] (away - back Tue April 15) from comment #25)
>
> Passing a const bool ref seems odd to me (unless you're using this as part
> of a template somewhere).
> 
> Why not just pass a bool ?

Because the functions this method is called from and the functions it calls are all templates. :)

Comment 27

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

https://hg.mozilla.org/integration/b2g-inbound/rev/4953b30842cd

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4953b30842cd

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ea3724cf62ea

Comment 31

[Robert Mead [:RobertM]]

Test case needs to be updated.

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

Comment 32

[KTucker [:KTucker][Inactive 3/4/2016]]

Test case in moztrap:

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