diff mbox series

[v3,07/10] topology: decode: Fix decoding PCM formats and rates

Message ID 1594725911-14308-8-git-send-email-piotrx.maziarz@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series topology: decode: Various fixes | expand

Commit Message

Piotr Maziarz July 14, 2020, 11:25 a.m. UTC
Not checking _LAST format and rate, which are valid indexes in arrays,
makes data loss while converting binary to standard ALSA configuration
file.

Signed-off-by: Piotr Maziarz <piotrx.maziarz@linux.intel.com>
---
 src/topology/pcm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Pierre-Louis Bossart July 14, 2020, 3:40 p.m. UTC | #1
On 7/14/20 6:25 AM, Piotr Maziarz wrote:
> Not checking _LAST format and rate, which are valid indexes in arrays,
> makes data loss while converting binary to standard ALSA configuration
> file.

I must be really thick on this one.

alsatplg converts from alsa-conf format to binary topology file.
The binary topology file is used by drivers.

In which cases would you convert from binary to alsa-conf files? And 
what tool would you use?


> Signed-off-by: Piotr Maziarz <piotrx.maziarz@linux.intel.com>
> ---
>   src/topology/pcm.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/topology/pcm.c b/src/topology/pcm.c
> index b15b950..db40114 100644
> --- a/src/topology/pcm.c
> +++ b/src/topology/pcm.c
> @@ -549,7 +549,7 @@ int tplg_save_stream_caps(snd_tplg_t *tplg ATTRIBUTE_UNUSED,
>   	if (err >= 0 && sc->formats) {
>   		err = tplg_save_printf(dst, pfx, "\tformats '");
>   		first = 1;
> -		for (i = 0; err >= 0 && i < SND_PCM_FORMAT_LAST; i++) {
> +		for (i = 0; err >= 0 && i <= SND_PCM_FORMAT_LAST; i++) {
>   			if (sc->formats & (1ULL << i)) {
>   				s = snd_pcm_format_name(i);
>   				err = tplg_save_printf(dst, NULL, "%s%s",
> @@ -563,7 +563,7 @@ int tplg_save_stream_caps(snd_tplg_t *tplg ATTRIBUTE_UNUSED,
>   	if (err >= 0 && sc->rates) {
>   		err = tplg_save_printf(dst, pfx, "\trates '");
>   		first = 1;
> -		for (i = 0; err >= 0 && i < SND_PCM_RATE_LAST; i++) {
> +		for (i = 0; err >= 0 && i <= SND_PCM_RATE_LAST; i++) {
>   			if (sc->rates & (1ULL << i)) {
>   				s = get_rate_name(i);
>   				err = tplg_save_printf(dst, NULL, "%s%s",
>
Piotr Maziarz July 15, 2020, 9:37 a.m. UTC | #2
On 2020-07-14 17:40, Pierre-Louis Bossart wrote:
> 
> 
> On 7/14/20 6:25 AM, Piotr Maziarz wrote:
>> Not checking _LAST format and rate, which are valid indexes in arrays,
>> makes data loss while converting binary to standard ALSA configuration
>> file.
> 
> I must be really thick on this one.
> 
> alsatplg converts from alsa-conf format to binary topology file.
> The binary topology file is used by drivers.
> 
> In which cases would you convert from binary to alsa-conf files? And 
> what tool would you use?
> 
./alsatplg --decode topology.bin --output decoded_topology.conf,
This feature was added around the end of 2019. And why to use it? For 
binary topologies to which conf files are lost for example. It's easier 
to analyze and edit it in conf than directly in binary.

> 
>> Signed-off-by: Piotr Maziarz <piotrx.maziarz@linux.intel.com>
>> ---
>>   src/topology/pcm.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/topology/pcm.c b/src/topology/pcm.c
>> index b15b950..db40114 100644
>> --- a/src/topology/pcm.c
>> +++ b/src/topology/pcm.c
>> @@ -549,7 +549,7 @@ int tplg_save_stream_caps(snd_tplg_t *tplg 
>> ATTRIBUTE_UNUSED,
>>       if (err >= 0 && sc->formats) {
>>           err = tplg_save_printf(dst, pfx, "\tformats '");
>>           first = 1;
>> -        for (i = 0; err >= 0 && i < SND_PCM_FORMAT_LAST; i++) {
>> +        for (i = 0; err >= 0 && i <= SND_PCM_FORMAT_LAST; i++) {
>>               if (sc->formats & (1ULL << i)) {
>>                   s = snd_pcm_format_name(i);
>>                   err = tplg_save_printf(dst, NULL, "%s%s",
>> @@ -563,7 +563,7 @@ int tplg_save_stream_caps(snd_tplg_t *tplg 
>> ATTRIBUTE_UNUSED,
>>       if (err >= 0 && sc->rates) {
>>           err = tplg_save_printf(dst, pfx, "\trates '");
>>           first = 1;
>> -        for (i = 0; err >= 0 && i < SND_PCM_RATE_LAST; i++) {
>> +        for (i = 0; err >= 0 && i <= SND_PCM_RATE_LAST; i++) {
>>               if (sc->rates & (1ULL << i)) {
>>                   s = get_rate_name(i);
>>                   err = tplg_save_printf(dst, NULL, "%s%s",
>>
Pierre-Louis Bossart July 15, 2020, 2:37 p.m. UTC | #3
On 7/15/20 4:37 AM, Piotr Maziarz wrote:
> On 2020-07-14 17:40, Pierre-Louis Bossart wrote:
>>
>>
>> On 7/14/20 6:25 AM, Piotr Maziarz wrote:
>>> Not checking _LAST format and rate, which are valid indexes in arrays,
>>> makes data loss while converting binary to standard ALSA configuration
>>> file.
>>
>> I must be really thick on this one.
>>
>> alsatplg converts from alsa-conf format to binary topology file.
>> The binary topology file is used by drivers.
>>
>> In which cases would you convert from binary to alsa-conf files? And 
>> what tool would you use?
>>
> ./alsatplg --decode topology.bin --output decoded_topology.conf,
> This feature was added around the end of 2019. And why to use it? For 
> binary topologies to which conf files are lost for example. It's easier 
> to analyze and edit it in conf than directly in binary.

I must admit I completely missed this feature, thanks for the clarification.
Cezary Rojewski July 15, 2020, 4:18 p.m. UTC | #4
On 2020-07-15 4:37 PM, Pierre-Louis Bossart wrote:
> 
> 
> On 7/15/20 4:37 AM, Piotr Maziarz wrote:
>> On 2020-07-14 17:40, Pierre-Louis Bossart wrote:
>>>
>>>
>>> On 7/14/20 6:25 AM, Piotr Maziarz wrote:
>>>> Not checking _LAST format and rate, which are valid indexes in arrays,
>>>> makes data loss while converting binary to standard ALSA configuration
>>>> file.
>>>
>>> I must be really thick on this one.
>>>
>>> alsatplg converts from alsa-conf format to binary topology file.
>>> The binary topology file is used by drivers.
>>>
>>> In which cases would you convert from binary to alsa-conf files? And 
>>> what tool would you use?
>>>
>> ./alsatplg --decode topology.bin --output decoded_topology.conf,
>> This feature was added around the end of 2019. And why to use it? For 
>> binary topologies to which conf files are lost for example. It's 
>> easier to analyze and edit it in conf than directly in binary.
> 
> I must admit I completely missed this feature, thanks for the 
> clarification.

In general, the idea is to be able to validate or debug (if necessary) 
topology binaries provided by users when access to FE file e.g. conf, or 
XML in our case, is not possible.

In perfect world one can do the following and receive the exact same 
results on each iteration:


(assume FE file in XML format and FE tool e.g. itt which allows for
converting XML into conf)

XML -> itt -> UCM
UCM -> alsatplg -> bin

bin -> alsatplg -> UCM
UCM -> itt -> XML

Ability to compile and decompile was very handy in the Android world. We 
developed a simplified approach quite a while ago and finally decided to 
upstream the solution. Turned out Jaroslav pushed few commits lately 
that make a stub for the idea itself. Unfortunately, as you see in the 
series, there are several problems with the existing code rendering 
--decode unusable. Next step, after the fixes, is to allow for custom 
handlers to be provided (decompiling vendor's private data).

Czarek
diff mbox series

Patch

diff --git a/src/topology/pcm.c b/src/topology/pcm.c
index b15b950..db40114 100644
--- a/src/topology/pcm.c
+++ b/src/topology/pcm.c
@@ -549,7 +549,7 @@  int tplg_save_stream_caps(snd_tplg_t *tplg ATTRIBUTE_UNUSED,
 	if (err >= 0 && sc->formats) {
 		err = tplg_save_printf(dst, pfx, "\tformats '");
 		first = 1;
-		for (i = 0; err >= 0 && i < SND_PCM_FORMAT_LAST; i++) {
+		for (i = 0; err >= 0 && i <= SND_PCM_FORMAT_LAST; i++) {
 			if (sc->formats & (1ULL << i)) {
 				s = snd_pcm_format_name(i);
 				err = tplg_save_printf(dst, NULL, "%s%s",
@@ -563,7 +563,7 @@  int tplg_save_stream_caps(snd_tplg_t *tplg ATTRIBUTE_UNUSED,
 	if (err >= 0 && sc->rates) {
 		err = tplg_save_printf(dst, pfx, "\trates '");
 		first = 1;
-		for (i = 0; err >= 0 && i < SND_PCM_RATE_LAST; i++) {
+		for (i = 0; err >= 0 && i <= SND_PCM_RATE_LAST; i++) {
 			if (sc->rates & (1ULL << i)) {
 				s = get_rate_name(i);
 				err = tplg_save_printf(dst, NULL, "%s%s",