1154665 - [Camera] flash button cannot disable/enable flashing

Status: RESOLVED FIXED

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

Description

[Hermes Cheng (inactive after July 27, 2015)]

* STR:
1. use "Flash On" to take a picture
2. tap flash button to "Flash Off"
3. take a picture

* Expected result:
flash is not used

* Actual result:
flash is still used at "Flash Off" mode

Comment 1

[Hermes Cheng (inactive after July 27, 2015)]

*Test env:

Build ID               20150414162502
Gaia Revision          16e948bfaaa15dbc0200135d52f16257b4eab193
Gaia Date              2015-04-14 21:08:25
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/0eec28e78eb1
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150414.201606
Firmware Date          Tue Apr 14 20:16:21 EDT 2015
Bootloader             HHZ12f

Comment 2

[Hema Koka [:hema]]

Blocking: Flash Off is not functioning on L based nexus5 (assuming this is working fine on Flame with KK)

Andrew,

Can you borrow nexus5 from MikeH and check this out?

thanks
Hema

Comment 3

[Hermes Cheng (inactive after July 27, 2015)]

(In reply to Hema Koka [:hema] from comment #2)
> Blocking: Flash Off is not functioning on L based nexus5 (assuming this is
> working fine on Flame with KK)
> 
> Andrew,
> 
> Can you borrow nexus5 from MikeH and check this out?
> 
> thanks
> Hema

If I use "Flash Off" mode to take some photos and then change to "Flash On", "Flash On" is not functioning then.

It seems sometimes the first config cannot be changed.

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Hermes Cheng[:hermescheng] from comment #3)
> (In reply to Hema Koka [:hema] from comment #2)
> > Blocking: Flash Off is not functioning on L based nexus5 (assuming this is
> > working fine on Flame with KK)
> > 
> > Andrew,
> > 
> > Can you borrow nexus5 from MikeH and check this out?
> > 
> > thanks
> > Hema
> 
> If I use "Flash Off" mode to take some photos and then change to "Flash On",
> "Flash On" is not functioning then.
> 
> It seems sometimes the first config cannot be changed.

I tried on master and 2.2. The flash fires for me when I turn on flash, and doesn't fire when I turn it off. Switching it on and off repeatedly is fine.

The only oddity I've noticed is how the screen flashes white when it takes the picture. This is not caused by the flash (although the preview does brighten due to the flash if enabled), rather this is an artifact of from camera preview data from the driver.

Using LMY47D factory image, the stock Android camera app has the same behaviour as above.

Is the flash actually turning on when it should not or does it just look like it is because of the preview flashing white?

Comment 5

[Keven Kuo [:kkuo]]

Hi! Becker,

Please help to take a look. Thanks

--
Keven

Comment 6

[Hermes Cheng (inactive after July 27, 2015)]

@Andrew,

No, it is not the screen flashing. Just like my comment 3, this issue exists either way.

I have retired several times, but the reproduce rate is really low (10%?, I think). Still try to figure out the STR pattern.

Comment 7

[Hermes Cheng (inactive after July 27, 2015)]

Attachment: camera.log

Collect some log with enableing gaia debug log.

* STR of this log
1. Camera with Flash on
2. restart the phone
3. open Camera
4. take a photo with Flash On
5. switch to Flash Off
6. take a photo with Flash off

At step 6, the flash is still working

Comment 8

[Andrew Osmond [:aosmond] (he/him)]

@Hermes,

Could you load the Android 5.1 image and make sure flash works fine with their application so we can rule out any hardware / base image defects?

https://intranet.mozilla.org/QA/B2G_Tips_and_Tricks#Android_5.1

I've tried your STR in comment 7 but I've had no luck in reproducing (yet).

Comment 9

[Hermes Cheng (inactive after July 27, 2015)]

(In reply to Andrew Osmond [:aosmond] from comment #8)
> @Hermes,
> 
> Could you load the Android 5.1 image and make sure flash works fine with
> their application so we can rule out any hardware / base image defects?
> 
> https://intranet.mozilla.org/QA/B2G_Tips_and_Tricks#Android_5.1
> 
> I've tried your STR in comment 7 but I've had no luck in reproducing (yet).

My N5 is already 5.1 (comment 1). I tried another N5 and also got this issue.
Sometimes, I would take more than 1 photo with Flash on and then switch to Flash off.

Comment 10

[Hayden Huang [:haydenh]]

@Hermes,

I couldn't reproduce this issue on my N5 as well. Per Andrew's comment, could you try the Android 5.1 LMY47D factory image here <https://developers.google.com/android/nexus/images> and see if flash is working normally on Android?

Comment 11

[Hermes Cheng (inactive after July 27, 2015)]

(In reply to Hayden Huang from comment #10)
> @Hermes,
> 
> I couldn't reproduce this issue on my N5 as well. Per Andrew's comment,
> could you try the Android 5.1 LMY47D factory image here
> <https://developers.google.com/android/nexus/images> and see if flash is
> working normally on Android?

@Hayden,
No, I can't reproduce this on Android 5.1.

Comment 12

[Hayden Huang [:haydenh]]

Hermes found that this bug can be produced if Wifi is enabled. 

From the log, flash mode is not set correctly because setParameters fail in camera service when it is processing GPS information.

E/Camera2-Parameters(  190): set: Incomplete set of GPS parameters provided

Since we did not provide value to the KEY_GPS_PROCESSING_METHOD, setParameters call will just return without setting the rest config to the HAL. After setting KEY_GPS_PROCESSING_METHOD like below, this bug is fixed.

--- a/dom/camera/GonkCameraParameters.cpp
+++ b/dom/camera/GonkCameraParameters.cpp
@@ -558,6 +558,7 @@ GonkCameraParameters::SetTranslated(uint32_t aKey, const ICameraControl::Positio
     ClearImpl(Parameters::KEY_GPS_TIMESTAMP);
   }
 
+  SetImpl(Parameters::KEY_GPS_PROCESSING_METHOD, "unknown");
   return NS_OK;
 }

@Andrew, KEY_GPS_PROCESSING_METHOD could be either GPS" or "NETWORK". Since geo location information is passed by application layer, do you know who might be familiar with geo location?

Hayden

Comment 13

[Keven Kuo [:kkuo]]

Hi! Hayden,

Good finding.

--
Keven

Comment 14

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Hayden Huang from comment #12)
> Hermes found that this bug can be produced if Wifi is enabled. 
> 
> From the log, flash mode is not set correctly because setParameters fail in
> camera service when it is processing GPS information.
> 
> E/Camera2-Parameters(  190): set: Incomplete set of GPS parameters provided
> 
> Since we did not provide value to the KEY_GPS_PROCESSING_METHOD,
> setParameters call will just return without setting the rest config to the
> HAL. After setting KEY_GPS_PROCESSING_METHOD like below, this bug is fixed.
> 
> --- a/dom/camera/GonkCameraParameters.cpp
> +++ b/dom/camera/GonkCameraParameters.cpp
> @@ -558,6 +558,7 @@ GonkCameraParameters::SetTranslated(uint32_t aKey, const
> ICameraControl::Positio
>      ClearImpl(Parameters::KEY_GPS_TIMESTAMP);
>    }
>  
> +  SetImpl(Parameters::KEY_GPS_PROCESSING_METHOD, "unknown");
>    return NS_OK;
>  }
> 
> @Andrew, KEY_GPS_PROCESSING_METHOD could be either GPS" or "NETWORK". Since
> geo location information is passed by application layer, do you know who
> might be familiar with geo location?
> 
> Hayden

Oh my, add GPS to the list of things to consider when testing the camera... Very nice catch indeed ;). I have WiFi enabled but I'm not sure if it is providing any GPS parameters. In any event, I can force it.

The GPS method is not currently exposed by the geolocation service (selected at the Gonk integration layer). It uses "GPS", "NETWORK" or "HYBRID" modes. Specifying "UNKNOWN" as a workaround for now is fine until/if we get the plumbing in place. The value should be opaque to the driver and it just needs it for the EXIF header (as far as I know).

Comment 15

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

Great find, Hayden. I hope Camera3 does away with CameraParameters -- it's a terrible API.

Comment 16

[Hermes Cheng (inactive after July 27, 2015)]

I am curious that is this a nexus 5 only issue?

Comment 17

[Hayden Huang [:haydenh]]

Attachment: bg1154665.patch

I can reproduce this issue on both Flame and Nexus 5, it should be a common issue among all FxOS phone. Since gps processing method is not exposed by geo location service, we will set it to "UNKNOWN".

Comment 18

[Hermes Cheng (inactive after July 27, 2015)]

Based on comment 17, remove Nexus 5 only keyword.
@Eric & No-jun, just let you know this bug.

Comment 19

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8594606 [details] [diff] [review]
bg1154665.patch

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

Before I r+ this, does this need to be set when no other GPS parameters are set? My expectation would be that if we clear all of the other GPS parameters, we should clear the GPS processing method parameter as well. But if the driver doesn't let us do that, then I will accept this as implemented.

Comment 20

[David Flanagan [:djf]]

If you want to avoid using "unknown" here, you could look at the accuracy value that we get from the geolocation api and guess what method is used based on that. If the accuracy is 10m or less, assume GPS and otherwise say "unknown" maybe?  That would give better EXIF data. But I don't feel strongly about this at all.

Comment 21

[Hayden Huang [:haydenh]]

Attachment: bg1154665.patch, v2

In fact, the GPS parameters in Android camera service is either "all of them" or "none of them". So after a second thought, I modified the code a bit to fulfil this requirement. If any one of the parameter (lat,lng,alt,timestamp) is null, then it will clear all of the parameters.

Other than this, maybe we should expose a processing method field for camera app to fill. then application can decide if they want to guess the processing method based on the accuracy or not. However, I don't feel the strong needs of this, so I would suggest to phase in this fix first.

Comment 22

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8595240 [details] [diff] [review]
bg1154665.patch, v2

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

Looks good although I have some feedback :).

