939679 - [B2G][helix][camera][dingyu]The 1.1.0hd camera does not have a fullscreen viewfinder

Status: RESOLVED FIXED

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

Description

[lecky]

User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/535.1 (KHTML, like Gecko) Chrome/13.0.782.112 Safari/535.1

Steps to reproduce:

1. open the camera
2. check the preview


Actual results:

1. there are  black border at left and right in viewfinder


Expected results:

1. the camera should have a fullscreen viewfinder


the build id is :
gaia：ca94f480808a6a9d6cbb9116833a40f67ca55422
gecko：6922cdcfa9606b399d0fc195550fd76c819ad500
and the patch https://github.com/mozilla-b2g/gaia/commit/c841954b1b19e628efba5262fe7a0f2dad9a71c9 is merged

In my opinion, if the width/height ratio of preview is 4:3, but the  width/height ratio of screen is 5:3. It hard to make a fullscreen viewfinder. 
Compare to Android OS and WinPhones OS, when this different ratio happens, there are  black border at up and down. and it seem's ok.
but firefox UI is different with them,so this bug will leads to a bad user experience

Comment 1

[John Hu [:johnhu][:johu][:醬糊小弟]]

The reason of having black border at left and right is at bug 939047 comment 12.

Comment 2

[lecky]

Attachment: 1980-01-07-01-14-50.png

Comment 3

[Wayne Chang [:wchang]]

John,

I'll let you explain the black strips here? :) thanks

Comment 4

[John Hu [:johnhu][:johu][:醬糊小弟]]

According to the algorithm in camera.js, we will search the best resolution which is smallest but larger than screen size and with the similar width/height ratio[1]. If none of them find, we use the first preview resolution to show the image.

In the default case, I mean no customized resolution, the resolution of picture is 1536x2048 whose width/height ratio is 0.75. The screen resolution is 800x480. We iterate all preview size from 1280x720, 864x480, 800x480, etc. Only 1280x720, 864x480, 800x480 are larger than or equal to screen solution. But the width/height ratio of all of them are not close to 0.75, they are: 0.56, 0.56, 0.6. The "close" word means their difference is less than 0.05. So, we cannot find one and use 1280x720 to show the preview. When we fit the 1280x720 preview into the screen 800x480, the resolution is 800x450 and leaves 15px on each side.

Comment 5

[Wayne Chang [:wchang]]

Thanks Diego.

If we have to trim sides of the image off to get full screen, that's what we already do on other v1.1 release devices.

Comment 6

[John Hu [:johnhu][:johu][:醬糊小弟]]

Hi all,

We may need to face different kind of form factor, like helix is 320x533, but others are 320x480. If we support more and more devices, we may face multiple form factors. Do we need to ask for UX input for a general rule on this kind of situations??

Comment 7

[William Hsu [:whsu]]

Change the bug status.

Comment 8

[Wayne Chang [:wchang]]

Diego, how are we looking here? Can you give some update on this bug? thanks :)

Comment 9

[Hema Koka [:hema]]

It looks like this is an enhancement request for the preview matching algorithm per John's comments. Is this an absolute blocker for 1.1 HD? It seems risky to make such changes this late in the cycle. 

NI Sri for product input on this bug. 

Also I believe UX is looking into recommendations as part of camera enhancements for 1.4. 

Thanks
Hema

Comment 10

[Wayne Chang [:wchang]]

If we can get some efforts/risks assessment here we can push back on this, but when I last spoke to dmarcos on IRC about this he had some ideas.

Comment 11

[David Flanagan [:djf]]

(In reply to John Hu [:johnhu] from comment #4)

> So, we cannot find one and use 1280x720 to show the preview. When
> we fit the 1280x720 preview into the screen 800x480, the resolution is
> 800x450 and leaves 15px on each side.

Then there is a bug in the code that does the fit. We should zoom so that we get a preview that is 480 wide and larger than 800 high and chop a bit of the preview off the top and the bottom.  I thought that is what we already did.  Did something change and regress this, or is there just a bug in the preview fitting code that went unrecognized until we tried it at this form factor?

I have not looked at the code, but I would guess that the fix is a trivial adjustment to our arithmetic.  We probably need to replace a Math.max with a Math.min or something.

On the other hand, I've been using a helix for a few weeks now and never noticed the 15px margins. They are black surrounded by a black phone so I don't see them. Because the buttons are translucent overlays, it doesn't actually look bad to me. Trying it out now, the only time I notice the margins is when the filmstrip is showing.  So I question the reporter's assertion in comment 0 that this is a "bad user experience".  This doesn't strike me as a real blocker.

Comment 12

[Hema Koka [:hema]]

Wayne,

Per David's analysis, it is a trivial fix if it is done on the hd branch, but this does not strike as a blocker (just going by the blocking criteria and the fact that it is hardly noticeable on a black phone). If you also agree, we can address this on master.

Thanks
Hema

Comment 13

[Wayne Chang [:wchang]]

I am discussing this with partner base on the above comments, will update here when some conclusion is reached.
They are seeing this as a regression as previously we had it full screen (although probably slightly blurred) but now we have a narrower preview with more of the actual photo area cropped off two sides.

Comment 14

[David Flanagan [:djf]]

Taking this bug from Diego who seems to have forgotten about it.

Comment 15

[David Flanagan [:djf]]

If I reflash my helix with the latest firmware (20131217) I no longer see this bug. So it looks like our partner has fixed it on their own.

So I don't know if a fix is required to be uplifted to 1.1hd.  But we should still probably fix it in our tree.

Right now we are multiplying the screen size by devicePixelRatio and trying to find a preview of the right aspect ratio that fits is that big.  There isn't a 3:4 preview that is larger than 480x800 and we end up with something the wrong size.

If I remove the code that multiplies by devicePixelRatio the bug goes away, and I get a 3:4 preview size that is bigger than 320x533. I would have thought that the preview would not look crisp because it was being resized. But it appears just as sharp. So maybe in this case, the preview sizes being reported by the camera hardware are already divided by the device pixel ratio and we can treat them as CSS pixels.

After updating to the latest firmware from our partner and flashing gaia/gecko master to the phone, adb logcat no longer works.  So I can't debug this anymore on master.

I'll try working on the 1.1hd branch directly, I suppose.

Comment 16

[David Flanagan [:djf]]

Still working on getting a Helix with a working logcat.  But for now I notice that the application.zip file for the camera app in the lastest firmware has exactly the fix I propose above. It just gets rid of the devicePixelRatio code.

So if the preview is crisp enough for our partners, then I think that is the right way to fix this.  (Too bad they didn't contribute their fix back to us in this bug!)

Comment 17

[David Flanagan [:djf]]

Attachment: patch for the v1.1.0hd branch

This patch brings our camera code into sync with the code our partner is shipping on the Helix.  It might be worth applying if only for that reason.

Comment 18

[David Flanagan [:djf]]

Attachment: patch for master

Diego,

This is the same patch as the other one, but against master this time.

Comment 19

[David Flanagan [:djf]]

Its a trivial fix.  It looks like it is already fixed in our partner's version of the 1.1hd branch, but they just didn't contribute it back to us.

Obviously we want to fix this on master.  Where else should we uplift it to, though?

I'll leave that for the release management folks.

Comment 20

[David Flanagan [:djf]]

I have no idea whether this bug should retain its hd+ flag, since our partner has already fixed it in their build.

If not, do we make it koi+ and uplift the fix to 1.2 and 1.3?

Someone please figure it out and apply the right patches!

Comment 21

[lecky]

(In reply to David Flanagan [:djf] from comment #16)
> So if the preview is crisp enough for our partners, then I think that is the
> right way to fix this.  (Too bad they didn't contribute their fix back to us
> in this bug!)
David, thanks for your work, in 1.1hd version, we didn't fixed by myself, we just do not merge 1.1hd new patch about devicePixelRatio. attachment 8350825 [details] [review] is mozilla's old proposal.

By the way, taking into account our current program，it still have a problem, you can see Bug 896339

Comment 22

[Sri Kasetti]

We should definitely pull this into 1.3

Comment 23

[David Flanagan [:djf]]

Landed on the 1.1.0hd branch: https://github.com/mozilla-b2g/gaia/commit/a2858cef75fb65bd3b78b66c23349fd18252dd84

Waiting for travis to re-run before landing on master.

Comment 24

[David Flanagan [:djf]]

Landed on master: https://github.com/mozilla-b2g/gaia/commit/4245fb93e747d3e3cc9e50ba6fa2f084c9cb682f

Comment 25

[David Flanagan [:djf]]

Uplifted to v1.3: https://github.com/mozilla-b2g/gaia/commit/6116df369eaf209348e0d944ab09808dad8c800f

Comment 26

[David Flanagan [:djf]]

Uplifted to v1.2: https://github.com/mozilla-b2g/gaia/commit/1e8bca8f06a23208e32504300bb612275d8e573c