351.32 KB, image/png
46 bytes, text/x-github-pull-request
|Details | Review | Splinter Review|
46 bytes, text/x-github-pull-request
|Details | Review | Splinter Review|
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
Severity: normal → critical
Priority: -- → P1
The reason of having black border at left and right is at bug 939047 comment 12.
John, I'll let you explain the black strips here? :) thanks
Flags: needinfo?(wchang) → needinfo?(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. 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.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Diego, how are we looking here? Can you give some update on this bug? thanks :)
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
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.
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
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 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!)
Created attachment 8350825 [details] [review] 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.
Attachment #8350825 - Flags: review?(dmarcos)
Created attachment 8350826 [details] [review] patch for master Diego, 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.
status-b2g-v1.1hd: --- → affected
status-b2g-v1.2: --- → affected
status-b2g-v1.3: --- → affected
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
Attachment #8350825 - Flags: review?(dmarcos) → review+
Attachment #8350826 - Flags: review?(dmarcos) → review+
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.
status-b2g-v1.1hd: affected → fixed
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-b2g-v1.2: affected → fixed
status-b2g-v1.3: affected → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.