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.

Sign up for GitHub

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

Jump to bottom

feat: show progress of the audio extraction #45

Merged
merged 2 commits into from
Sep 1, 2023
Merged

feat: show progress of the audio extraction #45

merged 2 commits into from
Sep 1, 2023

Conversation

devinburnette
Copy link
Contributor

I decided to take a stab at #8 also. This one proved to be a bit tougher. I can understand if there's a certain part of the code you want to refactor a bit, but figured this was a good enough starting point. Happy to talk through any changes you'd like to make.

I wasn't sure what looked better, just doing the processQueueCard or the ResultCard or both. So I did both.

I didn't include the instrumental as one of the total outputs in the progress output since it's not generated by demucs and ffmpeg does it super fast.

Screenshot included below:
Screenshot 2023-08-19 at 4 21 06 AM

@devinburnette devinburnette force-pushed the feature-real-time-progress branch 3 times, most recently from 364f11c to bdcc658 Compare August 19, 2023 15:50
@devinburnette devinburnette force-pushed the feature-real-time-progress branch from bdcc658 to 220f7cb Compare August 19, 2023 15:52
@iffyloop
Copy link
Member

Wow Devin, this is super helpful! Thanks for getting involved in StemRoller development. I'll try to get this (and your previous PR) tested and merged into main sometime this week.

@devinburnette devinburnette force-pushed the feature-real-time-progress branch from d2773ee to 6240a7c Compare August 22, 2023 03:09
@iffyloop iffyloop changed the base branch from main to develop September 1, 2023 18:01
@iffyloop iffyloop merged commit 38eaaec into stemrollerapp:develop Sep 1, 2023
@iffyloop
Copy link
Member

iffyloop commented Sep 1, 2023

@devinburnette I went ahead and merged this into develop in order to test - seems to work great!
I do agree that it'd be worth refactoring, primarily because we already have a system in place for passing video processing status to the frontend, so exposing an additional onUpdateProgress handler is confusing. The problem with the existing system is that the status is a plain string, so it can't contain additional information such as a numerical progress. If we could refactor this such that status is an object, and we use a property (e.g. type) which is used for the same purpose as the status string is currently, then we could add additional properties such as a progress value where appropriate. This would be a significant refactor; would you like to give this a try or would you rather me take this on? I'll link some relevant portions of the code below:
https://github.com/stemrollerapp/stemroller/blob/develop/main-src/main.cjs#L177
https://github.com/stemrollerapp/stemroller/blob/develop/main-src/processQueue.cjs#L418
https://github.com/stemrollerapp/stemroller/blob/develop/renderer-src/components/ProcessQueueCard.svelte#L55
https://github.com/stemrollerapp/stemroller/blob/develop/renderer-src/components/ResultCard.svelte#L47

@devinburnette devinburnette mentioned this pull request Sep 2, 2023
Merged
@devinburnette
Copy link
Contributor Author

@iffyloop okay! I took a stab at that over here in a separate PR against main. Let me know if this is more aligned with what you were thinking. If so, it wasn't all that troublesome to refactor.

iffyloop added a commit that referenced this pull request Oct 8, 2023
@iffyloop @devinburnette
Progress indicator, MP3, dependency updates (#49)
ef4978e 
* support mp3 file format for resulting stems (#44)

* feat: show progress of the audio extraction (#45)

* show progress of the audio extraction

* refactor it a little by using the parent components instead to fix mem leak

* feat: real time progress updates / refactor (#47)

* first refactor status to be an object

* pass progress through with the status object to processQueueCard

* also pass progress through to the resultCard

* add quantity also

* fix done step

* fix indentation and spacing

---------

Co-authored-by: iffyloop <[email protected]>

* Minor fixes

* Fix RegEx and progress display

* Update NPM dependencies

* Remove unused exports

* Revert initial progress update implementation

* Rename quantity -> stemIdx

---------

Co-authored-by: Devin Burnette <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@devinburnette @iffyloop