932174 - [flatfish][camera] Camera preview performance is bad

Status: RESOLVED WONTFIX

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

Description

[JustinLee]

Now camera preview performance is bad on flatfish platform. Kernel message shows below when opening camera preview.

s_fmt set width = 2560, height = 1920
[ 2167.181416] [VFE]buffer_setup, buffer count=10, size=7372800

It seems the preview size has been set to 2560*1920 as default. The setting (2560*1920) causes the preformance of camera preview is bad.
Do you have any suggestion for camera preview size?

Comment 1

[William Liang]

Justin has actually a simple work-around on this issue in Gaia. Could you describe it here? Thanks.

Comment 2

[JustinLee]

I modified geco/dom/camera/GonkCameraControl.cpp to set preview size to 640*480. The performance of camera preview got improvement by this modification. The below shows my modification.

---
 dom/camera/GonkCameraControl.cpp |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/dom/camera/GonkCameraControl.cpp b/dom/camera/GonkCameraControl.cpp
index 9e5a75a..c066a16 100644
--- a/dom/camera/GonkCameraControl.cpp
+++ b/dom/camera/GonkCameraControl.cpp
@@ -1114,6 +1114,10 @@ nsGonkCameraControl::SetPreviewSize(uint32_t aWidth, uint32_t aHeight)

   mWidth = bestWidth;
   mHeight = bestHeight;
+  /* Justin: fixed the previewSize to 640*480 */
+  mWidth = 640;
+  mHeight = 480;
+  /* ---end--- */
   mParams.setPreviewSize(mWidth, mHeight);
   PushParameters();
 }
--

Comment 3

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

(In reply to JustinLee from comment #2)
> I modified geco/dom/camera/GonkCameraControl.cpp to set preview size to
> 640*480. The performance of camera preview got improvement by this
> modification. The below shows my modification.

Is a valid preview size being passed into getPreviewStream() in camera.js?

Comment 4

[JustinLee]

(In reply to Mike Habicher [:mikeh] from comment #3)
> (In reply to JustinLee from comment #2)
> > I modified geco/dom/camera/GonkCameraControl.cpp to set preview size to
> > 640*480. The performance of camera preview got improvement by this
> > modification. The below shows my modification.
> 
> Is a valid preview size being passed into getPreviewStream() in camera.js?

The preview size is calculated by below codes. It seems that preview size can't be set by manual config.

// Previews should match the aspect ratio and not be smaller than the screen
    var validPreviews = camera.capabilities.previewSizes.filter(function(res) {
      var isLarger = res.height >= screenHeight && res.width >= screenWidth;
      var aspectRatio = res.height / res.width;
      var matchesRatio = Math.abs(aspectRatio - pictureAspectRatio) < 0.05;
      return matchesRatio && isLarger;
    });

    // We should always have a valid preview size, but just in case                              
    // we dont, pick the first provided.
    if (validPreviews.length) {
      // Pick the smallest valid preview
      this._previewConfig = validPreviews.sort(function(a, b) {
        return a.width * a.height - b.width * b.height;
      }).shift();
    } else {
      this._previewConfig = camera.capabilities.previewSizes[0];
    }

Comment 5

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

What does that function say it's setting the preview size to?

Comment 6

[Mike Lee [:mlee]]

Kevin,

FxOS Perf has no Flatfish devices so can you find someone on the Taipei team to own this.

FxOS Perf Triage

Comment 7

[Marco Chen [:mchen]]

Hi Vincent/Gary,

Could you help to check this case?

Normally Gaia - Camera will choose the resolution which is most close to the panel size.
Maybe the possible preview resolution set passed from Camera HAL didn't fit current panel size.

Thanks.
Sincerely yours.

Comment 8

[Vincent Liu[:vliu]]

(In reply to Marco Chen [:mchen] from comment #7)
> Hi Vincent/Gary,
> 
> Could you help to check this case?
> 
> Normally Gaia - Camera will choose the resolution which is most close to the
> panel size.
> Maybe the possible preview resolution set passed from Camera HAL didn't fit
> current panel size.
> 
> Thanks.
> Sincerely yours.

Talked with Gary and he can track this issue.

Comment 9

[JustinLee]

(In reply to Mike Habicher [:mikeh] from comment #5)
> What does that function say it's setting the preview size to?

In fact, I'm trying to know why that function does.

Comment 10

[JustinLee]

There has a configuration file called "camera.cfg" in flatfish codebase. The file is used for camera setting include preview size, picture size, etc.
About camera preview size setting, it is configured to 2560x1920,1280x720,640x480,1920x1080 by camera.cfg. The preview size has 4 available resolutions on flatfish platform. And camera.cfg also defined a default_preview_size (the setting is 1280x720 now) for default setting.
B2G choices the highest available resolution (2560X1920) for camera previewing. It is the root cause of bad camera preview performance.
Now, I modified the preview size configuration to 640X480 only in camera.cfg. Camera preview performance looks better after that modification.

Comment 11

[William Liang]

Three questions:
1. Why B2G doesn't choose to use the default_preview_size, but the largest one instead?
2. It seems that the supported preview size is predefined in camera.cfg. Can it be determined by the system during the run-time (by querying the camera hal?
3. Is there any existing method which can be used to evaluate a proper preview size according to the camera and processor capability?

Comment 12

[Marco Chen [:mchen]]

Hi Gary,

Could you give an explanation for what is the rule to select preview resolution?
Thanks. 

As I know that camera app will iterate all possible resolution and choose one which 
  1. is larger or equal to the screen resolution.
  2. aspect ratio is the same with the screen or it's deviation is less then 5%.
 
  3. if there is no one there then choose first one in the array.
https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/camera.js#L950

Comment 13

[Vincent Liu[:vliu]]

I want to explain that Android also use the similar implementation to chose the optimal preview size. Please see getOptimalPreviewSize(...) in the below link.

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

Maybe we can check the method used in Android and compared with our implement in camera.js to find a better way to chose a better resolution. Otherwise, I think the current implementation in FFOS is the reference way to pick up a resolution like the Android did.

Comment 14

[Ben Tian (inactive)]

(In reply to William Liang from comment #11)
> Three questions:
> 1. Why B2G doesn't choose to use the default_preview_size, but the largest
> one instead?
> 2. It seems that the supported preview size is predefined in camera.cfg. Can
> it be determined by the system during the run-time (by querying the camera
> hal?
> 3. Is there any existing method which can be used to evaluate a proper
> preview size according to the camera and processor capability?

Camera app queries supported preview sizes from camera hal and selects the one that fits screen size best. Please configure supported preview sizes in camera hal if you don't want upper layer app to choose certain sizes. (I think that's the camera.cfg for since neither camera app nor gecko camera look up that file.)

Comment 15

[Ben Tian (inactive)]

> Could you give an explanation for what is the rule to select preview
> resolution?
> Thanks.
Please refer to bug 866790. The bug intends to solve the inconsistency between preview and postview due to different aspect ratios. The fix is to select preview that matches postview (=picture) aspect ratio.

>   1. is larger or equal to the screen resolution.
>   2. aspect ratio is the same with the screen or it's deviation is less then
> 5%.
Camera app queries supported preview sizes from camera hal and selects sizes that 1) match picture aspect ratio and 2) are larger than screen size to fulfill screen [1]. Then it sorts valid preview sizes from small to large width*height value, and picks the smallest as it already suffices for screen [2].
 
[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/camera.js#L949
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/camera.js#L960

Comment 16

[JustinLee]

(In reply to Ben Tian [:btian] from comment #15)
> > Could you give an explanation for what is the rule to select preview
> > resolution?
> > Thanks.
> Please refer to bug 866790. The bug intends to solve the inconsistency
> between preview and postview due to different aspect ratios. The fix is to
> select preview that matches postview (=picture) aspect ratio.
> 
> >   1. is larger or equal to the screen resolution.
> >   2. aspect ratio is the same with the screen or it's deviation is less then
> > 5%.
> Camera app queries supported preview sizes from camera hal and selects sizes
> that 1) match picture aspect ratio and 2) are larger than screen size to
> fulfill screen [1]. Then it sorts valid preview sizes from small to large
> width*height value, and picks the smallest as it already suffices for screen
> [2].

The screen size is 1280X800 (aspect ratio is 1.6) on faltfish platform. And the supported preview sizes are 2560x1920 ,1280x720,640x480,1920x1080. (the aspect ratio of supported preview sizes are 1.3333, 1.7778, 1.3333, 1.7778.)

I still can't understand that why dose camera app choose 2560X1920 (aspect ratio is 1.3333) for its preview size. And why camera app doesn't choose the default setting of preview size (default_preview_size = 1280X720) or the other resolutions (1920X1080)?

>  
> [1]
> https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/camera.js#L949
> [2]
> https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/camera.js#L960

Comment 17

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

Hi Justine,
   Here is the log.
   
   screen size 1280X800
   ratio is 1.6

@@@camera - capabilities.previewSizes[
     {"width":2560,"height":1920},  //ratio is 0.75
     {"width":1280,"height":720},   //ratio is 1.77 isLarger = false;
     {"width":640,"height":480},    //ratio is 1.33 isLarger = false;
     {"width":1920,"height":1080}]  //ratio is 0.56 matchesRatio = false;
After filter, we got only one fit on gaia's condition.
https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/camera.js#L949

@@@camera - orign validPreviews[{"width":2560,"height":1920}]
@@@camera _previewConfig{"width":2560,"height":1920}

BTW, I've a patch to fix this issue. :)

Comment 18

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

(In reply to GaryChen [:GaryChen][:PYChen] from comment #17)
> Hi Justine,
>    Here is the log.
sorry corrected the ration.
>    
>    screen size 1280X800
>    ratio is 1.6
> 
> @@@camera - capabilities.previewSizes[
>      {"width":2560,"height":1920},  //ratio is 0.75
>      {"width":1280,"height":720},   //ratio is 1.77 isLarger = false;
                                      //ratio is 0.56 matchesRatio = false;
>      {"width":640,"height":480},    //ratio is 1.33 isLarger = false;
                                      //ratio is 0.75
>      {"width":1920,"height":1080}]  //ratio is 0.56 matchesRatio = false;
> After filter, we got only one fit on gaia's condition.
> https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/camera.js#L949
> 
> @@@camera - orign validPreviews[{"width":2560,"height":1920}]
> @@@camera _previewConfig{"width":2560,"height":1920}
> 
> BTW, I've a patch to fix this issue. :)

Comment 19

[JustinLee]

(In reply to GaryChen [:GaryChen][:PYChen] from comment #18)
> (In reply to GaryChen [:GaryChen][:PYChen] from comment #17)
> > Hi Justine,
> >    Here is the log.
> sorry corrected the ration.
> >    
> >    screen size 1280X800
> >    ratio is 1.6

Is this ratio wrong? Is the right ratio 0.625?

> > 
> > @@@camera - capabilities.previewSizes[
> >      {"width":2560,"height":1920},  //ratio is 0.75

> >      {"width":1280,"height":720},   //ratio is 1.77 isLarger = false;
>                                       //ratio is 0.56 matchesRatio = false;
> >      {"width":640,"height":480},    //ratio is 1.33 isLarger = false;
>                                       //ratio is 0.75
> >      {"width":1920,"height":1080}]  //ratio is 0.56 matchesRatio = false;

Why does 0.75 (2560X1920) match 0.625 (1280X800)?
And why doesn't 0.56 (1920X1080) match 0.625 (1280X800)?

It's hard to understand...

> > After filter, we got only one fit on gaia's condition.
> > https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/camera.js#L949
> > 
> > @@@camera - orign validPreviews[{"width":2560,"height":1920}]
> > @@@camera _previewConfig{"width":2560,"height":1920}
> > 
> > BTW, I've a patch to fix this issue. :)

Thanks for the patch. Where can I find the patch? and what did you do via the patch?

Comment 20

[Ben Tian (inactive)]

> The screen size is 1280X800 (aspect ratio is 1.6) on faltfish platform. And
> the supported preview sizes are 2560x1920 ,1280x720,640x480,1920x1080. (the
> aspect ratio of supported preview sizes are 1.3333, 1.7778, 1.3333, 1.7778.)
> 
> I still can't understand that why dose camera app choose 2560X1920 (aspect
> ratio is 1.3333) for its preview size. And why camera app doesn't choose the
> default setting of preview size (default_preview_size = 1280X720) or the
> other resolutions (1920X1080)?

screen size:
  1280x800
picture size (aspect ratio = H/W):
  2560x1920(0.75)
preview sizes (aspect ratio = H/W):
 - 2560x1920 (0.75)  [valid]   is larger than screen size AND match picture aspect ratio=0.75
 - 1280x720 (0.56)   [invalid] height < screen height
 - 640x480 (0.75)    [invalid] width < screen width
 - 1920x1080 (0.56)  [invalid] mismatches picture aspect ratio=0.75

=> 2560x1920 is the only valid preview size.

--
Where is default_preview_size=1280X720 stored and who reads it? Camera app or hal?

Comment 21

[Ben Tian (inactive)]

> Why does 0.75 (2560X1920) match 0.625 (1280X800)?
> And why doesn't 0.56 (1920X1080) match 0.625 (1280X800)?

Please see gaia code [1]. It's the picture size to match, not screen size.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/camera.js#L953

Comment 22

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

Attachment: setPreviewSize.text

Comment 23

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

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

Hi all,
   This is my WIP patch, I use distance of the screenSize(width+height) to validPreviewSize for choosing more approximate valid |previewConfig|.

   Please refer to Android |getOptimalPreviewSize| on #c13, and give me some feedback.

   BTW, I would like to file a new bug for this patch, as :btian mentioned on comment 14, configuring supported preview sizes in 'camera hal' is the best way.

Comment 24

[Dale Harvey (:daleharvey)]

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

Obviously the commented code will be removed, but the general approach looks good to me

Comment 25

[Ben Tian (inactive)]

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

Looks good to me.

One minor thing that square-root computation for diff is unnecessary as we only care their relative difference.

Comment 26

[Joe Cheng [:jcheng] (please needinfo)]

clear blocking flag until we have clarity for flatfish

Comment 27

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

unassigned, since we won't fix gaia part for this issue.

Comment 28

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

The camera app and stack have changed significantly since this issue was first filed. Does the viewfinder still have performance issues?

Comment 29

[BMO Automation]

Firefox OS is not being worked on