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

Episode multiple versions #8004

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

Conversation

SenorSmartyPants
Copy link
Contributor

@SenorSmartyPants SenorSmartyPants commented Jun 24, 2022

Changes
Enable multiple versions for episodes. Episode suffixes work or episode may be placed in it's own directory. Will not collapse episodes in series directory. Unit tests included.

/TV/Dexter/Dexter - S01E01/Dexter - S01E01 - One.mkv
/TV/Dexter/Dexter - S01E01/Dexter - S01E01 - Two.mkv
/TV/Dexter/Dexter - S01E02 - Another.mkv
/TV/Dexter/Dexter - S01E03 - Again.mkv
/TV/Dexter/Season 2/Dexter - S02E01/Dexter - S02E01 - First.mkv
/TV/Dexter/Season 2/Dexter - S02E01/Dexter - S02E01 - Second.mkv
/TV/Dexter/Season 2/Dexter - S02E02 - More.mkv

JF will show 3 episodes in season 1. 2 episodes in season 2.

@SenorSmartyPants
Copy link
Contributor Author

One change that could affect current users. Movie versions that include a year as part of the version name need to be placed in brackets. Otherwise they appear as a possible episode and won't merge with other versions.

@SenorSmartyPants
Copy link
Contributor Author

@SenorSmartyPants
Copy link
Contributor Author

Related discussion. #7900

@SenorSmartyPants
Copy link
Contributor Author

Latest commit adds support for multiversion episodes in mixed folders (like season and series) so this file list is now supported. (Which Emby supports)

/TV/Dexter/Dexter - S01E01 - One.mkv
/TV/Dexter/Dexter - S01E01 - One plus.mkv
/TV/Dexter/Dexter - S01E02 - Two.mkv
/TV/Dexter/Dexter - S01E03 - Three.mkv
/TV/Dexter/Dexter - S02E01 - Ia.mkv
/TV/Dexter/Dexter - S02E01 - I.mkv
/TV/Dexter/Dexter - S02E02.mkv

@SenorSmartyPants SenorSmartyPants force-pushed the EpisodeMultipleVersions branch 2 times, most recently from 296c01e to bcedad2 Compare July 1, 2022 20:48
@SenorSmartyPants
Copy link
Contributor Author

Force pushed to rebase to current master.

@JaidenW
Copy link

JaidenW commented Jul 5, 2022

Super interested in this pull.

Question, in it's current format, would this allow me to have multiple versions of an episode, even if they are stored in different directories? or would they all have to be combined within one folder? What i'm hoping to achieve is to store the majority of my content on one server, while dedicating another to strictly UHD/4k content.

Something like this;

server 1
server 2
Thanks!

@SenorSmartyPants
Copy link
Contributor Author

Unfortunately no. The episodes would have to be in the same folder to be merged.

@JaidenW
Copy link

JaidenW commented Jul 5, 2022

Unfortunately no. The episodes would have to be in the same folder to be merged.

Alas, perhaps some day. Keep up the good work though!

@SenorSmartyPants
Copy link
Contributor Author

Lots of code would have to be changed to get this to work.
Check out mergerfs or another union file system. Maybe you can make it look like they are in the same directory to jellyfin.

@ireun
Copy link

ireun commented Aug 17, 2022

Also, this would probably be helpful in implementing this: https://features.jellyfin.org/posts/284/convert-option-similar-to-plexs-optimise

@JeWe37
Copy link
Contributor

JeWe37 commented Sep 20, 2022

