985012 - [Camera][Madai] Control buttons are disabled when we change from Video mode to camera mode and take a picture

Status: VERIFIED FIXED

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

Description

[Hema Koka [:hema]]

From Qualcomm testing on QRD 

STR:
1. Run Camera
2. Change to Video Mode
3. Change to Camera Mode and try to take a picture at the same time.  

Expected: The camera app is able to take a picture.
Actual: Focus UI is shown and all the control buttons are disable. 

Please try to reproduce with devices that you have (Nexus4, Buri, Helix)

Comment 1

[Diego Marcos [:dmarcos]]

We disable controls while the camera is loading. That's expected behavior.

The focus ring should not be displayed.

UX, Do we need additional feedback for the user to understand that the camera is loading and that's why you cannot take a picture? 

This is how you can update your camera:

git remote add mozilla https://github.com/mozilla-b2g/gaia
git fetch mozilla
git checkout mozilla/camera-new-features
make install-gaia APP=camera

Comment 2

[Hung Nguyen]

Flagging Amy for review for Diego's request.

Comment 3

[Tiffanie Shakespeare [:tif]]

Neither Amy nor I could see any lag on our Nexus 4 or Inari devices. How long is the lag until the user can use the shutter button to take a photo/video?

I would say generally, if the screen is black the user knows they can't use the shutter button. But if the user can see "stuff" in the viewfinder, they would fully expect to be able to take a photo or start recording.

Are all controls disabled or just the ability to capture?

Comment 4

[Amy Lee [:amylee] UX]

(In reply to Tiffanie Shakespeare from comment #3)
> Neither Amy nor I could see any lag on our Nexus 4 or Inari devices. How
> long is the lag until the user can use the shutter button to take a
> photo/video?
> 
> I would say generally, if the screen is black the user knows they can't use
> the shutter button. But if the user can see "stuff" in the viewfinder, they
> would fully expect to be able to take a photo or start recording.
> 
> Are all controls disabled or just the ability to capture?

I spoke to Diego and he says he can fix this issue.

Comment 5

[hyuna.cho]

Attachment: pull-request

Comment 6

[hyuna.cho]

Comment on attachment 8393952 [details]
pull-request

If we use the callback of fadeIn() to emit the event in ViewfinderController.prototype.updatePreview(),
the viewfinder updated the screen with blink.
Please check the patch

Comment 7

[David Flanagan [:djf]]

Comment on attachment 8393952 [details]
pull-request

It looks like Diego was reviewing this when Youngjun switched the review request to me.

I agree with the comment that Diego left on github, and have given r- because that need to be addressed.

More generally, I'm concerned that our mechanism for enabling and disabling things may not be full-featured enough for what we need. There are multiple reasons that we might want the controls to be disabled. The camera might not be ready, or the timer might be counting down. Or, as in this case, the viewfinder might not be ready. But all of these different reasons for disabling the controls rely on a single boolean.  We can call disable twice, but then a single call to enable is enough to turn the controls back on.

For now, it is probably good enough to get the events stuff correct and just fix the bug. But I worry that we're going to have problems in the future because we don't have a robust enable/disable mechanism that can deal with multiple conditions that can cause things to be disabled.

Comment 9

[Diego Marcos [:dmarcos]]

this is the last blocker pending for review

Comment 10

[David Flanagan [:djf]]

Comment on attachment 8393952 [details]
pull-request

The patch seems to work.

I don't understand why, though, so I'm going to try to trace everything that happens when I tap on the mode switch button:

1) tapping the button just calls this.app.settings.mode.next();  

2) The mode change in 1) triggers a 'change:mode' event. Three different things handle this unknown order:

2a) controllers/controls.js handles that event by setting something on the view. Presumably this is what makes the switch's visual state change.

2b) controllers/hud.js handles the event and calls its own updateFlash() method. (Not sure what that does and how it is synchronized with the calls to setFlash mode in step 3)

2c) controllers/camera handles the event and calls its own setMode() method.

3) The setMode() function in controllers/camera calls setFlashMode on the camera library, then calls the setMode() function of the camera library and then fades the viewfinder, and registers the camera library's configure() function 

4) This patch adds controls.disable() after the call to settings.mode.next() in 1). It probably runs at this point, while the viewfinder is fading out. (Would it be better to disable the controls before calling this.app.settings.mode.next() so that it runs before anything else happens?)

5) When the viewfinder is done fading, the configure method in lib/camera.js is called.  That calls setConfiguration() on the Gecko camera API and asynchronously, on success, emits a 'configured' event.

6) The camera controllers listens for the 'configured' event and broadcasts 'camera:configured' on the app.

7) controllers/viewfinder.js listens for camera:configured twice and calls loadStream() and updatePreview(), presumably in that order

8) The controllers/viewfinder.js loadStream() method called in step 7 calls lib/camera.js loadStreamInto() which synchronously emits 'streamLoaded' and 'ready'. streamLoaded is not used anywhere.  controllers/camera.js listens for 'ready' and broadcasts 'camera:ready' on the app.

9) controllers/controls.js handles 'camera:ready' and re-enables the controls.

10) the updatePreview() method called in step 7 calls the fadeIn method of the viewfinder view after 300ms.

11) The fadeIn() method shows the viewfinder. And, with this patch applied, it emits 'updatedPreview'

12) The 'updatedPreview' method is handled by controllers/viewfinder which broadcasts 'viewfinder:ready' on the app.

13) controllers/controls.js handles viewfinder:ready and re-enables the controls.  (even though they were already re-enabled in step 9)


Two things I take from this analysis:

