Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Migrate video player from video.js to shaka-player #4978

Open
wants to merge 76 commits into
base: development
Choose a base branch
from
Open

Migrate video player from video.js to shaka-player #4978

wants to merge 76 commits into from
+4,959 −7,136

Conversation

absidue
Copy link
Member

@absidue absidue commented Apr 18, 2024 •
edited by efb4f5ff-1298-471a-8973-3d47447115dc
Loading

Migrate video player from video.js to shaka-player

Pull Request Type

  • Bugfix
  • Feature Implementation

Related issue

Description

After almost a years worth of work, here is the shaka-player migration.

Significant improvements and new features

  • vp9 and opus during DASH playback. vp9 only works on the local API due to Invidious limitations
  • The video player will now match the users display language when possible (41/45 locales) and custom player elements added by FreeTube are now translatable. shaka-player doesn't support Estonian et (eesti keel), Basque eu (Euskera), Galician gl (galego) and Icelandic is (Íslenska), for those languages it will use English (US) instead.
  • Stats are now supported for all 3 Media Formats, not just DASH.
  • Switching formats will keep your playback rate, selected subtitle, audio language and video quality settings, additionally the player will no longer flash while switching.
  • The player should be a lot more usuable on mobile screens, as player elements get moved into an overflow menu when they don't fit.
  • Seeking on live streams when the uploader hasn't disabled it (4 hours back for normal ones, 2 hours on low latency live streams). Not supported when video proxying is enabled, as Invidious doesn't support proxying DASH live streams.
  • You can now watch just finished live streams (Post-Live-DVR) from the start with the local API and you'll get thumbnails on the seek bar when they are available (the thumbnails seem to be missing on about 50% of Post-Live-DVR streams).
  • The video player will pick up most settings changes without having to leave the watch page, such as changing the display language or the playback rate interval.
  • Subtitles will now work on live streams that have them (very rare but they do exist), local API only.
  • Support for VR videos that use equirectangular projection, shaka-player doesn't support YouTube's other projection types yet, local API only (The ANDROID_TESTSUITE client that Invidious uses, claims that all video streams use normal rectangular projection, so we don't have a way to identify VR videos through Invidious).

Significant bug fixes

  • The subtitle translation feature now works on all videos, not just ones that are in English.
  • Starting at a certain timestamp from the video URL or resuming a partially watched video, should now work all the time, instead of just randomly.
  • Long videos start straight away and no longer freeze FreeTube for almost a minute.

Missing/Removed features

  • shaka-player doesn't have any subtitle settings. Maybe in the future we could implement our own ones, but it's not-trivial to implement, so creating a replacement is out-of-scope for this pull request.
  • I removed the ability to seek into sponsor block segments that are set to skip, as it was causing the skipping to not work properly, due to the existing implementation being fundamentally flawed. If someone would like to add something like that back in the future, it should require a specific user-interaction to unskip the segment, e.g. a button in the skip message, instead of allowing users to manually seek into the segment.

Known issues

  • Invidious' video proxy doesn't support live or post live DVR DASH, so we have to keep using the HLS manifests when the "Proxy Videos Through Invidious" setting is enabled, which means you can't seek in them or use the captions if there are any.
  • Invidious' DASH manifests don't include the vp9 or opus streams and the API doesn't return enough information on the vp9 streams for us to be able to generate our own DASH manifest for them (height and width are missing), so vp9 playback is only supported on the local API, when "Proxy Videos Through Invidious" is disabled. Return actual height, width and fps for streams in /api/v1/videos iv-org/invidious#4586
  • Focus is not restored properly from exiting video player menus using keyboard navigation (will not be addressed in this pull request).

Screenshots

DASH

dash

DASH with controls faded away

dash-controls-faded-away

DASH in full screen mode

dash-fullscreen

DASH in full window mode

dash-full-window

DASH stats

dash-stats

Audio only with seek bar thumbnails and stats visible

audio-only-with-stats-thumbnails

Audio only with quality selector visible

audio-only-with-stats-quality

Live DASH at live edge

live-dash-live-edge

Live DASH seeked back a few hours

live-dash-seeked

Live audio only with subtitles

live-audio-seeked-subtitles

DASH with a vertical video/short

short-dash

Audio only with a vertical video/short and the FreeTube locale preference set to French

ui-language-french

Mobile audio only with the the overflow menu closed

mobile-menu-closed

Mobile audio only with the overflow menu open

mobile-menu-open

Mobile DASH with the overflow menu open on a video with subtitles and multiple audio languages

mobile-menu-open-everything

VR video that uses equirectangular projection

vr-equirectangular-projection

Testing

Desktop

  • OS: Windows
  • OS Version: 10
  • FreeTube version: 0.20.0

@absidue
Copy link
Member Author

absidue commented Apr 21, 2024 •
edited
Loading

Relevant Invidious pull requests:

@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
1 similar comment
@github-actions GitHub Actions

This comment was marked as duplicate.

Sign in to view
@efb4f5ff-1298-471a-8973-3d47447115dc

This comment was marked as outdated.

Sign in to view
@absidue @ChunkyProgrammer
Fix typo
1be8648 
Co-authored-by: ChunkyProgrammer <78101139+ChunkyProgrammer@users.noreply.github.com>
@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc linked an issue Apr 24, 2024 that may be closed by this pull request
[Feature Request] Hide Progress Bar / Old Progress Bar Option #920
Open
@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) July 2, 2024 18:00
@github-actions github-actions bot added PR: dependencies Pull requests that update a dependency file PR: waiting for review For PRs that are complete, tested, and ready for review labels Jul 2, 2024
@absidue absidue changed the title DRAFT: Migrate video player from video.js to shaka-player Migrate video player from video.js to shaka-player Jul 2, 2024
@absidue absidue mentioned this pull request Jul 2, 2024
Draft
@github-actions github-actions bot added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Jul 4, 2024
@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@ChunkyProgrammer
Copy link
Member

