Skip to content

Conversation

@dbaluta
Copy link
Collaborator

@dbaluta dbaluta commented Mar 2, 2021

This is the first PR in the series of preparing support for compressed streams decoding. Mainly, this takes care of adding the codec initialization phase during copy() function.

Initializaton phase for decoders needs to process input frames in order to get a valid header, parse it and figure out the codec parameters. It then also initalizes codec internal state and data structures.

dbaluta added 6 commits March 2, 2021 20:49
With decoders, usually, consumed bytes is not equal
with produced bytes. We need to take this into account
in order to copy the entire produced bytes by the processing algorithm.

- consumed bytes -> number of bytes consumed from the input buffer.
- produced bytes -> number of bytes produced into the output buffer.

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
Initialization of the processing shouldn't be done in the prepare
function as the decoder needs some input frames in order to figure
out decoding parameters.

First step is to just factour out the init process code into
a function and move it to the proper place later.

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
Init process function searches for valid header, does header
decoding to get the parameters and initializes state and configuration
structures.

In order for this to work we need to set the input bytes and also
update consumed bytes.

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
…g_data

We need to save initialization status to a persistent (between calls)
variable in order to check it each time we want to do processing.

Initialization only takes place once before first frames are decoded.

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
This function should be called before processing. It searches
for a valid header, does header decoding to get the parameters
and initializes state and configuration structures.

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
During the initialization of a decoder, the initialization task reads the
input stream to discover the parameters of the encoding.

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
Copy link
Collaborator

@paulstelian97 paulstelian97 left a comment

Choose a reason for hiding this comment

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

I'm a bit at odds with the first commit (the one with the produced/consumed bytes) but I also don't really see anything specifically wrong.

@dbaluta
Copy link
Collaborator Author

dbaluta commented Mar 3, 2021

@paulstelian97 thanks Paul for having a look. As described in the commit message the current implementation only works when the processing algorithm deal with PCM frames. At this point the number of bytes consumed equals the number of bytes produced.

But for MP3 for example, for specific parameters a frame of 572 bytes gets decompressed to 9216 PCM bytes.

@paulstelian97
Copy link
Collaborator

@paulstelian97 thanks Paul for having a look. As described in the commit message the current implementation only works when the processing algorithm deal with PCM frames. At this point the number of bytes consumed equals the number of bytes produced.

But for MP3 for example, for specific parameters a frame of 572 bytes gets decompressed to 9216 PCM bytes.

Yes, I get the intent and of course, this is needed, but the thing is I'm not 100% certain of the implementation (if you missed something or not). Can't figure out if you covered all bases on where this needs to be split.

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

LGTM, lets also keep it easy to converge into component API.

@dbaluta dbaluta changed the title WIP: Prepare for Cadence decoder support Prepare for Cadence decoder support Mar 17, 2021
@dbaluta
Copy link
Collaborator Author

dbaluta commented Mar 17, 2021

@mrajwa is this change OK for your tests with Dolby Audio Processing?

I just finished a first working draft of mp3 patches and I need this to get in in order to send the next batch.

@lgirdwood I need an Approval from Intel on these patches.

@mrajwa I can locally test this for DAP if you just send me some sample files and DAP binary.

@mrajwa
Copy link
Contributor

mrajwa commented Mar 17, 2021

@dbaluta but CI uses DAP as far as I know, @zrombel I don't see post processing tests in CI, what happened with them?

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

@dbaluta ok lets make this teh last codec feature update in codec adapter. We can now work on codec adapter alignment and component/module API merging.

@dbaluta
Copy link
Collaborator Author

dbaluta commented Mar 17, 2021

@lgirdwood This specific patch series doesn't depend on component/module API merging. Anyhow, I still have time until Q2 release so we can first sort out codec adapter alignment and stuff.

I will continue tough updating this PR or maybe create another one for mp3 patches. I want to get an early feedback .

@lgirdwood
Copy link
Member

@dbaluta ok, but I'm good for this now if you are happy here, I just dont want to add extra ops or APIs on top of codec until we can align.

@dbaluta
Copy link
Collaborator Author

dbaluta commented Mar 17, 2021

@lgirdwood I understand your concern. My proposal is:

  • lets merge this BUT need @zrombel to run the Dolby Audio Processing tests
  • lets align on the Cadence API + everything we discussed at the TSC
  • merge mp3 codec patches
  • etc.

Also @cujomalainey I can look starting next week on sorting out things discussed at TSC.

@cujomalainey
Copy link
Contributor

Thanks, I have had a few internal projects pop up that have kept me from finishing #3881. Hoping to get back to it sooner rather than later

@zrombel
Copy link

zrombel commented Mar 18, 2021

All Codec test are a PASS so this PR is good to merge.

@dbaluta dbaluta merged commit e50ae6b into thesofproject:master Mar 18, 2021
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.

7 participants