-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Color information is being removed by the vtt parser (since Shaka 3.1.0) #4545
Comments
I just double-checked in Chrome 106 on Linux. If you pass text like If you use Shaka's UI, or enable the
The video container must be marked by the application as .shaka-text-container {
position: absolute;
left: 0;
right: 0;
top: 0;
bottom: 0;
pointer-events: none;
font-size: 20px;
line-height: 1.4;
color: white;
} Your application must provide those. (If you used our full UI, that would be included in the full UI stylesheet.) Other styles will be set as needed inline on the elements within shaka-text-container, to manage the styling of individual cues. I'm embarrassed not to find any of this in our docs. I'll correct that soon. Does this help? |
Thank you for your fast and detailed response 🙏
I see. I can accept that for sure. Maybe I should have clarified that that was just a side point though. I'm not using either the native browser subtitle renderer or Shakas UITextDisplayer (I was confused about how to do that for testing purposes though, so thanks for elaborating on this and improving the documentation). The company I work for have to support multiple players, so we can't use the players or browsers custom subtitle renderers or the browser renderers. Our solution is to hide the native subtitles but still use the html 5 video's textTracks and You can do this in any modern browser: But as of f42ccd2 Shaka tries to parse the cue text payload and split it up into multiple cues etc via the We can override it by overriding either Object.assign(shaka.text.VttTextParser, {
parseCueStyles(payload, rootCue) {
rootCue.payload = payload;
},
}); Then it works again for our purposes, but breaks internal Shaka logic for A more proper solution (that I ended up using) is to create a custom "text displayer" similar to SimpleTextDisplayer and solve it there.
|
I just spent the whole day debugging a similar issue (shaka-player without the UI rendering TTML cues without colors), just to find out this comment ! 😌 Also, when I have a look at .shaka-text-container {
// ...
span.shaka-text-wrapper { // <<===
display: inline;
background: none;
}
} Do we also need this one ? Thanks ! |
This is clearly a pretty complex workflow that has some room for improvements. Shaka supports parsing VTT, TTML as well as several other (mostly simpler) formats. These are converted to Shaka's custom cue format, and if using SimpleTextDisplayer or WebVttGenerator they are converted (back) to VTTCues. The VTT parser removes the original color classes, and neither of those classes adds them back when it converts back to VTT. My preferred solution at the moment would be to
This of course would still lose TTML colors unless they were the exact values as in I would love to be able to add some contributions, but the more I looked into this the more complex the picture became. |
I'm very much open to discussing a redesign of the cue system. I think what you are pointing out is only one of several complexities and drawbacks of the system we have today. I'm not certain, though, that adding on additional methods to Cue or adding the original payload to Cue is the best solution. It might make some operations more efficient at the expense of increasing overall complexity of Cue somewhat. @friday, if you want to spend some time thinking about the cue system, you could write a proposal for what cues and text parsing could look like in the future (e.g., with no limits on breaking changes in Shaka Player v5). Would you be interested in designing something like that? |
I don't think I'm qualified to write a proposal, since I haven't spent as much time with the Shaka source code as you, and hence wouldn't know all the complexities and drawbacks of the current system. Just the ones I bumped into. But I have some thoughts on the parts I'd like to improve. From my perspective we may not need a breaking/major version bump.
If I take on these tasks and make PRs, would you prefer multiple small PRs? Personally I prefer multiple smaller ones. |
/**
* If true, this represents a container element that is "above" the main
* cues. For example, the <body> and <div> tags that contain the <p> tags
* in a TTML file. This controls the flow of the final cues; any nested cues
* within an "isContainer" cue will be laid out as separate lines.
* @type {boolean}
* @exportDoc
*/
this.isContainer; It impacts the layout (display styles) of elements in lib/text/ui_text_displayer.js. If we're redesigning the system, anything could be replaced with some other way to get the same functionality. But this is what it's doing today.
You're referring to flattenPayload? That could definitely be factored out. Whether or not we export it as part of the API contract of the library is another question. I'm not against it, though. We have lots of utilities exported because of their usefulness in writing network filters, manifest parsers, etc.
I'm not so familiar with getCueAsHTML(), so I don't know the answer. We would have to check that it works on all platforms (including Xbox, Tizen, etc). Our test automation lab is pretty good for things like this, though. You could write a test in Jasmine (test/ folder) that checks the functionality, and send it in a standalone PR. I could then trigger the lab to test your PR and we would find out if it is usable or not.
That all sounds reasonable.
Yeah, smaller PRs are easier to review and we can make incremental progress. |
Can you test with main branch? Thanks! |
It's not fixed in main (v4.3.4-main-2-g7439a264). The issue as I defined it is that the vtt parser stores colors in a way that the default (simple) text displayer can't access/understand, and doesn't even try. In effect, the parser takes the VTT input payload So if someone tries to listen to TextTrack changes (the standard HTML5 Video way) and implement their own rendering, then the color information is lost, although as confirmed it works with UITextDisplayer (which doesn't create TextTracks). |
Have you read the FAQ and checked for duplicate open issues?
Yes
What version of Shaka Player are you using?
4.2.1
Can you reproduce the issue with our latest release version?
Yes
Can you reproduce the issue with the latest code from
main
?Yes
Are you using the demo app or your own custom app?
Custom app
If custom app, can you reproduce the issue using our demo app?
No. It doesn't load the subtitle at all in your demo (because
language
is "" I think)What browser and OS are you using?
Chromium and Firefox. Happens on all OSes
For embedded devices (smart TVs, etc.), what model and firmware version are you using?
Doesn't apply
What are the manifest and license server URIs?
Doesn't apply. See code below
What configuration are you using? What is the output of
player.getConfiguration()
?What did you do?
Load a manifest with colored subtitles and add a
cuechange
listener logging the active cue and itsgetCueAsHTML()
result.Then play the video and watch the debug log. When the subtitle says the name of a color ex "cyan", "lime", "red", these should have matching classes. This is needed to implement custom subtitle cue display logic.
What did you expect to happen?
The cue at 0:16 for example should have this text:
<c.lime>lime</c>
(or<c.lime>lime</c.lime>
which seems to be valid too and works with your xml parser) and thegetCueAsHTML()
value should be<span class="lime">lime</span>
. This is how it works in dash.js, hls.js and how it worked in Shaka before d882d28 (3.1.0).What actually happened?
The cue at 0:16 just has the text
lime
and thegetCueAsHTML()
value is also justlime
.In addition to that, all the cues are actually displayed as white, rather than their real colors, but this is already covered by #4479
The text was updated successfully, but these errors were encountered: