-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
arch/sim: add sim audio support #2124
Conversation
Very nice! |
@GUIDINGLI |
I probably won't be able to really look at this until the weekend, but if someone wants to review and merge ahead of that I can look at it in master then. |
In fact, this is a good idea to have a sim:audio example in place. |
I'll update the test repo to be able to support this when I do my testing. We will need to have the alsa libraries available. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GUIDINGLI please take @acassis and @btashton comment and update the patch.
@xiaoxiang781216 should I go ahead and get the ALSA libraries/headers in the CI images? I can also reserve a name for the sim with an exception for Darwin. That way this can run against CI, I don't see any reason this wont eventually make it in. |
Sure we need install |
I'll do this tomorrow when do a quick verification on my hardware. |
@xiaoxiang781216 I went ahead and just added it. Note I only included @GUIDINGLI The config name I reserved is |
1dac91b
to
ba24caf
Compare
N/A Cause lots of apps who use external library needs big stack size. e.g. alsa, ffmpeg Change-Id: I3b46333da9b18d103ea2ea71ed6e81d79a2d1d6c Signed-off-by: ligd <[email protected]>
@btashton could you please verify if everything were fixed? |
@GUIDINGLI @xiaoxiang781216
|
Could you show me your nxplayer cmd line ? mine : |
@GUIDINGLI
|
Have you pick another patch for increase stack size ? From your log: filep address is invaild in my gdb: filep is the same area with ap, both 0x7ffff2c... #0 file_vioctl (filep=0x7ffff2c7a8c8, req=4100, ap=0x7ffff2c9af40) at vfs/fs_ioctl.c:87 |
@GUIDINGLI |
You should also update apps,: |
Squashed commit of the following: sim audio: call alsa to playback/capture data sim/audio: correct the format capability sim/audio: add pause/resume support sim/audio: add auto stop when meet AUDIO_APB_FINAL sim/audio: fix abort when set small buffer_size sim/audio: move sim_audio.c to sim_alsa.c Change-Id: I8e00ece79159e844ca17fd4c363480b985ee0490 Signed-off-by: ligd <[email protected]>
@GUIDINGLI |
@btashton @xiaoxiang781216 @GUIDINGLI |
Hi @masayuki2009 could you please give an Approved from your side? If nobody is against it we can merge it now! |
No. My comment has not been addressed. |
@btashton could you review again? I think your comment is already addressed in the latest revision. |
direction = priv->playback ? SND_PCM_STREAM_PLAYBACK | ||
: SND_PCM_STREAM_CAPTURE; | ||
|
||
ret = snd_pcm_open(&pcm, "default", direction, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I guess this is actually where my comment should be. Don't we want to specify this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to change the device name? Ok, it depends on how many pcm instances we want to create. only one playback/capture is created in the current design, "default" should be the best choice in this case. On the other hand, sim_audio_initialize need accept name argument and open the different sound device if we want to simulate multiple sound devices. But I think it's better to create a new patch to change both the alsa driver and board initialization for the mulitple devices. Do you think so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I don't care if we limit to one device, but I want to be able to at least specify it via a config. "default" is certainly the correct default choice.
My default microphone is a poor choice in this case (I'm doing a loop back)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In ALSA design, user can modify alsa.conf to map "default" to the different sound hardware. That's why we don't make the device name configurable initially. If you really need this feature, we can add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just seems trivial to add the two kconfig values here and then not require people to modify their system for testing. This makes it a lot easier to use an alsa loopback device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to still merge fine, it just does not align with other sim hardware where the user can specify how it connects to to the host hardware.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we merge this patch first? and @GUIDINGLI provide a new patch to support configuration in tomorrow.
Summary
arch/sim: add sim audio support
Impact
sim audio will call alsa to playback/capture data
Testing
nxplayer
nxrecoder