833280 - [Camera] Wrong Camera flash sequence

Status: RESOLVED FIXED

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

Description

[Leo]

1. Title : Wrong Camera flash sequence
2. Precondition : 
3. Tester's Action : Idle - camera - flash icon - touch
4. Detailed Symptom (ENG.) : Current order of camera flash: Flash off> Auto flash> Flash on> Auto Flash> Flash off.
5. Detailed Symptom (KOR.) : Flash off> Auto flash> Flash on> Auto Flash> Flash off 의 순서대로 변경됨 (On다음이 off가 아니라 Auto이다.)
6. Expected : Expected order for camera flash: Flash off> Auto flash> Flash on >  Flash off.
7.Reproducibility: Y
1)Frequency Rate : 100%
8.Comparison Results : 
1)Model Comparing : 
9. Attached files: Attach video
1)Log : 
2)Test Contents : 
3)Video file : 
10. Tel:

Comment 1

[Leo]

For Still Image capture only three flash modes should be used:
1. Flash Off, 2. Auto Flash, 3. Flash On

For camcorder mode, only two flash modes:
1. Flash Off, 2. Always Flash Off.

Changes were made and Will commit to master git.

Comment 2

[Dietrich Ayala (:dietrich)]

The easy workaround is to continue to press the icon, so not blocking. Tracking+ to accept the fix. Thanks in advance for your patch!

Comment 3

[Leo]

Attachment: Patch for Bug 833280: Fixed Camera flash sequence for both image and video captures

Comment 4

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

Comment on attachment 724311 [details] [diff] [review]
Patch for Bug 833280: Fixed Camera flash sequence for both image and video captures

djf, not sure if you want to look this over while daleharvey is OoO.

Comment 5

[David Flanagan [:djf]]

I can review the code, but I don't have a camera with a flash, so I can't test it.  We'll need someone else to give the final go ahead to land it.

Comment 6

[David Flanagan [:djf]]

Comment on attachment 724311 [details] [diff] [review]
Patch for Bug 833280: Fixed Camera flash sequence for both image and video captures

The existing code in the camera app for dealing with flash modes is (I think) from the days before we had any hardware to test it on. It is as generic as possible and just cycles through the list of modes that the hardware reports it supports.

It was a hack to begin with and this patch attempts to make it more intelligent by layering more hacks on top of the original hack. The patch seems brittle because it assumes that the hardware will always return the camera modes in the same order, so I'm not convinced that this will work correctly on all phones.

Mike: can we make the assumption that all phones that have a flash will support 'on', 'off', 'auto', and 'torch' modes?  Or do we have to check for 'torch' support separately from 'on' and 'auto'?

I'd suggest that we modify the app to maintain two separate flash modes for photos and videos. And even check for flash support separately for the two modes.
If the hardware supports 'on' and 'auto', then we display a flash button in picture mode.  If it supports 'torch', then we display a button in video mode.

I think we can get rid of the _flashModes[] array entirely. We just need to check capabilities.flashModes in enableCameraFeatures()

Comment 7

[David Flanagan [:djf]]

needinfo on Mike H for the question above

Comment 8

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

(In reply to David Flanagan [:djf] from comment #6)
> 
> Mike: can we make the assumption that all phones that have a flash will
> support 'on', 'off', 'auto', and 'torch' modes?  Or do we have to check for
> 'torch' support separately from 'on' and 'auto'?

djf: the driver can return pretty much anything it likes, even combinations that don't make sense; BUT in the past I've seen what you've described: either no flash support, or { on, off, auto, torch }.

I actually have a Nexus S handy.  It has a flash, so I can test any patches.

Comment 9

[Alex Keybl [:akeybl]]

leo+ and POVB since this should be fixed on the driver side

Comment 10

[jisaeng.kim]

Flash Device can support various mode, But why are you willing to make a limitation in driver side? I cannot understand. Application can get device information. It shouuld be fixed on the application side.
All other applications are working like that.

Comment 11

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

I attempted to test the above patch using a Nexus S, but am blocked on the following issues: bug 865243 and bug 865249.

Comment 12

[Ben Tian (inactive)]

Attachment: link to https://github.com/mozilla-b2g/gaia/pull/9401

David: I wrote a patch per your suggestion in comment 6. Can you review it?

Changes:
- separate flash types for picture and video
- fix the inconsistency of 'auto' flash icon for 'torch' mode by showing 'on' flash icon instead.

Comment 13

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

Comment on attachment 741745 [details] [diff] [review]
link to https://github.com/mozilla-b2g/gaia/pull/9401

I can confirm that on the Nexus S, while taking pictures, the flash button starts at 'on' then cycles through 'off' --> 'auto' --> 'on' --> 'off'.  (Maybe we want to start at auto, by default?)

I wasn't able to test 'off'/'torch' modes with video recording, since the switch to video mode seems to be broken on the Nexus S (in b2g18, anyway).

Comment 14

[David Flanagan [:djf]]

Comment on attachment 741745 [details] [diff] [review]
link to https://github.com/mozilla-b2g/gaia/pull/9401

I don't have hardware that I can test this on, so I'm assuming that you have tested carefully.

I agree with Mike that the default flash mode should probably be auto, not off.

Also, see github for some suggestions about ways to restructure the code to make it cleaner.

I'd like it if you are able to make some of those changes, but am giving r+ now, so if time is pressing (and if you've tested!) then you can land it as it is if you need to.

Comment 15

[Ben Tian (inactive)]

David & Mike, thanks for your suggestion. I made following changes and tested on leo & tara. Please let me know if you have further suggestion. I will merge it into master tomorrow.

https://github.com/mozilla-b2g/gaia/pull/9401
Changes:
- set default flash mode 'auto' for camera and 'off' for video
- use capture mode to index camera and video flash states
- move isSubset() into enableCameraFeatures() where it's used
- simplify toggleFlash()

Comment 16

[Ben Tian (inactive)]

master: https://github.com/mozilla-b2g/gaia/commit/758e5907b3c8e1a88bb2947311385bdd45ff21c5

Comment 17

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 758e5907b3c8e1a88bb2947311385bdd45ff21c5 to:
v1-train: 2643e7a5a41608f8424e73c1694196e74f6cb1d2