feat: allow music videos to be imported from Jellyfin #1870
Conversation
shidetian
changed the title
| @@ -454,7 +456,15 @@ async def get_stream_details( | |||
| jellyfin_track = await self._client.get_track(item_id) | |||
| mimetype = self._media_mime_type(jellyfin_track) | |||
| media_stream = jellyfin_track[ITEM_KEY_MEDIA_STREAMS][0] | |||
| url = self._client.audio_url(jellyfin_track[ITEM_KEY_ID], SUPPORTED_CONTAINER_FORMATS) | |||
| if jellyfin_track[ITEM_KEY_MEDIA_TYPE] == "Video": | |||
There was a problem hiding this comment.
Right now the code on dev assumes there is a MediaStream[0] and if there is len(MediaStream) > 1, then the first one is the right one. This has been something I've wanted to change.
I think you looping over all streams is something we should do for all streams, regardless of the ITEM_KEY_MEDIA_TYPE.
Maybe loop once and find a supported codec of type audio, and just stream it as is. If that falls through, loop again and find video containers and transcode those.
If neither matches right now i think its best if we raise a MusicAssistantError.
| url = self._client.audio_url(jellyfin_track[ITEM_KEY_ID], SUPPORTED_CONTAINER_FORMATS) | ||
| if jellyfin_track[ITEM_KEY_MEDIA_TYPE] == "Video": | ||
| for stream in jellyfin_track[ITEM_KEY_MEDIA_STREAMS]: | ||
| if stream.get(ITEM_KEY_MEDIA_CODEC) in SUPPORTED_CONTAINER_FORMATS: |
There was a problem hiding this comment.
Theres a couple of bits i'm not sure about here.
- The for loop doesn't have an else, so if every stream fails this test, it will transcode the last stream in the list regardless. Feels like it should bail if there are no matching streams?
- I'm surprised that mypy is passing here -
.getcan return None, andSUPPORTED_CONTAINER_FORMATSis a string, so you'd be doingNone in "foo"which is a typing error?
(Anywhere you see us passing strings containing a comma seperated list (SUPPORTED_CONTAINER_FORMATS), its something from the old upstream jellyfin library that needs fixing (list/set of enums?) that we haven't had time to get to yet).
There was a problem hiding this comment.
- This code as currently written (unless I am mistaken) keeps the current behavior by choosing the first element if no matching streams are found
SUPPORTED_CONTAINER_FORMATS is an array, not stringsorry I misread
There was a problem hiding this comment.
Mypy might not be passing. I don't think it ran in CI pipeline.
There was a problem hiding this comment.
Ok I see it now. It selects the media_stream that matches SUPPORTED_CONTAINER_FORMATS, or it selects the first one.
Then, it transcodes. It doesn't matter which stream we have selected, Jellyfin picks its own.
Then the AudioFormat data is based on either the stream that matches SUPPORTED_CONTAINER_FORMATS, or the first stream in the list. Not the stream the Jellyfin returns.
I think if there are multiple audio or video streams, we actually can't predict which of those Jellyfin will return, so we might set AudioFormat incorrectly (thats a bug that already exists). If there is only 1 stream, and its a video, we can set the content type because we know that we asked for flac?
There was a problem hiding this comment.
Yes, I found that even you choose the exact same format as the original, Jellyfin always transcodes if the source is a video (hence my comment about demux). Since it will always transcode, I choose flac to maintain the original audio quality. I found that without specifiying the transcode container, Jellyfin just picks ACC as the codec to transcode to. As for what Jellyfin picks as the audio stream if there are multiple, the documented behavior for the Audio endpoint is If omitted the first audio stream will be used. which I assume applies to the universal endpoint as well. Since the AudioFormat ends up being shown on the UI, I choose to report the source's original format even though in actuality it's always going to be flac when playing videos.
As for what happens in the fallback, if it's a video stream, then it will not have a channel param, so the code fails on media_stream[ITEM_KEY_MEDIA_CHANNELS], and playback will fail. Maybe the first stream that contains channels might be better as a fallback since we're transcoding anyways. I didn't test the really pathological case where a video contains no audio stream but that should currently fail the same way.
|
Marked as draft until the discussion in Jc2k/aiojellyfin#2 is settled. |
This PR adds basic support for music video type from Jellyfin to allow music only playback. Technically the same approach can be applied to all videos from Jellyfin, but that is unlikely to be desirable for most users. Maybe this should be a setting for what libraries to include.
Note this requires Jc2k/aiojellyfin#2
Maybe also need a note as this requires the user set up in Jellyfin to have transcode permissions.