Skip to content
This repository has been archived by the owner on Jul 22, 2024. It is now read-only.

improve default playback quality of YouTube videos to 1440p (or vq=, quality= query-string parameter) (fixes issue #1051) #1052

Merged
merged 4 commits into from
Apr 11, 2019

Conversation

cvan
Copy link
Contributor

@cvan cvan commented Apr 2, 2019

for issue #1051

@cvan cvan added this to the v1.1.4 milestone Apr 2, 2019
@cvan cvan self-assigned this Apr 2, 2019
@MortimerGoro
Copy link
Contributor

I tried with this video but it still selects 380 or 480p by default: https://www.youtube.com/watch?v=sPyAQQklc1s

Can you check it?

@cvan
Copy link
Contributor Author

cvan commented Apr 3, 2019

@MortimerGoro I fiddled with it a bit. fixing now, should have it in a good state by tomorrow. will ping you when it's ready to test again. thanks!

Copy link
Contributor

@bluemarvin bluemarvin left a comment

Choose a reason for hiding this comment

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

This is great, the hard part will be to maintain it when YouTube breaks it.

@cvan
Copy link
Contributor Author

cvan commented Apr 4, 2019

This is great, the hard part will be to maintain it when YouTube breaks it.

quite possibly, yes. still tinkering here and there. I think I can cut the code down considerably.

another thought: we could avoid having to roll out new releases but possibly remotely hosting the CSS + JS so if (I mean when) things break on YouTube (though the selectors + DOM structure for changing the Quality haven't changed in years to my knowledge [besides the <div class="ytd-… -> <ytd…> changes for the Polymer version]). thoughts?

@bluemarvin
Copy link
Contributor

Since we don’t sign the extensions remote hosting would be an exploit vector.

@cvan
Copy link
Contributor Author

cvan commented Apr 4, 2019

Since we don’t sign the extensions remote hosting would be an exploit vector.

yeah, I agree. CSP in the manifest.json isn't enforced on these content scripts (yet; GV bug?), so we're already pushing it heh.

@cvan cvan force-pushed the youtube-hd-default branch 2 times, most recently from fdeb64c to dac2b13 Compare April 4, 2019 05:55
@bluemarvin
Copy link
Contributor

Web extensions are only intended for internal use right now in GV so there aren’t security measures by design I think.

@philip-lamb
Copy link
Contributor

Can we also comment/test whether this works against issues #832 #837 #793.

@cvan
Copy link
Contributor Author

cvan commented Apr 5, 2019

Can we also comment/test whether this works against issues

Thanks for checking!

#832

Nope, unrelated - we can probably close that. I haven't found it necessary.

#837

Yep, will be handled. I will reference the issue in the commit message when I push it to this PR branch.

#793

No, but we could eventually in that issue. VR180 support will require a lot more investigation and likely coordination w/ other browser vendors.

@cvan cvan force-pushed the youtube-hd-default branch 4 times, most recently from a94be9b to 7debaaf Compare April 6, 2019 02:20
@philip-lamb philip-lamb added the compatibility Web content compatibility issues label Apr 10, 2019
@cvan
Copy link
Contributor Author

cvan commented Apr 10, 2019

@MortimerGoro ready for another review, if you have a min

@bluemarvin
Copy link
Contributor

I just tested this patch with https://www.youtube.com/watch?v=hFjzFSidX3s&mozVideoProjection=360_auto and it default to 360p resolution.

@MortimerGoro
Copy link
Contributor

I see this error in the console

[JavaScript Error: "SyntaxError: missing = in const declaration" {file: "moz-extension://a9fe3216-9463-45c6-bf52-4eb51c350ea4/main.js" line: 69 column: 10 source: " const prefs.qualities = ["}]

@MortimerGoro
Copy link
Contributor

MortimerGoro commented Apr 11, 2019

If fixed the SyntaxError but it still doesn't work.

@cvan I see that you have changed the code to use player.setPlaybackQuality(). I already tried that the first time we added the webextension and added some comments there. I found that setPlaybackQuality is a no-op in the current Youtube API. It's mentioned in the docs:

setPlaybackQuality is now considered a "no-op"; calling this function will not change the player behavior. The player will use a variety of signals to determine the optimal playback quality.
Users are able to manually request a specific playback quality via the quality selector in the player controls.

So I think the only way would be to try the fake touches again

@cvan
Copy link
Contributor Author

cvan commented Apr 11, 2019

@MortimerGoro:

@cvan I see that you have changed the code to use player.setPlaybackQuality(). I already tried that the first time we added the webextension and added some comments there. I found that setPlaybackQuality is a no-op in the current Youtube API. It's mentioned in the docs:

Yeah, I can actually remove the player.setPlaybackQuality(newBestQuality) line.

But what I discovered is that you can use player.setPlaybackQualityRange(newBestQuality, newBestQuality), and that actually seems to work.

I've been able to run my branch successfully on YouTube videos on desktop Firefox + Chrome, and the default minimum quality is changed or if specified by the vq= or quality= query-string parameter, that quality gets enforced.

Where I'm stuck on:

  • What's made this branch difficult is the loading of the content script via GV's Web Extension API seems to load differently than desktop Firefox's. I might want to change document_start to document_end in the manifest.json.
  • The firing of the onStateChange event from YouTube's video player isn't fired on FxR but is on Firefox desktop w/ the same UA (w/ disable_polymer=1 correctly set in the URL) and Chrome too.
  • For some reason (likely user-activation requirement or YouTube's policies for progressively limiting the experience based on screen size/bandwidth/performance/etc.), YouTube doesn't begin playing videos, and most times player.getAvailableQualityLevels() isn't available. I use player.getAvailableQualityLevels() to cross-reference with all the supported YouTube quality levels to identify which index in the array to select to change the quality. There's also code to find the next best quality level if 1440p is not available (or the forced value in the query-string parameter of the URL).

To overcome these issues, I think I'm going to have to stop relying on these proprietary YouTube events to know when we are ready to set the quality.

I'll likely have to use a setInterval loop every couple milliseconds to keep checking until the player is ready and the player.getAvailableQualityLevels() actually returns the video's quality (which seems to be only after the video starts playing, which requires a user-activation click to play the video).

If anyone has any better approaches here, let me know.

As you can imagine, the development process of making changes with Android Studio -> Oculus Go every time there's a JS change is slow and difficult to remotely debug via desktop Firefox's about:debugging and the WebIDE.

Earlier, I did some experimenting with injecting a script hosted elsewhere so we'd be able to update the content script for YouTube whenever without having to ship a new release with the hardcoded content script.

@bluemarvin expressed concerns that because GeckoView's WebExtension API is not intended for public/end-developer consumption yet, it's likely unreasonable or irresponsible to inject a third-party script (even if it's ours and hosted on a Mozilla-owned domain). (CSP policies can be enforced, but still that's a possible MITM attack vector.)

The precedent I've seen in Gecko, specifically for Android WebCompat issues, is a hardcoded extension that's stored in mozilla-central and bootstrapped into Firefox. Every new adjustment requires a new Firefox release, but it seems to not been an issue from the bugs and commit activity I read. @miketaylr likely has some recommendations of best practices here for a scalable, realistic solution for maintaining WebCompat content scripts in FxR.

To speed up development, I've been writing the code and running it from about:addons on desktop Firefox, and that's helped me iterate faster.

Writing the hacks has been fun, and I still have the old code for manually pressing the buttons in the DOM. So we could go back to that if the player.setPlaybackQualityRange(newBestQuality, newBestQuality) code doesn't run properly in FxR. (I got it all working in FxR several times but accidentally broke it. Fragile stuff, but I'll battle-test this and make sure it always works.)

This patch is in a broken state right now because I've rewritten it so many times. In the meantime, in between coordinating the v1.1.4 release and other work tasks, I'm going to prioritise working on this to get it consistently stable and ready to review + land. If anyone has recommendations on the above items, I'd appreciate the help and will adjust the patch accordingly. Thanks, everyone.

@MortimerGoro
Copy link
Contributor

I think setInterval is the only realiable way right now to wait for quality availabilities. I'm ok with using that for now

@MortimerGoro
Copy link
Contributor

just tried player.setPlaybackQualityRange(newBestQuality, newBestQuality) using remote debugger in FxR and desktop and it worked in both. Good catch! So if we make the load detection more reliable with setInterval it should be ready to land

cvan and others added 4 commits April 11, 2019 10:34
…40p (fixes issue #1051); hide `Full screen is unavailable` message (fixes issue #1056)

- or via `vq=`/`query=` query-string parameter in URLs for youtube.com and youtube-nocookie.com
@bluemarvin
Copy link
Contributor

I've updated the PR to use the methods discussed. Please give it a try.

@MortimerGoro
Copy link
Contributor

It works. I'm going to merge this for the release

@MortimerGoro MortimerGoro merged commit a125258 into master Apr 11, 2019
@cvan cvan deleted the youtube-hd-default branch April 12, 2019 01:11
@cvan
Copy link
Contributor Author

cvan commented Apr 12, 2019

thanks for the assists! much appreciated ❤️

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compatibility Web content compatibility issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants