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=-10.0 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham 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 042E5C433E0 for ; Sun, 9 Aug 2020 07:06:54 +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 7617320768 for ; Sun, 9 Aug 2020 07:06:53 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=alsa-project.org header.i=@alsa-project.org header.b="cmXrVOmy"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=ivitera.com header.i=@ivitera.com header.b="sQfO2Fdl"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=ivitera.com header.i=@ivitera.com header.b="A3lRWb4j" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7617320768 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ivitera.com 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 E8BAC15E2; Sun, 9 Aug 2020 09:06:01 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz E8BAC15E2 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1596956812; bh=k8yMOT7T1og/ToDKwtv1pODNrfSH7Ear44U5CHZ1E6Y=; h=Subject:From:To:References:Date:In-Reply-To:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=cmXrVOmy7CmU2E3aqU/yuaaS0wzQPKk1Jp5p7Vd0/at9Ji78GlX5MMGbHTg/G2g0A E5+wLaGc6AmXPgecND+Vy3AV9GOCfLt+/FObl0/dON3UnwhClMVxJO3z/3QBhxy4GA PoG0A0KukhZMmFSTsEST4r1Jbm2R/03YgF8YkLF4= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id 5C732F80139; Sun, 9 Aug 2020 09:06:01 +0200 (CEST) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 2C559F8015B; Sun, 9 Aug 2020 09:05:59 +0200 (CEST) Received: from cable.insite.cz (cable.insite.cz [84.242.75.189]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id 9A5FBF800BC for ; Sun, 9 Aug 2020 09:05:49 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz 9A5FBF800BC Authentication-Results: alsa1.perex.cz; dkim=pass (1024-bit key) header.d=ivitera.com header.i=@ivitera.com header.b="sQfO2Fdl"; dkim=pass (1024-bit key) header.d=ivitera.com header.i=@ivitera.com header.b="A3lRWb4j" Received: from localhost (localhost [127.0.0.1]) by cable.insite.cz (Postfix) with ESMTP id AFEEFA2296ED3; Sun, 9 Aug 2020 09:05:46 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=ivitera.com; s=mail; t=1596956746; bh=k8yMOT7T1og/ToDKwtv1pODNrfSH7Ear44U5CHZ1E6Y=; h=Subject:From:To:Cc:References:Date:In-Reply-To:From; b=sQfO2Fdl87S3FD2LUuIUV1bGnB+n9SUj3O3KFOQNZpMcEh7ZQNoJUXogikSu8eEEJ poZ89/iWAV/xxmXYFHFGo103pZea0L1FVUqdFvt5mgY9SIgHcQAeWfKfjoN7tdJj52 xrZNK6xB5A1pAw04xXI0XkR0Lg6gFKVZqdhaqTZ8= Received: from cable.insite.cz ([84.242.75.189]) by localhost (server.insite.cz [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id H6jGAfFd6MiP; Sun, 9 Aug 2020 09:05:41 +0200 (CEST) Received: from [192.168.105.151] (ip28.insite.cz [81.0.237.28]) (Authenticated sender: pavel) by cable.insite.cz (Postfix) with ESMTPSA id 0EFCBA1F65A2D; Sun, 9 Aug 2020 09:05:41 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=ivitera.com; s=mail; t=1596956741; bh=k8yMOT7T1og/ToDKwtv1pODNrfSH7Ear44U5CHZ1E6Y=; h=Subject:From:To:Cc:References:Date:In-Reply-To:From; b=A3lRWb4jlbNg3RpQMDTxEjx9AJibx7qc8nlIidBeRA715paJJUiNP004/1GQUK7HE QTQmjW54pmUsg8gvv2KwPcVMdOgU5Qn6eGVfVqZ/YQ8VPu09+EVnobyS1tK8F/7vLf GNaih4H98yl8kJAbqUulI1QaXH7BmCd+8mbOEazg= Subject: Re: pcm_meter.c issue at s16_update From: Pavel Hofman To: Jaroslav Kysela , Takashi Iwai References: <9957e124-be4b-cdc9-ffad-579b631455df@ivitera.com> <1cd5de43-5f67-78d3-f5e1-bbbaa8856873@ivitera.com> <9bad013a-0306-90e4-adc5-547ebcac1b55@ivitera.com> <142255de-556a-bc73-dfe9-df031fb79b28@perex.cz> Message-ID: Date: Sun, 9 Aug 2020 09:05:40 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US 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" Dne 03. 08. 20 v 12:48 Pavel Hofman napsal(a): > > > Dne 03. 08. 20 v 9:22 Jaroslav Kysela napsal(a): >> Dne 03. 08. 20 v 8:17 Takashi Iwai napsal(a): >>> On Sun, 02 Aug 2020 19:50:44 +0200, >>>>> >>>>> 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. >> >> It would be probably better to check against the boundary / 2 value to >> check >> correctly the boundary wrap instead to drop all negative size values: >> >>    if (size < 0) { >>       if (size < -(spcm->boundary / 2)) >>          size += spcm->boundary; >>       else >>          size = 0; >>    } > > Is there a reliable way to detect the boundary wraparound, at best using > some dedicated API? I could find any, IMO the wraparound does not create > any notification. The check is OK for a rewind, half of boundary is > usually a very large number too. I am not sure what would happen at > reset when application pointer was already past the boundary half - see > below. > >> >> The "hidden" pcm restart referred in the comment should not occur, >> otherwise >> it's another bug somewhere. > > I do not know the exact moments when plugin API methods are called. The > fact is Takashi's suggestion to call s16 reset explicitely in > snd_pcm_meter_reset created this order: > > snd_pcm_meter_reset -> s16->reset > s16_update: meter->now 22751, s16->old 22751, size 0 > s16_update: meter->now 839, s16->old 22751, size -21912 > > I.e. AFTER resetting meter/s16 the variable meter->now was still at the > original large 22751 (with s16->old equal to its value due to > s16->reset). The value of meter->now was reset to 839 (= app pointer - > delay) only in the next call of s16_update (when s16->old was still the > previous old value => size < 0 => huge size => high CPU load).  From > this I kind of conclude that the reset is buggy. Maybe the reset code > should re-calculate meter->now = appl.pointer - delay before aligning > s16->old = meter->now. > > Nevertheless all this (except for the boundary wraparound) would result > in the same size = 0, thus skipping samples from the last cycle, just > like what the proposed patch does. > > Please can we reach a decision and close the problem so that affected use cases do not have to be patched with the next the alsa-lib version? Thanks a lot in advance, Pavel.