1018820 - [Camera] Exposure control API sets an invalid index

Status: RESOLVED FIXED

Description

[shinuk]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 (Beta/Release)
Build ID: 20140528030219

Steps to reproduce:

Set a value through webidl:
  void setExposureCompensation(optional double compensation);

In GonkCameraParameters::SetTranslated(uint32_t aKey, const double& aValue),
that value is converted to a Gonk index:

    /**
    * Convert from real value to a Gonk index, round
    * to the nearest step; index is 1-based.
    */
    index = (aValue - mExposureCompensationMin + mExposureCompensationStep / 2) / mExposureCompensationStep + 1;
    DOM_CAMERA_LOGI("Exposure compensation = %f --> index = %d\n", aValue, index);
    return SetImpl(CAMERA_PARAM_EXPOSURECOMPENSATION, index);



Actual results:

If I set,
  cameracontrol.setExposureCompensation(0);//zero

the converted index in SetTranslated() is 73.0 which is not proper for exposure compensation.

I don't know why this compensation value needs to be converted to a Gonk index,
which results in camera configuration fail.




Expected results:

I think that a compensation value is passed to camera hal without any Gonk index conversion.

Comment 1

[Mike Habicher [:mikeh] (high bugzilla latency)]

Hi shinuk, what values does your test platform return for:
- KEY_MIN_EXPOSURE_COMPENSATION
- KEY_MAX_EXPOSURE_COMPENSATION
- KEY_EXPOSURE_COMPENSATION_STEP
...?

Comment 2

[Jason Smith [:jsmith]]

Mike - Is this an internal camera API bug only triggered if a vendor tries to use the particular API in a certain manner or is there a potential for end-user impact in the camera app?

Comment 3

[Mike Habicher [:mikeh] (high bugzilla latency)]

Jason, it's exposed to DOM/JS, but not currently used by the Camera app.

Comment 4

[Hema Koka [:hema]]

(In reply to Mike Habicher [:mikeh] from comment #3)
> Jason, it's exposed to DOM/JS, but not currently used by the Camera app.

Based on this, I am removing it from 1.4? 

Thanks
Hema

Comment 5

[shinuk]

(In reply to Mike Habicher [:mikeh] from comment #1)
> Hi shinuk, what values does your test platform return for:
> - KEY_MIN_EXPOSURE_COMPENSATION
> - KEY_MAX_EXPOSURE_COMPENSATION
> - KEY_EXPOSURE_COMPENSATION_STEP
> ...?

KEY_MIN_EXPOSURE_COMPENSATION: -12.0
KEY_MAX_EXPOSURE_COMPENSATION: +12.0
KEY_EXPOSURE_COMPENSATION_STEP: (float)1/6

Comment 6

[Mike Habicher [:mikeh] (high bugzilla latency)]

Hi shinuk,

From my reading of CameraParameters.h[1], setting KEY_EXPOSURE_COMPENSATION to 0 sets the automatic mode, and setting values 1..N chooses a specific fixed value. Using your above values, -12.0..12.0 in steps of 1/6 map to index values of 1..145.

Thus, given EV=0, the code in comment 0 maps it to 73, which corresponds to the middle EV value, which should be 0, and which is the correct value.

1. http://androidxref.com/4.0.4/xref/frameworks/base/include/camera/CameraParameters.h#299

Comment 7

[shinuk]

From my reading of CameraParameters.h[2],
Exposure compensation index multiply by step eqals to EV. 
Ex: if exposure compensation index is 6 and step is 0.3333, EV is -2.(which is wrong, EV is 2)

2. http://androidxref.com/4.0.4/xref/frameworks/base/include/camera/CameraParameters.h#308

In this case, the maximum exposure compensation index (>=0) is 12,
the minimum exposure compensation index (<=0) is -12.

Thus, given compensation indices range from -12 to +12 and step is 1/6,
which corresponds to EV values range from -2 to +2, which means EV should be between -2 and +2.

If index is 145 and step is 1/6, EV is 24.167, which results in camera configuration failure.

I'm not understood comment 6:
> Thus, given EV=0, the code in comment 0 maps it to 73, which corresponds to
> the middle EV value, which should be 0, and which is the correct value.

Is this really correct?

Comment 8

[Mike Habicher [:mikeh] (high bugzilla latency)]

Shinuk, thanks for your explanation--I understand what you mean. I will fix this.

Comment 9

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Fix exposure compensation translation, v1

Comment 10

[Dave Hylands [:dhylands]]

Comment on attachment 8434321 [details] [diff] [review]
Fix exposure compensation translation, v1

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

r=me with nits addressed

::: dom/camera/GonkCameraParameters.cpp
@@ +554,5 @@
>        /**
>         * Convert from real value to a Gonk index, round
>         * to the nearest step; index is 1-based.
>         */
> +      if (aValue < 0.0) {

nit: It seems to me that you could just do:

double i = round(aValue / mExposureCompensationStep);

round says (http://linux.die.net/man/3/round) it rounds away from zero (for the halfway case):
See: http://git.mozilla.org/?p=external/caf/platform/bionic.git;a=blob;f=libm/src/s_round.c;h=274c119418debffeb498fe267aa7ec5c66943c2a;hb=HEAD for the implementation of round.

And then just check that it's between mExposureMinIndex and mExposureMaxIndex

@@ +556,5 @@
>         * to the nearest step; index is 1-based.
>         */
> +      if (aValue < 0.0) {
> +        double i = (aValue - mExposureCompensationStep / 2.0) / mExposureCompensationStep;
> +        if (i < INT32_MIN || i > INT32_MAX) {

nit: (if you don't use round) Couldn't you just use mExposureCompensationMinIndex here? (instead of INT32_MIN) Then you wouldn't need the additional test below.

@@ +566,5 @@
> +          index = mExposureCompensationMinIndex;
> +        }
> +      } else {
> +        double i = (aValue + mExposureCompensationStep / 2.0) / mExposureCompensationStep;
> +        if (i < INT32_MIN || i > INT32_MAX) {

nit: (If you don't use round) Similarly here. Couldn't you just use mExposureCompensationMaxIndex here? (instead of INT32_MAX)

Comment 11

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Fix exposure compensation translation, v1.1 r=dhylands

Incorporate feedback review, add extra test to verify rounding.

Comment 12

[Mike Habicher [:mikeh] (high bugzilla latency)]

https://hg.mozilla.org/integration/b2g-inbound/rev/c9f413c5774e

Comment 13

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/c9f413c5774e

Comment 14

[shinuk]

Hello, Mike.

I think you are confused Exposure Compensation with Exposure Value.
It isn't obvious to set or get EV with exposureCompensation attribute.

Comment 15

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to shinuk from comment #14)
> 
> I think you are confused Exposure Compensation with Exposure Value.
> It isn't obvious to set or get EV with exposureCompensation attribute.

Hi shinuk, the attribute is the direct EV. Using your numbers from comment 6, this means 'exposureCompensation' is set to [-2, -1.83, -1.67, ..., 1.67, 1.83, 2]. The underlying indices are not exposed to JS.