Re: [PATCH v5 1/2] ALSA: pcm: reinvent the stream synchronization ID API

[Amadeusz Sławiński <amadeuszx.slawinski@xxxxxxxxxxxxxxx>]

On 6/24/2024 3:56 PM, Jaroslav Kysela wrote:
> On 24. 06. 24 15:38, Amadeusz Sławiński wrote:
> > On 6/24/2024 3:07 PM, Jaroslav Kysela wrote:
> > > Until the commit e11f0f90a626 ("ALSA: pcm: remove SNDRV_PCM_IOCTL1_INFO
> > > internal command"), there was a possibility to pass information
> > > about the synchronized streams to the user space. The mentioned
> > > commit removed blindly the appropriate code with an irrelevant comment.
> > >
> > > The revert may be appropriate, but since this API was lost for several
> > > years without any complains, it's time to improve it. The hardware
> > > parameters may change the used stream clock source (e.g. USB hardware)
> > > so move this synchronization ID to hw_params as read-only field.
> > >
> > > It seems that pipewire can benefit from this API (disable adaptive
> > > resampling for perfectly synchronized PCM streams) now.
> > >
> > > Note that the contents of ID is not supposed to be used for direct
> > > comparison with a specific byte sequence. The "empty" case is when
> > > all bytes are zero (driver does not offer this information)
> > > and all other cases must be only used for equal comparison among
> > > PCM streams (including different sound cards) if they are using
> > > identical hardware clock.
> > >
> > > Cc: Takashi Sakamoto <takaswie@xxxxxxxxxx>
> > > Signed-off-by: Jaroslav Kysela <perex@xxxxxxxx>
> > > ---
> >
> > (...)
> >
> > > @@ -420,7 +420,8 @@ struct snd_pcm_hw_params {
> > >        unsigned int rate_num;        /* R: rate numerator */
> > >        unsigned int rate_den;        /* R: rate denominator */
> > >        snd_pcm_uframes_t fifo_size;    /* R: chip FIFO size in frames */
> > > -    unsigned char reserved[64];    /* reserved for future */
> > > +    unsigned char sync[16];        /* R: synchronization ID (perfect 
> > > sync - one clock source) */
> >
> > If it is introduced as new API, can't this be done better? Maybe like:
> > struct sync {
> >     char cardnum[4];
> >     char id[12];
> > };
> > or maybe just:
> > struct sync {
> >     u32 cardnum;
> >     char id[12];
> > };
> > or something like that? It is bit hard to follow in next patch all this
> > params->sync + 4 and memset/strncpy. And having named fields would help.0
>
> The ID may be not related to one sound card. Multiple cards can share 
> one clock source. It's internal kernel ID generation scheme which may be 
> changed later when there's another demand in future. The applications 
> should handle this as 16-byte blob which is used only for equal 
> comparison among multiple PCM streams.

Ah, yes rereading it few more times it makes sense.

Thanks,
Amadeusz