Shoutcast calls sleep()

[RJ Skerry-Ryan]

EngineShoutcast calls sleep(). This blocks the sidechain thread which could cause the sidechain buffers to overflow which could cause a skip in the sidechain output (shoutcast and recording).

[William Good (bkgood)]

This looks fun. Any idea why we sleep in those places? The second sleep appears to be waiting for libshout to connect to the server because the shout_open call is async and doesn't provide an update callback (head. broke. keyboard.), but I can't figure out the first sleep call (after it prints the number of failed attempts).

[Phillip Whelan (pwhelan)]

Audio is timing sensitive, even (and especially if) broadcasted. We really should be shout_delay(3) to find out how long to sleep for: http://www.aelius.com/njh/libshout-doc/libshout.html#shout_delay.

[Phillip Whelan (pwhelan)]

Alternatively we can call shout_sync(3): http://www.aelius.com/njh/libshout-doc/libshout.html#shout_sync, although shout_delay(3) could allow us to do other processing in the meantime.

[RJ Skerry-Ryan (rryan)]

Bill if you're up for this task I think the whole sidechain could use a
slight refactor to be like, you add a SidechainWorker to the
EngineSidechain. That will make things a lot cleaner. Shoutcast and
recording would need to be changed to be SidechainWorkers.

Anyway, we definitely should not sleep. If shoutcast allows you to check
for the appropriate delay, we should just go into a 'wait' state and every
time EngineShoutcast gets an update, check if the appropriate amount of
time has passed (discarding all samples that it was fed before the
shoutcast system was ready so that there isn't a big backlog). Sleeping in
the sidechain thread can actually cause either incorrect behavior (e.g. a
glitch in the recorded output of mixxx) or in the worst case I think it
could cause an xrun due to priority inversions issues with the engine (in
the process of being fixed in my refactor branch).

On Fri, Jan 6, 2012 at 7:29 PM, Phillip Whelan <email address hidden>wrote:

> Audio is timing sensitive, even (and especially if) broadcasted. We
> really should be shout_delay(3) to find out how long to sleep for:
> http://www.aelius.com/njh/libshout-doc/libshout.html#shout_delay.
>
> --
> You received this bug notification because you are a member of Mixxx
> Development Team, which is subscribed to Mixxx.
> https://bugs.launchpad.net/bugs/883374
>
> Title:
> Shoutcast calls sleep()
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/mixxx/+bug/883374/+subscriptions
>

[Phillip Whelan (pwhelan)]

How exactly should we go into a 'wait state'? The sleep(2) system call is supposed to do that. Switching to some other work until the delay is up is a much better solution, as long as we can guarantee we will continue to transmit before the delay is up. Going into a busyloop in userspace is, IMO, the worst idea; it can pin a CPU completely and it will definitely cause problems for the scheduler (which is already being overloaded with our 16+ threads).

[RJ Skerry-Ryan (rryan)]

I'm not talking about a busy loop. EngineShoutcast is asked to do work
every .. 65k samples or so. Whatever the size of the sidechain buffers are.
Simply set a state variable that says it's waiting until libshout is ready
for it. Each time through, via process() switch on the state variable. If
it's in the wait state, check if the appropriate duration has elapsed. If
so, go from wait to ready and process the buffer.

The sidechain thread is not exclusive to shoutcast. You can't just sleep it
because that blocks all other sidechain features.

On Fri, Jan 6, 2012 at 8:18 PM, Phillip Whelan <email address hidden>wrote:

> How exactly should we go into a 'wait state'? The sleep(2) system call
> is supposed to do that. Switching to some other work until the delay is
> up is a much better solution, as long as we can guarantee we will
> continue to transmit before the delay is up. Going into a busyloop in
> userspace is, IMO, the worst idea; it can pin a CPU completely and it
> will definitely cause problems for the scheduler (which is already being
> overloaded with our 16+ threads).
>
> --
> You received this bug notification because you are a member of Mixxx
> Development Team, which is subscribed to Mixxx.
> https://bugs.launchpad.net/bugs/883374
>
> Title:
> Shoutcast calls sleep()
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/mixxx/+bug/883374/+subscriptions
>

[William Good (bkgood)]

RJ: my initial thought was to spawn a thread to connect to shoutcast whenever a connection was needed but creating a separate thread for each sidechain feature would certainly be more extensible. I'm taking this up.

I have a hard time believing that our ~16 threads are of any consequence to modern process schedulers that happily handle hundreds of processes concurrently without issue.

[Daniel Schürmann (daschuer)]

https://github.com/mixxxdj/mixxx/pull/665

[Tuukka Pasanen (pasanen-tuukka)]

Interesting thread. I'm with @bkgood. Threads should be launched when needed because in long run this not going to get any easier. People want to save and send and then they want to compress and what ever so easier would be that things are not sleeping but just rised when needed.
I'm fixing this currently I also fix wait() in destructor so it doesn't block it everything goes as bad it always goes.

[Tuukka Pasanen (pasanen-tuukka)]

And actually this is not correct bug report because engineshoutcast is inherited from QThread which have non-blocking sleep and msleep but waiting for 1 second is too long..

[Daniel Schürmann (daschuer)]

> People want to save and send and then they want to compress and what ever so easier would be that things are not sleeping but just rised when needed.

I do not understand this comment.

Basically this bug is already solved because we have now our own thread for shoutcast,
which can sleep or wait for all other condition as require, without bad effects for other thread.

Here some details to the wait() issue:
http://stackoverflow.com/questions/20073401/what-is-the-use-of-qthread-wait-function

We may add a band aid for the pending wait issue like that:

    m_pShoutcastEnabled->set(0);
    m_readSema.release();
    wait(); // until the thread ends.
    if(!wait(5000)) { // Wait until it actually has terminated (max. 5 sec)
        qWarning("Thread deadlock detected, bad things may happen !!!");
        terminate(); // Thread didn't exit in time, probably deadlocked, terminate it!
        wait(); // Note: We have to wait again here!
    }

But finding and solving the deadlock makes much more sends.
Terminate() will probably stop the tread in the middle of a critical system call which will makes remaining Mixxx unstable, since a thread is not an encapsulated process.

[Tuukka Pasanen (pasanen-tuukka)]

Made Pull Request: https://github.com/mixxxdj/mixxx/pull/763. I took little bit diffrent approact but ending should be the same. I was toying around terminate but ended up not using. In my implementation we can end up situation that thread is still running but Mixxx exit.

[Swiftb0y (swiftb0y)]

Mixxx now uses GitHub for bug tracking. This bug has been migrated to:
https://github.com/mixxxdj/mixxx/issues/6052