On Thu, Nov 16, 2017 at 19:49:36 +0100, Takashi Iwai wrote:
> On Thu, 16 Nov 2017 19:11:37 +0100,
> Pierre-Louis Bossart wrote:
> >
> > On 11/16/17 8:48 AM, Takashi Iwai wrote:
> > > On Thu, 16 Nov 2017 13:43:57 +0100,
> > > Henrik Eriksson wrote:
> > >>
> > >> This reverts a change that was part of a larger commit. That change
> > >> broke timestamps in some application code for us as outlined in the log,
> > >> and reverting it obviously fixes this. Since the original change did
> > >> not explain why the behaviour was modified I can't speak for the effects
> > >> on that use case.
> > > So this is a regression and we need to address it.
> > >
> > > I *guess* that the reason to update tstamp event at the same hwptr is
> > > that now we can get a more fine-grained tstamp. OTOH, we have to
> > > avoid the regression by that.
> > >
> > > What about the (untested) patch like below? It skips the tstamp
> > > update as default but follows the new standard when a better tstamp
> > > mode is deployed.
> >
> > update_audio_tstamp() only updates the audio timestamp when the type
> > is DEFAULT already, but indeed update runtime->status->tstamp is
> > updated unconditionally.
> >
> > I have a bit of heartburn with the suggested solutions because the
> > delay can change even with the same hw_ptr. Not updating the
> > timestamps would be going back to the days where timing updates was
> > dependent on DMA granularity.
> >
> > Maybe we could make the tstamp update dependent on a change in
> > audio_tstamp, e.g.
> >
> >
> > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> > index a93a4235a332..b606f3ea7a17 100644
> > --- a/sound/core/pcm_lib.c
> > +++ b/sound/core/pcm_lib.c
> > @@ -248,8 +248,10 @@ static void update_audio_tstamp(struct
> > snd_pcm_substream *substream,
> > runtime->rate);
> > *audio_tstamp = ns_to_timespec(audio_nsecs);
> > }
> > - runtime->status->audio_tstamp = *audio_tstamp;
> > - runtime->status->tstamp = *curr_tstamp;
> > + if (runtime->status->audio_tstamp != *audio_tstamp) {
This should be
if (!timespec_equal(&runtime->status->audio_tstamp, audio_tstamp)) {
> > + runtime->status->audio_tstamp = *audio_tstamp;
> > + runtime->status->tstamp = *curr_tstamp;
> > + }
>
> OK for me as long as it works.
> Henrik?
This appears to work for our use case.
How do I progress this, i.e., do I update my patch to use Pierre-Louis's
fix? How should I attribute that?
Regards,
/henrik
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
