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

Test stempeg 0.2.0 adapter #357

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Conversation

faroit
Copy link
Contributor

@faroit faroit commented May 3, 2020

testing stempeg adapter

The purpose of this PR is just to show how little changes are required to add the stems support using stempeg. The stempeg API is not finished yet. For further discussion about the API, please use faroit/stempeg#28

@faroit faroit mentioned this pull request May 3, 2020
@mmoussallam
Copy link
Collaborator

Hi @faroit

We've been looking at your proposal and here's our take on it:

  • It would definitely be nice to allow writings of stem files as output of spleeter separate. Using stempeg to this aim seems to be the best option. So we're definitely supporting the idea of adding stempeg as a dependency and allow to use the new StempegProcessAudioAdapter through the existing --adapter option, and eventually set it as default.

  • However, a lot of our users are accustomed to the fact that separated tracks are written as multiple files in the output and not multichannel ones. If I understand correctly, that's what is achieved with the streams_as_files flag of the write_streams method. So It should probably be set to True by default.

Also I notice that in your implementation you're not writing the outputs in parallel as we do, is there any reason for that ?

Best

@faroit
Copy link
Contributor Author

faroit commented May 6, 2020

@mmoussallam sounds great

It would definitely be nice to allow writings of stem files as output of spleeter separate. Using stempeg to this aim seems to be the best option. So we're definitely supporting the idea of adding stempeg as a dependency and allow to use the new StempegProcessAudioAdapter through the existing --adapter option, and eventually set it as default.

sounds good. As we use stempeg in many other projects it would be great if we could discuss the API for this first in faroit/stempeg#28 before we continue here.

However, a lot of our users are accustomed to the fact that separated tracks are written as multiple files in the output and not multichannel ones. If I understand correctly, that's what is achieved with the streams_as_files flag of the write_streams method. So It should probably be set to True by default.

exactly. Maybe this could be made a bit more abstract by adding stream_map classes that users could provide. Eg. stream_map = stempeg.map.multichannel for multi-channel, stream_map = stempeg.map.files(['mix', 'drum', ...]) for streams_as_files... ?

Also I notice that in your implementation you're not writing the outputs in parallel as we do, is there any reason for that ?

indeed, that would be simple to re-add for the streams_as_files but I don't think it would make sense for the stems format as this is not iterating over the stems - hence there is no speed benefit.

Did you really get much speedup for multiprocessing on write?

@faroit
Copy link
Contributor Author

faroit commented Nov 8, 2020

@mmoussallam I've updated this showcase draft PR to reflect the recent changes in the stempeg beta (See faroit/stempeg#28). This PR now does not implement a stems output but instead revert to saving multiple files. Hence the output is the same as before: multiple files saved in multiprocessed fashion. If you like this, additional adapters could be added that would support the new stempeg.ChannelWriter and stempeg.NIStemsWriter... But lets do this step by step and handle faroit/stempeg#28 first so that all future use cases here are covered.

You will note that I added container vs. codec. I think this is currently incorrect and should be addressed in a separate PR.

Also, note, that I didn't update the unit tests, hence they probably fail.

@mmoussallam
Copy link
Collaborator

Hi @faroit

I've been making some tests and with a few very minor changes I can make it work flawlessly. I have a question though, is there a specific reason you're not catching ffprobe errors ?
I'm asking because a lot of the issues we received in the early days were users having either bad audio input files or mistakes in their paths (which led us to write this in the FAQ ) and ffprobe errors were not straightforward to understand for them.

Do you think we can add an equivalent mechanism in stempeg to catch common input errors ? I can drop you a PR for that eventually. Otherwise I can probably tweak a bit on spleeter side to catch those.

@faroit
Copy link
Contributor Author

faroit commented Dec 5, 2020

Yes, I know about ffprobe errors, I get these issue from users of musdb all the time as well ;-)

Do you think we can add an equivalent mechanism in stempeg to catch common input errors ? I can drop you a PR for that eventually. Otherwise I can probably tweak a bit on spleeter side to catch those.

@mmoussallam of course error handling for ffmpeg should be in stempeg.

  1. FFmpeg binary is not found (not installed, or not in OS PATH so executable is not found). This is handled here:

https://github.com/faroit/stempeg/blob/ed80514503afbe93bdb463c7eb5bd30a310df852/stempeg/cmds.py#L23-L31

  1. The track path is not valid (meaning ffmpeg does not find your file) or the track could not be read by ffmpeg (because it's corrupted, codec not supported, or whatever reason). This is handled here:

https://github.com/faroit/stempeg/blob/ed80514503afbe93bdb463c7eb5bd30a310df852/stempeg/read.py#L150-L160

This bit was copied over from spleeter so in the end the level of ffmpeg failsafetyness should be the same ;-)

@axeldelafosse
Copy link

Hey guys! Any news on this?

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.

3 participants