Closed Bug 969761 Opened 7 years ago Closed 7 years ago

Camera viewfinder resolution is not the size of the screen

Categories

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

defect

Tracking

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

RESOLVED FIXED
1.4 S1 (14feb)
blocking-b2g 1.3+
Tracking Status
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- unaffected

People

(Reporter: diego, Assigned: djf)

References

Details

(Whiteboard: [caf priority: p2][CR 580386])

Attachments

(1 file)

46 bytes, text/x-github-pull-request
justindarc
: review+
diego
: feedback+
Details | Review
In bug 939679 the camera viewfinder calculations were changed to assume the available viewfinder resolutions are in CSS pixels. This is not true in the two devices which I have.

In fact, I'm pretty sure the camera gonk always talks in device pixels. So I'm doubtful that any device uses CSS pixels for its camera viewfinder resolution.

The problem here is when the camera requests a viewfinder in CSS pixels (not device pixels), you won't get frames of the same resolution as the device screen.
blocking-b2g: --- → 1.3?
No longer blocks: 942199
> you won't get frames of the same resolution as the device screen.

Can you please elaborate on why this should be considered to be blocking?
dwilson: what is the devicePixelRatio of your device?  And what do you see on the screen when you run the existin code? Is the size of the preview image visibly wrong, or is it sized and positioned correctly but not sharp enough? Would you attach the patch you used to correct the problem?

In bug 942199, it sounded like you were saying that the preview was actually at the wrong size. The nexus-4 has a devicePixelRatio of 2 and the 1.3 code seems to work for it as it stands: the preview stream is at the right place.  I think I checked using device pixels instead of css pixels and found that it did not make the viewfinder image any sharper, but I'd have to check that again to verify.

mikeh: could you tell us whether gecko does any translation of preview streams sizes to account for devicePixelRatio?  On the helix, for example, does it divide preview sizes by 1.5 to convert from device pixels to CSS pixels before reporting the sizes to JavaScript?

dmarcos, wilson and justin: I'm setting needinfo for all of you to get this bug on your radar since it is a followup to a 1.3+ blocker and may become a blocker itself.  I can't remember why we removed the multiplication by devicePixelRatio in bug 939679. But if the Helix behaves differently than dwilson's reference device then we'll probably need to define another build-time configuration option that allows us to hardcode the preview stream size.
Flags: needinfo?(wilsonpage)
Flags: needinfo?(mhabicher)
Flags: needinfo?(jdarcangelo)
Flags: needinfo?(dwilson)
Flags: needinfo?(dmarcos)
Looking over 939679, I see that I'm responsible for that patch and that I based it on my own eyes, and figured that the viewfinder image must be sharp enough because it was sharp enough for our partners on the Helix device.  I really didn't see any difference in image quality, though, or wouldn't have landed the patch.

dwilson: to fix this, it would be really helpful to have the complete list of supported preview sizes on your hardware.  In camera.js, in setPreviewSize() could you do a console.log(JSON.stringify(camera.capabilities.previewSizes)); and report the output you see in logcat?

