Closed
Bug 1251644
Opened 8 years ago
Closed 8 years ago
A vertical line is displayed in the right side of YouTube's videos when Quality is toggled
Categories
(Core :: Audio/Video: Playback, defect, P2)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: sbadau, Assigned: mattwoodrow)
Details
Attachments
(2 files, 1 obsolete file)
378.57 KB,
image/png
|
Details | |
1.61 KB,
patch
|
seth
:
review+
|
Details | Diff | Splinter Review |
[Affected versions]: Firefox 44.0.2 RC Firefox 45.0b9 Firefox 46.0a2 Latest Developer Edition 46.0a2 Latest Nightly 47.0a1 [Affected versions]: Mac OS X 10.11 Windows 8 - 64 bit [Steps to reproduce]: 1. Navigate to https://www.youtube.com/watch?v=lJnvq0A_7WQ. 2. In the controls bar, click on Settings. 3. Click on the Quality option. 4. Toggle between the 720p and 480p resolutions. [Expected results]: The margins of the video should remain the same. [Actual results]: A vertical line is displayed on the video's right side. [Regression range]: This is not a recent regression - reproducible also on Firefox 41 RC. I will investigate more and come back with a regression range as soon as possible. [Additional notes]: Not reproducible on Ubuntu machines. Please see the attachment for more details. If the issue doesn't reproduce at first you can easily reproduce it by resizing the Firefox window.
Reporter | ||
Comment 1•8 years ago
|
||
Jean-Yves - this looks like an off-by-1 sizing issue. Can you take a squiz at it?
Flags: needinfo?(jyavenard)
Priority: -- → P2
Comment 3•8 years ago
|
||
I don't believe the issue is in the decoder. this can only be seen when the player has a dimension of 854x480 The video decoder has no idea on what the display resolution will end up being, it always output a 1280x720 frame. Matt, is this gfx or something else?
Flags: needinfo?(jyavenard) → needinfo?(matt.woodrow)
Assignee | ||
Comment 4•8 years ago
|
||
Looks like this our code to preserve aspect ratio. http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsVideoFrame.cpp#184 The video is 854x480, but the destination is 1280x720 (on my machine). These two sizes have (very marginally) different aspect ratios. We end up computing 1280x719 as our scaleHint, so the compositor is scaling to that, leaving a black line across the top. This is somewhat tricky, the video isn't drawing all the pixels that it said it would, and so we haven't draw the background colour for that gap. I'll think about the best way to fix this. A video with a weird aspect ratio would probably show a much worse version of this.
Flags: needinfo?(matt.woodrow)
Assignee | ||
Comment 5•8 years ago
|
||
Ok, so the fix for this particular case might be different from the general solution. In this particular case, the aspect ratio of the video and the <video> element are incredibly close and are clearly intended to be the same (we compute 719.4 before rounding as the height). I think we could introduce a fuzz factor into our aspect ratio calculation to get these treated as identical and that would solve this bug. We still have the more general problem of what to do when the aspect ratios are actually different. Do we: * Render black strips to pad out the size of the <video> element * Render transparency, effectively ignoring one of the dimensions provided to the <video> element. * Something else? Should these strips (black or transparent) be evenly distributed across both sides of the element, or should they all be at one edge? Media team should probably make the call here, though I haven't looked at the spec for <video> yet.
Flags: needinfo?(jyavenard)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(cpearce)
Comment 6•8 years ago
|
||
can't you just round the aspect ratio to say floor(PAR*100)/100 854/480 = 1.77916666666667 -> 1.77 1280 / 720 = 1.77777777777778 -> 1.77 How was the destination dimensions calculated in the first place though?
Assignee | ||
Comment 7•8 years ago
|
||
Yes, rounding will solve it for YouTube. We still need to solve the case where the aspect ratios are widly different though. The destination dimension are specified in YouTubes' HTML code, as attributes to the <video> element.
Assignee | ||
Comment 8•8 years ago
|
||
Oops, I jumped to conclusions a bit here. We are treating the strips as transparent, and the black we're seeing is because YouTube specified a black background manually. So we're basically correct here, we just want to fix the aspect ratio calculation to be slightly less fussy.
Flags: needinfo?(jyavenard)
Flags: needinfo?(cpearce)
Assignee | ||
Comment 9•8 years ago
|
||
What do you think about this Seth? Basically, the different video streams aren't exactly the same aspect ratio (but as close as they can get with integer numbers of pixels). We end up computing this as the result of ComputeObjectDestRect: (lldb) p dest (nsRect) $17 = { mozilla::gfx::BaseRect<int, nsRect, nsPoint, nsSize, nsMargin> = (x = 4560, y = 3617, width = 76800, height = 43166) } (lldb) p destGFXRect (gfxRect) $18 = { mozilla::gfx::BaseRect<double, gfxRect, gfxPoint, gfxSize, gfxMargin> = (x = 152, y = 120.56666666666666, width = 2560, height = 1438.8666666666666) } Which we then round to nearest pixels, making y=120 and shows a 1px black background and looks silly.
Attachment #8738868 -
Flags: review?(seth)
Assignee | ||
Comment 10•8 years ago
|
||
Assignee: nobody → matt.woodrow
Attachment #8738868 -
Attachment is obsolete: true
Attachment #8738868 -
Flags: review?(seth)
Attachment #8738895 -
Flags: review?(seth)
Comment 11•8 years ago
|
||
Comment on attachment 8738895 [details] [diff] [review] snap object-fit:contain Review of attachment 8738895 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, I buy these. We have similar hacks elsewhere; unfortunately scaling and snapping are hard problems. BTW, in this operation: > float scaleX = double(aConstrainingSize.width) / aIntrinsicRatio.width; both the conversion to |float| and the division introduce error. Similarly here: > size.height = NSCoordSaturatingNonnegativeMultiply(aIntrinsicRatio.height, scaleX); the operation is effectively |aIntrinsicRatio.height * (aConstrainingSize.width / aIntrinsicRatio.width)|. Performing the division before the multiplication introduces error. If these sources of error are the source of the problem, it may be possible to reorder the calculations to reduce the error. (In general the basic trick is to either avoid subtraction and division, or put it off so it's the very last step of the calculation.) Worth checking, though I'm totally fine with the patch as is.
Attachment #8738895 -
Flags: review?(seth) → review+
Comment 13•8 years ago
|
||
This appears to have made Android reftests angry. https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=51922d1ebfa13a4932ac4c6dfd6010a39d145812&filter-searchStr=android%20ref
Flags: needinfo?(matt.woodrow)
Comment 14•8 years ago
|
||
(The opt R2 isn't yours, all the debug ones are)
Comment 15•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/9e7edcb43594
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8723a2bd8ecf
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(matt.woodrow)
You need to log in
before you can comment on or make changes to this bug.
Description
•