Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AudioFrame playback chunk and the simulator #195

Closed
microbit-carlos opened this issue Apr 11, 2024 · 8 comments
Closed

AudioFrame playback chunk and the simulator #195

microbit-carlos opened this issue Apr 11, 2024 · 8 comments
Assignees
Milestone

Comments

@microbit-carlos
Copy link
Contributor

From @microbit-matt-hillsdon in #163 (comment), broken out here to be able to keep discussions on their own thread:

The existing sim implementation of playing AudioBuffers has always been problematic because of the 4ms chunk size. For audio frames of longer duration I had more hope, but at the moment the chunk size used is the same even though we likely have a much larger buffer in the frame itself.

Our work-in-progress record/playback sim implementation works OK if you increase LOG_AUDIO_CHUNK_SIZE from 5 to 6. (it's possible that on slower computers a bigger buffer still might help, I've not had a chance to test yet). We can't just do that as I think it means regular audio frames are pitch/rate shifted but it might show a way forward for the sim.

The sim changes without the LOG_AUDIO_CHUNK_SIZE change can be seen here: https://review-python-simulator.usermbit.org/beta-updates/demo.html (microbit-foundation/micropython-microbit-v2-simulator#113) - see sample "Record". For now you'll have to rebuild locally to see the benefit of the buffer size change. We've not yet looked at edge cases etc. but I think this problem will remain.

@microbit-carlos microbit-carlos added this to the 2.2.0-beta.1 milestone Apr 11, 2024
@microbit-carlos
Copy link
Contributor Author

@dpgeorge are you able to provide some guidance here?

@jaustin
Copy link
Contributor

jaustin commented May 13, 2024

@dpgeorge this is currently the only thing blocking putting audio/record in the simulator for beta so we'd appreciate a view on this - especially if it needs a design change as we iterate the audio stuff anyway to make sure it works. (I'm thinking about our conversation about how a contiguous memoryview is more natural and efficient compared to a generator of smaller buffers)

@dpgeorge
Copy link
Collaborator

I think I have a way forward here to allow for a minimum AudioFrame size of 32 bytes, but also have a larger output buffer, and the output buffer is filled as much as possible before calling down in to the lower layer (CODAL, simulator) to play the data.

@dpgeorge
Copy link
Collaborator

I have pushed some commits to the audio-recording branch which should resolve this issue. You can now define (at the C level, in mpconfigport.h) the following macro to increase the audio output buffer size:

#define AUDIO_OUTPUT_BUFFER_SIZE (256) // this can be any value, defaults to 32 bytes

That will be the number of bytes that are sent at a time down to the lower layers of the audio pipeline (eg to the simulator).

@microbit-matt-hillsdon see if that works!

@microbit-matt-hillsdon
Copy link
Contributor

microbit-matt-hillsdon commented May 24, 2024

Thank you, this is way better for playback of recordings and improves the situation for traditional audio frame use.

Updated here:
https://review-python-simulator.usermbit.org/beta-updates/demo.html
https://review-python-editor-v3.microbit.org/beta-micropython/
(At some point in these changes we've broken regular AudioFrame use on Firefox - see notes on microbit-foundation/micropython-microbit-v2-simulator#113 (comment)).

Needs some testing on slower hardware but I think this is more an asyncify/event loop tick resolution issue than a performance one so I'm optimistic.

I used the smallest viable buffer (64) because of an issue with wait=True.

As I understand it, we stop waiting when we've finished sending the audio to play not when it's finished playing.
The Web Audio side currently request an extra buffer when we start playing (to avoid stalling) and more buffers as we finish playing each buffer. So the last writeData returns with two 64 sample buffers still to play.

Initially we had a sim-only issue where we'd overlap audio if there was a subsequent play after a wait=True play but we've fixed that. So the remaining issue is that control is arguably returned to the user program early. These issues are somewhat noticeable in the "Audio" sample on the sim demo page.

It might still be possible to overlap different types of audio by the two buffers time - e.g. I think we could start playing speech while still processing the last two raw audio buffers.

I'm not sure these issues have a straightforward fix and they might be acceptable.

There's one more thing I wanted to check: If you ever play an AudioFrame then we now seem to play silence after it indefinitely. Is that an intentional change? We don't think this happened before the MicroPython update and maybe happened in this latest change. I'm dropping the silent frames in the C-side of the HAL at the moment.

@dpgeorge
Copy link
Collaborator

Thank you, this is way better for playback of recordings and improves the situation for traditional audio frame use

Great! I'm glad it works.

There's one more thing I wanted to check: If you ever play an AudioFrame then we now seem to play silence after it indefinitely. Is that an intentional change?

Oops, that's a mistake on my side. Now fixed, see the most recent commit on the audio-recording branch. Please can you test this to see if it fixes it for you?

As I understand it, we stop waiting when we've finished sending the audio to play not when it's finished playing.

This is a bug on the MicroPython side that we will try to resolve. See #182 (comment), and related #206

@microbit-matt-hillsdon
Copy link
Contributor

There's one more thing I wanted to check: If you ever play an AudioFrame then we now seem to play silence after it indefinitely. Is that an intentional change?

Oops, that's a mistake on my side. Now fixed, see the most recent commit on the audio-recording branch. Please can you test this to see if it fixes it for you?

Looks good now that I've updated the sim to 17aaa11 (same demo links as above). Thanks.

@dpgeorge
Copy link
Collaborator

Looks good now that I've updated the sim to ...

Very good! Thanks for confirming.

I think we can probably close this issue now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants