[FTE] Videos showing up very blurred

VERIFIED FIXED in Firefox OS v2.0

Status

VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: epang, Assigned: sfoster)

Tracking

unspecified
2.0 S6 (18july)
x86
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:2.0+, b2g-v2.0 verified, b2g-v2.1 verified)

Details

(Whiteboard: [systemsfe])

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Hi Sam,

Filing this as a follow up to bug 1019289.
We need to figure out what's causing the videos to be blurry.  You said earlier you don't think it's the assets themselves, if any work needs to be done with the assets please let me know.
Whiteboard: [systemsfe]
Target Milestone: --- → 2.0 S4 (20june)
(Assignee)

Comment 1

5 years ago
Eric, I think what we a probably seeing here is the effect of scaling the <video> to fit the layout - which is variable depending on the device. Currently, we have a fixed height for the step text, and fill the remainder of the height with the video: 

#tutorial-step-media {
  height: calc(100% - 9.5rem);
}

.step-video-base {
  height: 100%;
}

I'll do some work to confirm if the blurriness we are seeing is soley attributable to this scaling. If that is the case we'll need to consider our options: which screen sizes do we optimize for and how.
(Assignee)

Comment 2

5 years ago
Created attachment 8443739 [details]
ftu-tutorial-2.png - screenshot from Flame, pvt build

For reference, I flashed gaia+gecko using the auto_flash_from_pvt.sh tool and took this screenshot of the FTU tutorial. There is some blurring visible. I'll upload a screenshot of how it looks in desktop firefox for comparison
(Assignee)

Comment 3

5 years ago
Created attachment 8443741 [details]
screenshot from desktop firefox, <video> scaled to match dimensions on Flame

This is screenshot of Firefox playing the same mp4. I scaled the <video> element to 270px/404px to get like-for-like comparison; the result is sharper than we see on-device.
(Note that if I set height to 404 and width auto, width ends up at 269px, however the blurriness still doesn't reproduce so I'm not sure this is relevant)
(Assignee)

Comment 4

5 years ago
I think the attachments rule out scaling as the sole issue here. I notice if I browse on the Flame to the same mp4 on github, I see the same blurriness: https://raw.githubusercontent.com/mozilla-b2g/gaia/master/apps/ftu/style/images/tutorial/Notifications@1.5x.mp4

Could this be hardware issue, are there any dials we can tweak at all to get sharper rendering on device?
Flags: needinfo?(dflanagan)
If this actually a gecko issue with how the <video> tag is rendering your video, then I have no clue. I know about the Gaia video app, but it just uses the a video element. If there is a bug in video rendering, it would be something to ask about in the #media IRC channel.

The first thing I'd check here, though, is whether the correct video file is actually being copied into your application.zip file. I'm not sure whether the build scripts that do the @1.5x image magic also work for .mp4 files.  If they are working right, then your @1.5x.mp4 should be copied into the application.zip file without the @1.5x in it.

If you want to put some console.log statements in the app, you could print out the .width and .height of the video element: these should give you the size in CSS pixels of the element itself.  Then also print out .videoWidth and .videoHeight (after you get a playing event or a metadataloaded event) to find out how big the video is.  You want the video frames to be as big as the element width and height times window.devicePixelRatio
Flags: needinfo?(dflanagan)
(Assignee)

Comment 6

5 years ago
djf, I had the same thought about the @1.5x.mp4 file possibly not being copied correctly. And also with scaling as a result of the size of <video> element. But as I can reproduce the issue by loading the correct .mp4 directly via github - I'm pretty sure we can rule out both those possibilities - at least as the sole causes.

OTOH this sample .ogv looks pretty good to my eyes (sharp, comparable to how it renders on desktop firefox) when viewed on the Flame: https://dl.dropboxusercontent.com/u/1719101/share/sample.ogv - so its possible using a different codec will yield better results.
(Assignee)

Comment 7

5 years ago
Eric, if you can also reproduce the blurriness we are seeing by just browsing on your Flame to https://raw.githubusercontent.com/mozilla-b2g/gaia/master/apps/ftu/style/images/tutorial/Notifications@1.5x.mp4, then can put stuff in box/dropbox/flickr/wherever and see if the .mov files or other codecs/formats have the same issue? I tried the box.com link for the .mov files you emailed but it told me those files had been removed.
Flags: needinfo?(epang)
(Reporter)

Comment 8

5 years ago
(In reply to Sam Foster [:sfoster] from comment #7)
> Eric, if you can also reproduce the blurriness we are seeing by just
> browsing on your Flame to
> https://raw.githubusercontent.com/mozilla-b2g/gaia/master/apps/ftu/style/
> images/tutorial/Notifications@1.5x.mp4, then can put stuff in
> box/dropbox/flickr/wherever and see if the .mov files or other
> codecs/formats have the same issue? I tried the box.com link for the .mov
> files you emailed but it told me those files had been removed.

Hey Sam, can't seem to get the github link to play the video on the device.  But the 2x version worked, it was slightly blurry but not as bad as on the FTE screen.

I must have moved the mov files, sorry about that.  Here's an updated link:
https://www.dropbox.com/sh/xctd9hcedzotuk2/AADpHCH62bJz32j6hGmPC4b_a

Going to the link I wasn't able to play the video files (only gave me the option to download them.  But judging from the preivew images they were pretty distorted/blurry.
Flags: needinfo?(epang) → needinfo?(sfoster)
Target Milestone: 2.0 S4 (20june) → 2.0 S5 (4july)
Target Milestone: 2.0 S5 (4july) → 2.0 S6 (18july)
(Assignee)

Updated

4 years ago
Blocks: 1019289
(Assignee)

Comment 9

4 years ago
Created attachment 8453321 [details]
1.5x FTE video Open-C screenshot

Open C has 1.65ppi, 320x533px. With the scaling we do to ensure it fits on screen with the title, the video ends up at 246px * 368px, scaled from 204 x 306. This screenshot is the result when I manually set the video src to the @1.5x.mp4. I think its better? (ignore the font issue) If that's the case then it seems the @ suffixed assets arent getting the same treatment as images and we're always using the low res.
Flags: needinfo?(sfoster) → needinfo?(epang)
(Assignee)

Comment 10

4 years ago
Looks like the post-app build step only looks for images, at: https://github.com/mozilla-b2g/gaia/blob/master/build/image-resolution.js#L21, so our @1.5x.mp4 etc files are just being ignored. Alexandre, what do you think? I suspect if we add .mp4 to that regex it might come as a surprise - soley because of the module and method names. But the result would be what we want to happen here I think - to extend the GAIA_DEV_PIXELS_PER_PX treatment to video media assets.
Flags: needinfo?(apoirot)
(Assignee)

Comment 11

4 years ago
More context: https://github.com/mozilla-b2g/gaia/tree/master/apps/ftu/style/images/tutorial shows mp4 files for the FTU tutorial at 1x, 1.5x, and 2x (we still need to add 2.25x). Currently these are all getting copied over in the build, but the 1x asset is being used always. 

A quick workaround (this bug blocks 2.0) would be to use say the 2x video and let it scale - we are scaling anyhow to ensure it fits the available height. I guess the "proper" fix is to either rename ImageResolution to something media-agnostic and add video file extensions to the regex in pickImageByResolution, or to add a VideoResolution / video-resolution module into post-app that does the same thing.
(Assignee)

Comment 12

4 years ago
Also flagging Yuren Ju, I'm not sure which of you owns this area.
Flags: needinfo?(yurenju.mozilla)
Last time I did something around 2x files was loooong time ago during 1.0 development...
But what you are suggesting makes sense to me: renaming it to MediaResolution and extend the regexp at line 21 to include videos. If that just work, it looks like a simple enough modification for uplift.
I'm puzzled we are copying at least one (and may be many?!) duplicated videos. Given that our image size is very limited on some devices, That's surprising how we missed such easy optimization of the final build size.
Flags: needinfo?(apoirot)
(Assignee)

Comment 14

4 years ago
ok I'll put a patch together along those lines. Eric, I still need your confirmation that the 1.5x screenshot looks good - that's what we'll end up with if I do this.
Flags: needinfo?(yurenju.mozilla)
(Assignee)

Comment 15

4 years ago
Created attachment 8454113 [details] [review]
PR to add video file handling to image-resolution post-app build step

* ImageResolution becomes MediaResolution, with the regex expanded to include a set of web-friendly video file extensions
* image-resolution.js becomes media-resolution
* New test to sanity-check that pickMediaByResolution handles .mp4
* Includes fix to sort directory listing before processing(!) - fixes a nasty error where files could get removed and omitted entirely from the application.zip

I don't know how node sorts its listings by default or why this just showed up for me, but when you get 

VerticalScroll@2x.mp4
VerticalScroll@1.5x.mp4
VerticalScroll.mp4

the result was that VerticalScroll@1.5x.mp4 was copied to VerticalScroll.mp4 and then VerticalScroll.mp4 was removed. Sorting on .path before looping fixes that.
Attachment #8454113 - Flags: review?(poirot.alex)
(Reporter)

Comment 16

4 years ago
(In reply to Sam Foster [:sfoster] from comment #9)
> Created attachment 8453321 [details]
> 1.5x FTE video Open-C screenshot
> 
> Open C has 1.65ppi, 320x533px. With the scaling we do to ensure it fits on
> screen with the title, the video ends up at 246px * 368px, scaled from 204 x
> 306. This screenshot is the result when I manually set the video src to the
> @1.5x.mp4. I think its better? (ignore the font issue) If that's the case
> then it seems the @ suffixed assets arent getting the same treatment as
> images and we're always using the low res.

Hey Sam, it's better but still looks blurry in the screen.  Do we need to scale or is it possible to use the asset that is closest to the size and adjust on the UI (if not too far off - if the sizes are too different this probably won't work).  But either way we should always scale down if it has to be done.  Am I able to test this on the flame, might look different on a device :).
Flags: needinfo?(epang) → needinfo?(sfoster)
(Assignee)

Comment 17

4 years ago
We might be able to resize/recreate the videos closer to the target size on the reference device in most locales, but the video height is dynamically assigned based on whatever vertical space is left between the step description text at the top and the buttons at the bottom. So it will vary based on device screen height and also locale - we may have more or less lines of text depending on language. IOW its not practical to know in advance what height/width the video will be, the best we can do is always scale down rather than up. I think the current screenshot represents that best effort, but we'll take a look at it in person at the work week to see if its good enough
Flags: needinfo?(sfoster)
(Assignee)

Comment 18

4 years ago
Fixed a couple of lint/style errors and updated PR. Try run is at: https://tbpl.mozilla.org/?rev=6ccece0faf5699ad7035196ee858d96f198e01c6&tree=Gaia-Try
(Assignee)

Comment 19

4 years ago
Comment on attachment 8454113 [details] [review]
PR to add video file handling to image-resolution post-app build step

Whichever of you can get to this first, thanks.
Attachment #8454113 - Flags: review?(yurenju.mozilla)
(Assignee)

Updated

4 years ago
blocking-b2g: --- → 2.0?
(Assignee)

Comment 20

4 years ago
Eric and I looked at this in person on the Flame at 1.5x and visually we are good to go.
blocking-b2g: 2.0? → 2.0+
(Assignee)

Updated

4 years ago
No longer blocks: 1019289
Whiteboard: [systemsfe] → [systemsfe][ETA=7/17]
Comment on attachment 8454113 [details] [review]
PR to add video file handling to image-resolution post-app build step

looks good to me and I also verified the result when execute |pickMediaByResolution| for Notification.mp4, r=yurenju
Attachment #8454113 - Flags: review?(yurenju.mozilla)
Attachment #8454113 - Flags: review?(poirot.alex)
Attachment #8454113 - Flags: review+
(Assignee)

Comment 22

4 years ago
Merged: https://github.com/mozilla-b2g/gaia/commit/1d2e1d146d3efd4f91eb15cca4c37209f70da795
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 23

4 years ago
Comment on attachment 8454113 [details] [review]
PR to add video file handling to image-resolution post-app build step

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Tutorial videos are low quality and blurry, also all resolution versions of the videos are included in each build
[Describe test coverage new/current, TBPL]: tbpl green, manual verification by author and reviewer
[Risks and why]: Low risk
[String/UUID change made/needed]: None
Attachment #8454113 - Flags: approval-mozilla-aurora?
Comment on attachment 8454113 [details] [review]
PR to add video file handling to image-resolution post-app build step

No approval needed for 2.0 blockers.
Attachment #8454113 - Flags: approval-mozilla-aurora?
Needs rebasing for v2.0 uplift.
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → fixed
Flags: needinfo?(sfoster)
Keywords: branch-patch-needed
Whiteboard: [systemsfe][ETA=7/17] → [systemsfe]
(Assignee)

Comment 26

4 years ago
There was a refactoring between 2.0 and 2.1 so I'll need to re-work this patch a little to land on 2.0.
Flags: needinfo?(sfoster)
(Assignee)

Comment 27

4 years ago
Created attachment 8456836 [details] [diff] [review]
bug-1024440-hidpi-video-20.patch

Backported patch for the hidpi application.zip video support in 2.0
(Assignee)

Comment 28

4 years ago
Comment on attachment 8456836 [details] [diff] [review]
bug-1024440-hidpi-video-20.patch

Looks like this code got a major refactor between 2.0 and current master. This is the same patch backported. The unit tests pass (make build-test-unit) and I've tested on Flame (1.5x device) and verified the right videos only are copied into the application.zip during the build.
Attachment #8456836 - Flags: review?(yurenju.mozilla)
(Assignee)

Comment 29

4 years ago
Created attachment 8456883 [details] [diff] [review]
bug-1024440-hidpi-video-20.patch

Sorry, this is the right version of the patch. Also in a branch in my clone: https://github.com/sfoster/gaia/tree/bug-1024440-hidpi-video-20
Attachment #8456836 - Attachment is obsolete: true
Attachment #8456836 - Flags: review?(yurenju.mozilla)
Attachment #8456883 - Flags: review?(yurenju.mozilla)
Attachment #8456883 - Flags: review?(yurenju.mozilla) → review+
(Assignee)

Updated

4 years ago
Keywords: branch-patch-needed
status-b2g-v2.0: affected → fixed

Updated

4 years ago
Status: RESOLVED → VERIFIED
status-b2g-v2.0: fixed → verified
status-b2g-v2.1: fixed → verified

Comment 31

4 years ago
Created attachment 8530134 [details]
Picturesofverification.zip

This issue has been successfully verified on Flame 2.0:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3
Build-ID        20141127000203
Version         32.0
Device-Name     flame
FW-Release      4.4.2


This issue has been successfully verified on Flame 2.1:
Gaia-Rev        5372b675e018b6aac97d95ff5db8d4bd16addb9b
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/f34377ae402b
Build-ID        20141127001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
You need to log in before you can comment on or make changes to this bug.