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

Allow playing tracks longer than 6 h #11511

Merged
merged 11 commits into from
Aug 11, 2023
Merged

Allow playing tracks longer than 6 h #11511

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
a5ddb48
Assert that we have no integer overflow in the AudioSource.
daschuer Apr 25, 2023
ae94ca7
Analyzer: Work with SINT frameLength and make sure it is not multipli…
daschuer Apr 25, 2023
7490535
Pass number of track samples as doubel via trackLoaded to work around…
daschuer Apr 25, 2023
504f5ba
Fix int overflow in LoopingControl
daschuer Apr 25, 2023
c1a2f61
Fix integer overflow in waveform classes
daschuer Apr 25, 2023
5aa00fb
Merge remote-tracking branch 'upstream/2.3' into gh11504
daschuer May 22, 2023
017aca6
Merge remote-tracking branch 'upstream/2.3' into gh11504
daschuer Jun 1, 2023
4d227d0
Fix assertion against backward ranges
daschuer Aug 9, 2023
7cc5df0
Introduce kCanarySampleRate
daschuer Aug 10, 2023
f07fe40
Replace tio by pTrack
daschuer Aug 10, 2023
2a3436e
Replace kCanarySampleRate with m_pTrack->getSampleRate()
daschuer Aug 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/engine/cachingreader/cachingreader.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ class CachingReader : public QObject {
signals:
// Emitted once a new track is loaded and ready to be read from.
void trackLoading();
void trackLoaded(TrackPointer pTrack, int iSampleRate, int iNumSamples);
void trackLoaded(TrackPointer pTrack, int trackSampleRate, double trackNumSamples);
Copy link
Member

Choose a reason for hiding this comment

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

why do you prefer double of int64_t/long long int in this case?

Copy link
Member Author

@daschuer daschuer Aug 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is used as double in the receiver EngineBuffer::slotTrackLoaded() and was double in CachingReaderWorker::loadTrack(). The change is a fix.

Copy link
Member

Choose a reason for hiding this comment

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

yes but why use double when we only want/expect precise integer values?

Copy link
Member Author

@daschuer daschuer Aug 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

At one point we need to go from the Audio Source SINT to the Engine double. The original 2.3 code does it in the CachingReaderWorker. Using int for passing this value was the bug. double Int double.
Now it is double double double.

Copy link
Member

Choose a reason for hiding this comment

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

Right that makes sense, now why is the engine using double?

Copy link
Member Author

Choose a reason for hiding this comment

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

The engine transport is controlled by the output after resampling in whole frames. Fractional rates are causing fractional input positions from the sound source. That's why the engine buffer is using double for positions.

Copy link
Member

Choose a reason for hiding this comment

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

right, but that's for positions, why do we have fractional lengths? There are no sample buffers with fractional sizes, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is right. The lengths is also the end position. Sure, we can change that to SINT and compare it to the double position on the fly. But that will be a refactoring PR and not a bug fix. I also don't see a benefit to change that.

Copy link
Member

Choose a reason for hiding this comment

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

Well if we changed that to SINT, wouldn't we suffer the overflow issue again? The thing I'm worried about is that the code inside those methods will get more difficult since now all of a sudden we might get fractional lengths passed. That will make the code more difficult to reason about. I think a comment and/or a DEBUG_ASSERT is the least we can do document that thats not the case.

void trackLoadFailed(TrackPointer pTrack, const QString& reason);

private:
Expand Down
9 changes: 6 additions & 3 deletions src/engine/cachingreader/cachingreaderchunk.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,20 @@ class CachingReaderChunk {
static const SINT kSamples;

// Converts frames to samples
inline static SINT frames2samples(SINT frames) {
static SINT frames2samples(SINT frames) {
return frames * kChannels;
}
static double dFrames2samples(SINT frames) {
return static_cast<double>(frames) * kChannels;
}
// Converts samples to frames
inline static SINT samples2frames(SINT samples) {
static SINT samples2frames(SINT samples) {
DEBUG_ASSERT(0 == (samples % kChannels));
return samples / kChannels;
}

// Returns the corresponding chunk index for a frame index
inline static SINT indexForFrame(
static SINT indexForFrame(
/*const mixxx::AudioSourcePointer& pAudioSource,*/
SINT frameIndex) {
//DEBUG_ASSERT(pAudioSource->frameIndexRange().contains(frameIndex));
Expand Down
4 changes: 2 additions & 2 deletions src/engine/cachingreader/cachingreaderworker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,8 +215,8 @@ void CachingReaderWorker::loadTrack(const TrackPointer& pTrack) {
m_pReaderStatusFIFO->writeBlocking(&update, 1);

// Emit that the track is loaded.
const SINT sampleCount =
CachingReaderChunk::frames2samples(
const double sampleCount =
CachingReaderChunk::dFrames2samples(
m_pAudioSource->frameLength());

// The engine must not request any chunks before receiving the
Expand Down
2 changes: 1 addition & 1 deletion src/engine/cachingreader/cachingreaderworker.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ class CachingReaderWorker : public EngineWorker {
signals:
// Emitted once a new track is loaded and ready to be read from.
void trackLoading();
void trackLoaded(TrackPointer pTrack, int iSampleRate, int iNumSamples);
void trackLoaded(TrackPointer pTrack, int sampleRate, double numSamples);
void trackLoadFailed(TrackPointer pTrack, const QString& reason);

private:
Expand Down
13 changes: 7 additions & 6 deletions src/engine/enginebuffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,14 +498,15 @@ void EngineBuffer::loadFakeTrack(TrackPointer pTrack, bool bPlay) {
if (bPlay) {
m_playButton->set((double)bPlay);
}
slotTrackLoaded(pTrack, pTrack->getSampleRate(),
pTrack->getSampleRate() * pTrack->getDurationInt());
slotTrackLoaded(pTrack,
pTrack->getSampleRate(),
pTrack->getSampleRate() * pTrack->getDuration());
}

// WARNING: Always called from the EngineWorker thread pool
void EngineBuffer::slotTrackLoaded(TrackPointer pTrack,
int iTrackSampleRate,
int iTrackNumSamples) {
int trackSampleRate,
double trackNumSamples) {
if (kLogger.traceEnabled()) {
kLogger.trace() << getGroup() << "EngineBuffer::slotTrackLoaded";
}
Expand All @@ -515,8 +516,8 @@ void EngineBuffer::slotTrackLoaded(TrackPointer pTrack,
m_visualPlayPos->setInvalid();
m_filepos_play = kInitialSamplePosition; // for execute seeks to 0.0
m_pCurrentTrack = pTrack;
m_pTrackSamples->set(iTrackNumSamples);
m_pTrackSampleRate->set(iTrackSampleRate);
m_pTrackSamples->set(trackNumSamples);
m_pTrackSampleRate->set(trackSampleRate);
// Reset slip mode
m_pSlipButton->set(0);
m_bSlipEnabledProcessing = false;
Expand Down
6 changes: 4 additions & 2 deletions src/engine/enginebuffer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,8 +182,10 @@ class EngineBuffer : public EngineObject {

private slots:
void slotTrackLoading();
void slotTrackLoaded(TrackPointer pTrack,
int iSampleRate, int iNumSamples);
void slotTrackLoaded(
TrackPointer pTrack,
int trackSampleRate,
double trackNumSamples);
void slotTrackLoadFailed(TrackPointer pTrack,
const QString& reason);
// Fired when passthrough mode is enabled or disabled.
Expand Down