Seems like audio languages are being duplicated
image

ChunkyProgrammer
ChunkyProgrammer reviewed Jul 5, 2024
View reviewed changes
Copy link
Member

@ChunkyProgrammer ChunkyProgrammer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some very minor changes

_scripts/getShakaLocales.js Outdated Show resolved Hide resolved
_scripts/patchShaka.mjs
const cssResponse = await fetch('https://fonts.googleapis.com/icon?family=Material+Icons+Round', {
headers: {
// Without the user-agent it returns the otf file instead of the woff2 one
'user-agent': 'Firefox/125.0'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not super important since it is only ran for developers but maybe we could include this in another file so we can keep it up to date (doesn't need to be changed for this PR imo)

src/renderer/components/ft-shaka-video-player/ft-shaka-video-player.js Outdated Show resolved Hide resolved
static/locales/en-US.yaml Outdated Show resolved Hide resolved
static/locales/en-US.yaml Outdated Show resolved Hide resolved
@PikachuEXE
Copy link
Collaborator

One style issue spotted about segment skipping message
2024-07-07 09_39_38-Window

@PikachuEXE
Copy link
Collaborator

Seek by arrow key not working (but mouse scrolling works)
image

@absidue @ChunkyProgrammer
Apply suggestions from code review
c85062a 
Co-authored-by: ChunkyProgrammer <78101139+ChunkyProgrammer@users.noreply.github.com>
@absidue
Copy link
Member Author

absidue commented Jul 7, 2024

One style issue spotted about segment skipping message 2024-07-07 09_39_38-Window

I might be missing something obvious but what's the styling issue that you are seeing?

@github-actions GitHub Actions

This comment was marked as resolved.

Sign in to view
@absidue
Copy link
Member Author

absidue commented Jul 7, 2024

Seems like audio languages are being duplicated image

Looks like a shaka-player bug, I'll have to report it upstream.

@PikachuEXE
Copy link
Collaborator

I might be missing something obvious but what's the styling issue that you are seeing?

The toast Skipped segment is on the right & high

@github-actions GitHub Actions

This comment was marked as resolved.

Sign in to view
efb4f5ff-1298-471a-8973-3d47447115dc
efb4f5ff-1298-471a-8973-3d47447115dc reviewed Jul 9, 2024
View reviewed changes
Copy link
Member

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing phase 1 - Issues to be closed list

Related issue

Closes #624

LGTM! shaka auto closes PIP window when player is destroyed.

Closes #661

LGTM! Only lets the user seek back up to 4hrs as expected.

Closes #959

Is there a way for me to test this on the Local API side? Never had this one happen to me before in stable/nightly builds. This does still happen on the IV API but that is expected

Closes #2013

LGTM! Local API works. Couldt test IV because of Random bug: IV_watchpage_error but expected behavior should be that it doesnt work for IV.

Closes #2098

Somehow im unable to force a network error to test this in fullwindow/fullscreen. Others please test!

But

Only Local API: Application lockup occurred when player is in the default view and the user loses their connection.

Closes #2365

Local API works but couldnt test IV API because of Random bug: IV_watchpage_error

Closes #637

It does close this issue but there is some weird behavior going on with the Enter/Exit Fullscreen label (looks like similar behavior as 1st clip in #4978 (review))

637.mp4

Closes #4039
Closes #3920
Closes #3560
Closes #3534
Closes #3163
Closes #2931
Closes #2455
Closes #845
Closes #1344
Closes #920
Closes #3049
Closes #1803
Closes #4983
Closes #5253
Closes #5252
Closes #1348
Closes #5187

LGTM!

Random bugs

I think this one is the same as #5305 and has nothing todo with shaka. Waiting for confirmation before I remove it from this recording.

IV_seachpage_error.mp4
IV_watchpage_error.mp4

@github-actions GitHub Actions

This comment was marked as resolved.

Sign in to view
@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Jul 9, 2024 •
edited
Loading

After validation, can confirm that this closes #5187, and have updated the OP accordingly.

Will mark this comment as resolved in 24h

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc left review comments

@ChunkyProgrammer ChunkyProgrammer ChunkyProgrammer left review comments

At least 3 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
PR: dependencies Pull requests that update a dependency file
Projects
None yet
Milestone
No milestone
6 participants
@absidue @efb4f5ff-1298-471a-8973-3d47447115dc @kommunarr @Damus765 @PikachuEXE @ChunkyProgrammer