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. thanks, Takashi
