Closed
Bug 1018820
Opened 7 years ago
Closed 7 years ago
[Camera] Exposure control API sets an invalid index
Categories
(Firefox OS Graveyard :: Gaia::Camera, defect, P2)
Tracking
(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)
People
(Reporter: shinuk153, Assigned: mikeh)
References
Details
(Keywords: common-issue+, dev-doc-needed, Whiteboard: camera exposure compensation gonk index)
Attachments
(1 file, 1 obsolete file)
8.93 KB,
patch
|
mikeh
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•7 years ago
|
||
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)
Comment 2•7 years ago
|
||
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
Assignee | ||
Comment 3•7 years ago
|
||
Jason, it's exposed to DOM/JS, but not currently used by the Camera app.
Comment 4•7 years ago
|
||
(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)
Assignee | ||
Comment 6•7 years ago
|
||
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)
Assignee | ||
Comment 8•7 years ago
|
||
Shinuk, thanks for your explanation--I understand what you mean. I will fix this.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(mhabicher)
Assignee | ||
Comment 9•7 years ago
|
||
Attachment #8434321 -
Flags: review?(dhylands)
Attachment #8434321 -
Flags: feedback?(shinuk153)
Comment 10•7 years ago
|
||
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+
Assignee | ||
Comment 11•7 years ago
|
||
Incorporate feedback review, add extra test to verify rounding.
Attachment #8434321 -
Attachment is obsolete: true
Attachment #8434321 -
Flags: feedback?(shinuk153)
Attachment #8434381 -
Flags: review+
Assignee | ||
Comment 12•7 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/c9f413c5774e
Assignee | ||
Updated•7 years ago
|
status-b2g18:
--- → wontfix
status-b2g-v1.1hd:
--- → wontfix
status-b2g-v1.2:
--- → wontfix
status-b2g-v1.3:
--- → wontfix
status-b2g-v1.3T:
--- → wontfix
status-b2g-v1.4:
--- → ?
status-b2g-v2.0:
--- → affected
https://hg.mozilla.org/mozilla-central/rev/c9f413c5774e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S3 (6june)
Reporter | ||
Comment 14•7 years ago
|
||
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)
Assignee | ||
Comment 15•7 years ago
|
||
(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.
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(mhabicher)
Assignee | ||
Updated•7 years ago
|
Keywords: dev-doc-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•