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

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

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@linaro.org>
>> ---
>>   .../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

>