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

use thought when calling to vfs for esoteric media #766

Closed

Conversation

KyleSanderson
Copy link

so I barely know python, but saw that there were massive stalls for populating the database with a very tiny media library. There's still some massive slowdown on series retrieval, but the episodes load in in about a second now from minutes previously. This results in a 7x performance improvement (time reduction) for library import overall, with about a 90x reduction on per-episode import.

@sonarcloud
Copy link

sonarcloud bot commented Aug 27, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
100.0% 100.0% Duplication

@codecov
Copy link

codecov bot commented Sep 2, 2023

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 21.07%. Comparing base (68c49ea) to head (df2e0d5).
Report is 260 commits behind head on master.

Files with missing lines Patch % Lines
jellyfin_kodi/objects/movies.py 0.00% 2 Missing ⚠️
jellyfin_kodi/objects/tvshows.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #766   +/-   ##
=======================================
  Coverage   21.07%   21.07%           
=======================================
  Files          63       63           
  Lines        8542     8542           
  Branches     1572     1572           
=======================================
  Hits         1800     1800           
  Misses       6726     6726           
  Partials       16       16           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@oddstr13 oddstr13 left a comment

Choose a reason for hiding this comment

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

You are making a bunch of duplicate logic here, I think it would make sense to move this check into the validate_*_dir functions (and probably just pass the obj in there for now).

TODO

  • verify if validate_*_dir functions are also called from elsewhere

@mcarlton00
Copy link
Member

Did some benchmarking here. Synced my entire movies and shows libraries from a fresh install on my main rig.

Current master: 4 minutes, 23 seconds
This PR: 4 minutes, 25 seconds

That's close enough I'd chalk it up to network speed variations, but also seems to indicate this is insignificant as far as speeding up the sync. It's possible this is only meaningful on lower power devices. Would need more investigating to say for sure.

@KyleSanderson
Copy link
Author

Did some benchmarking here. Synced my entire movies and shows libraries from a fresh install on my main rig.

Current master: 4 minutes, 23 seconds This PR: 4 minutes, 25 seconds

That's close enough I'd chalk it up to network speed variations, but also seems to indicate this is insignificant as far as speeding up the sync. It's possible this is only meaningful on lower power devices. Would need more investigating to say for sure.

Add 50ms of latency between your Library and the client, or even 10ms.

@oddstr13 oddstr13 added the Native Mode Issue is related to Native playback mode label Jun 10, 2024
@oddstr13
Copy link
Member

This would in theory cut out two round-trips when adding items in native mode.

I'm not sure if Container is ever bluray or dvd, but I see that is checked elsewhere too.
Is that an API change, or did this never work?

Container isn't included in the toplevel item object when the media is BluRay or DVD (but an empty string in the MediaSources entries).

VideoType can have the value Dvd or BluRay however. It is VideoFile for mkv files.

@oddstr13 oddstr13 closed this Sep 10, 2024
@oddstr13 oddstr13 added the wontfix This will not be worked on label Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge conflict Native Mode Issue is related to Native playback mode wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants