linux-kernel - Re: [PATCH 10/11] ASoC: qdsp6-dai: add gapless support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 8 Jul 2020 16:23:19 +0100
From:   Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        broonie@...nel.org
Cc:     alsa-devel@...a-project.org, ckeepax@...nsource.cirrus.com,
        tiwai@...e.com, lgirdwood@...il.com, linux-kernel@...r.kernel.org,
        vkoul@...nel.org
Subject: Re: [PATCH 10/11] ASoC: qdsp6-dai: add gapless support



On 08/07/2020 14:32, Pierre-Louis Bossart wrote:
> 
>>>> Add support to gapless playback by implementing metadata,
>>>> next_track, drain and partial drain support.
>>>>
>>>> Gapless on Q6ASM is implemented by opening 2 streams in a single asm 
>>>> stream
>>>
>>> What does 'in a single asm stream' means?
>>
>>
>> So in QDSP6 ASM (Audio Stream Manager) terminology we have something 
>> called "asm session" for each ASoC FE DAI, Each asm session can be 
>> connected with multiple streams (upto 8 I think). However there will 
>> be only one active stream at anytime. Also there only single data 
>> buffer associated with each asm session.
>>
>> For Gapless usecase, we can keep two streams open for one asm-session, 
>> allowing us to fill in data on second stream while first stream is 
>> playing.
> 
> Ah, that's interesting, thanks for the details. So you have one DMA 
> transfer and the data from the previous and next track are provided in 
> consecutive bytes in a ring buffer, but at the DSP level you have a 
> switch that will feed data for the previous and next tracks into 
> different decoders, yes?

Yes, that's true, we can drain and stop first stream and start next 
stream which will do the switch!

> 
> If that is the case, indeed the extension you suggested earlier to 
> change the profile is valid. You could even change the format I guess.
> 

Exactly, we did test this patchset along with the extension suggested!


> To avoid confusion I believe the capabilities would need to be extended 
> so that applications know that gapless playback is supported across 
> unrelated profiles/formats. The point is that you don't want a 
> traditional implementation to use a capability that isn't supported in 
> hardware or will lead to audio issues.
>  >>>> and toggling them on next track.
>>>
>>> It really seems to me that you have two streams at the lowest level, 
>>> along with the knowledge of how many samples to remove/insert and 
>>> hence could do a much better job - including gapless support between 
>>> unrelated profiles and cross-fading - without the partial drain and 
>>> next_track mechanism that was defined assuming a single stream/profile.
>> At the end of the day its a single session with one data buffer but 
>> with multiple streams.
>>
>> Achieving cross fade should be easy with this design.
> 
> looks like it indeed.
> 
>> We need those hooks for partial drain and next track to allow us to 
>> switch between streams and pass silence information to respective 
>> stream ids.
> 
> right, but the key point is 'switch between streams'. That means a more 
> complex/capable implementation that should be advertised as such to 
> applications. This is not the default behavior assumed initially: to 
> allow for minimal implementations in memory-constrained devices, we 
> assumed gapless was supported with a single decoder.
> 
> Maybe the right way to do this is extend the snd_compr_caps structure:
> 
> /**
>   * struct snd_compr_caps - caps descriptor
>   * @codecs: pointer to array of codecs
>   * @direction: direction supported. Of type snd_compr_direction
>   * @min_fragment_size: minimum fragment supported by DSP
>   * @max_fragment_size: maximum fragment supported by DSP
>   * @min_fragments: min fragments supported by DSP
>   * @max_fragments: max fragments supported by DSP
>   * @num_codecs: number of codecs supported
>   * @reserved: reserved field
>   */
> struct snd_compr_caps {
>      __u32 num_codecs;
>      __u32 direction;
>      __u32 min_fragment_size;
>      __u32 max_fragment_size;
>      __u32 min_fragments;
>      __u32 max_fragments;
>      __u32 codecs[MAX_NUM_CODECS];
>      __u32 reserved[11];
> } __attribute__((packed, aligned(4)));
> 
> 
> and use a reserved field to provide info on capabilities, and filter the 
> set_codec_params() addition based this capability - i.e. return -ENOTSUP 
> in 'traditional' implementations based on a single 'stream'/decoder 
> instance.
Sounds good!
I will give it a go and see how it ends up!


--srini