-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bad subtitle rendering #2524
Comments
Could you please send the manifest URL? It will be difficult to test with just the text segment since it's likely a problem with several components working together. |
The content is geoblocked, and is not available in the USA. So I have shared the segment. :( |
We should be able to add the VTT text segment to some other content with |
Reproduced. I'll look into it. |
This is a result of how we place our controls, I believe. The DASH.js reference player's controls are beneath the video element; meanwhile, our controls are on top of the video element. We position the text box such that it stops above the controls bar, which takes up the bottom 15% of the video element. I'm not sure what the ideal way to solve this would be. I can see a few possibilities, but none of them feel like great options to me.
@michellezhuogg, the UI text displayer was your project. What's your opinion on this? |
I'm not certain, but I think Chrome's native controls and native text display might adjust position automatically. So it might be good to follow suit. It could probably be done just with CSS and an attribute. |
Hm. I think you're right. Looking at this sample with our UI disabled, it definitely looks like the box is higher than it is in DASH.js. Interestingly, it still looks like that even if you turn off the video element's controls. |
Previously, we were positioning subtitles relative to the video, but this lead to problems with subtitles that were manually positioned (via the line attribute) to be low (85% or lower); they would overlap with the controls bar, and thus be hidden. This changes cues to be positioned relative to the text-usable area of the screen (e.g. the space above the controls bar). In addition, the less values have been changed to not trim cues that extend out of the container, so that if they do overlap with the controls, they aren't clipped in an ugly way. Examining the Chrome native controls, this appears to be closer to how they render captions. Fixes #2524 Backported to v2.5.x Change-Id: I4500b8f637f3f43f48d019d14ef527e75d960fec
This change produces a side effect, now all the subtitles occupy the entire width of the video instead of just the width they need. Url to test: https://storage.googleapis.com/shaka-demo-assets/angel-one/dash.mpd |
You're right! We'll take another look. Thanks! |
Is this actually a bug? The captions file for that asset does not specify a size, and according to the webVTT spec for the size attribute:
So it seems like this is actually the expected behavior, according to the spec at least. I experimentally tried feeding in cues with a size of something besides 100, and the text displayer honors that as expected. |
The point @theodab was making was that this change seems to be in line with the VTT spec, which states that the default width for a cue is 100%. That said, we can see your point, @avelad, that this is unexpected and different from previous releases. We can make the default width effectively "auto" in CSS terms, to fit the size of the text. |
In the case of https://irtdashreference-i.akamaihd.net/dash/live/901161/bfs/manifestARD.mpd There is another additional bug due to the fact that text overlaps when in the previous version did not occur. @joeyparrish , I understand that the spec says that it must be 100%, but Safari does not apply it like this and also this supposes a groundbreaking change with respect to what was there before, can I suggest making this change configurable and setting the old value in 2.5.x by default and the spec in 2.6, but allowing to choose behavior? |
I think we can revert to the old behavior without undoing the placement fixes made by @theodab. |
If a cue specifies 100%, the background will extend from left edge to right edge. If there is no specific width, the background will fit to the text as it did before. This would apply to v2.5 and v2.6, and would first appear in v2.5.12. Sorry we didn't notice the difference before v2.5.11 came out. Since the "Angel One" clip starts with a mostly black frame, it was an easy mistake to overlook on that content. |
@joeyparrish I think that there are another error... (sorry about that) I'm pretty sure the regions in TTML have been broken too.
|
@avelad, the nightly build has not been updated since the fix. It normally updates around 3 am US/Pacific time. I'll update it now. |
@avelad, please try again and let us know if there are any further issues. |
@joeyparrish , The position now it's correct, but the subtitle overlap persist Note: I shared the stream privately |
When no specific size has been set on a Cue object, the default will now be 0, which means "auto" size to the text displayer. If a cue has no specific width, the background will fit to the text. If a cue has an explicit width of 100%, the background will fill the entire horizontal space of the text container. This fixes the 100%-width background introduced in Change-Id I4500b8f637f3f43f48d019d14ef527e75d960fec in #2524. Fixes #2524 Backported to v2.5.x Change-Id: I98caeb89d3da640a8175560810c8ccf2de27a2bb
We have fixed the overlap, and I'm updating the nightly build now to reflect this change. |
The nightly has now been updated. |
Now it works as expected. Thanks! |
:( @joeyparrish , I found another regression with the last change.. The subtitle appears on the middle of the video |
I think I see an easy fix, but we've broken this too many times for me to keep making one-off fixes without test automation. We need tests that can check the layout of many various types of subtitles in the UI and compare them to known-good screenshots. |
This has been fixed and the updates have been pushed to the nightly build. Furthermore, I will have screenshot-based layout tests in internal code review soon to help prevent mistakes of this kind in the future. |
@joeyparrish Is it possible to migrate the last change ( 754f16a )to version 2.5? |
Good catch! I thought I had cherry-picked it already. I'll double-check and make sure that all the text fixes are in v2.5.12. |
The position: 'absolute' style should only be set when other positioning attributes (left, right, top, bottom) are being set. Otherwise, the natural layout flow is disturbed, without anything to replace it. Closes #2524 Backported to v2.5.x Change-Id: I4062c68253218a0f4ace6560a8847381d80c48d5
The margin: auto style centers the element both horizontally and vertically. We don't want vertical centering, and we already get horizontal centering when appropriate through other styles. So removing margin: auto is the best fix. Closes #2524 Change-Id: Icff74e49b3917b50141d1205232ebb0856fbcf16
We have just landed layout tests to help avoid repeating these mistakes. See b839340 for details. |
@joeyparrish do you have any ETA for 2.5.12? |
Have you read the FAQ and checked for duplicate open issues? Yes
What version of Shaka Player are you using? 2.5.10
Can you reproduce the issue with our latest release version? Yes
Can you reproduce the issue with the latest code from
master
? YesAre you using the demo app or your own custom app? Both
If custom app, can you reproduce the issue using our demo app? Yes
What browser and OS are you using?
Chrome 81 macOS 10.15.4
For embedded devices (smart TVs, etc.), what model and firmware version are you using?
What are the manifest and license server URIs?
I can send a link if it's necessary, I attached the subtitle segment
What did you do?
Load the video, and select a subtitle
What did you expect to happen?
The subtitle is displayed correctly
What actually happened?
The subtitle is displayed misplaced and offset of the video player.
Screenshot from DASH.js

Screenshot from ShakaPlayer

Screenshot from ShakaPlayer but I commented the problematic css property (top)

WVTT segment
SUB5_00000117.m4s.zip
The text was updated successfully, but these errors were encountered: