Feature: Media Segments #10530
Feature: Media Segments #10530
Conversation
Changes in OpenAPI specification found. Expand to see details.What's New
|
There was a problem hiding this comment.
Awesome to see an implementation of our long discussed segment API! I've looked into your current implementation comparing it with the meta discussion (which I found some things I disagree with as well) and analyzed it from a client developer's perspective (as I maintain the Android apps). It's essential to get the HTTP API right from the start to avoid breaking changes in future releases. Here is a list of my initial thoughts and suggestions:
Timing is not using ticks
All timings in the API use ticks. While I still think this is stupid we should also keep everything consistent and thus use ticks for MediaSegments as well.
Segments should be per source/stream
A single BaseItem can have multiple MediaSources with various MediaStreams. When playing a video, we typically have one video stream, one audio stream and one subtitle stream active. These streams can all originate from the same media source or from different ones. If I have one source that doesn't contain advertisements it will have need segments returned. The API needs to be changed to return all the segments for all streams, not for the item. The client can then use only the segments from active streams.
Integration with existing APIs
The current implementation ads the "MediaSegmentApi" that can be used to list/get/update/delete segments. While this is nice to have we must also return segments with their respective items. When (video/audio) playback starts we already have a lot of API requests which is slow and inefficient (lots of duplicate database querying etc.). We should reduce this amount, not increase it. Adding a "segments" property to MediaStream should be fine for this.
Media segment actions
I'm not sure if I like this. Different users will want different actions to happen. For instance, user A might want to see all intros/credits but user B wants to skip them and user C wants to mute only the credits. The current design does not account for that. It might be better to drop the actions and add some preferences that can vary per user.
Documentation
The DTO documentation is lacking. "Class MediaSegment." is not descriptive enough. If we want all official and unofficial clients to implement the segment API we should make sure it's behavior and usage is documented first. Fortunately some of the properties have better documentation so this should be a relatively easy change.
SyncPlay
How is this going to work with SyncPlay? I feel like we need to give this some special attention because SyncPlay is known to break a lot when playback stuff changes.
Chapters
One thing we discussed was to merge chapters and the segment proposals to make it easier to work with both. This is currently not the case. But if you think about it a chapter is basically a segment with no end (or we could just calculate the end based on the video duration/next chapter start) and some additional metadata (name/image).
I'm not sure I agree with this. While I do see the use-case for per-user configuration, adding any per-user configuration, especially as complex as this, is non-trivial, and our per-user configuration is already huge. I may be wrong, but I imagine the vast majority of the use of this feature will be for prompting to skip intros/credits. IMO, it's best to keep it server-wide for now, since we can always re-visit it if there is a demand for it to be user-configurable. |
|
Thank you for your fast response.
Moved to ticks
Added a
Yes, is missing and i will look into it.
Will look into it.
Never tested, as i have just a implementation for jellyfin-vue (which has no syncplay implementation i think). If two users seek at the same time what happens? I think there is nothing we can do at this point. Probably that needs to be disabled when syncplay is active.
This is a harder one as it changes more than it looks like. I checked the source code and noticed some things. Chapters or better said the
The idea was to have access to every single segment action and influence the action also from plugin providers while creating them. By default the action is evaluated client side with possible user settings or |
|
I had a look at the current structure. I implemented now a very simple version of the api integration. Thoughts
"Name": "School of Rick",
"Id": "cbdd1d49c4fa548247e2238855e6a759",
"MediaSources": [
{
"Protocol": "File",
"Id": "cbdd1d49c4fa548247e2238855e6a759",
"MediaStreams": [
{
"Codec": "h264",
"Index": 0
},
{
"Codec": "ac3",
"Index": 1
},
{
"Codec": "ac3",
"Index": 2
}
],
"Bitrate": 4570855,
},
{
"Protocol": "File",
"Id": "abcderfghijklmnopqrstuvwqyz123456789",
"MediaStreams": [
{
"Codec": "h265",
"Index": 0
},
{
"Codec": "ac3",
"Index": 1
},
{
"Codec": "ac3",
"Index": 2
}
],
"Bitrate": 4570855,
}
],
"SeriesName": "Rick and Morty",
"SeasonName": "Staffel 1",
"MediaStreams": [
{
"Codec": "h264",
"Index": 0
},
{
"Codec": "ac3",
"Index": 1
},
{
"Codec": "ac3",
"Index": 2
}
],
"MediaSegments": [
{
"StartTicks": 0,
"EndTicks": 1000000,
"Type": "Intro",
"TypeIndex": 0,
"ItemId": "cbdd1d49c4fa548247e2238855e6a759",
"StreamIndex": 0
}
],
// can be also moved to a cleaner data structure like the trickplay layout
"MediaSegments": {
"cbdd1d49c4fa548247e2238855e6a759": [
{
"StartTicks": 0,
"EndTicks": 1000000,
"Type": "Intro",
"StreamIndex": 0
}
],
"abcderfghijklmnopqrstuvwqyz123456789": [
{
"StartTicks": 0,
"EndTicks": 1000000,
"Type": "Intro",
"StreamIndex": 0
}
]
},
"Chapters": [
{
"StartPositionTicks": 0,
"Name": "Kapitel 1",
"ImageDateModified": "0001-01-01T00:00:00.0000000Z"
},
{
"StartPositionTicks": 1498400000,
"Name": "Kapitel 2",
"ImageDateModified": "0001-01-01T00:00:00.0000000Z"
},
{
"StartPositionTicks": 4953600000,
"Name": "Kapitel 3",
"ImageDateModified": "0001-01-01T00:00:00.0000000Z"
},
{
"StartPositionTicks": 12447600000,
"Name": "Kapitel 4",
"ImageDateModified": "0001-01-01T00:00:00.0000000Z"
}
], |
JPVenson
added
good first issue
Jellyfin.Server.Implementations/MediaSegments/MediaSegmentsManager.cs
Outdated
Show resolved
Hide resolved
|
fwiw this was my start at implementing this: thornbill@6504478. I think the model captures what we had ended up agreeing to in the original discussion fairly well. Generally I would agree with the other comments that actions should be left to clients to handle... although there are a couple segment types that blur the line somewhat. Those were mainly to support comskip and/or sponsorblock types. I'm really glad you picked this up though because I had not made much progress with the related database changes. 🤣 |
|
@thornbill Chapters |
jellyfin-bot
added
the
merge conflict
This comment has been minimized.
This comment has been minimized.
jellyfin-bot
removed
the
merge conflict
This comment has been minimized.
This comment has been minimized.
jellyfin-bot
added
the
merge conflict
I think a better approach is that the "host" of the sync play session is the only one who can adjust the media playback. Perhaps I'm ill informed but I don't see this as a major issue either way. |
This comment has been minimized.
This comment has been minimized.
|
Hey @endrl |
jellyfin-bot
removed
the
merge conflict
This reverts commit 5551f0c.
There was a problem hiding this comment.
In my last review I've asked for better descriptions of the various (API exposed) properties so it's documented what kind of values to expect and how to use them. While some changes were made, some other properties have not been updated (e.g. TypeIndex, still no idea what that is for).
Additionally, some of these changes were made to the <value> element only. The content of those are not used for the generated OpenAPI specification, only the <summary> is. So please update that one as well.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@nielsvanvelzen Regarding the |
jellyfin-bot
added
the
merge conflict
This comment has been minimized.
This comment has been minimized.
jellyfin-bot
removed
the
merge conflict
|
@endrl can you update this PR to the latest master branch and address the remaining review feedback? |
|
Why is no one allowed to take over this pull request despite the authors' three month disappearance? |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment has been minimized.
This comment has been minimized.
This reverts commit c6486d0.
|
Hey everyone. This PR is a hot topic for quite some time now with a lot of back and forth. Lots of people anticipated work and a result of this PR but i unfortunatly have some rather bad news for the current state of this PR. As maybe not many of you know, the jellyfin server team tries to communicate its design ideas and architecture at a seperate repro and this includes a long time discussion about media segments. This discussion jellyfin/jellyfin-meta#30 (comment) however resulted in the server teams decision that a media segement api should be build with a very tight scope and very specific set of features, for example excluding a webapi to create new segements. This is done to ensure the system is as streamlined as possible. So in discussion with the server team, i have to notify you that this PR will no longer be considered for as long as it does not follow the points laid out in the discussion. |
This comment has been minimized.
This comment has been minimized.
I want to propose my take for the "Intro Outro" feature. This is considered a basic implementation that can be advanced.
Meta Ref: jellyfin/jellyfin-meta#30
Thoughts
Testing
To explore the ecosystem i created some implementations. A docker compose file is available.
Related projects
Happy testing! Feedback welcome.