966834 - [User story] Add white balance support in camera

Status: RESOLVED FIXED

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

Comment 1

[Youngjun Kim]

Ashish! Please check the user stories and update the current status.

Comment 2

[singhashish1887]

We will set the white balance mode "Auto" in camera configuration .

Comment 3

[singhashish1887]

Attachment: Pointer to Pull Request.html

Hi David,

WE are Setting White Balance as auto in camera configuration .
It is work in progress code .Test codes are not included.

Comment 4

[David Flanagan [:djf]]

Comment on attachment 8379605 [details]
Pointer to Pull Request.html

Adding Diego and Wilson for feedback

Comment 5

[David Flanagan [:djf]]

Comment on attachment 8379605 [details]
Pointer to Pull Request.html

Clearing the review request.  I've left comments on github. Overall this looks good, and I think only minor changes will be needed.

Comment 6

[David Flanagan [:djf]]

Also, remember not to change the file modes, and note that the PR has two commits in it. We have a one commit per PR rule, so remember to "squash your commits" (I use git rebase -i) before submitting a final PR.

Comment 7

[singhashish1887]

Attachment: Pull Request

Comment 8

[singhashish1887]

Attachment: Pull Request (camera-dev)

Comment 9

[singhashish1887]

Attachment: Pull Request (camera-dev)

Comment 10

[singhashish1887]

Attachment: Pull Request (camera-dev)

Comment 11

[Hema Koka [:hema]]

Pending David's review, Justin, please create PR

Comment 12

[singhashish1887]

Attachment: Pull Request (camera-dev) UT Test

Comment 13

[Diego Marcos [:dmarcos]]

Attachment: pull-request (camera-new-features)

Comment 14

[David Flanagan [:djf]]

Comment on attachment 8385828 [details] [review]
pull-request (camera-new-features)

This is a nice, simple patch that is almost ready to go.

But r- because we need a way to disable it at build time on a per-device basis.

If we need to run on a device that says it supports auto white balance but the auto white balance actually sucks and looks horrible, we don't want to use the feature on that device.

So please link this to config/app.js somehow so that we can easily enable and disable the feature. I suggest that you add this:

  defaultWhiteBalanceMode: 'auto'

Look this up in as a setting. If it doesn't exist, don't set the white balance at all.  If it does exist, use the value of the setting instead of auto.  If we have some other chipset where there is some better auto value with a different name, then we can support that, too.

Comment 15

[David Flanagan [:djf]]

Needinfo Ashish since the r- notification might go to Diego instead of him.

Comment 16

[David Flanagan [:djf]]

I've added a few comments on the unit test pull request as well.

If at all possible, please make the white balance tests part of the same commit that adds the white balance feature so that they can be landed and backed out as a single unit.

And again, please see if there is some way you can change the file permissions back to 644 before creating your pull requests.

Comment 17

[singhashish1887]

Attachment: Pull Request (camera-dev)

Comment 18

[singhashish1887]

We have updated the white balance as per David comments. please review and let us if there is any changes requires

Comment 19

[Wilson Page [:wilsonpage]]

Comment on attachment 8385796 [details] [review]
Pull Request (camera-dev) UT Test

This is confusing, I've already reviewed this. Your patch is spread across more than one Bugzilla bug.

Also did Ashish write these tests? If they were written by the team in India, why aren't the commits under their names, and why aren't they requesting review themselves?

It seems you may be working against the tools we have in place. Meaning work is being proxied through other devs, communication is going to the wrong people, and I don't know who I'm working with.

Comment 20

[singhashish1887]

Wilson It is me who uploaded the patch for white balance issue as per David comments .I used Mr. Yoon's server to upload the patches because of space issue on my working server.

Comment 21

[singhashish1887]

Attachment: Pull Request (camera-dev)

Comment 22

[Wilson Page [:wilsonpage]]

Comment on attachment 8387534 [details] [review]
Pull Request (camera-dev)

Comments inline on Github

Comment 23

[singhashish1887]

Attachment: Pull Request (camera-dev)

Updated as per Wilson comments. Open for review

Comment 24

[singhashish1887]

Comment on attachment 8388001 [details]
Pull Request (camera-dev)

>bug966834_whitebalance

Comment 25

[singhashish1887]

Attachment: Pull Request (camera-dev)

Comment 26

[Wilson Page [:wilsonpage]]

Comment on attachment 8388049 [details] [review]
Pull Request (camera-dev)

Conditional r+. Please address comments on Github.

Comment 27

[David Flanagan [:djf]]

This looks good to me, too.  If you fix the whitespace issues that Wilson and I have pointed out on github, I will r+ a pull request made against the camera-new-features branch.  I think we're very close on this one. Let's get it landed!

Comment 28

[singhashish1887]

Attachment: Pull Request (camera-dev)

Updated as per Wilson Comments ,Open for  review

Comment 29

[David Flanagan [:djf]]

Comment on attachment 8388987 [details] [review]
Pull Request (camera-dev)

Cancelling the review request.  Please create a single-commit pull request against the camera-new-features branch. This is a small enough change that we ought to be able to land it directly that way.

If you can, please include test cases in your patch. If you can't do that, then as part of your review request explain why there are not unit tests and tell me when those unit tests will land.

Comment 30

[singhashish1887]

we will upload the patches in new branch.

Comment 31

[singhashish1887]

Attachment: Pull Request(Camera-new-feature) With UT

Comment 32

[David Flanagan [:djf]]

Comment on attachment 8389226 [details] [review]
Pull Request(Camera-new-feature) With UT

The unit tests are inadequate, but I'm giving r+ anyway so we can just get this landed.

Comment 33

[Diego Marcos [:dmarcos]]

Attachment: Pull Request rebased on camera new features

Comment 34

[Diego Marcos [:dmarcos]]

I rebased the r+ed PR and landed on camera-new-features:

https://github.com/mozilla-b2g/gaia/commit/3e37c4b6af11c95b0d650d9b8919f5c5f7e7b5d2

Comment 35

[David Flanagan [:djf]]

Bulk edit for camera bugs.

If earlier comments do not show how this bug landed to master, it probably landed as part of https://github.com/mozilla-b2g/gaia/pull/17599 which merged the camera-new-features branch into master.

This bug was uplifted from master to v1.4 as part of https://github.com/mozilla-b2g/gaia/commit/a8190d08e61316a86bba572ba8d894d081a20530