[RFC] make codec adapter stand in layer #3980

cujomalainey · 2021-03-29T21:55:46Z

continuation of #3881

Essentially these steps

Clean up adapter
Abstract adapter out of components
Generate UUIDs for existing adapters
Fix setup config fields

In order to act as a stand in layer for codecs going forward we need to not log using our comp_driver as that will be removed in the future. Instead we need to log using the context of comp_dev. This commit also reduces the verbosity of a lot of the traces and increases a few. Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>

not sure why this was added. There are very few cases where a static declaration in a header makes sense but this does not appear to be one of them. Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>

This commit strips the codec adapter as it is more of a library/layer for codecs to simplify their interface against rather than a component in and of itself. This gives each codec its own UUID solving the ID problem while opening an opertunity to register codecs directly for future multiplexing capabilities. Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>

dbaluta · 2021-03-30T07:58:34Z

src/audio/codec_adapter/codec/passthrough.c

+	.free = passthrough_codec_free
+};
+
+DECLARE_CODEC_ADAPTER(passthrough_interface, passthrough_uuid, passthrough_tr)


You will need a semicolon here, otherwise this won't compile. I am also looking at this. Let me know if you want me to take this over and send the next PR.

I have tested cadence codec adapter with the previous version of your PR and it works.

Ah that is what is missing, thanks i was struggling with that. I will update this once we figure out the corruption issues in the adapter. I don't want to land anything until those are fixed.

@cujomalainey btw, how do you generate the UUID?

@dbaluta I think the simple "uuidgen" tool is used, should work well enough.

i mean, these are not cryptographically secure, you could use a webtool if desired

mrajwa · 2021-03-30T08:56:38Z

src/audio/codec_adapter/codec_adapter.c

 	uint32_t buff_size; /* size of local buffer */

-	comp_info(dev, "codec_adapter_prepare() start");
+	comp_dbg(dev, "codec_adapter_prepare() start");


do we really want these not very frequent logs to be dbg? I would keep it as it is.

+1 for keeping functions that are called very rarely with comp_info(). Of course we wont add comp_info to copy() function for example.

@dbaluta you merged this change for other components such as volume control, we should have a consistent stance on logging levels so components change in unison 8212900

@cujomalainey Indeed, it looks like there is no clear rule when to use _info vs _dbg. I've noticed this in several components.

I am personally fine with all callbacks except prepare,copy and reset being of level info. I think those three should remain dbg. That is just my 2cents

mrajwa · 2021-03-30T09:04:10Z

src/include/sof/audio/codec_adapter/interfaces.h

 #endif

 #define CADENCE_ID 0xCADE01
-#define DUMMY_ID   0xD03311


shouldn't we also get rid of CADENCE_ID and WAVES_ID ?

@mrajwa will do this but gradually. This commit only deals with ~~Dummy~~ passthrough codec.

I will add the rest, this patch is still WIP, currently debugging fmt and corruption bugs

mrajwa · 2021-03-30T10:03:20Z

src/audio/codec_adapter/codec/passthrough.c


-int passthrough_codec_init(struct comp_dev *dev)
+/* 0495ef10-254b-473d-8cbf-3d2292945c24 */
+DECLARE_SOF_RT_UUID("passthrough_codec", passthrough_uuid, 0x0495ef10, 0x254b, 0x473d,


I don't like the passthru name here, I would stick to the adapter name as passthru suggest literally no processing. Also I miss the general picture, how are we going to approach the scenario where codecs are dynamically binded with the adapter base on their UUID?

@mrajwa adapter base is now just some sort of library helper functions. Each codec will create their own component based on UUID. The binding you mentioned is created statically inside the code like this:

static const struct comp_driver comp_codec_adapter = { \ .type = SOF_COMP_NONE, \ .uid = SOF_RT_UUID(uuid), \ .tctx = &tr, \ .ops = { \ .create = adapter_shim_new, \ .prepare = codec_adapter_prepare, \ .params = codec_adapter_params, \ .copy = codec_adapter_copy, \ .cmd = codec_adapter_cmd, \ .trigger = codec_adapter_trigger, \ .reset = codec_adapter_reset, \ .free = codec_adapter_free, \ }, \ }; \

The codec adapter interface is more open to change than the component interface, therefore we can register that later in my macro and then use that to dynamically switch based on information we add into the interface. This is just a step toward using only UUIDs as identification.

dbaluta · 2021-04-01T17:28:43Z

src/audio/codec_adapter/codec_adapter.c

+ * \return integer representing either:
+ *	0 -> success
+ *	negative value -> failure.
+ */


@cujomalainey is this function just moved around? Because the diff looks odd.

dbaluta · 2021-04-01T17:49:01Z

@cujomalainey I continued your work here. https://github.com/dbaluta/sof/tree/refactor_ca

Rebased your PR on latest origin/master and made the code to work for passthrough codec. Tommorrow will finish the other two codecs and sent a PR if that's OK with you.

cujomalainey · 2021-04-02T05:41:32Z

@cujomalainey I continued your work here. https://github.com/dbaluta/sof/tree/refactor_ca

Rebased your PR on latest origin/master and made the code to work for passthrough codec. Tommorrow will finish the other two codecs and sent a PR if that's OK with you.

@dbaluta thanks, just discovered the core issue, am blocked internally and wont have time to finish this

cujomalainey added 3 commits March 19, 2021 15:19

codec_adapter: remove unneeded declaration

658a532

not sure why this was added. There are very few cases where a static declaration in a header makes sense but this does not appear to be one of them. Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>

dbaluta reviewed Mar 30, 2021

View reviewed changes

mrajwa reviewed Mar 30, 2021

View reviewed changes

mrajwa suggested changes Mar 30, 2021

View reviewed changes

dbaluta reviewed Apr 1, 2021

View reviewed changes

cujomalainey closed this Apr 2, 2021

[RFC] make codec adapter stand in layer #3980

[RFC] make codec adapter stand in layer #3980

Uh oh!

Conversation

cujomalainey commented Mar 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbaluta commented Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cujomalainey commented Apr 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dbaluta commented Apr 1, 2021 •

edited

Loading