[Camera] Exposure control API sets an invalid index

RESOLVED FIXED in 2.0 S3 (6june)

Status

defect
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: shinuk153, Assigned: mikeh)

Tracking

({common-issue+, dev-doc-needed})

unspecified
2.0 S3 (6june)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:-, b2g18 wontfix, b2g-v1.1hd wontfix, b2g-v1.2 wontfix, b2g-v1.3 wontfix, b2g-v1.3T wontfix, b2g-v1.4 ?, b2g-v2.0 affected)

Details

(Whiteboard: camera exposure compensation gonk index)

Attachments

(1 attachment, 1 obsolete attachment)

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.
blocking-b2g: --- → 1.4?
Flags: needinfo?(mhabicher)
Keywords: common-issue+
OS: All → Gonk (Firefox OS)
Priority: -- → P2
Hardware: All → ARM
Whiteboard: camera exposure compensation gonk index
Hi shinuk, what values does your test platform return for:
- KEY_MIN_EXPOSURE_COMPENSATION
- KEY_MAX_EXPOSURE_COMPENSATION
- KEY_EXPOSURE_COMPENSATION_STEP
...?
Assignee: nobody → mhabicher
Flags: needinfo?(mhabicher) → needinfo?(shinuk153)
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?
Component: General → Gaia::Camera
Jason, it's exposed to DOM/JS, but not currently used by the Camera app.
(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
blocking-b2g: 1.4? → -
(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
Flags: needinfo?(shinuk153)
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
Flags: needinfo?(shinuk153)
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?
Flags: needinfo?(shinuk153) → needinfo?(mhabicher)
Shinuk, thanks for your explanation--I understand what you mean. I will fix this.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(mhabicher)
Attachment #8434321 - Flags: review?(dhylands)
Attachment #8434321 - Flags: feedback?(shinuk153)
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)
Attachment #8434321 - Flags: review?(dhylands) → review+
Incorporate feedback review, add extra test to verify rounding.
Attachment #8434321 - Attachment is obsolete: true
Attachment #8434321 - Flags: feedback?(shinuk153)
Attachment #8434381 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/c9f413c5774e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S3 (6june)
Hello, Mike.

I think you are confused Exposure Compensation with Exposure Value.
It isn't obvious to set or get EV with exposureCompensation attribute.
Flags: needinfo?(mhabicher)
(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.
Flags: needinfo?(mhabicher)
You need to log in before you can comment on or make changes to this bug.