Re: [RFC PATCH v2 2/6] ALSA: compress: add new ioctl for setting codec parameters

[Srinivas Kandagatla]

On 21/07/2020 21:05, Pierre-Louis Bossart wrote:
> On 7/21/20 12:00 PM, Srinivas Kandagatla wrote:
> > For gapless playback it is possible that each track can have different
> > codec profile with same decoder, for example we have WMA album,
> > we may have different tracks as WMA v9, WMA v10 and so on
> >
> > Or if DSP's like QDSP have abililty to switch decoders on single stream
>
> ability
>
> > for each track, then this call could be used to set new codec parameters.
> >
> > Existing code does not allow to change this profile while doing gapless
> > playback.
> >
> > This patch adds new SNDRV_COMPRESS_SET_CODEC_PARAMS IOCTL to allow
> > userspace to set this new parameters required for new codec profile.
> >
> > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> > ---
> > Â .../sound/designs/compress-offload.rstÂÂÂÂÂÂÂ |Â 6 ++++
> > Â include/sound/compress_driver.hÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |Â 5 +++
> > Â include/uapi/sound/compress_offload.hÂÂÂÂÂÂÂÂ |Â 1 +
> > Â sound/core/compress_offload.cÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ | 34 +++++++++++++++++++
> > Â 4 files changed, 46 insertions(+)
> >
> > diff --git a/Documentation/sound/designs/compress-offload.rst 
> > b/Documentation/sound/designs/compress-offload.rst
> > index 935f325dbc77..305ccc7bfdd9 100644
> > --- a/Documentation/sound/designs/compress-offload.rst
> > +++ b/Documentation/sound/designs/compress-offload.rst
> > @@ -128,6 +128,12 @@ set_params
> > ÂÂÂ cases decoders will ignore other fields, while encoders will strictly
> > ÂÂÂ comply to the settings
> > +set_codec_params
> > +Â This routine is very much simillar to set_params but exculding stream
>
> typos: similar, excluding
>
> > +Â information. Only codec related information is set as part of this.
> > +Â It is used in gapless playback where its required to change decoder
> > +Â or its parameters for next track. This is optional.
> > +
> > Â get_params
> > ÂÂÂ This routines returns the actual settings used by the DSP. Changes to
> > ÂÂÂ the settings should remain the exception.
> > diff --git a/include/sound/compress_driver.h 
> > b/include/sound/compress_driver.h
> > index 70cbc5095e72..d9c00bcfce9b 100644
> > --- a/include/sound/compress_driver.h
> > +++ b/include/sound/compress_driver.h
> > @@ -93,6 +93,9 @@ struct snd_compr_stream {
> > ÂÂ * @set_params: Sets the compressed stream parameters, mandatory
> > ÂÂ * This can be called in during stream creation only to set codec 
> > params
> > ÂÂ * and the stream properties
> > + * @set_codec_params: Sets the compressed stream codec parameters, 
> > Optional
> > + * This can be called in during gapless next track codec change only 
> > to set
> > + * codec params
>
> Would it be clearer if this was called set_next_codec_params()? or 
> set_next_track_codec_params()?
>
> Having set_params() and set_codec_params() is a bit confusing since the 
> semantic difference is not captured in the callback name.

set_next_track_codec_params seems more sensible as its next track params.
Will change this in next version!

--srini

>