AAudio: don't count the time of stopped stream and replace stop request with pause. #789

jhlin · 2024-06-21T03:40:23Z

If a stream has previously been stopped and then started, the elapsed time in between should not be included when calculating the position.

Also, AAudioStream_requestStop() doesn't behave as expected.

padenot · 2024-06-21T16:20:58Z

src/cubeb_aaudio.cpp

+  }
+
+private:
+  std::mutex mutex;


Unfortunately, we can't use locks here because we're on a real-time thread. Thankfully, we only have a few arithmetic operations to do here so we can use std::atomic.

Thanks a lot for the review! I've updated the logic to use std::atomic rather than std::mutex. Please let me know if any issues.

jhlin · 2024-06-26T20:15:01Z

In the latest revision, the synchronization logic was removed—access to states/values only in user-called functions.

jhlin · 2024-06-26T20:52:22Z

Not sure why the test failed on Windows. The patches only affect Android behavior.

jhlin · 2024-06-27T16:47:10Z

@Pehrsons @padenot suggested I seek your help for review. Could you please help? Please let me know if you have any questions. Thanks!

Pehrsons · 2024-06-27T20:19:19Z

The windows failure is probably an intermittent. It succeeded after a re-run.

Just looking fundamentally at this I am on board with fixing the interpolation (we also have other issues in the area, see https://bugzilla.mozilla.org/show_bug.cgi?id=1902263).
But why do we want to pause rather than stop? Is the concern that AAudioStream_requestStop flushes, so throws away some data that we lose track of?

I see an issue:
If the client calls AAudioStream_requestPause via cubeb_stream_stop, then after some time and some seeks (i.e. the next audio frame the client will write is not immediately after the last frame it wrote) have passed, it calls AAudioStream_requestStart via cubeb_stream_start, wouldn't the AAudioStream initially play old audio? That seems wrong.

That the flushed data doesn't reach the user's ears doesn't seem like a problem. It's short enough so they're unlikely to notice. For the sake of cubeb_stream_get_position though, we should probably treat the flushed audio as played.

Let me know if I understood this right!

jhlin · 2024-06-27T22:10:21Z

Thanks a lot for the review!

The windows failure is probably an intermittent. It succeeded after a re-run.

Just looking fundamentally at this I am on board with fixing the interpolation (we also have other issues in the area, see https://bugzilla.mozilla.org/show_bug.cgi?id=1902263). But why do we want to pause rather than stop? Is the concern that AAudioStream_requestStop flushes, so throws away some data that we lose track of?

Exactly, the already buffered data in the previous data callback will be flushed by AAudioStream_requestStop() but not AAudioStream_requestPause().

I see an issue: If the client calls AAudioStream_requestPause via cubeb_stream_stop, then after some time and some seeks (i.e. the next audio frame the client will write is not immediately after the last frame it wrote) have passed, it calls AAudioStream_requestStart via cubeb_stream_start, wouldn't the AAudioStream initially play old audio? That seems wrong.

Luckily, seeking destroys then initializes audio stream, so old data isn't played.

That the flushed data doesn't reach the user's ears doesn't seem like a problem. It's short enough so they're unlikely to notice. For the sake of cubeb_stream_get_position though, we should probably treat the flushed audio as played.

TBH, I never noticed skipped audio. I only found that some video frames were dropped when resuming a paused video because the audio clock advanced further than expected. This change didn't eliminate the issue completely in my tests, but it seems mitigated.

Besides, this behavior seems inconsistent with the other backends.

Let me know if I understood this right!

Pehrsons · 2024-06-28T09:32:45Z

Luckily, seeking destroys then initializes audio stream, so old data isn't played.

Ok, so that's true for Gecko but might not be for other clients.

TBH, I never noticed skipped audio. I only found that some video frames were dropped when resuming a paused video because the audio clock advanced further than expected. This change didn't eliminate the issue completely in my tests, but it seems mitigated.

Makes sense that some dropped video frames is more noticable.

Besides, this behavior seems inconsistent with the other backends.

Out of curiosity I went through the common ones as I'm not familiar with all:

CoreAudio is unclear.
Pulse corks, which doesn't flush, unless the data cb already told it to drain, then it waits for the drain.
Wasapi doesn't seem to flush.

Paul confirms that cubeb_stream_stop is more for pausing also, let me take a deeper look in a bit.

Pehrsons

Some early feedback. I'll have to look closer at the interpolator later.

Pehrsons · 2024-06-28T09:39:45Z

src/cubeb_aaudio.cpp

-      assert(!istate || istate == AAUDIO_STREAM_STATE_STOPPING ||
-             istate == AAUDIO_STREAM_STATE_STOPPED);
-      assert(!ostate || ostate == AAUDIO_STREAM_STATE_STOPPING ||
-             ostate == AAUDIO_STREAM_STATE_STOPPED);
-      if ((!istate || istate == AAUDIO_STREAM_STATE_STOPPED) &&
-          (!ostate || ostate == AAUDIO_STREAM_STATE_STOPPED)) {


It seems to me stopping and stopped should be considered invalid or handled.

Sorry but I don't follow. In the patch, only AAUDIO_STREAM_STATE_{PAUSING|PAUSED} are allowed, and AAUDIO_STREAM_STATE_{STOPPING|STOPPED} are considered invalid. Am I missing something?

Re invalid I was thinking higher up under handle invalid stream states. Maybe we cannot do that because we still call AAudioStream_requestStop in some other cases?

Thanks for the explanation. Yes, since AAudioStream_requestStop() is called in case stream_state::DRAINING, I don't think AAUDIO_STREAM_STATE_{STOPPING|STOPPED} need to be listed in the handle invalid stream states conditions.

Pehrsons · 2024-06-28T09:51:40Z

src/cubeb_aaudio.cpp

+  // index == 0 && get<0> == 0: stream has no stop record
+  // index == 0 && get<0> > 0: stream stopped
+  // index == 1: stream stopped and then started
+  std::variant<uint64_t, uint64_t> time;


If these uint64_ts could be distinct types so you can do something like std::get<StopTimestamp>(time), I think this would be easier to read.

Maybe even

struct None {}; struct Stopped { uint64_t time = {}; }; struct StartedAfterStopped { uint64_t time = {}; }; struct Running {}; std::variant<None, Stopped, StartedAfterStopped, Running> state;

where both None and Running correspond to your current std::get<0>(time) == 0 (but with explicit semantics), and changing from index to type getters like if (auto* sas = std::get_if<StartedAfterStopped>(state)) {...}

Good idea. I will do it in the next revision. Thanks!

Pehrsons · 2024-06-28T09:54:57Z

src/cubeb_aaudio.cpp

@@ -135,6 +136,66 @@ struct AAudioTimingInfo {
  uint32_t input_latency;
 };

+class position_interpolator {


Please give a high-level overview of what this class' intended behavior is. Expected inputs and outputs and that sort of thing.

It seems to me we are really tracking the time spent in the starting/started states with a stale callback timestamp.

Please give a high-level overview of what this class' intended behavior is. Expected inputs and outputs and that sort of thing.

I will add comments for the class in the next revision.

It seems to me we are really tracking the time spent in the starting/started states with a stale callback timestamp.

Agree. Calling AAudioStream_requestStop() does generate a new callback invocation immediately after starting to get more data, but since it flushes previously buffered data, the position always jumps ahead, and video frames get dropped. I hope calling AAudioStream_requestPause() can make it less serious.

Pehrsons · 2024-06-28T09:55:27Z

src/cubeb_aaudio.cpp

+    assert(time.index() == 0 && (std::get<0>(time) > 0 /* stopped */ ||
+                                 callback_timestamp == 0 /* inited */));
+    if (std::get<0>(time) > 0) {
+      // Has been stopped: calculate elipsed time.


s/elipsed/elapsed/
(and below)

Pehrsons

The logic seems to check out to me, thanks!
I left some ideas for making it easier to read. Do with them as you wish.
I also left a comment about AAUDIO_STREAM_STATE_{STOPPING|STOPPED}. Please at a minimum address this one before landing.

Pehrsons · 2024-06-28T14:20:08Z

src/cubeb_aaudio.cpp

+    assert(time.index() == 1 || (time.index() == 0 && std::get<0>(time) == 0));
+    uint64_t prev = 0;
+    if (time.index() == 1) {
+      // Offset timestamp by the last elasped time if stream stops again before


s/elasped/elapsed/

Pehrsons · 2024-06-28T15:57:05Z

src/cubeb_aaudio.cpp

+  // index == 0 && get<0> == 0: stream has no stop record
+  // index == 0 && get<0> > 0: stream stopped
+  // index == 1: stream stopped and then started
+  std::variant<uint64_t, uint64_t> time;


Maybe even

struct None {}; struct Stopped { uint64_t time = {}; }; struct StartedAfterStopped { uint64_t time = {}; }; struct Running {}; std::variant<None, Stopped, StartedAfterStopped, Running> state;

where both None and Running correspond to your current std::get<0>(time) == 0 (but with explicit semantics), and changing from index to type getters like if (auto* sas = std::get_if<StartedAfterStopped>(state)) {...}

Pehrsons · 2024-06-28T15:59:52Z

src/cubeb_aaudio.cpp

@@ -135,6 +136,66 @@ struct AAudioTimingInfo {
  uint32_t input_latency;
 };

+class position_interpolator {


It seems to me we are really tracking the time spent in the starting/started states with a stale callback timestamp.

aaudio_stream_get_position() calculates position using elapsed time since last callback, including the time during stream is stopped. To interpolate correctly, record the stop time and use it in calculation.

According to the document, AAudioStream_requestStop(): "The stream will stop after all of the data currently buffered has been played." AAudioStream_requestPause(): "Pausing a stream will freeze the data flow but not flush any buffers."

jhlin force-pushed the aaudio-get-postion branch from c912469 to f356a07 Compare June 21, 2024 03:53

padenot requested changes Jun 21, 2024

View reviewed changes

jhlin force-pushed the aaudio-get-postion branch from f356a07 to b93825d Compare June 22, 2024 01:22

jhlin requested a review from padenot June 22, 2024 01:39

jhlin force-pushed the aaudio-get-postion branch from b93825d to e75d3a6 Compare June 26, 2024 20:10

Pehrsons reviewed Jun 28, 2024

View reviewed changes

Pehrsons approved these changes Jun 28, 2024

View reviewed changes

jhlin added 2 commits July 3, 2024 16:01

[AAudio] track stopped time.

fe51ff3

aaudio_stream_get_position() calculates position using elapsed time since last callback, including the time during stream is stopped. To interpolate correctly, record the stop time and use it in calculation.

[AAudio] request pause rather than stop.

644b119

According to the document, AAudioStream_requestStop(): "The stream will stop after all of the data currently buffered has been played." AAudioStream_requestPause(): "Pausing a stream will freeze the data flow but not flush any buffers."

jhlin force-pushed the aaudio-get-postion branch from e75d3a6 to 644b119 Compare July 4, 2024 01:31

padenot merged commit 063a090 into mozilla:master Jul 8, 2024
15 checks passed

jhlin deleted the aaudio-get-postion branch July 9, 2024 16:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AAudio: don't count the time of stopped stream and replace stop request with pause. #789

AAudio: don't count the time of stopped stream and replace stop request with pause. #789

jhlin commented Jun 21, 2024

padenot Jun 21, 2024

jhlin Jun 22, 2024

jhlin commented Jun 26, 2024

jhlin commented Jun 26, 2024

jhlin commented Jun 27, 2024

Pehrsons commented Jun 27, 2024

jhlin commented Jun 27, 2024

Pehrsons commented Jun 28, 2024

Pehrsons left a comment

Pehrsons Jun 28, 2024

jhlin Jul 4, 2024

Pehrsons Jul 4, 2024

jhlin Jul 4, 2024

Pehrsons Jun 28, 2024

Pehrsons Jun 28, 2024

jhlin Jul 4, 2024

Pehrsons Jun 28, 2024

Pehrsons Jun 28, 2024

jhlin Jul 4, 2024

Pehrsons Jun 28, 2024

Pehrsons left a comment

Pehrsons Jun 28, 2024

Pehrsons Jun 28, 2024

Pehrsons Jun 28, 2024

AAudio: don't count the time of stopped stream and replace stop request with pause. #789

AAudio: don't count the time of stopped stream and replace stop request with pause. #789

Conversation

jhlin commented Jun 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhlin commented Jun 26, 2024

jhlin commented Jun 26, 2024

jhlin commented Jun 27, 2024

Pehrsons commented Jun 27, 2024

jhlin commented Jun 27, 2024

Pehrsons commented Jun 28, 2024

Pehrsons left a 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

Pehrsons left a 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