Skip to content

RFC: Add final support for compress audio #3959

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.

Sign up for GitHub

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

Closed
dbaluta wants to merge 7 commits into from
Closed

RFC: Add final support for compress audio #3959

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8c74976
audio: codec_adapter: Use generic init_process
dbaluta Mar 25, 2021
5d82014
audio: codec_adapter: Fix local_buffer size computation
dbaluta Mar 25, 2021
14ad9c2
audio: codec_adapter: Updated processed bytes
dbaluta Mar 25, 2021
f1edc11
audio: codec_adapter: Fix sink bytes copy
dbaluta Mar 25, 2021
a3245fb
audio: codec_adapter: Make sure local_buff has enough room for proces…
dbaluta Mar 25, 2021
7f21aa8
audio: codec_adapter: Fix processing loop
dbaluta Mar 25, 2021
ac6c89b
topology: imx8: Add processing pipeline for i.MX8/i.MX8X
dbaluta Feb 3, 2021
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: 0 additions & 4 deletions src/audio/codec_adapter/codec/cadence.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,10 +399,6 @@ int cadence_codec_prepare(struct comp_dev *dev)
goto free;
}

ret = cadence_codec_init_process(dev);
if (ret)
goto free;

comp_dbg(dev, "cadence_codec_prepare() done");
return 0;
free:
Expand Down
27 changes: 20 additions & 7 deletions src/audio/codec_adapter/codec_adapter.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ static int codec_adapter_prepare(struct comp_dev *dev)
}

/* Allocate local buffer */
buff_size = MAX(cd->period_bytes, codec->cpd.in_buff_size) * buff_periods;
buff_size = MAX(cd->period_bytes, codec->cpd.out_buff_size) * buff_periods;
if (cd->local_buff) {
ret = buffer_set_size(cd->local_buff, buff_size);
if (ret < 0) {
Expand Down Expand Up @@ -392,6 +392,7 @@ static int codec_adapter_copy(struct comp_dev *dev)
return ret;

bytes_to_process -= codec->cpd.consumed;
processed += codec->cpd.consumed;
comp_update_buffer_consume(source, codec->cpd.consumed);
}

Expand All @@ -405,6 +406,13 @@ static int codec_adapter_copy(struct comp_dev *dev)
break;
}

/* TODO: remove magic number and use possible maximum output per
* decoding algorithm type
* 9216 is maximum frame length bytes for mp3 at 192Khz, 24bits,
Copy link
Member

@plbossart plbossart Mar 26, 2021

Choose a reason for hiding this comment

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

that makes no sense. mp3 never supported 192kHz and perceptual coders don't care about bit depth.

I don't recall what the max value was but this was something at 320 kbits/s

Copy link
Collaborator Author

@dbaluta dbaluta Mar 26, 2021

Choose a reason for hiding this comment

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

@plbossart It is clear that I still need to read upon mp3 stuff and all. I'm still confused about all the version / etc.

I started with a sample mp3 file:

root@imx8qxpc0mek:~# file samples/lb.mp3    
samples/lb.mp3: MPEG ADTS, layer III, v1, 192 kbps, 48 kHz, Stereo

From the internet I have this:

// Bitrates - use [version][layer][bitrate]
const uint16_t mpeg_bitrates[4][4][16] = {
  { // Version 2.5
    { 0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0, 0 }, // Reserved
    { 0,   8,  16,  24,  32,  40,  48,  56,  64,  80,  96, 112, 128, 144, 160, 0 }, // Layer 3
    { 0,   8,  16,  24,  32,  40,  48,  56,  64,  80,  96, 112, 128, 144, 160, 0 }, // Layer 2
    { 0,  32,  48,  56,  64,  80,  96, 112, 128, 144, 160, 176, 192, 224, 256, 0 }  // Layer 1
  },
  { // Reserved
    { 0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0, 0 }, // Invalid
    { 0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0, 0 }, // Invalid
    { 0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0, 0 }, // Invalid
    { 0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0, 0 }  // Invalid
  },
  { // Version 2
    { 0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0, 0 }, // Reserved
    { 0,   8,  16,  24,  32,  40,  48,  56,  64,  80,  96, 112, 128, 144, 160, 0 }, // Layer 3
    { 0,   8,  16,  24,  32,  40,  48,  56,  64,  80,  96, 112, 128, 144, 160, 0 }, // Layer 2
    { 0,  32,  48,  56,  64,  80,  96, 112, 128, 144, 160, 176, 192, 224, 256, 0 }  // Layer 1
  },
  { // Version 1
    { 0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0, 0 }, // Reserved
    { 0,  32,  40,  48,  56,  64,  80,  96, 112, 128, 160, 192, 224, 256, 320, 0 }, // Layer 3
    { 0,  32,  48,  56,  64,  80,  96, 112, 128, 160, 192, 224, 256, 320, 384, 0 }, // Layer 2
    { 0,  32,  64,  96, 128, 160, 192, 224, 256, 288, 320, 352, 384, 416, 448, 0 }, // Layer 1
  }
};

If my understanding is correct MP3 is MPEG-1 Audio Layer III or MPEG-2 Audio Layer III.

So, based on the table above mp3 should also support 192kHZ. But I admit my comment there is just bad! I will remove it.

Copy link
Member

@plbossart plbossart Mar 26, 2021

Choose a reason for hiding this comment

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

you may be confusing kbs and kHz :-)

the rates are
version 1: 48, 44.1 and 32kHz
version 2: 24, 22.05 and 16 kHz
version 2.5: 12, 11.025 and 8 kHz (this is actually not a standard but an extension by FhG).

https://en.wikipedia.org/wiki/MP3 has most of the needed information.

Copy link
Collaborator Author

@dbaluta dbaluta Mar 26, 2021

Choose a reason for hiding this comment

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

Yes, you are right the comment should be read 192kbps.

*/
if (local_buff->stream.free < 9217)
Copy link
Contributor