(In reply to David Flanagan [:djf] from comment #20)
> If you want to avoid using "unknown" here, you could look at the accuracy
> value that we get from the geolocation api and guess what method is used
> based on that. If the accuracy is 10m or less, assume GPS and otherwise say
> "unknown" maybe?  That would give better EXIF data. But I don't feel
> strongly about this at all.

I looked at the geolocation code, it shouldn't be too hard to expose this as part of that API. But I don't feel too strongly on this, given the data is opaque, I am happy as it is; the standard does not specify values, I just made suggestions based on the Android impl. File a new bug on this and add it to camera-backlog.

(In reply to Hayden Huang [:haydenh] from comment #21)
> Created attachment 8595240 [details] [diff] [review]
> bg1154665.patch, v2
> 
> In fact, the GPS parameters in Android camera service is either "all of
> them" or "none of them". So after a second thought, I modified the code a
> bit to fulfil this requirement. If any one of the parameter
> (lat,lng,alt,timestamp) is null, then it will clear all of the parameters.
> 
> Other than this, maybe we should expose a processing method field for camera
> app to fill. then application can decide if they want to guess the
> processing method based on the accuracy or not. However, I don't feel the
> strong needs of this, so I would suggest to phase in this fix first.

Please add a test case added to dom/camera/test/test_camera_take_picture.html. You can combine all of the GPS tests into one. The existing test cases should demonstrate how to use them, test_camera_fake_parameters.html has a lot more exercising the capture/verification of camera parameters.

Intercepting the takePicture call at the GonkCameraHardware level should be sufficient, something like:

function takePictureGps(p) {
  suite.hw.attach({
    takePicture: function() {
      ... verify suite.hw.params['gps-latitude'], etc are set/cleared as expected ...
    }
  });

  return suite.camera.takePicture({
  });
}

return suite.getCamera()
  .then(takePictureGps)
  .then(takePictureNoGps)
  .then(takePicturePartialGps)

::: dom/camera/GonkCameraParameters.cpp
@@ +634,3 @@
>      DOM_CAMERA_LOGI("setting picture latitude to %lf\n", aPosition.latitude);
>      SetImpl(CameraParameters::KEY_GPS_LATITUDE, nsPrintfCString("%lf", aPosition.latitude).get());
> +    DOM_CAMERA_LOGI("setting picture longitude to %lf\n", aPosition.longitude);

Can you combine all of these into one print and save some log lines? (i.e. use coordinate system like (latitude, longitude, altitude, timestamp) -> setting picture gps coordinates (%lf, %lf, %lf, %lf))

@@ +634,4 @@
>      DOM_CAMERA_LOGI("setting picture latitude to %lf\n", aPosition.latitude);
>      SetImpl(CameraParameters::KEY_GPS_LATITUDE, nsPrintfCString("%lf", aPosition.latitude).get());
> +    DOM_CAMERA_LOGI("setting picture longitude to %lf\n", aPosition.longitude);
> +    SetImpl(CameraParameters::KEY_GPS_LONGITUDE, nsPrintfCString("%lf", aPosition.longitude).get());

I realize it was already done this way, but can you clean this up by adding a SetImpl(const char *, double) in GonkCameraParameters.h? It should be identical to SetImpl(const char *, float) as nsCString::AppendFloat should have an overloaded version for doubles.

Comment 23

[Hayden Huang [:haydenh]]

Attachment: bg1154665.patch -- master

Since SetImpl(const char *, double) is already in GonkCameraParameters.h, I call SetImpl(const char *, double) instead per Andrew's comment.

Comment 24

[Hayden Huang [:haydenh]]

Attachment: bg1154665_testcase.patch

This is the patch for the test case. I am trying to set up mochitest on my platform but is facing some difficulty, so this has not been verified ( I will continue working on this tomorrow). Attached here so that if anyone has any comment on the test case, please let me know.

Comment 25

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8595915 [details] [diff] [review]
bg1154665.patch -- master

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

LGTM!

Comment 26

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8595916 [details] [diff] [review]
bg1154665_testcase.patch

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

Not sure if you've seen the CameraControl WebIDL before, it will provide API usage details which will put my comments in context:

https://dxr.mozilla.org/mozilla-central/source/dom/webidl/CameraControl.webidl

::: dom/camera/test/test_camera_take_picture.html
@@ +169,5 @@
> +        ok(suite.hw.params['gps-altitude'] == 10, "gps-altitude = " + suite.hw.params['gps-altitude']);
> +        ok(suite.hw.params['gps-timestamp'] == 1429699895, "gps-timestamp = " + suite.hw.params['gps-timestamp']);
> +      }
> +    });
> +    return suite.camera.takePicture();

