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 X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 41CC5C433DF for ; Mon, 3 Aug 2020 06:18:43 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id C6C2D20672 for ; Mon, 3 Aug 2020 06:18:42 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=alsa-project.org header.i=@alsa-project.org header.b="Aaja9pRn" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C6C2D20672 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=alsa-devel-bounces@alsa-project.org Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id F3D2D1664; Mon, 3 Aug 2020 08:17:51 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz F3D2D1664 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1596435522; bh=RcZ7qnTgER6eCXbtA2fjWj63gxq/fyrOVnpzTTrOG/U=; h=Date:From:To:Subject:In-Reply-To:References:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=Aaja9pRntd1zxNNWaWY/zAIvnFiPnfvHRILmo5CoIgnt+yyZWOgNy+45b5QBvccyk QZdZwoVCIqNhjNLsgLZ7cDIaGNaGDchsri43m+wx7RXr+G4juQJeW7k2aUM5qXJGJ0 i/TBCkC4bS/aEOHeXvZVJ+f3WG1a1s2TXxjtB+5Q= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id 9FC59F80234; Mon, 3 Aug 2020 08:17:51 +0200 (CEST) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 10F70F8023E; Mon, 3 Aug 2020 08:17:50 +0200 (CEST) Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id 78FFFF800B7 for ; Mon, 3 Aug 2020 08:17:47 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz 78FFFF800B7 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 315DEAF38; Mon, 3 Aug 2020 06:18:02 +0000 (UTC) Date: Mon, 03 Aug 2020 08:17:47 +0200 Message-ID: From: Takashi Iwai To: Pavel Hofman Subject: Re: pcm_meter.c issue at s16_update In-Reply-To: <9bad013a-0306-90e4-adc5-547ebcac1b55@ivitera.com> References: <9957e124-be4b-cdc9-ffad-579b631455df@ivitera.com> <1cd5de43-5f67-78d3-f5e1-bbbaa8856873@ivitera.com> <9bad013a-0306-90e4-adc5-547ebcac1b55@ivitera.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cc: "alsa-devel@alsa-project.org" X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" On Sun, 02 Aug 2020 19:50:44 +0200, Pavel Hofman wrote: > > > Dne 28. 07. 20 v 20:54 Pavel Hofman napsal(a): > > > > Dne 28. 07. 20 v 20:04 Pavel Hofman napsal(a): > >> Dne 28. 07. 20 v 19:04 Takashi Iwai napsal(a): > >>> Would adding atomic_add(&meter->reset, 1) in snd_pcm_meter_reset() > >>> help? > >>> > >> Unfortunately not. > >> > >> s16_reset is called correctly, setting s16->old = meter->now;  But at > >> that time meter->now is still 22751, setting s16->old to the same value. > >> > >> s16_update 1: meter->now 22751, s16->old 22751, size 0 > >> > >> However, in the next update call meter->now comes from the freshly > >> started application pointer: > >> > >> s16_update 1: meter->now 839, s16->old 22751, size -21912 > >> > >> > >> Of course this helps: > >> > >> -       if (size < 0) > >> -               size += spcm->boundary; > >> +       if (size < 0) { > >> +               size = meter->now; > >> +               s16->old = 0; > >> +       } > >> > >> But I understand this is not a solution because: > >> > >> * it will not work at reaching spcm->boundary (after thousands of hours?) > >> * it will cause the same problem when the stream is rewound (which is > >> the problem now too) - size will equal to large meter->now (length from > >> the beginning of the stream minus the rewound = large number). > >> > > > > IMHO there are two cases of the [application pointer + delay] drop > > compared to the previous run: > > > > * stream start, rewinding => s16->old = meter->now; size =0, i.e. > > skipping the samples to show > > * wrapping at spcm->boundary => size += spcm->boundary, i.e. showing the > > wrapped samples > > > > Optionally the second case could be handled just like the first case by > > resetting s16->old, assuming the boundary wrap occurs very infrequently. > > The following patch is tested to work OK, no CPU peaks and no meter > output glitches when the size < 0 condition occurs: > > diff --git a/src/pcm/pcm_meter.c b/src/pcm/pcm_meter.c > index 20b41876..48df5945 100644 > --- a/src/pcm/pcm_meter.c > +++ b/src/pcm/pcm_meter.c > @@ -1098,8 +1098,15 @@ static void s16_update(snd_pcm_scope_t *scope) > snd_pcm_sframes_t size; > snd_pcm_uframes_t offset; > size = meter->now - s16->old; > - if (size < 0) > - size += spcm->boundary; > + if (size < 0) { > + /** > + * Application pointer adjusted for delay (meter->now) > has dropped compared > + * to the previous update cycle. Either spcm->boundary > wraparound, pcm rewinding, > + * or pcm restart without s16->old properly reset. > + * In any case the safest solution is skipping this > conversion cycle. > + */ > + size = 0; > + } > offset = s16->old % meter->buf_size; > while (size > 0) { > snd_pcm_uframes_t frames = size; > > > > Please will you accept this (workaround) bugfix? If so, I would send a > proper patch. It looks OK, at least this must be safe. So yes, I'll happily apply if you submit a proper patch. thanks, Takashi