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 User-Configurable Playback Duration for Media Segment Start and End #6200

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rlauuzo
Copy link
Contributor

@rlauuzo rlauuzo commented Oct 14, 2024

Introduces new user-configurable settings for adjusting the playback duration of media segment start and end times. The MediaSegmentManager is updated to pre-adjust segments based on these settings, giving users greater flexibility in playback control.

@rlauuzo rlauuzo requested a review from a team as a code owner October 14, 2024 14:14
@jellyfin-bot
Copy link
Collaborator

jellyfin-bot commented Oct 14, 2024

Cloudflare Pages deployment

Latest commit b34977b
Status ✅ Deployed!
Preview URL https://980065ff.jellyfin-web.pages.dev
Type 🔀 Preview

View build logs

Comment on lines 46 to 47
StartTicks: segment.StartTicks !== undefined ? segment.StartTicks + startPlayTime : undefined,
EndTicks: segment.EndTicks !== undefined ? segment.EndTicks - endPlayTime : undefined
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should validate that this doesn't result in StartTicks > EndTicks?

src/scripts/settings/userSettings.js Outdated Show resolved Hide resolved
src/scripts/settings/userSettings.js Outdated Show resolved Hide resolved
@dmitrylyzo
Copy link
Contributor

giving users greater flexibility in playback control.

Could you explain your use case?

It can be an adjustment to seek before an exact start/end time. But one setting is enough, imo. It can also be used for prev/next chapter buttons.

@rlauuzo
Copy link
Contributor Author

rlauuzo commented Oct 14, 2024

My intention is to allow users to adjust playback slightly before the segment ends, similar to how Netflix jumps a couple of seconds before the intro ends so that you still see the show's logo and avoid cutting off any actual content.

Regarding the settings—did I overlook an existing option that already handles this, or would this be a useful new feature?

@dmitrylyzo
Copy link
Contributor

dmitrylyzo commented Oct 14, 2024

My intention is to allow users to adjust playback slightly before the segment ends, similar to how Netflix jumps a couple of seconds before the intro ends so that you still see the show's logo and avoid cutting off any actual content.

That's exactly what I meant. But why do you need two separate settings, one for the beginning and one for the end?

UPD:
Answering myself: play a little more, then skip.

Regarding the settings—did I overlook an existing option that already handles this, or would this be a useful new feature?

There is currently no such adjustment. I just mean that the new setting can also be used to navigate through chapters.

Copy link

sonarcloud bot commented Oct 15, 2024

@rlauuzo
Copy link
Contributor Author

rlauuzo commented Oct 15, 2024

Applies the user's segment end setting to the next chapter skip feature. Also removes the previous adjustment for start position.

@thornbill
Copy link
Member

I really just don't see the point in this. If there is an issue with the segment timing that seems like an issue with either the media or the plugin providing the segment data. 🤷‍♂️

@jumoog
Copy link
Contributor

jumoog commented Oct 15, 2024

It's not a segment issue, it's more a user preference. At the moment we have that in our plugin and we have users who adjust the timings. Since there can be multiple plugins, it makes sense to move this to jellyfin now. I think that was @rlauuzo intention.

@AbandonedCart
Copy link

AbandonedCart commented Oct 15, 2024

I really just don't see the point in this. If there is an issue with the segment timing that seems like an issue with either the media or the plugin providing the segment data. 🤷‍♂️

Allow me to chime in with some use cases that may have been overlooked.

  1. Demon Slayer and a number of other anime use the same intro and then the same sound for the title splash screen. This title is indistinguishable from the intro itself. A lot of people want to see this title. Chapter markers often discard this.

  2. Users often want to retain a portion of the credits because action is still happening as they begin to roll. This is, once again, somewhat indistinguishable from the actual credits. Despite the lack of dialog, it is a part of the show. Chapter markers often overlook this.

  3. User preference. It’s something that a lot of users have requested for a number of other reasons. The other two points cover most of the logical reasons, but there are still a number of others with other reasons that specifically ask for instructions on how to edit.

I completely understand the assumption that the detection is the problem. Ironically, it’s that the detection is too precise, but it’s far easier to provide flexibility than try to force every file and user to come to an agreement.

@nielsvanvelzen
Copy link
Member

Shifting the start and end times of segments is something the segment provider should do. We could add options for that to our chapter segment provider if there is demand. It's definitely not up to the client to do. A client does not know the source of a segment and thus what the timings are based on.

@AbandonedCart
Copy link

AbandonedCart commented Oct 16, 2024

Shifting the start and end times of segments is something the segment provider should do.

That's fine. Keep in mind that the more you defer to the provider, the more heavily you are relying on a provider to be the feature. While a clean UI matters, the button and an endpoint is essentially the only part of the feature Jellyfin has actually integrated. All of the settings will still be performed in a plugin.

We could add options for that to our chapter segment provider if there is demand

Please don't. This only creates a more elaborate and confusing process for the user with far more risk of issues and error.

@rlauuzo
Copy link
Contributor Author

rlauuzo commented Oct 17, 2024

Shifting the start and end times of segments is something the segment provider should do. We could add options for that to our chapter segment provider if there is demand. It's definitely not up to the client to do. A client does not know the source of a segment and thus what the timings are based on.

I strongly disagree with that position. It's akin to suggesting that subtitle offset adjustments shouldn't be handled by the client. Instead, subtitle providers should rewrite all subtitle files with modified timings whenever a simple setting like offset is changed.

@jellyfin-bot
Copy link
Collaborator

This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.

@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Oct 25, 2024
@joshjryan
Copy link

joshjryan commented Oct 26, 2024

Shifting the start and end times of segments is something the segment provider should do. We could add options for that to our chapter segment provider if there is demand. It's definitely not up to the client to do. A client does not know the source of a segment and thus what the timings are based on.

I think the key part of this improvement is that it's not shifting the segment. It's giving an option to resume playback a few seconds before the segment ends.

I think it's great that segments are exactly accurate in start/end ticks. But, a user should have the option to skip the segment, but resume a few seconds early.

@ethaldeman
Copy link

I really just don't see the point in this. If there is an issue with the segment timing that seems like an issue with either the media or the plugin providing the segment data. 🤷‍♂️

Shifting the start and end times of segments is something the segment provider should do.

It isn't an issue with the segment data. The segment data is 100% accurate. Is is just the user preference to show a few seconds of the skipped intro or other media segment. Skipping right to end end of an intro can be jarring, and an option to view the last few seconds of the intro helps with this.

This is a capability of the Intro Skipper plugin I heavily use and would appreciate being formally added to Jellyfin.

} catch (err) {
console.error('[MediaSegmentManager] failed to fetch segments', err);
this.mediaSegments = [];
}
}

private adjustMediaSegments(segments: MediaSegmentDto[]): MediaSegmentDto[] {

Choose a reason for hiding this comment

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

In order to decrease confusion, I wonder if this method should be renamed to applySegmentSkipPreferences() (or something along those lines, so that it's clear that you aren't adjusting the segment itself).

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.

10 participants