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

Misc fixes #107

Merged
merged 14 commits into from
Sep 27, 2023
Merged

Misc fixes #107

merged 14 commits into from
Sep 27, 2023

Conversation

hawken93
Copy link
Contributor

@hawken93 hawken93 commented Dec 31, 2020

Trying to collect small fixes here while the big PR is merged.

Will rebase this after.

Currently in:

  • Refactoring mediaManager to playerManager because it's called that in CAF.
  • HLS doesn't allow wider streams than the window rez, so limit to window.innerWIdth instead of by device maximums
    • TODO: We should do the same for height, I think portrait mode videos would break without that.
    • TODO: this is bad for directplay and non-hls playback, we should consider reverting this and make mkv the only way to play video. Tough choices ahead. I left the old code commented so it can be easily restored.
  • Receiver ID is set after the initial capabilities are reporting, resulting in two devices registered server side. Refactored that inside of JellyfinApi to get it under control and reordered it just a bit more so it obeys the bitrate limit as well (hopefully). Fixes Two new active devices reported on start of each cast session #43
  • Getting rid of the extend() method, it just doesn't belong.
  • Fixing start seek. Fixes Receiver always starts playback from the beginning #63
  • Fixing playback position reporting
  • Fixing stream change that requires transcoding to restart
  • Don't tell CAF to completely stop after each item played back
  • cap max bitrate to device spec as well.
  • Improve bitrate measurement algorithm
  • Refactor away window.playlist to get the playlist working
  • Start in the middle of a playlist (options.startIndex)

@hawken93
Copy link
Contributor Author

hawken93 commented Jan 1, 2021

#43

@hawken93 hawken93 force-pushed the misc-fixes branch 2 times, most recently from 1990b76 to 4a8abe2 Compare January 1, 2021 00:09
@hawken93
Copy link
Contributor Author

hawken93 commented Jan 1, 2021

Yeah so this means that there is another parameter that is arbitrarily restricted when using HLS: Width.
It won't accept any HLS stream that exceeds the actual window width.
Even if it would work.

I suspect that this is used to pick the best stream that makes sense in a scenario where the manifest has many alternatives. For example if you loaded up some youtube video, a cast device connected to 720p would only consider streams up to 720p. If you take your fancy CC Ultra and connect it to 720p, it doesn't really make sense for it to load up some 4k video. My guess is that for HLS it doesn't know the difference between something that is not ideal and something that wouldn't play at all.

However it would still work for directplay because it doesn't seem to care about this check.

This means that we really have to treat direct streamed mkv vs HLS completely differently from a parameter perspective. I also think we need to cap the HLS bitrate based on device because of the inefficient HLS implemenetation.

@hawken93 hawken93 force-pushed the misc-fixes branch 4 times, most recently from 6163426 to feac1e3 Compare January 3, 2021 16:29
@hawken93
Copy link
Contributor Author

hawken93 commented Jan 3, 2021

added #105

@hawken93
Copy link
Contributor Author

hawken93 commented Jan 3, 2021

Added #102

@hawken93
Copy link
Contributor Author

hawken93 commented Jan 3, 2021

Added #108

@hawken93
Copy link
Contributor Author

hawken93 commented Jan 4, 2021

Possible regression on playback of some audio files delivered as opus..

@hamburglar2160
Copy link

When changing the transcoding stream (enabling subtitles mid-transcode), waitingcontainer covers the video.

@hawken93
Copy link
Contributor Author

hawken93 commented Jan 4, 2021

Waiting on #93

@hawken93 hawken93 changed the title WIP: Misc fixes Misc fixes Jan 5, 2021
@hawken93 hawken93 marked this pull request as ready for review January 5, 2021 02:08
@hawken93
Copy link
Contributor Author

hawken93 commented Jan 5, 2021

still want documentmanager in first

@hawken93 hawken93 marked this pull request as draft January 5, 2021 09:21
@hawken93 hawken93 force-pushed the misc-fixes branch 2 times, most recently from eca4cdc to a9ef049 Compare January 5, 2021 15:21
@hawken93
Copy link
Contributor Author

I've put this on top of DocumentManager, so this will look nicer after DocumentManager is merged :)

@hawken93 hawken93 force-pushed the misc-fixes branch 3 times, most recently from 688fa8e to b34cd01 Compare January 14, 2021 13:04
@itsmejoeeey
Copy link

itsmejoeeey commented Aug 27, 2023

@YouKnowBlom Any update on the state of this PR?

I think it's clear that I'm on board with option 2 here. I'm very very willing to contribute by cherry picking certain fixes from here and applying them to smaller PRs in hopes that I can get chromecast working.

If the maintainers can provide guidance on the best path forward, I'd additionally like to get the ball rolling and contribute towards bringing this project back to a working state.

Please let me know.


Although I understand the Android TV app works fine, having the ability to cast to a Chromecast (such as when away from home with friends and family) is an important feature to me - and one that I miss terribly after migrating from Plex.

Although I have done everything I can to get it the best chance of working (HTTPS with Lets Encrypt, loopback NAT and split DNS, valid Published Server URIs, etc) - both Stable and Unstable do not work properly currently.

@itsmejoeeey
Copy link

@YouKnowBlom @hawken93 @nielsvanvelzen

Considering the state of the repository (completely borked), I don't understand the risk in merging this PR. It will likely fix more than it will break. This PR is effectively blocking any further wider contribution to the repository.

I hope you'll consider just sending it - but ultimately I understand this decision isn't up to me.

If I don't hear anything further, in 10 days I will proceed down the path of creating smaller more manageable PRs from this one.


@tnyeanderson Are you still willing to help contribute to this? In 10 days I'll make an issue to track the work of getting this repo back on track.

@nielsvanvelzen Are you still happy to review smaller more manageable PRs if we go down this route?

@sfwr
Copy link

sfwr commented Sep 21, 2023

@itsmejoeeey I'm not sure if setting it via env does something differently than setting it via GUI, but I was able to solve most of my problems by setting this env variable on the docker container running jellyfin:

JELLYFIN_PublishedServerUrl=https://example.com/

(replace example.com with your server url)

I'm running with nginx proxy manager.

@tnyeanderson
Copy link
Contributor

@sfwr yes that has been known to help in some cases. But in many cases, it's just buggy or flat-out doesn't work anyways.

@itsmejoeeey At this point, I am going to say that I do not think that "sending it" is a good answer here. You can posit that this might fix more than it breaks, but how was that determined? This PR is so complex and old that merging it in at this point honestly seems dangerous, unless someone can explain in detail what it does and can thoroughly test it.

It seems clear to me that hardly anyone really knows exactly what this PR does, we are just desperate for a fix. And I am too!

I am absolutely willing to contribute smaller PRs. I think we need to start by dissecting this PR and figuring out all the changes it proposes. Then we should make manageable, atomic goals that can actually be met, reviewed, and merged in a reasonable time.

@nielsvanvelzen
Copy link
Member

Considering the state of the repository (completely borked), I don't understand the risk in merging this PR. It will likely fix more than it will break. This PR is effectively blocking any further wider contribution to the repository.

I hope you'll consider just sending it - but ultimately I understand this decision isn't up to me.
....
@nielsvanvelzen Are you still happy to review smaller more manageable PRs if we go down this route?

So as you've probably noticed the casting support is not great. This is mainly because none of our contributors use it (most use the native Android TV app that I maintain) and developing for the cast framework is a pain. I'd be willing to help out merging the smaller pull requests but I'm not able to do releases/troubleshooting/complex reviews as my knowledge about the framework is too low. We need to find solutions for that in the long term.

@BloodyIron
Copy link

I would like to add to the discussion topic that there are those of us who choose to stick to Chromecast devices (in the classical sense) and do not want to switch to Android TV or other such devices. Not trying to say anyone related to Jellyfin stuff is pressuring me (or others) to make the switch, more-so that there are users who intentionally plan (such as myself) to not have a future-path to migrate away from said devices. I have no real way of telling how many there are like this, but I know there's at least one ;)

Either way, I hope Chromecasty stuff for Jellyfin (and related client apps) gets to the point of actually being usable, as in my exhaustive testing (and yes I know I have a complex setup too) I cannot get more than two songs to play before playback just refuses to continue.

@Sky-High
Copy link
Contributor

Like others in this thread I am quite interested in the webreceiver as well.

Actually, I have created my own version for testing by merging https://github.com/YouKnowBlom/jellyfin-chromecast/tree/misc-fixes-tnye-merge with the current unstable, and that works remarkably well. For me many major bottlenecks are removed, and it is much, much better than the current unstable (and in fact, the stable released version). But that might be due to me having only a simple cast setup.

A lot of issues obviously will remain, but hey, no application without bugs ;-) From my point of view, proceeding with the YouKnowBlom/tnyeanderson branch seems easier than taking the current unstable as starting point, but that may well be a personal perspective. I do not understand everything of that branch, but neither do I of the unstable. I intend to continue with gradually fixing some more in my clone, even if it is only out of interest.

I am obviously quite willing to share my fixes if others are interested, but like @itsmejoeeey not sure how to do that. Create my own fork for that? Based on the unstable, or on YouKnowBlom's fork?

If another approach is chosen, I of course don't mind to follow that as well.

@nielsvanvelzen
Copy link
Member

I will help out with the pull requests opened in this repository targeting the master branch. It should be fine to use the existing open pull requests and fix them (fix conflicts/bugs/pending review comments etc.). All branches from forks that have no pull request to this repository will not be accepted without explicit from the owner of the fork.

When there has been some activity I will discuss future steps for the repository with the Jellyfin team and make the results public.

@BloodyIron
Copy link

It warms my heart that it looks like this is going in a productive direction <3

@SteveDinn
Copy link

Oh man, it was clearly a mistake when like 18 months ago, I set my notifications for this project to "All Activity". It was maybe one email a month until today. Then, BAM!

Glad to see things are moving again!

@nielsvanvelzen
Copy link
Member

Haha sorry about that, I started with some dependency updates but there are a lot. Should be less busy from now on.

@Sky-High
Copy link
Contributor

I'll try to submit an appropriate PR after the weekend for review :-)

@Sky-High
Copy link
Contributor

As the present PR contains a lot of commits already, it seems cleaner to create a separate PR to just implement the YouKnowBlom/tnyeanderson branch. That PR is #465.

@nielsvanvelzen
Copy link
Member

Thank you @Sky-High! I'm still working on getting access to the cast developer console. I was billed twice but didn't get any access yet so I have to deal with Google Support to get that fixed.
When that's solved I will review the PR.

You might have also noticed that all dependency updates I did have not been deployed to the unstable instance, this is because the SSH key expired. I've rewritten the deployment workflow for GitHub Actions (#464) and am working with our infrastructure & secret people to get that running again.

@Sky-High
Copy link
Contributor

@nielsvanvelzen thanks.

I was indeed wondering: jellyfin-web refers to two registered webreceiver app versions

https://github.com/jellyfin/jellyfin-web/blob/25c4da34b2812ec28047f5ae0b6ece17b6afa658/src/plugins/chromecastPlayer/plugin.js#L63-L64

Is the unstable version with applicationID=6F511C87 automatically available when a PR is merged?

I suppose the 'dist' for this is somewhere at a jellyfin server, altough I don't know the URL.

@nielsvanvelzen
Copy link
Member

Yes, the "unstable" option is automatically deployed from the master branch and the "stable" one is deployed from tags (what you see in the releases tab). They are deployed to apps1.jellyfin.org.

In 10.9 you will be able to customize the available chromecast clients if you want to self-host them / use a developer version / use a third-party one. See discussion here.

@BloodyIron
Copy link

Yes, the "unstable" option is automatically deployed from the master branch and the "stable" one is deployed from tags (what you see in the releases tab). They are deployed to apps1.jellyfin.org.

In 10.9 you will be able to customize the available chromecast clients if you want to self-host them / use a developer version / use a third-party one. See discussion here.

Yay to self-hosting! \o/

@nielsvanvelzen nielsvanvelzen merged commit 3bb63c3 into jellyfin:master Sep 27, 2023
@itsmejoeeey
Copy link

itsmejoeeey commented Sep 27, 2023

🥳🎉 Exciting!

Big appreciation to the many people that have worked hard to get to this stage. And a big thanks to @Sky-High and @nielsvanvelzen for getting it across the line (especially considering the latter doesn't even personally use this project).

@BloodyIron
Copy link

How soon before us non-dev plebs can see the fixes in Jellyfin for Chromecast so I can test and see good/bad/ugly results? Also, would I be looking for the stable or unstable version to select? 🤔

@nielsvanvelzen
Copy link
Member

After a pull request is merged it should be live in about 10 minutes. You need to use the unstable version.

@BloodyIron
Copy link

@nielsvanvelzen okay so I finally got around to testing/validating the "Unstable" Chromecast App in the Jellyfin app. Here's what I found:

  1. When testing against a Chromecast that is not blocked from reaching DNS 8.8.8.8 and 8.8.4.4, the Stable AND Unstable Chromecast App setting results in zero playback. This is for music, and video content.
  2. When testing against a Chromecast that IS blocked from reaching DNS 8.8.8.8 and 8.8.4.4 (and is fed the DNS servers I have on LAN that are known to work for this and other stuff), Stable results in zero playback. Buuuttt UNSTABLE... so far as I can tell works completely!

So what I did to do blocking, I took the MAC address for the Chromecast I wanted to affect, and created a static IP assignment in my gateway (pfSense), so the DHCP server gives it the same IP every time. In this static IP assignment (via DHCP Server) I also explicitly declare only the gateway as the DNS server, and no other DNS servers (on the internet or on LAN).

I then went over to my friendly-neighbourhood Firewall > Rules section of pfSense, then for the LAN interface added a "Pass" rule with the following parameters (it's at the bottom of the list so I don't think it needs to execute first):

  1. Interface LAN
  2. Address Family IPv4
  3. Protocol TCP/UDP
  4. Source, Single host or alias: IP of Chromecast
  5. Destination: Single host or alias, 127.0.0.1, Destination Port Range, from DNS (53), to DNS (53)
  6. Description: Something useful for future-humans

Then hit save, and APPLY.

It also just occurred to me as I was writing this that I should also validate other Chromecast clients if they can still work against the Chromecast in this configuration, and these apps worked, one on LAN, the other from the internet:

  1. Emby (LAN)
  2. YouTube
  3. Disney+
  4. CBC Listen

At this point it looks like EVERYTHING WORKS!

Oh, I also went to the states section of pfSense and confirmed that the affected Chromecast is having it's DNS traffic to 8.8.8.8 and 8.8.4.4 explicitly and 100% reliably redirected to the 127.0.0.1 IP.

Now, I really do appreciate all this work and such! Here are some areas I'm interested in improvement for Jellyfin stuff:

  1. Somehow figure out how to make it so the DNS 8.8.8.8 and 8.8.4.4 blocking no longer is needed (Emby doesn't need this, why does Jellyfin? I don't know)
  2. Have a new Stable release roll out for the Jellyfin-Chromecast app :^) as it's been uhhh more than two years ;P

Either way this looks 100% workable!!!! Thanks so much! :)))

@nielsvanvelzen
Copy link
Member

nielsvanvelzen commented Oct 13, 2023

Somehow figure out how to make it so the DNS 8.8.8.8 and 8.8.4.4 blocking no longer is needed (Emby doesn't need this, why does Jellyfin? I don't know)

This is probably because of your setup. Most apps will set-up a tunnel between their cloud and your local server, so all traffic from your chromecast goes trough that instead. Jellyfin does NOT use cloud servers and we have NO way of sniffing this traffic. The cast receiver needs to connect to your server and to do that it needs to be started with a valid host (DNS resolvable on the public internet or an reachable ip address). 10.9 includes many fixes to prevent the web client from sending "localhost" or "127.0.0.1" as address solving most connectivity issues. I have a prototype branch already that also displays the server address below the "Ready to cast!" text to help figuring out what address is sent.

I'd also like to ask everyone to use proper issues from now on instead of this pull request. There is also the option to use our Matrix/Discord chats for quick questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet