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

Add Vimu external player support and fix some MX Player issues #1719

Merged
merged 1 commit into from
May 28, 2022

Conversation

Andy2244
Copy link
Contributor

Changes

  • add Vimu external player support
  • add better title support on external playback
  • add filename to mx player extras
  • fix mPosition conversion to int
  • add detection for playback completion

@nielsvanvelzen nielsvanvelzen self-requested a review May 23, 2022 20:02
Copy link
Member

@nielsvanvelzen nielsvanvelzen left a comment

Choose a reason for hiding this comment

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

The external player support is planned for removal. The existing code for it will be kept "as-is" without any new features added to it. So unfortunately we won't be merging this pull request. When the playback rewrite (#1057) is done, external player support will be entirely removed.

@Andy2244
Copy link
Contributor Author

Andy2244 commented May 24, 2022

When the playback rewrite (#1057) is done, external player support will be entirely removed.

@nielsvanvelzen Yeah already heard about this from thornbill and noted my concerns on this decision. So i can only plead to please keep this option and allow me to get my PR merged? It does not really affect any of the other parts so why not just keep the option?

Here are some reasons users actually need this feature:

  • boxes like the Zidoo, Dune HD, Zapiti only really work correctly with the internal player and the enabled dolby VS10 engine
  • in players like Vimu/Kodi you can setup live AC3 transcoding, which makes it much easier to setup a working audio playback chain, without relying on the ability from the server to correctly detect what audio to transcode (also broken atm and it transcodes video as well)
  • Jellyfin does not allow transcode surround PCM from ogg/opus 5.1 into a AC3 5.1, there is no setting for this?
  • Exoplayer does not correctly support ASS subtitles with all features
  • Jellyfin has a limited way on setting/handling, audio/subtitle track selection, in contrast mpv, MX Player allows for multiple language selection and correctly handles default/forced flagged tracks

I checked Emby and even they still keep the external player option, so i wonder why would you remove a good option that seems reasonable to maintain?
I always liked FOSS because it gives options to users, so would have expect this from something like Plex/Emby not Jellyfin.

@nielsvanvelzen
Copy link
Member

Right now external players work (although the code is quite a hack..) but with the rewritten playback code it doesn't. So sure, merging this now is possible, but in the end it will be removed. That's why I preferred not to change it.

I'm well aware that with the current releases video playback isn't perfect. The arguments about incorrect codecs, lack of advanced substation subtitles support are all things that will be fixed in the rewritten code together with options to manually change supported codecs (and thus when transcoding will happen).

I'll review this PR and we'll merge it, but if there's any issues with it after release the PR will be reverted as I have no interest in fixing potential bugs with it.

@Andy2244
Copy link
Contributor Author

Andy2244 commented May 24, 2022

I'm well aware that with the current releases video playback isn't perfect.

I don't expect it to-be perfect that is the point, not even Plex manages to get to what i want. That's why i explicitly picked Jellyfin over Emby/Plex, since i can just use a external player which fixes most of my issues and send in PR for issues i can fix.

Yet as noted the biggest issue is that the default Jellyfin player will "probably" never be able to support stuff like dolby VS10. It also seems a lot of the streaming boxes out there, also have custom hacks/fixes in there internal player (gallery3d app) to circumvent issues that are specific to the chip. We also know most vendors are just too lazy to try backport those fixes to Exoplayer, so users will be left stranded and either cant use Jellyfin or must accept the hardware issues of there device.

I'll review this PR and we'll merge it, but if there's any issues with it after release the PR will be reverted as I have no interest in fixing potential bugs with it.

Fair enough, thanks!

@Andy2244 Andy2244 force-pushed the external_player_fixes branch 2 times, most recently from e96d95b to 143d34e Compare May 24, 2022 16:00
@Andy2244
Copy link
Contributor Author

ok all fixed

@Andy2244 Andy2244 force-pushed the external_player_fixes branch from 143d34e to de7d72d Compare May 24, 2022 17:39
@nielsvanvelzen nielsvanvelzen self-requested a review May 24, 2022 19:51
* add Vimu external player support
* add better title support on external playback
* add filename to mx player extras
* fix mPosition conversion to int
* add detection for playback completion
@Andy2244 Andy2244 force-pushed the external_player_fixes branch from de7d72d to e180106 Compare May 25, 2022 08:18
@nielsvanvelzen nielsvanvelzen merged commit f9b647c into jellyfin:master May 28, 2022
@Andy2244 Andy2244 deleted the external_player_fixes branch June 11, 2022 19:06
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.

2 participants