Closed
Bug 1024440
Opened 10 years ago
Closed 10 years ago
[FTE] Videos showing up very blurred
Categories
(Firefox OS Graveyard :: Gaia::First Time Experience, defect)
Firefox OS Graveyard
Gaia::First Time Experience
x86
Gonk (Firefox OS)
Tracking
(blocking-b2g:2.0+, b2g-v2.0 verified, b2g-v2.1 verified)
People
(Reporter: epang, Assigned: sfoster)
Details
(Whiteboard: [systemsfe])
Attachments
(6 files, 1 obsolete file)
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.
Updated•10 years ago
|
Whiteboard: [systemsfe]
Target Milestone: --- → 2.0 S4 (20june)
Assignee | ||
Comment 1•10 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•10 years ago
|
||
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•10 years ago
|
||
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•10 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)
Comment 5•10 years ago
|
||
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•10 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•10 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•10 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)
Updated•10 years ago
|
Target Milestone: 2.0 S4 (20june) → 2.0 S5 (4july)
Updated•10 years ago
|
Target Milestone: 2.0 S5 (4july) → 2.0 S6 (18july)
Assignee | ||
Comment 9•10 years ago
|
||
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•10 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•10 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•10 years ago
|
||
Also flagging Yuren Ju, I'm not sure which of you owns this area.
Flags: needinfo?(yurenju.mozilla)
Comment 13•10 years ago
|
||
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•10 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•10 years ago
|
||
* 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•10 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•10 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•10 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•10 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•10 years ago
|
blocking-b2g: --- → 2.0?
Assignee | ||
Comment 20•10 years ago
|
||
Eric and I looked at this in person on the Flame at 1.5x and visually we are good to go.
Updated•10 years ago
|
blocking-b2g: 2.0? → 2.0+
Updated•10 years ago
|
Whiteboard: [systemsfe] → [systemsfe][ETA=7/17]
Comment 21•10 years ago
|
||
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•10 years ago
|
||
Merged: https://github.com/mozilla-b2g/gaia/commit/1d2e1d146d3efd4f91eb15cca4c37209f70da795
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 23•10 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 24•10 years ago
|
||
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?
Comment 25•10 years ago
|
||
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•10 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•10 years ago
|
||
Backported patch for the hidpi application.zip video support in 2.0
Assignee | ||
Comment 28•10 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•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8456883 -
Flags: review?(yurenju.mozilla) → review+
Assignee | ||
Comment 30•10 years ago
|
||
On v2.0: https://github.com/mozilla-b2g/gaia/commit/daa21c0ab02d2e09751b48ac6ac92cb608aa5b08
Assignee | ||
Updated•10 years ago
|
Keywords: branch-patch-needed
Updated•10 years ago
|
Comment 31•10 years ago
|
||
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.
Description
•