takePicture is where you should supply the location information. E.g.

return suite.camera.takePicture({position: {latitude: 32.920650, longitude: -117.136894, altitude: 10, timestamp: 1429699895}});

@@ +175,5 @@
> +
> +  function takePictureNoGps(p) {
> +    suite.hw.attach({
> +      takePicture: function() {
> +        var cam = suite.camera;

Don't need cam/suite.camera as you don't use them.

@@ +179,5 @@
> +        var cam = suite.camera;
> +        ok(suite.hw.params['gps-latitude'] == null, "gps-latitude = null");
> +        ok(suite.hw.params['gps-longitude'] == null, "gps-longitude = null");
> +        ok(suite.hw.params['gps-altitude'] == null "gps-altitude = null");
> +        ok(suite.hw.params['gps-timestamp'] == null, "gps-timestamp = null");

This might pass, but I think the following would more appropriate:

ok(!('gps-latitude' in suite.hw.params), "gps-latitude not set");

Using GonkCameraParameters::ClearImpl should cause the parameter to be removed from the object/map entirely so if it is set at all, it should fail.

@@ +190,5 @@
> +    suite.hw.attach({
> +      takePicture: function() {
> +        var cam = suite.camera;
> +        cam.setLocation(
> +          {latitude: 32.920650, longitude: null, altitude: 10, timestamp: 1429699895}

I think this will fail because null will get evaluated to 0 rather than NaN. You should leave longitude unspecified or set it to NaN.

Comment 27

[David Flanagan [:djf]]

Hayden: would you update this bug with your status? As a 2.2+ bug, we must land it by Wednesday the 29th. Are you still working on updating the test based on Andrew's comments above?

Andrew: will you be able to take over, finish the test, and get the patch landed if Hayden is not able to?

Comment 28

[Hayden Huang [:haydenh]]

Attachment: bg1154665_testcase_v2.patch -- master

sorry I was on annual leaves for past couple of days.

Attached the new testcase patch.

Comment 29

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8598558 [details] [diff] [review]
bg1154665_testcase_v2.patch -- master

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

LGTM. I will land this on try/b2g-inbound today.

Comment 30

[Andrew Osmond [:aosmond] (he/him)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e71803354a2c

Comment 31

[Andrew Osmond [:aosmond] (he/him)]

Attachment: bg1154665.patch -- v2.2

Needed changes to land on 2.2. Try is closed for the moment.

Comment 32

[Andrew Osmond [:aosmond] (he/him)]

try on b2g 2.2: https://treeherder.mozilla.org/#/jobs?repo=try&revision=140fdcb18fce

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/1cfae48390c9
https://hg.mozilla.org/integration/b2g-inbound/rev/6cab63690075

Comment 34

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8598643 [details] [diff] [review]
bg1154665.patch -- v2.2

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: Flash may fire inconsistently depending on the ability of the camera to localize and whether or not the user requested photos be tagged by location.
Testing completed: Verified flash consistently fires regardless of whether we provide no localization info, partial or complete. Added regression test.
Risk to taking this patch (and alternatives if risky): Low risk, changes limited to a single platform function so well understood. Logic choosing to tag an image or not has been made more restrictive so worst case we don't tag the geolocation when we should.
String or UUID changes made by this patch: None.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/e0aca65a472a

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1cfae48390c9
https://hg.mozilla.org/mozilla-central/rev/6cab63690075

Comment 37

[Hermes Cheng (inactive after July 27, 2015)]

verified with below test env

*test env
**2.2
Build ID               20150503162504
Gaia Revision          8d14361337e608c8cdf165ea5034db5eda23b618
Gaia Date              2015-05-01 18:23:46
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/cb7cb6597c91
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150503.195932
Firmware Date          Sun May  3 19:59:50 EDT 2015
Bootloader             HHZ12f

**master
Build ID               20150503160200
Gaia Revision          e18cce173840d6ff07fb6f1f0e0ffb58b99aab3e
Gaia Date              2015-05-02 04:27:01
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/dc5f85980a82
Gecko Version          40.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150503.193743
Firmware Date          Sun May  3 19:38:00 EDT 2015
Bootloader             HHZ12f