From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 01492C77B75 for ; Wed, 3 May 2023 11:22:12 +0000 (UTC) Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id 7E10E135A; Wed, 3 May 2023 13:21:19 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 7E10E135A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1683112929; bh=EnWLn1vXahO7FiNHDy9I+HhXhc+6FGpUSzUy8Sz7fdQ=; h=Date:From:To:Subject:References:In-Reply-To:CC:List-Id: List-Archive:List-Help:List-Owner:List-Post:List-Subscribe: List-Unsubscribe:From; b=WOiXZpNxC6GTsu80hY224zNji5waGGwVVc7vXGMROSrC0NMW9ERnVthyIj3eJzsQz smCEhm/VKpkUfyneIrgU8lHC1IE53VgvX2hahWWVPl5PE1pPRP7jH346EiiEybm2GC ZfbGGWa712HyvPpaPoMf1lK1zkxgcLgDbFykIY50= Received: from mailman-core.alsa-project.org (mailman-core.alsa-project.org [10.254.200.10]) by alsa1.perex.cz (Postfix) with ESMTP id B7132F8049E; Wed, 3 May 2023 13:20:53 +0200 (CEST) Received: by alsa1.perex.cz (Postfix, from userid 50401) id C16E6F8049E; Wed, 3 May 2023 13:20:48 +0200 (CEST) Received: from bluemchen.kde.org (bluemchen.kde.org [IPv6:2001:470:142:8::100]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id DA903F800EC for ; Wed, 3 May 2023 13:20:41 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz DA903F800EC Received: from ugly.fritz.box (localhost [127.0.0.1]) by bluemchen.kde.org (Postfix) with ESMTP id 0076823FF0; Wed, 3 May 2023 07:20:38 -0400 (EDT) Received: by ugly.fritz.box (masqmail 0.3.4, from userid 1000) id 1puAXF-mQ4-00; Wed, 03 May 2023 13:20:37 +0200 Date: Wed, 3 May 2023 13:20:37 +0200 From: Oswald Buddenhagen To: Jaroslav Kysela Subject: Re: [PATCH alsa-lib 1/4] pcm: hw: setup explicit silencing for snd_pcm_drain by default Message-ID: Mail-Followup-To: Jaroslav Kysela , ALSA development References: <20230502115010.986325-1-perex@perex.cz> <20230502115010.986325-2-perex@perex.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20230502115010.986325-2-perex@perex.cz> Message-ID-Hash: OLK46E73Q2QVRFRQISUP4CDCAYVARIZG X-Message-ID-Hash: OLK46E73Q2QVRFRQISUP4CDCAYVARIZG X-MailFrom: ossi@kde.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; header-match-alsa-devel.alsa-project.org-0; header-match-alsa-devel.alsa-project.org-1; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: ALSA development X-Mailman-Version: 3.3.8 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Tue, May 02, 2023 at 01:50:07PM +0200, Jaroslav Kysela wrote: >Some applications may not follow the period_size transfer blocks and >also the driver developers may not follow the consequeces of the >access beyond valid samples in the playback DMA buffer. > i find this way too vague. >To avoid clicks, fill a little silence at the end of the playback >ring buffer when the snd_pcm_drain() is called. > no 'the' here. (see https://www.eltconcourse.com/training/inservice/lexicogrammar/article_system.html for more than you ever wanted to know about articles.) >--- a/src/pcm/pcm_hw.c >+++ b/src/pcm/pcm_hw.c >@@ -718,7 +732,40 @@ static int snd_pcm_hw_drop(snd_pcm_t *pcm) > static int snd_pcm_hw_drain(snd_pcm_t *pcm) > { > snd_pcm_hw_t *hw = pcm->private_data; >+ snd_pcm_sw_params_t sw_params; >+ snd_pcm_uframes_t silence_size; > int err; >+ >+ if (pcm->stream != SND_PCM_STREAM_PLAYBACK) >+ goto __skip_silence; > arguably, you should use the inverse condition and a block instead of a goto. if this is a measure to keep the indentation down, factoring it out to a separate snd_pcm_hw_auto_silence() function should do the job. >+ /* compute end silence size, align to period size + extra time */ >+ snd_pcm_sw_params_current_no_lock(pcm, &sw_params); > if you swap these lines here, there will be less churn in the followup patch. in the comment, better use a colon instead of a comma. >+ if ((pcm->boundary % pcm->period_size) == 0) { >+ silence_size = pcm->period_size - (*pcm->appl.ptr % pcm->period_size); >+ if (silence_size == pcm->period_size) >+ silence_size = 0; > you can avoid the condition by rewriting it like this: silence_size = pcm->period_size - ((*pcm->appl.ptr + pcm->period_size - 1) % pcm->period_size) - 1; (it may be possible to simplify this further, but this makes my head hurt ...) >+ } else { >+ /* it not not easy to compute the period cross point > "it is not" "crossing point" >+ * in this case because period is not aligned to the boundary > "the period" >+ * - use the full range (one period) in this case >+ */ >+ silence_size = pcm->period_size; >+ } >+ silence_size += pcm->rate / 10; /* 1/10th of second */ >+ if (sw_params.silence_size < silence_size) { >+ /* fill the silence soon as possible (in the bellow ioctl > "as soon as possible" "in the ioctl below" >+ * or the next period wake up) >+ */ >+ sw_params.silence_threshold = pcm->buffer_size; >+ sw_params.silence_size = silence_size; > so at this point i got the thought "huh, that can exceed the buffer size. is that ok?" ... and yes, it is. but ... the kernel doesn't check silence_threshold, so user space can trigger the snd_BUG_ON() in snd_pcm_playback_silence(). whoops. (yes, this predates my patch.) i'm not sure what the best way to deal with this is. anyway, different tree, different patch. regards