All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mike Brady <mikebrady@eircom.net>
To: Takashi Iwai <tiwai@suse.de>
Cc: Kirill Marinushkin <k.marinushkin@gmail.com>,
	stefan.wahren@i2se.com, devel@driverdev.osuosl.org,
	alsa-devel@alsa-project.org, f.fainelli@gmail.com,
	eric@anholt.net, gregkh@linuxfoundation.org,
	linux-kernel@vger.kernel.org, julia.lawall@lip6.fr,
	linux-rpi-kernel@lists.infradead.org,
	nishka.dasgupta_ug18@ashoka.edu.in,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [alsa-devel] [PATCH v2] staging: bcm2835-audio: interpolate audio delay
Date: Sun, 28 Oct 2018 14:26:12 +0000	[thread overview]
Message-ID: <126FA055-050B-4AAC-9392-CA8CCA821768@eircom.net> (raw)
In-Reply-To: <s5hftwuo4t0.wl-tiwai@suse.de>



> On 25 Oct 2018, at 08:37, Takashi Iwai <tiwai@suse.de> wrote:
> 
> On Thu, 25 Oct 2018 00:02:34 +0200,
> Kirill Marinushkin wrote:
>> 
>>>> When you play sound - the pointer increments.
>>> 
>>> Unfortunately, when you play sound, the pointer does not actually increment, for up to about 10 milliseconds. I know of no way to actually access the true “live” position of the frame that is being played at any instant; hence the desire to estimate it.
>>> 
>> 
>> Your vision of situation in the opposite from my vision. What you see as a
>> symptom - I see as a root cause. As I see, you should fix the
>> pointer-not-incrementing. Why do you think that it's okay that the pointer is
>> not updating during sound play? Why do you insist that there is a delay? I don't
>> understand why we are so stuck here.
> 
> Well, in the API POV, it's nothing wrong to keep hwptr sticking while
> updating only delay value.  It implies that the hardware chip doesn't
> provide the hwptr update.
> 
> Though, usually the delay value is provided also from the hardware,
> e.g. reading the link position or such.  It's a typical case like
> USB-audio, where the hwptr gets updated and the delay indicates the
> actual position *behind* hwptr.  That works because hwptr shows the
> position in the ring buffer at which you can access the data.  And it
> doesn't mean that hwptr is the actually playing position, but it can
> be ahead of the current position, when many samples are queued on
> FIFO.  The delay is provided to correct the position back to the
> actual point.
> 
> But, this also doesn't mean that the delay shouldn't be used for the
> purpose like this patchset, either.  OTOH, providing a finer hwptr
> value would be likely more apps-friendly; there must be many programs
> that don't evaluate the delay value properly.
> 
> So, I suppose that hwptr update might be a better option if the code
> won't become too complex.  Let's see.

Indeed. It will take me a few days to look into this…

Regards
Mike

> One another thing I'd like to point out is that the value given in the
> patch is nothing but an estimated position, optimistically calculated
> via the system timer.  Mike and I had already discussion in another
> thread, and another possible option would be to provide the proper
> timestamp-vs-hwptr pair, instead of updating the timestamp always at
> the status read.
> 
> Maybe it's worth to have a module option to suppress this optimistic
> hwptr update behavior, in case something went wrong with clocks?
> 
> 
> thanks,
> 
> Takashi


WARNING: multiple messages have this Message-ID (diff)
From: Mike Brady <mikebrady@eircom.net>
To: Takashi Iwai <tiwai@suse.de>
Cc: stefan.wahren@i2se.com, devel@driverdev.osuosl.org,
	alsa-devel@alsa-project.org, f.fainelli@gmail.com,
	julia.lawall@lip6.fr, gregkh@linuxfoundation.org,
	linux-kernel@vger.kernel.org, eric@anholt.net,
	linux-rpi-kernel@lists.infradead.org,
	nishka.dasgupta_ug18@ashoka.edu.in,
	Kirill Marinushkin <k.marinushkin@gmail.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2] staging: bcm2835-audio: interpolate audio delay
Date: Sun, 28 Oct 2018 14:26:12 +0000	[thread overview]
Message-ID: <126FA055-050B-4AAC-9392-CA8CCA821768@eircom.net> (raw)
In-Reply-To: <s5hftwuo4t0.wl-tiwai@suse.de>



> On 25 Oct 2018, at 08:37, Takashi Iwai <tiwai@suse.de> wrote:
> 
> On Thu, 25 Oct 2018 00:02:34 +0200,
> Kirill Marinushkin wrote:
>> 
>>>> When you play sound - the pointer increments.
>>> 
>>> Unfortunately, when you play sound, the pointer does not actually increment, for up to about 10 milliseconds. I know of no way to actually access the true “live” position of the frame that is being played at any instant; hence the desire to estimate it.
>>> 
>> 
>> Your vision of situation in the opposite from my vision. What you see as a
>> symptom - I see as a root cause. As I see, you should fix the
>> pointer-not-incrementing. Why do you think that it's okay that the pointer is
>> not updating during sound play? Why do you insist that there is a delay? I don't
>> understand why we are so stuck here.
> 
> Well, in the API POV, it's nothing wrong to keep hwptr sticking while
> updating only delay value.  It implies that the hardware chip doesn't
> provide the hwptr update.
> 
> Though, usually the delay value is provided also from the hardware,
> e.g. reading the link position or such.  It's a typical case like
> USB-audio, where the hwptr gets updated and the delay indicates the
> actual position *behind* hwptr.  That works because hwptr shows the
> position in the ring buffer at which you can access the data.  And it
> doesn't mean that hwptr is the actually playing position, but it can
> be ahead of the current position, when many samples are queued on
> FIFO.  The delay is provided to correct the position back to the
> actual point.
> 
> But, this also doesn't mean that the delay shouldn't be used for the
> purpose like this patchset, either.  OTOH, providing a finer hwptr
> value would be likely more apps-friendly; there must be many programs
> that don't evaluate the delay value properly.
> 
> So, I suppose that hwptr update might be a better option if the code
> won't become too complex.  Let's see.

Indeed. It will take me a few days to look into this…

Regards
Mike

> One another thing I'd like to point out is that the value given in the
> patch is nothing but an estimated position, optimistically calculated
> via the system timer.  Mike and I had already discussion in another
> thread, and another possible option would be to provide the proper
> timestamp-vs-hwptr pair, instead of updating the timestamp always at
> the status read.
> 
> Maybe it's worth to have a module option to suppress this optimistic
> hwptr update behavior, in case something went wrong with clocks?
> 
> 
> thanks,
> 
> Takashi

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

WARNING: multiple messages have this Message-ID (diff)
From: mikebrady@eircom.net (Mike Brady)
To: linux-arm-kernel@lists.infradead.org
Subject: [alsa-devel] [PATCH v2] staging: bcm2835-audio: interpolate audio delay
Date: Sun, 28 Oct 2018 14:26:12 +0000	[thread overview]
Message-ID: <126FA055-050B-4AAC-9392-CA8CCA821768@eircom.net> (raw)
In-Reply-To: <s5hftwuo4t0.wl-tiwai@suse.de>



> On 25 Oct 2018, at 08:37, Takashi Iwai <tiwai@suse.de> wrote:
> 
> On Thu, 25 Oct 2018 00:02:34 +0200,
> Kirill Marinushkin wrote:
>> 
>>>> When you play sound - the pointer increments.
>>> 
>>> Unfortunately, when you play sound, the pointer does not actually increment, for up to about 10 milliseconds. I know of no way to actually access the true ?live? position of the frame that is being played at any instant; hence the desire to estimate it.
>>> 
>> 
>> Your vision of situation in the opposite from my vision. What you see as a
>> symptom - I see as a root cause. As I see, you should fix the
>> pointer-not-incrementing. Why do you think that it's okay that the pointer is
>> not updating during sound play? Why do you insist that there is a delay? I don't
>> understand why we are so stuck here.
> 
> Well, in the API POV, it's nothing wrong to keep hwptr sticking while
> updating only delay value.  It implies that the hardware chip doesn't
> provide the hwptr update.
> 
> Though, usually the delay value is provided also from the hardware,
> e.g. reading the link position or such.  It's a typical case like
> USB-audio, where the hwptr gets updated and the delay indicates the
> actual position *behind* hwptr.  That works because hwptr shows the
> position in the ring buffer at which you can access the data.  And it
> doesn't mean that hwptr is the actually playing position, but it can
> be ahead of the current position, when many samples are queued on
> FIFO.  The delay is provided to correct the position back to the
> actual point.
> 
> But, this also doesn't mean that the delay shouldn't be used for the
> purpose like this patchset, either.  OTOH, providing a finer hwptr
> value would be likely more apps-friendly; there must be many programs
> that don't evaluate the delay value properly.
> 
> So, I suppose that hwptr update might be a better option if the code
> won't become too complex.  Let's see.

Indeed. It will take me a few days to look into this?

Regards
Mike

> One another thing I'd like to point out is that the value given in the
> patch is nothing but an estimated position, optimistically calculated
> via the system timer.  Mike and I had already discussion in another
> thread, and another possible option would be to provide the proper
> timestamp-vs-hwptr pair, instead of updating the timestamp always at
> the status read.
> 
> Maybe it's worth to have a module option to suppress this optimistic
> hwptr update behavior, in case something went wrong with clocks?
> 
> 
> thanks,
> 
> Takashi

  parent reply	other threads:[~2018-10-28 14:26 UTC|newest]

Thread overview: 54+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-18 10:57 [PATCH] staging: bcm2835-audio: interpolate audio delay Mike Brady
2018-10-18 10:57 ` Mike Brady
2018-10-18 11:02 ` Takashi Iwai
2018-10-18 11:02   ` Takashi Iwai
2018-10-18 19:48 ` Kirill Marinushkin
2018-10-18 19:48   ` Kirill Marinushkin
2018-10-22 19:08   ` Mike Brady
2018-10-22 19:08     ` Mike Brady
2018-10-22 19:17 ` [PATCH v2] " Mike Brady
2018-10-22 19:17   ` Mike Brady
2018-10-22 19:17   ` Mike Brady
2018-10-22 22:25   ` Kirill Marinushkin
2018-10-22 22:25     ` Kirill Marinushkin
2018-10-24  8:20     ` [alsa-devel] " Mike Brady
2018-10-24  8:20       ` Mike Brady
2018-10-24  8:20       ` Mike Brady
2018-10-24 18:06       ` [alsa-devel] " Kirill Marinushkin
2018-10-24 18:06         ` Kirill Marinushkin
2018-10-24 19:54         ` Mike Brady
2018-10-24 19:54           ` Mike Brady
2018-10-24 19:54           ` Mike Brady
2018-10-24 22:02           ` [alsa-devel] " Kirill Marinushkin
2018-10-24 22:02             ` Kirill Marinushkin
2018-10-25  7:37             ` Takashi Iwai
2018-10-25  7:37               ` Takashi Iwai
2018-10-25  7:37               ` Takashi Iwai
2018-10-25 17:20               ` [alsa-devel] " Kirill Marinushkin
2018-10-25 17:20                 ` Kirill Marinushkin
2018-10-28 14:24                 ` Mike Brady
2018-10-28 14:24                   ` Mike Brady
2018-10-28 14:24                   ` Mike Brady
2018-10-28 14:26               ` Mike Brady [this message]
2018-10-28 14:26                 ` [alsa-devel] " Mike Brady
2018-10-28 14:26                 ` Mike Brady
2018-11-05 15:57                 ` [alsa-devel] " Mike Brady
2018-11-05 15:57                   ` Mike Brady
2018-11-05 15:57                   ` Mike Brady
2018-11-05 16:11                   ` [alsa-devel] " Takashi Iwai
2018-11-05 16:11                     ` Takashi Iwai
2018-11-06 21:05                     ` Mike Brady
2018-11-06 21:05                       ` Mike Brady
2018-11-06 21:31                       ` Takashi Iwai
2018-11-06 21:31                         ` Takashi Iwai
2018-11-11 18:08                         ` Mike Brady
2018-11-11 18:21                           ` [PATCH v2] ARM: " Mike Brady
2018-11-11 18:08                           ` [alsa-devel] [PATCH v2] " Mike Brady
2018-11-11 20:18                           ` [PATCH v2] ARM: " Stefan Wahren
2018-11-11 20:18                             ` Stefan Wahren
2018-11-11 20:18                             ` Stefan Wahren
2018-11-13 16:50                           ` Takashi Iwai
2018-11-13 16:50                             ` Takashi Iwai
2019-01-01 10:02                             ` Mike Brady
2019-01-01 10:02                               ` Mike Brady
2019-01-01 10:02                               ` Mike Brady

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=126FA055-050B-4AAC-9392-CA8CCA821768@eircom.net \
    --to=mikebrady@eircom.net \
    --cc=alsa-devel@alsa-project.org \
    --cc=devel@driverdev.osuosl.org \
    --cc=eric@anholt.net \
    --cc=f.fainelli@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=julia.lawall@lip6.fr \
    --cc=k.marinushkin@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rpi-kernel@lists.infradead.org \
    --cc=nishka.dasgupta_ug18@ashoka.edu.in \
    --cc=stefan.wahren@i2se.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.