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

Enhance Trickplay #11883

Merged
merged 10 commits into from
Sep 7, 2024
Merged

Conversation

Shadowghost
Copy link
Contributor

@Shadowghost Shadowghost commented May 30, 2024

Companion to jellyfin/jellyfin-web#5635

Changes

  • Allow trickplay images to be saved in the media folder
  • Add library option for the above
  • Add dedicated option on refresh to replace trickplay images instead of relying on replace all images
  • Add task to move existing trickplay images after library option was changed
  • Migrate to better folder schema ({Width} - {TileWidth}x{TileHeight} to be portable

ToDo / Questions:

  • Should the trickplay controller try the internal/media dir if the other returns no trickplay?

Issues
Fixes #11747

Copy link

Changes in OpenAPI specification found. Expand to see details.

What's Changed


GET /Videos/{itemId}/Trickplay/{width}/{index}.jpg
GET /Library/VirtualFolders
Return Type:

Changed response : 200 OK

Virtual folders retrieved.

  • Changed content type : application/json

    Changed items (object):
    > Used to hold information about a user's list of configured virtual folders.

    • Changed property LibraryOptions (object)

      Updated LibraryOptions :

      • Added property SaveTrickplayWithMedia (boolean)
  • Changed content type : application/json; profile="CamelCase"

    Changed items (object):
    > Used to hold information about a user's list of configured virtual folders.

    • Changed property LibraryOptions (object)

      Updated LibraryOptions :

      • Added property SaveTrickplayWithMedia (boolean)
  • Changed content type : application/json; profile="PascalCase"

    Changed items (object):
    > Used to hold information about a user's list of configured virtual folders.

    • Changed property LibraryOptions (object)

      Updated LibraryOptions :

      • Added property SaveTrickplayWithMedia (boolean)
POST /Library/VirtualFolders
Request:

Changed content type : application/json

Updated AddVirtualFolderDto :

  • Changed property LibraryOptions (object)

    Gets or sets library options.

    Updated LibraryOptions :

    • Added property SaveTrickplayWithMedia (boolean)

Changed content type : text/json

Updated AddVirtualFolderDto :

  • Changed property LibraryOptions (object)

    Gets or sets library options.

    Updated LibraryOptions :

    • Added property SaveTrickplayWithMedia (boolean)

Changed content type : application/*+json

Updated AddVirtualFolderDto :

  • Changed property LibraryOptions (object)

    Gets or sets library options.

    Updated LibraryOptions :

    • Added property SaveTrickplayWithMedia (boolean)
POST /Library/VirtualFolders/LibraryOptions
Request:

Changed content type : application/json

Updated UpdateLibraryOptionsDto :

  • Changed property LibraryOptions (object)

    Gets or sets library options.

    Updated LibraryOptions :

    • Added property SaveTrickplayWithMedia (boolean)

Changed content type : text/json

Updated UpdateLibraryOptionsDto :

  • Changed property LibraryOptions (object)

    Gets or sets library options.

    Updated LibraryOptions :

    • Added property SaveTrickplayWithMedia (boolean)

Changed content type : application/*+json

Updated UpdateLibraryOptionsDto :

  • Changed property LibraryOptions (object)

    Gets or sets library options.

    Updated LibraryOptions :

    • Added property SaveTrickplayWithMedia (boolean)

@jellyfin-bot
Copy link
Contributor

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 Merge conflicts should be resolved before a merge label Jun 1, 2024
@Shadowghost Shadowghost force-pushed the trickplay-enhancement branch from c566106 to 54e7c64 Compare June 7, 2024 08:19
@jellyfin-bot jellyfin-bot removed the merge conflict Merge conflicts should be resolved before a merge label Jun 7, 2024
@Simsala91
Copy link

Simsala91 commented Jun 9, 2024

Allow trickplay images to be saved in the media folder

How is this implemented? Are the trickplay images stored along side each episode/movie like this:

\\media\movies\movie1\movie1.mkv
\\media\movies\movie1\trickplay\0.jpg

or can you specify a folder that is located on the media drive where every trickplay image will be saved?

@gnattu
Copy link
Member

gnattu commented Jun 9, 2024

´´´ Allow trickplay images to be saved in the media folder ´´´

How is this implemented? Are the trickplay images stored along side each episode/movie like this:

Current implementation uses a suffixed folder with the same name as the original video file.

For you example:

\media\movies\movie1\movie1.mkv

will have its own trickplay folder:

\media\movies\movie1\movie1.trickplay

And inside that folder there will be all the trickplay images for that video:

\media\movies\movie1\movie1.trickplay\{resolution}-{tilesize}\0.jpg

@kevoc
Copy link
Contributor

kevoc commented Jun 13, 2024

I have two questions on this implementation. For some background, my Jellyfin instance runs in an LXD container with read-only access to the media folders.

If I leave the setting for trickplay files untouched, i.e. all files are created in /var/lib/jellyfin/metadata/library as normal, but I write my own script to move the Jellyfin-created trickplay files to the media folder (conforming to the naming scheme from @gnattu above), and I delete the originals in the container, my questions are:

  1. Will Jellyfin use the media folder trickplay files, although it's set to use /var/lib/jellyfin/metadata/library?
  2. If Q1 answer is yes, will Jellyfin know not to re-create the trickplay files in /var/lib/jellyfin/metadata/library because they already exist in the media folder?

I'll definitely be using this feature, but the answers to these questions will advise how I implement the solution.
Many thanks for your help.

@Shadowghost
Copy link
Contributor Author

That's up for question right now. I'd say we should check both locations and only respect the setting when it comes to generating and saving/replacing trickplay images.

@thermionic
Copy link

I will be very happy to see this completed.

@michaelcurry
Copy link

michaelcurry commented Jul 1, 2024

Was just searching for this functionality within Jellyfin 10.9.6, and came across this PR. Super cool!
Thanks for making this happen @Shadowghost 🎉 I hope it is merged soon.

@SabinVI
Copy link

SabinVI commented Jul 9, 2024

When can we expect a merge on this? I'm also very excited to use this feature.

@Shadowghost Shadowghost force-pushed the trickplay-enhancement branch from 54e7c64 to ad26b22 Compare July 9, 2024 09:05
@sjorge
Copy link

sjorge commented Jul 9, 2024

When can we expect a merge on this? I'm also very excited to use this feature.

It doesn't have the backport label, so I don't think it will end up in the next 10.9.x release.

@Shadowghost
Copy link
Contributor Author

This is a quite invasive API change, so won't be in 10.9.x (also the reason this is targeting master).
It should be backportable to 10.9.z though if you want to build you own.

@RandomNamer
Copy link

RandomNamer commented Jul 11, 2024

Thank you for implementing this feature🎉 As someone who has multiple servers that access the same media library storage, I've been waited a long time for this. One little question here: It seems currently Jellyfin determines tile size and interval from the server options, does that mean users have to keep trickplay configs of multiple servers the same when using the same trickplay copy? Or can we just save localTrickPlayInfo to local folder to help other servers read these trickplay images properly?

@Shadowghost
Copy link
Contributor Author

I was thinking about this too and for at least the first implementation the prerequisite will be having the same config. This is because the config is global and if we have multiple different resolutions inconsistently over different movies/series it is impossible to auto-import them.

@thermionic
Copy link

I was thinking about this too and for at least the first implementation the prerequisite will be having the same config. This is because the config is global and if we have multiple different resolutions inconsistently over different movies/series it is impossible to auto-import them.

If stored with media, could the size and "interval" be part of the path or name of the trickplay files, so that if a different size is "expected" from the config if would create (without overwriting) if not pre-existing?

From my perspective, the advantage of having them stored with media far outweighs what I would consider to be a minor inconvenience of having to have the trickplay config the same on all hosts that access the media.

@Shadowghost
Copy link
Contributor Author

We do not remove existing trickplay files, if the resolution does not exists, the files should be created on next scan/run of the scheduled task.

@Shadowghost Shadowghost force-pushed the trickplay-enhancement branch from ad26b22 to a7ec808 Compare August 3, 2024 10:30
@soultaco83
Copy link

soultaco83 commented Aug 20, 2024

I built this PR and the web PR
but I can not get the server to load. I get the error:


[16:43:34] [FTL] [1] Main: Error while starting server
System.InvalidOperationException: Unable to resolve service for type 'Jellyfin.Server.Implementations.Trickplay.TrickplayManager' while attempting to activate 'Jellyfin.Server.Migrations.Routines.MoveTrickplayFiles'.
   at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.ConstructorMatcher.CreateInstance(IServiceProvider provider)
   at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.CreateInstance(IServiceProvider provider, Type instanceType, Object[] parameters)
   at Jellyfin.Server.Migrations.MigrationRunner.<>c__DisplayClass2_0.<Run>b__0(Type m) in /mnt/e/Jellyfin/jellyfin-server/Jellyfin.Server/Migrations/MigrationRunner.cs:line 61
   at System.Linq.Enumerable.SelectArrayIterator`2.MoveNext()
   at System.Linq.Enumerable.OfTypeIterator[TResult](IEnumerable source)+MoveNext()
   at System.Collections.Generic.LargeArrayBuilder`1.AddRange(IEnumerable`1 items)
   at System.Collections.Generic.EnumerableHelpers.ToArray[T](IEnumerable`1 source)
   at Jellyfin.Server.Migrations.MigrationRunner.Run(CoreAppHost host, ILoggerFactory loggerFactory) in /mnt/e/Jellyfin/jellyfin-server/Jellyfin.Server/Migrations/MigrationRunner.cs:line 60
   at Jellyfin.Server.Program.StartServer(IServerApplicationPaths appPaths, StartupOptions options, IConfiguration startupConfig) in /mnt/e/Jellyfin/jellyfin-server/Jellyfin.Server/Program.cs:line 160
[16:43:34] [INF] [1] Main: Running query planner optimizations in the database... This might take a while```

@jellyfin-bot jellyfin-bot added the merge conflict Merge conflicts should be resolved before a merge label Aug 23, 2024
@jellyfin-bot
Copy link
Contributor

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

@Shadowghost Shadowghost force-pushed the trickplay-enhancement branch from a7ec808 to ac874fd Compare August 30, 2024 07:17
@Shadowghost
Copy link
Contributor Author

@soultaco83 should be fixed now.

Copy link
Member

@gnattu gnattu left a comment

Choose a reason for hiding this comment

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

We might also want to have a scheduled task to handle #11476 which will be more common after this change

@Bond-009 Bond-009 self-requested a review September 6, 2024 14:10
@sjorge
Copy link

sjorge commented Sep 6, 2024

We might also want to have a scheduled task to handle #11476 which will be more common after this change

Maybe something for one big cleanup task #12427

For clarification, I meant there should probably be a generic cleanup task where this is done. As it could be used to clean up other things too.

@Shadowghost Shadowghost requested a review from Bond-009 September 6, 2024 16:14
@crobibero crobibero merged commit c56dbc1 into jellyfin:master Sep 7, 2024
20 of 21 checks passed
@SabinVI
Copy link

SabinVI commented Sep 7, 2024

So this feature is complete now? It’s ready and will be available to use in Jellyfin 10.10? It won’t be available in any 10.9.x releases?

Does anyone have a timeline for when we can expect 10.10?

@crobibero
Copy link
Member

No, this PR will not be available in 10.9.x.
No, there is no timeline in which you can expect 10.10.

@SabinVI
Copy link

SabinVI commented Sep 7, 2024

No, this PR will not be available in 10.9.x. No, there is no timeline in which you can expect 10.10.

Damn. I'm really excited to use this feature on my server. I just can't use it until I have this update because of my limited storage on the drive Jellyfin is installed on.

Thanks for the update though! I guess its another thing I'll look forward to when 10.10 does get released one day. :)

@soultaco83
Copy link

Just got done building this and the Webui(left a note there about a change). This is a perfect addon. Thank you.

@eomanis
Copy link

eomanis commented Sep 26, 2024

The way I understand this enhancement is that, once finished, it will be possible to store the Trickplay images in

  • The "metadata" path, subdirectory "library" (as is currently the case)
  • The media libraries' file trees themselves

I neither want the "metadata" directory to blow up in size on my SSD, nor do I want Jellyfin cluttering up the media directories with Trickplay images. It does not have write permissions there anyway, principle of least privilege and all.

Having Trickplay files sprayed into the media directories would at the very least give me the ick, not to mention that it would also complicate directory listings for my FTP users.

What I would like is to have a designated Trickplay images directory at a custom path "somewhere off to the side" on the large media file system. Would that be possible?

If there is no dedicated option for a custom Trickplay images path, do you think a symbolic link from within the "metadata" directory would work?

At any rate, I do not care for an admin user web interface input for such a custom path; if I could set it with an environment variable that would be sufficient. I mean, how often am I going to change it.

@Shadowghost
Copy link
Contributor Author

Symlinks should always work. No reason why they aren't as long as your permissions are set correctly.

@eomanis
Copy link

eomanis commented Sep 26, 2024

Symlinks should always work. No reason why they aren't as long as your permissions are set correctly.

Within "metadata/library" there is what looks like a file-hash-based directory tree though. Assuming Jellyfin puts the Trickplay images into that tree and not into a dedicated subdirectory, symlinking the Trickplay images away will not be possible.

@brkr1
Copy link

brkr1 commented Oct 28, 2024

So, is it working on 10.10.0? I'm only waiting for it to update from 10.8.13.

@coreykovacs1
Copy link

This release is great. Definitely prefer the trickplay files stored within the media folders. One issue I am having though is that the trickplay files in the media folders aren't portable between servers. The new server on the trickplay scan is capable of detecting the trickplay files but doesn't get the metadata stored in the trickplayinfos table correct. In my experience it doesn't change the Height value by the number of rows of tiles in the output JPG files.

Would it be possible to have the correct metadata added to the trickplayinfos folder easily?

@flashlab
Copy link

This release is great. Definitely prefer the trickplay files stored within the media folders. One issue I am having though is that the trickplay files in the media folders aren't portable between servers. The new server on the trickplay scan is capable of detecting the trickplay files but doesn't get the metadata stored in the trickplayinfos table correct. In my experience it doesn't change the Height value by the number of rows of tiles in the output JPG files.

Would it be possible to have the correct metadata added to the trickplayinfos folder easily?

As my opinion, you should create an issue which point here instead of comment under a merged commit, to make sure more people can see it.

@Bryan792
Copy link

I do have #12887 up

@thegranddesign
Copy link

thegranddesign commented Dec 12, 2024

@Shadowghost thank you SO MUCH for doing this!! Especially the ability to move trickplay images next to media!

I had one quick question. How difficult would it be to have trickplay images be segregated under a trickplay folder?

So the structure would be more like:

<library_directory>/<movie_or_show_directory>/trickplay/<media_item_base_filename>.trickplay/<trickplay_resolution> <trickplay_tile_height>x<trickplay_tile_width>

Why would I want this?

For something like a movie, where there is only one media file in a directory, it doesn't really matter, but some "shows" can contain dozens of "episodes". Having all of those folders in the same directory kind of clutters everything up. At the end of the day, the thing that I care about is that the trickplay images are in the same directory as the media file, but I don't really need to see them 99% of the time.

@Shadowghost
Copy link
Contributor Author

You'd need to modify the code in regards of the devised path for trickplay images. Currently you can't set something custom there

@Simsala91
Copy link

I would also prefer to be able to save trickplay images at an arbitrary location.
Not really happy with saving them alongside media, but I also don't want to save them in the default location because they take up quite a bit of space on the SSD.
Since I want posters to be saved on SSD, it's also kinda difficult to just map the trickplay location to another drive, since they are stored within the metadata directory alongside posters.

@thegranddesign
Copy link

Would anyone be opposed to making this the default?

@Shadowghost
Copy link
Contributor Author

Yes. A lot of people mount their media read only for consumption, so saving anything next to it is impossible.

@kevoc
Copy link
Contributor

kevoc commented Dec 12, 2024

@Simsala91 if your Jellyfin instance is running on Linux, you can already achieve what you're looking for using overlayfs. Your media folder with the movie files will be the lower dir, and you can have a different folder for your trickplay files as upper dir. Any writes or changes made by Jellyfin only change the upper dir.

Disclaimer: I haven't done this yet, but intend to. It works in theory. As Shadowghost pointed out, I'm one of those people whose media collection is mounted read-only, so I'll be using overlayfs to put a writable layer on top.

@eomanis
Copy link

eomanis commented Dec 12, 2024

@Simsala91 if your Jellyfin instance is running on Linux, you can already achieve what you're looking for using overlayfs. (...)

Requiring overlayfs for a Jellyfin feature that could conceivably be implemented within Jellyfin seems excessive though.

@kevoc
Copy link
Contributor

kevoc commented Dec 13, 2024

@eomanis I agree, in the universe where Jellyfin has infinite funding and infinite developers.

On Planet Earth, highly custom requests should be approached with pragmatism.

@eomanis
Copy link

eomanis commented Dec 13, 2024

@eomanis I agree, in the universe where Jellyfin has infinite funding and infinite developers.

On Planet Earth, highly custom requests should be approached with pragmatism.

Also true.

Please do not read the following as greedy; who writes the code gets to decide.

I am mentally hung up about the fact that someone would prefer a solution that

  • Requires entirely new logic for calculating file paths, which better not have any glitches because it dangerously operates among the precious media files The code for that already exists and is in use for other things
  • Pollutes the media directories with ephemeral files (yuck)
  • Requires security concessions (read-write media) or complex and non-trivial operating system specific workarounds (overlayfs) to function if security of the media files should not be compromised

to a solution that just re-uses the Library directory's logic of path calculation below a user-customizable root directory. I. e. an "extra Library directory for Trickplay files" kind of thing.

Of those two ways to solve the request "have the Trickplay images in a custom location" I subjectively view the first one as "highly custom" and the latter as "pragmatic".

@gnattu
Copy link
Member

gnattu commented Dec 13, 2024

What you proposed is a new logic, what we currently have is not. We are doing the exactly same with subtitles, posters and lyrics. All of those are saved alongside the media files and "pollutes" the directory because the user asked us to do so. Trickplay just does what all other metadata handlers do and most users are satisfied with such implementation. Please don't get mentally hung up with the fact that not everyone thinks the same way as you.

@thegranddesign
Copy link

Yes. A lot of people mount their media read only for consumption, so saving anything next to it is impossible.

That's not what I meant. What I meant was that for people who choose to place trickplay images next to their media, could we not make it the default so that all trickplay images are within a single trickplay subdirectory?

And yes, I agree with others that if the trickplay images were a single file, placing it next to the media wouldn't be that bad, but that's not what's happening. Having them be within a directory means that, for almost everyone using it, your ls and desktop file browser is going to sort all of those folders at the top, followed by all of the files. That's what makes it cluttered.

Doing this for trickplay images is similar to Jellyfin looking for trailers in trailers directory as well as next to the media.

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

Successfully merging this pull request may close these issues.

[Issue]: trickplay files are stored in /config/metadata/library and not with media