[v4,1/3] ALSA: compress: document the compress audio state machine
diff mbox series

Message ID 20200629075002.11436-2-vkoul@kernel.org
State New
Headers show
Series
  • ALSA: compress: Document stream states and fix gapless SM
Related show

Commit Message

Vinod Koul June 29, 2020, 7:50 a.m. UTC
So we had some discussions of the stream states, so I thought it is a
good idea to document the state transitions, so add it documentation

Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 .../sound/designs/compress-offload.rst        | 51 +++++++++++++++++++
 1 file changed, 51 insertions(+)

Comments

Amadeusz Sławiński June 29, 2020, 9:05 a.m. UTC | #1
On 6/29/2020 9:50 AM, Vinod Koul wrote:
> So we had some discussions of the stream states, so I thought it is a
> good idea to document the state transitions, so add it documentation
> 
> Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>   .../sound/designs/compress-offload.rst        | 51 +++++++++++++++++++
>   1 file changed, 51 insertions(+)
> 
> diff --git a/Documentation/sound/designs/compress-offload.rst b/Documentation/sound/designs/compress-offload.rst
> index ad4bfbdacc83..b6e9025ae105 100644
> --- a/Documentation/sound/designs/compress-offload.rst
> +++ b/Documentation/sound/designs/compress-offload.rst
> @@ -151,6 +151,57 @@ Modifications include:
>   - Addition of encoding options when required (derived from OpenMAX IL)
>   - Addition of rateControlSupported (missing in OpenMAX AL)
>   
> +State Machine
> +=============
> +
> +The compressed audio stream state machine is described below ::
> +
> +                                        +----------+
> +                                        |          |
> +                                        |   OPEN   |
> +                                        |          |
> +                                        +----------+
> +                                             |
> +                                             |
> +                                             | compr_set_params()
> +                                             |
> +                                             v
> +         compr_free()                   +----------+
> +  +------------------------------------|          |
> +  |                                    |   SETUP  |
> +  |          +------------------------>|          |<-------------------------+
> +  |          | compr_drain_notify()    +----------+                          |
> +  |          |         or                   ^                                |
> +  |          |      compr_stop()            |                                |
> +  |          |                              | compr_write()                  |
> +  |          |                              |                                |
> +  |          |                              |                                |
> +  |          |                         +----------+                          |
> +  |          |                         |          |   compr_free()           |
> +  |          |                         |  PREPARE |---------------> A        |
> +  |          |                         |          |                          |
> +  |          |                         +----------+                          |
> +  |          |                              |                                |
> +  |          |                              |                                |
> +  |          |                              | compr_start()                  |
> +  |          |                              |                                |
> +  |          |                              v                                |
> +  |    +----------+                    +----------+                          |
> +  |    |          |    compr_drain()   |          |        compr_stop()      |
> +  |    |  DRAIN   |<-------------------|  RUNNING |--------------------------+
> +  |    |          |                    |          |                          |
> +  |    +----------+                    +----------+                          |
> +  |                                       |    ^                             |
> +  |          A                            |    |                             |
> +  |          |              compr_pause() |    | compr_resume()              |
> +  |          |                            |    |                             |
> +  |          v                            v    |                             |
> +  |    +----------+                   +----------+                           |
> +  |    |          |                   |          |         compr_stop()      |
> +  +--->|   FREE   |                   |  PAUSE   |---------------------------+
> +       |          |                   |          |
> +       +----------+                   +----------+
> +
>   
>   Gapless Playback
>   ================
> 

Line containing compr_free (between SETUP and FREE) seems to be misaligned?

If you move prepare to the left and drain in place of drain, it feels 
like you won't need this weird indirection with A
Something like:

 >> +                                             v
>> +         compr_free()                   +----------+
>> +  +------------------------------------|          |
>> +  |                                    |   SETUP  |
>> +  |          +------------------------>|          |<-------------------------+
>> +  |          | compr_write()           +----------+                          |
>> +  |          |                              ^                                |
>> +  |          |                              | compr_drain_notify() or        |
>> +  |          |                              | compr_stop()                   |
>> +  |          |                              |                                |
>> +  |          |                              |                                |
>> +  |          |                         +----------+                          |
>> +  |          |                         |          |                          |
>> +  |          |                         |  DRAIN   |                          |
>> +  |          |                         |          |                          |
>> +  |          |                         +----------+                          |
>> +  |          |                              ^                                |
>> +  |          |                              |                                |
>> +  |          |                              | compr_drain()                  |
>> +  |          |                              |                                |
>> +  |          |                              |                                |
>> +  |    +----------+                    +----------+                          |
>> +  |    |          |    compr_start()   |          |        compr_stop()      |
>> +  |    |  PREPARE |------------------->|  RUNNING |--------------------------+
>> +  |    |          |                    |          |                          |
>> +  |    +----------+                    +----------+                          |
>> +  |          |                            |    ^                             |
>> +  |          | compr_free()               |    |                             |
>> +  |          |              compr_pause() |    | compr_resume()              |
>> +  |          |                            |    |                             |
>> +  |          v                            v    |                             |
>> +  |    +----------+                   +----------+                           |
>> +  |    |          |                   |          |         compr_stop()      |
>> +  +--->|   FREE   |                   |  PAUSE   |---------------------------+
>> +       |          |                   |          |
>> +       +----------+                   +----------+
>> +

but this makes me question PREPARE state, how do you enter it?
Vinod Koul June 29, 2020, 9:34 a.m. UTC | #2
Hi Amadeusz,

On 29-06-20, 11:05, Amadeusz Sławiński wrote:
> 
> 
> On 6/29/2020 9:50 AM, Vinod Koul wrote:
> > So we had some discussions of the stream states, so I thought it is a
> > good idea to document the state transitions, so add it documentation
> > 
> > Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > ---
> >   .../sound/designs/compress-offload.rst        | 51 +++++++++++++++++++
> >   1 file changed, 51 insertions(+)
> > 
> > diff --git a/Documentation/sound/designs/compress-offload.rst b/Documentation/sound/designs/compress-offload.rst
> > index ad4bfbdacc83..b6e9025ae105 100644
> > --- a/Documentation/sound/designs/compress-offload.rst
> > +++ b/Documentation/sound/designs/compress-offload.rst
> > @@ -151,6 +151,57 @@ Modifications include:
> >   - Addition of encoding options when required (derived from OpenMAX IL)
> >   - Addition of rateControlSupported (missing in OpenMAX AL)
> > +State Machine
> > +=============
> > +
> > +The compressed audio stream state machine is described below ::
> > +
> > +                                        +----------+
> > +                                        |          |
> > +                                        |   OPEN   |
> > +                                        |          |
> > +                                        +----------+
> > +                                             |
> > +                                             |
> > +                                             | compr_set_params()
> > +                                             |
> > +                                             v
> > +         compr_free()                   +----------+
> > +  +------------------------------------|          |
> > +  |                                    |   SETUP  |
> > +  |          +------------------------>|          |<-------------------------+
> > +  |          | compr_drain_notify()    +----------+                          |
> > +  |          |         or                   ^                                |
> > +  |          |      compr_stop()            |                                |
> > +  |          |                              | compr_write()                  |
> > +  |          |                              |                                |
> > +  |          |                              |                                |

Not sure, this is not in the version I posted, arrow is from setup ->
prepare. See https://lore.kernel.org/alsa-devel/20200629075002.11436-2-vkoul@kernel.org/

I don't know how it got reversed in your version, maybe the MUA messed
up??

> > +  |          |                         +----------+                          |
> > +  |          |                         |          |   compr_free()           |
> > +  |          |                         |  PREPARE |---------------> A        |
> > +  |          |                         |          |                          |
> > +  |          |                         +----------+                          |
> > +  |          |                              |                                |
> > +  |          |                              |                                |
> > +  |          |                              | compr_start()                  |
> > +  |          |                              |                                |
> > +  |          |                              v                                |
> > +  |    +----------+                    +----------+                          |
> > +  |    |          |    compr_drain()   |          |        compr_stop()      |
> > +  |    |  DRAIN   |<-------------------|  RUNNING |--------------------------+
> > +  |    |          |                    |          |                          |
> > +  |    +----------+                    +----------+                          |
> > +  |                                       |    ^                             |
> > +  |          A                            |    |                             |
> > +  |          |              compr_pause() |    | compr_resume()              |
> > +  |          |                            |    |                             |
> > +  |          v                            v    |                             |
> > +  |    +----------+                   +----------+                           |
> > +  |    |          |                   |          |         compr_stop()      |
> > +  +--->|   FREE   |                   |  PAUSE   |---------------------------+
> > +       |          |                   |          |
> > +       +----------+                   +----------+
> > +
> >   Gapless Playback
> >   ================
> > 
> 
> Line containing compr_free (between SETUP and FREE) seems to be misaligned?

not in the version I posted and see on lore

> If you move prepare to the left and drain in place of drain, it feels like
> you won't need this weird indirection with A

yeah that was a compromise to make it bit neater to me than adding
overlapping arrows...

> Something like:
> 
> >> +                                             v
> > > +         compr_free()                   +----------+
> > > +  +------------------------------------|          |
> > > +  |                                    |   SETUP  |
> > > +  |          +------------------------>|          |<-------------------------+
> > > +  |          | compr_write()           +----------+                          |
> > > +  |          |                              ^                                |
> > > +  |          |                              | compr_drain_notify() or        |
> > > +  |          |                              | compr_stop()                   |
> > > +  |          |                              |                                |
> > > +  |          |                              |                                |
> > > +  |          |                         +----------+                          |
> > > +  |          |                         |          |                          |
> > > +  |          |                         |  DRAIN   |                          |
> > > +  |          |                         |          |                          |
> > > +  |          |                         +----------+                          |
> > > +  |          |                              ^                                |
> > > +  |          |                              |                                |
> > > +  |          |                              | compr_drain()                  |
> > > +  |          |                              |                                |
> > > +  |          |                              |                                |
> > > +  |    +----------+                    +----------+                          |
> > > +  |    |          |    compr_start()   |          |        compr_stop()      |
> > > +  |    |  PREPARE |------------------->|  RUNNING |--------------------------+
> > > +  |    |          |                    |          |                          |
> > > +  |    +----------+                    +----------+                          |
> > > +  |          |                            |    ^                             |
> > > +  |          | compr_free()               |    |                             |
> > > +  |          |              compr_pause() |    | compr_resume()              |
> > > +  |          |                            |    |                             |
> > > +  |          v                            v    |                             |
> > > +  |    +----------+                   +----------+                           |
> > > +  |    |          |                   |          |         compr_stop()      |
> > > +  +--->|   FREE   |                   |  PAUSE   |---------------------------+
> > > +       |          |                   |          |
> > > +       +----------+                   +----------+
> > > +
> 

