Skip to content
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.

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

codec_adapter: prepare for dynamic codec id #4572

Closed
wants to merge 1 commit into from

Conversation

Potochi
Copy link
Contributor

@Potochi Potochi commented Jul 29, 2021

For now the codec id is determined from the topology file, this
doesn't scale well when we have to support a lot of codecs.

The earlyest time when we can get the codec id is in
"cadence_codec_prepare". So we move some of the codec initialisation
from "cadence_codec_init" into "cadence_codec_prepare".

The current topologies which specify the codec id directly will still
work, new topologies that want to use this functionality must specify
"0" as the codec id.

The codec id should be sent by the linux kernel. Until this will happen
we assume the default codec id to be for MP3.

Signed-off-by: Bud Liviu-Alexandru budliviu@gmail.com

@sofci
Copy link
Collaborator

sofci commented Jul 29, 2021

Can one of the admins verify this patch?

reply test this please to run this test once

@dbaluta
Copy link
Collaborator

dbaluta commented Jul 29, 2021

test this please

/* req_api_id will be passed via parameters from the linux
* kernel and it will contain the api_id of the file format.
*/
uint32_t req_api_id = CADENCE_CODEC_MP3_DEC_ID;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this some sort of default?

Copy link
Collaborator

Choose a reason for hiding this comment

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

As mentioned in the commit message, we use this as default for now because codec id should be sent from the kernel side. Why don't have a clean way to do that now.

Copy link
Contributor

@mrajwa mrajwa left a comment

Choose a reason for hiding this comment

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

"we assume the default codec id to be for MP3." This does not sound good - the default shall be just pass through.

@dbaluta
Copy link
Collaborator

dbaluta commented Jul 30, 2021

@mrajwa that sounds good. As far as I remember we do not support passthrough with Cadence libraries. Should we create a dummy library which exposes a function that just copies input to output?

@mrajwa
Copy link
Contributor

mrajwa commented Jul 30, 2021

@dbaluta by passthrough I mean no lib was selected at all here so we just copy in -> out. I don't get why Mp3 codec (or any other) should be assumed as the default one if none was specified.

@dbaluta
Copy link
Collaborator

dbaluta commented Jul 30, 2021

@mrajwa i understood you from your first comment. But we still need to plugin this passthrough functionality in the codec_adapter framework.

We need to understand at what level we implement the passthrough:

  • at codec adapter level
  • OR at little bit down at specific Cadence level.

Notice that we have UUID that identifies the library type (e.g Dolby, Cadence) and then we also have codec id inside Cadence which identifies which particular API / library for Cadence is used.

Indeed you are right, we didn't took into cosnsideration the passthrough case so for this reason we considered MP3 as default case.

For now the codec id is determined from the topology file, this
doesn't scale well when we have to support a lot of codecs.

The earlyest time when we can get the codec id is in
"cadence_codec_prepare". So we move some of the codec initialisation
from "cadence_codec_init" into "cadence_codec_prepare".

The current topologies which specify the codec id directly will still
work, new topologies that want to use this functionality must specify
"0" as the codec id.

The codec id should be sent by the linux kernel. Until this will happen
we assume the default codec id to be for MP3.

Signed-off-by: Bud Liviu-Alexandru <budliviu@gmail.com>
@dbaluta
Copy link
Collaborator

dbaluta commented Aug 2, 2021

@mrajwa we had a deeper look at this and your comment suggests that we should implement the passthrough functionality using the Cadence API, this is not really what we intended with this commit. Maybe we can leave this for later.

Why, I'm advocating for using an already implemented codec ID for now is that actually that values shouldn't matter once we send the parameters from kernel side.

We don't have yet a field for codec_id inside parameters structure passed from the kernel.

We can eventually go with a default like:

 	uint32_t req_api_id = CADENCE_CODEC_WRAPPER_ID;

what do you say?

@lgirdwood
Copy link
Member

@mrajwa ping - need your inputs.

@lgirdwood lgirdwood added this to the v1.9 milestone Aug 6, 2021
@mrajwa
Copy link
Contributor

mrajwa commented Aug 10, 2021

@dbaluta if the codec selection is not complete in any way we should set the adapter to pass thru mode where we only copy input to output. I would not make any assumption about unspecified codec id.

Sorry for late responses I am on vacations currently.

@dbaluta
Copy link
Collaborator

dbaluta commented Aug 10, 2021

@mrajwa lets move this to v1.10 milestone, as there are a lot of things needed to be discussed. Including Kernel side changes.

@dbaluta dbaluta modified the milestones: v1.9, v1.10 Aug 10, 2021
@lgirdwood
Copy link
Member

@dbaluta ping ?

@dbaluta
Copy link
Collaborator

dbaluta commented Nov 15, 2021

@lgirdwood thanks for reminder. we were busy with some bugfixing. will send this week a follow up to this patch.

@dbaluta dbaluta closed this Jan 21, 2022
@dbaluta
Copy link
Collaborator

dbaluta commented Jan 21, 2022

Replaced with #5245

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants