Closed wisp3rwind closed 5 years ago
Ok, I'll rebase this on master and have a look at the testing framework
Just force-pushed an updated patch that includes tests and hopefully is also better structured into several smaller commits. What do you think? There are some comments left in the code about issues I encountered: In particular, actually always being in sync with the source artwork is probably not worth the filesystem load it would require.
EDIT: Force-pushed again with a dest
-> path
bugfix.
I’d be ok with paying a performance penalty for this feature. We already check a lot of mtimes and we can follow-up with performance improvements later.
I'm not quite sure if I understand you correctly here: You're saying that checking a few additional mtimes is ok, or that it would be fine to also do the MediaFile
check for actual embedded art? I didn't benchmark the latter (or anything for that matter), but I would assume that it's a significant slowdown due to accessing file contents instead of filesystem metadata only.
One approach could be to drop the artwork removal codepaths and the comment about caching the result of if (album.artpath and os.path.isfile(album.artpath) [...])
for now, but open another PR/issue with them to track the issue?
Once you address the comments and fix the test this should be ok to merge.
That might warrant some thorough investigation... I've seen this test failure before, but then it disappeared. tox
succeed locally before I pushed the last changes. The only reason that I can think of right now is some non-deterministic padding being applied to the image data. A quick look at the MediaFile
and mutagen
sources didn't show any obvious way for modifications to the data to happen. Maybe there's a problem with the file not being synced to disk in time?
You're saying that checking a few additional mtimes is ok, or that it would be fine to also do the
MediaFile
check for actual embedded art?
The first one. I think your right that checking the MediaFile
would be costly. That means we would ignore the artwork remove edge case for now and track in in an issue.
I’m going to have a look at the test failures later.
So I did change everything you commented on in your review. I couldn't reproduce the test failure, though (running tox 100 times in a loop does not change that either):
beets-alternatives
does not even schedule another SYNC_ART
after changing the artwork. If it did, the ~<filename>
line would be printed a second time.Anyway, I'm going to push the new patch. If Travis continues to fail, I'll push a version that logs the mtimes and which decisions it takes in matched_item_action
.
Temporarily pushed a brunch with some more debugging instrumentation since Travis kept failing. Some local testing suggests that mtime granularity could indeed be a problem given that the tests run rather fast.
EDIT: Now the tests suceed -.- I'll modify test setup to ensure that mtimes are indeed distinct on the order of seconds to rule out this specific problem. There's no other possible reason for failure I can think of right now.
EDIT 2: While the problem might not be time granularity by the filesystem itself, this answer on stackoverflow suggest another underlying cause for identical mtimes. I'm on btrfs, maybe times are handled differently there and therefore I don't see the failures.
EDIT 3: Increasing the mtime by 2 seconds in between all the calls to beet alt update
in the test, travis now suceeds (except for flake8, gonna fix that in a minute).
Third in a series of PRs to backport changes from wisp3rwind/beets-alternatives to this repo
Previously, embedded artwork was not guaranteed to be up-to-date with the cover file tracked by beets. The patch includes a little rework of the
item_action
handling, I hope this seems a like a reasonable idea to you, too.