1) As far as I can tell, without this patch the controls never actually get disabled when we switch modes.  Without the patch, they are disabled only on camera:busy or camera:loading.  camera:loading is never fired. And camera:busy does not seem to ever be fired in this case.  There is a onPreviewStateChange handler in lib/camera.js which emits 'busy' and 'ready', but it is never registered anywhere, so it is not emitting 'busy'.

2) The existing code already re-enables the controls, and this patch adds an extra re-enable.

I suspect, therefore, that the patch would work just by adding the call to controls.disable(), and it is not necessary to add the two new events to re-enable.  I would prefer a solution that did all enabling and disabling in really obvious pairs where it was clear that every disabled control would be re-enabled. But since we've got lots of things enabling and disabling all using a single state variable, its a mess anyway.  So in this case, I'd prefer a solution that just calls disable at the right time and does not include a superfluous re-enable call.

I also wonder if modifying the patch to call controls.disable() before calling settings.mode.next() would be more robust and would fix the related bug 986847

Comment 11

[David Flanagan [:djf]]

So I just tried fixing this with only the call to disable(), and it didn't work. The problem seems to be that in lib/camera.js loadStreamInto(), if the mozCamera object did not change, then we return early without sending a camera:ready event.

I would guess that we should fix that in lib/camera to ensure we always emit ready.

I'll attach a patch and ask Diego and/or Hyuna to review my approach to fixing this bug.  Unfortunately, my patch does not solve bug 986847, even though I disable the controls earlier.  I'm having github difficulties now, so I don't seem to be able to create a PR.  But the branch is here: https://github.com/davidflanagan/gaia/tree/bug985012

Comment 12

[David Flanagan [:djf]]

Attachment: a slightly simpler patch

Comment 13

[Diego Marcos [:dmarcos]]

I've just seen that there was an issue with merge/rebase. The line below was introduced by bug 974730 and reverted by the patch of bug 980599. 

mozCamera.onPreviewStateChange = this.onPreviewStateChange;

The callback emits the events busy or ready depending on the state of the preview. When the preview is on its way we're not allowed to take pictures. 

I made a new version of the patch that restores the lost line.

Comment 14

[Diego Marcos [:dmarcos]]

Attachment: Pull request with line lost in merge

Comment 15

[David Flanagan [:djf]]

Comment on attachment 8395298 [details] [review]
Pull request with line lost in merge

This is a simple fix that seems to get at the root of the bug.

Before landing, please check lib/camera.js loadStreamInto() (seem comment #11).  It is currently sending a ready event in some cases but not in all, and I'm not convinced that that is correct.  I suspect that hooking up the onPreviewStateChange handler means that we should never emit 'ready' from loadStreamInto(). Or maybe to be save it should be emitted in both cases.  Or, since the code is too confusing more me to figure out, it should at least have a comment explaining why the ready event is only emitted when the stream actually changes.

Comment 16

[Diego Marcos [:dmarcos]]

I removed emit 'ready' from loadStreamInto. It was something remaining from the time when we didn't have the onPreviewStateChange callback. When doing videoElement.mozSrcObject = this.mozCamera;
onPreviewStateChange is called twice, first when the previous preview stops and second when the new one is available. The events 'ready' and 'busy' are emitted from the callback and the controls are disabled and enabled as expected. Now it's clearer how the busy/ready cycle works when switching previews. I created bug 987213 requesting MDN documentation for the mozCameras API so everybody in the future can understand how this works.

Comment 17

[Diego Marcos [:dmarcos]]

Attachment: Pull Request on camera new features with ready event removed

Carrying on r+

Comment 18

[Diego Marcos [:dmarcos]]

Landed on camera-new-features:

https://github.com/mozilla-b2g/gaia/commit/29e180f9dd35c32dab153441b85e834c149be997

Comment 19

[Hema Koka [:hema]]

Qualcomm says this is not fixed when they tried verifying:

Here are the steps

    Go to camera app, and switch to camcorder

·         Start recording the video

·         Switch back to camera without stopping the recording.

·         Recording still continues and you are able to take picture as well.

Please test

Comment 20

[Youngjun Kim]

(In reply to Hema Koka [:hema] from comment #19)
> Qualcomm says this is not fixed when they tried verifying:
> 
> Here are the steps
> 
>     Go to camera app, and switch to camcorder
> 
> ·         Start recording the video
> 
> ·         Switch back to camera without stopping the recording.
> 
> ·         Recording still continues and you are able to take picture as well.
> 
> Please test

I think that Bug-986763 can cover it.

Comment 21

[Diego Marcos [:dmarcos]]

The problem refered in https://bugzilla.mozilla.org/show_bug.cgi?id=985012#c19 has been solved in bug 986763

Comment 22

[David Flanagan [:djf]]

Bulk edit for camera bugs.

If earlier comments do not show how this bug landed to master, it probably landed as part of https://github.com/mozilla-b2g/gaia/pull/17599 which merged the camera-new-features branch into master.

This bug was uplifted from master to v1.4 as part of https://github.com/mozilla-b2g/gaia/commit/a8190d08e61316a86bba572ba8d894d081a20530

Comment 23

[sarsenyev]

The bug is 
As per comment 21, switch mode is disabled when recording.

1.4 Environmental Variables:
Device: Buri 1.4 MOZ
BuildID: 20140410000201
Gaia: 9b2da43dfee3792cd311ae55f0b06272313208f0
Gecko: 9d9ead7d6afa
Version: 30.0a2
Firmware Version: v1.2-device.cfg

1.5 Environmental Variables:
Device: Buri 1.5 MOZ
BuildID: 20140410040201
Gaia: 9d0b1bdf746823a94b13e6574c1d8304dc584763
Gecko: 690c810c8e3e
Version: 31.0a1
Firmware Version: v1.2-device.cfg