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]

Message-ID: <cf9b2d33-9b63-f3d2-2e51-a88c528dad53@linux.intel.com>
Date:   Wed, 8 Jul 2020 08:32:02 -0500
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        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

>>> 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?

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.

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.