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

MPV #475

Closed
wants to merge 1 commit into from
Closed

MPV #475

wants to merge 1 commit into from

Conversation

CarlosOlivo
Copy link
Contributor

Here is my attempt to integrate MPV with Jellyfin Android

This PR is made up of 2 parts:

  • The native Java-Android binding of libmpv (see here)
  • And a Player wrapper (this PR)

Jellyfin <-> MPVPlayer <-> Player <-> MPVLib <-> libmpv <-> mpv

The advantage of doing it this way is that you can share code with ExoPlayer, so there is no need to maintain another UI, for example.

The only thing left is to decide how to build, publish and integrate the native part, so I would appreciate your help in this regard.

At this PR, I pre-built the native part for the following architectures to test:

  • arm64-v8a
  • armeabi-v7a
  • x86
  • x86_64

At the time of sending this PR it supports the same ExoPlayer features + more codecs support (mainly support for .ssa, .ass subtitles, the reason why this PR exists in the first place)

The mpv files are located at:

/sdcard/Android/data/org.jellyfin.mobile[.debug]/files/mpv/

Here you can modify the configuration, add fonts or even scripts to customize mpv

@nielsvanvelzen nielsvanvelzen self-requested a review July 31, 2021 19:36
@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Jul 31, 2021
@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Aug 3, 2021
@CarlosOlivo
Copy link
Contributor Author

Typo

if (player is MPVPlayer) {
    return player.selectTrack(
        trackType = TrackType.AUDIO,
-       index = mediaSource.selectedSubtitleStream?.index ?: C.INDEX_UNSET
+       index = mediaSource.selectedAudioStream?.index ?: C.INDEX_UNSET
    )
}

Can you try again?

@steelersfan7
Copy link

Hi,

I was testing and came across something that plays in exoplayer, but not in mpv. Its possible that its the way the server sends the request, as the file is a strm file. I have the logs, it is the debug version, so long and full of spam. Do you prefer just a raw post in here, or something like a pastebin.

@CarlosOlivo
Copy link
Contributor Author

I have the logs, it is the debug version, so long and full of spam.

Well yeah it's verbose on purpose.

Do you prefer just a raw post in here, or something like a pastebin.

You can also attach a .txt file with the log here or upload it to pastebin, as you like.

@steelersfan7
Copy link

Here are the logs as a .txt.
thanks
logs.txt

@CarlosOlivo
Copy link
Contributor Author

2021-08-03 23:20:21.510 15580-16338/org.jellyfin.mobile.debug V/mpv: [ffmpeg:v] Opening https://domain.com/Videos/66074ce5-908f-e423-1180-14f2f53c7944/stream?static=true&mediaSourceId=66074ce5908fe423118014f2f53c7944&streamOptions=%7B%7D
2021-08-03 23:21:22.136 15580-16338/org.jellyfin.mobile.debug V/mpv: [ffmpeg:error] tcp: ffurl_read returned 0xffffff92
2021-08-03 23:21:22.143 15580-16338/org.jellyfin.mobile.debug V/mpv: [stream:error] Failed to open https://domain.com/Videos/66074ce5-908f-e423-1180-14f2f53c7944/stream?static=true&mediaSourceId=66074ce5908fe423118014f2f53c7944&streamOptions=%7B%7D.
2021-08-03 23:21:22.143 15580-16338/org.jellyfin.mobile.debug V/mpv: [cplayer:v] Opening failed or was aborted: https://domain.com/Videos/66074ce5-908f-e423-1180-14f2f53c7944/stream?static=true&mediaSourceId=66074ce5908fe423118014f2f53c7944&streamOptions=%7B%7D

Apparently ffmpeg just gives up, can you test if using mpv-android is the same result?

  • Client Settings
    Video Player type = External Player
    External Player app = MPV Player

Also Jellyfin Media Player on your desktop

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.

As you might have noticed we didn't review this PR (and some others) yet. The reason being that we (Max and I) were busy with the 2.3 release. My primary focus is on the Android TV app and Max was busy with life so that's why this repository is not moving that fast. With 2.3 now released the other PR's should be reviewed soon.

This PR however is a significant change and we're still discussing this internally. Personally I'd love to support MPV natively. But I'm not sure if we want it right now as this would require the playback rewrite (see below) to also support MPV before moving the app over.

The playback rewrite is a big project happening in the Android TV app that completely rewrites the playback code. It is intended for use in both apps so the behavior is consistent. The rewrite already plans to support LibVLC and ExoPlayer (the TV app already does that now) and MPV is on the future roadmap but requires additional work (and time).

More information about the playback rewrite is posted in this issue which coincidentally was published not long after this PR, although I started writing it a few weeks ago and planning it months ago :p.


Related to this PR however I have question. You made a fork of the mpv-android repository to re-use their work and build it as a library. Did you consider working with the authors to make changes to their repo to allow publishing the library part? Because right now we would need to maintain another repository for the mpv library and backport their fixes when they happen which is quite some work.

@steelersfan7
Copy link

2021-08-03 23:20:21.510 15580-16338/org.jellyfin.mobile.debug V/mpv: [ffmpeg:v] Opening https://domain.com/Videos/66074ce5-908f-e423-1180-14f2f53c7944/stream?static=true&mediaSourceId=66074ce5908fe423118014f2f53c7944&streamOptions=%7B%7D
2021-08-03 23:21:22.136 15580-16338/org.jellyfin.mobile.debug V/mpv: [ffmpeg:error] tcp: ffurl_read returned 0xffffff92
2021-08-03 23:21:22.143 15580-16338/org.jellyfin.mobile.debug V/mpv: [stream:error] Failed to open https://domain.com/Videos/66074ce5-908f-e423-1180-14f2f53c7944/stream?static=true&mediaSourceId=66074ce5908fe423118014f2f53c7944&streamOptions=%7B%7D.
2021-08-03 23:21:22.143 15580-16338/org.jellyfin.mobile.debug V/mpv: [cplayer:v] Opening failed or was aborted: https://domain.com/Videos/66074ce5-908f-e423-1180-14f2f53c7944/stream?static=true&mediaSourceId=66074ce5908fe423118014f2f53c7944&streamOptions=%7B%7D

Apparently ffmpeg just gives up, can you test if using mpv-android is the same result?

* Client Settings
  Video Player type = External Player
  External Player app = MPV Player

Also Jellyfin Media Player on your desktop

Jellyfin mpv on desktop works and the external to

@CarlosOlivo
Copy link
Contributor Author

My primary focus is on the Android TV app and Max was busy with life so that's why this repository is not moving that fast

I already knew, no problem, take your time ...

But I'm not sure if we want it right now as this would require the playback rewrite (see below) to also support MPV before moving the app over.

The idea is that it is not necessary to add any additional support, since MPVPlayer.kt shares the same interface as ExoPlayer, that is, if it supports SimpleExoPlayer, it supports MPVPlayer with minimal changes

...which coincidentally was published not long after this PR, although I started writing it a few weeks ago and planning it months ago :p.

Well me too, at least since:

mpv 0.33.0-109-gd0c530919d Copyright © 2000-2020 mpv/MPlayer/mplayer2 projects
built on Wed Apr  7 00:11:38 CDT 2021

Did you consider working with the authors to make changes to their repo to allow publishing the library part?

Not gonna happen, mpv does not provide official packages, you're expected to build your own.
See mpv.io page. All are unofficial third party builds

...we would need to maintain another repository for the mpv library and backport their fixes

It is not necessary to backport their fixes, the contents of the mpv-android fork are the buildscripts, every time the library is built it clones the mpv@master and the FFmpeg@master repository. So if any changes happen, you just need to rebuild the library. (That is possibly what I need to do to fix the above error with the strm files.)

Changes only need to be made when:

  • When external dependencies need to be updated (here)
  • When API changes happen (here)

@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Aug 6, 2021
@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Dec 2, 2021
@CarlosOlivo
Copy link
Contributor Author

Modified the profile to be similar to ExoPlayer profile
Rebased to mpv v0.34.0 & Jellyfin v2.4.2

Bonus: Added an experimental script to skip OP/ED client-side, it works by detecting silences in the video, its effectiveness depends on the quality of the media, the network and the device itself (RAM / CPU / GPU)
YpzpQVdPv9

@steelersfan7 if it still doesn't work for you, could you share a sample .strm file?

Download build here.

@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Dec 15, 2021
@Optinux
Copy link

Optinux commented Apr 24, 2022

Is this PR dead? Would love to see this finally get merged into the master branch!

@nielsvanvelzen
Copy link
Member

Is this PR dead? Would love to see this finally get merged into the master branch!

As mentioned before, there is a rewrite in progress for the playback code used in both Android apps. Significant changes like this one are therefor blocked. Besides, this PR has some issues; the major one being the fact that is relies on a binary blob, which could in theory contain malicious code (although I'm sure that's not the case here of course). For now using MPV as external player is the recommended usage if you want to use MPV.

@voronind-com

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge conflict Conflicts prevent merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants