[alsa-devel] Conversion to int16_t and resolution loss with rate converter plugins
Hi Folks,
The libsamplerate and speex rate converter plugins only implement the convert_s16 callback from snd_pcm_rate_ops_t. This has ALSA convert the buffer to int16_t before handing it over to the plugin for resampling. On devices with more than 16 effective bits, this implies words gets truncated and bits are lost.
Specifically for libsamplerate, the resampling is internally implemented with floats, so truncating to 16 bits does not offer a computational benefit.
I get why only convert_s16 is implemented for plugins that aren't part of the main alsa-lib package. The more generic convert callback comes with the burden having the plugin support conversion from/to any of the ALSA data types (or alternatively, having the plugin source include plugin_ops.h and its dependencies). So here's what I propose: replace convert_s16 with a convert_s32 callback which uses int32_t instead of int16_t. This would preserve all bits from the device and keep the plugin interface simple. Of course this means plugins have to be updated to use convert_s32 and rebuilt.
Thoughts?
-Flavio
Hi,
On Apr 5 2018 12:05, Flavio Protasio Ribeiro wrote:
The libsamplerate and speex rate converter plugins only implement the convert_s16 callback from snd_pcm_rate_ops_t. This has ALSA convert the buffer to int16_t before handing it over to the plugin for resampling. On devices with more than 16 effective bits, this implies words gets truncated and bits are lost.
Specifically for libsamplerate, the resampling is internally implemented with floats, so truncating to 16 bits does not offer a computational benefit.
I get why only convert_s16 is implemented for plugins that aren't part of the main alsa-lib package. The more generic convert callback comes with the burden having the plugin support conversion from/to any of the ALSA data types (or alternatively, having the plugin source include plugin_ops.h and its dependencies). So here's what I propose: replace convert_s16 with a convert_s32 callback which uses int32_t instead of int16_t. This would preserve all bits from the device and keep the plugin interface simple. Of course this means plugins have to be updated to use convert_s32 and rebuilt.
Indeed. Current implementation of each of PCM rate plugins have the restriction. All of them get callbacks for buffer with s16 PCM samples. However, there seems to be a reason for what they're, in my opinion.
At present, we have 'snd_pcm_rate_ops_t' as a SDK interface for alsa-lib PCM rate. Let's see layout of the structure.
(include/pcm_rate.h) 58 /** Callback table of rate-converter */ 59 typedef struct snd_pcm_rate_ops { ... 80 /** 81 * convert the data 82 */ 83 void (*convert)(void *obj, 84 const snd_pcm_channel_area_t *dst_areas, 85 snd_pcm_uframes_t dst_offset, unsigned int dst_frames, 86 const snd_pcm_channel_area_t *src_areas, 87 snd_pcm_uframes_t src_offset, unsigned int src_frames); 88 /** 89 * convert an s16 interleaved-data array; exclusive with convert 90 */ 91 void (*convert_s16)(void *obj, int16_t *dst, unsigned int dst_frames, 92 const int16_t *src, unsigned int src_frames); ... 117 } snd_pcm_rate_ops_t;
I understand you suggest to merge these two methods; 'convert' and 'convert_s16'. These two methods were introduced at a commit 33d69ef33be2 ('Create rate converter plugin SDK')[1]. which introduced framework for the SDK.
Here, let me consider about the reason to have added these two methods. In my opinion, the biggest reason comes from consideration about variety of 'snd_pcm_channel_area_t'. This represents layout of buffer for PCM samples. Let me see the detail.
(include/pcm.h) 467 /** PCM area specification */ 468 typedef struct _snd_pcm_channel_area { 469 /** base address of channel samples */ 470 void *addr; 471 /** offset to first sample in bits */ 472 unsigned int first; 473 /** samples distance in bits */ 474 unsigned int step; 475 } snd_pcm_channel_area_t;
When applications parse data of this structure, they can know layout of the buffer; e.g. 'interleaved', 'non-interleaved' and 'complex'. It means that it's not enough for implementation of pcm rate plugin if developers of rate plugin just consider 'interleaved'. For example, some drivers in kernel land use 'non-interleaved' buffer and expose the area data for 'non-interleaved' PCM buffer. Additionally, old rme-related drivers use unique layout of the buffer and expose the area data for this purpose.
On the other hand, according to code comment, 'convert_s16' method is just for 'interleaved' case, thus plugin developers has enough distance of the complexity. I can imagine this comes from practical convenience.
Overall, your idea might push such complication to developers of rate plugin, while in a practical point it covers most of use cases we have. However, at least, we should keep that current design has enough care of the above reasons.
[1] http://git.alsa-project.org/?p=alsa-lib.git;a=commit;h=33d69ef33be2
Thanks
Takashi Sakamoto
Hi Takashi,
Thanks for the detailed explanations.
My original proposal was to replace 'convert_s16' with 'convert_s32', while keeping 'convert'. Looking at your previous thread with Pavel on this topic, recognize your policy of not breaking binary compatibility with previous plugins.
So instead I propose adding a 'convert_s32' method (at the end of snd_pcm_rate_ops_t, with appropriate versioning) that gets called by default if the plugin implements it. The simplified code would look like:
(pcm_rate.c) static void do_convert(const snd_pcm_channel_area_t *dst_areas, snd_pcm_uframes_t dst_offset, unsigned int dst_frames, const snd_pcm_channel_area_t *src_areas, snd_pcm_uframes_t src_offset, unsigned int src_frames, unsigned int channels, snd_pcm_rate_t *rate) { if (rate->ops.convert_s32) { convert_to_s32(rate, rate->src_buf, src_areas, src_offset, src_frames, channels); rate->ops.convert_s32(rate->obj, dst, dst_frames, src, src_frames); convert_from_s32(rate, rate->dst_buf, dst_areas, dst_offset, dst_frames, channels); } else if (rate->ops.convert_s16) { convert_to_s16(rate, rate->src_buf, src_areas, src_offset, src_frames, channels); rate->ops.convert_s16(rate->obj, dst, dst_frames, src, src_frames); convert_from_s16(rate, rate->dst_buf, dst_areas, dst_offset, dst_frames, channels); } else { rate->ops.convert(rate->obj, dst_areas, dst_offset, dst_frames, src_areas, src_offset, src_frames); } }
What do you think? If this looks good to you I can provide a patch.
Thanks, Flavio
-----Original Message----- From: Takashi Sakamoto o-takashi@sakamocchi.jp Sent: Wednesday, April 4, 2018 9:36 PM To: Flavio Protasio Ribeiro flaviopr@microsoft.com; alsa-devel@alsa-project.org Subject: Re: Conversion to int16_t and resolution loss with rate converter plugins
Hi,
On Apr 5 2018 12:05, Flavio Protasio Ribeiro wrote:
The libsamplerate and speex rate converter plugins only implement the convert_s16 callback from snd_pcm_rate_ops_t. This has ALSA convert the buffer to int16_t before handing it over to the plugin for resampling. On devices with more than 16 effective bits, this implies words gets truncated and bits are lost.
Specifically for libsamplerate, the resampling is internally implemented with floats, so truncating to 16 bits does not offer a computational benefit.
I get why only convert_s16 is implemented for plugins that aren't part of the main alsa-lib package. The more generic convert callback comes with the burden having the plugin support conversion from/to any of the ALSA data types (or alternatively, having the plugin source include plugin_ops.h and its dependencies). So here's what I propose: replace convert_s16 with a convert_s32 callback which uses int32_t instead of int16_t. This would preserve all bits from the device and keep the plugin interface simple. Of course this means plugins have to be updated to use convert_s32 and rebuilt.
Indeed. Current implementation of each of PCM rate plugins have the restriction. All of them get callbacks for buffer with s16 PCM samples. However, there seems to be a reason for what they're, in my opinion.
At present, we have 'snd_pcm_rate_ops_t' as a SDK interface for alsa-lib PCM rate. Let's see layout of the structure.
(include/pcm_rate.h) 58 /** Callback table of rate-converter */ 59 typedef struct snd_pcm_rate_ops { ... 80 /** 81 * convert the data 82 */ 83 void (*convert)(void *obj, 84 const snd_pcm_channel_area_t *dst_areas, 85 snd_pcm_uframes_t dst_offset, unsigned int dst_frames, 86 const snd_pcm_channel_area_t *src_areas, 87 snd_pcm_uframes_t src_offset, unsigned int src_frames); 88 /** 89 * convert an s16 interleaved-data array; exclusive with convert 90 */ 91 void (*convert_s16)(void *obj, int16_t *dst, unsigned int dst_frames, 92 const int16_t *src, unsigned int src_frames); ... 117 } snd_pcm_rate_ops_t;
I understand you suggest to merge these two methods; 'convert' and 'convert_s16'. These two methods were introduced at a commit 33d69ef33be2 ('Create rate converter plugin SDK')[1]. which introduced framework for the SDK.
Here, let me consider about the reason to have added these two methods. In my opinion, the biggest reason comes from consideration about variety of 'snd_pcm_channel_area_t'. This represents layout of buffer for PCM samples. Let me see the detail.
(include/pcm.h) 467 /** PCM area specification */ 468 typedef struct _snd_pcm_channel_area { 469 /** base address of channel samples */ 470 void *addr; 471 /** offset to first sample in bits */ 472 unsigned int first; 473 /** samples distance in bits */ 474 unsigned int step; 475 } snd_pcm_channel_area_t;
When applications parse data of this structure, they can know layout of the buffer; e.g. 'interleaved', 'non-interleaved' and 'complex'. It means that it's not enough for implementation of pcm rate plugin if developers of rate plugin just consider 'interleaved'. For example, some drivers in kernel land use 'non-interleaved' buffer and expose the area data for 'non-interleaved' PCM buffer. Additionally, old rme-related drivers use unique layout of the buffer and expose the area data for this purpose.
On the other hand, according to code comment, 'convert_s16' method is just for 'interleaved' case, thus plugin developers has enough distance of the complexity. I can imagine this comes from practical convenience.
Overall, your idea might push such complication to developers of rate plugin, while in a practical point it covers most of use cases we have. However, at least, we should keep that current design has enough care of the above reasons.
[1] https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgit.alsa-pro...
Thanks
Takashi Sakamoto
Hi,
Sorry for delay.
On Apr 6 2018 06:10, Flavio Protasio Ribeiro wrote:
My original proposal was to replace 'convert_s16' with 'convert_s32', while keeping 'convert'. Looking at your previous thread with Pavel on this topic, recognize your policy of not breaking binary compatibility with previous plugins.
So instead I propose adding a 'convert_s32' method (at the end of snd_pcm_rate_ops_t, with appropriate versioning) that gets called by default if the plugin implements it. The simplified code would look like:
(pcm_rate.c) static void do_convert(const snd_pcm_channel_area_t *dst_areas, snd_pcm_uframes_t dst_offset, unsigned int dst_frames, const snd_pcm_channel_area_t *src_areas, snd_pcm_uframes_t src_offset, unsigned int src_frames, unsigned int channels, snd_pcm_rate_t *rate) { if (rate->ops.convert_s32) { convert_to_s32(rate, rate->src_buf, src_areas, src_offset, src_frames, channels); rate->ops.convert_s32(rate->obj, dst, dst_frames, src, src_frames); convert_from_s32(rate, rate->dst_buf, dst_areas, dst_offset, dst_frames, channels); } else if (rate->ops.convert_s16) { convert_to_s16(rate, rate->src_buf, src_areas, src_offset, src_frames, channels); rate->ops.convert_s16(rate->obj, dst, dst_frames, src, src_frames); convert_from_s16(rate, rate->dst_buf, dst_areas, dst_offset, dst_frames, channels); } else { rate->ops.convert(rate->obj, dst_areas, dst_offset, dst_frames, src_areas, src_offset, src_frames); } }
What do you think? If this looks good to you I can provide a patch.
Hm. In my opinion, in a point of existent 'struct snd_pcm_rate_ops.convert()', the new 'struct snd_pcm_rate_ops.convert_s32()' is not necessarily required. Developers can implement conversion with data of any 32 bit physical width as long as implementing buffer handling according to 'snd_pcm_channel_area_t' in backend of PCM plugin.
On the other hand, I recognize the new 'struct snd_pcm_rate_ops.convert_s32()' might be a fast path for cases that sample data is 32 bit physical width and aligned to interleaved (packed) order. I can assume this can cover most use cases with '.convert_s16()'.
However, at first place, it's better to utilize existent '.convert_s32()' because no need to update interface version. When you face performance issues, then work for the new API. Thus at present, each plugin needs to parse 'snd_pcm_channel_area_t' and align sample data according to alignment which used libraries; e.g. libsamplerate requires.
Pavel's patch (0001-Support-for-float-samples-in-rate-converters.patch) converts sample data to/from float/integer inner ALSA PCM rate plugin according to API specification of libsamplerate/libspeex, as he said[1]. However, when considering that libsamplerate/libspeex have conversion helper APIs or perform conversion internally, it's better to delegate the task to judge the way to convert float/interger into each implementation of PCM rate backend. As long as I know, below methods are available for this purpose on below backends:
libsamplerate: * src_short_to_float_array() * src_float_to_short_array() * src_int_to_float_array() * src_float_to_int_array() libspeex: * Below conversion API performs conversion of sample data to float as well. * speex_resampler_process_int() * speex_resampler_process_interleaved_int() libswresample (obsoletes libavresample): * Below conversion API performs conversion of sample data to float as well if correctly configured. * swr_convert()
However, his patch is a good example for how to parse 'snd_pcm_channel_area_t', in my opinion. But I don't avoid to change the other parts; e.g. code on pcm rate plugin itself.
On Apr 6 2018 15:32, Pavel Hofman wrote:
But the existing rate API will have to be modified since libsoxr behaves a bit different to speex or libresample - see the library author discussing the issues
http://mailman.alsa-project.org/pipermail/alsa-devel/2013-September/066764.h...
I get 404 from these links. Could I request more detail about a requirement of modification for libsoxr?
[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2018-April/134065.html
Regards
Takashi Sakamoto
Dne 9.4.2018 v 09:32 Takashi Sakamoto napsal(a):
Hi,
http://mailman.alsa-project.org/pipermail/alsa-devel/2013-September/066764.h...
I get 404 from these links. Could I request more detail about a requirement of modification for libsoxr?
Hi Takashi,
The links work fine for me, but here are alternatives:
http://alsa-devel.alsa-project.narkive.com/u9WYIjpV/pcm-rate-api-question
https://www.google.com/search?q=There%27s+an+ongoing+attempt+to+see+if+libso....
Thanks a lot,
Pavel.
Hi,
On Apr 9 2018 16:38, Pavel Hofman wrote:
Dne 9.4.2018 v 09:32 Takashi Sakamoto napsal(a):
Hi, > http://mailman.alsa-project.org/pipermail/alsa-devel/2013-September/066764.h...
I get 404 from these links. Could I request more detail about a requirement of modification for libsoxr?
Hi Takashi,
The links work fine for me, but here are alternatives:
http://alsa-devel.alsa-project.narkive.com/u9WYIjpV/pcm-rate-api-question
https://www.google.com/search?q=There%27s+an+ongoing+attempt+to+see+if+libso....
Ah... sorry but I mean that two links in the above pages. In short, below URLs: - http://blog.ivitera.com/pavel/linux-audio/pulseaudio-with-ld_preloading-libs... - http://blog.ivitera.com/pavel/linux-audio/simple-use-of-libsoxr-lsr-in-alsa-...
Thanks
Takashi Sakamoto
Dne 9.4.2018 v 10:20 Takashi Sakamoto napsal(a):
Hi,
Ah... sorry but I mean that two links in the above pages. In short, below URLs:
Sorry
Dne 5.4.2018 v 05:05 Flavio Protasio Ribeiro napsal(a):
Hi Folks,
The libsamplerate and speex rate converter plugins only implement the convert_s16 callback from snd_pcm_rate_ops_t. This has ALSA convert the buffer to int16_t before handing it over to the plugin for resampling. On devices with more than 16 effective bits, this implies words gets truncated and bits are lost.
Specifically for libsamplerate, the resampling is internally implemented with floats, so truncating to 16 bits does not offer a computational benefit.
I get why only convert_s16 is implemented for plugins that aren't part of the main alsa-lib package. The more generic convert callback comes with the burden having the plugin support conversion from/to any of the ALSA data types (or alternatively, having the plugin source include plugin_ops.h and its dependencies). So here's what I propose: replace convert_s16 with a convert_s32 callback which uses int32_t instead of int16_t. This would preserve all bits from the device and keep the plugin interface simple. Of course this means plugins have to be updated to use convert_s32 and rebuilt.
Hi,
Many years ago I tried to extend the rate API to float, the native formats of libsamplerate and speex.
I go stuck on float support of all platforms, perhaps you can get something from the discussion and patches.
http://mailman.alsa-project.org/pipermail/alsa-devel/2011-June/041059.html
http://mailman.alsa-project.org/pipermail/alsa-devel/2011-July/041503.html
If we succeed in adding >16 bit API to the rate plugin, perhaps supporting libsoxr would be the next step which would bring great value (performance vs. CPU load) to alsa-lib. Unfortunately that is not just about conversion, there are other quirks to iron out...
Anyway, thanks a lot and good luck to your very useful endeavour.
Best regards,
Pavel.
Hi Pavel,
Thanks for the reference to your previous discussion and the patches. They were super helpful. As you can see from my reply to Takashi, I'm proposing adding a convert_s32 method that doesn't impose dependency on a HW FPU on alsa-lib. The convert_s32 method would operate on an array of int32's.
From the point of view of the plugin developer, the change from convert_s16 to convert_s32 is pretty trivial. The plugins that I personally care about operate internally with floats. The plugin would remain responsible for converting between int32 and float.
I agree that a libsoxr plugin would be great. Getting higher quality real-time resampling is my ultimate goal :)
Thanks, Flavio
-----Original Message----- From: Pavel Hofman pavel.hofman@ivitera.com Sent: Wednesday, April 4, 2018 11:17 PM To: Flavio Protasio Ribeiro flaviopr@microsoft.com; alsa-devel@alsa-project.org Subject: Re: [alsa-devel] Conversion to int16_t and resolution loss with rate converter plugins
Dne 5.4.2018 v 05:05 Flavio Protasio Ribeiro napsal(a):
Hi Folks,
The libsamplerate and speex rate converter plugins only implement the convert_s16 callback from snd_pcm_rate_ops_t. This has ALSA convert the buffer to int16_t before handing it over to the plugin for resampling. On devices with more than 16 effective bits, this implies words gets truncated and bits are lost.
Specifically for libsamplerate, the resampling is internally implemented with floats, so truncating to 16 bits does not offer a computational benefit.
I get why only convert_s16 is implemented for plugins that aren't part of the main alsa-lib package. The more generic convert callback comes with the burden having the plugin support conversion from/to any of the ALSA data types (or alternatively, having the plugin source include plugin_ops.h and its dependencies). So here's what I propose: replace convert_s16 with a convert_s32 callback which uses int32_t instead of int16_t. This would preserve all bits from the device and keep the plugin interface simple. Of course this means plugins have to be updated to use convert_s32 and rebuilt.
Hi,
Many years ago I tried to extend the rate API to float, the native formats of libsamplerate and speex.
I go stuck on float support of all platforms, perhaps you can get something from the discussion and patches.
https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmailman.alsa...
https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmailman.alsa...
If we succeed in adding >16 bit API to the rate plugin, perhaps supporting libsoxr would be the next step which would bring great value (performance vs. CPU load) to alsa-lib. Unfortunately that is not just about conversion, there are other quirks to iron out...
Anyway, thanks a lot and good luck to your very useful endeavour.
Best regards,
Pavel.
Dne 5.4.2018 v 23:21 Flavio Protasio Ribeiro napsal(a):
Hi Pavel,
Thanks for the reference to your previous discussion and the patches. They were super helpful. As you can see from my reply to Takashi, I'm proposing adding a convert_s32 method that doesn't impose dependency on a HW FPU on alsa-lib. The convert_s32 method would operate on an array of int32's.
From the point of view of the plugin developer, the change from convert_s16 to convert_s32 is pretty trivial. The plugins that I personally care about operate internally with floats. The plugin would remain responsible for converting between int32 and float.
Sounds much better than my attempt at float API.
I agree that a libsoxr plugin would be great. Getting higher quality real-time resampling is my ultimate goal :)
If you make it work, you will be praised by countless users :-)
But the existing rate API will have to be modified since libsoxr behaves a bit different to speex or libresample - see the library author discussing the issues http://mailman.alsa-project.org/pipermail/alsa-devel/2013-September/066764.h...
Thanks,
Pavel.
participants (3)
-
Flavio Protasio Ribeiro -
Pavel Hofman -
Takashi Sakamoto