Plugin support in tinycompress #6

rohkkumar · 2020-08-26T09:57:58Z

These are initial set of patches to add the plugins support to tinycompress.

This pull request corresponds to #5

dbaluta · 2020-08-27T07:30:22Z

src/lib/utils/snd_utils.c

@@ -0,0 +1,140 @@
+/* snd_utils.c
+**
+** Copyright (c) 2019, The Linux Foundation. All rights reserved.


Can we switch to SPDX identifiers?

sure .. we can do that .. only thing is current files has not moved to SPDX identifiers. So followed the same.

Cc: @perexg. I would say we could start using SPDX identifiers and add a mental note to change existing files later.

dbaluta · 2020-08-27T07:42:38Z

@rohkkumar do you have any dummy decoding .so library to better understand the interface?

For example it is not very clear to me how do you decode an mp3 frame.

So, with the current interface would be possible to input an mp3 file and get the corresponding uncompressed pcm file resulted after applying the decoding algorithm?

We have this legacy interface: https://github.com/FreeSrk/codec/blob/master/ghdr/common/fsl_unia.h

It offers the following interface:

typedef int32 (*UniACodec_decode_frame) (UniACodec_Handle pua_handle,
                                         uint8 * InputBuf,
                                         uint32 InputSize,
                                         uint32 * offset,
                                         uint8 ** OutputBuf,
                                         uint32 *OutputSize);

dbaluta · 2020-08-27T07:44:22Z

src/lib/compress.c

+#include "compress_ops.h"
+#ifdef TINYCOMPRESS_USES_PLUGINS
+#include "utils/snd_utils.h"
+#endif


So, do you need to recompile the library to use plugins? Isn't possible to determine at runtime if one should use plugins or real hw?

Actually, our initial implementation had the same design where virtual(plugin) device was decided if the hw node is present or not and if there is virtual node in configuration file. But as discussed in PR for tinyalsa tinyalsa/tinyalsa#137 , we got a feedback to handle it using compile time flag too so that it can be disabled for those who don't want plugin support.

@rohkkumar ok.

rohkkumar · 2020-08-27T07:57:38Z

@rohkkumar do you have any dummy decoding .so library to better understand the interface?

For example it is not very clear to me how do you decode an mp3 frame.

So, with the current interface would be possible to input an mp3 file and get the corresponding uncompressed pcm file resulted after applying the decoding algorithm?

We have this legacy interface: https://github.com/FreeSrk/codec/blob/master/ghdr/common/fsl_unia.h

It offers the following interface:
typedef int32 (*UniACodec_decode_frame) (UniACodec_Handle pua_handle,
                                         uint8 * InputBuf,
                                         uint32 InputSize,
                                         uint32 * offset,
                                         uint8 ** OutputBuf,
                                         uint32 *OutputSize);

Yes, I have implemented a ffmpeg based example plugin to decode FLAC data to PCM and do tinyalsa calls. I will plan to post that out.

Thanks

dbaluta · 2020-08-27T10:38:09Z

@rohkkumar Great, looking forward to read the code for that example.

rohkkumar · 2020-08-28T10:05:42Z

Updated patches with SPDX identifiers and added ffmpeg based plugin example.

rohkkumar · 2020-08-28T10:24:38Z

src/utils-lgpl/ffmpeg_compress_plugin.c

+
+		/* initialize the resampling context */
+		if ((ret = swr_init(swr_ctx)) < 0) {
+			fprintf(stderr, "Failed to initialize the resampling context\n");


Reference taken from https://github.com/FFmpeg/FFmpeg/blob/master/doc/examples/decode_audio.c and https://github.com/FFmpeg/FFmpeg/blob/master/doc/examples/resampling_audio.c

vinodkoul · 2020-09-09T04:36:27Z

@perexg can you please review as well, we need to make sure this aligns with overall way we want alsa plugins to behave...

vinodkoul · 2020-09-09T04:43:04Z

src/lib/utils/snd_utils.c

@@ -0,0 +1,117 @@
+// SPDX-License-Identifier: GPL-2.0-only


So this needs to be modified. The Tinycompress project is dual licensed as LGPL and BSD. This license is incompatible to this project, so I can not consider this change. Please reconsider licensing as LGPL or BSD or both.

Sure @vinodkoul , I will update the license.

vinodkoul

License needs to be clarified before we forward on these patches. Right now it is incompatible

perexg · 2020-09-09T09:09:24Z

I have problem with 'libsndcardparser.so' references. It's not a standard library. It's a bit pity that compress_open() does not accept the device name (string) as argument - it's better for applications. I see the possibility to use a configuration file with card/device number to shared library mapping or special .so lookups in compress_open(). Also, the environment variable may be used to override the hardware.

Also, the ops functions may copy the exported functions to let implementation on the plugin itself. The exported API for tinycompress is simple and straight, so if I understand the problem correctly, the functionality should be only redirected for given card/device to another plugin/library if configured. Why use internals like ops->ioctl(), ops->poll() ?

rohkkumar · 2020-09-10T07:05:17Z

I have problem with 'libsndcardparser.so' references. It's not a standard library. It's a bit pity that compress_open() does not accept the device name (string) as argument - it's better for applications. I see the possibility to use a configuration file with card/device number to shared library mapping or special .so lookups in compress_open(). Also, the environment variable may be used to override the hardware.

@perexg , we are planning to open libsndcardparser and will be hosted soon.
Regarding compress_open(), we have not changed the interface and applications will still be able to use the same API. tinycompress clients are not supposed to call compress_hw.c/compress_plugin.c APIs directly. Instead clients needs to pass virtual compress device node id. We can change compress->ops->open to pass string but with that compress_plugin.c will have to again parse the string and get the card and device number. Please suggest.

Also, the ops functions may copy the exported functions to let implementation on the plugin itself. The exported API for tinycompress is simple and straight, so if I understand the problem correctly, the functionality should be only redirected for given card/device to another plugin/library if configured. Why use internals like ops->ioctl(), ops->poll() ?

The problem with calling the plugin library ops directly from compress.c is that for all the ops we would need to check if it is a virtual node or a real hw node and call either the ioctl on hw device node or ops->ioctl to plugin library. This will lead to too many if/else statements in compress.c . Currently, we set the ops to either compress_hw_ops or compress_plugin_ops at compress_open() and then use it at all other places. This way, changes in compress.c is cleaner and all plugin related implementation is handled in compress_plugin.c . It will be more inline with tinyalsa plugins where plugin related common implementation is done in mixer_plugin.c (https://github.com/tinyalsa/tinyalsa/blob/master/src/mixer_plugin.c). Please let us know your opinion.

rohkkumar · 2020-09-10T09:09:01Z

License needs to be clarified before we forward on these patches. Right now it is incompatible

Fixed License in all files.

perexg · 2020-09-10T12:53:43Z

I have problem with 'libsndcardparser.so' references. It's not a standard library. It's a bit pity that compress_open() does not accept the device name (string) as argument - it's better for applications. I see the possibility to use a configuration file with card/device number to shared library mapping or special .so lookups in compress_open(). Also, the environment variable may be used to override the hardware.

@perexg , we are planning to open libsndcardparser and will be hosted soon.

I'm sorry, but this scheme and libtinyalsa scheme looks like a big hack to create emulated cards / devices.

My problem is the dependency on another library at the time. You're proposing:

libtinycompress -> libsndcardparser -> some redirection

I'm proposing:

libtinycompress -> redirection via open -> plugin system based on your libsndcardparser (but it should not be part of tinycompress)

I don't think that libtinyalsa nor libtinycompress should be dependent on third abstraction. Just create a straight way in open functions (compress_open) to redirect the functionality to somewhere else. And then implement your plugin system based on some special libsndcardparser code out-of-tree, if you like. My proposal allows other implementations (not only libsndcardparser one), too.

The much cleaner solution is to implement the counterpart to pcm_open_by_name() for example (compress_open_by_name()) and handle virtual names like 'mp3decoder:0,0' where mp3decoder specifies the plugin and the extra arguments specifies the output PCM card and device numbers. In this case, you may try to dynamically load libtinycompress_mp3decoder.so which will implement this decoder.

Regarding compress_open(), we have not changed the interface and applications will still be able to use the same API. tinycompress clients are not supposed to call compress_hw.c/compress_plugin.c APIs directly. Instead clients needs to pass virtual compress device node id. We can change compress->ops->open to pass string but with that compress_plugin.c will have to again parse the string and get the card and device number. Please suggest.

Also, the ops functions may copy the exported functions to let implementation on the plugin itself. The exported API for tinycompress is simple and straight, so if I understand the problem correctly, the functionality should be only redirected for given card/device to another plugin/library if configured. Why use internals like ops->ioctl(), ops->poll() ?

The problem with calling the plugin library ops directly from compress.c is that for all the ops we would need to check if it is a virtual node or a real hw node and call either the ioctl on hw device node or ops->ioctl to plugin library. This will lead to too many if/else statements in compress.c .

The virtual devices may be identified by special device names. I just say, that for this case, you can just do (for all exported functions in tinycompress.h which touches som):

int compress_resume(struct compress *compress)
{
      if (!compress->ops->resume)
              return -ENXIO;
      return compress->ops->resume(compress);
}

... and don't deal with kernel APIs like ioctl/poll etc. Keep this only in compress_hw.c . Use the upper level abstraction also for the plugin interface.

rohkkumar · 2020-09-11T08:08:40Z

I'm sorry, but this scheme and libtinyalsa scheme looks like a big hack to create emulated cards / devices.

My problem is the dependency on another library at the time. You're proposing:

libtinycompress -> libsndcardparser -> some redirection

I'm proposing:

libtinycompress -> redirection via open -> plugin system based on your libsndcardparser (but it should not be part of tinycompress)

I don't think that libtinyalsa nor libtinycompress should be dependent on third abstraction. Just create a straight way in open functions (compress_open) to redirect the functionality to somewhere else. And then implement your plugin system based on some special libsndcardparser code out-of-tree, if you like. My proposal allows other implementations (not only libsndcardparser one), too.

The much cleaner solution is to implement the counterpart to pcm_open_by_name() for example (compress_open_by_name()) and handle virtual names like 'mp3decoder:0,0' where mp3decoder specifies the plugin and the extra arguments specifies the output PCM card and device numbers. In this case, you may try to dynamically load libtinycompress_mp3decoder.so which will implement this decoder.

Thanks @perexg for the suggestion. It does solve the problem and reduces a lot of code. But I think there are certain limitation with this approach like:

With current approach, we can use compress_open() for plugin which is not possible if we use compress_open_by_name() to denote the library name.
This would need existing tinycompress clients to change the code if they want to use plugin as they won't be able to use compress_open().

Another reference for configuration file is alsa-lib which parses alsa.conf and uses the information.
I understand that this dependency on libsndcardparser should be avoided but we had kept it as a separate library as it was supposed to be common for tinyalsa and tinycompress.
Please provide your inputs.

The virtual devices may be identified by special device names. I just say, that for this case, you can just do (for all exported functions in tinycompress.h which touches som):
int compress_resume(struct compress *compress)
{
      if (!compress->ops->resume)
              return -ENXIO;
      return compress->ops->resume(compress);
}
... and don't deal with kernel APIs like ioctl/poll etc. Keep this only in compress_hw.c . Use the upper level abstraction also for the plugin interface.

Sure. Actually, our intention was :

To make plugin behave as userspace driver.
To reuse common code in compress.c and avoid code duplication. e.g. - compress_write/compress_get_hpointer/etc has some handling in compress.c which now needs to be implemented in the plugin library again if we use upper level abstraction.
Please let me know your opinion.
I can start making the changes based on your suggestion.

rohkkumar · 2020-09-18T07:52:44Z

@perexg , I was evaluating your proposal and changes needed to handle all ops.
I found that with new design, existing is_codec_supported() API will not work for plugins. One solution to that is to expose another API similar to compress_open_by_name(). Another solution can be to get the codec_support after compress_open_by_name(). In that case, we need to pass struct compress * to get the info. Can you please provide your suggestion.

Other issue which I see with new approach is that:
Currently struct compress is not exposed to external components (https://github.com/alsa-project/tinycompress/blob/master/src/lib/compress.c#L87) . So, if we make Ops abstraction at upper layer, the common code and variables defined in struct compress cannot be used for compress_hw.c. In that case, we will have to move these to compress_hw.c. Does this sounds ok ?
This can be avoided if he have ops abstraction at same layer which is currently implemented.

perexg · 2020-09-18T09:07:33Z

@perexg , I was evaluating your proposal and changes needed to handle all ops.
I found that with new design, existing is_codec_supported() API will not work for plugins. One solution to that is to expose another API similar to compress_open_by_name(). Another solution can be to get the codec_support after compress_open_by_name(). In that case, we need to pass struct compress * to get the info. Can you please provide your suggestion.

I would add a new function is_codec_supported_by_name() for this case.

The other way may be separate open with the params setup and capabilities check (open -> codec supported? -> configure -> I/O). But it will make the current API incompatible. It's much cleaner solution in my eyes.

Perhaps, we can add compress_open_by_name() without 'struct compr_config' config argument and add new functions compress_set_params() and compress_is_codec_supported() and obsolete the card/device based functions.

@vinodkoul : Do you think that we can change the current compress API? I would prefer to separate the parameter setup from open.

Other issue which I see with new approach is that:
Currently struct compress is not exposed to external components (https://github.com/alsa-project/tinycompress/blob/master/src/lib/compress.c#L87) . So, if we make Ops abstraction at upper layer, the common code and variables defined in struct compress cannot be used for compress_hw.c. In that case, we will have to move these to compress_hw.c. Does this sounds ok ?
This can be avoided if he have ops abstraction at same layer which is currently implemented.

Yes, the 'struct compress' should be private to implementation. I don't see a problem to move it to compress_hw.c completely.

vinodkoul · 2020-09-22T05:48:24Z

@perexg , I was evaluating your proposal and changes needed to handle all ops.
I found that with new design, existing is_codec_supported() API will not work for plugins. One solution to that is to expose another API similar to compress_open_by_name(). Another solution can be to get the codec_support after compress_open_by_name(). In that case, we need to pass struct compress * to get the info. Can you please provide your suggestion.

I would add a new function is_codec_supported_by_name() for this case.

The other way may be separate open with the params setup and capabilities check (open -> codec supported? -> configure -> I/O). But it will make the current API incompatible. It's much cleaner solution in my eyes.

Perhaps, we can add compress_open_by_name() without 'struct compr_config' config argument and add new functions compress_set_params() and compress_is_codec_supported() and obsolete the card/device based functions.

@vinodkoul : Do you think that we can change the current compress API? I would prefer to separate the parameter setup from open.

@perexg yes we can add new APIs for that, I would not change the old API as we have users for these.
So adding news one should be fine.

I think we should have compress_open_by_name()

Other issue which I see with new approach is that:
Currently struct compress is not exposed to external components (https://github.com/alsa-project/tinycompress/blob/master/src/lib/compress.c#L87) . So, if we make Ops abstraction at upper layer, the common code and variables defined in struct compress cannot be used for compress_hw.c. In that case, we will have to move these to compress_hw.c. Does this sounds ok ?
This can be avoided if he have ops abstraction at same layer which is currently implemented.

Yes, the 'struct compress' should be private to implementation. I don't see a problem to move it to compress_hw.c completely.
Agree, we dont want to expose that to users

Add compress_ops.h needed to support plugins. Compress_ops can be used to jump to either hw compress node ops or to virtual compress node ops. Signed-off-by: Rohit kumar <rohitkr@codeaurora.org>

rohkkumar · 2020-09-22T12:57:54Z

I would add a new function is_codec_supported_by_name() for this case.
The other way may be separate open with the params setup and capabilities check (open -> codec supported? -> configure -> I/O). But it will make the current API incompatible. It's much cleaner solution in my eyes.
Perhaps, we can add compress_open_by_name() without 'struct compr_config' config argument and add new functions compress_set_params() and compress_is_codec_supported() and obsolete the card/device based functions.
@vinodkoul : Do you think that we can change the current compress API? I would prefer to separate the parameter setup from open.

@perexg yes we can add new APIs for that, I would not change the old API as we have users for these.
So adding news one should be fine.

I think we should have compress_open_by_name()

Thanks @vinodkoul and @perexg for the suggestions. I have updated the PR accordingly.

rohkkumar · 2020-09-23T07:29:30Z

Moved parsing of compress_name to an API as it is used by compress_open_by_name() and compress_is_codec_supported_by_name()

rohkkumar · 2020-09-29T04:24:57Z

@perexg , Can you please check if updated PR has proper changes.

perexg · 2020-09-30T08:26:00Z

The overall change looks good. Some little comment: I won't insist to pass card number and device numbers to plugins, they should just receive the abstract compress device name and do own parsing. For example, we may be prepared for compress devices like: net:192.168.1.10:9999.

Add compress_hw.c to handle all operations associated with HW compress nodes. Move handling to compress_hw using compress_hw_ops. Signed-off-by: Rohit kumar <rohitkr@codeaurora.org>

rohkkumar · 2020-10-01T08:21:54Z

The overall change looks good. Some little comment: I won't insist to pass card number and device numbers to plugins, they should just receive the abstract compress device name and do own parsing. For example, we may be prepared for compress devices like: net:192.168.1.10:9999.

Thanks @perexg for the suggestion. Updated PR accordingly. Please review.

src/lib/compress_hw.c

include/tinycompress/compress_ops.h

src/lib/compress.c

vinodkoul · 2020-10-01T10:03:57Z

@charleskeepax I think you folks also have some stuff on top of this lib, can you guys test and confirm this is okay

rohkkumar · 2020-10-02T11:37:36Z

Addressed comments. Please review.

rohkkumar · 2020-10-08T05:52:54Z

@vinodkoul / @perexg / @charleskeepax , can you please check if there is any concern with latest patchset.

vinodkoul · 2020-10-13T13:27:36Z

@perexg any comments?

src/lib/compress.c

Add compress_open_by_name() and is_codec_supported_by_name() to support plugins. Format of name is 'hw:<card>,<device>' for hw compress nodes and '<plugin_name>:<custom_data>' for virtual compress nodes. It dynamically loads the plugin library whose name is libtinycompress_module_<plugin_name>.so. Plugin library needs to expose compress_plugin_ops. Default path of plugin lib is /usr/lib/tinycompress-lib/ and it can be updated by defining TINYCOMPRESS_PLUGIN_DIR in makefile. Signed-off-by: Rohit kumar <rohitkr@codeaurora.org>

dbaluta reviewed Aug 27, 2020

View reviewed changes

rohkkumar force-pushed the plugin_support branch from f58f3a3 to 415406c Compare August 28, 2020 10:04

rohkkumar commented Aug 28, 2020

View reviewed changes

rohkkumar force-pushed the plugin_support branch from 415406c to 28eadca Compare September 7, 2020 05:52

vinodkoul reviewed Sep 9, 2020

View reviewed changes

vinodkoul requested changes Sep 9, 2020

View reviewed changes

rohkkumar force-pushed the plugin_support branch from 28eadca to f348d71 Compare September 10, 2020 09:08

include: tinycompress: Add compress_ops.h

3f4832e

Add compress_ops.h needed to support plugins. Compress_ops can be used to jump to either hw compress node ops or to virtual compress node ops. Signed-off-by: Rohit kumar <rohitkr@codeaurora.org>

rohkkumar force-pushed the plugin_support branch from f348d71 to 3454482 Compare September 22, 2020 12:55

rohkkumar force-pushed the plugin_support branch 2 times, most recently from 01f0e60 to d89c51e Compare September 23, 2020 07:28

src: lib: Move functionality to compress_hw.c

d132bb6

Add compress_hw.c to handle all operations associated with HW compress nodes. Move handling to compress_hw using compress_hw_ops. Signed-off-by: Rohit kumar <rohitkr@codeaurora.org>

rohkkumar force-pushed the plugin_support branch from d89c51e to 64e7e9f Compare October 1, 2020 08:17

perexg requested changes Oct 1, 2020

View reviewed changes

src/lib/compress_hw.c Outdated Show resolved Hide resolved

src/lib/compress_hw.c Outdated Show resolved Hide resolved

vinodkoul reviewed Oct 1, 2020

View reviewed changes

include/tinycompress/compress_ops.h Show resolved Hide resolved

vinodkoul reviewed Oct 1, 2020

View reviewed changes

include/tinycompress/compress_ops.h Show resolved Hide resolved

vinodkoul reviewed Oct 1, 2020

View reviewed changes

include/tinycompress/compress_ops.h Show resolved Hide resolved

vinodkoul reviewed Oct 1, 2020

View reviewed changes

src/lib/compress.c Outdated Show resolved Hide resolved

rohkkumar force-pushed the plugin_support branch from 64e7e9f to 2dbaf37 Compare October 2, 2020 11:35

perexg requested changes Oct 13, 2020

View reviewed changes

src/lib/compress.c Outdated Show resolved Hide resolved

src/lib/compress.c Outdated Show resolved Hide resolved

src/lib/compress.c Show resolved Hide resolved

rohkkumar force-pushed the plugin_support branch from 2dbaf37 to cffaaf9 Compare October 15, 2020 06:46

perexg approved these changes Oct 15, 2020

View reviewed changes

vinodkoul merged commit 22f1152 into alsa-project:master Oct 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plugin support in tinycompress #6

Plugin support in tinycompress #6

rohkkumar commented Aug 26, 2020

dbaluta Aug 27, 2020

rohkkumar Aug 27, 2020

dbaluta Aug 27, 2020

dbaluta commented Aug 27, 2020

dbaluta Aug 27, 2020

rohkkumar Aug 27, 2020

dbaluta Aug 27, 2020

rohkkumar commented Aug 27, 2020

dbaluta commented Aug 27, 2020 •

edited

rohkkumar commented Aug 28, 2020

rohkkumar Aug 28, 2020

vinodkoul commented Sep 9, 2020

vinodkoul Sep 9, 2020

rohkkumar Sep 10, 2020

vinodkoul left a comment

perexg commented Sep 9, 2020

rohkkumar commented Sep 10, 2020

rohkkumar commented Sep 10, 2020

perexg commented Sep 10, 2020

rohkkumar commented Sep 11, 2020 •

edited

rohkkumar commented Sep 18, 2020 •

edited

perexg commented Sep 18, 2020

vinodkoul commented Sep 22, 2020

rohkkumar commented Sep 22, 2020

rohkkumar commented Sep 23, 2020

rohkkumar commented Sep 29, 2020

perexg commented Sep 30, 2020

rohkkumar commented Oct 1, 2020

vinodkoul commented Oct 1, 2020

rohkkumar commented Oct 2, 2020

rohkkumar commented Oct 8, 2020

vinodkoul commented Oct 13, 2020

Plugin support in tinycompress #6

Plugin support in tinycompress #6

Conversation

rohkkumar commented Aug 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbaluta commented Aug 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohkkumar commented Aug 27, 2020

dbaluta commented Aug 27, 2020 • edited

rohkkumar commented Aug 28, 2020

Choose a reason for hiding this comment

vinodkoul commented Sep 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinodkoul left a comment

Choose a reason for hiding this comment

perexg commented Sep 9, 2020

rohkkumar commented Sep 10, 2020

rohkkumar commented Sep 10, 2020

perexg commented Sep 10, 2020

rohkkumar commented Sep 11, 2020 • edited

rohkkumar commented Sep 18, 2020 • edited

perexg commented Sep 18, 2020

vinodkoul commented Sep 22, 2020

rohkkumar commented Sep 22, 2020

rohkkumar commented Sep 23, 2020

rohkkumar commented Sep 29, 2020

perexg commented Sep 30, 2020

rohkkumar commented Oct 1, 2020

vinodkoul commented Oct 1, 2020

rohkkumar commented Oct 2, 2020

rohkkumar commented Oct 8, 2020

vinodkoul commented Oct 13, 2020

dbaluta commented Aug 27, 2020 •

edited

rohkkumar commented Sep 11, 2020 •

edited

rohkkumar commented Sep 18, 2020 •

edited