896874 - [B2G][Helix][Camera]There is no tone when start or stop a video recording

Status: RESOLVED FIXED

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

Description

[DingYu]

User Agent: Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.1; Trident/4.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; .NET4.0C; .NET4.0E; Tablet PC 2.0)

Steps to reproduce:

1.Open the camer app
2.start and stop a video recording


Actual results:

2.there is no tone when start or stop a video recording


Expected results:

2.It should  have a tone when start or stop a video recording

Comment 1

[Wayne Chang [:wchang]]

Hi Sri,
This may be required by law in some regions. Have we got this planned or backlogged somewhere?

Hi Dale, 
Can you estimate the LOE for this? to have audible indication on start and stop of video recording?

Comment 2

[Dale Harvey (:daleharvey)]

I havent seen it in any backlog, it will likely require a new setting for locations where it isnt a legal requirement but I would expect it to be a fairly small amount of work given the assets

Comment 3

[Wayne Chang [:wchang]]

Thanks Dale. Let me flag product here for some input.

(In reply to Dale Harvey (:daleharvey) from comment #2)
> I havent seen it in any backlog, it will likely require a new setting for
> locations where it isnt a legal requirement but I would expect it to be a
> fairly small amount of work given the assets

Comment 4

[Sri Kasetti]

DingYu, Wayne, I just tested this feature on the latest V1.1 build.
When you touch the video icon to start the recording, you can hear a tone and then the icon turns red.When you stop the recording, you can hear the same tone. Am I missing something here?

Comment 5

[Sotaro Ikeda [:sotaro]]

FYI, CameraService's recoring start sound is disabled in Bug 858469.

Comment 6

[DingYu]

Sri Kasetti, It's just I want. By the way, I have a question, In setting-->sound-->Camera shutter, the option can enable and disable the camera shutter tone, The video tone is controled by this option too?

Comment 7

[Wayne Chang [:wchang]]

Sri, it seems that it'd been removed, and also on other devices there is no sound. What build are you running?

Adding William here who confirmed the sound missing.

Sotaro, 858469 seemed to have removed the sound to eliminate some unwanted behaviour. Can that be fixed correctly so that we can add sound indication if required?

Comment 8

[Sotaro Ikeda [:sotaro]]

(In reply to DingYu from comment #6)
> Sri Kasetti, It's just I want. By the way, I have a question, In
> setting-->sound-->Camera shutter, the option can enable and disable the
> camera shutter tone, The video tone is controled by this option too?

Current Firefox OS does not implement the video recording tone.

Comment 9

[Sotaro Ikeda [:sotaro]]

(In reply to Wayne Chang [:wchang] from comment #7)
> 
> Sotaro, 858469 seemed to have removed the sound to eliminate some unwanted
> behaviour. Can that be fixed correctly so that we can add sound indication
> if required?

The bug just remove video recording tone file from ROM. It works when the build is same as AOSP. If partner built ROM modified it, the fix does not work. In fact, Leo changed how the build script extracts the recording tone. And fixed it on leo side by different ways.

Comment 10

[Wayne Chang [:wchang]]

Hema, would you triage this for backlogging as well?

Comment 11

[Hema Koka [:hema]]

Will add this to the backlog list

Thanks
Hema

Comment 12

[Sri Kasetti]

Responding to comment 7 from Wayne. Looks like my build was an earlier one.
Satoro updated my build. So I don't get the tone now.
As Hema said we will add this to our backlog.

Comment 13

[Stephany Wilkes]

Just flagging UX in case we want to give input here. If it's fine as-is, that's OK.

Comment 14

[Preeti Raghunath(:Preeti)]

moving to media team for 1.2 backlog. Made a koi?

Comment 15

[Dietrich Ayala (:dietrich)]

Not a regression, and no partner/certification requirements around this yet so not blocking yet.

Once it's clear which markets legally require it, re-nominate.

Comment 16

[Hema Koka [:hema]]

As discussed in the sprint planning, we will be taking this on. 

John, assigning this to you

Comment 17

[John Hu [:johnhu][:johu][:醬糊小弟]]

Yes, no problem. I can start to work on this.

Comment 18

[John Hu [:johnhu][:johu][:醬糊小弟]]

Because waiting for the suggested sound effect, I will discuss this one with system platform team to add the support of recording tone as a build-time configuration. So, the OEM can customize it and we can also use the same behavior to add the default sound effect.

Comment 19

[John Hu [:johnhu][:johu][:醬糊小弟]]

I found the enablement of shutter sound of camera was already in build-time customization list. I will use the same setting for the enablement of the recording sound and add an additional filename for recording sound which is not customizable.

Because we are waiting for sound effect from UX, I will copy the original camera shutter sound as the recording sound. And open a bug for the replacement of sound effect, if the code is landed before sound effect ready.

Comment 20

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: extract sound effect object and add shutter sound to start/stop recording

1. extract the code about shutter sounds to SoundEffect object.
2. add shutter sound effect to the place starting/stoping recording.
3. copy shutter.ogg as camcorder.ogg. I will open a follow-up bug for sound effect replacement, if this patch is landed before UX gives us sound effect.

Comment 21

[John Hu [:johnhu][:johu][:醬糊小弟]]

Sri, 

How do you think about the customization of sound effect resources?? I means OEM can change the sound effects, shutter.ogg and camcorder.ogg, at the build-time.

Comment 22

[David Flanagan [:djf]]

Comment on attachment 798772 [details]
extract sound effect object and add shutter sound to start/stop recording

John,

I have a number of comments on github. My primary concerns with this patch are:

1) I think you may be playing the sounds too late.

2) It would be more efficient in soundeffect.js to not create the Audio elements if the sounds are turned off in settings.

3) I'd guess that UX will want different sounds for "start recording" and "stop recording" so you may have to modify your code for that.

Comment 23

[Sri Kasetti]

(In reply to John Hu [:johnhu] from comment #21)
> Sri, 
> 
> How do you think about the customization of sound effect resources?? I means
> OEM can change the sound effects, shutter.ogg and camcorder.ogg, at the
> build-time.

We don't need to make this build time customizable

Comment 24

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 798772 [details]
extract sound effect object and add shutter sound to start/stop recording

The followings are changed in this patch:
1. move the sound playing before recording start and right after recording end.
2. create the audio element when needed and use cloneNode to create a cloned one for playing rapidly.
3. add recording start and recording end API to have different start and end sound resources. 
4. rename the variables.

Comment 25

[David Flanagan [:djf]]

Comment on attachment 798772 [details]
extract sound effect object and add shutter sound to start/stop recording

John,

You've addressed all my code concerns. Thank you!  I haven't had time to try the patch myself, but if you have tested it, the go ahead and land.

Please do ensure that none of the recording end sound is captured in the recorded video.  And check that there isn't a very noticeable delay for the very first picture while the shutter sound is loaded.

Comment 26

[Rob MacDonald [:robmac]]

Hi John...

Thanks for flagging me on this. Would it be possible for you to package the attachment as per James Burke's instructions? 

https://github.com/jrburke/gaia-dev-zip#for-the-developer 

This will allow UX and PM to review the attachment. 

Otherwise, David is correct in his assumption that we would want a different sound at the start and the end of the recording. It's really important to differentiate between the two.

Comment 27

[John Hu [:johnhu][:johu][:醬糊小弟]]

(In reply to Rob MacDonald [:robmac] from comment #26)
> Hi John...
> 
> Thanks for flagging me on this. Would it be possible for you to package the
> attachment as per James Burke's instructions? 
> 
> https://github.com/jrburke/gaia-dev-zip#for-the-developer 

I will do it right away. But this bug still need the sound resources. I thought the need info flag is to request those sound resources. That's why I didn't put ui-review on you. But I will do it now.


(In reply to David Flanagan [:djf] from comment #25)
> Comment on attachment 798772 [details]
> Please do ensure that none of the recording end sound is captured in the
> recorded video.  And check that there isn't a very noticeable delay for the
> very first picture while the shutter sound is loaded.

Hi David, 

I had confirmed that before reseting the review. The start and end sound didn't recorded into video. And as to the delay, I can't notice it with my low-end device.

Comment 28

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: bug896874-recording-tone.zip

Hi Rob and Sri,

The attachment is the patched version. And I just use the camera shutter sound should to be the recording start and recording end sound.

Thanks...

Comment 29

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: bug896874-recording-sound-2.zip

use the notify_bap for recording start and notify_bop for recording stop.

Comment 30

[John Hu [:johnhu][:johu][:醬糊小弟]]

According to the email discussion with Patryk, I will add a setting item into Settings app, under Sound->Other sounds->Recording Sound (new one).

Comment 31

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 798772 [details]
extract sound effect object and add shutter sound to start/stop recording

The new patch only add a new setting item and use it to determine if recording sound is played or not, about 90% of camera code remaining the same. So, I didn't reset the review+ flag of David.

I add another review flag to Arthur for reviewing the new setting item to settings app.

Comment 32

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: video for reviewing

Rob, 

To help the reviewing, I make a video of the patch. I already added a new setting item to settings app. And the tool of UX-review can't pack two apps. So, I make this video to show the result of changing settings.

The attachment 801341 [details] is the version that always enabled recording sound. So, you can test it without the update of settings app and the build.

Comment 33

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 798772 [details]
extract sound effect object and add shutter sound to start/stop recording

Simple change. r=me.

Comment 34

[Rob MacDonald [:robmac]]

Comment on attachment 801341 [details]
bug896874-recording-sound-2.zip

Minused based on wording of "Settings". Please change "Video recording sound" to "Video recording" to make it consistent with the other settings. 

Otherwise everything seems fine from an interaction perspective. I initially felt there was a bit of lag at the start of the recording but this is a separate issue from this bug.

Also I will flag Patryk for review of the audio file and Carrie for a review of the Settings. 

Thanks also for putting together the zip file and video - extremely helpful! :)

Comment 35

[Patryk Adamczyk [:patryk] UX]

The audio files sound correct to me.

Comment 36

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: updated settings UI

Comment 37

[John Hu [:johnhu][:johu][:醬糊小弟]]

Rob, Patryk, and Carrie, 

Thanks for your information. I had changed the patch to have "Video recording". Please see the attachment 802736 [details].

As to the following question, I also think this is another issue, but I can explain something here.

> Otherwise everything seems fine from an interaction perspective. I initially
> felt there was a bit of lag at the start of the recording but this is a
> separate issue from this bug.

The root of this lag is that we need to do: 1. create a filename according to design rule of camera file system, 2. create a dummy file for writing test, and 3. check the free space for it. The total consumes about 300ms. That may be improved. I had tested different places to play the sound. Like, I can play it after user pressed the button, before video recording and after video recording. But I prefer to play it right before video recording because it reflects the real status.

You may notice another lag of the enablement of capture button, that's on purpose. We met the bug 899864 before. Finally, we found the recorded video files lack audio/video samples because user press capture button too fast. So, we delayed the enablement of capture button when recording. Android plays the same trick, too.

Comment 38

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 801341 [details]
bug896874-recording-sound-2.zip

I had changed the wording of settings item. So, I put the ui-review? back.

Comment 39

[Carrie Wang [New account: [e-mail]]]

I'm fine with the new string. Thanks.

Comment 40

[Rob MacDonald [:robmac]]

Comment on attachment 801341 [details]
bug896874-recording-sound-2.zip

(In reply to John Hu [:johnhu] from comment #37)
> Rob, Patryk, and Carrie, 
> 
> Thanks for your information. I had changed the patch to have "Video
> recording". Please see the attachment 802736 [details].

Hi John...

So close! :) The zip file looks great. However, in the screenshot (attachment 802736 [details]) the string "Camera recording" needs to be changed to "Video recording" as per your comment above. 

No need to add me for another review if you modify the string. And thanks also for the useful background information.

Best regards,
Rob

Comment 41

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: updated settings UI - 2

Oh, I am so sorry about this. I just forgot to replace the "Camera" as "Video". I just removed the word "sound".

This attachment has the correct words.

Comment 42

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 801341 [details]
bug896874-recording-sound-2.zip

As per rob's comment, comment 40, I removed the r- flag from this file.

Comment 43

[John Hu [:johnhu][:johu][:醬糊小弟]]

mreged to master:

https://github.com/mozilla-b2g/gaia/commit/d2f2ea5e3f402cba11ace91c35449be62d4ce53c

Comment 44

[lecky]

Hi Wayne :
 Please help us to push to merge this case into V1.1 HD. It blocked the Helix Device .If you can not resolve it , please get the authorization from carrier.

Thanks.

Comment 45

[Wayne Chang [:wchang]]

We're not taking changes on v1.1.

This has however been included on v1.2 branch.