Skip to content

Conversation

@MingJenTai
Copy link
Contributor

This PR adds an audio stream layer between sof and RTNR for consistency.

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.

Can the commit message say why you are making the changes and how you are making them otherwise it's difficult for the reviewers to follow.

@cujomalainey
Copy link
Contributor

I would like to hold on this commit till I can get it to work, I have not been successful yet

@MingJenTai
Copy link
Contributor Author

This is because in the main branch the audio_stream structure is modified and is different to the one in other branches such as adl-004-drop-stable. To maintain the compatibility between branches I added a layer between sof and RTNR.

The audio_stream structure might be different between branches. To maintain the compatibility between branches, a layer between sof and RTNR is added.

Signed-off-by: Ming Jen Tai <mingjen_tai@realtek.com>
@MingJenTai MingJenTai force-pushed the main_add_audio_stream_layer branch from 53cae5e to f94f114 Compare March 16, 2022 06:08
@MingJenTai
Copy link
Contributor Author

Can the commit message say why you are making the changes and how you are making them otherwise it's difficult for the reviewers to follow.

The commit message is updated. Thanks!

@lgirdwood
Copy link
Member

This is because in the main branch the audio_stream structure is modified and is different to the one in other branches such as adl-004-drop-stable. To maintain the compatibility between branches I added a layer between sof and RTNR.

ok, but this just adds another layer of indirection. Are you doing this to retain API compatibility or ABI compatibility ?

@cujomalainey
Copy link
Contributor

I would like to hold on this commit till I can get it to work, I have not been successful yet

I am going to retry this now that I have #5570

@lgirdwood
Copy link
Member

I would like to hold on this commit till I can get it to work, I have not been successful yet

I am going to retry this now that I have #5570

Now merged.

@sys-pt1s
Copy link

Can one of the admins verify this patch?

@lgirdwood
Copy link
Member

@MingJenTai any update here ?

@MingJenTai
Copy link
Contributor Author

@MingJenTai any update here ?

Hi @lgirdwood , I think we need to wait for the testing result by @cujomalainey for this PR. Thank you!

@MingJenTai
Copy link
Contributor Author

@MingJenTai any update here ?

Hi @lgirdwood , I think we need to wait for the testing result by @cujomalainey for this PR. Thank you!

@cujomalainey Can this PR be merged now? Thanks.

@lgirdwood
Copy link
Member

@MingJenTai any update here ?

Hi @lgirdwood , I think we need to wait for the testing result by @cujomalainey for this PR. Thank you!

@cujomalainey Can this PR be merged now? Thanks.

@MingJenTai I'm still not following why we need this additional level of wrapping/copies ?

@MingJenTai
Copy link
Contributor Author

Originally RTNR uses audio_stream as an interface between sof and RTNR library as below.

struct audio_stream {
	/* runtime data */
	uint32_t size;	/**< Runtime buffer size in bytes (period multiple) */
	uint32_t avail;	/**< Available bytes for reading */
	uint32_t free;	/**< Free bytes for writing */
	void *w_ptr;	/**< Buffer write pointer */
	void *r_ptr;	/**< Buffer read position */
	void *addr;	/**< Buffer base address */
	void *end_addr;	/**< Buffer end address */

	/* runtime stream params */
	enum sof_ipc_frame frame_fmt;	/**< Sample data format */
	uint32_t rate;		/**< Number of data frames per second [Hz] */
	uint16_t channels;	/**< Number of samples in each frame */

	bool overrun_permitted; /**< indicates whether overrun is permitted */
	bool underrun_permitted; /**< indicates whether underrun is permitted */
};

But audio_stream is changed later in the main branch as below, which makes the structure definition of audio_stream not consistent between branches.

struct audio_stream {
	/* runtime data */
	uint32_t size;	/**< Runtime buffer size in bytes (period multiple) */
	uint32_t avail;	/**< Available bytes for reading */
	uint32_t free;	/**< Free bytes for writing */
	void *w_ptr;	/**< Buffer write pointer */
	void *r_ptr;	/**< Buffer read position */
	void *addr;	/**< Buffer base address */
	void *end_addr;	/**< Buffer end address */

	/* runtime stream params */
	enum sof_ipc_frame frame_fmt;	/**< Sample data format */
	enum sof_ipc_frame valid_sample_fmt;

	uint32_t rate;		/**< Number of data frames per second [Hz] */
	uint16_t channels;	/**< Number of samples in each frame */

	bool overrun_permitted; /**< indicates whether overrun is permitted */
	bool underrun_permitted; /**< indicates whether underrun is permitted */
};

Therefore we decided to abstract this layer as an internal structure in RTNR, so that the interface between sof and RTNR would always be consistent.


#define MicNum 2
#define SpkNum 2

Copy link
Collaborator

Choose a reason for hiding this comment

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

preferred format for subject is:

rtnr: Add audio stream layer...

instead of

[RTNR] Add ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this. Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cujomalainey Can this PR be merged? Thanks.

@MingJenTai MingJenTai changed the title [RTNR] Add audio stream layer between sof and RTNR rtnr: Add audio stream layer between sof and RTNR May 4, 2022
@thesofproject thesofproject deleted a comment from CliffsDover May 11, 2022
@cujomalainey
Copy link
Contributor

Sorry yes, given other issues were traced to other systems, debugged and fixed, we can go ahead here.

@cujomalainey cujomalainey merged commit 4aa202d into thesofproject:main May 13, 2022
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.

5 participants