Closed Bug 1072420 Opened 10 years ago Closed 10 years ago

Change in camera preview size for v2.1

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::Camera
Platform:
ARM
Gonk (Firefox OS)
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: bhargavg1, Unassigned)

References

Details

Attachments

(1 file)

TEST pull-request (v2.1)
(deleted), text/x-github-pull-request
Details 
Hi See that in v2.1 we changed the camera preview size from

v2.0 - 640*480   (wxh)
v2.1 - 1920*1080

which is having an impact on the total ION memory to grow by 33MB. was this change intentional?
I believe that's because in v2.1 we switched to using a higher default video resolution, and the older code ties the two (preview and video) together.

Bug 1014877 (landed in v2.2/master) uncouples them.
Flags: needinfo?(jdarcangelo)
Are you referring to the preview size in picture mode or video mode? As Mike mentioned in Comment 1, the Camera is unable to use a preview size that is different than the recorder profile when in video recording mode. If you are recording 1080p video, then the preview size must also be 1080p. In v2.2, this constraint is no longer in place.
Flags: needinfo?(jdarcangelo) → needinfo?(bhargavg1)
(In reply to Justin D'Arcangelo [:justindarc] from comment #2)
> Are you referring to the preview size in picture mode or video mode? 

the case in question is for preview size in picture mode
Flags: needinfo?(bhargavg1)
(In reply to bhargavg1 from comment #3)
> (In reply to Justin D'Arcangelo [:justindarc] from comment #2)
> > Are you referring to the preview size in picture mode or video mode? 
> 
> the case in question is for preview size in picture mode

What is the screen size of the device you are using?
Flags: needinfo?(bhargavg1)
(In reply to Justin D'Arcangelo [:justindarc] from comment #4)
> (In reply to bhargavg1 from comment #3)
> > (In reply to Justin D'Arcangelo [:justindarc] from comment #2)
> > > Are you referring to the preview size in picture mode or video mode? 
> > 
> > the case in question is for preview size in picture mode
> 
> What is the screen size of the device you are using?

Its the same hardware used for v2.0/v2.1 1080p from dmesg,

mdss_fb_register: FrameBuffer[0] 1080x1920 size=8388608 registered successfully!
Flags: needinfo?(bhargavg1)
[Blocking Requested - why for this release]:
blocking-b2g: --- → 2.1?
If anything, v2.0 is incorrect because we shouldn't be selecting a 640x480 preview size for a 1080p display. This change is a result of Bug 1056096 which was fixing the problem of having *too small* of a preview size for certain devices' displays. Currently, if we are getting a 1080p preview size on a 1080p display, that *should* be the correct intended behavior (which it sounds like is what we're getting).

Flagging David and Hema for more info on how we should proceed. Perhaps we simply need an optional way to override the preview size selection completely manually at build time to satisfy all vendors/devices?
Flags: needinfo?(hkoka)
Flags: needinfo?(dflanagan)
Obviously the camera preview needs to be at least as big as the screen or it will look bad. If this device actually has a 1080p screen (like the iphone 6+!) then the camera preview is the right size and there is nothing to do here.

I think it is more likely that Bhargav misunderstood Justin's question in comment #4 and reported the preview size in comment #5, not the screen size.

Bhargav: are you actually testing the Camera app on a device with a 1080x1920 screen?
Flags: needinfo?(dflanagan) → needinfo?(bhargavg1)
Attached file TEST pull-request (v2.1) (deleted) — 
Bhargav: Also, can you also let me know what all of the available preview sizes are for the camera in the device you're testing?

The function that selects the "optimal" preview size in v2.1 is located here:

https://github.com/mozilla-b2g/gaia/blob/v2.1/apps/camera/js/lib/camera-utils.js#L57

If that function cannot find an "optimal" preview size that matches the photo aspect ratio, it ultimately will fall back to simply selecting the largest preview size. This pull request adds a logging output to list all the available preview sizes. Please run this and let me know what your available preview sizes are. Thanks!
Attachment #8494776 - Flags: feedback?(bhargavg1)
(In reply to Justin D'Arcangelo [:justindarc] from comment #9)
> Created attachment 8494776 [details]
> TEST pull-request (v2.1)
> 
> Bhargav: Also, can you also let me know what all of the available preview
> sizes are for the camera in the device you're testing?
> 
> The function that selects the "optimal" preview size in v2.1 is located here:
> 
> https://github.com/mozilla-b2g/gaia/blob/v2.1/apps/camera/js/lib/camera-
> utils.js#L57
> 
> If that function cannot find an "optimal" preview size that matches the
> photo aspect ratio, it ultimately will fall back to simply selecting the
> largest preview size. This pull request adds a logging output to list all
> the available preview sizes. Please run this and let me know what your
> available preview sizes are. Thanks!

D/QCameraParameters(  197): int32_t qcamera::QCameraParameters::initDefaultParameters(): supported preview sizes: 1920x1080,1280x720,864x480,800x480,768x432,720x480,640x480,576x432,480x320,384x288,352x288,320x240,240x160,176x144
Flags: needinfo?(bhargavg1)
Can you confirm David's question in Comment 8? We just want to double-check and make sure that the device you are testing on has a *screen* size of 1080 x 1920. If that is the case, then I do not think this is a valid bug.
Flags: needinfo?(bhargavg1)
That debug log in comment #10 identifies the problem. The preview sizes are being passed as strings like "1280x720". But the code expects JS objects like { width: 1280, height: 720 }.

Something has changed somewhere in the driver or in gecko.

Mike: the camera app code is written assuming that mozCamera.capabilities.previewSizes is an array of objects. Has something changed that would cause it to be an array of strings?
Flags: needinfo?(mhabicher)
(In reply to Justin D'Arcangelo [:justindarc] from comment #11)
> Can you confirm David's question in Comment 8? We just want to double-check
> and make sure that the device you are testing on has a *screen* size of 1080
> x 1920. If that is the case, then I do not think this is a valid bug.

yes the screen size is 1080 x 1920

what change have we made for considering the picture size.

looks like the selected aspect ratio is different.

If selected picture size is 5 MB (2592x1944) - aspect ratio = 1.3

The selected preview size is 1920 x 1080 - aspect ration = 1.7

In v2.0 we had selected VGA resolution 640x480 - aspect ration = 1.3 inline with the picture size and the same used to be upscaled
Flags: needinfo?(bhargavg1)
(In reply to bhargavg1 from comment #13)
> (In reply to Justin D'Arcangelo [:justindarc] from comment #11)
> > Can you confirm David's question in Comment 8? We just want to double-check
> > and make sure that the device you are testing on has a *screen* size of 1080
> > x 1920. If that is the case, then I do not think this is a valid bug.
> 
> yes the screen size is 1080 x 1920
> 
> what change have we made for considering the picture size.
> 
> looks like the selected aspect ratio is different.
> 
> If selected picture size is 5 MB (2592x1944) - aspect ratio = 1.3
> 
> The selected preview size is 1920 x 1080 - aspect ration = 1.7
> 
> In v2.0 we had selected VGA resolution 640x480 - aspect ration = 1.3 inline
> with the picture size and the same used to be upscaled

The preview aspect ratio shouldn't make any difference since we scale the preview to fill the screen regardless of the aspect ratio and previews are cropped from the center of the sensor. In many cases, we've encountered devices with screen sizes that don't match the aspect ratio of any photo sizes which caused us to pick a preview that was entirely too small for the screen. In this case, the same would be true; 640x480 would have to be scaled up significantly to fill the 1920x1080 screen and would result in a very blurry preview.

As for David's comment in Comment 12: I'm not sure where the logger output in Comment 10 came from, but it doesn't look like the output that would come from my patch:

https://github.com/mozilla-b2g/gaia/pull/24402/files#diff-a7156164db289d9e4fb818805c9bbc56R58

You'll note you should see the text "**** previewSizes ****" in the output and I don't see it in Comment 10. So, its possible that it is just a simple formatting issue in the logger output that Bhargav used.

In any case, for a screen size of 1920x1080 and an available preview size of 1920x1080, the "optimal" preview size would indeed be 1920x1080. So, we are correctly seeing expected behavior currently.
(In reply to David Flanagan [:djf] from comment #12)
> That debug log in comment #10 identifies the problem. The preview sizes are
> being passed as strings like "1280x720". But the code expects JS objects
> like { width: 1280, height: 720 }.
> 
> Something has changed somewhere in the driver or in gecko.
> 
> Mike: the camera app code is written assuming that
> mozCamera.capabilities.previewSizes is an array of objects. Has something
> changed that would cause it to be an array of strings?

The "W1xH1,W2xH2,..." string is the form in which the drivers provides the preview sizes. The quoted print in comment 10 dumps that string to the log (I think that's in the driver code). However in gecko we convert this into a dictionary array.
Flags: needinfo?(mhabicher)
It would be a bug if the preview size was smaller than the screen. Having a preview size that matches the screen exactly is the best possible size. So it appears that the camera is working exactly as intended.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WORKSFORME
on a side note where does camera store the last preview config items. i was trying out some changes and not able to revert the old changes unless i flash the entire image. 

Tried deleting the /data/local/storage/persistent/1006+camera... folder even that didnt help
(In reply to bhargavg1 from comment #17)
> on a side note where does camera store the last preview config items. i was
> trying out some changes and not able to revert the old changes unless i
> flash the entire image. 
> 
> Tried deleting the /data/local/storage/persistent/1006+camera... folder even
> that didnt help

I believe it is in localStorage, but I don't know where that equates to on the file system. Wilson might know of an easy way to clear it.
Flags: needinfo?(wilsonpage)
(In reply to Justin D'Arcangelo [:justindarc] from comment #18)
> (In reply to bhargavg1 from comment #17)
> > on a side note where does camera store the last preview config items. i was
> > trying out some changes and not able to revert the old changes unless i
> > flash the entire image. 
> > 
> > Tried deleting the /data/local/storage/persistent/1006+camera... folder even
> > that didnt help
> 
> I believe it is in localStorage, but I don't know where that equates to on
> the file system. Wilson might know of an easy way to clear it.

The data is stored in: `localStorage.cameraBootConfig`
This can be cleared within the app (via devtools/webIDE) with: `localStorage.removeItem('cameraBootConfig')`
Flags: needinfo?(wilsonpage)
blocking-b2g: 2.1? → ---
Flags: needinfo?(hkoka)
Resolution: WORKSFORME → INVALID
What device are you using?
Flags: needinfo?(bhargavg1)
(In reply to Diego Marcos [:dmarcos] from comment #20)
> What device are you using?

I am using 8x26 1080p device.

I am still not convinced that the preview size calculation should take only display size into consideration first. Looks like on Android [1] as well first we try to find the nearest aspect ratio that matches the picture size.

Matching directly the display size has side effect of memory as well, did we look into that side. 

when checked with internal camera team they say, 

"Its always good to use preview size which matches the picture size aspect ratio, so that there is no difference in FOV. Otherwise due to difference in FOV, what we see may be different from what we actually capture."

[1] https://www.codeaurora.org/cgit/quic/la/platform/packages/apps/Camera2/tree/src/com/android/camera/util/CameraUtil.java?h=LNX.LF.3.5.1_RB1.2#n493
Flags: needinfo?(bhargavg1)
(In reply to bhargavg1 from comment #21)
> (In reply to Diego Marcos [:dmarcos] from comment #20)
> > What device are you using?
> 
> I am using 8x26 1080p device.
> 

this is the same device which we have used for v2.0 as well
(re-opening as it seems like this has not been resolved yet)
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
(In reply to Michael Vines [:m1] [:evilmachines] from comment #23)
> (re-opening as it seems like this has not been resolved yet)

If we use the old method of calculating the "optimal" preview size, you'd end up with a 640x480 preview size scaled up to fill a 1920x1080 screen... In which case, someone else will file a bug claiming the preview is blurry. On a 1920x1080 screen device with an available camera preview size of 1920x1080, the best case scenario you could have is to use the 1920x1080 preview size.

Additionally, because of the UI design of the Camera app, we use an aspect-fill algorithm to ensure that the entire screen gets covered by the preview video stream. This, coupled with the fact that the previews are cropped from the center of the camera sensor means that the exact same content will appear at the exact same scale regardless of what size preview we use. Meaning, the aspect ratio of the picture size is completely irrelevant to what size the preview is (we must stretch to fill the screen size which in many cases is a different aspect ratio than the picture size).

Now, if we were not using a full-screen viewfinder, this would be a different scenario and we would indeed have to take the aspect ratio of the photo into consideration. In that case (and I believe this is true with the Android camera app), we would use an aspect-FIT algorithm to ensure the entire preview is visible on-screen. If the entire preview was visible on-screen, then yes, Comment 21 would be correct because we would not want to show/hide parts of the preview that wouldn't ultimately become part of the photo. But, with the current UI design of our Camera app calling for a cropped/truncated preview that FILLS the entire screen, this is not valid for us.

We have had bugs with every release since v1.4 that have claimed that the preview size we selected on a particular device was either too large or too small. This was mainly due to the fact that we were also trying to satisfy the condition of finding a matching aspect ratio to the photo size. Now that we've removed that constraint, nearly every device we've tested has been able to select a preview size that has enough pixels to fill the device screen without requiring scaling. If we go back to introducing the photo aspect ratio constraint, we are begging for future bugs on devices with a blurry viewfinder. AFAIK, there is nothing left to debate here and I would think it would be difficult to argue that a 1080p preview size is the wrong size for a 1080p display.

Ultimately, we may need to introduce a build-time setting to allow vendors to override our programmatic selection. If that is the case, we should open a new bug to allow Camera's preview size to be overridden at build time.
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → WONTFIX
One final thought about this bug. Bhavarg: in your build for the 1080p device, did you configure the camera to record videos at 1080p?  I suppose it might not make sense to display a crisp 1080p video preview when we are recording at 720p or at a lower resolution. If we do that, then the image will look worse during fullscreen playback than it does when we recorded it.

I suppose that ultimately this might be a UX question. But maybe our algorithm for selecting a preview size should be modified so that it never picks a size larger than the actual recording size.  What do you think Justin?
Flags: needinfo?(jdarcangelo)
Flags: needinfo?(bhargavg1)
(In reply to David Flanagan [:djf] from comment #25)
> One final thought about this bug. Bhavarg: in your build for the 1080p
> device, did you configure the camera to record videos at 1080p?  I suppose
> it might not make sense to display a crisp 1080p video preview when we are
> recording at 720p or at a lower resolution. If we do that, then the image
> will look worse during fullscreen playback than it does when we recorded it.
> 
> I suppose that ultimately this might be a UX question. But maybe our
> algorithm for selecting a preview size should be modified so that it never
> picks a size larger than the actual recording size.  What do you think
> Justin?

There's two solutions to this problem. In the case of a 1080p device, we could simply remove '1080p' from the excluded recorder profiles in config.js. Or, once Bug 1068231 is resolved, the preview size will finally be uncoupled from the recorder profile size.
Flags: needinfo?(jdarcangelo)
(In reply to David Flanagan [:djf] from comment #25)
> One final thought about this bug. Bhavarg: in your build for the 1080p
> device, did you configure the camera to record videos at 1080p?  I suppose
> it might not make sense to display a crisp 1080p video preview when we are
> recording at 720p or at a lower resolution. If we do that, then the image
> will look worse during fullscreen playback than it does when we recorded it.
> 
> I suppose that ultimately this might be a UX question. But maybe our
> algorithm for selecting a preview size should be modified so that it never
> picks a size larger than the actual recording size.  What do you think
> Justin?

In CAF builds we enable 1080p profile [1] specifically for the video encode case

[1] https://git.quicinc.com/?p=b2g/build.git;a=blob;f=patch/all/gaia/0001-Enable-recorderProfiles-menu-and-1080p-video-encode.patch;h=b323263c16a04da896dabd7a8598e4b6a603c388;hb=refs/heads/v2.1
Flags: needinfo?(bhargavg1)
(In reply to bhargavg1 from comment #27)
> (In reply to David Flanagan [:djf] from comment #25)
> https://git.quicinc.com/?p=b2g/build.git;a=blob;f=patch/all/gaia/0001-Enable-
> recorderProfiles-menu-and-1080p-video-encode.patch;
> h=b323263c16a04da896dabd7a8598e4b6a603c388;hb=refs/heads/v2.1

Oops wrong link, correct one 

https://www.codeaurora.org/cgit/quic/lf/b2g/build/tree/patch/all/gaia/0001-Enable-recorderProfiles-menu-and-1080p-video-encode.patch?h=v2.1
(In reply to Justin D'Arcangelo [:justindarc] from comment #24)

Although the 'core' Camera app uses 'aspect-fill', but partners (such as Madai) have chosen to use 'aspect-fit'. This is an option defined in the config, so we need to make sure any changes still consider the aspect-fit case.

I also wouldn't be surprised if one day we chose to go with the 'aspect-fit' case in core, so I think it's important that we continue to support it.
(In reply to Wilson Page [:wilsonpage] from comment #29)
> (In reply to Justin D'Arcangelo [:justindarc] from comment #24)
> 
> This is an option defined in the config

Wilson, can you point to the config that you are referring to?
Flags: needinfo?(wilsonpage)
(In reply to bhargavg1 from comment #30)
> Wilson, can you point to the config that you are referring to?

apps/camera/js/config/config.js > viewfinder.scaleType
Flags: needinfo?(wilsonpage)
Comment on attachment 8494776 [details]
TEST pull-request (v2.1)

test patch
Attachment #8494776 - Flags: feedback?(bhargavg1)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: