Re: [alsa-devel] [patch][saa7134] do not change mute state for capturing audio
Em 24-07-2011 14:45, Stas Sergeev escreveu:
23.07.2011 19:09, Mauro Carvalho Chehab wrote:
In this case, it will not be autounmuted after tuning.
Hard to tell about your solution without seeing a patch.
OK, it turns out the automute code is already there, but it doesn't work. The driver for some reasons starts the scan on initialization, finds the carrier:
saa7134[0]/audio: found PAL main sound carrier @ 6.000 MHz [3969/324]
and, because of that, disables the automute. If the real mute is not enabled at that point, you get the white noise right away.
The automute code works fine. Maybe you have a strong interference at the default tuning frequency, leading into saa7134 miss-detection.
For saa7134 driver: 1) There is an audio carrier; 2) an application wants to listen audio; 3) the device is at automute mode. So, when there's an audio carrier, it will unmute the audio at streaming start.
The driver is doing the _right_ thing by letting the audio to flow to PA, when it starts the capture.
Unfortunately, on your specific device, starting audio capture also enables audio at the audio output pin. So, you're noticing a problem.
People with a saa7134 device without alsa stream won't notice it (old devices). People with alsa stream without anything connected to the LINE OUT people aren't noticing it, if PA is not copying the saa7134 PCM IN stream to the sound card PCM out device (the default, for PA).
So, only people that has saa7134 with alsa stream that opted to wire the saa7134 device to the sound card, and with a strong interference at the default tunning frequency (400 MHz) would notice a problem.
Since I have no idea why it finds some carrier, I can't fix that in any way. Or, maybe, not to call the scan on driver init? What will that break?
Analog tuners need to be tuned at the device init on a high frequency according with their datasheets, otherwise the PLL may fail.
Anyway, as long as the automute code is broken, we should either start fixing it, or fix PA, or fix mplayer...
As I always said, the fix should be at PA, as it makes no sense to start capturing at saa7134 without first configuring it. So, or PA should be converted into a V4L2-aware application, in order to properly init the device (with seems to be the wrong approach) or it _SHOULD_NOT_ automatically enable capture on those devices, as this may cause undesired side effects, on the devices that have the capture pin directly wired to the output pin, witch seems to be the case of your device.
PS.: it seems that you've removed Lennart/alsa people from the c/c. I'm re-adding them, as I'm expecting a fix from their side.
Dunno. I wonder how come so many bugs left unfixed for so long, resulting in a white noise to people...
You're the first one that reported it, and the code is there for _years_. So, this is not a commonly noticed problem at all.
Mauro.
24.07.2011 22:36, Mauro Carvalho Chehab wrote:
So, only people that has saa7134 with alsa stream that opted to wire the saa7134 device to the sound card, and with a strong interference at the default tunning frequency (400 MHz) would notice a problem.
No, the "strong interference" thing have nothing to do with it, I think. My card detects signal sometimes, not always. Otherwise it says this: --- saa7134[0]/audio: audio carrier scan failed, using 5.500 MHz [default] --- yet the moise is still there. If you look into a tvaudio_thread() function, you'll notice that it disables automute _unconditionally_! saa7134_tvaudio_do_scan() also disables automute unconditionally. That's why I think there are bugs. Can we start from fixing at least this, and see what happens then?
Since I have no idea why it finds some carrier, I can't fix that in any way. Or, maybe, not to call the scan on driver init? What will that break?
Analog tuners need to be tuned at the device init on a high frequency according with their datasheets, otherwise the PLL may fail.
OK. Maybe, not disabling the automute when the scan was started at init, rather than when it was requested by an app?
You're the first one that reported it, and the code is there for _years_. So, this is not a commonly noticed problem at all.
I am only the first one who reported it _to that list_. I think most other reports were against pulseaudio.
24.07.2011 22:36, Mauro Carvalho Chehab wrote:
The automute code works fine. Maybe you have a strong interference at the default tuning frequency, leading into saa7134 miss-detection.
OK, so my accusation to the automute code is that it gets disabled unconditionally, no matter have the scan failed or succeeded. Also, since that scan is done on driver init, the automute state stands no chance to survive: it is getting disabled unconditionally, on the driver init. Do we agree that this is a bug? Do we agree that fixing it will also fix the PA problem, or, at the very least, will advance us a lot in getting it fixed? If so, can you take a look into fixing that code? It seems the automute code is rather fragile right now, I'd better not touch it if you have some time to take a look.
Hi Mauro, I've finally found the time (and an energy) to go look into the automute breakage. With the attached automute fix I no longer have any problems with pulseaudio. I also attached the patch that introduces an "std" option to limit the scan list, resulting in a faster scan. It is completely unrelated to the automute one, it is here just in case. What do you think?
Em 18-09-2011 12:18, Stas Sergeev escreveu:
Hi Mauro, I've finally found the time (and an energy) to go look into the automute breakage. With the attached automute fix I no longer have any problems with pulseaudio. I also attached the patch that introduces an "std" option to limit the scan list, resulting in a faster scan. It is completely unrelated to the automute one, it is here just in case. What do you think?
Please, one patch per email. Patchwork (or any kernel maintainer script) won't catch more than one patch per email. See:
http://patchwork.linuxtv.org/patch/7862/
As those are the two last patches marked as new at patchwork, I've manually uploaded it as two separate emails, in order to allow me to queue them: http://patchwork.linuxtv.org/patch/7940/ http://patchwork.linuxtv.org/patch/7941/
With respect to this patch: http://patchwork.linuxtv.org/patch/7941/
I don't see any sense on it. Video standard selection is done by software, when a standards mask is passed via VIDIOC_S_STD ioctl. Drivers should not mess it with modprobe hacks.
I'll comment later http://patchwork.linuxtv.org/patch/7940/. It seems to be going into the right direction, but I need to take a deeper code inspection and maybe do some tests here.
Regards, Mauro
24.09.2011 14:57, Mauro Carvalho Chehab wrote:
Please, one patch per email. Patchwork (or any kernel maintainer script) won't catch more than one patch per email. See:
Sorry about that.
With respect to this patch: http://patchwork.linuxtv.org/patch/7941/
I don't see any sense on it. Video standard selection is done by software, when a standards mask is passed via VIDIOC_S_STD ioctl. Drivers should not mess it with modprobe hacks.
Yes, but we already have "secam=" option, and also the first scan, that is being done on driver init, scans too much without that option, and sometimes, unfortunately, detects the PAL carrier for me. By limiting it to secam, I avoid the problem and shorten the scan time. But this patch is not very important, so do whatever you think necessary with it.
I'll comment later http://patchwork.linuxtv.org/patch/7940/. It seems to be going into the right direction, but I need to take a deeper code inspection and maybe do some tests here.
Thanks! Of course, in my view, the _only_ right direction is to export the mute control to the alsa mixer and then fix mplayer. But at least I'm glad I've managed to find the hack that satisfies your opinion and works around the problem at the same time.
Em 24-09-2011 08:12, Stas Sergeev escreveu:
24.09.2011 14:57, Mauro Carvalho Chehab wrote:
Please, one patch per email. Patchwork (or any kernel maintainer script) won't catch more than one patch per email. See:
Sorry about that.
With respect to this patch: http://patchwork.linuxtv.org/patch/7941/
I don't see any sense on it. Video standard selection is done by software, when a standards mask is passed via VIDIOC_S_STD ioctl. Drivers should not mess it with modprobe hacks.
Yes, but we already have "secam=" option, and also the first scan, that is being done on driver init, scans too much without that option, and sometimes, unfortunately, detects the PAL carrier for me. By limiting it to secam, I avoid the problem and shorten the scan time. But this patch is not very important, so do whatever you think necessary with it.
The scan audio logic only enables multiple audio standard detection if the userspace application tells it to do. The right fix here is to fix the application. The secam hack is due to a problem related to Secam L and Secam L'.
I'll comment later http://patchwork.linuxtv.org/patch/7940/. It seems to be going into the right direction, but I need to take a deeper code inspection and maybe do some tests here.
Thanks! Of course, in my view, the _only_ right direction is to export the mute control to the alsa mixer and then fix mplayer. But at least I'm glad I've managed to find the hack that satisfies your opinion and works around the problem at the same time.
The right fix that pulseaudio should not touch at the audio mixers for the video boards. Not all boards have an audio carrier detection like saa7134.
Regards, Mauro.
24.09.2011 16:12, Mauro Carvalho Chehab wrote:
The scan audio logic only enables multiple audio standard detection if the userspace application tells it to do.
No: the _first_ scan is done on the driver init. It is a multi-standard one, and a long one, too. Do we need it at all? It seems to me the results of that scan are not even used, or what am I missing?
Em 24-09-2011 09:36, Stas Sergeev escreveu:
24.09.2011 16:12, Mauro Carvalho Chehab wrote:
The scan audio logic only enables multiple audio standard detection if the userspace application tells it to do.
No: the _first_ scan is done on the driver init. It is a multi-standard one, and a long one, too. Do we need it at all? It seems to me the results of that scan are not even used, or what am I missing?
A first scan at driver's init can be removed, IMO. The thing is that newer versions of udev will open the device, to do a VIDIOC_QUERYCAP. Not sure if this will wake up the tvaudio kthread to do a scan.
Regards, Mauro
24.09.2011 16:48, Mauro Carvalho Chehab wrote:
A first scan at driver's init can be removed, IMO.
OK, that's the great news. Will write a new patch then.
There's nothing the driver can do if the hardware missdetects a carrier. Dirty tricks to try solving it are not good, as they'll do the wrong thing on some situations.
Well, if we assume the first scan can be removed, then we also assume the previous "dirty trick" is harmless, as it affects only the first scan. But I'll better remove both the trick and the first scan then, as the fewer the hacks, the better the code.
If someone is using the board on an environment without udev and pulseaudio, this trick will break the first tuning.
I feel this somehow contradicts with your suggestion to remove the first scan, so could you clarify?
Well, if you think that this would solve, then just write a patch exporting the mute control via ALSA. I have no problems with that.
That would solve all the problems, but only if: 1. The mplayer is then moved to the use of that new control to not depend on the autounmute hack. I can write the patch for that too. 2. Make sure all the other apps are fixed the same way (I hope there are none though) 3. The autounmute hack is then removed. (no regressions if steps 1 and 2 are carefully done)
If you are fine with that plan, then I'll try to find the time and do the things that way. Otherwise, I'll remove the first scan, and that will do the trick in a simpler, though less cleaner way.
Em 24-09-2011 10:20, Stas Sergeev escreveu:
24.09.2011 16:48, Mauro Carvalho Chehab wrote:
A first scan at driver's init can be removed, IMO.
OK, that's the great news. Will write a new patch then.
OK.
There's nothing the driver can do if the hardware missdetects a carrier. Dirty tricks to try solving it are not good, as they'll do the wrong thing on some situations.
Well, if we assume the first scan can be removed, then we also assume the previous "dirty trick" is harmless, as it affects only the first scan. But I'll better remove both the trick and the first scan then, as the fewer the hacks, the better the code.
Yes.
If someone is using the board on an environment without udev and pulseaudio, this trick will break the first tuning.
I feel this somehow contradicts with your suggestion to remove the first scan, so could you clarify?
What I meant to say is that both udev and pulseaudio opens the device, and these might initialize the audio thread. The driver should be able to work the same way with or without the first open by udev/pulseaudio.
Well, if you think that this would solve, then just write a patch exporting the mute control via ALSA. I have no problems with that.
That would solve all the problems, but only if:
- The mplayer is then moved to the use of that new
control to not depend on the autounmute hack. I can write the patch for that too.
The autounmute is not a hack. It is a logic to suppress audio when the audio carrier is not detected. It should not be removed.
I'm not sure if it is safe to make mplayer to use the audio mixer. It is probably a good idea doing that, as it will also work fine with webcams that provide alsa inputs.
- Make sure all the other apps are fixed the same way
(I hope there are none though) 3. The autounmute hack is then removed. (no regressions if steps 1 and 2 are carefully done)
If you are fine with that plan, then I'll try to find the time and do the things that way. Otherwise, I'll remove the first scan, and that will do the trick in a simpler, though less cleaner way.
24.09.2011 19:09, Mauro Carvalho Chehab wrote:
If someone is using the board on an environment without udev and pulseaudio, this trick will break the first tuning.
I feel this somehow contradicts with your suggestion to remove the first scan, so could you clarify?
What I meant to say is that both udev and pulseaudio opens the device, and these might initialize the audio thread. The driver should be able to work the same way with or without the first open by udev/pulseaudio.
But the first scan I was referring to, and am going to remove, happens not on the device open, but on the driver init (modprobe time). open()s are safe, fortunately.
The autounmute is not a hack. It is a logic to suppress audio when the audio carrier is not detected. It should not be removed.
You are confusing the automute and autoUNmute. Autounmute is a must-die hack, and we only need to fix mplayer first. Automute just needs a fix. Though I'd personally remove the automute too, by exporting some interface for an app to query the signal strength... but that's another story. :)
I'm not sure if it is safe to make mplayer to use the audio mixer.
Why, if otherwise it already uses alsa in our case? The mixer control is just another part of an alsa interface, and it is already exported to the v4l apps, so...
It is probably a good idea doing that, as it will also work fine with webcams that provide alsa inputs.
And will make pulseaudio happy, that's for sure. :)
Hello.
24.09.2011 16:48, Mauro Carvalho Chehab wrote:
A first scan at driver's init can be removed, IMO.
Done, sorry for the delay. Patch is attached.
The attached patch fixes the automute logic of saa7134. It avoids the white noise on the pulseaudio startup. (pulseaudio reads the saa7134 alsa device on startup)
Signed-off-by: Stas Sergeev stsp@users.sourceforge.net
Em 18-09-2011 12:18, Stas Sergeev escreveu:
Hi Mauro, I've finally found the time (and an energy) to go look into the automute breakage. With the attached automute fix I no longer have any problems with pulseaudio. I also attached the patch that introduces an "std" option to limit the scan list, resulting in a faster scan. It is completely unrelated to the automute one, it is here just in case. What do you think?
Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [1/2,saa7134] do not change mute state for capturing audio Date: Sun, 18 Sep 2011 14:18:34 -0000 From: Stas Sergeev stsp@list.ru X-Patchwork-Id: 7940 Message-Id: 4E760BCA.6080900-patch1@list.ru To: Mauro Carvalho Chehab mchehab@infradead.org Cc: linux-media@vger.kernel.org, "Nickolay V. Shmyrev" nshmyrev@yandex.ru, Lennart Poettering lpoetter@redhat.com, ALSA devel alsa-devel@alsa-project.org
Hi Mauro, I've finally found the time (and an energy) to go look into the automute breakage. With the attached automute fix I no longer have any problems with pulseaudio. I also attached the patch that introduces an "std" option to limit the scan list, resulting in a faster scan. It is completely unrelated to the automute one, it is here just in case. What do you think?
From ccdfa126e98b5484f4a08de591ac8d89f775251c Mon Sep 17 00:00:00 2001
From: Stas Sergeev stsp@users.sourceforge.net Date: Sun, 18 Sep 2011 19:06:21 +0400 Subject: [PATCH 1/2] saa7134: fix automute
drivers/media/video/saa7134/saa7134-tvaudio.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/media/video/saa7134/saa7134-tvaudio.c b/drivers/media/video/saa7134/saa7134-tvaudio.c index 57e646b..62a6287 100644 --- a/drivers/media/video/saa7134/saa7134-tvaudio.c +++ b/drivers/media/video/saa7134/saa7134-tvaudio.c @@ -332,7 +332,7 @@ static int tvaudio_checkcarrier(struct saa7134_dev *dev, struct mainscan *scan) { __s32 left,right,value;
- if (audio_debug > 1) {
- if (audio_debug > 1 && (dev->tvnorm->id & scan->std)) { int i; dprintk("debug %d:",scan->carr); for (i = -150; i <= 150; i += 30) {
Better to post it as a separate patch, and to simplify the code with:
diff --git a/drivers/media/video/saa7134/saa7134-tvaudio.c b/drivers/media/video/saa7134/saa7134-tvaudio.c index 57e646b..a61ed1e 100644 --- a/drivers/media/video/saa7134/saa7134-tvaudio.c +++ b/drivers/media/video/saa7134/saa7134-tvaudio.c @@ -332,6 +332,12 @@ static int tvaudio_checkcarrier(struct saa7134_dev *dev, struct mainscan *scan) { __s32 left,right,value;
+ if (!dev->tvnorm->id & scan->std)) { + dprintk("skipping %d.%03d MHz [%4s]\n", + scan->carr / 1000, scan->carr % 1000, scan->name); + return 0; + } + if (audio_debug > 1) { int i; dprintk("debug %d:",scan->carr); @@ -348,30 +354,25 @@ static int tvaudio_checkcarrier(struct saa7134_dev *dev, struct mainscan *scan) } printk("\n"); } - if (dev->tvnorm->id & scan->std) { - tvaudio_setcarrier(dev,scan->carr-90,scan->carr-90); - saa_readl(SAA7134_LEVEL_READOUT1 >> 2); - if (tvaudio_sleep(dev,SCAN_SAMPLE_DELAY)) - return -1; - left = saa_readl(SAA7134_LEVEL_READOUT1 >> 2); - - tvaudio_setcarrier(dev,scan->carr+90,scan->carr+90); - saa_readl(SAA7134_LEVEL_READOUT1 >> 2); - if (tvaudio_sleep(dev,SCAN_SAMPLE_DELAY)) - return -1; - right = saa_readl(SAA7134_LEVEL_READOUT1 >> 2); - - left >>= 16; - right >>= 16; - value = left > right ? left - right : right - left; - dprintk("scanning %d.%03d MHz [%4s] => dc is %5d [%d/%d]\n", - scan->carr / 1000, scan->carr % 1000, - scan->name, value, left, right); - } else { - value = 0; - dprintk("skipping %d.%03d MHz [%4s]\n", - scan->carr / 1000, scan->carr % 1000, scan->name); - } + tvaudio_setcarrier(dev,scan->carr-90,scan->carr-90); + saa_readl(SAA7134_LEVEL_READOUT1 >> 2); + if (tvaudio_sleep(dev,SCAN_SAMPLE_DELAY)) + return -1; + left = saa_readl(SAA7134_LEVEL_READOUT1 >> 2); + + tvaudio_setcarrier(dev,scan->carr+90,scan->carr+90); + saa_readl(SAA7134_LEVEL_READOUT1 >> 2); + if (tvaudio_sleep(dev,SCAN_SAMPLE_DELAY)) + return -1; + right = saa_readl(SAA7134_LEVEL_READOUT1 >> 2); + + left >>= 16; + right >>= 16; + value = left > right ? left - right : right - left; + dprintk("scanning %d.%03d MHz [%4s] => dc is %5d [%d/%d]\n", + scan->carr / 1000, scan->carr % 1000, + scan->name, value, left, right); + return value; }
@@ -546,6 +546,7 @@ static int tvaudio_thread(void *data) dev->tvnorm->name, carrier/1000, carrier%1000, max1, max2); dev->last_carrier = carrier;
dev->automute = !(dev->thread.scan1 > 1);
Why?
If the carrier is good, this should be enough:
dev->automute = 0;
} else if (0 != dev->last_carrier) { /* no carrier -- try last detected one as fallback */@@ -553,6 +554,7 @@ static int tvaudio_thread(void *data) dprintk("audio carrier scan failed, " "using %d.%03d MHz [last detected]\n", carrier/1000, carrier%1000);
dev->automute = 1;} else { /* no carrier + no fallback -- use default */
@@ -560,9 +562,9 @@ static int tvaudio_thread(void *data) dprintk("audio carrier scan failed, " "using %d.%03d MHz [default]\n", carrier/1000, carrier%1000);
dev->automute = 1;
dev->automute = 0;@@ -1020,6 +1022,7 @@ int saa7134_tvaudio_init2(struct saa7134_dev *dev) }
dev->thread.thread = NULL;
- dev->thread.scan1 = dev->thread.scan2 = 0; if (my_thread) { saa7134_tvaudio_init(dev); /* start tvaudio thread */
The rest looked sane on my eyes, but I didn't double-checked it by running on my cards. Had you test calling it with just a single standard, and with a multiple standards mask?
Thanks, Mauro
24.09.2011 16:05, Mauro Carvalho Chehab wrote:
Better to post it as a separate patch, and to simplify the code with:
diff --git a/drivers/media/video/saa7134/saa7134-tvaudio.c b/drivers/media/video/saa7134/saa7134-tvaudio.c index 57e646b..a61ed1e 100644 --- a/drivers/media/video/saa7134/saa7134-tvaudio.c +++ b/drivers/media/video/saa7134/saa7134-tvaudio.c @@ -332,6 +332,12 @@ static int tvaudio_checkcarrier(struct saa7134_dev *dev, struct mainscan *scan) { __s32 left,right,value;
- if (!dev->tvnorm->id& scan->std)) {
Missing open bracket?
@@ -546,6 +546,7 @@ static int tvaudio_thread(void *data) dev->tvnorm->name, carrier/1000, carrier%1000, max1, max2); dev->last_carrier = carrier;
dev->automute = !(dev->thread.scan1> 1);Why?
Unfortunately, that's the trick. :(
If the carrier is good, this should be enough:
dev->automute = 0;
Unfortunately, sometimes it misdetects. Testing dev->thread.scan1 means that at least the first scan, done on the driver init, won't unmute. So either that, or this whole patch is unhelpful. I realize that this is a dirty hack, yes.
The rest looked sane on my eyes, but I didn't double-checked it by running on my cards. Had you test calling it with just a single standard, and with a multiple standards mask?
With just a single standard. That's the problem too. There are the fallbacks, like last_carrier etc, and do we need to unmute there or not? :(
The right fix that pulseaudio should not touch at the audio mixers
for the video boards. That's where we disagree. I wonder what other people think. I don't see the compelling reason for making the alsa interface to the v4l devs a special case. If there is just a mute control exported, there is no more a special case, and no more hacks and problems.
Not all boards have an audio carrier detection like saa7134.
Having the mute control exported would make this not a problem.
Em 24-09-2011 09:33, Stas Sergeev escreveu:
24.09.2011 16:05, Mauro Carvalho Chehab wrote:
Better to post it as a separate patch, and to simplify the code with:
diff --git a/drivers/media/video/saa7134/saa7134-tvaudio.c b/drivers/media/video/saa7134/saa7134-tvaudio.c index 57e646b..a61ed1e 100644 --- a/drivers/media/video/saa7134/saa7134-tvaudio.c +++ b/drivers/media/video/saa7134/saa7134-tvaudio.c @@ -332,6 +332,12 @@ static int tvaudio_checkcarrier(struct saa7134_dev *dev, struct mainscan *scan) { __s32 left,right,value;
- if (!dev->tvnorm->id& scan->std)) {
Missing open bracket?
Yes. patch were of course not tested ;)
I meant to say:
if (!(dev->tvnorm->id & scan->std)) {
@@ -546,6 +546,7 @@ static int tvaudio_thread(void *data) dev->tvnorm->name, carrier/1000, carrier%1000, max1, max2); dev->last_carrier = carrier;
dev->automute = !(dev->thread.scan1> 1);Why?
Unfortunately, that's the trick. :(
If the carrier is good, this should be enough:
dev->automute = 0;Unfortunately, sometimes it misdetects.
There's nothing the driver can do if the hardware missdetects a carrier. Dirty tricks to try solving it are not good, as they'll do the wrong thing on some situations.
Testing dev->thread.scan1 means that at least the first scan, done on the driver init, won't unmute. So either that, or this whole patch is unhelpful. I realize that this is a dirty hack, yes.
This is a dirty hack, and it assumes that the first scan should be discarded. This is true only on certain environments. If someone is using the board on an environment without udev and pulseaudio, this trick will break the first tuning.
The rest looked sane on my eyes, but I didn't double-checked it by running on my cards. Had you test calling it with just a single standard, and with a multiple standards mask?
With just a single standard. That's the problem too. There are the fallbacks, like last_carrier etc, and do we need to unmute there or not? :(
The right fix that pulseaudio should not touch at the audio mixers for the video boards.
That's where we disagree. I wonder what other people think. I don't see the compelling reason for making the alsa interface to the v4l devs a special case. If there is just a mute control exported, there is no more a special case, and no more hacks and problems.
Not all boards have an audio carrier detection like saa7134.
Having the mute control exported would make this not a problem.
Well, if you think that this would solve, then just write a patch exporting the mute control via ALSA. I have no problems with that.
Regards, Mauro.
participants (2)
-
Mauro Carvalho Chehab -
Stas Sergeev