Sample format coversion bug?

Sample format coversion bug?

[Alexander E. Patrakov]

Hello,

I am writing an application that uses ALSA, and I don't want to write 
code to support all possible sample formats. On IRC, I asked whether it 
is OK to support only S24 format internally and rely on the "plug" to do 
the required conversion. I got a positive answer, but, when testing my 
application against my SAA7134-based tuner as a capture device, found 
that it doesn't work.

The problem is also reproducible with "arecord". Given that plughw:1 
refers to a SAA7134-based tuner (which obviously doesn't support S24_LE 
natively), running the following command

arecord -vvv -f S24_LE -c 2 -r 32000 -D plughw:1 /dev/null

produces the following output:

ALSA lib pcm_mmap.c:369:(snd_pcm_mmap) mmap failed: Invalid argument
arecord: set_params:961: Unable to install hw params:
ACCESS:  RW_INTERLEAVED
FORMAT:  S24_LE
SUBFORMAT:  STD
SAMPLE_BITS: 32
FRAME_BITS: 64
CHANNELS: 2
RATE: 32000
PERIOD_TIME: 125000
PERIOD_SIZE: 4000
PERIOD_BYTES: 32000
PERIODS: 4
BUFFER_TIME: 500000
BUFFER_SIZE: 16000
BUFFER_BYTES: 128000
TICK_TIME: 4000

This is on Debian, the kernel is 2.6.22-2-amd64, libasound2 is 
1.0.14a-2. Same for S32_LE.

-- 
Alexander E. Patrakov

Re: Sample format coversion bug?

[Takashi Iwai]

At Fri, 26 Oct 2007 19:18:42 +0600,
Alexander E. Patrakov wrote:
> 
> Hello,
> 
> I am writing an application that uses ALSA, and I don't want to write 
> code to support all possible sample formats. On IRC, I asked whether it 
> is OK to support only S24 format internally and rely on the "plug" to do 
> the required conversion. I got a positive answer, but, when testing my 
> application against my SAA7134-based tuner as a capture device, found 
> that it doesn't work.
> 
> The problem is also reproducible with "arecord". Given that plughw:1 
> refers to a SAA7134-based tuner (which obviously doesn't support S24_LE 
> natively), running the following command
> 
> arecord -vvv -f S24_LE -c 2 -r 32000 -D plughw:1 /dev/null
> 
> produces the following output:
> 
> ALSA lib pcm_mmap.c:369:(snd_pcm_mmap) mmap failed: Invalid argument
> arecord: set_params:961: Unable to install hw params:
> ACCESS:  RW_INTERLEAVED
> FORMAT:  S24_LE
> SUBFORMAT:  STD
> SAMPLE_BITS: 32
> FRAME_BITS: 64
> CHANNELS: 2
> RATE: 32000
> PERIOD_TIME: 125000
> PERIOD_SIZE: 4000
> PERIOD_BYTES: 32000
> PERIODS: 4
> BUFFER_TIME: 500000
> BUFFER_SIZE: 16000
> BUFFER_BYTES: 128000
> TICK_TIME: 4000

Well, apparently something wrong with mmap, then.  Does the device
really support 24bit format?  S24_LE isn't 3bytes format but packed
into the lower 3 bytes in 4 byte frames.


Takashi

Re: Sample format coversion bug?

[Alexander E. Patrakov]

Takashi Iwai wrote:
> Well, apparently something wrong with mmap, then.

Do you mean "something is wrong with the kernel driver" or "plug really 
expects mmap access to the card to be available"?

>   Does the device really support 24bit format?

No, it doesn't - that's the whole point of the test. I expected "plug" 
to guess that the device supports a 16-bit format, record 16-bit samples 
from the hardware, multiply all recorded samples by 256, and return the 
result as 32-bit words (so that the result is a set of valid S24_LE 
samples, even though the least significant byte is always zero).

>   S24_LE isn't 3bytes format but packed
> into the lower 3 bytes in 4 byte frames.
>   

Yes, I know.

-- 
Alexander E. Patrakov

Re: Sample format coversion bug?

[Takashi Iwai]

At Mon, 29 Oct 2007 16:01:12 +0500,
Alexander E. Patrakov wrote:
> 
> Takashi Iwai wrote:
> > Well, apparently something wrong with mmap, then.
> 
> Do you mean "something is wrong with the kernel driver" or "plug really 
> expects mmap access to the card to be available"?

The latter case.  Does your driver support mmap?

> >   Does the device really support 24bit format?
> 
> No, it doesn't - that's the whole point of the test. I expected "plug" 
> to guess that the device supports a 16-bit format, record 16-bit samples 
> from the hardware, multiply all recorded samples by 256, and return the 
> result as 32-bit words (so that the result is a set of valid S24_LE 
> samples, even though the least significant byte is always zero).

Then yes, it should work like that.


Takashi

Re: Sample format coversion bug?

[Alexander E. Patrakov]

Takashi Iwai wrote:
> The latter case.  Does your driver support mmap?
>   

Unfortunately, I am at work now, writing from Windows, and cannot check 
for sure. The driver name is "saa7134-alsa", and, according to 
http://linuxtv.org/hg/v4l-dvb/file/6b1892ed1e74/linux/drivers/media/video/saa7134/saa7134-alsa.c, 
it does support mmap.

-- 

Alexander E. Patrakov

Re: Sample format coversion bug?

[Alexander E. Patrakov]

Takashi Iwai wrote:
> Does your driver support mmap?
>   

It looks like the hardware doesn't accept the default parameters:

patrakov@home:~$ arecord -M -f S16_LE -c 2 -r 32000 -D hw:1 /dev/null
Recording WAVE '/dev/null' : Signed 16 bit Little Endian, Rate 32000 Hz, 
Stereo
ALSA lib pcm_mmap.c:369:(snd_pcm_mmap) mmap failed: Invalid argument
arecord: set_params:961: Unable to install hw params:
ACCESS:  MMAP_INTERLEAVED
FORMAT:  S16_LE
SUBFORMAT:  STD
SAMPLE_BITS: 16
FRAME_BITS: 32
CHANNELS: 2
RATE: 32000
PERIOD_TIME: 125000
PERIOD_SIZE: 4000
PERIOD_BYTES: 16000
PERIODS: 4
BUFFER_TIME: 500000
BUFFER_SIZE: 16000
BUFFER_BYTES: 64000
TICK_TIME: 4000

Even though mmap seems to be supported in the driver source (judging 
from .info = SNDRV_PCM_INFO_MMAP | other flags), I could not figure out 
the parameters that work. Moreover, the driver seems to accept the same 
hardware parameters without the mmap, and I could not find parameters 
that the driver accepts for mmap.

Also: I have retested my original problem with the virtual ens1370 card 
emulated by qemu. The original testcase does work, so the problem seems 
to be specific to saa7134, due to non-working mmap access. Could you 
please help me transform my observations into something that the v4l 
development list will accept as a good bug report?

One more question: since plug doesn't work at all on devices without 
mmap available, does this mean that I should scrap my original idea 
about using plug to convert everything into S24?

-- 
Alexander E. Patrakov

Re: Sample format coversion bug?

[Trent Piepho]

On Mon, 29 Oct 2007, Alexander E. Patrakov wrote:
> Even though mmap seems to be supported in the driver source (judging
> from .info = SNDRV_PCM_INFO_MMAP | other flags), I could not figure out
> the parameters that work. Moreover, the driver seems to accept the same
> hardware parameters without the mmap, and I could not find parameters
> that the driver accepts for mmap.

I do not think the saa7134-alsa driver supports mmap.  The cx88-alsa driver
also claimed to support mmap, but it never worked until I fixed it.  It's
pretty clear that the code in saa7134-alsa was based on the same code as
cx88-alsa, so it's likely it has the same bug.

> One more question: since plug doesn't work at all on devices without
> mmap available, does this mean that I should scrap my original idea
> about using plug to convert everything into S24?

Does plug require mmap?  It does it just require that devices which claim
to support mmap actually work in mmap mode?

Re: mmap support in saa7134-alsa (was: Sample format coversion bug?)

[Alexander E. Patrakov]

Trent Piepho wrote:
> On Mon, 29 Oct 2007, Alexander E. Patrakov wrote:
>   
>> Even though mmap seems to be supported in the driver source (judging
>> from .info = SNDRV_PCM_INFO_MMAP | other flags), I could not figure out
>> the parameters that work. Moreover, the driver seems to accept the same
>> hardware parameters without the mmap, and I could not find parameters
>> that the driver accepts for mmap.
>>     
>
> I do not think the saa7134-alsa driver supports mmap.  The cx88-alsa driver
> also claimed to support mmap, but it never worked until I fixed it.  It's
> pretty clear that the code in saa7134-alsa was based on the same code as
> cx88-alsa, so it's likely it has the same bug.
>   

You are right. The patch below (based on your cx88 patch, but I don't 
really understand it) fixes mmap support in saa7134-alsa for me. 
Recording via mmap (arecord -M -f S16_LE -c 2 -r 32000 -D hw:1) didn't 
work at all before, works now, tested for at least 20 minutes (but, 
unfortunately, with one overrun at least 0.719 ms long).

Signed-off-by: Alexander E. Patrakov <patrakov@ums.usu.ru>

--- saa7134-alsa.c	2007-10-12 22:43:44.000000000 +0600
+++ saa7134-alsa.c	2007-10-30 21:01:10.000000000 +0500
@@ -544,8 +544,10 @@
 	   V4L functions, and force ALSA to use that as the DMA area */
 
 	substream->runtime->dma_area = dev->dmasound.dma.vmalloc;
+	substream->runtime->dma_bytes = dev->dmasound.bufsize;
+	substream->runtime->dma_addr = 0;
 
-	return 1;
+	return 0;
 
 }
 
@@ -653,6 +655,17 @@
 }
 
 /*
+ * page callback (needed for mmap)
+ */
+
+static struct page *snd_card_saa7134_page(struct snd_pcm_substream *substream,
+					unsigned long offset)
+{
+	void *pageptr = substream->runtime->dma_area + offset;
+	return vmalloc_to_page(pageptr);
+}
+
+/*
  * ALSA capture callbacks definition
  */
 
@@ -665,6 +678,7 @@
 	.prepare =		snd_card_saa7134_capture_prepare,
 	.trigger =		snd_card_saa7134_capture_trigger,
 	.pointer =		snd_card_saa7134_capture_pointer,
+	.page =			snd_card_saa7134_page,
 };
 
 /*



-- 
Alexander E. Patrakov

Re: mmap support in saa7134-alsa (was: Sample format coversion bug?)

[Takashi Iwai]

At Tue, 30 Oct 2007 21:24:44 +0500,
Alexander E. Patrakov wrote:
> 
> Trent Piepho wrote:
> > On Mon, 29 Oct 2007, Alexander E. Patrakov wrote:
> >   
> >> Even though mmap seems to be supported in the driver source (judging
> >> from .info = SNDRV_PCM_INFO_MMAP | other flags), I could not figure out
> >> the parameters that work. Moreover, the driver seems to accept the same
> >> hardware parameters without the mmap, and I could not find parameters
> >> that the driver accepts for mmap.
> >>     
> >
> > I do not think the saa7134-alsa driver supports mmap.  The cx88-alsa driver
> > also claimed to support mmap, but it never worked until I fixed it.  It's
> > pretty clear that the code in saa7134-alsa was based on the same code as
> > cx88-alsa, so it's likely it has the same bug.
> >   
> 
> You are right. The patch below (based on your cx88 patch, but I don't 
> really understand it) fixes mmap support in saa7134-alsa for me. 
> Recording via mmap (arecord -M -f S16_LE -c 2 -r 32000 -D hw:1) didn't 
> work at all before, works now, tested for at least 20 minutes (but, 
> unfortunately, with one overrun at least 0.719 ms long).
> 
> Signed-off-by: Alexander E. Patrakov <patrakov@ums.usu.ru>

Looks good to me.

Acked-by: Takashi Iwai <tiwai@suse.de>


Takashi


> --- saa7134-alsa.c	2007-10-12 22:43:44.000000000 +0600
> +++ saa7134-alsa.c	2007-10-30 21:01:10.000000000 +0500
> @@ -544,8 +544,10 @@
>  	   V4L functions, and force ALSA to use that as the DMA area */
>  
>  	substream->runtime->dma_area = dev->dmasound.dma.vmalloc;
> +	substream->runtime->dma_bytes = dev->dmasound.bufsize;
> +	substream->runtime->dma_addr = 0;
>  
> -	return 1;
> +	return 0;
>  
>  }
>  
> @@ -653,6 +655,17 @@
>  }
>  
>  /*
> + * page callback (needed for mmap)
> + */
> +
> +static struct page *snd_card_saa7134_page(struct snd_pcm_substream *substream,
> +					unsigned long offset)
> +{
> +	void *pageptr = substream->runtime->dma_area + offset;
> +	return vmalloc_to_page(pageptr);
> +}
> +
> +/*
>   * ALSA capture callbacks definition
>   */
>  
> @@ -665,6 +678,7 @@
>  	.prepare =		snd_card_saa7134_capture_prepare,
>  	.trigger =		snd_card_saa7134_capture_trigger,
>  	.pointer =		snd_card_saa7134_capture_pointer,
> +	.page =			snd_card_saa7134_page,
>  };
>  
>  /*
> 
> 
> 
> -- 
> Alexander E. Patrakov
>