the creation of thumbnail size is not compatible to hdpi device

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: johnhu, Assigned: johnhu)

Tracking

unspecified
x86
Mac OS X

Firefox Tracking Flags

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

Details

Attachments

(3 attachments)

The thumbnail size is calculated with 210 * window.innerWidth / 320 and 120 * window.innerWidth / 320. When we use HDPI device (1.5x), the width/height may not be sufficient. It should multiply window.devicePixelRatio to create more sharpened image
blocking-b2g: --- → hd?
Assignee: nobody → johu
Created attachment 782500 [details]
use devicePixelRatio to get the correct image size

The original implementation about scale ratio may be incorrect.First, It uses window.innerWidth / 320 to calculate the ratio. But in HDPI device, the window.innerWidth is still 320 (css pixel), but the real device pixel is 480. Second, this implementation doesn't take care about orientation of device. When the creation of thumbnail executing, it uses pre-calculated width and height based on scale ratio to create thumbnail. So, the thumbnail will always be 210x120, if it is in portrait mode. If it is in landscape mode, thumbnail will always be 315x180. But in css, it is configured as 21rem x 12rem and centered in the container. So, the thumbnail created in landscape mode will be cropped. This doesn't make sense.

This patch uses devicePixelRatio to calculate the scale ratio. When user uses non-hdpi device, this value is 1 and the thumbnail will be 210x120. When user uses hdpi device, this value is 1.5 and the thumbnail will be 315x180. In css, I configured it as 100% in background-size which makes thumbnail to scale to full size of its container which is 21rem x 12rem.

The attachments are single preview image and its screenshot.
Attachment #782500 - Flags: review?(rexboy)
Attachment #782500 - Flags: review?(dflanagan)
Created attachment 782501 [details]
device screenshot after patched
Created attachment 782502 [details]
created thumbnail image.
Attachment #782501 - Attachment description: device-screen-shot.png → device screenshot after patched
Comment on attachment 782500 [details]
use devicePixelRatio to get the correct image size

I'm OK with this patch.

Thanks for your work!
Attachment #782500 - Flags: review?(rexboy) → review+
HD+ UI BUG
blocking-b2g: hd? → hd+
Comment on attachment 782500 [details]
use devicePixelRatio to get the correct image size

This is a nice simple patch. Thanks for explaining it clearly. I get confused with the high dpi stuff.  Also, nice catch about the bug when the video app is launched in landscape mode!

I have one very minor nit on github, and suggest that you test this patch on a high dpi device with a very small video, like a video recorded as an MMS attachment.  If the video is smaller than the thumbnail size, does it do the right thing?
Attachment #782500 - Flags: review?(dflanagan) → review+
Flags: needinfo?(johu)
merged to master:
 https://github.com/mozilla-b2g/gaia/commit/49b1f68b0e2152557bb7c2defb2a5f9d7a0648c6
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: needinfo?(johu)
Resolution: --- → FIXED

Comment 8

5 years ago
Hi John, do we need this patch on v1.1hd as well?
Flags: needinfo?(johu)

Updated

5 years ago
status-b2g-v1.1hd: --- → affected
Keywords: checkin-needed
merged to v1.1.0hd:
https://github.com/mozilla-b2g/gaia/commit/9036cbea337897593779cb053ae285acc3201e43
status-b2g-v1.1hd: affected → fixed
Flags: needinfo?(johu)
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.