Allow playing tracks longer than 6 h #11511

daschuer · 2023-04-25T20:52:00Z

After this PR is merged, we can play 13h31:35.8 length @ 44100 Hz on 32 bit targets and 3186904 years on 64 bit.

This fixes #11504

This allows 13h31:35.8 length @ 44100 Hz on 32 bit targets and 3186904 years on 64 bit

…ed by the number of channels in the using code.

… an integer overflow.

daschuer · 2023-05-06T07:25:32Z

This can be considered as bugfix. Merge to 2.3.5?

# Conflicts: # src/test/analyserwaveformtest.cpp

daschuer · 2023-06-01T21:20:43Z

Conflicts again resolved

daschuer · 2023-08-09T06:32:14Z

Has one interest to test and review this or should we move this to 2.3.6?

Swiftb0y · 2023-08-09T12:47:52Z

src/sources/audiosource.cpp

@@ -27,6 +27,7 @@ AudioSource::AudioSource(
          m_signalInfo(signalInfo),
          m_bitrate(inner.m_bitrate),
          m_frameIndexRange(inner.m_frameIndexRange) {
+    DEBUG_ASSERT(m_frameIndexRange.length() >= 0);


I am a little concerned about these < 0 checks on signed ints. Their overflow is technically UB so I don't know how much the compiler might optimize this in other parts of the code. Also if we ever make SINT unsigned, all these checks will trigger warnings.
Because of that, instead of checking whether the value is negative we might instead want to check whether the value is smaller than the smallest possible buffer size, wdyt?

What do you mean with "technically UB"?

This basically asserts the trivial condition here:

mixxx/src/util/indexrange.h

Line 68 in 4a51c39

return (start() <= end()) ? (end() - start()) : (start() - end());

My guess is that the I originally mean to assert that the range is not backward like in AudioSource::initFrameIndexRangeOnce()
So I think it is reasonable to replace it with a check that range is not backward.

What do you mean with "technically UB"?

I mean integer over-/underflow is only defined behavior on unsigned integral types. So there is no reliable way of detecting them just by inspecting the value (strictly theoretically speaking). The only way would be to check after every arithmetic operation (feasible by using an abstraction such as SafeInt).

But that's just my theoretical two cents on the matter and not a change I propose for this PR.

My guess is that the I originally mean to assert that the range is not backward like in AudioSource::initFrameIndexRangeOnce()

I apologize, the location of the comment was chosen suboptimally. The concern rather applies to the changes in the Analyzer* classes such as here:

ae94ca7#diff-d76237758c9ef4a039bb906e468962e074a9464f40a0219a273d4263523ed88fR47

The link cites AnalyzerBeats::initialize()

if (frameLength <= 0) { return false; }

There is noting wrong with checking the the input frameLength here.

So there is no reliable way of detecting them just by inspecting the value (strictly theoretically speaking).

This is the case for signed AND unsigned values, because the overflowed value can be a valid value in an expected range in both cases. So we are here on the same page IMHO.

The undefined signed int overflow is fortunately only true for C++ itself. The int overflow behavior is well defined by our current target architectures. I know there are some exotic target architectures out there, where this is not the case, but they will never be a Mixxx build target. I am also in doubt that Qt will run on them.
Can we put a statement in our style guide or implement a unit test for in overflow. Than we can cite it next time.

There is noting wrong with checking the the input frameLength here.

Yes, in this particular case its fine since we're checking the length of something small, but we absolutely can't be doing that for detecting overflow.

This is the case for signed AND unsigned values, because the overflowed value can be a valid value in an expected range in both cases. So we are here on the same page IMHO.

Well since this is a count of samples, we expect negative values to be unexpected right? and that's why we're checking for <= 0.

The undefined signed int overflow is fortunately only true for C++ itself. The int overflow behavior is well defined by our current target architectures. I know there are some exotic target architectures out there, where this is not the case, but they will never be a Mixxx build target. I am also in doubt that Qt will run on them.

That does not matter. Its UB in C++ and thus compilers will make use of that for optimization. Here are two examples where the compiler optimizes based on the non-existence of overflow: https://godbolt.org/z/zezfcxWxE

I realize that I'm complaining about an issue that does not apply to this PR, so we can stop here now if you prefer to. Just to summarize, checking whether integer overflow occurred by inspecting a value (as it looks like on first sight in a5ddb48 judging by the commit message) will not work. Can we agree on that?

Can we put a statement in our style guide or implement a unit test for in overflow.

Whats the statement supposed to be? "Don't rely on undefined behavior"?

Just to summarize, checking whether integer overflow occurred by inspecting a value (as it looks like on first sight in a5ddb48 judging by the commit message) will not work. Can we agree on that?

Yes.

Whats the statement supposed to be? "Don't rely on undefined behavior"?

Something like that:

Note: Even though the encoding of signed integer is not defined by the C++ standard, the two's complement representation of signed numbers is enforced by the Qt library: https://github.com/qt/qtbase/blame/6.5/src/corelib/global/qtypes.cpp#L362
This means you can rely that for instance an int8_t of 127 will become -128 after increment by one. There is no need to consider other singed encodings.

Note: Even though the encoding of signed integer is not defined by the C++ standard, the two's complement representation of signed numbers is enforced by the Qt library: https://github.com/qt/qtbase/blame/6.5/src/corelib/global/qtypes.cpp#L362

The linked static assert does not check whether integer overflow is valid. It just checks the boundaries of int and infers that they're encoded in two's complement. It says nothing about overflowing behavior!!

Ah right. Here is the original commit qt/qtbase@34ae919
The overflow behavior is still undefined but the possible options are narrowed. Since we are at C++20 now and the undefined option is the fastest. There is nothing to do.

src/test/analyserwaveformtest.cpp

Swiftb0y · 2023-08-09T13:08:54Z

src/engine/cachingreader/cachingreader.h

@@ -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);


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

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

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

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.

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

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.

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

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.

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.

Swiftb0y · 2023-08-09T13:31:00Z

Tested with a 10 hour mp3 and it works.

src/analyzer/analyzerbeats.h

src/analyzer/analyzerebur128.h

src/test/analyserwaveformtest.cpp

Swiftb0y

LGTM. We can finish the unresolved discussion points later as they're not strictly relevant to this PR anymore.

daschuer added 5 commits April 25, 2023 08:04

Assert that we have no integer overflow in the AudioSource.

a5ddb48

This allows 13h31:35.8 length @ 44100 Hz on 32 bit targets and 3186904 years on 64 bit

Analyzer: Work with SINT frameLength and make sure it is not multipli…

ae94ca7

…ed by the number of channels in the using code.

Pass number of track samples as doubel via trackLoaded to work around…

7490535

… an integer overflow.

Fix int overflow in LoopingControl

504f5ba

Fix integer overflow in waveform classes

c1a2f61

github-actions bot added the code quality label Apr 25, 2023

daschuer mentioned this pull request Apr 25, 2023

Mixxx doesn't play very long track (9h) #11504

Closed

daschuer added this to the 2.3.6 milestone May 7, 2023

daschuer added 2 commits May 22, 2023 14:29

Merge remote-tracking branch 'upstream/2.3' into gh11504

5aa00fb

# Conflicts: # src/test/analyserwaveformtest.cpp

Merge remote-tracking branch 'upstream/2.3' into gh11504

017aca6

ronso0 linked an issue Jun 18, 2023 that may be closed by this pull request

Mixxx doesn't play very long track (9h) #11504

Closed

JoergAtGithub added the needs review label Aug 9, 2023

Swiftb0y requested changes Aug 9, 2023

View reviewed changes

Holzhaus reviewed Aug 9, 2023

View reviewed changes

src/analyzer/analyzerbeats.h Show resolved Hide resolved

src/analyzer/analyzerebur128.h Outdated Show resolved Hide resolved

daschuer added 3 commits August 9, 2023 23:19

Fix assertion against backward ranges

4d227d0

Introduce kCanarySampleRate

7cc5df0

Replace tio by pTrack

f07fe40

daschuer force-pushed the gh11504 branch from 8395d40 to f07fe40 Compare August 10, 2023 12:13

Swiftb0y reviewed Aug 10, 2023

View reviewed changes

src/test/analyserwaveformtest.cpp Outdated Show resolved Hide resolved

Replace kCanarySampleRate with m_pTrack->getSampleRate()

2a3436e

daschuer force-pushed the gh11504 branch from befead0 to 2a3436e Compare August 11, 2023 06:14

Swiftb0y approved these changes Aug 11, 2023

View reviewed changes

Swiftb0y merged commit 48879d7 into mixxxdj:2.3 Aug 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow playing tracks longer than 6 h #11511

Allow playing tracks longer than 6 h #11511

daschuer commented Apr 25, 2023

daschuer commented May 6, 2023

daschuer commented Jun 1, 2023

daschuer commented Aug 9, 2023

Swiftb0y Aug 9, 2023

daschuer Aug 9, 2023

Swiftb0y Aug 9, 2023 •

edited

Loading

daschuer Aug 10, 2023

Swiftb0y Aug 10, 2023

Swiftb0y Aug 10, 2023

Swiftb0y Aug 10, 2023

daschuer Aug 10, 2023 •

edited

Loading

Swiftb0y Aug 10, 2023

daschuer Aug 11, 2023 •

edited

Loading

Swiftb0y Aug 9, 2023

daschuer Aug 10, 2023 •

edited

Loading

Swiftb0y Aug 10, 2023

daschuer Aug 10, 2023 •

edited

Loading

Swiftb0y Aug 10, 2023

daschuer Aug 10, 2023

Swiftb0y Aug 10, 2023

daschuer Aug 10, 2023

Swiftb0y Aug 10, 2023

Swiftb0y commented Aug 9, 2023

Swiftb0y left a comment

Allow playing tracks longer than 6 h #11511

Allow playing tracks longer than 6 h #11511

Conversation

daschuer commented Apr 25, 2023

daschuer commented May 6, 2023

daschuer commented Jun 1, 2023

daschuer commented Aug 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y commented Aug 9, 2023

Swiftb0y left a comment

Choose a reason for hiding this comment

Swiftb0y Aug 9, 2023 •

edited

Loading

daschuer Aug 10, 2023 •

edited

Loading

daschuer Aug 11, 2023 •

edited

Loading

daschuer Aug 10, 2023 •

edited

Loading

daschuer Aug 10, 2023 •

edited

Loading