Closed Bug 939679 Opened 7 years ago Closed 7 years ago

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


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



(blocking-b2g:hd+, b2g-v1.1hd fixed, b2g-v1.2 fixed, b2g-v1.3 fixed)

blocking-b2g hd+
Tracking Status
b2g-v1.1hd --- fixed
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed


(Reporter: lecky.wanglei, Assigned: djf)




(3 files)

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 :
and the patch 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
Severity: normal → critical
Flags: needinfo?(wchang)
Priority: -- → P1
blocking-b2g: --- → hd?
The reason of having black border at left and right is at bug 939047 comment 12.
Attached image 1980-01-07-01-14-50.png

I'll let you explain the black strips here? :) thanks
Flags: needinfo?(wchang) → needinfo?(johu)
Assignee: nobody → dmarcos
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.
Flags: needinfo?(johu)
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.
blocking-b2g: hd? → hd+
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??
Change the bug status.
Ever confirmed: true
Diego, how are we looking here? Can you give some update on this bug? thanks :)
Flags: needinfo?(dmarcos)
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. 

Flags: needinfo?(skasetti)
Flags: needinfo?(rmacdonald)
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.
(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.

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.

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.
Taking this bug from Diego who seems to have forgotten about it.
Assignee: dmarcos → dflanagan
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.
Still working on getting a Helix with a working logcat.  But for now I notice that the 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!)
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.
Attachment #8350825 - Flags: review?(dmarcos)
Attached file patch for master

This is the same patch as the other one, but against master this time.
Attachment #8350826 - Flags: review?(dmarcos)
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.
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!
(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
We should definitely pull this into 1.3
Flags: needinfo?(skasetti)
Attachment #8350825 - Flags: review?(dmarcos) → review+
Flags: needinfo?(dmarcos)
Attachment #8350826 - Flags: review?(dmarcos) → review+
Landed on the 1.1.0hd branch:

Waiting for travis to re-run before landing on master.
Closed: 7 years ago
Resolution: --- → FIXED
Flags: needinfo?(rmacdonald)
Blocks: 969761
You need to log in before you can comment on or make changes to this bug.