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

Expanded file date options and added Cover art scanning #181

Closed
wants to merge 13 commits into from

Conversation

b-levin
Copy link
Contributor

@b-levin b-levin commented Dec 29, 2021

For Issue #135

Expanded the date parameters stored in the Album struct to include CreatedAt (in addition to ModifiedAt). Additionally, users can now use the create-sort option in the file config or command line flag to change which date (CreatedAt or ModifiedAt) is used for the sorting in the ServeHome function.

The ability to just scan for album cover art changes was added to mirror the feature from airsonic. Users can manually trigger the cover art scan from the homepage by clicking on the new cover scan button under recent folders on the admin/home page. Additionally, a ticker was added for the new cover art scan, which is disabled by default and can be modified via either the cover-scan-interval in the config file or command line flag.

@sentriz
Copy link
Owner

sentriz commented Dec 29, 2021

Hi and thanks. I'm not sure what the benefit of scanning only covers is. Is it a performance thing? It seems to me like a lot of added complexity

@b-levin
Copy link
Contributor Author

b-levin commented Dec 29, 2021

The idea of the cover scan was indeed performance related and to address the first point @bobberb made in their issue (#135). While it does add complexity to the code base, it is significantly faster than doing a full scan (seconds as opposed to minutes). If the added complexity is too much, I can redo the PR to just include the additional date options.

@sentriz
Copy link
Owner

sentriz commented Dec 29, 2021

Thanks for the context on cover scanning. Personally on my system a quick scan takes about 15s on 50k tracks and will pick up any new or changed covers. May I ask with your system, how many tracks? And where is the db file stored? Spinny disk or SSD? Also, is the music mounted locally, or a remote mount? Cheers

@bobberb
Copy link

bobberb commented Dec 29, 2021

@sentriz

I have pulled and built this PR multiple times over the last 4 hours.

Since I opened issue #135, unfortunately, airsonic already failed and was forked by another team. It still remains a very well-running docker image, albeit one that soaks up 2GB RAM at idle while your nice goland gonic does not.

I have been unable to fully commit to Gonic because moving and touching those files in the folder of an album could have reset its position in Recently Added.

I speak as a migrant from Airsonic, I much prefer having a stable "Recently Added" list, especially one that can tolerate album art changes. Current PR allows me to drop in album art without changing album order in that list. I've been running both your code and Airsonic side by side for a good while now. I much prefer gonic for speed and memory cost however my mind is still oriented to "Recently Added = Recently Downloaded order".

I agree with @b-levin that it should be based on a number given by the filesystem - not gonic.

@b-levin thank you!

@bobberb
Copy link

bobberb commented Dec 29, 2021

I should note that at the current time, this branch does not currently respect albums which are present prior to gonic. 1st import will be random order seemingly. Every album dropped in thereafter is accepted in a list which is immune from movement by album art updates.

@bobberb
Copy link

bobberb commented Dec 29, 2021

Thanks for the context on cover scanning. Personally on my system a quick scan takes about 15s on 50k tracks and will pick up any new or changed covers. May I ask with your system, how many tracks? And where is the db file stored? Spinny disk or SSD? Also, is the music mounted locally, or a remote mount? Cheers

I got 7s for both scans - I'm myself more excited for a stable list on this branch.

@b-levin
Copy link
Contributor Author

b-levin commented Dec 31, 2021

I was predominately leaning on @bobberb for performance testing, as my library is too small to have a real difference in testing. The more stable list is the highlight of these changes. I'm only working with a few hundred tracks on locally mounted SSD storage, plus when combined with the fact I've been testing these changes on my desktop results in both cover and quick scans to take only a second or two at the most.

Given the fact that the cover scan seems to be a bit redundant, would the best approach be to remove it from this PR and keep the additional date?

@sentriz
Copy link
Owner

sentriz commented Jan 5, 2022

howdy and sorry for the delay

yeah @b-levin I think we could remove the cover scanning feature for now - then it will be easy for me to review the changes around ctime/mtime

after that, we could have a look at cover scanning again as you suggest, or else see if the scanner performance could be improved in the case of only covers changing. a bit more easy for the user

thanks!

@b-levin
Copy link
Contributor Author

b-levin commented Jan 6, 2022

@sentriz Not a problem, this can definitely be a busy time of year!

Given that the cover scan feature is getting split out, I'm going to close this PR and open and open a new one just with the time changes.

@b-levin b-levin closed this Jan 6, 2022
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