Re: [PATCH RFC 2/2] ASoC: qcom: qdsp6/audioreach: add support for offloading raw opus playback

["Alexey Klimov" <alexey.klimov@linaro.org>]

On Wed Jun 18, 2025 at 1:34 PM BST, Srinivas Kandagatla wrote:
>
>
> On 6/16/25 4:26 PM, Alexey Klimov wrote:
>> Add support for OPUS module, OPUS format ID, media format payload struct
>> and make it all recognizable by audioreach compress playback path.
>> 
>> At this moment this only supports raw or plain OPUS packets not
>> encapsulated in container (for instance OGG container). For this usecase
>> each OPUS packet needs to be prepended with 4-bytes long length field
>> which is expected to be done by userspace applications. This is
>> Qualcomm DSP specific requirement.
>> > This patch is based on earlier work done by
>> Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>
> Thanks for picking this up Alexey,
>
> Same, co-dev by should be good attribute for such things.

I need your Signed-off-by then and/or permission to use your Sign off.

>> Cc: Annemarie Porter <annemari@quicinc.com>
>> Cc: Srinivas Kandagatla <srini@kernel.org>
>> Cc: Vinod Koul <vkoul@kernel.org>
>> Signed-off-by: Alexey Klimov <alexey.klimov@linaro.org>
>> ---
>>  sound/soc/qcom/qdsp6/audioreach.c | 33 +++++++++++++++++++++++++++++++++
>>  sound/soc/qcom/qdsp6/audioreach.h | 17 +++++++++++++++++
>>  sound/soc/qcom/qdsp6/q6apm-dai.c  |  3 ++-
>>  sound/soc/qcom/qdsp6/q6apm.c      |  3 +++
>>  4 files changed, 55 insertions(+), 1 deletion(-)
>> 
>> diff --git a/sound/soc/qcom/qdsp6/audioreach.c b/sound/soc/qcom/qdsp6/audioreach.c
>> index 4ebaaf736fb98a5a8a58d06416b3ace2504856e1..09e3a4da945d61b6915bf8b6f382c25ae94c5888 100644
>> --- a/sound/soc/qcom/qdsp6/audioreach.c
>> +++ b/sound/soc/qcom/qdsp6/audioreach.c
>> @@ -859,6 +859,7 @@ static int audioreach_set_compr_media_format(struct media_format *media_fmt_hdr,
>>  	struct payload_media_fmt_aac_t *aac_cfg;
>>  	struct payload_media_fmt_pcm *mp3_cfg;
>>  	struct payload_media_fmt_flac_t *flac_cfg;
>> +	struct payload_media_fmt_opus_t *opus_cfg;
>>  
>>  	switch (mcfg->fmt) {
>>  	case SND_AUDIOCODEC_MP3:
>> @@ -901,6 +902,38 @@ static int audioreach_set_compr_media_format(struct media_format *media_fmt_hdr,
>>  		flac_cfg->min_frame_size = mcfg->codec.options.flac_d.min_frame_size;
>>  		flac_cfg->max_frame_size = mcfg->codec.options.flac_d.max_frame_size;
>>  		break;
>> +	case SND_AUDIOCODEC_OPUS_RAW:
>> +		media_fmt_hdr->data_format = DATA_FORMAT_RAW_COMPRESSED;
>> +		media_fmt_hdr->fmt_id = MEDIA_FMT_ID_OPUS;
>> +		media_fmt_hdr->payload_size = sizeof(struct payload_media_fmt_opus_t);
>
> maybe sizeof(*opus_cfg)?

Yes, I can update that.

>> +		p = p + sizeof(*media_fmt_hdr);
>> +		opus_cfg = p;
>> +		/* raw opus packets prepended with 4 bytes of length */
>> +		opus_cfg->bitstream_format = 1;
>> +		/*
>> +		 * payload_type:
>> +		 * 0 -- read metadata from opus stream;
>> +		 * 1 -- metadata is provided by filling in the struct here.
>> +		 */
>> +		opus_cfg->payload_type = 1;
>> +		opus_cfg->version = mcfg->codec.options.opus_d.version;
>> +		opus_cfg->num_channels = mcfg->codec.options.opus_d.num_channels;
>> +		opus_cfg->pre_skip = mcfg->codec.options.opus_d.pre_skip;
>> +		opus_cfg->sample_rate = mcfg->codec.options.opus_d.sample_rate;
>> +		opus_cfg->output_gain = mcfg->codec.options.opus_d.output_gain;
>> +		opus_cfg->mapping_family = mcfg->codec.options.opus_d.mapping_family;
>> +		opus_cfg->stream_count = mcfg->codec.options.opus_d.stream_count;
>> +		opus_cfg->coupled_count = mcfg->codec.options.opus_d.coupled_count;
>> +
>> +		if (mcfg->codec.options.opus_d.num_channels == 1) {
>> +			opus_cfg->channel_mapping[0] = PCM_CHANNEL_FL;
>> +		} else if (mcfg->codec.options.opus_d.num_channels == 2) {
>> +			opus_cfg->channel_mapping[0] = PCM_CHANNEL_FL;
>> +			opus_cfg->channel_mapping[1] = PCM_CHANNEL_FR;
>> +		}
>
> Pl use audioreach_set_default_channel_mapping() to fill in the channel
> mapping data.
>
> Why are we not using channel mapping info from the snd_dec_opus struct here?

Okay, I was re-reading RFC and can't really get my head around this.

So first I came up with something like this:

	switch (opus_cfg->mapping_family) {
	case 0:
		if (num_chan == 1 || num_chan == 2)
			audioreach_set_default_channel_mapping(ch_map, num_chan);
		else
			/* mapping family 0 allows only 1 or 2 channels */
			return -EINVAL;
			break;
		case 1:
			if (num_chan > 8)
				return -EINVAL;
			if (mcfg->codec.options.opus_d.coupled_count > mcfg->codec.options.opus_d.stream_count)
				return -EINVAL;

			memcpy(ch_map, mcfg->codec.options.opus_d.channel_map, sizeof(mcfg->codec.options.opus_d.channel_map));
			break;
		default:
			/* mapping family 2..255 shouldn't be allowed to playback */
			return -EOPNOTSUPP;
		}

but I don't think above is correct at all.

After re-reading the RFC few more times. It looks that channel_mapping in
opus struct has nothing to do with channel mapping that we need to provide
to DSP here. The channel mapping maps "decoded" channels to output channels
and seems to be needed by opus decoder logic and in some sense is internal
thingy to correctly construct sound for output channel from opus stream(s).
In other words if output channel is present and valid then channel_mapping
describes how and which decoded stream or streams (coupled or uncoupled)
to use for producing sound output for that output channel.
This is described in https://www.rfc-editor.org/rfc/rfc7845#section-5.1.1

The number of output channels here actually matters for us. We can construct
mapping for channels that we pass to DSP based just only on the number of
output channels here and let DSP to figure out how to scatter or downmix or
upmix them to its own output channels.

Conclusion from my understanding:
-- we shouldn't mess with opus_cfg->channel_mapping here, it is needed for
the correct operation of decoder, we shouldn't call
audioreach_set_default_channel_mapping() on it;
-- mapping output channels to provide the mapping to DSP might require some
rework or I need to look into this.

Or something else that didn't came up in my mind yet.

Also, I don't have any test files to test mapping_family 1 and some tricky
cases here. As far as I understand, it works just fine right now with
mapping_family 0.

Best regards,
Alexey