Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions audio/Android.mk
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@ ifeq ($(strip $(TINYHAL_COMPRESS_PLAYBACK)),true)
LOCAL_CFLAGS += -DTINYHAL_COMPRESS_PLAYBACK
endif

ifeq ($(TARGET_DEVICE),kingfisher)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

LOCAL_CFLAGS += -DPLATFORM_RCAR3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and also use a generic name for this define instead of the name of a platform

endif

include $(BUILD_SHARED_LIBRARY)

endif
89 changes: 87 additions & 2 deletions audio/audio_hw.c
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
#endif

#include <audio_utils/resampler.h>
#include <audio_utils/channels.h>

#include <tinyhal/audio_config.h>

Expand All @@ -57,6 +58,23 @@
#include <vendor/cirrus/scchal/scc_audio.h>
#endif

#ifdef PLATFORM_RCAR3
/* R-Car3 driver supports only 8-channel streams and 48000 sample rate */
/* These values are defined in _frames_ (not bytes) to match the ALSA API */
#define OUT_PERIOD_SIZE_DEFAULT 256
#define OUT_PERIOD_COUNT_DEFAULT 4
#define OUT_CHANNEL_MASK_DEFAULT AUDIO_CHANNEL_OUT_7POINT1
#define OUT_CHANNEL_COUNT_DEFAULT 8
#define OUT_RATE_DEFAULT 48000

#define IN_PERIOD_SIZE_DEFAULT 256
#define IN_PERIOD_COUNT_DEFAULT 4
#define IN_CHANNEL_MASK_DEFAULT AUDIO_CHANNEL_IN_STEREO
#define IN_CHANNEL_COUNT_DEFAULT 8
#define IN_RATE_DEFAULT 48000

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@rfvirgil rfvirgil Dec 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you use a <set> constant I suggest naming it "hw-channels"

#else

/* These values are defined in _frames_ (not bytes) to match the ALSA API */
#define OUT_PERIOD_SIZE_DEFAULT 256
#define OUT_PERIOD_COUNT_DEFAULT 4
Expand All @@ -70,6 +88,8 @@
#define IN_CHANNEL_COUNT_DEFAULT 1
#define IN_RATE_DEFAULT 44100

#endif

#define IN_PCM_BUFFER_SIZE_DEFAULT \
(IN_PERIOD_SIZE_DEFAULT * IN_CHANNEL_COUNT_DEFAULT * sizeof(uint16_t))

Expand Down Expand Up @@ -234,6 +254,9 @@ struct stream_in_pcm {
uint32_t period_size; /* ... of PCM input */

struct in_resampler resampler;

void *pre_read_buf; /* Buffer to store read data before channel adjustment*/
size_t pre_read_buf_size;
};

static uint32_t out_get_sample_rate(const struct audio_stream *stream);
Expand Down Expand Up @@ -1660,6 +1683,11 @@ static unsigned int in_pcm_cfg_rate(struct stream_in_pcm *in)

static unsigned int in_pcm_cfg_channel_count(struct stream_in_pcm *in)
{
#ifdef PLATFORM_RCAR3
/* On R-Car3 driver supports only 8 channels reading */
return IN_CHANNEL_COUNT_DEFAULT;
#endif

if (in->common.channel_count != 0) {
return in->common.channel_count;
} else {
Expand Down Expand Up @@ -1741,6 +1769,22 @@ static int do_open_pcm_input(struct stream_in_pcm *in)

ALOGV("input buffer size=0x%zx", in->common.buffer_size);

if (in->common.channel_count != in->hw_channel_count) {
ALOGV("Setup buffer to read %d channel stream with adjustment (HW only supports %d channel reading)",
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (!in->pre_read_buf) {
ret = -ENOMEM;
goto fail;
}
}
else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preferred style is

} else {

all on one line

in->pre_read_buf = NULL;
in->pre_read_buf_size = 0;
}

/*
* If the stream rate differs from the PCM rate, we need to
* create a resampler.
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need to check for NULL pointer. It is safe to pass NULL to free()

free(in->pre_read_buf);
exit:
ALOGV("-do_open_pcm_input error:%d", ret);
return ret;
Expand Down Expand Up @@ -1864,6 +1910,8 @@ static ssize_t do_in_pcm_read(struct audio_stream_in *stream, void *buffer,
int ret = 0;
struct stream_in_pcm *in = (struct stream_in_pcm *)stream;
size_t frames_rq = bytes / in->common.frame_size;
void *read_buf;
size_t read_buf_size;

ALOGV("+do_in_pcm_read %zu", bytes);

Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
read_buf = buffer;
read_buf_size = bytes;
}

if (in->resampler.resampler != NULL) {
ret = read_resampled_frames(in, buffer, frames_rq);
ret = read_resampled_frames(in, read_buf, frames_rq);
} else {
ret = pcm_read(in->pcm, buffer, bytes);
ret = pcm_read(in->pcm, read_buf, read_buf_size);
}

if (ret < 0)
goto exit;

if (in->common.channel_count != in->hw_channel_count) {

ALOGV("do_in_pcm_read(): Adjust %d channel read stream to %d channel stream",
in->hw_channel_count, in->common.channel_count);

bytes = adjust_channels(read_buf, /* Input buffer */
in->hw_channel_count, /* Input channel count */
buffer, /* Output buffer */
in->common.channel_count, /* Output channel count */
in->common.frame_size / in->common.channel_count, /* Sample size */
read_buf_size /* Size of input buffer in bytes */
);
if (!bytes) {
ALOGE("do_in_pcm_read(): Failed to adjust channels");
ret = -1;
}
}

/* Assume any non-negative return is a successful read */
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need to check pre_read_buf for NULL. It is safe to pass NULL to free().

{
free(in->pre_read_buf);
in->pre_read_buf = NULL;
in->pre_read_buf_size = 0;
}

do_close_in_common(stream);
}

Expand Down