mozilla / cubeb

Cross platform audio library
ISC License
440 stars 124 forks source link

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

Closed jhlin closed 3 months ago

jhlin commented 3 months ago

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.

jhlin commented 3 months ago

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

jhlin commented 3 months ago

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

jhlin commented 3 months ago

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

Pehrsons commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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:

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