Closed Bug 972142 Opened 6 years ago Closed 6 years ago

[OPEN C_1.3][Camera] Camera preview looks very noisy through viewfinder during photo composition

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

(blocking-b2g:1.3+, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 unaffected)

RESOLVED FIXED
blocking-b2g 1.3+
Tracking Status
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- unaffected

People

(Reporter: marcia, Assigned: ben.tian)

References

Details

(Keywords: regression, Whiteboard: [mwcdemo2014])

Attachments

(2 files, 2 obsolete files)

Open C, while running:

Gaia      ce17d5eae7b1893ae4397c814b10ae598fcbdb58
Gecko     429fa73f31a1a54c24cdb60dcd847f8974fed354
BuildID   20140212175323
Version   28.0
ro.build.version.incremental=eng.duanxiaodong.20140129.175912
ro.build.date=Wed Jan 29 17:59:23 CST 2014

STR:
1. Open the camera. Note that the viewfinder display shows everything very grainy (looks very noisy) and it is not very sharp. This happens especially when there are patterns are textures that are part of the picture you are trying to shoot.

This will be very noticeable in any demos.
Please try with the patch from https://bugzilla.mozilla.org/show_bug.cgi?id=969761

Does that fix this?
If that patch does not fix it, there are some diagnostic tests we could run to find out if the problem is in the camera driver or in the Gaia Camera app.  In general, we don't have enough experience with new devices for the camera app to work smoothly on new hardware without tweaks.

If someone who has one of these devices can work with me (i.e. run test patches and send me the logcat output), we might be able to get this working.  Or overnight a phone to me and I'll see if I can do anything.
Flags: needinfo?(mozillamarcia.knous)
Marcia,

Please also assess for the related bug 942199. If there is a visible delay when you tap on the filmstrip while the photo preview loads and displays, then your camera is affected by that bug. The fix for that bug just creates a build-time configuration option that can be used to workaround (trading off image quality for performance).  But you won't get the workaround automatically, you have to create a customization file like the one shown here: https://bugzilla.mozilla.org/show_bug.cgi?id=942199#c56
Adding mwu as my understanding is that he will be spinning builds for the Open C. So far I have a base image from ZTE and I have just been flashing Gecko/Gaia on top.
Flags: needinfo?(mozillamarcia.knous)
Marcia,

But see comment 1: I may have fixed this bug in gaia at around the same time you were filing the bug. By now a nightly build should have my fix for bug 969761, and it may have resolved this.

The stuff in comment #3 is a separate bug that is worth assessing for.
Marcia: the "custom build" I'm talking about in comment #3 is a custom gaia build. Something that you can try out yourself if needed without anything from mwu.
Flags: needinfo?(mozillamarcia.knous)
Updating summary to be more accurate. The current builds I have been testing that have the fix still seem to have the noise issue.
Flags: needinfo?(mozillamarcia.knous)
Summary: [OPEN C_1.3][Camera] Pictures look very grainy through viewfinder during photo composition → [OPEN C_1.3][Camera] Camera preview looks very noisy through viewfinder during photo composition
Flashing the non-ENG build from the latest directory seems to show the preview correctly, however I cannot flash the latest build over it.
What do you mean by non-ENG build? Is it from our partner, or?
The gaia Marcia used in comment 1 already includes bug 969761 fix. The fix makes camera app show zoomed (grainier) preview. Attached picture compares preview before the fix (left) and after the fix (right). Both devices are of build 20140220053054 with different version of camera app:
  Gaia before fix (left): e439f8630fdea790595efaebab24ec21be6bc0e4
  Gaia after fix (right): a43904d9646109b48836d62f7aa51e499fbf4b2e

