Storyboards: Various fixes and code cleaning #4153
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SamantazFox
force-pushed
the
fix-storyboard
branch
from
abaee5c to
14d704b
Compare
|
The refactor LGTM, but I'm not sure it fixes #3441 - when I open the url in the browser after replacing |
That's what bugged me out for a moment! Actually, the first URL should now look like this: Note that it's not |
Oh I totally missed that. That's pretty interesting! |
iBicha
approved these changes
Oct 8, 2023
absidue
reviewed
Oct 8, 2023
| # Parse the JSON structure from Youtube | ||
| def self.from_yt_json(container : JSON::Any, length_seconds : Int32) : Array(Storyboard) | ||
| # Livestream storyboards are a bit different | ||
| # TODO: document exactly how |
There was a problem hiding this comment.
Tried messing around with them in shaka for FreeTube (as we can use the live dash manifest with shaka and get 4 hours of seeking) but the code I came up with based on reversing the player.js file and stepping through it with a debugger, ended up requesting loads of future not yet existing thumbnails (something in my assumptions or calculations must have been off), so I gave up and continued doing the rest of the shaka migration.
I'll let you know if I ever get it working in FreeTube.
There was a problem hiding this comment.
Ok, thanks for your research ^^
There was a problem hiding this comment.
Implementing live storyboards is probably something better done on the client side anyway, as it will require information from the streaming URL response headers or the live DASH manifest (at least that's the only way I can think of figuring out the current live segment) and you'll have to update it every 5 seconds, so generating a VTT file on the server side doesn't seem like a viable option for that.
My previous attempt revolved around using shaka's manifest preprocessing function to inject the live storyboards into the DASH manifest based on information in the manifest. Although as shaka already struggles enough with YouTube's 4 hour DVR window (4 hours worth of 5 second segments = 2880, which it has to recalculate every 5 seconds x) ) in the live manifests, so I'm not sure if adding that extra delay is a good idea, even if my solution had worked, which it didn't.
SamantazFox
added
the
need-testing
Closed
|
I tried testing this PR and it seems like the storyboards aren't loading properly in Invidious While using this PR: Image urls for the VTT plugin used to have url like |
|
@ChunkyProgrammer Yes, we're aware of that problem. Syeopite added a workaround here: #4040 Edit: Though, I realised that his solution doesn't seem to handle URLs starting with |
|
Just to clarify, storyboards were working for me when running master locally at this commit 60fae01 (which was the latest at the time of my testing) but not working when using this PR locally |
|
Okay, I found the issue! |
SamantazFox
added
in-testing
SamantazFox
force-pushed
the
fix-storyboard
branch
2 times, most recently
from
139dc6c to
b18c6a8
Compare
|
It's so weird. Before that PR, all webVTT timestamps were null (see below), but Before
After
|
|
@SamantazFox Looks like videojs-vtt-thumbnails doesn't follow the spec properly and doesn't unescape the escaped characters. So an exception will need to be added to #4414 to not escape characters for thumbnail files. |
SamantazFox
added
unfinished
SamantazFox
force-pushed
the
fix-storyboard
branch
from
b18c6a8 to
a335bc0
Compare
SamantazFox
added
need-testing
The workarounds are as follow: * Unescape HTML entities * Always use 0:00:00.000 for cue start/end
SamantazFox
force-pushed
the
fix-storyboard
branch
from
e9694a0 to
5b05f3b
Compare
|
It should be good now. 5b05f3b adds the required workarounds for videojs-vtt-thumbnails: unescaped HTML entities and cue start/end always at 0:00:00.000 |
|
Well, I was wrong: videojs-vtt-thumbnails still requires proper cue timing, but the start and end times must overlap (see comment in last commit). If they don't, it leaves a 1sec gap and the GUI glitches, resulting in poor user experience. |
SamantazFox
added
in-testing
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #3441
@ChunkyProgrammer and @iBicha can I have your feedback on this one?