All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jaroslav Kysela <perex@perex.cz>
To: Takashi Iwai <tiwai@suse.de>, Pavel Hofman <pavel.hofman@ivitera.com>
Cc: "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>
Subject: Re: pcm_meter.c issue at s16_update
Date: Mon, 3 Aug 2020 09:22:28 +0200	[thread overview]
Message-ID: <142255de-556a-bc73-dfe9-df031fb79b28@perex.cz> (raw)
In-Reply-To: <s5hbljs6yno.wl-tiwai@suse.de>

Dne 03. 08. 20 v 8:17 Takashi Iwai napsal(a):
> 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.

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;
  }

The "hidden" pcm restart referred in the comment should not occur, otherwise
it's another bug somewhere.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

  reply	other threads:[~2020-08-03  7:23 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-26 18:20 pcm_meter.c issue at s16_update Pavel Hofman
2020-07-28 16:46 ` Pavel Hofman
2020-07-28 17:04   ` Takashi Iwai
2020-07-28 18:04     ` Pavel Hofman
2020-07-28 18:54       ` Pavel Hofman
2020-08-02 17:50         ` Pavel Hofman
2020-08-03  6:17           ` Takashi Iwai
2020-08-03  7:22             ` Jaroslav Kysela [this message]
2020-08-03 10:48               ` Pavel Hofman
2020-08-09  7:05                 ` Pavel Hofman
2020-08-09 20:29                   ` Jaroslav Kysela
2020-08-09 21:05                     ` Pavel Hofman
2020-09-15  3:40 Go Peppy
2020-09-17 19:13 ` Pavel Hofman
2020-10-13 17:35   ` Jaroslav Kysela
2020-10-15  3:59     ` Go Peppy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=142255de-556a-bc73-dfe9-df031fb79b28@perex.cz \
    --to=perex@perex.cz \
    --cc=alsa-devel@alsa-project.org \
    --cc=pavel.hofman@ivitera.com \
    --cc=tiwai@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.