samd: audio_dma: Shift responsibility for two-dma-channel audio #2533
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This removes downscaling (halving-add) when multiple voices are being mixed. To avoid clipping, and get similar behavior to before, set the "level" of each voice to (1/voice_count). Slow paths that were applicable to only M0 chips were removed. As a side effect, the internal volume representation is now 0 .. 0x8000 (inclusive), which additionally makes a level of exactly 0.5 representable. Testing performed, on PyGamer: For all 4 data cases, for stereo and mono, for 1 and 2 voices, play pure sign waves represented as RawSamples and view the result on a scope and through headphones. Also, scope the amount of time spent in background tasks. Code size: growth of +272 bytes Performance (time in background task when mixing 2 stereo 16-bit voices): 76us per down from 135us (once per ~2.9ms long term average) (Decrease from 4.7% to 2.4% of all CPU time)
What I didn't understand was that initially, the sequence of channel numbers is "0011", while in normal operation it should consistently be "0101". Just keeping track of the "other channel" works during the steady state, but at startup it causes the right channel to be shifted by 1. This cures the small glitches that could be seen clearly when playing stereo sine wave mp3s.
While working on audio playback, I noticed that there were problems affecting only the right channel, and only on samd dac; not on i2s or on nrf. These problems affected both the mp3 and mixer audio sources, and stem from the necessary handling of the "single_channel" flag to the "get_buffer" method being complicated and not (as far as I could see) documented. Rather than fix the similar bug at least twice, I think it is preferable to shift the responsibility to the port that requires it, so that it can be gotten right once. (Furthermore, I *suspect* that after a DMA overrun, there's simply no way to get the right data back in each channel, because in this case they may have been advanced independently and the series of "almost exactly alternating" calls to get_buffer per channel was broken) So: * For samd51, add the concept of a "sibling" DMA channel. When playing stereo on DAC, the left channel is made the "first" sibling (so its sibling channel number is the right channel number) and the right channel is made the second channel. * To facilitate this, the step of preloading audio blocks has to be split out, so that the channels can be linked at the right moment * Some details around stopping channels together Testing checklist (pygamer + jeplayer, DAC): * [X] Listen to the whole bartlebeats album without an audio glitch * [ ] Run through the RawSample test cases * [ ] Run through the WaveFile + AudioMixer test cases Testing checlist (Metro M4 Express + I2S): * [ ] Run through the RawSample test cases * [ ] Run through the WaveFile + AudioMixer test cases Testing checklist (Trinket M0 / Pyruler): * [ ] Run through the mono RawSample test cases
These are no longer needed, as the implementation that ever passed 'true' to single_channel is gone. This also allows removal of some state from audio sample sources, such as the "channel read count" of WaveFile. This change reindents code and as such may be better to view with a whitespace-ignoring diff such as "git show -w"
.. this allows type confusion for all parameters (and even parameter counts), rather than just for the type of the first parameter.
.. spacing is Just Wrong(TM)
This out-parameter was always being set to 1 for all implementations
|
@tannewt It's not review-ready yet, but can I get your comment on the general idea of this change before I spend more time on my testing plan? |
tannewt
reviewed
Jan 22, 2020
There was a problem hiding this comment.
No major concerns from me. I like how the get_buffer calls are simplified.
I would rather not see as many void* and use mp_obj_t (which I think is a typedef for void*) because it implies that type info is in the first word.
I did these gymnastics on SAMD51 to try an minimize memory bus and DMA traffic by doing the one 32bit write instead of two 16bit ones.
|
Thanks for the quick feedback. This won't resurface as a priority for awhile, so consider the ball firmly in my court. |
|
Let's close this one up too. It needs to be heavily reworked with #2552 in, as outlined in my comments there. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While working on audio playback, I noticed that there were problems affecting only the right channel, and only on samd dac; not on i2s or on nrf.
These problems affected both the mp3 and mixer audio sources, and stem from the necessary handling of the "single_channel" flag to the "get_buffer" method being complicated and not (as far as I could see) documented.
Rather than fix the similar bug at least twice, I think it is preferable to shift the responsibility to the port that requires it, so that it can be gotten right once.
(Furthermore, I suspect that after a DMA overrun, there's simply no way to get the right data back in each channel, because in this case they may have been advanced independently and the series of "almost exactly alternating" calls to get_buffer per channel was broken)
So:
This touches a lot of stuff, so I'm leaving it in draft state until I get through this self-made testing checklist. Also, I ended up writing it cumulative with #2526 because otherwise there would be conflicts between the two PRs; After that PR is merged this one should be rebased before its own merge.
Testing checklist (pygamer, DAC):
Testing checlist (Metro M4 Express + I2S):
Testing checklist (Trinket M0 / Pyruler):