Closed Bug 966834 Opened 6 years ago Closed 6 years ago

[User story] Add white balance support in camera

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.4+, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
1.4 S5 (11apr)
blocking-b2g 1.4+
Tracking Status
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: skasetti, Assigned: singhashish1887)

References

Details

(Whiteboard: [fxos:media])

User Story

As a user, I want the white balance option be set to auto if the device supports white balance

Attachments

(6 files, 8 obsolete files)

39 bytes, text/x-github-pull-request
wilsonpage
: review+
Details | Review
40 bytes, text/x-github-pull-request
wilsonpage
: review-
Details | Review
46 bytes, text/x-github-pull-request
djf
: review-
Details | Review
39 bytes, text/x-github-pull-request
Details | Review
46 bytes, text/x-github-pull-request
djf
: review+
Details | Review
46 bytes, text/x-github-pull-request
Details | Review
No description provided.
User Story: (updated)
Ashish! Please check the user stories and update the current status.
Assignee: nobody → singhashish1887
We will set the white balance mode "Auto" in camera configuration .
Blocks: 966764
Depends on: 971460
No longer depends on: 971460
Attached file Pointer to Pull Request.html (obsolete) —
Hi David,

WE are Setting White Balance as auto in camera configuration .
It is work in progress code .Test codes are not included.
Attachment #8379605 - Flags: review?(dflanagan)
Comment on attachment 8379605 [details]
Pointer to Pull Request.html

Adding Diego and Wilson for feedback
Attachment #8379605 - Flags: feedback?(wilsonpage)
Attachment #8379605 - Flags: feedback?(dmarcos)
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.
Attachment #8379605 - Flags: review?(dflanagan)
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.
Attached file Pull Request (obsolete) —
Attachment #8381038 - Flags: review?(wilsonpage)
Attached file Pull Request (camera-dev) (obsolete) —
Attachment #8381076 - Flags: review?(wilsonpage)
Attachment #8379605 - Attachment is obsolete: true
Attachment #8379605 - Flags: feedback?(wilsonpage)
Attachment #8379605 - Flags: feedback?(dmarcos)
Attachment #8381038 - Attachment is obsolete: true
Attachment #8381038 - Flags: review?(wilsonpage)
Attachment #8381076 - Attachment is obsolete: true
Attachment #8381076 - Flags: review?(wilsonpage)
Attached file Pull Request (camera-dev) (obsolete) —
Attachment #8381180 - Flags: review?(wilsonpage)
Attachment #8381180 - Attachment is obsolete: true
Attachment #8381180 - Flags: review?(wilsonpage)
Attachment #8381954 - Flags: review?(wilsonpage)
Attachment #8381954 - Flags: review?(wilsonpage) → review+
Pending David's review, Justin, please create PR
Flags: needinfo?(jdarcangelo)
Attachment #8385796 - Flags: review?(wilsonpage)
Attachment #8385828 - Flags: review?(dflanagan)
Flags: needinfo?(jdarcangelo)
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.
Attachment #8385828 - Flags: review?(dflanagan) → review-
Needinfo Ashish since the r- notification might go to Diego instead of him.
Flags: needinfo?(singhashish1887)
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.
Attached file Pull Request (camera-dev) (obsolete) —
Attachment #8386633 - Flags: review?(dmarcos)
Attachment #8386633 - Flags: review?(dflanagan)
Flags: needinfo?(singhashish1887)
We have updated the white balance as per David comments. please review and let us if there is any changes requires
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.
Attachment #8385796 - Flags: review?(wilsonpage) → review-
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.
Attachment #8386633 - Attachment is obsolete: true
Attachment #8386633 - Flags: review?(dmarcos)
Attachment #8386633 - Flags: review?(dflanagan)
Attached file Pull Request (camera-dev) (obsolete) —
Attachment #8387534 - Flags: review?(wilsonpage)
Comment on attachment 8387534 [details] [review]
Pull Request (camera-dev)

Comments inline on Github
Attachment #8387534 - Flags: review?(wilsonpage) → review-
Attachment #8387534 - Attachment is obsolete: true
Attached file Pull Request (camera-dev) (obsolete) —
Updated as per Wilson comments. Open for review
Attachment #8388001 - Flags: review?(wilsonpage)
Comment on attachment 8388001 [details]
Pull Request (camera-dev)

>bug966834_whitebalance
Attachment #8388001 - Attachment is obsolete: true
Attachment #8388001 - Flags: review?(wilsonpage)
Attached file Pull Request (camera-dev) (obsolete) —
Attachment #8388049 - Flags: review?(wilsonpage)
Comment on attachment 8388049 [details] [review]
Pull Request (camera-dev)

Conditional r+. Please address comments on Github.
Attachment #8388049 - Flags: review?(wilsonpage) → review+
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!
Flags: needinfo?(singhashish1887)
Attachment #8388049 - Attachment is obsolete: true
Flags: needinfo?(singhashish1887)
Updated as per Wilson Comments ,Open for  review
Attachment #8388987 - Flags: review?(wilsonpage)
Attachment #8388987 - Flags: review?(dflanagan)
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.
Attachment #8388987 - Flags: review?(dflanagan)
Flags: needinfo?(singhashish1887)
we will upload the patches in new branch.
Flags: needinfo?(singhashish1887)
Attachment #8389226 - Flags: review?(wilsonpage)
Attachment #8389226 - Flags: review?(dflanagan)
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.
Attachment #8389226 - Flags: review?(dflanagan) → review+
I rebased the r+ed PR and landed on camera-new-features:

https://github.com/mozilla-b2g/gaia/commit/3e37c4b6af11c95b0d650d9b8919f5c5f7e7b5d2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Attachment #8389226 - Flags: review?(wilsonpage)
Attachment #8388987 - Flags: review?(wilsonpage)
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
Target Milestone: --- → 1.4 S5 (11apr)
blocking-b2g: --- → 1.4+
You need to log in before you can comment on or make changes to this bug.