939047 - [B2G][Helix][camera][dingyu]The camera preview is not correct and the left of the screen have a black area in hd device

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


Actual results:

1. The camera preview is not correct and the left of the screen have a black area.
You can see the preview screenshot and correspond picture from the attachment.

In camera.js:
var screenWidth = document.body.clientHeight * window.devicePixelRatio;
var screenHeight = document.body.clientWidth * window.devicePixelRatio;
"window.devicePixelRatio" is added recently.
I am sure the  "window.devicePixelRatio" have caused the problem. But I am not very clearly know why?

I have do a test in Mozilla original version, the build id is :
gaia：ca94f480808a6a9d6cbb9116833a40f67ca55422
gecko：6922cdcfa9606b399d0fc195550fd76c819ad500



Expected results:

1. the preview is fine

Comment 1

[Wayne Chang [:wchang]]

there is no attachment? Please attach image and point out what's incorrect.

Comment 2

[lecky]

Attachment: 问题图像.zip

Comment 3

[Wayne Chang [:wchang]]

Hi John, please check this as discussed. Thanks.

Comment 4

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

This bug also happens to master.

Comment 5

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

The description about window.devicePixelRatio is incorrect. We introduce this to find more clear preview image. I believe to multiply window.devicePixelRatio is the correct behavior, but may be in wrong place.

Comment 6

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

Attachment: move devicePixelRatio to the correct place

The root cause of this bug is that the devicePixelRatio was used at wrong place. We only need to multiple it when comparing the image size not all the case. This patch moves devicePixelRatio to introduce two variable, deviceWidth and deviceHeight, for preview size filtering.

Comment 7

[David Flanagan [:djf]]

Comment on attachment 832834 [details] [review]
move devicePixelRatio to the correct place

John,

I think I'm the one that told Diego to make that change. I may also have been the reviewer for the patch.  Sorry that we broke this, and thank you for fixing it.

Comment 8

[David Flanagan [:djf]]

Diego,

I'm setting needinfo for you because it was your patch that caused this regression.  Please take a look at John's patch just so you understand the fix.  Obviously we didn't test well enough before landing this!

Comment 9

[Diego Marcos [:dmarcos]]

I recently got a Helix device and I missed this issue. Sorry for that. johnhu's patch looks good to me. Thanks for helping with this. Go ahead and merge it.

Comment 10

[lecky]

David, Deigo, John:

  I am afraid, if I merge the change according to attachment 832834 [details] [review], there is still some problem. 
  I will attach the screenshot of the preview.

Comment 11

[lecky]

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

Comment 12

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

Hi Lecky,

I know what you want to say, the black border at left and right. Wayne had asked me the same question when I showed the patch to him.

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.

The preview size matching algorithm may be improved but not in this bug. This is a regression bug.

I will land this patch.

[1]: https://github.com/mozilla-b2g/gaia/blob/e6c7af8f05c5aa68586bf6d4c21d5464d0cff917/apps/camera/js/camera.js#L1001

Comment 13

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

merged to master:
https://github.com/mozilla-b2g/gaia/commit/16682523182fa4d047b3ffb63059dd45732078ee

Comment 14

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

I will continue to help to land to v1.1.0hd, since it is a hd+ bug.

Comment 15

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

merged to v1.1.0hd:
https://github.com/mozilla-b2g/gaia/commit/c841954b1b19e628efba5262fe7a0f2dad9a71c9

Comment 16

[lecky]

Thanks John, I am afraid if the merge the patch https://github.com/mozilla-b2g/gaia/commit/c841954b1b19e628efba5262fe7a0f2dad9a71c9, our test department can not accept this modified effect.

So currently, I plan that I don't merge the the patch to our code and 

-----------------------------------------------------------------------------------------
var screenWidth = document.body.clientHeight * window.devicePixelRatio;
var screenHeight = document.body.clientWidth * window.devicePixelRatio;

to

var screenWidth = document.body.clientHeight;
var screenHeight = document.body.clientWidth;
-----------------------------------------------------------------------------------------
Is there any bad effect?

(In reply to John Hu [:johnhu] from comment #14)
> I will continue to help to land to v1.1.0hd, since it is a hd+ bug.

Can you give any schedule for this bug？

Thanks!

Comment 17

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

Lecky,

The side effect is your viewfinder isn't as clear as its native resolution. That's the original version before bug 931125 landed. After this patch landed, you may find the screen is so clear and runs in its native resolution to preview the image.

As per comment 10, 11, 12, I know you want to have a fullscreen viewfinder, with native resolution, and maintaining the aspect of picture resolution. But, according to our algorithm, what you see is the current result.

IMHO, this patch doesn't fixed the algorithm of choosing preview size. If you feel something uncomfortable, you should file another bug and tell us what you thought. After some discussions and resource allocation, we may implement it more close to what you thought.

Comment 18

[lecky]

Thank John, I will file a new bug to track this bug！