Solve recurrent calls to captureSamples bug

[krunal-shah]

Solves #11616 using the simple proposed solution at doc (suggested in the issue).

[welcome]

Thank you for submitting a pull request! If this is your first PR, make sure to add yourself to AUTHORS.

[kripken]

Thanks @krunal-shah ! Good find, and very good explanation.

I think this is correct, but don't know this code very well, leaving for @juj who wrote it.

[juj]

I was not actually the author of the code, @jpernst was (though I did review the PR #5367 where it landed.

To me this fix does not look proper. The captured samples are written to a ring buffer, with ring buffer head being tracked as the c.capturePlayhead variable. The variable c.bufferFrameCapacity is the ring buffer total slot capacity, and c.capturedFrameCount specifies how many slots are in use in the ring buffer. The used slots are indices c.capturePlayhead-1, c.capturePlayhead-2, ..., c.capturePlayhead-c.capturedFrameCount (mod c.bufferFrameCapacity).

To me it looks like the whole read of the capture frame buffer is wrong, and the proper fix would be the following:

--- a/src/library_openal.js
+++ b/src/library_openal.js
@@ -2009,10 +2009,12 @@ var LibraryOpenAL = {
     var dstfreq = c.requestedSampleRate;
     var srcfreq = c.audioCtx.sampleRate;
 
+    var capturePlaytail = (c.capturePlayhead + c.bufferFrameCapacity - c.capturedFrameCount) % c.bufferFrameCapacity;
+
     if (srcfreq == dstfreq) {
       for (var i = 0, frame_i = 0; frame_i < requestedFrameCount; ++frame_i) {
         for (var chan = 0; chan < c.buffers.length; ++chan, ++i) {
-          var src_i = (frame_i + c.capturePlayhead) % c.capturedFrameCount;
+          var src_i = (frame_i + c.capturePlaytail) % c.bufferFrameCapacity;
           setSample(i, c.buffers[chan][src_i]);
         }
       }
@@ -2031,8 +2033,12 @@ var LibraryOpenAL = {
       for (var i = 0, frame_i = 0; frame_i < requestedFrameCount; ++frame_i) {
 
         var t = frame_i / dstfreq; // Most exact time for the current output sample
-        var src_i = (Math.floor(t*srcfreq) + c.capturePlayhead) % c.capturedFrameCount;
-        var src_next_i = (src_i+1) % c.capturedFrameCount;
+        var src_i = (Math.floor(t*srcfreq) + c.capturePlaytail) % c.bufferFrameCapacity;
+        var src_next_i = (src_i+1) % c.bufferFrameCapacity;
+
+        // Avoid out of bounds read on the sample ring buffer at the last captured sample frame.
+        if (src_next_i == c.capturePlayhead) src_next_i = src_i;
+
         var between = t*srcfreq - src_i; //(t - src_i/srcfreq) / ((src_i+1)/srcfreq - src_i/srcfreq);
 
         for (var chan = 0; chan < c.buffers.length; ++chan, ++i) {
@@ -2043,10 +2049,7 @@ var LibraryOpenAL = {
       }
     }
 
-    // Spec doesn't say if alcCaptureSamples() must zero the number
-    // of available captured sample-frames, but not only would it
-    // be insane not to do, OpenAL-Soft happens to do that as well.
-    c.capturedFrameCount = 0;
+    c.capturedFrameCount -= requestedFrameCount;
   },

In particular, in the current code in the tree, when the ring buffer is being written to, the ring buffer is (properly) accessed % c.bufferFrameCapacity, but when reading, the ring buffer is (incorrectly) accessed as % c.capturedFrameCount, which does not make sense - and happened to only work when there is no wraparound present in the ring buffer.

@krunal-shah Would you be able to test the above patch instead, and see if that fixes the issue?

[krunal-shah]

Hi @juj, I have the following points:

1. I agree that the current implementation of reading is not ideal, however, it is correct for the assumption mentioned

     // Spec doesn't say if alcCaptureSamples() must zero the number
     // of available captured sample-frames, but not only would it
     // be insane not to do, OpenAL-Soft happens to do that as well.
     c.capturedFrameCount = 0;

Under this assumption, you want to reset capturePlayhead and capturedFrameCount after every call to alcCaptureSamples which make the logic of % c.capturedFrameCount correct. I have explained it briefly in my attached doc.

Since I had assumed that we will be going forward with this assumption, I made the suggestion of resetting capturePlayhead (which was missed somehow in the existing implementation) as well as the solution to the bug.

2. If we decide to forego the assumption mentioned in the comments, we can go ahead with your suggested change. I tried running your change but it did not run out of the box. I did some debugging and suggest the following changes to your change, which work on my sample. Note that the diff line numbers are not precise since I made changes to my local sample.js file and not the repository.

--- a/src/library_openal.js
+++ b/src/library_openal.js
@@ -2009,10 +2009,12 @@ var LibraryOpenAL = {
     var dstfreq = c.requestedSampleRate;
     var srcfreq = c.audioCtx.sampleRate;
 
     var capturePlaytail = (c.capturePlayhead + c.bufferFrameCapacity - c.capturedFrameCount) % c.bufferFrameCapacity;

     if (srcfreq == dstfreq) {
       for (var i = 0, frame_i = 0; frame_i < requestedFrameCount; ++frame_i) {
         for (var chan = 0; chan < c.buffers.length; ++chan, ++i) {
-          var src_i = (frame_i + c.capturePlaytail) % c.bufferFrameCapacity;
+          var src_i = (frame_i + capturePlaytail) % c.bufferFrameCapacity;
           setSample(i, c.buffers[chan][src_i]);
         }
       }
@@ -2031,8 +2033,12 @@ var LibraryOpenAL = {
       for (var i = 0, frame_i = 0; frame_i < requestedFrameCount; ++frame_i) {
 
         var t = frame_i / dstfreq; // Most exact time for the current output sample
-        var src_i = (Math.floor(t*srcfreq) + c.capturePlaytail) % c.bufferFrameCapacity;
+        var src_i = (Math.floor(t*srcfreq) + capturePlaytail) % c.bufferFrameCapacity;
         var src_next_i = (src_i+1) % c.bufferFrameCapacity;
 
         // Avoid out of bounds read on the sample ring buffer at the last captured sample frame.
         if (src_next_i == c.capturePlayhead) src_next_i = src_i;
 
-        var between = t*srcfreq - src_i; //(t - src_i/srcfreq) / ((src_i+1)/srcfreq - src_i/srcfreq);
+        var between = t*srcfreq - Math.floor(t*srcfreq);
 
         for (var chan = 0; chan < c.buffers.length; ++chan, ++i) {
@@ -2043,10 +2049,7 @@ var LibraryOpenAL = {
       }
     }

-    c.capturedFrameCount -= requestedFrameCount;
+    c.capturedFrameCount += c.bufferFrameCapacity - Math.floor(requestedFrameCount * (srcfreq/dstfreq));
+    c.capturedFrameCount %= c.bufferFrameCapacity;
   },

Let me know how you want to proceed and I can make the necessary change and update the commit.

[krunal-shah]

Hi @juj, just following up if you have any update on the previous comment.

Thank you.

[kripken]

ping @juj, looks like this may have been missed?

[krunal-shah]

Hi @juj, could you kindly take a look at the above updates and let me know your thoughts?

[stale]

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

[sbc100]

@krunal-shah @juj, can we look into getting this landed?

@krunal-shah can you upload the updated PR, with the suggestions from @juj. If that version also fixes your use case I think its good to land.