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

feat: add advanced stem loading COs #13268

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

acolombier
Copy link
Member

@acolombier acolombier commented May 23, 2024

Currently, this goes with a mix of extending the existing load signal/slot and creating a second one, called subsequently. Is there any preference? Should I change the implementation to use only one? (only extending existing signal or only subsequently emit)

@acolombier acolombier changed the title Feat/add advanced stem load cos feat: add advanced stem loading COs May 23, 2024
@acolombier acolombier mentioned this pull request May 23, 2024
11 tasks
@acolombier acolombier force-pushed the feat/add-advanced-stem-load-cos branch 2 times, most recently from 30fd666 to 2f2a22a Compare May 23, 2024 19:42
@acolombier acolombier force-pushed the feat/add-advanced-stem-load-cos branch 2 times, most recently from b4db8c8 to fdd8810 Compare June 3, 2024 10:56
@acolombier acolombier force-pushed the feat/add-advanced-stem-load-cos branch 2 times, most recently from a36c6a0 to 31becb9 Compare June 29, 2024 15:57
@acolombier acolombier mentioned this pull request Jul 25, 2024
@JoergAtGithub JoergAtGithub added this to the 2.6-beta milestone Jul 28, 2024
@acolombier acolombier force-pushed the feat/add-advanced-stem-load-cos branch from 31becb9 to fab6880 Compare July 30, 2024 21:34
@acolombier
Copy link
Member Author

Please note that UI changes (1st and 3rd commits) will be moved to #13515 once this PR is ready to be merged!

@acolombier acolombier force-pushed the feat/add-advanced-stem-load-cos branch 2 times, most recently from 2fc47ba to 868a67e Compare July 31, 2024 07:45
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.

I have some doubts if this is consistent with the other PRs and the overall user experience.

The users sees only the track in the library which has four stems. The use can now load a single stem form a file or load the whole stem file and mute all 3 of them.
In the first case they cannot add a second stem in the second case they suffer the penalty of stem processing. They need a brief understanding of the engine interna to make the right decisions during mixing which could be limiting.

Can we join both? How can we allow to seamlessly switch between these cases.

A typical use case is probably:

  • Play/Cue a full track
  • Notice unusable part
  • Switch to stem mode an silence that part
  • Instant double the track in a second deck
  • Bring the silenced part in using an effect

How can we allow this using the new single stem mode introduced here behind the scenes?

,
stemIdx
#endif
);
Copy link
Member

Choose a reason for hiding this comment

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

That looks not right. How about use two lines? We may also consider to reduce the use of __STEM__ where it has no performance impact like here. We already have the default = 0.

Copy link
Member Author

Choose a reason for hiding this comment

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

Make sense. So would you prefer I update the signature globally?

Copy link
Member

Choose a reason for hiding this comment

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

Yes in general. I made this comment before raising my general concerns regarding this single stem mode. Maybe we need a slightly different signature .. unsure..

Copy link
Member Author

Choose a reason for hiding this comment

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

I will wait for the full review before starting refactoring then :)

Copy link
Member Author

@acolombier acolombier Aug 22, 2024

Choose a reason for hiding this comment

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

I have replaced all the signature changes with two explicit lines, which makes it easier to understand now. Hopefully this works for you!

@acolombier
Copy link
Member Author

How can we allow to seamlessly switch between these cases.

The first scenario is the only way on non-primary decks (sampler and preview), while second is the default for primary deck. realistically, I only added the second option for primary deck for feature parity and enabled that advanced capability, or workaround for light platform which would struggle with more than one stem deck + rubberband (eg. raspberrypi)

How can we allow this using the new single stem mode introduced here behind the scenes?

By default, the load process will still load the full stem file. Loading an individual stem should be reserved for "aware users". A typical use case is to load vocals or drum only in a sampler, or a deck if you want more transport control.

By the sounds of it, the use case you are describing (clone switching between individually loaded stem vs full deck with muted tracks) are falling under the the split use-case we've discussed in the epic, or am I reading this wrong?

@daschuer
Copy link
Member

Ah, OK I forget about the sampler use case.

Than let's think about a way to gerearize this topic. Since it is handled by the same engine class anyway.

