tinyhal: audio: Impl channel adjustment for input sound flow #10
Conversation
AlexBobro
force-pushed
the
Impl_channel_adjustment_for_input
branch
from
4c88a65
to
d04fa5d
Compare
Fix the issue where applications can not capture a sound through Tinyhal with requested channel count less than 8 (the min/max interval for allowed channel count values is set to [8..8] in 'pcm3168a' driver, so pcm_open() function returns error in case count != 8). Implement a patch that provides a channel adjustment for input sound by converting the 8-channel flow to requested Mono or Stereo flow. Signed-off-by: Oleksandr Bobro <oleksandr.bobro@globallogic.com>
AlexBobro
force-pushed
the
Impl_channel_adjustment_for_input
branch
from
d04fa5d
to
e48358e
Compare
|
I have excluded from this PR the commits provided by @AntonDehtiarov in his PR#9. |
| @@ -76,6 +76,10 @@ ifeq ($(strip $(TINYHAL_COMPRESS_PLAYBACK)),true) | |||
| LOCAL_CFLAGS += -DTINYHAL_COMPRESS_PLAYBACK | |||
| endif | |||
|
|
|||
| ifeq ($(TARGET_DEVICE),kingfisher) | |||
There was a problem hiding this comment.
Please don't create defines and dependencies for a specific target. Make a generic define that describes the functionality it controls and define it in your device.mk. For example see the 3 defines above (lines 66..77)
| @@ -76,6 +76,10 @@ ifeq ($(strip $(TINYHAL_COMPRESS_PLAYBACK)),true) | |||
| LOCAL_CFLAGS += -DTINYHAL_COMPRESS_PLAYBACK | |||
| endif | |||
|
|
|||
| ifeq ($(TARGET_DEVICE),kingfisher) | |||
| LOCAL_CFLAGS += -DPLATFORM_RCAR3 | |||
There was a problem hiding this comment.
... and also use a generic name for this define instead of the name of a platform
| #define IN_CHANNEL_MASK_DEFAULT AUDIO_CHANNEL_IN_STEREO | ||
| #define IN_CHANNEL_COUNT_DEFAULT 8 | ||
| #define IN_RATE_DEFAULT 48000 | ||
|
|
There was a problem hiding this comment.
This is all a nasty hack, changing the default values to match some specific hardware instead of using the configuration.
The defaults are just that - defaults to use if nothing else is defined by the xml or driver.
The correct way to do this is use a defined constant in the xml file (https://github.com/CirrusLogic/tinyhal/blob/master/audio.example.xml#L342) to set the hardware channel count. Or read the limits from ALSA and work it out automatically.
There was a problem hiding this comment.
If you use a <set> constant I suggest naming it "hw-channels"
| goto fail; | ||
| } | ||
| } | ||
| else { |
There was a problem hiding this comment.
Preferred style is
} else {
all on one line
| @@ -1758,6 +1802,8 @@ static int do_open_pcm_input(struct stream_in_pcm *in) | |||
| fail: | |||
| pcm_close(in->pcm); | |||
| in->pcm = NULL; | |||
| if (in->pre_read_buf) | |||
There was a problem hiding this comment.
Don't need to check for NULL pointer. It is safe to pass NULL to free()
| @@ -2015,6 +2092,14 @@ static void do_close_in_pcm(struct audio_stream *stream) | |||
| { | |||
| struct stream_in_pcm *in = (struct stream_in_pcm *)stream; | |||
|
|
|||
| ALOGV("do_close_in_pcm(%p)", in); | |||
| if (in && in->pre_read_buf) | |||
There was a problem hiding this comment.
Don't need to check pre_read_buf for NULL. It is safe to pass NULL to free().
| in->common.channel_count, in->hw_channel_count); | ||
| in->pre_read_buf_size = in->common.buffer_size * in->hw_channel_count / in->common.channel_count; | ||
| ALOGV("in->pre_read_buf_size=0x%zx", in->pre_read_buf_size); | ||
| in->pre_read_buf = calloc(1, in->pre_read_buf_size); |
There was a problem hiding this comment.
malloc()?
The only reason I can see to use calloc() like this is to zero-fill the pre_read_buf. But it will be overwritten by the first pcm_read(). So the zero-fill is just overhead.
| @@ -1874,10 +1922,39 @@ static ssize_t do_in_pcm_read(struct audio_stream_in *stream, void *buffer, | |||
| goto exit; | |||
| } | |||
|
|
|||
| if (in->pre_read_buf) { | |||
| read_buf = in->pre_read_buf; | |||
| read_buf_size = in->pre_read_buf_size; | |||
There was a problem hiding this comment.
This assumes bytes == (pre_read_buf_size / in->hw_channel_count) * in->common.channel_count
If it isn't, you'll read the wrong amount of data from ALSA. And also possibly adjust_channels() will fall off the end of buffer and overwrite memory.
Hello.
We have detected the issue on RCAR3 platform where high level applications can not capture a sound through Tinyhal with requested channel count less than 8. This occurs because the min-max interval for allowed channel count values
is set to [8..8] in 'pcm3168a' driver, so pcm_open() function returns error in case requested count != 8.
But only AUDIO_CHANNEL_IN_MONO & AUDIO_CHANNEL_IN_STEREO channel masks are supported by functionality in alsa_device_profile.c file ('alsa_utils' directory).
So it is required to apply the current patch, which provides a channel adjustment for input sound by converting the N-channel flow (N - supported by driver) to requested Mono or Stereo flow.
When this patch is merged, we will be able to fix described problem just by a setting IN_CHANNEL_COUNT_DEFAULT and
'in->hw_channel_count' to the value N supported by our driver.