AudioSource v2 API #1317

uklotzde · 2017-07-22T09:37:23Z

The new and simplified API for AudioSources. New sources only need to implement a single function for decoding audio data!! The index and range calculations in various implementations can still be tweaked and optimized, but at least the API is clean and hopefully stable.

All existing sources have been migrated and tested extensively. With only a single exception: SoundSourceCoreAudio. This one still uses an adapter that I have written while gradually migrating all implementations from the v1 API. Until Apple decides to provide cross-platform development VMs instead of forcing everyone to buy their hardware, I'm not able to finish this task myself. But the adapter works flawlessly, so we don't need to migrate this remaining implementation now.

Please also note that the new seekBoundaries() tests I added cause failures for SoundSourceFFmpeg and is therefore disabled for this source -> OpenAudioSourceMode::DisableFFmpeg. I'm not familiar with the internals of the implementation and gave up to repair it ;)

I also found and fixed a flaw in the CachingReader framework: When skipping randomly through the track the next frames might not have been decoded when a read request is queued. Instead of silently delivering a buffer filled with silence as before it now returns an empty buffer to signal that no data is available, yet. The caller is able to decide when to retry or to continue instantly without any data. You will notice some debug logs whenever this happens, just skip through some track while playing it.

The original idea and motivation has been sketched here: https://blueprints.launchpad.net/mixxx/+spec/audiosourcev2

[Update 2017-10-04] I was able to streamline the API even further. There is no need to explicitly "skip" through an audio stream in the public API. This functionality is only useful for testing. During tests for sample accurate decoding you need to explicitly control this behavior yourself instead of relying on some hidden implementation that might decide to seek whenever it thinks to do so! Otherwise you are comparing apples with apples ;)

daschuer · 2017-07-24T07:06:13Z

src/util/indexrange.h

+        Empty,
+        Forward,
+        Backward,
+    };


I am not entirely through with my reading...
I just wonder if we need a range class that supports forward and backward in the same time. Will the using code simpler or not save if we have dedicated classes or sub classes only supporting one direction?

I have actually written an unidirectional version first, but changed it to a symmetric, bidirectional version that is more consistent and universally usable.

I did not find anything suitable in some library, otherwise I would have used it.

daschuer · 2017-07-24T07:12:16Z

src/util/indexrange.h

+    // Splits this range into two adjacent parts by slicing off
+    // and returning a range of given length and same direction
+    // from the head side. The given length must not exceed
+    // the length of this range.


this comment is not clear enough. What will be returned what will remain in the class? Would it be better to just return both parts, and leave the original unchanged?

Those mutable operations sacrifice safety for usability. In languages like Rust or Scala that support pattern matching for variable bindings I would have done it the way you proposed.

daschuer · 2017-07-26T17:51:05Z

src/sources/legacyaudiosourceadapter.cpp

