[pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option

[Georg Chini]

On 20.02.2018 19:49, Raman Shishniou wrote:
> On 02/20/2018 07:02 PM, Georg Chini wrote:
>> On 20.02.2018 16:38, Raman Shyshniou wrote:
>>> Currently the pipe-source will remain running even if no
>>> writer is connected and therefore no data is produced.
>>> This patch adds the autosuspend=<bool> option to prevent this.
>>> Source will stay suspended if no writer is connected.
>>> This option is enabled by default.
>>> ---
>>>    src/modules/module-pipe-source.c | 279 +++++++++++++++++++++++++++++----------
>>>    1 file changed, 212 insertions(+), 67 deletions(-)
>>>
> I think I need post a simple pseudo code of new thread loop because it
> was completely rewritten. There are too many changes in one patch.
> It can be difficult to see the whole picture of new main loop.

Well, I applied the patch and looked at the result. I still don't like 
the approach.

I would propose this:

auto_suspended = false;
revents = 0
events = POLLIN

for (;;) {

       /* This is the part that is run when the source is opened
        * or auto suspended
       if (SOURCE_IS_OPENED(source) || auto_suspended) {

           /* Check if we wake up from user suspend */
           if (corkfd >= 0 && !auto_suspended) {
                len = 0
                close pipe for writing
           }

           /* We received POLLIN or POLLHUP or both */
           if (revents) {

              /* Read data from pipe */
              len = read data

              /* Got data, post it */
              if (len > 0) {
                  if (auto_suspend) {
                      send unsuspend message
                      auto_suspend = false
                 }
                 post data

              /* Got POLLHUP or POLLIN without data, writer disconnected */
              } else if (len = 0) {
                   send suspend message
                   auto_suspend = true
              }

              /* else Handle error */
              else
                  handle error

              events = POLLIN
          }

       /* This is the part that is run, when the source is
       * suspended by user */
       } else {

            if (corkfd < 0)
               open pipe for writing
            events = 0
      }

       /* This is run always */
       run rtpoll
       ... check errors ...
}

I think this is more consistent with how the other thread functions are
written and also has a clearer structure.

>
> pollfd = NULL;
> rtpoll_item = NULL;
> chunk.length = 0; // length of pending data in chunk.memblock
> chunk.memblock = pa_memblock_new(); // always allocated
>
> for (;;) {
>
>      /**************************************************
>       * run rtpoll
>       */
>      if (chunk.length > 0) {
>          /* we have a pending data */
>          if (rtpoll_item) {
>              /* stop polling pipe
>               * the only way to be here:
>               * source was just suspended */
>              ... free rtpoll_item ...
>              rtpoll_item = NULL;
>          }
>      } else {
>          /* we have no pending data */
>          if (rtpoll_item == NULL) {
>              /* start polling pipe
>               * the only way to be here:
>               * source was just resumed */
>              ... allocate rtpoll_item, get pollfd ...
>              pollfd->events = POLLIN;
>              pollfd->fd = u->fd;
>          }
>      }
>
>      pa_rtpoll_run()
>
>      ... check errors ...
>
>      if (rtpoll_item) {
>          /* we polled pipe */
>          ... refresh pollfd ...
>      } else
>          /* we are waiting for source state changes */
>          pollfd = NULL;
>
>      /**************************************************
>       * Read data
>       */
>      if (pollfd && pollfd->revents) {
>
>          ... read data from pipe ...
>
>          if (len > 0) {
>              chunk.length = len;
>
>              /* source is autosuspended? */
>              if (u->corkfd >= 0) {
>                  ... send resume message ...
>                  close(u->corkfd);
>                  u->corkfd = -1;
>              }
>          }
>
>          if (len == 0) {
>              /* sourece not autosuspended? */
>              if (u->corkfd < 0) {
>                  ... send suspend message ...
>                  u->corkfd = open(pipe, O_WRONLY);
>              }
>          }
>
>          if (len < 0) {
>              ... check read error ...
>          }
>      }
>
>      /**************************************************
>       * Post data
>       */
>      if (source is opened) {

After an auto-unsuspend, the result of the above comparison is undefined.
It may be that the main thread already processed the unsuspend message,
maybe not.

>          ... post data ...
>          unref(chunk.memblock);
>          chunk.memblock = pa_memblock_new();
>          chunk.length = 0;
>      }
>
>      /* chunk.length can be greater than 0 only if we are suspended right now
>       * we need to stop polling pipe and wait while state will be changed to RUNNING or IDLE
>       * to post pending data */

If we are user suspended right now, the data should never be posted. 
Data that is read
while we are user-suspended should be discarded. Data that arrives while 
we are
auto-suspended should be posted.


> }
>
> I can convert all changes to patch series if you want.
>
> --
> Raman