1056096 - Camera preview stream is blurry on some hardware

Status: RESOLVED FIXED

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

Description

[Andreas Pehrson [:pehrsons]]

Attachment: Top: vanilla v1.4 is blurry (320x240); bottom: patched v1.4 is sharp (640x480)

On some hardware, the camera app would select a preview resolution that is significantly lower than the screen resolution - thus resulting in blurry pictures.

We have a device almost bound for release on v1.4 where getOptimalPreviewSize() (at https://github.com/mozilla-b2g/gaia/blob/v1.4/apps/camera/js/lib/camera-utils.js#L54) is called with the following arguments (some pseudo objects here, might be more preview sizes but they wouldn't affect the result):

getOptimalPreviewSize([{320x240},{352x288},{640x480},{720x480}], {2048x1536})
Viewportsize is not passed in, but is calculated inside the method to the same as the screen's resolution, 480x320.

This returns {320x240}, which is half of the screen resolution. It is then scaled up to 480x320 and becomes blurry.

If we instead returned the larger size {640x480} and scaled it down, we'd get a nice sharp image. I patched my local gaia and tried it. See the attachment for a comparison, the upper device is vanilla v1.4 with 320x240 being displayed while the lower one is patched and uses 640x480. Zoom in on the statue of liberty to easily see the difference.

This is currently blocking our release.

Comment 1

[Andreas Pehrson [:pehrsons]]

Attachment: [1.4] Always prefer higher preview resolution than screen

This is the patch I used when taking the attached picture.

I tried not to touch any existing code here so the code could probably be shrunk a bit. The general approach should be clear however.

Comment 2

[Andreas Pehrson [:pehrsons]]

This appears to be the same in 2.0 and 2.1 as well.

On a nightly keon I see a preview resolution of 384x288 in the log, and it appears somewhat blurry on screen:
V/QualcommCameraHardware( 4098): No Record Size requested, use the preview dimensions
I/QualcommCameraHardware( 4098): setRecordSize: preview dimensions: 384x288
I/QualcommCameraHardware( 4098): setRecordSize: video dimensions: 384x288

Comment 3

[Andreas Pehrson [:pehrsons]]

I edited the pull request so the change is now considerably smaller, but should be functionally equivalent to before.

Comment 4

[David Flanagan [:djf]]

Andreas,

First of all, let me apologize for this bug. I had serious misgivings about this code when I reviewed it. I should have pushed back harder, because the AOSP algorithm that this code is based on is quite obviously broken. What is surprising, however, is that we've gone this long without anyone reporting a bug.

Your patch looks okay to me, and seems fine to land in the Telenor tree. 

Unless we get this marked 1.4+, however, we won't be able to land it in the Mozilla tree. 

For the Mozilla tree, I'd prefer a patch that goes to the heart of the issue. Instead of adding a filter to remove preview sizes that are too small, I'd suggest one that just changes the comparison that allows sizes that are too small.  

But I'd also like to consult with Justin on this since he is the one who landed the code originally. Justin: do you remember why we ever thought it would be okay to select a preview size that was too small to fill the screen?  If that is still necessary for some reason could we fix this bug to allow previews that are too small, but only if they are at least 90% of the screen size, for example?

Justin: also note that the AOSP algorithm is also badly flawed (and I think we discussed this in review) because it only compares the (landscape-mode) height of the preview to the height of the screen.  If the camera offered a 4:3 preview with resolution 427x320, its height would match the screen height, and it would be selected. But its width would not match the screen width.

Justin: assuming we land Andreas's patch or something like it for 1.4, can you take care of fixing this on master and for 2.0? It seems to me that this is something we might need to block on for 2.0.  Though if it causes us to change preview sizes on devices, it will have performance and power implications.

Comment 5

[David Flanagan [:djf]]

Andreas: is it the Dolphin device that this bug is occuring on?

Justin: if this is a Dolphin, that means a different chipset. So perhaps we have not observed this bug before because the preview sizes offered by the Dolphin's chipset are different than the preview sizes offered by camera drivers from the other chipset vendor.

Comment 6

[Justin D'Arcangelo [:justindarc]]

(In reply to David Flanagan [:djf] from comment #4)
> For the Mozilla tree, I'd prefer a patch that goes to the heart of the issue. Instead of adding a
> filter to remove preview sizes that are too small, I'd suggest one that just changes the comparison
> that allows sizes that are too small.  
> 

I think this patch does go to the heart of this issue. It shifts priorities to first take the actual dimensions (height) into consideration *before* looking at aspect ratios.

> But I'd also like to consult with Justin on this since he is the one who
> landed the code originally. Justin: do you remember why we ever thought it
> would be okay to select a preview size that was too small to fill the
> screen?  If that is still necessary for some reason could we fix this bug to
> allow previews that are too small, but only if they are at least 90% of the
> screen size, for example?
> 

I don't think we ever considered it to be "ok" to select a preview size smaller than the screen. However, it seems that is possible that this *could* happen given the right combination of previewSizes + screen size. The root of the problem here is that this algorithm discards previewSizes that don't meet the aspect ratio tolerance threshold before it even considers the size.

> Justin: also note that the AOSP algorithm is also badly flawed (and I think
> we discussed this in review) because it only compares the (landscape-mode)
> height of the preview to the height of the screen.  If the camera offered a
> 4:3 preview with resolution 427x320, its height would match the screen
> height, and it would be selected. But its width would not match the screen
> width.
> 

Since this algorithm is trying to optimize for a matching aspect ratio *first* and the size itself is secondary, its not necessary to compare widths. The algorithm first filters previewSizes that have a matching aspect ratio, then simply compares the heights to find the one that is closest to the screen height (the width is assumed to be a match because the aspect ratio matches).

> Justin: assuming we land Andreas's patch or something like it for 1.4, can
> you take care of fixing this on master and for 2.0? It seems to me that this
> is something we might need to block on for 2.0.  Though if it causes us to
> change preview sizes on devices, it will have performance and power
> implications.

Sure, I can do that. This patch looks good to me and I think it will do a good job at preventing further issues down the road with future devices.

Comment 7

[David Flanagan [:djf]]

Comment on attachment 8475927 [details] [review]
[1.4] Always prefer higher preview resolution than screen

The patch does look like it fixes the issue for Telenor, and landing the existing patch in a device-specific tree as is seems like it might be an expedient thing to do.

If we're going to land this in Mozilla's tree, however, it needs some changes, as noted on Github.

Comment 8

[David Flanagan [:djf]]

Justin: in the comment for getOptimalPreviewSize, you say that it is "loosely based" on the AOSP. But in fact, it is pretty much a straight port, using the same variable names and the same comments in the same broken english. You didn't just study their algorithm and re-implement it, you created a derivative work, which means that we are out of compliance with the Apache license.

So I think you either need to rewrite the entire method or add: 

 Copyright (C) 2009 The Android Open Source Project

to the comment at the top.

Comment 9

[David Flanagan [:djf]]

Hmm. It should have been obvious that this bug was not about the Dolphin device since that has a 480x800 screen, not a 320x480 screen.  I guess I should try 1.4 on my hamachi and see if I can reproduce there.

Comment 10

[David Flanagan [:djf]]

My understanding is that 1.4 uplifts are for Dolphin only, and if this is not a bug that affects Dolphin, we will probably never get 1.4+ for it.  We should probably try to get something uplifted to 2.0, however.

Comment 11

[David Flanagan [:djf]]

On a hamachi running 1.4, getOptimalPreviewSize picks 384x288 which is smaller than 480x320

Comment 12

[David Flanagan [:djf]]

With Andreas's patch I get a preview stream of 576x432 on hamachi

Comment 13

[Andreas Pehrson [:pehrsons]]

This is the dolphin's chipset but with a smaller screen than the reference device. 

So hamachi 1.4 gets the same result as keon 2.1. I think they have the same screen size and similar chipsets if not the same. 

We have tops a week to play with for landing this. I'm in Europe so that will clearly delay the review process somewhat. If you want to fix this Justin, feel free. I'll address the review comments tomorrow if nothing new has popped up.

Comment 14

[Sri Kasetti]

Can we move the nomination to 2.0? since only dolphin bugs are being fixed for 1.4

Comment 15

[David Flanagan [:djf]]

Attachment: pull request for v1.4

Andreas,

What do you think of this patch instead of yours?

Comment 16

[David Flanagan [:djf]]

Attachment: pull request for v2.0

Justin, would you review this please?  If you like what I've done, you can presumably create the version you want for master by just removing the two loops that consider aspect ratio.

Comment 17

[David Flanagan [:djf]]

Comment on attachment 8476382 [details] [review]
pull request for v2.0

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 #): old regression, don't know bug number

[User impact] if declined: phones with 320x480 screens will have slightly blurry camera previews

[Testing completed]: works on my hamachi

[Risk to taking this patch] (and alternatives if risky): there might be performance or memory differences after this patch is applied for devices that were getting an inappropriately small preview. I don't know if anyone has been testing 2.0 on small screen devices, though, so this may not matter.
[String changes made]: none

Comment 18

[Andreas Pehrson [:pehrsons]]

(In reply to Sri Kasetti from comment #14)
> Can we move the nomination to 2.0? since only dolphin bugs are being fixed
> for 1.4

Uhm, no. As I said, this is a dolphin with a smaller screen.

Comment 19

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8476379 [details] [review]
pull request for v1.4

Looks good!

I just had a small nit on naming noted in the pull request.

Comment 20

[David Flanagan [:djf]]

Andreas: good point about naming. That's what I get for changing variable names after writing the code...

At this point I no longer know who is responsible for making uplift decisions for the 1.4 tree. If you know, please ping them about this bug.

Comment 21

[David Flanagan [:djf]]

I've updated both the 1.4 and the 2.0 pull request to address the naming issue Andreas identified.  (I left the naming the same, but changed the quality metric so that bigger numbers are better, so that the naming becomes intuitive.)

Comment 22

[David Flanagan [:djf]]

Justin,

Are you willing to take this bug to create a patch for master and shepard the various patches through to landing? I don't want this to fall through the cracks while I'm OOO next week.

Comment 23

[David Flanagan [:djf]]

Justin,

Also, you'll notice that my patches don't update any tests. All the existing camera tests pass on 1.4, so I don't think I broke anything. But presumably you'll want to add a test for 320x480 screen sizes to the existing test somewhere. (I'd do it myself if I wasn't in a panic about bug 1051172.)

Comment 24

[Andreas Pehrson [:pehrsons]]

That'd be Wayne I believe.

Comment 25

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8476382 [details] [review]
pull request for v2.0

David: Patch looks great! Very thorough! I'll likely use a variation of this for master (v2.1), except I'm going to try and remove the photo aspect ratio restriction as I don't think its necessary (as we discussed yesterday, this may be the case but is too risky to attempt for 1.4/2.0).

Comment 26

[Justin D'Arcangelo [:justindarc]]

(In reply to David Flanagan [:djf] from comment #22)
> Justin,
> 
> Are you willing to take this bug to create a patch for master and shepard
> the various patches through to landing? I don't want this to fall through
> the cracks while I'm OOO next week.

Yes. I will take this bug and see that it gets landed on master. Thanks for looking into it!

Comment 27

[Wayne Chang [:wchang]]

Thanks.

Andreas, does this impose any impact on the camera launch time on 1.4?

Comment 28

[Andreas Pehrson [:pehrsons]]

Wayne: The perceived launch time was about the same, though I don't have the proper tools to measure it.

Comment 29

[Andreas Pehrson [:pehrsons]]

Please land v1.4 only. a=1.4+ and r=jdarcangelo as the v1.4 patch is a mere cherry-pick of the 2.0 one.

I'll let you take care of the other landings justin, but I'm requesting this now since our deadline is coming up tomorrow.

Comment 30

[Justin D'Arcangelo [:justindarc]]

Landed on v1.4:

https://github.com/mozilla-b2g/gaia/commit/45e0b0d242b1d2deb31d0f5fded49b9932668e3f

Comment 31

[Andreas Pehrson [:pehrsons]]

Thanks :)

Comment 32

[Justin D'Arcangelo [:justindarc]]

Attachment: pull-request (master)

Wilson: Would you be available to review this since David is out this week? If you want some background on the discussion surrounding this change, just ping me on IRC.

Comment 33

[Wilson Page [:wilsonpage]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #32)
> Wilson: Would you be available to review this since David is out this week?
> If you want some background on the discussion surrounding this change, just
> ping me on IRC.

Just skimmed the patch, initial thoughts:

- We are now ignoring the aspect ratio of the selected picture/video profile. I was under the impression that the aspect ratios had to match. Perhaps this was only in video mode, but I'd like mikeh to clarify this. Mikeh: are we theoretically able to select a 16:9 video profile but use a 4:3 preview stream?
- We are likely to see a bump in memory usage by using a larger (scaled down) preview stream.

Comment 34

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Wilson Page [:wilsonpage] from comment #33)
 
> - We are now ignoring the aspect ratio of the selected picture/video
> profile. I was under the impression that the aspect ratios had to match.
> Perhaps this was only in video mode, but I'd like mikeh to clarify this.
> Mikeh: are we theoretically able to select a 16:9 video profile but use a
> 4:3 preview stream?

Right now, the Camera app uses the same resolution for the preview and the recorded frames. There was at least one device, however, that had trouble with a picture size setting that didn't match the aspect ratio of the video stream. IIRC, it only caused a problem when recording video.

Comment 35

[Justin D'Arcangelo [:justindarc]]

(In reply to Wilson Page [:wilsonpage] from comment #33)
> - We are now ignoring the aspect ratio of the selected picture/video
> profile. I was under the impression that the aspect ratios had to match.
> Perhaps this was only in video mode, but I'd like mikeh to clarify this.
> Mikeh: are we theoretically able to select a 16:9 video profile but use a
> 4:3 preview stream?

As Mike already mentioned, in video recording mode, the preview size is ignored and the actual preview size used is the same as the video recorder profile. However, for pictures, it shouldn't matter because the frames are always supposed to be cropped from the center of the camera sensor.

> - We are likely to see a bump in memory usage by using a larger (scaled
> down) preview stream.

Yes, perhaps. But in my tests so far, we never have to scale down anything. On Hamachi, Helix and Nexus 4, there is a preview size that perfectly fits the screen. But, on Flame, the 864x480 preview size gets selected and the screen size is 854x480 (so, 10px gets cut-off, but we don't "scale down"). This might increase memory utilization a little bit, but results in a much, much clearer camera preview stream.

Comment 36

[Justin D'Arcangelo [:justindarc]]

Landed on master:

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

Still waiting for approval to land David's patch for v2.0.

Comment 37

[bhavana bajaj [:bajaj]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #35)
> (In reply to Wilson Page [:wilsonpage] from comment #33)
> > - We are now ignoring the aspect ratio of the selected picture/video
> > profile. I was under the impression that the aspect ratios had to match.
> > Perhaps this was only in video mode, but I'd like mikeh to clarify this.
> > Mikeh: are we theoretically able to select a 16:9 video profile but use a
> > 4:3 preview stream?
> 
> As Mike already mentioned, in video recording mode, the preview size is
> ignored and the actual preview size used is the same as the video recorder
> profile. However, for pictures, it shouldn't matter because the frames are
> always supposed to be cropped from the center of the camera sensor.
> 
> > - We are likely to see a bump in memory usage by using a larger (scaled
> > down) preview stream.
> 
> Yes, perhaps. But in my tests so far, we never have to scale down anything.
> On Hamachi, Helix and Nexus 4, there is a preview size that perfectly fits
> the screen. But, on Flame, the 864x480 preview size gets selected and the
> screen size is 854x480 (so, 10px gets cut-off, but we don't "scale down").
> This might increase memory utilization a little bit, but results in a much,
> much clearer camera preview stream.

Given the memory concens here and keeping in mind the stability/perf state of 2.0, I wanted to seek feedback from Inder/Hema before landing this on 2.0. Ni'ing.

Comment 38

[Hema Koka [:hema]]

(In reply to bhavana bajaj [:bajaj] from comment #37)
> (In reply to Justin D'Arcangelo [:justindarc] from comment #35)
> > (In reply to Wilson Page [:wilsonpage] from comment #33)
> > > - We are now ignoring the aspect ratio of the selected picture/video
> > > profile. I was under the impression that the aspect ratios had to match.
> > > Perhaps this was only in video mode, but I'd like mikeh to clarify this.
> > > Mikeh: are we theoretically able to select a 16:9 video profile but use a
> > > 4:3 preview stream?
> > 
> > As Mike already mentioned, in video recording mode, the preview size is
> > ignored and the actual preview size used is the same as the video recorder
> > profile. However, for pictures, it shouldn't matter because the frames are
> > always supposed to be cropped from the center of the camera sensor.
> > 
> > > - We are likely to see a bump in memory usage by using a larger (scaled
> > > down) preview stream.
> > 
> > Yes, perhaps. But in my tests so far, we never have to scale down anything.
> > On Hamachi, Helix and Nexus 4, there is a preview size that perfectly fits
> > the screen. But, on Flame, the 864x480 preview size gets selected and the
> > screen size is 854x480 (so, 10px gets cut-off, but we don't "scale down").
> > This might increase memory utilization a little bit, but results in a much,
> > much clearer camera preview stream.
> 
> Given the memory concens here and keeping in mind the stability/perf state
> of 2.0, I wanted to seek feedback from Inder/Hema before landing this on
> 2.0. Ni'ing.


Given that the changes may have memory usage increase per previous comments, it will be good to get Inder's input on whether this is an issue for 2.0 devices that are being tested and if so provide feedback on Justin/djf's 2.0 patch. 

If this is not an issue with 2.0 devices, I suggest leaving it as-is (fix already in master for 2.1)

Thanks
Hema

Comment 39

[Casper van Donderen (Telenor)]

For us (Telenor) it would be great if this lands in v2.0 as well, since we would like to provide a v1.4 -> v2.0 -> v2.1 upgrade path.

Comment 40

[Hema Koka [:hema]]

(In reply to Casper van Donderen (Telenor) from comment #39)
> For us (Telenor) it would be great if this lands in v2.0 as well, since we
> would like to provide a v1.4 -> v2.0 -> v2.1 upgrade path.

Andreas/Casper: It would be great if you can test the 2.0 patch on your device.

Justin: Please check the memory usage with 2.0 patch and let us know what the impact is.

Thanks
Hema

Comment 41

[Andreas Pehrson [:pehrsons]]

I'll just note that there shouldn't be any change in app memory usage. The texture is normally passed from the camera as a gralloc buffer, i.e. stored in graphic memory. It stays there until blitted to the screen, so there would only be an increase in graphic memory usage. Of course if a device's camera returns another type of buffer, it could have other implications. Question is, do we have such devices?

Comment 42

[Andreas Pehrson [:pehrsons]]

Just tested before and after the 2.0 patch. It works great on our device :)

Comment 43

[Inder]

Please don't land this change in 2.0. We have gone through all our testing and this issue was not raised on 2.0. We are post CC at this point and don't want any changes which causes risks.

Comment 44

[Hema Koka [:hema]]

Thanks for the input Inder and Andreas.

Since there is no impact on 2.0 based on testing, we will leave it as-is (canceled 2.0 approval request). This is already fixed on master, 2.1 and 1.4

Thanks
Hema