Closed Bug 981047 Opened 6 years ago Closed 6 years ago

[Camera][Gecko] Expose luminance attribute to DOM

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.0 S1 (9may)

People

(Reporter: mikeh, Assigned: mikeh)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 2 obsolete files)

The Camera app would like to get a luminance condition (either "high" or "low") from the camera library, to indicate whether or not it should turn on the flash LED (by setting flashMode="torch") when starting video recording.

The luminance condition is exposed by the library through the CameraParameters object, using a key value of "luminance-condition".

We are currently working with an implementation that updates the luminance condition inside the camera driver based on the current amount of light falling on the sensor. It does not, however, send a notification to camera clients indicating a change. Nor is it sufficient to simply call CameraParameters.get("luminance-condition") in Gecko because CameraParameters is a snapshot of the camera configuration from the last time we pulled a parameter set from the camera driver.

Current CameraControl attributes are read synchronously, directly from the CameraParameters object; pushing this object to and pulling it from the camera hardware is an asynchronous operation that happens off-Main Thread.

Other camera implementations may be able to notify us when conditions such as luminance change; many implementations will not support luminance reporting of any kind at all.

To cover all of these possibilities, a luminance API will need the following components:

  interface CameraCapabilities {
    // Values:
    //  - "": (falsy) indicates luminance reporting is not supported
    //  - "live": the CameraControl luminance attribute can be read directly
    //  - "cached": the attribute reflects a cached value that may be stale
    [Constant, Cached] readonly attribute DOMString luminanceMode;
  };

The corresponding CameraControl interface will then need:

  // Called when the luminance condition is updated, the argument is
  // either "high", "low", or "".
  CameraLuminanceCallback = void (DOMString luminance);

  interface CameraControl {
    // Values:
    //  - "", (falsy) indicates not supported; else "high" or "low"
    // If capabilities.luminance is:
    //  - "cached": this returns the last value pulled from the driver
    //  - "live": this returns the last value posted by the driver
    [Throws]
    readonly attribute DOMString luminance;

    // Method to get the current luminance; although this is intended
    // to be used when luminanceMode is "cached", it can be used for
    // "live" as well. If luminance reporting is not supported, the
    // success callback is called with an empty string argument.
    [Throws]
    void getLuminance(optional CameraLuminanceCallback onSuccess,
                      optional CameraErrorCallback onError);

    // If capabilities.luminance is:
    //  - "live": invoked whenever the driver posts a new luminance value
    //  - "cached": whenever getLuminance() gets a new value from the driver
    attribute CameraLuminanceCallback? onLuminanceChange;
  };
I suggest a different API instead: adding a new boolean value to CameraStartRecordingOptions called flashInLowLight (bikeshedding welcome!).  Setting that value to true would put the flash into the torch mode if we detect a low light condition when startRecording is called, and setting it to false or dropping it would mean that the flash status should not be changed based on the luminance conditions.

In the future, if we have a use case for getting notified of the luminance changes at other times rather than when we call startRecording, we should expose that information as an event, which will require the way that the information is delivered to Gecko to change to be some kind of an event which we can subscribe to, but I think my proposal above will address the current use case in a much easier way, and it will be more efficient to implement as well, since we already to a roundtrip when startRecording is called to retrieve the updated camera parameters value and we can just piggyback on that.

Does this sound good?
Thanks for the comments, Ehsan. I like the simplicity of that approach.
Some pseudo-code:

// In Gecko
startRecording(options)
{
  mAutoFlashModeOverridden = false;

  if (options.autoEnableLowLightFlash) {
    PullParameters(); // we need a current set
    bool lowLight;
    mParams.Get("luminance-condition", lowLight);
    const char* flashMode;
    mParams.Get("flash-mode", flashMode);
    if (lowLight && flashMode == "auto") {
      mAutoFlashModeOverridden = true;
      mParams.Set("flash-mode", "torch");
    }
  }

  recorder->start();
}

stopRecording()
{
  if (mAutoFlashModeOverridden) {
    mParams.Set("flash-mode", "auto");
  }
  recorder->stop();
}

set(parameter, value)
{
  if (parameter == "flash-mode") {
    mParams.Set("flash-mode", value);
    // explicit mode set always wins and sticks
    mAutoFlashModeOverridden = false;
  }
  mParams.Set(parameter, value);
}
(In reply to comment #3)
> Some pseudo-code:
> 
> // In Gecko
> startRecording(options)
> {
>   mAutoFlashModeOverridden = false;
> 
>   if (options.autoEnableLowLightFlash) {
>     PullParameters(); // we need a current set
>     bool lowLight;
>     mParams.Get("luminance-condition", lowLight);
>     const char* flashMode;
>     mParams.Get("flash-mode", flashMode);
>     if (lowLight && flashMode == "auto") {
>       mAutoFlashModeOverridden = true;
>       mParams.Set("flash-mode", "torch");
>     }
>   }
> 
>   recorder->start();
> }
> 
> stopRecording()
> {
>   if (mAutoFlashModeOverridden) {
>     mParams.Set("flash-mode", "auto");
>   }
>   recorder->stop();
> }
> 
> set(parameter, value)
> {
>   if (parameter == "flash-mode") {
>     mParams.Set("flash-mode", value);
>     // explicit mode set always wins and sticks
>     mAutoFlashModeOverridden = false;
>   }
>   mParams.Set(parameter, value);
> }

This is exactly what I had in mind in comment 1.  Thanks for putting this into more concrete terms!
Here's an initial version of a patch that adds 'autoEnableLowLightTorch' to the startRecording() options. I don't have a device that supports luminance condition reporting, so I can't test it; but it builds.

There are currently no automated tests as the emulator doesn't support recording.
Attachment #8388738 - Flags: feedback?(hiro7998)
(This patch applies on top of those in bug 965421, bug 965420, and bug 981318.)
Applies on top of the patches in bug 965421 and bug 965420.

Ehsan, the only DOM-facing change is the addition of the new attribute to the CameraStartRecordingOptions dictionary.
Attachment #8388738 - Attachment is obsolete: true
Attachment #8388738 - Flags: feedback?(hiro7998)
Attachment #8389323 - Flags: review?(ehsan)
Attachment #8389323 - Flags: review?(dhylands)
Comment on attachment 8389323 [details] [diff] [review]
If supported, use scene luminance to control camera flash, v2

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

::: dom/camera/GonkCameraControl.cpp
@@ +888,5 @@
> +nsGonkCameraControl::SetupRecordingFlash(bool aAutoEnableLowLightTorch)
> +{
> +  mAutoFlashModeOverridden = false;
> +
> +  if (aAutoEnableLowLightTorch && mLuminanceSupported && mFlashSupported) {

nit: flip logic around and return early
Attachment #8389323 - Flags: review?(dhylands) → review+
Attachment #8389323 - Flags: review?(ehsan) → review+
Comment on attachment 8389363 [details] [diff] [review]
If supported, use scene luminance to control camera flash, v2.1, r=dhylands,ehsan

Youngwoo, can you try out this patch with a modified Camera app to make sure it works as expected?
Attachment #8389363 - Flags: feedback?(hiro7998)
I verified the patch is working fine in our device.
Great--thanks, Youngwoo! Attachment 8389363 [details] [diff] is based on the patch for bug 965420, so this will land as soon as that one does.
https://hg.mozilla.org/mozilla-central/rev/958d209e788e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.5 S1 (9may)
You need to log in before you can comment on or make changes to this bug.