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 media segment skipping #6157

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

thornbill
Copy link
Member

@thornbill thornbill commented Oct 2, 2024

Changes

  • Adds support for skipping media segments

Needs cleanup + testing so marking as a draft for now

Issues
N/A

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

ESLint doesn't pass. Please fix all ESLint issues.

private debouncedOnTimeUpdate = throttle(this.handleSegmentActions, 500, { leading: true });

private async fetchMediaSegments(api: Api, itemId: string, includeSegmentTypes: MediaSegmentType[]) {
// FIXME: Replace with SDK getMediaSegmentsApi function when available in stable
Copy link
Contributor

Choose a reason for hiding this comment

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

Unexpected 'fixme' comment: 'FIXME: Replace with SDK...'. no-warning-comments

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

ESLint doesn't pass. Please fix all ESLint issues.

@viown
Copy link
Member

viown commented Oct 4, 2024

Just some things I noted when testing:

  • If I have "Skip" enabled and seek to a duration between the segment, it'll immediately take me to the end of the segment. If there is something there I wanted to take a look at I wouldn't be able to get to it. Maybe it should only skip when it hits the start time of the segment or perhaps avoid skipping altogether when the user manually seeks?
  • When it's about to skip a segment, it can look like the stream is frozen (this is amplified when transcoding). Having some kind of visual indicator (like the one for syncplay) would probably work well here.

@solidsnake1298
Copy link
Member

From my initial testing, this reliably skips intros, but it has not skipped outros or previews.

I'm going to create some test media so I can test the other segment types.

@viown
Copy link
Member

viown commented Oct 7, 2024

From my initial testing, this reliably skips intros, but it has not skipped outros or previews.

I'm going to create some test media so I can test the other segment types.

It looks like all segments after the first will not be skipped because segment is not updated here:

for (let index = lastIndex, segment = segments[index]; index >= 0 && index < segments.length; index += direction) {

Copy link

sonarcloud bot commented Oct 7, 2024

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

ESLint doesn't pass. Please fix all ESLint issues.

@jellyfin-bot
Copy link
Collaborator

Cloudflare Pages deployment

Latest commit 54713f90d93b5bdf7f81657b77ee88741cd4ba11
Status ✅ Deployed!
Preview URL https://d77c436c.jellyfin-web.pages.dev
Type 🔀 Preview

@solidsnake1298
Copy link
Member

The latest commit is skipping intro, outro, and preview segments reliably. I haven't gotten a chance to create test media with other segment types, but I don't see why those wouldn't also skip.

@solidsnake1298
Copy link
Member

I created some test media for recap and commercial segments, which were also skipped.

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

Successfully merging this pull request may close these issues.

4 participants