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

feat: Chapter additions #466

Conversation

Natanel-Shitrit
Copy link
Contributor

@Natanel-Shitrit Natanel-Shitrit commented Aug 12, 2023

Summary

PR Additions:

  • Chapter markers.
    markers
  • Move to previous / next chapter on left / right long press on screen.
    skip-chapter
  • Chapter name on skip.
    chapter-name
  • App preferences.
    Screenshot_20230812-155849~3
    Screenshot_20230812-155849~2

Issues

TODO:

  1. Implement chapter extraction from ExoPlayer. (StackOverflow)
    Dropped ExoPlayer because I couldn't find any way to get the metadata 😢
    ExoPlayer now supported, thanks @jarnedemeulemeester for pointing out Jellyfin API chapter info!
  2. Implement app preference for turning off / on markers / gesture.
    Done.
  3. Use system "Material You" theme for markers.
    This is probably fine as white (timebar contrast color)
  4. Fix code reuse in PlayerGestureHelper.kt.
    Would be better to refactor in a new PR with other changes to the same file.
  5. Add chapter name in player title.
    I think I'll keep it simple without changing the title for now.

Credits:

@Natanel-Shitrit Natanel-Shitrit marked this pull request as draft August 12, 2023 01:10
@Natanel-Shitrit Natanel-Shitrit changed the title Add chapter markers and "skip chapter" on long press feat: MKV Chapter additions Aug 12, 2023
@Natanel-Shitrit Natanel-Shitrit marked this pull request as ready for review August 12, 2023 15:54
@Natanel-Shitrit
Copy link
Contributor Author

@jarnedemeulemeester I think this is ready 😄
If you have any suggestion, would be happy to implement!

@Natanel-Shitrit Natanel-Shitrit force-pushed the feature/chapter-selector branch from adaad0f to d921c83 Compare August 17, 2023 21:41
@Natanel-Shitrit
Copy link
Contributor Author

Rebased with main PiP changes

@Natanel-Shitrit
Copy link
Contributor Author

First version crashed when skipping an episode, because it tried to load the chapters too early.
This is now fixed and ready for review 😄
@jarnedemeulemeester

@realFPS
Copy link

realFPS commented Jan 13, 2024

Thanks for doing this. I don't know how helpful it can be but there is this player which also uses exoplayer and has chapter support.

@Natanel-Shitrit
Copy link
Contributor Author

Natanel-Shitrit commented Jan 13, 2024

Thanks for doing this. I don't know how helpful it can be but there is this player which also uses exoplayer and has chapter support.

That's certainly better than nothing.
I'll look into this :)
Thanks!

@Natanel-Shitrit
Copy link
Contributor Author

Seems like a lot of work...
moneytoo/Player@63c9200

He pulls the chapters from the media info he gets from FFmpeg library.
I originally tried this and gave up because it felt too much work for the insignificant importance of such feature.

I think the MPV player is mature enough to be preferred over ExoPlayer and thus it's not really a big deal.
But, of course anyone is welcome to try and implement this for the ExoPlayer.

@Natanel-Shitrit
Copy link
Contributor Author

Oops, I didn't mean to merge this.
Please don't merge until I fix this.

Copy link
Owner

@jarnedemeulemeester jarnedemeulemeester left a comment

Choose a reason for hiding this comment

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

Hi @Natanel-Shitrit
Thanks a lot for implementing this!

I do have a few suggestions.
I see you are using mpv to get the chapters from the item and that's the reason why it is only available when using mpv. But apparently Jellyfin already extracts the chapters from items as well.
You should be able to use the API to retrieve the chapters and display them in both Exoplayer and mpv 😄

It seems to be part of the BaseItem so no need to call an extra API endpoint. You probably only need to add a few properties to some Findroid* classes and maybe create a new FindroidChapter class or something like that.

"Chapters": [
    {
        "StartPositionTicks": 0,
        "Name": "string",
        "ImagePath": "string",
        "ImageDateModified": "2019-08-24T14:15:22Z",
        "ImageTag": "string"
    },
]

I think we can ignore the Image part of the chapter for now since this has to be manually enabled for each library anyway.

Also I recently merged a PR which add 2x playback speed on long press. That obviously conflicts with the skipping to next chapter on long press. So we have to change one or the other, but I don't really know which one yet and to what action.

@Jcuhfehl
Copy link
Contributor

Also I recently merged a PR which add 2x playback speed on long press. That obviously conflicts with the skipping to next chapter on long press. So we have to change one or the other, but I don't really know which one yet and to what action.

I already mentioned this in the discord, but I think using the next/previous episode buttons for chapters would be a good option, since it feels like it's doing a similar thing. Another option might be the long-press on the fast forward button.

I think long pressing the right of the screen is a very appropriate gesture for 2x so I'd prefer leaving that unchanged

@Natanel-Shitrit
Copy link
Contributor Author

Natanel-Shitrit commented Jan 23, 2024

@jarnedemeulemeester Thanks for the detailed response!

That's a nice surprise that Jellyfin already extracts the chapter info, this will make it easier for compatibility since it will not rely on the MPV functionality.

I'll try and Implement a POC for the chapter extraction from Jellyfin.

Regarding the skip chapter gesture I think giving the user the ability to customize it will be the best approach here - obviously this will take time and effort but I think the best way to implement this is to create a settings page for player gestures - list of default gestures, and give the ability to remove / modify / add from that list.

Each gestures can be represented like this:

  • Tap Type - Double tap / Long press
  • Location - Right / Middle / Left / All
  • Action - Action to perform

I'll open a feature request and try to implement this too.

But probably only after this will be merged since it will probably something bigger.

@Natanel-Shitrit Natanel-Shitrit force-pushed the feature/chapter-selector branch from 8be5fec to 90099b7 Compare January 23, 2024 22:21
@Natanel-Shitrit Natanel-Shitrit marked this pull request as draft January 23, 2024 23:38
@Natanel-Shitrit
Copy link
Contributor Author

@garvongithub @jarnedemeulemeester Now both players are supported!
I've successfully migrated to Jellyfin API for getting information about chapters.

@Natanel-Shitrit
Copy link
Contributor Author

Natanel-Shitrit commented Jan 24, 2024

TODO:

  • Integrate chapter skipping in the "skip media" buttons.

when chapters are enabled and available for specific media, clicking on previous / next media buttons in the player will skip chapters instead of the media it-self.

  • Save chapter info for local items (downloaded media) to support new features offline.

@Natanel-Shitrit Natanel-Shitrit force-pushed the feature/chapter-selector branch from b8e7b87 to b3e00fb Compare January 28, 2024 19:24
@Natanel-Shitrit Natanel-Shitrit marked this pull request as ready for review January 28, 2024 23:32
Copy link
Owner

@jarnedemeulemeester jarnedemeulemeester left a comment

Choose a reason for hiding this comment

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

Nice work implementing the chapters from the Jellyfin API.
I just tested it out but there are still a few issues:

  • App immediately crashes on launch because of database changes. A database version bump and automigration need to be added. But when I tried this it still crashed (I'm still investigating this)
  • When seeking to chapters (using the long press gesture) it would display the name of the chapter I came from instead of the one I seeked to. This only happens when using mpv and seems to be a race condition issue. I'm working on a fix which would return the chapter when seeking to it so we always display the correct name.
  • Maybe we should change the name of this PR and in the app to something more generic like "chapters" and not specific "MKV chapters" because this feature will also work for chapters in mp4 files.

Great work so far! I don't mind fixing up the last few issues 🙂

@Natanel-Shitrit Natanel-Shitrit changed the title feat: MKV Chapter additions feat: Chapter additions Feb 12, 2024
@Natanel-Shitrit
Copy link
Contributor Author

Thanks for testing!

Feel free to fix / change anything you see fit!

if you want - I can tackle the bugs on the weekend but I guess you can do that much faster and better than me😅

@jarnedemeulemeester
Copy link
Owner

  • Fixed seeking in mpv by returning the chapter that is being sought to
  • Simplified getCurrentChapterIndex
  • Seeking to previous chapter now returns the current chapter if playback progress is more than 5 seconds past the start of the chapter
  • Added some docstring
  • Changed the naming to just "Chapters"

Still need to fix the database issue 😅

@Natanel-Shitrit
Copy link
Contributor Author

@jarnedemeulemeester I think I fixed it?

@jarnedemeulemeester
Copy link
Owner

@Natanel-Shitrit Yep, that fixed it!

@jarnedemeulemeester
Copy link
Owner

I will do one final check of everything. If nothing goes wrong I will merge it!

@jarnedemeulemeester jarnedemeulemeester merged commit c39bdce into jarnedemeulemeester:main Feb 17, 2024
2 checks passed
@Natanel-Shitrit Natanel-Shitrit deleted the feature/chapter-selector branch February 17, 2024 15:48
@Natanel-Shitrit
Copy link
Contributor Author

I think the only thing we forgot is to add the new feature in the README.md, I'll open a new PR 😄

ghost pushed a commit to pruthvi-21/findroid that referenced this pull request Jul 22, 2024
* Add chapter markers and "skip chapter" on long press

* Fix linting problems

- Missing comma
- Unused import
- Comment block

* Add preferences options

* Drop chapter support for ExoPlayer

* Fix linting

* Remove Trailing spaces

* Remove TODO from marker color

* Move code to function

* Optimize imports

* Fix crash on episode skip

* Disable player control view animation

* Avoid crash when there are no chapters for media item

* Skip to next episode when skipping last chapter

* Load chapters from Jellyfin API instead of MPV Player

* Remove chapter gesture

* Fix build

* Fix linting

* Fix linting

* Support chapters with offline media

* Remove debug print

* Add chapter skipping

* Remove trailing spaces

* fix(chapters): display correct chapter while seeking

* refactor: faster and cleaner `getCurrentChapterIndex`

* refactor: seek to start of current chapter if player position is more than 5 seconds past start of chapter

* refactor: change "Matroska chapters" to just "Chapters"

The chapters feature also works for MP4 files so just make it generic

* Bump database version

* Add auto-migration for database version bump

* Save database schema

* chore: clean up

---------

Co-authored-by: Jarne Demeulemeester <[email protected]>
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.

[Feature Request] Support for MKV chapters [Feature Request] Video Chapter Features
4 participants