+
+    SINT outputSampleOffset = 0;
+    if (seekFrameIndex > readableFrames.head()) {
+        const auto unreadableFrames = readableFrames.splitHead(seekFrameIndex - readableFrames.head());


It was not instantly clear for me, what this line does.
Head is in one case a single position, and in an other case a new range. the auto keyword hides this fact even more.

Can we move to head = range and top and bottom = a single index? O really other better words? Than we have the top of head.
This can than become for example cutOutHead(seekFrameIndex - readableFrames.top())

~~I will change head() and tail() to front() and back() like in std::vector~~ not so good idea

I adopted start() and end() from Rust's Range.

daschuer · 2017-07-26T17:54:20Z

src/sources/legacyaudiosourceadapter.cpp

+
+    SINT outputSampleOffset = 0;
+    if (seekFrameIndex > readableFrames.head()) {
+        const auto unreadableFrames = readableFrames.splitHead(seekFrameIndex - readableFrames.head());


or End of Tail ..

daschuer · 2017-07-26T21:56:14Z

src/util/indexrange.h

    // the length of this range.
-    IndexRange splitHead(SINT headLength);
+    IndexRange splitFront(SINT frontLength);


This can be read as "split the Front part". Ideas: cutOutFront() .. cutFront() .. sliceOutFront() .. sliceOffFront()

daschuer · 2017-07-26T21:59:43Z

src/util/indexrange.h

-    // The opposite boundary (exclusive)
-    SINT tail() const {
+    // The next index beyond this range (exclusive)
+    SINT end() const {
        return second;
    }

    bool empty() const {


isEmpty() empty() can be an imperative.

But even Qt is using the simplified naming for boolean getters.

You are right for getValue() vs. just value()
But that does not apply for empty. Here Qt use IsEmpty() http://doc.qt.io/qt-4.8/qbytearray.html#isEmpty

On the other hand STL uses empty() everywhere and Qt adopts this usage for compatibility in QList for example.

Ok, in that case ;-) ... I will keep quite.

daschuer · 2017-07-26T22:05:08Z

src/util/indexrange.h

    }

-    // Clamps index by this range including both head() and tail()
+    // Clamps index by this range including both start() and end()
    // boundaries.
    SINT clamp(SINT index) const {


it is not clear that the index ins clamped-
clampIndex() .. or .. getNearestInBounds()

daschuer · 2017-07-29T13:58:01Z

src/sources/audiosource.cpp

+        return readableFrames;
+    }
+    DEBUG_ASSERT(readableFrames.start() >= frameIndexRange.start());
+    if (pOutputBuffer) {


this is already checked above

daschuer · 2017-07-29T14:28:17Z

src/sources/audiosource.h

+    // sample values.
+    //
+    // On errors only a sub range of the requested frames might be returned.
+    virtual IndexRange readOrSkipSampleFrames(


After reading the code I understand the function name. But the "OrSkip" part was confusing on the first read.
Now it is obvious that if you pass a null buffer that it skips frames. Isn't the main nature of this function that it seeks to the file and fills the buffer if it can do it?

How about seekAndReadSampleFrames()

daschuer · 2017-07-29T14:33:51Z

src/sources/audiosource.h

+    //
+    // Only that part of the output buffer corresponding to the returned
+    // range is allowed to be modified. All remaining samples in the output
+    // buffer should stay untouched. The samples in the output buffer need


Why we do not zero out the buffer, if the requested range starts earlier than the readable range of the file?

That's the responsibility of the caller. We don't want to make any assumptions what the caller is doing with the results. Moreover we would need to add extra code to each of the implementations to fill the unread parts with zero (that maybe are never used by the caller anyway).

daschuer · 2017-07-29T14:41:11Z

plugins/soundsourcem4a/soundsourcem4a.cpp

+        SampleBuffer::WritableSlice* pOutputBuffer) {
+    auto readableFrames =
+            adjustReadableFrameIndexRangeAndOutputBuffer(
+                    frameIndexRange, pOutputBuffer);


It is confusing, that the output buffer is modified by a pointer but not the frameIndexRange.
Can we modify both by pointer?

the same is also true for readOrSkipSampleFrames. Do we need the original IndexRange passed?

daschuer · 2017-07-29T14:50:29Z

plugins/soundsourcem4a/soundsourcem4a.cpp

+                + (readableFrames.start() / m_framesPerSampleBlock);
+        DEBUG_ASSERT(isValidSampleBlockId(sampleBlockId));
+        if ((readableFrames.start() < m_curFrameIndex) || // seeking backwards?
+                !isValidSampleBlockId(m_curSampleBlockId) || // invalid seek position?


How can that be invalid?

Ah, I see, after starting the decoder. Can you change the comment accordingly?

daschuer · 2017-07-29T15:07:47Z

plugins/soundsourcem4a/soundsourcem4a.cpp

+        const auto precedingFrames =
+                IndexRange::between(m_curFrameIndex, readableFrames.start());
+        if (!precedingFrames.empty()
+                && (precedingFrames != skipSampleFrames(precedingFrames))) {


Is this a recursive call? How is guaranteed that we not reed the prefetched part over and over again. Can't we avoid this recursion?

uklotzde · 2017-07-30T13:59:14Z

@daschuer Inspired by your remarks I will prepare an improved and simplified API:

enum class ReadMode {
    Store, // write/copy decoded sample data into buffer
    Skip,  // discard decoded sample data
};

virtual ReadableSampleFrames readSampleFrames(
    ReadMode readMode,
    WritableSampleFrames sampleFrames) = 0;

Readable-/WritableSampleFrames are simple DTOs that contain both an IndexRange of frames and the corresponding slice of a SampleBuffer for transferring/storing sample data. This results in a single function that covers all our use cases.

uklotzde · 2017-09-16T15:31:26Z

The OS X build on Travis works when switching to Xcode 8.3.

Be-ing · 2017-09-16T15:58:16Z

This doesn't introduce any user facing changes, does it? As far as I understand this is just making the code more maintainable. Does it fix identifiable bugs?

uklotzde · 2017-09-16T17:20:22Z

No new features, just a consolidated and lean API for plugin developers, more tests and lots of internal code rework. Common functionality like bounds checking and post-processing (downsampling to stereo) has been integrated into Mixxx. The decoding code was already pretty stable and well tested, now it should be even safer with only a minor performance degradation.

There actually were some decoding bugs when seeking near boundaries in some implementations. I'm not sure if I fixed all of them with a previous PR that we have split off this branch.

I wouldn't call the CachingReader flaw a bug, it is just undocumented and untested behavior. At least I didn't notice any audible dropouts, neither before nor after the fix and refactoring. We definitely need more tests for both the CachingReader and the engine itself. But those tests are difficult and brittle, because they have to consider timing. I even can't imagine how such tests should look like.

uklotzde · 2017-09-16T17:23:38Z

Too bad that both CI servers struggle with Windows and OS X builds. Travis aborts with a timeout when switching to Xcode 8.3, the build just takes too long.

Be-ing · 2017-09-16T18:06:10Z

With the free options for AppVeyor and Travis we use, they both time out randomly. I'm not sure if switching to Xcode 8.3 slows it down too much.

sblaisot · 2017-09-16T19:53:12Z

there is an issue with appveyor. They updated their NSIS version and our NSIS patching is rejected.
I need to check that and update the patch to restore appveyor CI.

Let me have a look at that.

sblaisot · 2017-09-16T20:21:20Z

please cherry-pick 2e9475b from #1344 to fix appveyor build.

sblaisot · 2017-09-16T23:36:43Z

You can also cherry-pick ac34d82 from #1345 to fix travis-ci mac build

uklotzde · 2017-09-17T08:24:52Z

Rebased on master once again for a clean commit history. Thanks @sblaisot for your fixes 👍 Looks like we are back on track.

uklotzde · 2017-09-17T13:27:37Z

I've taken the chance to quickly resolve another outstanding TODO issue: Apply a 2-pass strategy when opening files to find the best matching decoder.

Be-ing · 2017-09-17T13:51:58Z

I've taken the chance to quickly resolve another outstanding TODO issue: Apply a 2-pass strategy when opening files to find the best matching decoder.

Does this mean we can support ALAC via FFMPEG?

uklotzde · 2017-09-17T14:57:43Z

@Be-ing Yes, ALAC can be played through SoundSourceFFmpeg. Just checked it again with example files from the Linn page. This should already be possible with the current master. SoundSourceFFmpeg supports only 16-bit (24-bit: Unsupported sample format: s32p) while SoundSourceFLAC supports both 16-bit and 24-bit.

Please also note that SoundSourceFFmpeg still has issues when seeking to boundaries, see the disabled test. And the internal caching of audio data seems to need some rework according to strange log messages that appear while decoding.

Be-ing · 2017-10-19T05:02:59Z

It would be really helpful if you wrote a high level overview of how the different classes interact and how it interacts with the mixing engine. This could be comments in a header file or it might be better to add a page to the Developer Guide on the wiki.

uklotzde · 2017-10-19T17:19:27Z

@Be-ing I agree, a short HOWTO write and adding a new SoundSource(Plugin) would be helpful.

I'm not satisfied with the current plugin system which is not properly isolated from the main code base, so a HOWTO about plugins might follow only after fixing this. We should first investigate how other projects handle extensions and plugins and even consider the implications of a future build system like Meson!

Passing sample data to the engine is handled by ReadAheadManager/CachingReader and a different story. These components also need a second round of rework, maybe exchanging some of the brittle hand-written multi-threading code by standard C++ or Boost components ;)

Well, now I ended up with 3 follow-up tasks.

daschuer · 2017-11-19T22:56:47Z

plugins/soundsourcem4a/soundsourcem4a.cpp

            // have actually been decoded
-            m_sampleBuffer.readFifo(decodeBufferCapacity - numberOfSamplesDecoded);
+            m_sampleBuffer.readFromTail(decodeBufferCapacity - numberOfSamplesDecoded);


Did you consider to rename this function to shrinkTail or something? The return value is never used.

uklotzde · 2017-11-22T17:22:15Z

Regarding Readable-/WritableSlice: The code would not get safer by using 2 individual values (pointer + size) as before instead of wrapping them into a shallow wrapper class. The wrapper classes at least prevent that the values are modified. Even worse, individual values can be modified independently of each other! The term slice should indicate that the memory is borrowed and not owned. Well, not in a safe ways as in Rust ;) The different types clearly indicate their purpose, i.e. read from or write into.

daschuer · 2017-11-22T21:59:51Z

Yes, I agree, this approach is nice.

daschuer · 2017-11-23T22:04:34Z

plugins/soundsourcem4a/soundsourcem4a.cpp

+            // Shrink the size of the buffer to the samples that have
+            // actually been decoded, i.e. dropping unneeded samples
+            // from the back of the buffer.
+            m_sampleBuffer.dropFromTail(decodeBufferCapacity - numberOfSamplesDecoded);


Lets name the function to what it does here. It adjusts the readable range to the just written samples
m_sampleBuffer.shrinkToWritten(numberOfSamplesDecoded);
m_sampleBuffer.confirmSamplesWritten(numberOfSamplesDecoded);

daschuer · 2017-11-23T22:28:55Z

OK, I think I am through :-)

writeToTail() grows the readable range. So this function promises that the bytes are readable later.
It would be more logical to grow the radable range after the samples where actually written.
This requires an mandatory extra "confirmWritten()" call or something.

Currently it is silently assumed that we actually write. It is OK for me because it seems the most performance solution, but we should describe this in the writeToTail() comments.

The rest LGTM

uklotzde · 2017-11-24T17:32:40Z

I'm against introducing a confirmWritten(), because ReadAheadSampleBuffer class is NOT intended to be used with concurrent readers and writers as already mentioned in the class comment. I will try to modify the API to avoid this impression. Those restrictions should become obvious even by reading just the code without looking at the comments.

uklotzde · 2017-11-24T17:36:46Z

I was struggeling with the term "shrink" and have to think about how to align the operation names in both SampleBuffer and IndexRange. It should be obvious if we are "shrinking" just the range or actually the buffer. It's confusing if the same term is used for different purposes, so we need to decide which terms to use for ranges and buffers. Those terms should differ.

uklotzde · 2017-11-24T17:40:00Z

ReadAheadSampleBuffer is the combination of both a SampleBuffer and an IndexRange. So we need to carefully choose the corresponding operation names. There is also some confusion around "capacity"/"size"/"length". I will try to disambiguate the usage.

daschuer · 2017-11-24T18:03:16Z

That was my final thought. I think we can stop here to overoptimize it and put the our final thoughts to the comments.

This also avoids confusion between "slice" and "size" which sound very similar.

Currently not used, but both should work in the same way.

uklotzde · 2017-11-25T16:32:10Z

I've improved the documentation and renamed the member functions of ReadAheadSampleBuffer. Should be much more consistent and comprehensible than before.

All changes divided into single commits to illustrate what I actually changed.

daschuer · 2017-11-25T21:51:18Z

LGTM Really great now. Thank you very much!

JosepMaJAZ · 2017-12-06T15:29:53Z

Now that the audiosource v2 is in master, I found a bug that happens with M4A files under Windows causing a program crash (I should have helped testing it on Windows).

Concretely, a stack overflow happens because SoundSourceMediaFoundation::readSampleFramesClamped always calls SoundSourceMediaFoundation::seekSampleFrame, but the implementation of seekSampleFrame calls readSampleFramesClamped in some cases, causing the aforementioned stack overflow.

The problem happens when loading one track, playing and then seeking to a later part of the track (like clicking with the mouse to go to the half of the track).

```

soundsourcemediafoundation.dll!mixxx::SoundSourceMediaFoundation::readSampleFramesClamped(mixxx::WritableSampleFrames writableSampleFrames) Línea 252 C++
soundsourcemediafoundation.dll!mixxx::SoundSourceMediaFoundation::seekSampleFrame(__int64 frameIndex) Línea 209 C++

soundsourcemediafoundation.dll!mixxx::SoundSourceMediaFoundation::readSampleFramesClamped(mixxx::WritableSampleFrames writableSampleFrames) Línea 252 C++
soundsourcemediafoundation.dll!mixxx::SoundSourceMediaFoundation::seekSampleFrame(__int64 frameIndex) Línea 209 C++
soundsourcemediafoundation.dll!mixxx::SoundSourceMediaFoundation::readSampleFramesClamped(mixxx::WritableSampleFrames writableSampleFrames) Línea 252 C++
mixxx.exe!mixxx::AudioSourceTrackProxy::readSampleFramesClamped(mixxx::WritableSampleFrames sampleFrames) Línea 48 C++
mixxx.exe!mixxx::AudioSourceStereoProxy::readSampleFramesClamped(mixxx::WritableSampleFrames sampleFrames) Línea 60 C++
mixxx.exe!CachingReaderChunk::bufferSampleFrames(const std::shared_ptrmixxx::AudioSource & pAudioSource, mixxx::SampleBuffer::WritableSlice tempOutputBuffer) Línea 70 C++
mixxx.exe!CachingReaderWorker::processReadRequest(const CachingReaderChunkReadRequest & request) Línea 53 C++
mixxx.exe!CachingReaderWorker::run() Línea 110 C++

uklotzde · 2017-12-06T15:48:47Z

Thanks for reporting!!! I'll fix this immediately.

uklotzde · 2017-12-06T17:19:34Z

Work in progress: #1406

First I want to have a failing regression test in place to prevent this happening again.

daschuer reviewed Jul 24, 2017

View reviewed changes

daschuer reviewed Jul 26, 2017

View reviewed changes

daschuer reviewed Jul 29, 2017

View reviewed changes

uklotzde changed the title ~~[WiP] AudioSource v2 API~~ AudioSource v2 API Sep 16, 2017

uklotzde mentioned this pull request Sep 28, 2017

Universal SoundSource for FFmpeg 4.x #1356

Merged

4 tasks

daschuer reviewed Nov 19, 2017

View reviewed changes

uklotzde added 4 commits November 20, 2017 08:00

Replace readFromTail() with dropFromTail()

b7566c3

Reorder #include directives

a257340

Re-add missing #include directive

407b7f2

Merge branch 'master' into audiosourcev2

9382025

uklotzde mentioned this pull request Nov 23, 2017

Fix plugin build on Fedora 27 with SCons 2.5.1 #1392

Merged

Merge branch 'master' into audiosourcev2

755d55b

daschuer reviewed Nov 23, 2017

View reviewed changes

uklotzde added 7 commits November 25, 2017 16:02

Use "length" for both slices and ranges

ee517e3

This also avoids confusion between "slice" and "size" which sound very similar.

Add a utility function to reduce nesting of member function calls

8822348

Align function signatures of length() and data()

66eaa75

Currently not used, but both should work in the same way.

Clarify how ReadAheadSampleBuffer is intended to be used

70e29d5

Rename function: writeToTail() -> growForWriting()

665cf61

Rename function: dropFromTail() -> shrinkAfterWriting()

f63e2fa

Rename function: readFromHead() -> shrinkForReading()

d6765cf

daschuer merged commit 80f3ea5 into mixxxdj:master Nov 25, 2017

uklotzde deleted the audiosourcev2 branch December 10, 2017 14:19

mixxxbot mentioned this pull request Aug 22, 2022

Track metadata: Add field for "Subtitle" #8279

Open

AudioSource v2 API #1317

AudioSource v2 API #1317

Conversation

uklotzde commented Jul 22, 2017 • 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

uklotzde Jul 26, 2017 • 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

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

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

Choose a reason for hiding this comment

uklotzde commented Jul 30, 2017

uklotzde commented Sep 16, 2017

Be-ing commented Sep 16, 2017

uklotzde commented Sep 16, 2017 • edited Loading

uklotzde commented Sep 16, 2017

Be-ing commented Sep 16, 2017 • edited Loading

sblaisot commented Sep 16, 2017

sblaisot commented Sep 16, 2017

sblaisot commented Sep 16, 2017

uklotzde commented Sep 17, 2017

uklotzde commented Sep 17, 2017

Be-ing commented Sep 17, 2017

uklotzde commented Sep 17, 2017

Be-ing commented Oct 19, 2017 • edited Loading

uklotzde commented Oct 19, 2017 • edited Loading

Choose a reason for hiding this comment

uklotzde commented Nov 22, 2017 • edited Loading

daschuer commented Nov 22, 2017

Choose a reason for hiding this comment

daschuer commented Nov 23, 2017

uklotzde commented Nov 24, 2017

uklotzde commented Nov 24, 2017

uklotzde commented Nov 24, 2017

daschuer commented Nov 24, 2017

uklotzde commented Nov 25, 2017

daschuer commented Nov 25, 2017

JosepMaJAZ commented Dec 6, 2017

uklotzde commented Dec 6, 2017

uklotzde commented Dec 6, 2017

uklotzde commented Jul 22, 2017 •

edited

Loading

uklotzde Jul 26, 2017 •

edited

Loading

uklotzde commented Sep 16, 2017 •

edited

Loading

Be-ing commented Sep 16, 2017 •

edited

Loading

Be-ing commented Oct 19, 2017 •

edited

Loading

uklotzde commented Oct 19, 2017 •

edited

Loading

uklotzde commented Nov 22, 2017 •

edited

Loading