How is the transition from single stereo to stem now done? Is there way to keep the transport going during the switch? A switch from the full track to a stem mix is probably the same as a switch from a stem mix to one of the other stems. Do we need to consider something like "load as karaoke" where we process three of the stems?

I am not demanding to implement it here. I just like to brainstorm possible future solution to have the Co interface proved for that.
My feeling is that current one is too static.

Idea: can we use the mute controls for selecting the loaded stems?

@acolombier
Copy link
Member Author

How is the transition from single stereo to stem now done?

Currently there is no way transition "live" for a stem deck to a stereo deck with a track. With that current feature, can chose the load a track in an empty deck. If we think of a case where one would want to switch from a stem deck to a stereo deck, they would need to use an empty deck and handle the transport synch.
In the future, we could consider the ability to do this live on a deck, but I would emit some reserves as of whether or not this is possible. Particularly, I'm not sure how we would handle the CachingReader sync, and the arming of the RubberBand. I think it safer to expect that such a transition should always be done on another deck.

Is there way to keep the transport going during the switch?

It would be fairly trivial to implement to some kind of clone_as_stereo that makes the transport cloning as well, as the existing cloning does. I'm not planing to implement cloning feature as part of this PR or the GSoC as I have recently less time (sadly back to a full-time job...), but I'm hoping to still get something together before the end of Q3.

Do we need to consider something like "load as karaoke" where we process three of the stems?

I think it would be great to even go further an allow loading a set of stem pre-mixed to stereo (eg. 2+3, 1+3+4, ...), but same as above, I think it is big enough to split it as a future feature.

Note that in case not all the above feature wouldn't be ready for 2.6 and we would like to wait for completeness before releasing, the STEM build option should still be working and allow us to keep that feature behind curtain till 2.7. Personally, I hope we can release what we have in 2.6 but it's purely subjective and selfish opinion! :)

Idea: can we use the mute controls for selecting the loaded stems?

This is already how we were planning to make the split_* CO work. Say you are playing 1+2+3+4 on A, if you split 3, you would end up with 1+2+4 on A with 3 muted, and B playing 3 but 1+2+4 muted, allowing you to play with the transport between stem track, while still allowing you eventually to "merge" back to a unify transport (by unmuting 3 on A, or 1+2+4 on B)
Now that would require a fairly powerful processing power if we consider RubberBand-ing 8 stereo channels, so I guess we could add a variant (or a user preference?) so splitting would lead to having 1+2+4 on A with 3 muted, and B playing just 3 as a stereo deck (meaning you can only merge back to A and cannot add new stem tracks to B's transport)

@acolombier
Copy link
Member Author

I have removed the GUI element which aren't ready to be merged. I believe it shouldn't prevent testing this feature! I have also removed COs that are not yet ready to go and added the mask stem selection, as well as the menu for it.
Screencast from 22-08-24 15:49:45.webm

@acolombier acolombier force-pushed the feat/add-advanced-stem-load-cos branch from 03d252d to 6782bde Compare August 22, 2024 15:00
@acolombier acolombier force-pushed the feat/add-advanced-stem-load-cos branch from 6782bde to 0f433d4 Compare August 22, 2024 15:04
@JoergAtGithub
Copy link
Member

The multi stem selection menu works great! Like it!

But two improvements are needed:

  1. "Load as stem deck" should only be available for main decks, because the other will not have stem controls
  2. The waveforms should be standard waveforms, if we not using a stem deck (these need to be calculated on the fly at track load, as we can not generate all 2^4 - 1 possibilities during track analysis. But we can cache them once generated, which makes sense, if you restart Mixxx with many Sampler decks pre-loaded with stem-tracks)

@daschuer
Copy link
Member

Did you consider to put the stem selection in the root? Like proposed here: #13268 (comment)

The think we have two options:
"Load all Stems" and "Load the stereo mixxx (stream 0)"
In the load similar menu, we went a different approach instead of holding the Ctrl key, there is a dedicatthed load action.

I think we should keep the same UX in all such menues.
We have heated saving one menu entry vs reaching out to the control key when using by mouse/controller.
@ronso0 what is the best option?

"Load as stem deck" sounds wrong. There is no concept of a "stem deck" for a user. The deck has always stem controls. Maybe "Load for remixing"?

@acolombier
Copy link
Member Author

Did you consider to put the stem selection in the root? Like proposed here: #13268 (comment)

I did - AFAIS, I cannot see anyway to make a multiple selection solution like I did already, without making something overly complicated.
Personally, it also sounds redundant and confusing as the "Load to" sub menu would like clarity to the user.

2. The waveforms should be standard waveforms, if we not using a stem deck (these need to be calculated on the fly at track load, as we can not generate all 2^4 - 1 possibilities during track analysis. But we can cache them once generated, which makes sense, if you restart Mixxx with many Sampler decks pre-loaded with stem-tracks)

The waveform renderer uses the same data available in the analysis. The renderer consume the signal that expose the stem mask and only display the stem signal actually played, similar as if it was muted on stem deck. This allows the waveform to be more aligned with what is played. I suggest we keep it this way, as it is the only way to know what is loaded

@JoergAtGithub
Copy link
Member

Did you consider to put the stem selection in the root? Like proposed here: #13268 (comment)

I did - AFAIS, I cannot see anyway to make a multiple selection solution like I did already, without making something overly complicated.

I think @acolombier is right here! It can not work the other way around, because the possible operations depend on the type of deck. Therefore the deck type must be known, before we can show the possible load operations to the user.

@daschuer
Copy link
Member

Ok, than let's continue this way and wait for user's test results.

@daschuer
Copy link
Member

I think this looks almost mature regarding the upcoming deadline. I just need to do manual tests to confirm that it does not default or such. A decision about the visual sting suggestion would be also nice before merge, to not create double translatiors work. The rest can be postponed. Is that OK?

@acolombier
Copy link
Member Author

The think we have two options:
"Load all Stems" and "Load the stereo mixxx (stream 0)"

Currently, we cannot load the stream 0 due to DSP not being supported. However, I guess we could add a Load pre-mixed stereo track, which can currently load all stems pre-mixed, and load the pre-mixed stream in the future once DSP is fully supported, since "all stems pre-mixed + DSP" should give exactly the same than pre-mixed track.

"Load as stem deck" sounds wrong. [...] Maybe "Load for remixing"?

I'm not a big fan of this alternative, but appreciate that a "stem deck" may be misleading. I have gone with Load for stem mixing, hopefully this one can get the consensus.

@acolombier acolombier force-pushed the feat/add-advanced-stem-load-cos branch from 360b92a to 9e1ea3d Compare August 24, 2024 14:25
@acolombier acolombier force-pushed the feat/add-advanced-stem-load-cos branch from 9e1ea3d to 394ba4a Compare August 24, 2024 14:28
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.

If I "Load pre-mixed stereo track" into a main deck, I see no waveform at all:
grafik

@acolombier
Copy link
Member Author

@JoergAtGithub it took me sometime to reproduce - my theory is that selectedStem is updated between the render of the waveform layer, but I'm not quite sure. I have added a redraw schedule after the selectedStem update to make sure the all stack is updated with the new value and couldn't reproduce anymore, but since I was struggling to reproduce the issue in the first place, I'm not sure I nailed it completely...

Could you please give it another try?

@JoergAtGithub
Copy link
Member

I tried this again, and it does not fix the waveform issue. I found out, that if I select multiple stems with CTRL+Click it works up to three stems, but not if I select all 4.

@acolombier
Copy link
Member Author

acolombier commented Sep 20, 2024

As I cannot narrow down how the race condition is happening, and the PR has been blocked for more than a month now, I have reverted the commit which introduced the loading of the stereo waveform if the stem was loaded as stereo.
Appreciate this is sub-optimal, but this PR is already 50 files change and I wouldn't want it to go to stale.

Are we happy with that trade-off? @JoergAtGithub could you confirm that there is no issue with this latest version?

@JoergAtGithub
Copy link
Member

The waveforms now allways appear!
But I noticed another issue, it sometimes happens, that a track is loaded before I release the CTRL button, it loads in the moment of clicking on a aother stem to select it.

@JoergAtGithub
Copy link
Member

Works as expected now! Thank you!

@acolombier
Copy link
Member Author

Friendly ping @daschuer, in case you've missed the notification :)

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

Successfully merging this pull request may close these issues.

5 participants