@iuliana-prodan iuliana-prodan Mar 26, 2021

Choose a reason for hiding this comment

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

I would put this magic number in a define.
Also, maybe, it will be good to specify this limitation in commit message: "so far we support mp3 at 192khz and 24 bits".

Copy link
Collaborator Author

@dbaluta dbaluta Mar 26, 2021

Choose a reason for hiding this comment

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

@thanks Iulia for the comment. What I actually plan to do is to create a generic function that given a codec and its parameters it will return the maximum amount of the output data.

Something like code_adapter_get_max_bytes? I just couldn't find a good name. This work is still RFC. We won't submit the code like that.

Copy link
Contributor

@iuliana-prodan iuliana-prodan Mar 26, 2021

Choose a reason for hiding this comment

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

A generic function sounds good.
Regarding the naming, I think it's ok, but I'm not good with names either :)

Copy link
Member

@lgirdwood lgirdwood Mar 26, 2021

Choose a reason for hiding this comment

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

Fwiw the 9212 should also be a topology 2.0 attribute (as this can also be enforced in any MP3 topology)

Copy link
Collaborator Author

@dbaluta dbaluta Mar 26, 2021

Choose a reason for hiding this comment

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

@lgirdwood this could be an option. It looks like at least for mp3 the number of samples resulted after decoding one frame is always the same 1152 (9217 = 1152 samples * 2 channels * 4 bytes / sample + 1). If this holds true for all the decoding algorithms then getting this via topology could be an option.

Copy link
Member

@plbossart plbossart Mar 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think it would be a grave mistake to use a single period.

if the scheduling is constrained by the output size, then it may happen that you have to decode multiple frames in the allocated time. In our past integration, the decoder was part of a different pipeline and scheduled with a lower priority, so that there were no peaks in the MCPS usage. I really think you have to decouple the codec_adapter from the dai scheduling....

Copy link
Collaborator Author

@dbaluta dbaluta Mar 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@plbossart Definitely! That was my next step. I try to do it gradually and also have something that's working for our releases. Customers want to look at the DSP even if it isn't the final code.
I will try to see how we satisfy both requirements. In time! :)

  • the only thing that's definitely out of place right now is the m4 topology.

For the rest of the code:

  • existing DAP support (based on PCM frames, should still work)
  • no other functionality is impacted.

Next steps:
(1) decouple codec adapter from DAI scheduling. In fact here there is no clear line where should I do the decoupling.

Because codec_adapter has an output buffer that holds the decoded samples and DMA needs to copy based on DAI pace
each period "period_bytes" of data.
So, each DAI period I need to call codec_adapter_copy() to push data from codec_adapter local buffer to DMA buffer.

Also, the Host need to push data to codec_adapter based on its own pace. So, Host should copy one "compress period" of data each time the DAI has rendered one mp3 frame.

So, in simple terms:

  • Host should copy one mp3 compressed frame each 0.026 sec (compress period will be 26ms).
  • DAI should copy SAMPLE_RATE/1000 sampes each 1ms (compress period will be 1ms).

Also, we noticed that we could process around 3 to 4 MP3 frames within 1ms , so there is enough room for our simple sceario to work.

Copy link
Member

@plbossart plbossart Mar 26, 2021

Choose a reason for hiding this comment

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

I think you should really have 3 pipelines
a) dai with mixer
b) PCM for notifications/beeps that may occur at any time
c) compressed. That pipe may run at a lower priority or larger granularity than the DAI period. The last thing you want it to try and complete one decode at a 26x speed, you have to spread the work across multiple dai periods otherwise you will underrun when your system is loaded.

IIRC in all implementations, the DAI typically works with O(ms) buffers, and the compressed pipeline can work with much larger frames. In fact if you look at newer codecs they will generate 4k or 8k samples in one frame. So you need some sort of ring buffer to adjust between codec and dai.

Copy link
Member

@plbossart plbossart Mar 26, 2021

Choose a reason for hiding this comment

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

And I forget that you need an SRC on your compressed pipeline if you want to support all frequencies.

Copy link
Collaborator Author

@dbaluta dbaluta Mar 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Nice suggestion. I wonder if compress and PCM can be supported on the sample sound card device on the Linux side.

goto db_verify;

buffer_invalidate(source, codec_buff_size);
codec_adapter_copy_from_source_to_lib(&source->stream, &codec->cpd,
codec_buff_size);
Expand All @@ -426,18 +434,22 @@ static int codec_adapter_copy(struct comp_dev *dev)
bytes_to_process -= codec->cpd.consumed;
processed += codec->cpd.consumed;
produced += codec->cpd.produced;

audio_stream_produce(&local_buff->stream, codec->cpd.produced);
comp_update_buffer_consume(source, codec->cpd.consumed);
}

if (!produced && !cd->deep_buff_bytes) {
comp_dbg(dev, "codec_adapter_copy(): nothing processed in this call");
goto end;
} else if (!produced && cd->deep_buff_bytes) {
goto db_verify;
/* we haven't produced anything in this period but we
* still have data in the local buffer to copy to sink
*/
if (audio_stream_get_avail_bytes(&local_buff->stream) >= cd->period_bytes)
goto copy_period;
else
goto end;
}

audio_stream_produce(&local_buff->stream, produced);
comp_update_buffer_consume(source, processed);

db_verify:
if (cd->deep_buff_bytes) {
if (cd->deep_buff_bytes >= audio_stream_get_avail_bytes(&local_buff->stream)) {
Expand All @@ -450,6 +462,7 @@ static int codec_adapter_copy(struct comp_dev *dev)
}
}

