936400 - [Camera] Optimize Preview size selector

Status: RESOLVED FIXED

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

Description

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Current 'camera.js' always choose preview size which bigger or equal than screen size.
According to Bug 932174 comment 13, we found android will pick more approximate value with screen size.

Need a new way to pick more approximate value with screen size to enhance preview video.

[1] https://android.googlesource.com/platform/packages/apps/Camera/+/android-4.2.2_r1.2/src/com/android/camera/Util.java

Comment 1

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Attachment: pull request: https://github.com/mozilla-b2g/gaia/pull/13509

Hi Dale and Ben,
   Same topic with Bug 932174 just addressed your comment and send pull request again.

   Please help to review it again, thanks.

Comment 2

[Ben Tian (inactive)]

Comment on attachment 829161 [details] [review]
pull request: https://github.com/mozilla-b2g/gaia/pull/13509

f=me. Thanks.

Comment 3

[David Flanagan [:djf]]

Please coordinate these changes with Diego and Justin who are currently doing camera app refactoring, and with Rob MacDonald and Peter La who are doing UX and visual design work for the camera in 1.3.

Dale says that the code is good, but this strikes me as a UX change. The current designs do not (I think) allow letterboxing of the preview for video recording.  So I'm not certain that this change will be acceptible to UX and visual design.

Gary could you explain more clearly what the actual bug is here? What does your code change and why is that change needed?

We've got a lot of code in the camera app for picking resolutions. As we move to other form factors, I think we might want to just hardcode all the sizes and make them configurable at build time and just lose all of the dynamic resolution picking code.

Setting needinfo for all the other people working on this so they'll be aware of it.

Comment 4

[Dale Harvey (:daleharvey)]

Comment on attachment 829161 [details] [review]
pull request: https://github.com/mozilla-b2g/gaia/pull/13509

Codewise this is good to me, will defer to Davids comments for UX changes / landability though.

Comment 5

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

In Bug 932174, we found 'camera.js' picked more larger preview size(2560x1920) although screen size is 800x1280. So it cause flatfish's camera app has bad performance.

After discussing with :vliu and :btian who work on gecko, we all agree configure supported preview sizes in camera hal. It means hal should report suitable preview sizes for camera app.

But here is a question, if camera hal configured preview sizes are smaller than screen size, it will never get the optimize preview size in current gaia code.

So we refer to how does android pick preview size for camera, we can see 'getOptimalPreviewSize' in below link.
https://android.googlesource.com/platform/packages/apps/Camera/+/android-4.2.2_r1.2/src/com/android/camera/Util.java

for (Size size : sizes) {
   double ratio = (double) size.width / size.height;
   if (Math.abs(ratio - targetRatio) > ASPECT_TOLERANCE) continue;
   if (Math.abs(size.height - targetHeight) < minDiff) {
    optimalSize = size;
    minDiff = Math.abs(size.height - targetHeight);
   }
}

Android use 'minDiff' with size.height and targetHeight to pick optimalSize(preview size). 

I propose to use min distance to pick preview size for fitting screen size, it let camera has more efficient in preview mode.

I have did some test on buri and flatfish device. please refer to below link.
https://bug932174.bugzilla.mozilla.org/attachment.cgi?id=827908 

please do not hesitate to give me some feedback, thanks.

Comment 6

[Diego Marcos [:dmarcos]]

The current gaia code for preview selection works as expected. 

The patch basically suggests to match the aspect ratio of the preview to the screen size instead of the aspect ratio of the picture size selected. 

Is not the purpose of the current approach to provide the user an accurate representation of the picture that will be taken? If we use the screen as the target aspect ratio the preview might not be an accurate approximation of the resulting picture.

Comment 7

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Yes, the best preview size is approximation of the resulting 'picture size', I am totally agree that.

In fact, we have performance issue with large screen size devices. Phone devices might be fine, but it is obviously bad performance on tablet since we get more larger preview size than screen size.

I think that is why android use 'getDefaultDisplaySize' to be 'targetHeight' to find the 'optimalSize' for preivew size.

https://android.googlesource.com/platform/packages/apps/Camera/+/android-4.2.2_r1.2/src/com/android/camera/Util.java
Point point = getDefaultDisplaySize(currentActivity, new Point());
int targetHeight = Math.min(point.x, point.y);

Comment 8

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Hi David,
   Can we land it or just set to 'WONTFIX'?

Comment 9

[David Flanagan [:djf]]

Here are my thoughts about this.

1) It may not always be correct for camera to select a preview size that matches the actual recording size, since we generally force the preview to fit the whole screen. This may change on some devices in 1.3.  But in the cases where we are forcing the preview to be full-screen, I suppose we should consider previews that match the screen aspect ratio.

2) Gary: the tests you refer to in comment 5 seem to show that this patch ends up picking a preview size that is much smaller than the screen on Buri devices. That means we can't land it as is.

3) I agree that having a preview much larger than the screen on Flatfish gives terrible performance. From your tests, though, it looks like the choices are between previews that are too big and previews that are too small.  Unless the code is somehow missing a preview that is too big or too small, it seems to me that the problem is that the camera driver on Flatfish does not offer good preview sizes, and that the bug should be fixed there.

4) Another approach we could take here is to allow a preview size setting to be specified in config.js (which is generated at build time from a camera.json file in a distibution directory.)  If the preview size is hardcoded, then the camera app could just use it and not call the getPreviewSize() function.  This would allow flatfish builds to be created that did not have this problem.

Comment 10

[Rob MacDonald [:robmac]]

In Peter's analysis of the different screen sizes, from what I recall, video on Flatfish was the most problematic in that there was a big difference between the screen size and what is actually captured. We deferred a solution for this but I'll bring this up in my next conversations with Peter and Diego. 

Gary, you may also want to bring Juwei into the loop (I'll add her to the needsinfo as well) as she's been focusing heavily on Flatfish. Juwei, next time we chat I can review the latest camera specs and demonstrate the issues with Flatfish.

Comment 11

[Hema Koka [:hema]]

Bruce/Marco

Are you addressing this as part of the device team's user stories that were targeted for 1.3 (921078 and 921079). 

Thanks
Hema

Comment 12

[Marco Chen [:mchen]]

(In reply to Hema Koka [:hema] from comment #11)
> Bruce/Marco
> 
> Are you addressing this as part of the device team's user stories that were
> targeted for 1.3 (921078 and 921079). 
> 

From my personal understanding, this bug is not related to 921078/921079.
This one focuses on the criteria and mechanism for selecting a more suitable preview size.
1. Not allow user to select (921078)
2. Didn't consider to crop frame (921079)

Comment 13

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

No, this is not related to 921078/921079.
minus it from v1.3 first.

Comment 14

[Juwei Huang[:juwei]]

No need for flatfish support in here.

Comment 15

[Bruce Huang [:bhuang] (inactive)]

agree with comment 12 and comment 13

Comment 16

[Justin D'Arcangelo [:justindarc]]

Attachment: PR: https://github.com/mozilla-b2g/gaia/pull/15375

Comment 17

[Diego Marcos [:dmarcos]]

I tested the patch and the front camera preview doesn't cover the screen and it's displayed on the bottom right corner. 

For the video preview there are two big black bands on top and bottom

See attached images

Comment 18

[Diego Marcos [:dmarcos]]

Attachment: Video Preview

Comment 19

[Diego Marcos [:dmarcos]]

Attachment: Front Camera Preview

Comment 20

[Justin D'Arcangelo [:justindarc]]

Diego: this is the issue I was seeing yesterday. This patch does not address the video recording preview (yet, adding that in today), but I believe there is an issue with the camera on Helix that is not related to the preview size. For the front camera, if you toggle from picture to video and back to picture again, the preview will fill the screen. However, internally, it is using the exact same preview size both times. I need to investigate further, but I believe this is a completely different bug.

Comment 21

[Diego Marcos [:dmarcos]]

For the front camera after switching to video and back to picture again the preview fits the screen vertically but I get a black band on the left side. See attached picture (tested on helix)

Comment 22

[Diego Marcos [:dmarcos]]

Attachment: Left Band Front Camera

Comment 23

[Justin D'Arcangelo [:justindarc]]

Diego: That left band on the side is actually a CSS bug I believe because if you look closely, the entire UI is shifted slightly. Regardless, I think you would agree that doing the same thing twice should yield the same result, it's really strange behavior. I have a couple log statements in there to output the size that was selected from the list of available preview sizes and you can see that on Helix, it is requesting the same size every time, even though the video size appears to be different.

Comment 24

[Diego Marcos [:dmarcos]]

We ended up having a single patch for this bug and bug 959890.

It has landed in master:

https://github.com/mozilla-b2g/gaia/commit/c9e81cbe5671ba1bcb62aecf066111416fc0700d