On Mon, 12 Oct 2020 07:25:25 +0200, Vinod Koul wrote: > > 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)...? That's fine, then it's just a kernel's business, and it should be determined which one makes the code better. But, there are things to be considered, though: - SNDRV_PCM_STATE_* is defined as snd_pcm_state_t with __bitwise. This indicates that the type has to be defined in that way explicitly. - Having a value over SNDRV_PCM_STATE_LAST internally is hackish. > 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... Yes. But currently the API uses SND_PCM_* even for the compress stuff. Changing this value means to have influence on PCM, even if PCM stuff doesn't use it yet. (At least you'd need to increase SND_PCM_STATE_LAST, for example.) That said, if we want to change only for compress API by assuming that the impact must be negligible, the first step would be to move from SND_PCM_STATE_* to the own state, SND_COMPRESS_STATE_*. The values should be compatible, but this has to be changed at first. Then you can introduce a new value there. thanks, Takashi