What is the state of this now(I have a structure like @JaidenW , but I can work around that with a simple overlayfs mount, I don't need it to be writable)? It seems there is a lot of debate about how to best do the parsing and this PR currently seems to have gone dead? Anything preventing me from just rebasing onto master and building from source with this PR applied?

@SenorSmartyPants
Copy link
Contributor Author

This PR is waiting on reviewers from the JF team.

Nothing to stop you merging in your local copy.

@JeWe37
Copy link
Contributor

JeWe37 commented Sep 21, 2022

So I've been trying this out and have not been able to get it to work no matter what I tried. I use Season folders and a structure like

Name (2020)/Season 1/Name (2020) - S01E01.mkv
Name (2020)/Season 1/Name (2020) - S01E01 - [VERSION].mkv
Name (2020)/Season 1/Name (2020) - S01E02.mkv

The issue is that all episodes get grouped as versions. I also tried

Name (2020)/Season 1/Name (2020) S01E01.mkv
Name (2020)/Season 1/Name (2020) S01E01 - [VERSION].mkv
Name (2020)/Season 1/Name (2020) S01E02.mkv

with the same results. I tried making sense of the code itself, but frankly I do not quite understand the its logic.

Is there anything I'm doing wrong? Or should this work?

@SenorSmartyPants
Copy link
Contributor Author

I think you need all versions to have a name. Try this instead and let me know.

Name (2020)/Season 1/Name (2020) - S01E01 - [VERSION].mkv
Name (2020)/Season 1/Name (2020) - S01E01 - [VERSION2].mkv
Name (2020)/Season 1/Name (2020) - S01E02.mkv

@SenorSmartyPants
Copy link
Contributor Author

Did you merge all the commits in the PR not just the first one? There was an issue with all episodes merging before.

If my tip didn't help, can you include a screen shot of the episode all the videos are getting merged into with the version selection drop down open?

@JeWe37
Copy link
Contributor

JeWe37 commented Sep 21, 2022

Yes, I merged all of them, I can't really test right now, I'll get back to this though.

@JeWe37
Copy link
Contributor

JeWe37 commented Oct 2, 2022

@SenorSmartyPants I got around to testing it, even with everything named according to
Name (2020)/Season 1/Name (2020) - S01E01 - [ORIGINAL].mkv
Name (2020)/Season 1/Name (2020) - S01E01 - [VERSION].mkv
Name (2020)/Season 1/Name (2020) - S01E02 - [ORIGINAL].mkv
I still get the same effect of all episodes being shown as versions.

@SenorSmartyPants
Copy link
Contributor Author

I'll take a look at this today.

@SenorSmartyPants
Copy link
Contributor Author

@JeWe37 I was able to reproduce this with your Name (2020) example. So I will continue to dig into this.

It did work correctly when I used a different example to check if it was related to having the year in the series name.

Red/Season 1/Red - S01E01 - [ORIGINAL].mkv
Red/Season 1/Red - S01E01 - [VERSION].mkv
Red/Season 1/Red - S01E02 - [ORIGINAL].mkv

@SenorSmartyPants
Copy link
Contributor Author

SenorSmartyPants commented Oct 2, 2022

@JeWe37 Latest commit should fix your issue. Thanks for reporting it!

@JeWe37
Copy link
Contributor

JeWe37 commented Oct 2, 2022

I'll give it a shot tomorrow, thanks for the quick resolution.

@cvium
Copy link
Member

cvium commented Oct 7, 2022

I don't like that a folder is required. We need fewer filesystem rules, not more.

@SenorSmartyPants
Copy link
Contributor Author

I don't like that a folder is required. We need fewer filesystem rules, not more.

A folder is no longer required. Other than the folder requirements that already exist, that is a series folder.

@Fedec96
Copy link

Fedec96 commented Nov 15, 2022

Any update on this? This would be big.

@SenorSmartyPants
Copy link
Contributor Author

Any update on this? This would be big.

This needs to be reviewed and approved by team JF. Maybe I could get some feedback from @Bond-009 @Shadowghost @crobibero @cvium . I think they are some of the core server people.

@dotbanana
Copy link

dotbanana commented Jan 26, 2023

Thanks for this.
This is such an obvious feature i cannot understand why this wasn't implemented earlier and why this is taking so long to approve now.

@JeWe37
Copy link
Contributor

JeWe37 commented Jan 26, 2023

If it's any help: I've been running a jellyfin instance with this PR merged for a while now and it's been working fine. I have a fairly consistently named library, which is required to really get this feature to work, but I have not seen it interfering anywhere I did not want it to.

@SenorSmartyPants
Copy link
Contributor Author

If it's any help: I've been running a jellyfin instance with this PR merged for a while now and it's been working fine. I have a fairly consistently named library, which is required to really get this feature to work, but I have not seen it interfering anywhere I did not want it to.

Awesome. Glad someone is getting some use out of it other than myself.

@mammo0
Copy link
Contributor

mammo0 commented Nov 2, 2024

Hello,

I updated my fork to the latest 10.10.0 release: master...mammo0:jellyfin:10.10.z-mammo0
There were no merge conflicts since the last 10.9.11 release. Only a small fix for a test case was needed: f4f0051

All test cases are passing :)

I also updated my docker images on: https://hub.docker.com/r/mammo0/jellyfin_multi-episode-versions

@craftablescience
Copy link

This feature would be huge, I can't believe I have to switch to a fork to use it lmao. Thank you so much for making this PR

It's been two and a half years 😭

@qwertyuioasdfghjjkllmnbcvz

Changes Enable multiple versions for episodes. Episode suffixes work or episode may be placed in it's own directory. Will not collapse episodes in series directory. Unit tests included.

/TV/Dexter/Dexter - S01E01/Dexter - S01E01 - One.mkv
/TV/Dexter/Dexter - S01E01/Dexter - S01E01 - Two.mkv
/TV/Dexter/Dexter - S01E02 - Another.mkv
/TV/Dexter/Dexter - S01E03 - Again.mkv
/TV/Dexter/Season 2/Dexter - S02E01/Dexter - S02E01 - First.mkv
/TV/Dexter/Season 2/Dexter - S02E01/Dexter - S02E01 - Second.mkv
/TV/Dexter/Season 2/Dexter - S02E02 - More.mkv

JF will show 3 episodes in season 1. 2 episodes in season 2.

I would include cross over episodes like murder she wrote the Andy Griffith show gomer Pyle etc that way it pulls from one file

Copy link
Member

@JPVenson JPVenson left a comment

Choose a reason for hiding this comment

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

minor coding style detail, otherwise LGTM

Emby.Naming/Video/VideoListResolver.cs Outdated Show resolved Hide resolved
@mammo0
Copy link
Contributor

mammo0 commented Nov 19, 2024

@JPVenson: What about the changes from @JeWe37?
With them the grouping of movies and TV shows can be enabled/disabled via the web interface.

@JeWe37
Copy link
Contributor

JeWe37 commented Nov 19, 2024

So far @SenorSmartyPants hasn't implemented them, though it might be best to avoid a new PR so that the approvals aren't needed again. Can one of the maintainers perhaps push those directly? Or the rebased version from @mammo0's fork anyway.

@JPVenson JPVenson added the requires-web A server PR that requires a corresponding Web PR to be functional label Nov 19, 2024
@SenorSmartyPants
Copy link
Contributor Author

I'm hesitant to put effort into this without some assurances it will actually get merged. It's been a long time and my current energy level for projects is pretty low. Or if a JF team member wants to make alterations to get this merged you have my permission.

@mammo0
Copy link
Contributor

mammo0 commented Nov 20, 2024

@SenorSmartyPants If you want I can continue this PR. Since we have already an approval from @Shadowghost and now a 'LGTM' (after a small change) from @JPVenson, I hope this will get merged soon.

@SenorSmartyPants Can you give me write access to your fork (especially the EpisodeMultipleVersions branch)? Then I should be able to push changes directly to this PR.
Otherwise I can open a new PR and link it to this one.

What option is preferred?

@JeWe37: I can also open a PR on the jellyfin-web repo with your changes. Or you do it on your own.

@JeWe37
Copy link
Contributor

JeWe37 commented Nov 20, 2024

Feel free to open the jellyfin-web PR as you have a rebased version.

@SenorSmartyPants
Copy link
Contributor Author

Can you give me write access to your fork

@mammo0 I don't see any easy way to give you access to just one branch on my repo. So instead, could you make a PR to https://github.com/SenorSmartyPants/jellyfin/tree/EpisodeMultipleVersions which I can then merge and that will update this PR?

@mammo0
Copy link
Contributor

mammo0 commented Nov 24, 2024

@mammo0 I don't see any easy way to give you access to just one branch on my repo. So instead, could you make a PR to https://github.com/SenorSmartyPants/jellyfin/tree/EpisodeMultipleVersions which I can then merge and that will update this PR?

Please have a look at SenorSmartyPants#121 :)

Test for episodes in series folder


Turn on multiple versions for episodes


Support for multiversion episodes in mixed folders

Update 2 failing test cases. These were for passing for unofficially supported filenames. Dashes with no spaces, which is not how JF docs say multiversion files are supposed to be named.
Fix possible null


fix null take 2


Don't ParseName when calling ResolveVideos<Episode> 

ParseName drops everything after year in a filename. This results in episode information being dropped if year is present.

Update tests to set ParseName=false

Additional test with Mixed folder with Year in filename

Added case when calculating displayname for versions for mixed folders.
Add StringComparison.Ordinal to LastIndexOf

Was generating an error in recent build attempts.
Clean the episode filename to set the grouper value

This allows files like 
Name (2020) - S01E01 [BluRay-480p x264][AC3 2.0] - [ORIGINAL].mkv
Name (2020) - S01E01 [BluRay-1080p x264][AC3 5.1]- [Remaster].mkv

to be grouped on 'Name (2020) - S01E01'
Fix false positive merging

Only do cleanstring or " -" index cleaning, not both.
Compatiblity fix when stacking episodes and multiple versions are present


Fix linting problems
@jellyfin-bot jellyfin-bot removed the merge conflict Merge conflicts should be resolved before a merge label Dec 1, 2024
@jellyfin-bot
Copy link
Contributor

ABI Difference

Emby.Naming.dll
API compatibility errors between './abi-base/Emby.Naming.dll' (left) and './abi-head/Emby.Naming.dll' (right):
CP0002: Member 'System.Collections.Generic.IReadOnlyList<Emby.Naming.Video.VideoInfo> Emby.Naming.Video.VideoListResolver.Resolve(System.Collections.Generic.IReadOnlyList<Emby.Naming.Video.VideoFileInfo>, Emby.Naming.Common.NamingOptions, bool, bool)' exists on ./abi-base/Emby.Naming.dll but not on ./abi-head/Emby.Naming.dll
API breaking changes found. If those are intentional, the APICompat suppression file can be updated by specifying the '--generate-suppression-file' parameter.

@SenorSmartyPants
Copy link
Contributor Author

I fixed merge conflicts and tests. I did not include the library option to enable/disable grouping. Let that be a separate PR after this is merged. So this PR does not require any web changes.

@gnattu
Copy link
Member

gnattu commented Dec 1, 2024

I think it’s best to hold off on adding these functions right now. We’re currently in the middle of a major database refactoring, and we need to make sure we have a solid data structure for multi-version videos before we proceed. We don’t want to end up with a bunch of bugs, especially with playlists and collections like the already merged muiltversion movie, which has been a pain to maintain, and I don’t want to see episodes get caught up in the same mess. Let’s wait until things are a bit more stable before we dive into this.

@fishcharlie
Copy link

@gnattu Totally reasonable. Do you think after that database migration this can be looked at again? This would be a super useful feature to have in Jellyfin. I really hope it doesn't get abandoned forever.

@JeWe37
Copy link
Contributor

JeWe37 commented Dec 1, 2024

There are no database changes with this since it only makes it so that episode merging happens automatically rather than manually. While this could exacerbate any possible issues by making merged series more common, even in the worst case of an imperfect database migration in the future, a rescan should deal with any merging-related issues with little hassle.

Frankly it is somewhat frustrating to hear that this still cannot move forward. For my part I have been running a fork of jellyfin because of this(and similar issues) for multiple years now and I seem to not be the only one frustrated with the current situation:

I'm hesitant to put effort into this without some assurances it will actually get merged. It's been a long time and my current energy level for projects is pretty low. Or if a JF team member wants to make alterations to get this merged you have my permission.

This is particularly true with this now being one of the more requested, yet still not available feature requests.

@gnattu
Copy link
Member

gnattu commented Dec 1, 2024

There are no database changes with this since it only makes it so that episode merging happens automatically rather than manually. While this could exacerbate any possible issues by making merged series more common, even in the worst case of an imperfect database migration in the future, a rescan should deal with any merging-related issues with little hassle.

If writing migrations and doing data schema refactors sounds easy to you can you help us with the current refactor so that we can move to the right direction faster? #12798

Frankly it is somewhat frustrating to hear that this still cannot move forward. For my part I have been running a fork of jellyfin because of this(and similar issues) for multiple years now and I seem to not be the only one frustrated with the current situation:

I am also frustrated by current multi version implementation as it is causing bugs due to the design flaw. It failed to predict a lot of common use cases and it is hard to fix because the already written data is hard to migrate even if we come up with a better design. I want to have this anticipated feature to land just like everyone in the feature request ticket because I want to implement features based on this. I just want to make sure the design flaws are addressed before we can adding more features like this.

@JeWe37
Copy link
Contributor

JeWe37 commented Dec 1, 2024

If writing migrations and doing data schema refactors sounds easy to you can you help us with the current refactor so that we can move to the right direction faster? #12798

My point is solely about the fact that merging this will not make the database issues worse. I obviously cannot claim that I were intimately familiar with the issues around multiple versions, though for the past years it has been working essentially fine for me.

Now, of course this doesn't mean those issues don't exist, but it does mean this PR can be used productively and, since it does not introduce anything new to the database format, inherently cannot change or complicate anything about the migrations that already have to be written.

@JPVenson
Copy link
Member

JPVenson commented Dec 1, 2024

whats not that widely known is that the current refactor brought up so many many issues with the codebase that streach into all areas of the code. The database is the core of jellyfin and unfortunatly a lot of bad design choices were made based on the horrific database design.

Now we can't just replace the database with better code, that would be far to easy and would have been done many years ago. To make the database somewhat usable, the code needs to still support attrocious design choices, like querying the database 1+n² times just for the next-up query. Sure we can rewrite that but i hope you see where i am going with that. I was so close to rewriting even bigger chunks just to get a somewhat sensible UI structure and the Pr is already +20.000 lines of code.

Overall we are currently focusing hard on the EfCore rewrite, because it has to be done some time and that time is now, which on the other hand means that we cannot really bring much else in right now because we just dont have the capacity for it. Please dont underestimate the reach the EfCore PR has, it touches like 60-70% of the codebase and adding even more new stuff to that will make it borderline impossible to debug in the end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Related to the server backend discussion needed Requires input from from the community feature Adding a new feature, or substantial improvements on existing functionality requires-web A server PR that requires a corresponding Web PR to be functional
Projects
None yet
Development

Successfully merging this pull request may close these issues.