Assigning this to myself for now, so I don't forget about it, but I might pass it on to Justin or dmarcos.
Assignee: nobody → dflanagan
David: I can take this off your plate if you'd like. Also, I believe the rule of thumb for when to use devicePixelRatio is when you have a visual media that you want to be as sharp as possible, you should get a source image/video that is the viewport dimensions multiplied by devicePixelRatio. Even though, in CSS you size the image/video to the reported viewport size as-is. This results in "squeezing" more pixels into the viewport area and ultimately maps the individual device's pixels up 1:1. But, I still have to second guess myself everytime the devicePixelRatio calculation comes up.
Flags: needinfo?(jdarcangelo)
(In reply to Michael Vines [:m1] [:evilmachines] from comment #1)
> > you won't get frames of the same resolution as the device screen.
> 
> Can you please elaborate on why this should be considered to be blocking?

Hi Michael,

Unless this bug is fixed bug 942199 may not truly be solved.

Bug 942199 depends on the thumbnail having a size that is reasonably close to the screen size. Currently on my WVGA 800x480 device the camera app selects a 512x288 preview which will result on a 384x288 thumbnail. This may be too grainy for using as the gallery preview.

If this bug is fixed the thumbnail will be 640x480.
Flags: needinfo?(dwilson) → needinfo?(mvines)
Diego: the dependent bug 942199 is marked as "resolved fixed".
It's probably worth noting that the logic to decide preview sizes has been shifted out of camera.js following my settings framework work. New architecture can be seen in https://github.com/wilsonpage/gaia/tree/image-video-sizes. 

This branch is currently being worked on. It was branched from the initial settings framework branch (bug/960611) and focuses on pre-selecting/restricting image/video size settings based on incoming 'pick' activity parameters.
Flags: needinfo?(wilsonpage)
Blocks: 942199
Status: NEW → ASSIGNED
Whiteboard: [CR 580386]
(In reply to David Flanagan [:djf] from comment #2)
> 
> mikeh: could you tell us whether gecko does any translation of preview
> streams sizes to account for devicePixelRatio?  On the helix, for example,
> does it divide preview sizes by 1.5 to convert from device pixels to CSS
> pixels before reporting the sizes to JavaScript?

djf, the sizes reported by the CameraControl API are all device pixels as reported by the camera library, with no translation of any kind.
Flags: needinfo?(mhabicher)
djf,

I changed the screen dimensions calculation to use device pixels instead of CSS pixels [1].

https://github.com/mozilla-b2g/gaia/blob/e439f8630fdea790595efaebab24ec21be6bc0e4/apps/camera/js/camera.js#L1037
Maybe that's enough here?
Flags: needinfo?(dflanagan)
Diego,

No, that's not enough because it just reverts bug 939679, and will break the camera viewfinder on the Helix device.

Also: would you answer my questions in comment #2 and comment #3, please?
Flags: needinfo?(dflanagan)
Nevermind, Diego. I think I can fix this without more answers.

Bug 939679 was occuring because the Helix device does not offer a 4:3 preview big enough to cover the screen when we measure the screen in device pixels. So since we couldn't find a preview that was the right aspect ratio and big enough, we just picked the first size offered. This turned out to be a bad aspect ratio and the viewfinder didn't look right.

The OEM for the Helix device fixed this by just switching to CSS pixels, and I followed suit. If we used CSS pixels there was a preview big enough, and it looked completely sharp, so I assumed that the preview sizes must have been divided by device pixel ratios and that using CSS pixels was the right thing to do.

We've since learned that device pixels are the right ones to use here. No one seems to know why the preview stream looks as good as it does when we use CSS pixels, but I'll just leave that as a mystery.

The patch I'm about to attach goes back to using device pixels. But if there is not a preview at the right aspect ratio that is also big enough to cover the screen (when measured in device pixels) it just picks the largest preview size that has the correct aspect ratio.

On a hamachi device, the preview size is 576x432 with and without this patch.

On a nexus 4, the preview size is 640x480 with and without this patch. (This is another phone that does not offer large 4:3 previews, but the stream looks remarkably sharp anyway.)

On the helix, we used 576x432 without the patch, but use 640x480 with it, which is an improvement!

Since this patch includes the change (using device pixels instead of css pixels) that dwilson wanted, I'm assuming that it will fix the bug on his device.
Justin: this is the patch we discussed on IRC. It is against the 1.3 branch. Would you review please?

dwilson: please confirm that the attached patch resolves the bug for you
Attachment #8373806 - Flags: review?(jdarcangelo)
Attachment #8373806 - Flags: feedback?(dwilson)
dwilson: the 1.4 camera has completely different code for selecting preview size. So if it is convienient, you might try your device on 1.4 as well just to see if the size calculations come out right there.

justin: I think you're making a mistake on the 1.4 branch by not forcing the preview size to be the same aspect ratio as the picture size as we do in previous releases. You might want to try 1.4 out on a Helix to see what preview size you're getting and to confirm that bug 939679 has not come back.

mvines: we really do need to land this in 1.3, and your + would be the easiest way to do that.
Flags: needinfo?(dmarcos)
justin: this bug isn't yet marked as 1.3+, but I'd appreciate it if you'd consider the review a high priority
David: No problem. I'll take a look either later tonight or early tomorrow.
hth! :)
blocking-b2g: 1.3? → 1.3+
Flags: needinfo?(mvines)
Comment on attachment 8373806 [details] [review]
link to patch on github

Looks good to me.
Attachment #8373806 - Flags: review?(jdarcangelo) → review+
Target Milestone: --- → 1.4 S1 (14feb)
Comment on attachment 8373806 [details] [review]
link to patch on github

Works!
Attachment #8373806 - Flags: feedback?(dwilson) → feedback+
Merged into 1.3: https://github.com/mozilla-b2g/gaia/commit/6722336310bc08ace9dc018e6b7aa179f3967d47

I don't think we need to fix anything on master since there is completely different preview selection code on that branch and it already uses device pixels.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: 972142
Flags: in-moztrap?
Flags: in-moztrap? → in-moztrap-
Whiteboard: [CR 580386] → [caf priority: p2][CR 580386]
You need to log in before you can comment on or make changes to this bug.