Cue Frame Refactoring #4069
Cue Frame Refactoring #4069
Conversation
src/test/analyzersilence_test.cpp
Outdated
| } | ||
|
|
||
| TEST_F(AnalyzerSilenceTest, EndToEndToneTrack) { | ||
| // Fill the entire buffer with 1 kHz tone | ||
| double omega = 2.0 * M_PI * kTonePitchHz / pTrack->getSampleRate(); | ||
| for (int i = 0; i < nTrackSampleDataLength; i++) { | ||
| pTrackSampleData[i] = static_cast<CSAMPLE>(cos(i / kChannelCount * omega)); | ||
| pTrackSampleData[i] = static_cast<CSAMPLE>(cos(i * kChannelCount * omega)); |
There was a problem hiding this comment.
This looks suspicious. Why did the division become a multiplication? And if it is wrong then why does this not affect the result of the test?
There was a problem hiding this comment.
Good catch, must have been slipped in while I was doing a regex replace and wasn't careful enough.
There was a problem hiding this comment.
I reverted and amended the original commit. But it makes no difference for the test result.
| mixxx::audio::SampleRate sampleRate) { | ||
| VERIFY_OR_DEBUG_ASSERT(sampleRate.isValid()) { | ||
| return std::nullopt; | ||
| } | ||
| if (positionSamples == Cue::kNoPosition) { | ||
| if (!position.isValid()) { | ||
| return std::nullopt; | ||
| } | ||
| // Try to avoid rounding errors |
There was a problem hiding this comment.
Does this comment still apply? I guess it referred to limiting the number of divisions.
There was a problem hiding this comment.
Ok. I removed the comment. c86cbb6
src/track/cue.h
Outdated
| void setStartPosition(mixxx::audio::FramePos position); | ||
| void setStartPosition(double samplePosition) { | ||
| mixxx::audio::FramePos position; | ||
| if (samplePosition != Cue::kNoPosition) { |
There was a problem hiding this comment.
Why not reuse fromEngineSamplePosMaybeInvalid() here and all other legacy adapter functions? This would require a static assertion that Cue::kNoPosition == kLegacyInvalidEnginePosition (which already exists as I just noticed).
src/track/track.h
Outdated
| if (!position.isValid()) { | ||
| return {}; | ||
| } | ||
| return {position.toEngineSamplePos()}; |
There was a problem hiding this comment.
Why do you use fromEngineSamplePosMaybeInvalid() (see below) but not toEngineSamplePosMaybeInvalid() here?
Holzhaus
force-pushed
the
frame-refactor-prerequisites
branch
from
e9d76d8
to
902e4a8
Compare
|
@uklotzde Sorry for the force push, I only reverted the unintended analyzersilencetest changes. |
|
Sorry for the PR flood, but the main reason for adding both FramePos-based and samplepos-based methods was that all changes in related code can be done independently and we can keep each PR reasonably small. |
This duplicates the Cue related methods in order to support Frame positions. This allows migrating the individual parts of the code individually instead of all at once. The original sample-based methods will be removed in #4061.