beetbox / audioread

cross-library (GStreamer + Core Audio + MAD + FFmpeg) audio decoding for Python
MIT License
483 stars 108 forks source link

Only run backend autodetection once per process #83

Closed ssssam closed 5 years ago

ssssam commented 5 years ago

This is a performance improvement. See https://github.com/beetbox/audioread/issues/81 for details.

ssssam commented 5 years ago

Thanks for the review. Yes, it's a good idea to avoid global state. We might need to rationalize the situation in pyacoustid and the beets/chroma plugin though:

sampsyo commented 5 years ago

Currently, beets/chroma doesn't depend on audioread. Can I change it so that it does depend on a future release of audioread that contains this branch?

Yes, sounds good!

  • pyacoustid currently only soft-depends on audioread. The fingerprint_file() function has a conditional which uses audioread and the 'chromaprint' library if both are available, and otherwise the fpcalc program.

Oh right; I completely forgot about that feature. This makes the situation a little more complicated, for both pyacoustid and for beets. It is actually really quite convenient for pyacoustid to support the command-line tool, which sometimes easier to install (e.g., on Windows) than the library. On the other hand, talking directly to the library is the "right way" to do things on systems with a proper package manager.

Here's a strategy that might be nice to have, even if we weren't doing this whole "persistent backend detection" thing: empower pyacoustid with flags that control which backend is used. The appropriate function could behave as it does now when nothing specific is supplied (preserving backwards compatibility), or it could:

Having that additional level of control would be nice, and it wouldn't impact the configuration-free use case either.

ssssam commented 5 years ago

Since #84 I've weirdly not seen any performance issues related to backend autodetection. I'm not sure why. However, I think it's still a good idea to add a separate available_backends() method, so I've updated this branch and slightly cleaned it up.

sampsyo commented 5 years ago

Looks great; thank you! This will be useful to have in case we find performance problems due to backend detection in the future.

On that note, thanks for all your attention to this project over the last few weeks! I've sent you an org invitation in case you'd like to commit more work in the future. PRs are of course always still an option if you want a code review, but you should now have permission to merge your own if that ever seems appropriate.

ssssam commented 5 years ago

OK, thanks! I promise not to sneak in any bitcoin mining code ;)