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)

defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- affected
firefox49 --- fixed

People

(Reporter: sbadau, Assigned: mattwoodrow)

Details

Attachments

(2 files, 1 obsolete file)

[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.
Jean-Yves - this looks like an off-by-1 sizing issue. Can you take a squiz at it?
Flags: needinfo?(jyavenard)
Priority: -- → P2
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)
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)
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)
Flags: needinfo?(cpearce)
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?
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.
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)
Attached patch snap object-fit:contain (obsolete) — Splinter Review
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: nobody → matt.woodrow
Attachment #8738868 - Attachment is obsolete: true
Attachment #8738868 - Flags: review?(seth)
Attachment #8738895 - Flags: review?(seth)
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+
(The opt R2 isn't yours, all the debug ones are)
https://hg.mozilla.org/mozilla-central/rev/8723a2bd8ecf
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Flags: needinfo?(matt.woodrow)
You need to log in before you can comment on or make changes to this bug.