copy_period:
comp_get_copy_limits_with_lock(local_buff, sink, &cl);
copy_bytes = cl.frames * cl.source_frame_bytes;
audio_stream_copy(&local_buff->stream, 0,
Expand Down
1 change: 1 addition & 0 deletions src/include/sof/audio/codec_adapter/interfaces.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ static struct codec_interface interfaces[] = {
.id = CADENCE_ID, /**< Cadence interface */
.init = cadence_codec_init,
.prepare = cadence_codec_prepare,
.init_process = cadence_codec_init_process,
.process = cadence_codec_process,
.apply_config = cadence_codec_apply_config,
.reset = cadence_codec_reset,
Expand Down
1 change: 1 addition & 0 deletions tools/topology/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ set(TPLGS
"sof-jsl-da7219\;sof-jsl-da7219\;-DPLATFORM=jsl"
"sof-jsl-da7219\;sof-jsl-da7219-mx98360a\;-DPLATFORM=jsl-dedede"
"sof-imx8mp-wm8960\;sof-imx8mp-wm8960"
"sof-imx8-processing-wm8960\;sof-imx8-processing-wm8960\;-DPP_CORE=0"
"sof-smart-amplifier-nocodec\;sof-smart-amplifier-nocodec"
"sof-jsl-rt5682\;sof-jsl-rt5682-rt1015\;-DPLATFORM=jsl-rt1015"
"sof-jsl-rt5682\;sof-jsl-rt5682-mx98360a\;-DPLATFORM=jsl-dedede"
Expand Down
91 changes: 91 additions & 0 deletions tools/topology/sof-imx8-processing-wm8960.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
#
# Topology with codec_adapter processing component for i.MX8QM/i.MX8QXP
#

# Include topology builder
include(`utils.m4')
include(`dai.m4')
include(`pipeline.m4')
include(`sai.m4')
include(`pcm.m4')
include(`buffer.m4')

# Include TLV library
include(`common/tlv.m4')

# Include Token library
include(`sof/tokens.m4')

# Include DSP configuration
include(`platform/imx/imx8qxp.m4')


# Post process setup config

#codec Post Process setup config
#
# Define the pipelines
#
# PCM0 <----> volume <-----> SAI3 (wm8960)
#


+# Post process setup config
define(`CA_SETUP_CONTROLBYTES',
`` bytes "0x53,0x4f,0x46,0x00,0x00,0x00,0x00,0x00,'
` 0x20,0x00,0x00,0x00,0x00,0x10,0x00,0x03,'
` 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,'
` 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,'
` 0x06,0x01,0xDE,0xCA,0x00,0x00,0x00,0x00,'
` 0x80,0xBB,0x00,0x00,0x20,0x00,0x00,0x00,'
` 0x02,0x00,0x00,0x00,0x00,0x00,0x00,0x00,'
` 0x0C,0x00,0x00,0x00,0x20,0x00,0x00,0x00"''
)

define(`CA_SETUP_CONTROLBYTES_MAX', 300)

undefine(`DAI_PERIODS')
define(`DAI_PERIODS', 16)

dnl PIPELINE_PCM_ADD(pipeline,
dnl pipe id, pcm, max channels, format,
dnl period, priority, core,
dnl pcm_min_rate, pcm_max_rate, pipeline_rate,
dnl time_domain, sched_comp)

# Low Latency playback pipeline 1 on PCM 0 using max 2 channels of s32le.
# Set 1000us deadline on core 0 with priority 0
PIPELINE_PCM_ADD(sof/pipe-codec-adapter-playback.m4,
1, 0, 2, s32le,
1000, 0, 0,
48000, 48000, 48000)

#
# DAIs configuration
#

dnl DAI_ADD(pipeline,
dnl pipe id, dai type, dai_index, dai_be,
dnl buffer, periods, format,
dnl period, priority, core, time_domain)

# playback DAI is SAI3 using 2 periods
# Buffers use s32le format, with 48 frame per 1000us on core 0 with priority 0
DAI_ADD(sof/pipe-dai-playback.m4,
1, SAI, 1, sai1-wm8960-hifi,
PIPELINE_SOURCE_1, 2, s32le,
1000, 0, 0, SCHEDULE_TIME_DOMAIN_DMA)


# PCM Low Latency, id 0
Copy link
Member

@plbossart plbossart Mar 26, 2021

Choose a reason for hiding this comment

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

PCM?

Copy link
Collaborator Author

@dbaluta dbaluta Mar 26, 2021

Choose a reason for hiding this comment

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

@plbossart copy paste of course.


dnl PCM_DUPLEX_ADD(name, pcm_id, playback, capture)
COMPR_PLAYBACK_ADD(Port0, 0, PIPELINE_PCM_1)
Copy link
Member

@plbossart plbossart Mar 26, 2021

Choose a reason for hiding this comment

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

CompressedPort0?

Copy link
Collaborator Author

@dbaluta dbaluta Mar 26, 2021

Choose a reason for hiding this comment

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

Makes sense.


dnl DAI_CONFIG(type, idx, link_id, name, sai_config)
DAI_CONFIG(SAI, 1, 0, sai1-wm8960-hifi,
SAI_CONFIG(I2S, SAI_CLOCK(mclk, 12288000, codec_mclk_in),
SAI_CLOCK(bclk, 3072000, codec_master),
SAI_CLOCK(fsync, 48000, codec_master),
SAI_TDM(2, 32, 3, 3),
SAI_CONFIG_DATA(SAI, 1, 0)))
4 changes: 2 additions & 2 deletions tools/topology/sof/pipe-codec-adapter-playback.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 +115,10 @@ W_CODEC_ADAPTER(0, PIPELINE_FORMAT, DAI_PERIODS, DAI_PERIODS, CA_SCHEDULE_CORE,
LIST(` ', "CA_SETUP_CONTROLBYTES_NAME_PIPE", "CA_RUNTIME_CONTROLBYTES_NAME_PIPE"))

# Playback Buffers
W_BUFFER(0, COMP_BUFFER_SIZE(DAI_PERIODS,
W_BUFFER(0, COMP_BUFFER_SIZE(8,
Copy link
Collaborator

@paulstelian97 paulstelian97 Mar 26, 2021

Choose a reason for hiding this comment

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

We need to figure out what's with this hardcoding.

Copy link
Collaborator Author

@dbaluta dbaluta Mar 26, 2021

Choose a reason for hiding this comment

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

Yes, yes! Will fix it asap.

COMP_SAMPLE_SIZE(PIPELINE_FORMAT), PIPELINE_CHANNELS, COMP_PERIOD_FRAMES(PCM_MAX_RATE, SCHEDULE_PERIOD)),
PLATFORM_HOST_MEM_CAP, SCHEDULE_CORE)
W_BUFFER(1, COMP_BUFFER_SIZE(DAI_PERIODS,
W_BUFFER(1, COMP_BUFFER_SIZE(2,
COMP_SAMPLE_SIZE(DAI_FORMAT), PIPELINE_CHANNELS, COMP_PERIOD_FRAMES(PCM_MAX_RATE, SCHEDULE_PERIOD)),
PLATFORM_DAI_MEM_CAP, SCHEDULE_CORE)

Expand Down