Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support exposure compensation #194

Merged
merged 2 commits into from
Feb 2, 2018
Merged

Support exposure compensation #194

merged 2 commits into from
Feb 2, 2018

Conversation

andrewcking
Copy link
Contributor

Hopefully this is more helpful than it is trouble. I am not entirely sure about my unit tests.

Since different devices will return a different IntRange for exposure compensation, I figured the best way to implement it would be with the typical highest, lowest and manual selectors.

In addition to those selectors I added an autoExposure selector which returns the exposure compensation back to the default zero. It isn’t strictly necessary since one could set it manually to zero themselves. I can remove it if you see fit.

@Diolor
Copy link
Member

Diolor commented Jan 27, 2018

Hi @andrewcking the PR looks very nice! Thanks! I have 2 minor comments only which would be nice to solve before merging.
Do we also need to use getExposureCompensationStep in the Camera.Parameters? Like, what happens with the in between of the step exposure values? They get rounded?

@@ -44,6 +45,10 @@ private infix fun Int.applyInto(parameters: Camera.Parameters) {
parameters.jpegQuality = this
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This used to be the only Int.applyInto but now needs to be called applyJpegQualityInto to distinct it from the new one (applyExposureCompensationInto)

/**
* @return Selector function which always provides the default exposure.
*/
fun autoExposure(): ExposureSelector = single(0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under the hood it's auto for the camera, that's true but I as user I would think ok, I want to reset it back to default so defaultExposure sounds more explicit, usage friendly name.

@andrewcking
Copy link
Contributor Author

andrewcking commented Jan 28, 2018

Updated, I agree defaultExposure is more intuitive.

Regarding getExposureCompensationStep, it is a little odd. You set the exposure compensation with an int since setExposureCompensation only accepts int values, but the int value you supply to has to be scaled by getExposureCompensationStep if you want to know the actual underlying exposure compensation value (EV). So for instance, my Galaxy S8 accepts any int from -20 to 20 for exposure compensation, but the actual allowed EV range is (-2,2), so you have to scale the int value you supplied by .1 (the exposure compensation step) if you want to know the underlying EV value. Setting it up this way allows manufactures to have any desired granularity of exposure compensation they want within their actual EV range. So a device could allow for the standard EV range of (-2,2) but accept int values between (-100,100) giving a lot of granularity. It isn't necessary to know that underlying EV value, but one may wish to display that value to a user and in that case you would need to know what the step value is.

So if my understanding is correct then we don't need getExposureCompensationStep to set or modify the exposure compensation. And we currently have the full range of values available to users.

We could however let the step value be queried via getCapabilities if you would like (I am happy to add that). It might make sense if someone wants to display the EV and if there are manufactures that allow for atypical EV ranges of (-3,3) for instance (to my knowledge everyone uses (-2,2) but there are seldom hard truths in the android camera world as I am sure you know).

@Diolor
Copy link
Member

Diolor commented Feb 2, 2018

Thanks for the explanation @andrewcking. Let's merge it as is.

The plan is to make a beta release before a stable one so some of us can test that it's totally ok to ignore steps. If it's proven that we need EV steps, we can add a step in the IntRange which supports it already.

@Diolor Diolor merged commit 8db7e7f into RedApparat:master Feb 2, 2018
@Diolor Diolor mentioned this pull request Feb 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants