Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Vinod Koul <vkoul@kernel.org>]

Hi Takashi, Jaroslav,

On 10-10-20, 11:08, Takashi Iwai wrote:
> On Fri, 09 Oct 2020 19:43:40 +0200,
> Jaroslav Kysela wrote:
> > 
> > Dne 09. 10. 20 v 17:13 Takashi Iwai napsal(a):
> > > On Thu, 08 Oct 2020 11:49:24 +0200,
> > > Gyeongtaek Lee wrote:
> > >>
> > >> On 10/06/20 11:57 PM, Pierre-Louis Bossart wrote:
> > >>>> The SM in kernel might be bit more convoluted so was wondering if we can
> > >>>> handle this in userland. The changelog for this patch says that for
> > >>>> test case was sending whole file, surely that is not an optimal approach.
> > >>>
> > >>> It's rather common to have to deal with very small files, even with PCM, 
> > >>> e.g. for notifications. It's actually a classic test case that exposes 
> > >>> design issues in drivers, where e.g. the last part of the notification 
> > >>> is not played.
> > >>>
> > >>>> Should we allow folks to send whole file to kernel and then issue
> > >>>> partial drain?
> > >>>
> > >>> I don't think there should be a conceptual limitation here. If the 
> > >>> userspace knows that the last part of the file is smaller than a 
> > >>> fragment it should be able to issue a drain (or partial drain if it's a 
> > >>> gapless solution).
> > >>>
> > >>> However now that I think of it, I am not sure what happens if the file 
> > >>> is smaller than a fragment. That may very well be a limitation in the 
> > >>> design.
> > >>>
> > >> Thanks for the comments.
> > >>
> > >> Actually, problem can be occurred with big file also.
> > >> Application usually requests draining after sending last frame.
> > >> If user clicks pause button after draining, pause will be failed
> > >> and the file just be played until end.
> > >> If application stop and start playback for this case, 
> > >> start of last frame will be heard again because stop sets state to SETUP,
> > >> and write is needed to set the state to PREPARED for start.
> > >> If bitrate of the file is low, time stamp will be reversed and be heard weird.
> > >> I also hope this problem can be handled in userspace easily but I couldn't find a way for now.
> > >>
> > >> I think that this is the time that I should share fixed patch following the comments to help the discussion.
> > >> Following opinions are added to the patch.
> > >> 1. it's be much nicer to have a new state - Takashi
> > > 
> > > Well, it wasn't me; I'm not against the new state *iff* it would end
> > > up with cleaner code.  Admittedly, the new state can be more
> > > "consistent" regarding the state transition.  If we allow the PAUSE
> > > state during DRAINING, it'll lead to multiple states after resuming
> > > the pause.
> > > 
> > >> 2. We can add this state to asound.h so the user space can be updated. - Jaroslav
> > >> 3. don't forget to increase the SNDRV_COMPRESS_VERSION - Jaroslav
> > >>
> > >> I'm bit wondering whether it is good to increase SNDRV_COMPRESS_VERSION
> > >> with a change in asound.h not in compress_offload.h.
> > >> Should I increase SNDRV_PCM_VERSION also?
> > > 
> > > Yes, if we really add the PCM state, it's definitely needed.
> > > 
> > >> And what happened if I request back-porting a patch which changes VERSION to stables?
> > > 
> > > If we introduce the new change, it must be safe to the old kernels,
> > > too.  The problem is only about the compatibility of the user-space
> > > program, not about the kernel.
> > > 
> > > 
> > > HOWEVER: I'm still concerned by the addition of a new PCM state.
> > > Jaroslav suggested two steps approach, (1) first add the state only in
> > > the uapi header, then use (2) the new state actually.  But, this
> > > doesn't help much, simply because the step 1 won't catch real bugs.
> > > 
> > > Even if we add a new state and change the SNDRV_PCM_STATE_LAST, I
> > > guess most of code can be compiled fine.  So, at the step 1, no one
> > > notices it and bothered, either.  But, at the step 2, you'll hit a
> > > problem.
> > > 
> > > Adding a new state is something like to add a new color to the traffic
> > > signal.  In some countries, the car turning right at a crossing
> > > doesn't have to stop at a red signal.  Suppose that we want to control
> > > it, and change the international rule by introducing a new color (say
> > > magenta) signal to stop the car turning right.  That'll be a big
> > > confusion because most drivers are trained for only red, green and
> > > yellow signals.
> > > 
> > > Similarly, if we add a new PCM state, every program code that deals
> > > with the PCM state may be confused by the new state.  It has to be
> > > reviewed and corrected manually, because it's no syntax problem the
> > > compiler may catch.
> > 
> > If there is a handshake between both side, this problem is gone. We can just
> > add another flag / ioctl / whatever to activate the new behaviour.
> 
> That's another tricky part.  We do have already some handshake in
> alsa-lib to determine the supported protocol.  However, if a code in
> question is outside that influence, we can't ensure that all belonging
> components understand the new one.  e.g. if a program uses an
> intermediate library, it's free from alsa-lib changes.  Or, imagine
> some plugin.
> 
> If this were a change of the API function, we may have a better
> control.  We may provide different versioned symbols in the worst
> case, too.  But, an enum is essentially hard-coded, so we have no
> direct influence once after it's compiled.

So what if we add another state but keep it in kernel (hidden from
userspace)...?

Right now tinycompress does not make use of PCM streams, kernel handles
these. I am not aware of any other implementation.

So if the scope if within compress then it might work...

Thanks
-- 
~Vinod