AutoDJProcessor refactoring and split into separate files - #16050
AutoDJProcessor refactoring and split into separate files#16050fonsargo wants to merge 17 commits into
Conversation
This is a first step in generalizing the transition logic in AutoDJProcessor to enable it to also be applied to tracks in the queue instead of just the current decks.
This is another step in generalizing the transition logic in AutoDJProcessor to enable calculating transition times for the whole queue.
This is the final step in generalizing the core transition timing logic in AutoDJProcessor, so that it can be applied to a whole playlist instead of just the current decks.
acolombier
left a comment
There was a problem hiding this comment.
Thank you for turning the situation around!
I have shared a first pass of review and will let @daschuer do the final one as he raised elements of the rebased PR.
| ControlProxy m_trackSamples; | ||
| ControlProxy m_sampleRate; | ||
| ControlProxy m_rateRatio; | ||
| BaseTrackPlayer* m_pPlayer; |
There was a problem hiding this comment.
We aim to avoid fat pointer, is there any chance to use smart pointer? Note that you may also consider our parented_ptr wrapper that ensure that fat pointer are at least parented, and thus would be deallocated when their parent is released.
There was a problem hiding this comment.
To be honest I don't know how to answer to this question. The object is created and parented in PlayerManager.
I am not the owner of its lifetime. However if we want to avoid pointer here, then it requires a lot of refactoring.
There was a problem hiding this comment.
Right, that makes sense. In this case you can use parented_ptr. The crux of it is here. It is basically a thin layer, which will help us to catch memory leak in case in changes is done in PlayerManager.
For you, it should change anything: you can dereference it like a pointer and nothing will happen to it (e.g auto dealloc)
Just the following change should be enough to use this wrapper:
| BaseTrackPlayer* m_pPlayer; | |
| parented_ptr<BaseTrackPlayer> m_pPlayer; |
There was a problem hiding this comment.
Check, got it! I changed fat pointer to parented ptr as you suggested and replaced
return m_pPlayer != nullptr ? m_pPlayer->getLoadedTrack() : TrackPointer();
with
return m_pPlayer ? m_pPlayer->getLoadedTrack() : TrackPointer();
in DeckAttributes::getLoadedTrack(). I think it should work.
acolombier
left a comment
There was a problem hiding this comment.
Thanks for addressing the feedback - I spotted one miss on my end.
| FadeableTrackOrDeckAttributes& fromTrack, | ||
| FadeableTrackOrDeckAttributes& toTrack, |
There was a problem hiding this comment.
Let's apply the const-correctness principle here too.
| FadeableTrackOrDeckAttributes& fromTrack, | |
| FadeableTrackOrDeckAttributes& toTrack, | |
| const FadeableTrackOrDeckAttributes& fromTrack, | |
| const FadeableTrackOrDeckAttributes& toTrack, |
In case you need them to remain mutable, you should keep the fat pointers
There was a problem hiding this comment.
Changed it back to fat pointers according to the const-correctness principle.
|
I've fixed compile error for other platforms (somehow on Windows it worked fine), however now I see that build is failing for Android 15 (before that it was green 🤔 ). Any suggestions how to fix it? |
I can't promise anything cause I'm busy rn, but if I get some free time this weekend I'll download your fork and try to build it, maybe I get some ideas... |
|
@btoplak I guess it's not connected with the changes, maybe I just need to rebase the branch. I'll try it when I have some time |
|
This PR is marked as stale because it has been open 90 days with no activity. |
|
Now that:
exists (which is my rebase of #13183), this PR can probably be closed. Thanks to @fonsargo for picking this up when I wasn't able to work on this in the last two years! |
This MR is all refactorings from #13183 carefully cherry-picked by me. I also removed some leftovers from new feature, so this MR doesn't contain anything new at all.
Main changes:
calculateTransition()tocalculateTransitionImpl()