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

Make Jellyfin provider compatible with Emby #1325

Merged
merged 9 commits into from
Jun 9, 2024

Conversation

kingy444
Copy link
Contributor

@kingy444 kingy444 commented Jun 2, 2024

I am currently trying to write the Emby integration using their native API (but getting some strange import issues and my time is limited). In the process of looking at that i have taken a quick look at the JellyFin integration.

JellyFin having forked from an old version of Emby still functions very much the same so with a few tweaks i have been able to get the JellyFin plugin to work while maintaining all JellyFin capabilities.

  • Icon and server display fix - Updated Icon to flow with the rest of the icons (removed black background) and removed title which was being tagged into the track screens for title attribute
  • Simplify looping - Simply code clean up
  • Move to Artists Endpoint - Needed to handle Emby artists better, and works perfectly with JellyFin. While the current method worked for both it was messy with Emby as Composers are considered an Artist too
  • Handle playlists for both providers - just some IF logic to handle that JellyFin have improved their playlist functionality. Emby still present only audio playlists
  • Drop ParentId in favor of AlbumId - The main change required here was to use AlbumId instead of ParentId. Emby has diverged from ParentId relationship JellyFin inherited but the AlbumId relationship remains the same.
  • Filter on ArtistType - Emby allows further filtering, Jellyfin ignores
  • Fix double & when property should be comma delim - Was creating &IncludeItemTypes=MusicArtists&IncludeItemTypes=MusicArtist this should be &IncludeItemTypes=MusicArtists,MusicArtist

One thing i couldnt get working was the Friendly Name from the provider settings appearing in the track details. I couldnt reverse engineer where that is being created from to update the property. I wanted the Friendly Name from the "Music Providers" to appear in "Provider Details"

Provider Details Music Providers
image image

@marcelveldt
Copy link
Member

I'm not a big fan of this, we know for a fact that these 2 projects are deviating from eachother. Maybe just a little bit today but it will become more over time.

Why not just ask @lokiberra if you may copy his work on the Jellyfin provider as a base for a new Emby provider ?
That way it can be a bit more isolated and this will not turn into a big "if emby, then...." soup at some point.

@kingy444
Copy link
Contributor Author

kingy444 commented Jun 9, 2024

I figured there was only one IF statement required here to make things work - they deviate and it stops working then it stops working but the improvements here make sense anyway as they follow the same api calls both jellyfin and emby use

I'm not suggesting this is even advertised as working/supported 🤷‍♂️
But one IF statement for playlists seemed acceptable and the other changes align with the both emby and jellyfin current design.

I am trying to use the core emby api for a dedicated plugin and will get that submitted soon(ish). That would use emby python libraries, not jellyfins.

@marcelveldt
Copy link
Member

Ah, so this is basically just a temporary patch which makes it compatible now and then at some point there will be a separate Emby provider ?

Copy link
Member

@marcelveldt marcelveldt left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I'll leave final review to @lokiberra as he's the author/maintainer of the Jellyfin provider.

Thanks!

@lokiberra
Copy link
Contributor

Overall it looks good and I appreciate some of the cleanup.

I haven't had a chance to test. Has it been tested with a Jellyfin server? I would want to make sure it isn't breaking Jellyfin functionality.

If this is just temporary I'm fine with it. I agree with Marcel that Emby should be it's own provider, and it sounds like that is your plan.

@kingy444
Copy link
Contributor Author

kingy444 commented Jun 9, 2024

Overall it looks good and I appreciate some of the cleanup.

I haven't had a chance to test. Has it been tested with a Jellyfin server? I would want to make sure it isn't breaking Jellyfin functionality.

If this is just temporary I'm fine with it. I agree with Marcel that Emby should be it's own provider, and it sounds like that is your plan.

Sorry should have noted in the PR - but yep I've got both emby and jellyfin running so tested all was working

@marcelveldt marcelveldt merged commit 22f9fb1 into music-assistant:main Jun 9, 2024
4 of 5 checks passed
@Jc2k
Copy link
Contributor

Jc2k commented Jun 10, 2024

I updated to the dev branch this morning to test #1338 and noticed a new error:

Traceback (most recent call last):
  File "/usr/local/lib/python3.12/site-packages/music_assistant/server/controllers/music.py", line 706, in run_sync
    await provider.sync_library(media_types)
  File "/usr/local/lib/python3.12/site-packages/music_assistant/server/models/music_provider.py", line 401, in sync_library
    async for prov_item in self._get_library_gen(media_type):
  File "/usr/local/lib/python3.12/site-packages/music_assistant/server/providers/jellyfin/__init__.py", line 630, in get_library_tracks
    yield await self._parse_track(track)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/music_assistant/server/providers/jellyfin/__init__.py", line 463, in _parse_track
    track.artists.append(await self._parse_artist(name=VARIOUS_ARTISTS_NAME))
                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: JellyfinProvider._parse_artist() got an unexpected keyword argument 'name'

This seems to be an existing bug, but wondering if this PR is why i'm heading down that branch. Will try and look at it some more ASAP, but wanted to flag it.

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

Successfully merging this pull request may close these issues.

4 participants