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

scrub: Restore album art in `auto` hook #1657

Closed
Kolbasz12 opened this Issue Oct 23, 2015 · 35 comments

Comments

Projects
None yet
3 participants
@Kolbasz12

Kolbasz12 commented Oct 23, 2015

recently finished importing all media into beets. Upon completion I decided to check replaygain as it was not something I did during import.

did a basic query, beet replaygain year:2015

It does as expected, going through and adding replaygain to all files, but what was not expected is the fact that it clears the embeded album art.

I can look at a folder, change view so the art appears and as replaygain goes through the files, one by one the embedded art disappears.

it is corrected by running beet embedart, but it seems like that shouldnt be needed. Would beets be doing any other processing that would call to clear art?

@sampsyo sampsyo added the needinfo label Oct 23, 2015

@sampsyo

This comment has been minimized.

Member

sampsyo commented Oct 23, 2015

As I mentioned on the mailing list, could you please do some investigation to see if anything else in your config could be causing this? For example, try turning off other plugins (e.g., zero) and checking whether the behavior still occurs.

We also need full bug report details (version, OS, etc. -- see the FAQ).

@Kolbasz12

This comment has been minimized.

Kolbasz12 commented Oct 24, 2015

I do have zero plug-in enabled, but if that is executed, wouldn't embedart
execute too like on my regular imports?

On Fri, Oct 23, 2015, 11:34 AM Adrian Sampson notifications@github.com
wrote:

As I mentioned on the mailing list, could you please do some investigation
to see if anything else in your config could be causing this? For example,
try turning off other plugins (e.g., zero) and checking whether the
behavior still occurs.

We also need full bug report details (version, OS, etc. -- see the FAQ).


Reply to this email directly or view it on GitHub
#1657 (comment).

@sampsyo

This comment has been minimized.

Member

sampsyo commented Oct 24, 2015

Yes, that would be the expected behavior—but this is a bug report, so we need to investigate the unexpected.

@Kolbasz12

This comment has been minimized.

Kolbasz12 commented Oct 27, 2015

OK, running 1.3.16 in a docker container.

The previously assumed relation to the zero plugin is inaccurate. I removed it from my configuration all together and the art is still being removed. I uploaded the data to pastebin that shows the info call to a track showing the art is TRUE. the version number, then the verbose replaygain and finally info where it shows art as FALSE, let me know if anything else is needed at this time.

http://pastebin.com/EYMcYeE6

@sampsyo

This comment has been minimized.

Member

sampsyo commented Oct 27, 2015

Great; thank you for investigating that. To complete the report, can you please check your other plugins in a similar way?

Please also post your configuration and other standard bug report details.

@Kolbasz12

This comment has been minimized.

Kolbasz12 commented Oct 29, 2015

It appears the issue is with the scrub plugin. As soon as I remove it, replaygain leave the art alone.

as soon as I put it back, the art is removed. So, whether this is by design, I am not sure.

scrub:
auto: yes

@sampsyo

This comment has been minimized.

Member

sampsyo commented Oct 29, 2015

Thanks for narrowing it down! Strangely enough, I can't reproduce this: I tried enabling scrub with the auto option, and:

$ beet info performers | grep art:
               art: True
$ beet replaygain performers
replaygain: analyzing Air - 10 000 Hz Legend - Electronic Performers
$ beet info Performers | grep art:
               art: True

And even:

$ beet scrub performers
scrub: scrubbing: [...]
scrub: restoring art
$ beet info Performers | grep art:
               art: True

That is, beet info confirms that the art is still present, even when I invoke scrub explicitly.

I'm not sure what else to try. Please let me know if you have any additional information that can help me reproduce the problem.

@Kolbasz12

This comment has been minimized.

Kolbasz12 commented Oct 30, 2015

OK, so I guess the only other thing I could offer is this, my configuration.

When I stripped it down completely, things broke and beets would not start. So I figure it easiest to post as is, minus scrub...

I mean I guess it is entirely possible that some other plugin with scrub is the cause, but that seems odd. If you are able to test with my config and it still works, shelf the issue as I can just as easily still run embed art after replay gain... obviously not ideal, but it does the trick and works without making it a pain in the ass.

http://pastebin.com/raw.php?i=jzRqHh1t

@sampsyo

This comment has been minimized.

Member

sampsyo commented Oct 30, 2015

Yes, it does seem possible that another plugin could be the cause. I don't quite have the bandwidth to try adapting your complete config—could you please try disabling other plugins and see if turning one off fixes it?

And if an empty config causes your beets to crash, that is definitely a problem we should investigate!

@Kolbasz12

This comment has been minimized.

Kolbasz12 commented Oct 30, 2015

how empty can my config be? I thought I could strip it all the way down, but this is not the case...
the following fails to load:

I took the example config, added replaygain and changed directory and library paths to match and things do not start. It is most bizarre that all filled out it works. I have no clue.