David, any idea about the problem?
Flags: needinfo?(dflanagan)
(In reply to Ben Tian [:btian] from comment #10)
> Created attachment 8378946 [details]
> zoomed preview after bug 969761 fix
> 
> The gaia Marcia used in comment 1 already includes bug 969761 fix. The fix
Correction. Should be gaia in comment 0.
I'm assuming that all of the testing is being done under 1.3, right?

Under the patch for 969761, the camera picks the smallest 4:3 preview stream size that is bigger than the screen (in device pixels), or if there is not such a preview size, it picks the largest 4:3 preview size that is offered.

Under the old code, it would pick smallest 4:3 preview that was bigger than the screen (in css pixels), and if it couldn't find one, would (I think) give up and use the first preview size listed.

So if this camera only offers tiny 4:3 previews, then my patch would force it to use that tiny preview. In that case, the old code would have been picking the first preview, regardless of its aspect ratio.  The camera code really wants a 4:3 preview, so if the camera can't offer one, I'd say that is a bug in the camera driver.  But if that can't be fixed, I guess we can work around it in Gaia.

I'm completely confused by the fact that the preview size is zoomed in because we scale the preview to fit the screen. So regardless of the preview size we should see the same portion of it. So that part is a mystery to me.

Since I don't have a device to try this on, all I can do is speculate.  

Ben: if you're willing to add console.log() statements to camera.js you can find out what preview stream sizes this camera offers and what preview size is being selected with and without this patch.  I think you should be able to find the right place in camera.js to insert console.log() calls easily. But if not I can create a patch that will enable us to figure out what is going on in Gaia.

Also setting needinfo for Justin because he and I have debated the 4:3 preview requirement, and he should know whether we're going to have to change that.
Flags: needinfo?(jdarcangelo)
Flags: needinfo?(dflanagan)
Flags: needinfo?(btian)
The style of preview [1] should be in CSS pixels instead of device pixels.
[1] https://github.com/mozilla-b2g/gaia/blob/v1.3/apps/camera/js/camera.js#L1123

Attach temp fix that outputs following logs for reference:
GeckoConsole: Content JS LOG at app://camera.gaiamobile.org/js/camera.js:1040 in Camera.setPreviewSize: inner 320 x 533 ratio 1.5
GeckoConsole: Content JS LOG at app://camera.gaiamobile.org/js/camera.js:1041 in Camera.setPreviewSize: screen 480 x 799.5
GeckoConsole: Content JS LOG at app://camera.gaiamobile.org/js/camera.js:1094 in Camera.setPreviewSize: preview config 1600 x 1200
GeckoConsole: Content JS LOG at app://camera.gaiamobile.org/js/camera.js:1112 in Camera.setPreviewSize: preview 799.5 x 599.625
Flags: needinfo?(btian)
(In reply to David Flanagan [:djf] from comment #12)
> I'm assuming that all of the testing is being done under 1.3, right?
Yes.

> Ben: if you're willing to add console.log() statements to camera.js you can
> find out what preview stream sizes this camera offers and what preview size
> is being selected with and without this patch.
The selected preview size is 1600x1200.
All available sizes are
1600x1200, 1280x720, 864x480, 800x480, 768x432
720x480,   640x480,  576x432, 480x320, 384x288
352x288,   320x240,  240x160, 176x144

> Also setting needinfo for Justin because he and I have debated the 4:3
> preview requirement, and he should know whether we're going to have to
> change that.
In this case the 4:3 preview requirement results in a preview size of >4x screen size overkill.
Comment on attachment 8379469 [details] [diff] [review]
WIP patch: preview style in CSS pixel

Review of attachment 8379469 [details] [diff] [review]:
-----------------------------------------------------------------

Nice catch, Ben!

I know you haven't asked for review yet, but when you remove the logging output, this fix looks good to me.
Attachment #8379469 - Flags: review+
Is the patch enough to resolve the visual issue? Can we call that good enough for now, or do we also have to address the overly large preview size in this bug?

Can we ask the device maker to modify the camera driver so that it offers a 800x600 preview?  Or do we need to modify the preview size selection algorithm?  Or add a build-time configuration option to hard-code the preview size?
djf: Just a heads up.. I'm updating the preview size selection algorithm in 1.4 (yet again). In bug 974264 we're tracking an issue where a less-than-optimal preview size was being selected for Nexus4 (preview selected had a similar aspect ratio to the screen, but needed significant scale adjustment to fit). So, we're considering setting a threshold where if too much scale adjustment is needed to fit the selected preview, to instead pick the preview that requires the least amount of scale adjustment. This should hopefully satisfy both ends of the spectrum.

That said, I still wouldn't be opposed to a build-time config option.
Flags: needinfo?(jdarcangelo)
1.3? since this bug is regressed by 1.3+ bug 969761.
blocking-b2g: --- → 1.3?
Assignee: nobody → btian
Attachment #8379469 - Attachment is obsolete: true
Attachment #8379622 - Attachment mime type: text/plain → text/html
Duplicate of this bug: 975296
Component: Vendcom → Gaia::Camera
Whiteboard: [mwcdemo2014][POVB] → [mwcdemo2014]
Keywords: regression
Blocks: 969761
Requesting QA to test the patch attached.
Keywords: verifyme
(In reply to Preeti Raghunath(:Preeti) from comment #21)
> Requesting QA to test the patch attached.

Too early to flag - we should only flag when it's actually landed.
Keywords: verifyme
blocking-b2g: 1.3? → 1.3+
Merged into 1.3: https://github.com/mozilla-b2g/gaia/commit/d7312ac94f9308a6893bc0d7281aa6ccaf1ab849

Nothing to do on master since bug 969761 change is on 1.3 only.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
(In reply to Ben Tian [:btian] from comment #23)
> Merged into 1.3:
> https://github.com/mozilla-b2g/gaia/commit/
> d7312ac94f9308a6893bc0d7281aa6ccaf1ab849
> 
> Nothing to do on master since bug 969761 change is on 1.3 only.

Ben - This needs approval before landing. Please backout & get approval.
Flags: needinfo?(btian)
(In reply to Jason Smith [:jsmith] from comment #24)
> (In reply to Ben Tian [:btian] from comment #23)
> > Merged into 1.3:
> > https://github.com/mozilla-b2g/gaia/commit/
> > d7312ac94f9308a6893bc0d7281aa6ccaf1ab849
> > 
> > Nothing to do on master since bug 969761 change is on 1.3 only.
> 
> Ben - This needs approval before landing. Please backout & get approval.
Backed out: https://github.com/mozilla-b2g/gaia/commit/8643484db1887b4219299ee70700be35483173ae
Status: RESOLVED → REOPENED
Flags: needinfo?(btian)
Resolution: FIXED → ---
(In reply to Ben Tian [:btian] from comment #25)

Please request approval-gaia-v1.3 on the patch :)
Flags: needinfo?(btian)
NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
  969761
[User impact] if declined:
  User sees noisy and zoomed camera preview.
[Testing completed]:
  Tested the patch fixes this bug on OpenC device. It restores camera preview as before bug 969761 change.
[Risk to taking this patch] (and alternatives if risky):
  None.
[String changes made]:
  None.
Attachment #8379622 - Attachment is obsolete: true
Attachment #8381872 - Flags: approval-gaia-v1.3?(fabrice)
Flags: needinfo?(btian)
Attachment #8381872 - Flags: approval-gaia-v1.3?(fabrice) → approval-gaia-v1.3+
Merged to 1.3: https://github.com/mozilla-b2g/gaia/commit/ad504390a7a5f094f8967f80a0f29a1e6552535e
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.