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

Add simple support for STEM files #13044

Merged
merged 8 commits into from
May 26, 2024
Merged

Add simple support for STEM files #13044

merged 8 commits into from
May 26, 2024

Conversation

acolombier
Copy link
Member

@acolombier acolombier commented Apr 3, 2024 •
edited
Loading

This feature allows loading STEM file, extract the stem data and ensure the correctness of the file.

The SoundSource will provide the main mix (the first stream by convention) to the audio engine and the stem info data to the track object.

This PR depends on #13042

@acolombier acolombier marked this pull request as draft April 3, 2024 21:22
@acolombier acolombier force-pushed the feat/add-support-for-stem-files branch 4 times, most recently from 5cf2e3d to 8e15057 Compare April 10, 2024 14:49
@JoergAtGithub JoergAtGithub mentioned this pull request Apr 10, 2024
Open
@acolombier acolombier force-pushed the feat/add-support-for-stem-files branch from 8e15057 to e55e13d Compare April 11, 2024 16:29
@acolombier acolombier force-pushed the feat/add-support-for-stem-files branch 4 times, most recently from 0635539 to 4dc066e Compare April 12, 2024 09:31
@acolombier acolombier marked this pull request as ready for review April 12, 2024 10:23
This was referenced Apr 13, 2024
Merged
Merged
@daschuer
Copy link
Member

Can you rebase this on main, to get rid of the first commit?

@acolombier acolombier force-pushed the feat/add-support-for-stem-files branch from 4dc066e to c9fc5ed Compare April 15, 2024 20:53
@acolombier acolombier mentioned this pull request Apr 16, 2024
Open
8 tasks
@acolombier acolombier force-pushed the feat/add-support-for-stem-files branch from c9fc5ed to 325c963 Compare April 16, 2024 21:00
@acolombier
feat: add support for STEM
bdfffb4 
The soundsource will ensure the file is a valid STEM file and load
manifest. It will however only expose the main mix to the sound engine
@acolombier acolombier force-pushed the feat/add-support-for-stem-files branch from 325c963 to bdfffb4 Compare April 16, 2024 21:00
JoergAtGithub
JoergAtGithub reviewed Apr 18, 2024
View reviewed changes
Copy link
Member

@JoergAtGithub JoergAtGithub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some minor remarks

src/track/steminfoimporter.cpp Outdated Show resolved Hide resolved
src/track/steminfoimporter.cpp Outdated Show resolved Hide resolved
src/track/steminfoimporter.cpp Outdated Show resolved Hide resolved
src/track/steminfoimporter.h Outdated Show resolved Hide resolved
src/sources/soundsourcestem.h Outdated Show resolved Hide resolved
JoergAtGithub
JoergAtGithub reviewed Apr 19, 2024
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
@JoergAtGithub
Copy link
Member

JoergAtGithub commented May 5, 2024 •
edited
Loading

Note that, as faired, I have noticed a performance impact on probing the STEM box in each MP4 file, especially when running the tests. This is slightly worrying as I have a pretty fast CPU and NVME, so I assume that performance impact will be noticeable.

Does it only consume that much time, if it is a STEM file, or also for normal MP4 audio files?

@acolombier
Copy link
Member Author

Hard to say - I suspect something is wrong in the way the file type is being detected, leading to the SoundSourceProxy test to timeout. I will do further investigation, and come back with further info.

@acolombier acolombier force-pushed the feat/add-support-for-stem-files branch 2 times, most recently from fba41fe to 2ee240f Compare May 5, 2024 16:07
@acolombier
Copy link
Member Author

After running the SoundSourceProxyTest through callgrind, it looks like the probing of the stem atom was too greedy and could stop recursion early on. There is still a slight performance impact but that should be acceptable now.

@acolombier acolombier force-pushed the feat/add-support-for-stem-files branch from 2ee240f to 23d7866 Compare May 5, 2024 16:24
JoergAtGithub
JoergAtGithub reviewed May 5, 2024
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
@acolombier acolombier force-pushed the feat/add-support-for-stem-files branch from f0d3a52 to 79614a0 Compare May 6, 2024 22:35
@acolombier acolombier requested a review from JoergAtGithub May 6, 2024 22:35
JoergAtGithub
JoergAtGithub approved these changes May 7, 2024
View reviewed changes
Copy link
Member

@JoergAtGithub JoergAtGithub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I compiled this locally on Windows11 and it compiles fast and tests pass. All my change requests are addressed!

@JoergAtGithub JoergAtGithub added this to the 2.6-beta milestone May 8, 2024
@acolombier acolombier requested a review from daschuer May 9, 2024 22:12
@acolombier acolombier requested a review from daschuer May 13, 2024 11:35
@acolombier @daschuer
Update docstring for STEM soundsource
f7dbac4 
Co-authored-by: Daniel Schürmann <daschuer@mixxx.org>
@acolombier acolombier force-pushed the feat/add-support-for-stem-files branch from 6dec854 to f7dbac4 Compare May 13, 2024 15:43
@JoergAtGithub JoergAtGithub mentioned this pull request May 14, 2024
Merged
@acolombier @dszakallas
Update src/sources/soundsourcestem.h
76b12c7 
Co-authored-by: Dávid Szakállas <5807322+dszakallas@users.noreply.github.com>
@acolombier
Copy link
Member Author

@daschuer friendly ping in case you missed the notification

daschuer
daschuer approved these changes May 26, 2024
View reviewed changes
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thank you.

Sorry for the delay. I have underestimated the time for branching out 2.5 compared to my free time this week.

@daschuer daschuer merged commit 3c27ef3 into mixxxdj:main May 26, 2024
13 checks passed
@acolombier acolombier mentioned this pull request Jun 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoergAtGithub JoergAtGithub JoergAtGithub approved these changes

@dszakallas dszakallas dszakallas approved these changes

@daschuer daschuer daschuer approved these changes

Assignees
No one assigned
Labels
build code quality soundsource
Projects
None yet
Milestone
2.6-beta
Development

Successfully merging this pull request may close these issues.
4 participants
@acolombier @daschuer @JoergAtGithub @dszakallas