Closed Bug 851823 Opened 8 years ago Closed 8 years ago

Too much Camera shutter being available (Nexus S only ?)

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(firefox20 wontfix, firefox21 wontfix, firefox22 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)

RESOLVED FIXED
B2G C4 (2jan on)
Tracking Status
firefox20 --- wontfix
firefox21 --- wontfix
firefox22 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix

People

(Reporter: gerard-majax, Assigned: ben.tian)

References

Details

(Whiteboard: [TD-8411])

Attachments

(1 file, 1 obsolete file)

After updating my Gaia and Gecko today, I noticed a camera shutter sound when taking pictures, while in settings it was explicitely disabled.

After digging a bit, I found that it's an Android camera shutter in action. More precisely, renaming /system/media/audio/ui/camera_click.ogg to /system/media/audio/ui/camera_click.bak makes the sound going away. After digging a bit more, one can read the source code of libcameraservice, in frameworks/base/services/camera/libcameraservice, and especially the CameraService.cpp file. As far as I can understand, it seems the gecko side of this layer is in netwerk/protocol/device/gonk/Camera.h.

Shutter sound is configured by:

void CameraService::loadSound() {
    Mutex::Autolock lock(mSoundLock);
    LOG1("CameraService::loadSound ref=%d", mSoundRef);
    if (mSoundRef++) return;

    mSoundPlayer[SOUND_SHUTTER] = newMediaPlayer("/system/media/audio/ui/camera_click.ogg");
    mSoundPlayer[SOUND_RECORDING] = newMediaPlayer("/system/media/audio/ui/VideoRecord.ogg");
}

Playing (or not) the shutter is controlled by the local variable mPlayShutterSound, whose access is encapsulated by status_t CameraService::Client::enableShutterSound(bool enable), as one can see in the handleShutter method:

// snapshot taken callback
void CameraService::Client::handleShutter(void) {
    if (mPlayShutterSound) {
        mCameraService->playSound(SOUND_SHUTTER);
    }

    sp<ICameraClient> c = mCameraClient;
    if (c != 0) {
        mLock.unlock();
        c->notifyCallback(CAMERA_MSG_SHUTTER, 0, 0);
        if (!lockIfMessageWanted(CAMERA_MSG_SHUTTER)) return;
    }
    disableMsgType(CAMERA_MSG_SHUTTER);

    mLock.unlock();
}

Looking at the Camera.h file of gecko, we see that there is no enableShutterSound glue, thus no way for Gecko, and Gaia/Camera application to disable the shutter at the Android level.
A quick fix for this: disabling the camera shutter from CameraService.
Assignee: nobody → lissyx+mozillians
Status: NEW → ASSIGNED
Alexandre, could you please add a simple STR so that we could try on Unagi and possibly ask for tracking ?
Sure.

Steps to reproduce are:

0. Ensure that the file /system/media/audio/ui/camera_click.ogg exists
1. Take a picture with Camera application and shutter sound disabled in settings

Expected:
No sound

Current behavior:
The /system/media/audio/ui/camera_click.ogg sound gets played.
The file does not exist on my Unagi.
Comment on attachment 725791 [details] [diff] [review]
Disabling Android-level Camera shutter sound

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

::: dom/camera/GonkCameraHwMgr.cpp
@@ +180,5 @@
> +  /**
> +    * From system/core/include/system/camera.h: CAMERA_CMD_ENABLE_SHUTTER_SOUND = 4
> +    * Disables Android-level Camera shutter sound.
> +    **/
> +  mCamera->sendCommand(4, 0, 0);

Is this a known/standard command?  Or is it Nexus S-specific?  If the latter, will there be unknown side-effects on other platforms?
(In reply to Mike Habicher [:mikeh] from comment #5)
> Comment on attachment 725791 [details] [diff] [review]
> Disabling Android-level Camera shutter sound
> 
> Review of attachment 725791 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/camera/GonkCameraHwMgr.cpp
> @@ +180,5 @@
> > +  /**
> > +    * From system/core/include/system/camera.h: CAMERA_CMD_ENABLE_SHUTTER_SOUND = 4
> > +    * Disables Android-level Camera shutter sound.
> > +    **/
> > +  mCamera->sendCommand(4, 0, 0);
> 
> Is this a known/standard command?  Or is it Nexus S-specific?  If the
> latter, will there be unknown side-effects on other platforms?

It  is a known/standard command. It is present in normal ICS and JB source code.

It seem that "CAMERA_CMD_ENABLE_SHUTTER_SOUND" should be used instead of "4". "system/camera.h" is included in "/frameworks/base/include/camera/Camera.h". It is already included at "GonkCameraHwMgr.h".
Thanks, since I did this as a quick workaround, I did not took the chance to get into troubles of headers :). If this is confirmed to be needed, I'll make a nicer patch.
It is same as Bug 853552.
Duplicate of this bug: 853552
a patch from Bug 853552.
Attachment #725791 - Attachment is obsolete: true
Attachment #728210 - Flags: review+
:gerard-majax, I reviewed a patch at Bug 853552. The patch is a good patch to fix the problem.
:bentian, can you handle the patch in the bug? The patch is already "review+".
:sotaro, no problem. I will handle the patch.
https://tbpl.mozilla.org/?tree=Try&rev=ecad55960d66

:gerard-majax, do you mind my taking this bug?
Of course not, its the same patch :)
Assignee: lissyx+mozillians → btian
(In reply to Ben Tian [:bentian] from comment #13)
> https://tbpl.mozilla.org/?tree=Try&rev=ecad55960d66

The oranges on platforms other than B2G are unrelated to my patch since Camera API is enabled only on B2G. The remaining B2G oranges [3] and [R3] are unrelated failures for all B2G builds nearby encounter [3] failure and [R3] fails before CameraService started.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9e6a306cbc51
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → B2G C4 (2jan on)
Ben, I don't know this code well but I don't see where we reenable the SHUTTER_SOUND when the user turns it on again ?
Flags: needinfo?(btian)
We dont, this disables the hardware shutter entirely as the hardware shutter doesnt exist in the unagi and gaia is responsible for playing the shutter in the case it is enabled.
Flags: needinfo?(btian)
Oh fine, I understand now. I'm requesting approval then.
Comment on attachment 728210 [details] [diff] [review]
Disable shutter sound in CameraService

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 #): 
User impact if declined: the shutter sound will be heard on some phones even if the user disabled it
Testing completed: yes
Risk to taking this patch (and alternatives if risky): very low, one-liner, and this probably has no impact on our target phone
String or UUID changes made by this patch: none
Attachment #728210 - Flags: approval-mozilla-b2g18?
Attachment #728210 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Whiteboard: [TD-8411]
Blocks: 1013313
You need to log in before you can comment on or make changes to this bug.