Implement Un-eject#4668
Merged
daschuer merged 7 commits intomixxxdj:mainfrom Feb 18, 2022
Merged
Conversation
ywwg
commented
Feb 8, 2022
| &PlayerManager::slotStoreParkedTrack); | ||
| } | ||
|
|
||
| // Connect the player to the analyzer queue so that loaded tracks are |
Member
Author
There was a problem hiding this comment.
I decided not to connect the signal to the preview deck, because that deck will have a lot more churn, but that could be changed
Member
Author
|
will add a test if people like the idea |
Member
|
Makes sense to me! |
Member
Author
|
there are some remaining questions around exact implementation but I think there's consensus at least for the concept. We can start with this and iterate as needed |
uklotzde
reviewed
Feb 9, 2022
| VERIFY_OR_DEBUG_ASSERT(track) { | ||
| return; | ||
| } | ||
| m_pLastEjectedTrack = track; |
Contributor
There was a problem hiding this comment.
Keeping more tracks loaded in-memory for an unforeseeable and unlimited amount of time will only fuel the various issues we already experience with delayed database updates and metadata synchronization. Uncontrolled accumulation of state gets you into trouble.
Member
There was a problem hiding this comment.
The trackId can be stored without issues, right?
Member
There was a problem hiding this comment.
Performance wise it is a good idea to use the pointer.
How is the situation with our cache? Is guaranteed that the last ejected track is kept in cache?
Holding the pointer here enforces that, which is a good idea from that perspective.
The the issue with this is that it is hidden behind the scenes and it is hard to predict when the metadata is written to the file. We are already in that state, right? Is this relevant here?
Contributor
There was a problem hiding this comment.
I see stale search results all the time, caused by these kind of issues. It's not only about metadata.
And it is bad practice to keep data alive for an unlimited of time. If you don't use the deck anymore the pointer is kept alive until Mixxx is finally closed.
Either store an ID are use a timer to reset the pointer periodically. I guess we have a similar caching issue in BaseTrackCache.
Contributor
There was a problem hiding this comment.
And remember. These are fat QObjects with all the wiring that may cause all kinds of weird side-effects. This design is already unmaintainable. Don't make the situation worse.
Member
Author
There was a problem hiding this comment.
yeah we can switch this to an ID.
As to Daniel's question above, if the ID is invalid (because of something strange like the user deleted the file) the track load will simply fail
Member
Author
There was a problem hiding this comment.
Any hint for how to load a track by trackid? I am a little lost in the library code
Contributor
There was a problem hiding this comment.
TrackCollectonManager::getTrackById()
Member
|
Schould this work for any unload action, like replacing a track? Or only for explicit "ejects"? A corner case would be to load an not existing track. Does it eject the old? Is this considered as an eject action? |
daschuer
requested changes
Feb 9, 2022
| return; | ||
| } | ||
| if (!m_pLoadedTrack) { | ||
| auto lastEjected = m_pPlayerManager->getLastEjectedTrack(); |
Member
There was a problem hiding this comment.
We do not know what lastEjected is here. So I would prefer to either rename it or replace auto.
| &PlayerManager::slotStoreParkedTrack); | ||
| } | ||
|
|
||
| // Connect the player to the analyzer queue so that loaded tracks are |
| VERIFY_OR_DEBUG_ASSERT(track) { | ||
| return; | ||
| } | ||
| m_pLastEjectedTrack = track; |
Member
There was a problem hiding this comment.
Performance wise it is a good idea to use the pointer.
How is the situation with our cache? Is guaranteed that the last ejected track is kept in cache?
Holding the pointer here enforces that, which is a good idea from that perspective.
The the issue with this is that it is hidden behind the scenes and it is hard to predict when the metadata is written to the file. We are already in that state, right? Is this relevant here?
Member
Author
any unload would trigger this, including replacing a track in a deck |
Member
In Zulip I also threw in "un-replace", i.e. reload the track which was (accidentally) replaced. |
Member
Author
I would really prefer not to implement a double-click function in this first change. Can that wait for a followup? |
Member
|
Sure, no hurry. I was just wondering if you think this is a valid use case / extension. If so, it'll probably be better to consider it now already, instead of having to bend over afterwards. |
Member
Author
Cool. Yeah I think an un-replace would be useful, and wouldn't be hard to implement with this as a foundation -- it could be a new CO, or a doubleclick / long press CO, and hook into the same basic structure. The refactor of the eject mechanism into playermanager is what makes it possible. |
Member
Author
|
I believe notes are addressed. I now store the trackid instead of trackpointer to free the resources. The test is pretty heavy now because we need a database to do the library lookup, but it's not so bad. |
Member
|
Oh there is an Qt 6 issue: |
Member
Author
|
I have been unable to install / test QT6. Can someone else figure out what it's complaining about? (a stack trace would help) |
Member
Author
|
or can we just disable the test on qt6 for now |
Member
|
I don't get the bad_alloc error but these 3 tests fail: |
Member
Author
|
looks like I'm double-creating a Library. |
Member
Author
|
That was unpleasant -- it would be nice if we could create a test-friendly coreservices mode, that would simplify a lot of this test. |
Member
Author
|
@daschuer can you retest with qt6 and if it still crashes can you build with |
Member
I'm not @daschuer but here's your stack trace: |
Member
Author
|
Try again -- and if it fails again can you print out the values of m_data.size() and iSize at the crash point? those values should be available. And also too, if you know the exact version of qt6 you have I can be more sure about which assertion is triggering the error. But I think that change might fix it. |
Member
Author
|
Member
Author
|
it looks like some invalid effect request is getting created with a bad channelhandle. |
Member
Author
|
asan: |
Member
Author
|
ok so we are passing around pointers to channel handles, and I believe those pointers become invalid -- perhaps due to them being pointers inside a QVarLengthArray |
Member
Author
|
The Bad Line: https://github.com/mixxxdj/mixxx/blob/main/src/effects/effectchain.cpp#L397 this address becomes invalid |
Contributor
I agree that this code is bad. But it does not take the address of a temporary, that's not true. It takes the address of a reference. If this is safe or not depends on the outer context. |
Contributor
My suspicion is that something is awry with the way EngineChannels are constructed in the test environment. |
Contributor
@be I checked the effects and |
Member
Author
that could well be the case but I've been checking against coreservices and can't find anything different yet |
uklotzde
suggested changes
Feb 12, 2022
Contributor
|
Running the test repeatedly with #4675 applied always succeeded when built with Qt 6. |
Member
Author
|
with #4672 the test crash should be fixed |
Member
Author
|
ah sorry a bunch of debug crap snuck in |
When ejecting a track, save a pointer to the ejected track. If the user presses eject on an empty track in any deck or sampler, the saved track is loaded. The track is loaded fresh, not as a "Deck clone" operation. This enables a sort of "eject undo" as well as "track stashing". It requires no changes to controller configs. This required a refactor of the eject code to remove it from the EngineBuffer. Tests still pass. Hand-testing works.
Updated test to exercise the library lookup
Member
|
I did a brief test and it is still working. Thank you very much. |
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When ejecting a track, save a pointer to the ejected track. If the user presses eject on an empty track in any deck or sampler, the saved track is loaded.
The track is loaded fresh, not as a "Deck clone" operation.
This enables a sort of "eject undo" as well as "track stashing". It requires no changes to controller configs.
This required a refactor of the eject code to remove it from the EngineBuffer. Tests still pass. Hand-testing works.