> but this makes me question PREPARE state, how do you enter it?

a compr_write() moves from SETUP -> PREPARE. Fixing that above looks
better version of mine..
Amadeusz Sławiński June 29, 2020, 10:02 a.m. UTC | #3
On 6/29/2020 11:34 AM, Vinod Koul wrote:
 >
 > Not sure, this is not in the version I posted, arrow is from setup ->
 > prepare. See 
https://lore.kernel.org/alsa-devel/20200629075002.11436-2-vkoul@kernel.org/
 >
 > I don't know how it got reversed in your version, maybe the MUA messed
 > up??
 >
Ah... I must have edited it myself when preparing edited version, sorry 
for noise ;)


 > a compr_write() moves from SETUP -> PREPARE. Fixing that above looks
 > better version of mine..
Yes, I feel it is easier to understand, A redirect confused me for a 
short bit when I first looked at graph and it can be easily avoided.
Vinod Koul June 29, 2020, 10:59 a.m. UTC | #4
On 29-06-20, 12:02, Amadeusz Sławiński wrote:
> On 6/29/2020 11:34 AM, Vinod Koul wrote:
> >
> > Not sure, this is not in the version I posted, arrow is from setup ->
> > prepare. See
> https://lore.kernel.org/alsa-devel/20200629075002.11436-2-vkoul@kernel.org/
> >
> > I don't know how it got reversed in your version, maybe the MUA messed
> > up??
> >
> Ah... I must have edited it myself when preparing edited version, sorry for
> noise ;)

No worries

> 
> > a compr_write() moves from SETUP -> PREPARE. Fixing that above looks
> > better version of mine..
> Yes, I feel it is easier to understand, A redirect confused me for a short
> bit when I first looked at graph and it can be easily avoided.

Thanks for the suggestion, it was a good one indeed, I will send the
update shortly

Patch
diff mbox series

diff --git a/Documentation/sound/designs/compress-offload.rst b/Documentation/sound/designs/compress-offload.rst
index ad4bfbdacc83..b6e9025ae105 100644
--- a/Documentation/sound/designs/compress-offload.rst
+++ b/Documentation/sound/designs/compress-offload.rst
@@ -151,6 +151,57 @@  Modifications include:
 - Addition of encoding options when required (derived from OpenMAX IL)
 - Addition of rateControlSupported (missing in OpenMAX AL)
 
+State Machine
+=============
+
+The compressed audio stream state machine is described below ::
+
+                                        +----------+
+                                        |          |
+                                        |   OPEN   |
+                                        |          |
+                                        +----------+
+                                             |
+                                             |
+                                             | compr_set_params()
+                                             |
+                                             v
+         compr_free()                   +----------+
+  +------------------------------------|          |
+  |                                    |   SETUP  |
+  |          +------------------------>|          |<-------------------------+
+  |          | compr_drain_notify()    +----------+                          |
+  |          |         or                   |                                |
+  |          |      compr_stop()            |                                |
+  |          |                              | compr_write()                  |
+  |          |                              |                                |
+  |          |                              v                                |
+  |          |                         +----------+                          |
+  |          |                         |          |   compr_free()           |
+  |          |                         |  PREPARE |---------------> A        |
+  |          |                         |          |                          |
+  |          |                         +----------+                          |
+  |          |                              |                                |
+  |          |                              |                                |
+  |          |                              | compr_start()                  |
+  |          |                              |                                |
+  |          |                              v                                |
+  |    +----------+                    +----------+                          |
+  |    |          |    compr_drain()   |          |        compr_stop()      |
+  |    |  DRAIN   |<-------------------|  RUNNING |--------------------------+
+  |    |          |                    |          |                          |
+  |    +----------+                    +----------+                          |
+  |                                       |    ^                             |
+  |          A                            |    |                             |
+  |          |              compr_pause() |    | compr_resume()              |
+  |          |                            |    |                             |
+  |          v                            v    |                             |
+  |    +----------+                   +----------+                           |
+  |    |          |                   |          |         compr_stop()      |
+  +--->|   FREE   |                   |  PAUSE   |---------------------------+
+       |          |                   |          |
+       +----------+                   +----------+
+
 
 Gapless Playback
 ================