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.

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

Fix DASH default quality and quality selection #3278

Merged

Conversation

absidue
Copy link
Member

@absidue absidue commented Mar 9, 2023

Fix DASH default quality and quality selection

Pull Request Type

  • Bugfix

Related issue

potentially addresses #595 (it fixes the original issue but someone left comments requesting automatically switching to legacy when certain qualities aren't available in DASH, which this pull request most certainly doesn't do)
potentially addresses #1533 (the quality dropping part)

Description

Fixes the default DASH quality selection and the DASH quality selector so that it should now be possible to switch between qualities in the quality selector when DASH is enabled. Fixing the default quality selection also fixes the slow loading times.

This also fixes the quality selector showing 1080p, even if you selected a different default quality.

Why wasn't the default quality selection working for DASH?
Well as it turns out even if you disable qualities videojs-http-streaming, it ignores your preferences if it thinks that it doesn't have enough bandwidth to play them (it's wrong but we can override it's decision as we know better). On my machine with the default quality set to 1080p it first requested 720p, then 144p and finally it switched up to 1080p, so the slow loading was caused by videojs-http-streaming wasting time doing pointless stuff.
The solution is to disable them as soon as possible and then also override videojs-http-streaming's default bandwidth with the one of the format, that way videojs-http-streaming will always select the correct quality at the start, as we've told it that it has enough bandwidth to play it.

As for why the quality selector works now, no idea it just started working while I was making all of these changes, so I'll take the win.

Loading and quality switching still isn't as fast as it is on YouTube but that's something for another day, if it's even possible to do with video.js.

Testing

Try various default qualites and switch between the qualities in the DASH quality selector (reminder for PikachuEXE as you asked last time: that's the quality selector that is shown when the DASH formats are selected).

Pika: For #595
(A) When low quality preferred

  • Update default quality to 240p in settings
  • Play a video with 720p available like https://youtu.be/v3wm83zoSSY
  • Ensure video is not being played in 720p

Desktop

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

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Mar 9, 2023
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) March 9, 2023 21:51
@PikachuEXE
Copy link
Collaborator

PikachuEXE commented Mar 10, 2023

This seems a bit slow...
Local API no VPN
image

I changed to this.selectedBitrate * VHS_BANDWIDTH_VARIANCE * 10 + 1 then it's much faster
image

Thoughts?

Update 1:
Logged bandwidth
After * 10 = 58892184 bit = 7361.523 KB
So before I guess it's 7xx KB
But I got ISP with max speed 500Mb = 62.5MB/s

Update 2:
Using Number.MAX_VALUE also works
(Just like vhs did in test)
https://github.com/videojs/http-streaming/blob/6ba70e0e172b283191c299d58489fd0ebf415ed9/test/videojs-http-streaming.test.js#L5456

Co-authored-by: PikachuEXE <pikachuexe@gmail.com>
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.

Typo

src/renderer/components/ft-video-player/ft-video-player.js Outdated Show resolved Hide resolved
Co-authored-by: ChunkyProgrammer <78101139+ChunkyProgrammer@users.noreply.github.com>
@efb4f5ff-1298-471a-8973-3d47447115dc

Im just putting this here before my brain forgets about this

Shorts dont select default quality

@PikachuEXE
Copy link
Collaborator

Just fixed a bug when default quality is set to auto
My IDE keeps warning me this.defaultQuality can only be number (before fix)

@PikachuEXE
Copy link
Collaborator

Also fixed quality selection when video height > width (e.g. W720 x H1280)
a.k.a Shorts/Vertical videos?

@absidue
Copy link
Member Author

absidue commented Mar 15, 2023

@PikachuEXE Your code will only work for the local API, Invidious doesn't return a height or width, it returns a resolution attribute with the larger dimension which we currently use as the height.

@PikachuEXE
Copy link
Collaborator

PikachuEXE commented Mar 15, 2023

I got the quality selection working for both local API and Invidious but the quality selector is not displaying the quality being used
image
image
image

Video used for testing:
https://youtu.be/v3wm83zoSSY

Update 1:
It would be workaround if we select video format based in fps (works on both APIs)
But the real issue is that this.player.qualityLevels() returns only 720p60 not 720p
image
image
image

Note that this issue is gone if I don't use Local API and proxy video through Invidious

@absidue
Copy link
Member Author

absidue commented Mar 16, 2023

@PikachuEXE sorting by bitrate is the only way to make sure hdr qualities are in the right order (they have formats that are identical in height and fps but differ in bitrate)

also will setting the bandwidth to max cause issues on lower end devices and bad internet connections?

@PikachuEXE
Copy link
Collaborator

Do we have some example videos for HDR formats?

I can only find https://gist.github.com/AgentOak/34d47c65b1d28829bb17c24c04a0096f for format IDs
For test video https://youtu.be/v3wm83zoSSY higher bitrate might not be a better quality (I can't tell)
image

I will revert the change since this might need some tweaking later (requires another PR)

@PikachuEXE
Copy link
Collaborator

Another followup issue
https://youtu.be/tO01J-M3g0U cannot be played with Invidious API with Allow DASH AV1 formats

@efb4f5ff-1298-471a-8973-3d47447115dc

Is selecting between multiple of the same quality in scope of this PR?

VirtualBoxVM_QIfUnyF667.mp4

@PikachuEXE
Copy link
Collaborator

Right now it just use highest bitrate one
It's selecting 720p on 2nd video coz it has higher bitrate (even 30 fps)
Not scope of this PR (mainly fix DASH quality on start, see issues)

@FreeTubeBot FreeTubeBot merged commit a53e7f0 into FreeTubeApp:development Mar 23, 2023
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Mar 23, 2023
@absidue absidue deleted the fix-dash-quality-selection branch March 23, 2023 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants