[Camera][Flame] Volume overlay should not be displayed when pressing volume button

VERIFIED FIXED in Firefox OS master

Status

Firefox OS
Gaia::Camera
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: njpark, Assigned: wilsonpage)

Tracking

({verifyme})

unspecified
2.2 S7 (6mar)
ARM
Gonk (Firefox OS)
verifyme
Dependency tree / graph

Firefox Tracking Flags

(b2g-v2.2 affected, b2g-master verified)

Details

(Reporter)

Description

3 years ago
STR:
Start the phone, and on homescreen, use the volume key to set it to vibrate mode
Open camera app, use volume key to turn volume on, take picture

Expected:
shutter sound heard
Actual:
shutter sound is not heard

-Go back to homescreen, use the volume key to turn up the call volume
-Go back to camera app, use the volume key to mute the app sound, take picture

Expected:
shutter sound not heard
Actual:
shutter sound heard

In 2.2, this is not reproducible.  But in 2.2, it was noted that when volume keys are pressed, the icon showed that it is controlling the call volume (phone icon), not the app volume (speaker ico)

Version Info:
Gaia-Rev        a6881205deae450757a8d1e1ed65e5e5be0ec633
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/86d2bb8bb1c9
Build-ID        20150223010224
Version         38.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150223.042702
FW-Date         Mon Feb 23 04:27:13 EST 2015
Bootloader      L1TC000118D0
(Reporter)

Comment 1

3 years ago
[Blocking Requested - why for this release]:
Marked as regression, since the feature itself was working in 2.2, although the icon shown was incorrect.

Impact: can't control sound in camera app.
blocking-b2g: --- → 3.0?
Keywords: regression
(Reporter)

Updated

3 years ago
Keywords: regressionwindow-wanted
The Camera app uses the 'notification' channel:

https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/lib/sounds.js#L158

From the description in comment 0, the observed behaviour sounds correct.
(Reporter)

Updated

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Keywords: regressionwindow-wanted
Resolution: --- → FIXED
(Reporter)

Comment 3

3 years ago
mistakenly changed its status. changing back.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Updated

3 years ago
Status: REOPENED → NEW
(Reporter)

Comment 4

3 years ago
ni?ing tif and amylee to get opinions on this UX behavior.
Flags: needinfo?(tshakespeare)
Flags: needinfo?(amlee)
Omega is the one who has done all the sound channel work. Here's his spec from a different bug:
https://bug991026.bugzilla.mozilla.org/attachment.cgi?id=8420872

Looking at this, the notification sound and ringer are tied to the same volume setting. So increasing the volume in the camera app should change the notification volume - unless the volume key is dedicated to another function (e.g. iphone does continuous photo)

Based on the actual results, it seems like the volume control isn't changing volume but it isn't doing anything else either...?
Flags: needinfo?(tshakespeare)
(Reporter)

Comment 6

3 years ago
> Looking at this, the notification sound and ringer are tied to the same
> volume setting. So increasing the volume in the camera app should change the
> notification volume - unless the volume key is dedicated to another function
> (e.g. iphone does continuous photo)

This was the behavior in 2.2, but in master, instead of changing the notification volume, it changes 'app volume' which I don't think the camera app actually uses at all.
In light of the current information I have, we should revert to the previous behaviour so that things work again. 

However, I'm ni'ing Omega who is the UX designer working on sound channels/settings. He's currently out for Chinese New Year so give him a minute to reply.

Omega - two things, firstly did anything change in regards to the sound setting spec I referenced in comment 5? If not, we are going to revert the behaviour so that the camera volume is tied to the notification volume again.

Additionally, I want to double check that the notification setting should still be tied to the ringer and they are not separate settings. Thanks!
Flags: needinfo?(amlee) → needinfo?(ofeng)
ni? audio UX owner Jenny. Please help on this, thanks!
Flags: needinfo?(ofeng) → needinfo?(jelee)

Comment 9

3 years ago
Hi Tiffanie,

See Bug 1068219 attachment 8541546 [details] for the latest spec (v1.5). On p.8 adjusting volume with volume key - it is stated that using volume key = take photo in Camera, volume shouldn't be adjustable in Camera at all. 

And yes notification setting is tied to the ringer. Thanks!
Flags: needinfo?(jelee)
(Reporter)

Comment 10

3 years ago
Sorry, just checked that it does take photos when the volume up/down switch is pressed, didn't notice this before.

I guess the according to the spec, then perhaps we should *not* display the volume control overlay when the volume button is pressed, since it might be confusing to the user?
No, volume should not be displayed if the volume button is taking photos. 

Thanks Jenny for the updated spec!
(Reporter)

Comment 12

3 years ago
Updated the bug title- ni?ing mikeh FYI.
Flags: needinfo?(mhabicher)
Summary: Flame: sound control does not work in camera app → Flame: volume overlay should not be displayed when pressing volume button in camera app
(Reporter)

Updated

3 years ago
Keywords: regression
Let me also be clear that the volume also should not be changing. The volume key should not have any impact on volume. It should only be taking photos.
Flags: needinfo?(mhabicher)
Summary: Flame: volume overlay should not be displayed when pressing volume button in camera app → [Camera][Flame] Volume overlay should not be displayed when pressing volume button

Comment 14

3 years ago
Hi y'all. I'm the Product Manager for Media and PM for the UX team, who knew nothing about this. Tif is UX for Media, including the Camera app, and she knew nothing about this. :-) Just saying. 

This is not primarily audio issue (which I see Jenny was flagged for), but a camera functionality change, which means Tif is the UX lead here. 

One of these two behaviors at a time is fine: the button can either control volume OR take a photo. It seems like Wilson and Jenny would like the button to take a photo, which is fine, but then the button cannot ALSO be the volume control at the same time. That's a bug.

Expected behavior:
1. Launch camera app on device.
2. Press volume rocker.
Expected: Photo taken OR video recording started, as per Wilson, AND the manufacturer must be able to disable this.
Actual: Photo taken AND volume notification sound changes. 
 
I'm taking the liberty of blocking on this because this cannot ship as it is today, both because of the UX but also because the manufacturer needs to be able disable this. Also, it sounds like the icons are wrong in 2.2, so there are three distinct issues going on here. I don't know exactly where everything has landed, so I'm using the 2.2+ flag until someone tells me otherwise.

I apologize to everyone for the UX process fail here. If there had been a ui-review? set for Tif or myself we would have set it to ui-review-. We'll look for that next time around.
blocking-b2g: 3.0? → 2.2+

Comment 15

3 years ago
Wilson, please add a link to the patch in the attachments so we can find it and test on device. Thanks!
Flags: needinfo?(wilsonpage)
(Assignee)

Comment 16

3 years ago
Fixed by bug 1137013
Status: NEW → RESOLVED
Last Resolved: 3 years ago3 years ago
Flags: needinfo?(wilsonpage)
Resolution: --- → FIXED
(Assignee)

Comment 17

3 years ago
(In reply to Stephany Wilkes from comment #15)
> Wilson, please add a link to the patch in the attachments so we can find it
> and test on device. Thanks!

This feature is already on master.
(Assignee)

Comment 18

3 years ago
I should also mention that this isn't in v2.2 branch. Although it is a pretty excellent feature, so maybe it should be uplifted?
Created follow up bugs - these need to be fixed for the feature to land in 2.2.

Bug 1137917 - [camera] volume setting UI is shown and volume is changed when pressing the volume rocker
Bug 1137918 - [camera] capture interaction via volume key needs to be optional for manufacturers
Bug 1137924 - [camera] volume key in preview should return to viewfinder

Ni'ing Wilson just in case I forgot him on one of the bugs. Also, here is a mini-spec.
https://github.com/mozilla-b2g/gaia-specs/blob/master/%5B2.2%5D%20Camera%20Shutter.pdf
Flags: needinfo?(wilsonpage)
(Assignee)

Updated

3 years ago
Flags: needinfo?(wilsonpage)
Assignee: nobody → wilsonpage
status-b2g-v2.2: --- → affected
status-b2g-master: --- → fixed
Target Milestone: --- → 2.2 S7 (6mar)
This issue is verified fixed on the latest Nightly Flame 3.0 build.  Setting verifyme to check this issue if/when this gets to 2.2.

Actual Results: The volume buttons only take pictures now.

Environmental Variables:
Device: Flame 3.0 KK (Full Flash) (319 MB)
BuildID: 20150312010235
Gaia: 0c4e8b0b330757e261b031b7e7f326ef419c9808
Gecko: 5334d2bead3e
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 39.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
status-b2g-master: fixed → verified
Flags: needinfo?(ktucker)
Keywords: verifyme
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Depends on: 1137013
(Assignee)

Comment 21

3 years ago
I'm super confused. What is the desired behaviour?

Current volume button behaviour:

- 'master': Shows no volume overlay now as volume buttons now capture picture/video.
- 'v2.2': Shows a volume overlay and doesn't capture picture.
Flags: needinfo?(tshakespeare)
Please see spec in comment 19...

In viewfinder, volume keys should capture. In preview, volume keys should change media volume (which affects video playback).

Hope that helps!
Flags: needinfo?(tshakespeare)
(Assignee)

Comment 23

3 years ago
(In reply to Tiffanie Shakespeare from comment #22)
> Please see spec in comment 19...
> 
> In viewfinder, volume keys should capture. In preview, volume keys should
> change media volume (which affects video playback).
> 
> Hope that helps!

comment 19 doesn't include the patch (bug 1133965) which actually implemented the volume button capture. I'm assuming we'll want to uplift that to v2.2 too?
Flags: needinfo?(tshakespeare)
(Assignee)

Comment 24

3 years ago
For volume key capture feature to be included in v2.2 the following patches must be uplifted:

- bug 1133965
- bug 1137918
- bug 1137013
Depends on: 1137918, 1133965
Oops! Yep, we definitely want to include the actual patch too along with the updates. Thanks Wilson :)
Flags: needinfo?(tshakespeare)
(Assignee)

Updated

3 years ago
Depends on: 1144830
(Assignee)

Comment 26

3 years ago
I've created 4 v2.2 patches layered in the following order:

1. bug 1133965
2. bug 1137013
3. bug 1137918
4. bug 1144830

They must land in this order and each be rebased against mozilla/v2.2 after its parent patch lands.

Comment 27

3 years ago
Stephany/Tif,

Why do we need this in 2.2, I see that this is moved from 3.0 to 2.2 blocker. I don't know of any asks on this in 2.2 release and moreover this late in the cycle, we should not be adding new features. Seems like there is some confusion or mis-understanding in the trail of comments on this bug.

Sticking it back into the 2.2 nom queue until we confirm the need for this to be blocking 2.2 release. Stephany, if this is not a blocker please remove the nom for 2.2

Wilson,

If any related patches have landed, can we please back it out. It is too late to land new features in 2.2 branch. We are way past that phase. 

https://bugzilla.mozilla.org/show_bug.cgi?id=1135789#c26

Thanks
Hema
blocking-b2g: 2.2+ → 2.2?
Flags: needinfo?(wilsonpage)
Flags: needinfo?(tshakespeare)
Flags: needinfo?(swilkes)

Comment 28

3 years ago
Hema, see my comments here. This did not come from UX.
https://bugzilla.mozilla.org/show_bug.cgi?id=1137918
Flags: needinfo?(swilkes)

Comment 29

3 years ago
(In reply to Stephany Wilkes from comment #28)
> Hema, see my comments here. This did not come from UX.
> https://bugzilla.mozilla.org/show_bug.cgi?id=1137918

Thanks Stephany, it looks like the previous comments in the bug and moving bug to blocker caused some confusion. I am removing this from 2.2 nom since it is not a blocker for this release.
blocking-b2g: 2.2? → ---
Flags: needinfo?(tshakespeare)
(Assignee)

Comment 30

3 years ago
(In reply to Hema Koka [:hema] from comment #27)
> Wilson,
> 
> If any related patches have landed, can we please back it out. It is too
> late to land new features in 2.2 branch. We are way past that phase. 
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1135789#c26
> 
> Thanks
> Hema

Only one related patch got landed on v2.2 and was backed out yesterday.
Flags: needinfo?(wilsonpage)
You need to log in before you can comment on or make changes to this bug.