directory: /opt/downloads/music/beets
library: /config/musiclibrary.blb
import:
copy: yes
write: yes
resume: ask
quiet_fallback: skip
timid: no
log: beetslog.txt
ignore: .AppleDouble ._* *~ .DS_Store
art_filename: albumart
plugins: bpd replaygain
pluginpath: ~/beets/myplugins
threaded: yes
ui:
color: yes

paths:
default: $genre/$albumartist/$album/$track $title
singleton: Singletons/$artist - $title
comp: $genre/$album/$track $title
albumtype:soundtrack: Soundtracks/$album/$track $title

replaygain:
auto: no
overwrite: yes
backend: command

@sampsyo

This comment has been minimized.

Member

sampsyo commented Oct 30, 2015

What do you mean by "do not start"? Is there an error message?

A completely empty configuration should work.

@Kolbasz12

This comment has been minimized.

Kolbasz12 commented Oct 31, 2015

der. I found it. I am using docker and there must be a call to the web plugin somewhere, somehow.

Once I looked at the docker container log, I saw it wanted the web plugin, I added it and now it starts and I can test replaygain...sorry for the confusion, it is my own dumb fault.

@Kolbasz12

This comment has been minimized.

Kolbasz12 commented Nov 2, 2015

OK, that was fun. Empty config with replaygain and info only leaves the art alone as you saw, so now I will try to add things back one at a time to see if I cannot pinpoint the issue.

@Kolbasz12

This comment has been minimized.

Kolbasz12 commented Nov 2, 2015

OK, found the plugin that is clearing the art. It is being cleared by the scrub plugin when running replaygain. If I run a regular import the art is pulled and imported fine, but for replaygain, scrub is clearing album art.

scrub:
auto: yes

@sampsyo sampsyo added bug and removed needinfo labels Nov 2, 2015

@sampsyo sampsyo changed the title from replaygain clears embeded album art to scrub: Restore album art in `auto` hook Nov 2, 2015

@sampsyo sampsyo added this to the 1.3.16 milestone Nov 2, 2015

@sampsyo

This comment has been minimized.

Member

sampsyo commented Nov 2, 2015

OK, thanks for narrowing that down.

It looks like scrub's auto mode doesn't do anything to restore album art, unlike the explicit beet scrub command. In fact, you can even see this when doing a beet modify---the replaygain connection was a red herring.

@Kolbasz12

This comment has been minimized.

Kolbasz12 commented Nov 3, 2015

So, for my sanity, does this mean that every time a command is executed all plugins in my config still run?

In the case of all my imports, the art was still there and I only started to use replaygain after I imported everything. The point is that scrub didnt affect my normal imports even though it was present.

I am just trying to grasp how this only became an issue with replay gain and my only thought was regarding the order of and how all my plugins execute when running an import.

@sampsyo

This comment has been minimized.

Member

sampsyo commented Nov 3, 2015

So, for my sanity, does this mean that every time a command is executed all plugins in my config still run?

Not exactly. Not all plugins run all the time -- it depends on the plugin. The scrub plugin in particular runs every time tags are written. That's unlike most import-related plugins (say, embedart), which only run during import.

@Kolbasz12

This comment has been minimized.

Kolbasz12 commented Nov 3, 2015

ah, that makes sense. so in my case, when my imports ran, embedart, kept the art or added it as needed, picking up after scrub. But, as noted, replaygain is not an import, so scrub is removing the art and embed art is not there to add it back.

So, now that that makes sense, is there any way to know which plugins run during which tasks and also, is there a specific order plugins execute? meaning, would there be a case when scrub runs after embedart? Is the order predefined or is there something that can control/change execution order? such as the order in which they appear in the config?

@sampsyo

This comment has been minimized.

Member

sampsyo commented Nov 3, 2015

So, now that that makes sense, is there any way to know which plugins run during which tasks

The documentation is the canonical source for that. I note, however, that the scrub docs are wrong --- any interest in helping fix that, to make it clear that it runs on every write, including not limited to imports?

is there a specific order plugins execute? meaning, would there be a case when scrub runs after embedart? Is the order predefined or is there something that can control/change execution order? such as the order in which they appear in the config?

No, there is no specific order and it is not controllable. The idea is that it shouldn't matter: plugins should be written carefully so that they don't interfere with each other and so that order isn't important.

@Kolbasz12

This comment has been minimized.

Kolbasz12 commented Nov 3, 2015

I can modify the doc, but how should it read? I mean technically, if scrub is not observing its setting, then there is no way to make the document correct, without simply removing the configuration section.

auto: yes is supposed to set scrub to run only on import, but it is running still for all tasks and therefore, breaks when run with replay gain.

make sense? scrub is currently broken, so there is no good way to edit the doc, other than noting that it will run every time beets runs. Or am I missing something?

@sampsyo

This comment has been minimized.

Member

sampsyo commented Nov 4, 2015

Yes, sorry for not being clear. Scrub should run on every write (I think?); the bug is just that it doesn't restore the album art when it does so. So we can just change the sentence in the docs about when the hook runs.

@reiv

This comment has been minimized.

Collaborator

reiv commented Nov 4, 2015

Is there a reason scrub should run on every write, rather than just imports? If its purpose is to remove extraneous tags then it would just be wasting time (and slowing everything down, too) running on every write since Beets itself does not add any untracked tags. Or am I missing something here?

@sampsyo

This comment has been minimized.

Member

sampsyo commented Nov 4, 2015

It's definitely debatable! Here's the only difference: if someone has the importer configured not to write to files, then the scrub plugin can run when they eventually run beet write explicitly. But maybe they should just then take responsibility for running beet scrub too if they want that.

@reiv

This comment has been minimized.

Collaborator

reiv commented Nov 4, 2015

I think the speed advantages afforded by only hooking into import would outweigh the need to be a little more explicit when invoking the plugin manually, but that's just my $0.02.

@sampsyo

This comment has been minimized.

Member

sampsyo commented Nov 4, 2015

Yeah, on further reflection I agree. Let's change the plugin to only run on import, like the docs say.

@Kolbasz12

This comment has been minimized.

Kolbasz12 commented Nov 4, 2015

Since the document doesnt say exactly what it does, I need to ask. What tags are actually scrubbed by scrub? is it all of them? Or just the ones not in musicbrainz or something? also, if we are to discuss performance increases, I would be curious the time it takes to process something with scrub and without it. I cannot imagine the performance increase being all that significant. but regardless, clearly understanding what it is doing and how it is doing it and when is key for defining the plugin in the document.

@sampsyo

This comment has been minimized.

Member

sampsyo commented Nov 4, 2015

It removes all the tags not supported by beets. That is, it deletes the tags, and then re-adds only what beets has in its database.

@Kolbasz12

This comment has been minimized.

Kolbasz12 commented Nov 4, 2015

so will you just remove the configuration setting "auto" yess/no? since that was supposed to control it running only on import?

meaning you will force it within the plugin to only run on import (unless called directly), removing the need for any other explicit configuration.

so, if I have a tag, bacon. since it is not a beets tag, it would be cleared and not put back, while artist is cleared but put back

@sampsyo

This comment has been minimized.

Member

sampsyo commented Nov 4, 2015

No, we'll make the auto setting control running on import. So you can still turn it off if you only want to run explicitly.

so, if I have a tag, bacon. since it is not a beets tag, it would be cleared and not put back, while artist is cleared but put back

Right.

@Kolbasz12

This comment has been minimized.

Kolbasz12 commented Nov 4, 2015

So, auto: yes in the config says run on import only. If nothing is in the config, then it never runs unless called explicitly beet scrub?

@sampsyo

This comment has been minimized.

Member

sampsyo commented Nov 5, 2015

The setting defaults to "yes." But if you turn it off, then you're right.

@sampsyo

This comment has been minimized.

Member

sampsyo commented Dec 13, 2015

OK, I just pushed the following changes:

  • The plugin actually runs on import, instead of on every write.
  • When it does so, it uses the same logic as the explicit command to restore album art.

@Kolbasz12, can you check that the plugin now behaves how you expect it to?

@sampsyo sampsyo closed this Dec 13, 2015

@Kolbasz12

This comment has been minimized.

Kolbasz12 commented Dec 15, 2015

Yes, I will update and test. And report. It might be a few days, but
once tested, I will report. Thanks!

On Sat, Dec 12, 2015, 9:31 PM Adrian Sampson notifications@github.com
wrote:

Closed #1657 #1657.


Reply to this email directly or view it on GitHub
#1657 (comment).

shamangeorge added a commit to shamangeorge/beets that referenced this issue Dec 16, 2015

Merge branch 'master' into spectral_music
* master: (94 commits)
  Clean up changelog for edit plugin
  scrub: Demote a log message to debug
  scrub: Restore tags & art in auto mode (beetbox#1657)
  scrub: Run on import in auto mode (beetbox#1657)
  Fix beetbox#1673: Escape regex terms in lyrics
  snake_case variable names
  Doc refinements for beetbox#1749
  Remove tests for Google fetchart backend (beetbox#1760)
  fetchart: Remove Google backend (fix beetbox#1760)
  fetchart: Better logging for iTunes (beetbox#1760)
  fetchart: Fix beetbox#1610: itunes install docs
  Fix test that depended on local time, 2
  Fix test that depended on local time
  Fix unused import leftover on test_library
  Add documentation for M:SS length
  Move raw_seconds_short to beets.util
  Add tests for library-specific field types
  Fix test that was expecting raw length format
  Fix pyflakes issues, variable name
  Add config option for human vs raw track duration
  ...
@Kolbasz12

This comment has been minimized.

Kolbasz12 commented Jan 2, 2016

just ran replay gain in 1.3.17 and it did not clear the art. after successfully completing, art was still true

@sampsyo

This comment has been minimized.

Member

sampsyo commented Jan 2, 2016

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment