PDA

View Full Version : Remote streaming - server blocks in



Triode
2005-03-10, 19:34
Hi,

I've been scratching my head about the scrolling jitter on SB2 and Softsqueeze. I think I have got it down to the fact that the
server currently blocks in Slim::Player::Protocols::HTTP within sub sysread at the line below when playing mp3 streams (well the
specific ones I am testing with is RadioIO ones). [I think it will actually block on all remote streams going though this]

unless (${*$self}{'_sel'}->can_read($timeout)) {

This is because it is called with a default remote timeout. Hence this line can block the server for up to 5 seconds (or whatever
is set).

Should this really be blocking at all - is there a way to create a buffer which reads the source stream on select and then send it
out when this function is called if there is anything in the buffer??? In the interim any views on setting the this to a lower
value - ideally sub millisecond to ensure the server does not block at it?

I wrapped some timestamps around the function call just to see the length of time it is blocking - here's a quick example when
playing a radioIO mp3 stream.
can_read: 0.000129
can_read: 0.002435
can_read: 0.003344
can_read: 0.001695
can_read: 0.028099
can_read: 0.000139
can_read: 0.034782
can_read: 0.050240
can_read: 0.000128
can_read: 0.021101
can_read: 0.191841
can_read: 0.029535
can_read: 0.048257
can_read: 0.000138

[Why is this not seen on SB1? Not sure at present, but I notice that the tcp send queue builds up for SB1 and with debug --d_http
it always sends 8K blocks, with the new Softsqueeze this is not the case, i.e. writing to the player may not block in the same way
so smaller blocks of audio are written. Hence it looks like backpressure up the audio chain operates in a different way? Is there
some new buffer management in Softsqueeze/SB2? Currently quessing what SB2 does...]

Any views?

Adrian

Dan Sully
2005-03-10, 19:38
* Triode shaped the electrons to say...

>I've been scratching my head about the scrolling jitter on SB2 and
>Softsqueeze. I think I have got it down to the fact that the server
>currently blocks in Slim::Player::Protocols::HTTP within sub sysread at the
>line below when playing mp3 streams (well the specific ones I am testing
>with is RadioIO ones). [I think it will actually block on all remote
>streams going though this]
>
>unless (${*$self}{'_sel'}->can_read($timeout)) {

Dave Cohen wrote some real non-blocking HTTP code that we can get in here.

The hack that I did was just that - it was meant to alleviate audio skipping.

-D
--
It's the wrong trousers Gromit, and they've gone wrong!

Triode
2005-03-10, 19:50
>>unless (${*$self}{'_sel'}->can_read($timeout)) {
>
> Dave Cohen wrote some real non-blocking HTTP code that we can get in here.
>
> The hack that I did was just that - it was meant to alleviate audio skipping.
>
I suspect this will also cause problems if you listen to a very low bit rate remote stream whilst trying to listen to multiple high
rate local files etc, so would be really good to get Dave's code in. [Will also make my scrolling code work better - though I seem
to be the one needing all the real time tuning at the moment..]

Adrian

Triode
2005-03-11, 06:40
> Dave Cohen wrote some real non-blocking HTTP code that we can get in here.
>
> The hack that I did was just that - it was meant to alleviate audio skipping.

Dan,

Does Dave's code cover all of the http code - including readMetaData (which is called from sysread and is also blocking!).

I notice that someone has already started to report this as a problem...

Anyway, as a quick hack on your hack(!), the attached attempts to avoid the blocking sysread in all cases but the one when metadata
is read. It moves the select back into the HTTP->convert() function. At present this is a mini idle loop which includes calling
timers - if you don't like this due to the possibility of recursion (not sure it could harm?), it could simply include the select
call for a short timeout - say 0.1

I think the right answer with Metadata is to read chucks as normal from the tcp session and then parse off the meta data from the
start if we expect it - do you have this covered, or is it worth me looking at it?

Adrian

vidurapparao
2005-03-11, 08:11
Adrian,

I think you're correct in identifying the can_read call for the chunky
animation behavior. Since the calling code should be able to deal with a
0 readlength (and $! set to EWOULDBLOCK), it seems unnecessary.

The blocking code in readMetaData, while not ideal, is similar to what
we were doing with 5.4. If you could make the state machine in HTTP.pm
deal with metadata split across non-blocking reads, definitely go for it.

The call to checkTimers in your loop, if included, should probably just
go into idleStreams. To be frank, I'm a little uncomfortable with he
whole notion of a recursive select loop. If it's working, it's because
of luck rather than a careful and deliberate understanding of the
reentrancy issues. Adding checkTimers to that loop has the potential to
increase our risk.

--Vidur

Triode wrote:

>> Dave Cohen wrote some real non-blocking HTTP code that we can get in
>> here.
>>
>> The hack that I did was just that - it was meant to alleviate audio
>> skipping.
>
>
> Dan,
>
> Does Dave's code cover all of the http code - including readMetaData
> (which is called from sysread and is also blocking!).
>
> I notice that someone has already started to report this as a problem...
>
> Anyway, as a quick hack on your hack(!), the attached attempts to
> avoid the blocking sysread in all cases but the one when metadata is
> read. It moves the select back into the HTTP->convert() function. At
> present this is a mini idle loop which includes calling timers - if
> you don't like this due to the possibility of recursion (not sure it
> could harm?), it could simply include the select call for a short
> timeout - say 0.1
>
> I think the right answer with Metadata is to read chucks as normal
> from the tcp session and then parse off the meta data from the start
> if we expect it - do you have this covered, or is it worth me looking
> at it?
>
> Adrian
>
>------------------------------------------------------------------------
>
>

Triode
2005-03-11, 08:24
Vidur,

OK - I think the potential recursion comes from a timer calling HTTP->convert which then checked timers and calls another timer
which also does HTTP->convert.

So assuming we want a simple solution, I would propose to put check timers into idleStreams and also put a semaphor on it to ensure
it is not called if HTTP->convert is called from inside a timer.

Does this sound reasonable? If so I can probably come up with some code rapidly for all but the readMetaData bit - which would get
us back to where 5.4 was?

Adrian
----- Original Message -----
From: "Vidur Apparao" <vidur (AT) slimdevices (DOT) com>
To: "Slim Devices Developers" <developers (AT) lists (DOT) slimdevices.com>
Sent: Friday, March 11, 2005 3:11 PM
Subject: Re: [Developers] Re: Remote streaming - serverblocks inProtocols::HTTP::sysread


>
> Adrian,
>
> I think you're correct in identifying the can_read call for the chunky animation behavior. Since the calling code should be able
> to deal with a 0 readlength (and $! set to EWOULDBLOCK), it seems unnecessary.
>
> The blocking code in readMetaData, while not ideal, is similar to what we were doing with 5.4. If you could make the state machine
> in HTTP.pm deal with metadata split across non-blocking reads, definitely go for it.
>
> The call to checkTimers in your loop, if included, should probably just go into idleStreams. To be frank, I'm a little
> uncomfortable with he whole notion of a recursive select loop. If it's working, it's because of luck rather than a careful and
> deliberate understanding of the reentrancy issues. Adding checkTimers to that loop has the potential to increase our risk.
>
> --Vidur
>
> Triode wrote:
>
>>> Dave Cohen wrote some real non-blocking HTTP code that we can get in here.
>>>
>>> The hack that I did was just that - it was meant to alleviate audio skipping.
>>
>>
>> Dan,
>>
>> Does Dave's code cover all of the http code - including readMetaData (which is called from sysread and is also blocking!).
>>
>> I notice that someone has already started to report this as a problem...
>>
>> Anyway, as a quick hack on your hack(!), the attached attempts to avoid the blocking sysread in all cases but the one when
>> metadata is read. It moves the select back into the HTTP->convert() function. At present this is a mini idle loop which
>> includes calling timers - if you don't like this due to the possibility of recursion (not sure it could harm?), it could simply
>> include the select call for a short timeout - say 0.1
>>
>> I think the right answer with Metadata is to read chucks as normal from the tcp session and then parse off the meta data from the
>> start if we expect it - do you have this covered, or is it worth me looking at it?
>>
>> Adrian
>>
>>------------------------------------------------------------------------
>>
>>

vidurapparao
2005-03-11, 08:47
Well...the potential recursion comes from any call to idleStreams (and
they are peppered around the code). The call stack is most likely to
include either a socket handler function or a timer callback before the
call to idleStreams, and the call to idleStreams could result in a
further call to a socket handler or timer callback. HTTP->content() is
not the only place where we may run into a reentrancy issue, so putting
a semaphore around just that method isn't the right fix. Making the
entire codebase reentrant safe is...but that's a huge undertaking. :-)

It seems like adding the checkTimers() call increases risk, but doesn't
specifically help the animation chunkiness while streaming - the
streaming code doesn't call HTTP->content(). Unless you have a specific
case we should consider that benefits from adding the checkTimers()
call, it seems like removing can_read() and making readMetaData
non-blocking are the best options to get back to and improve upon 5.4
behavior.

--Vidur

Triode wrote:

> Vidur,
>
> OK - I think the potential recursion comes from a timer calling
> HTTP->convert which then checked timers and calls another timer which
> also does HTTP->convert.
>
> So assuming we want a simple solution, I would propose to put check
> timers into idleStreams and also put a semaphor on it to ensure it is
> not called if HTTP->convert is called from inside a timer.
>
> Does this sound reasonable? If so I can probably come up with some
> code rapidly for all but the readMetaData bit - which would get us
> back to where 5.4 was?
>
> Adrian
> ----- Original Message ----- From: "Vidur Apparao"
> <vidur (AT) slimdevices (DOT) com>
> To: "Slim Devices Developers" <developers (AT) lists (DOT) slimdevices.com>
> Sent: Friday, March 11, 2005 3:11 PM
> Subject: Re: [Developers] Re: Remote streaming - serverblocks
> inProtocols::HTTP::sysread
>
>
>>
>> Adrian,
>>
>> I think you're correct in identifying the can_read call for the
>> chunky animation behavior. Since the calling code should be able to
>> deal with a 0 readlength (and $! set to EWOULDBLOCK), it seems
>> unnecessary.
>>
>> The blocking code in readMetaData, while not ideal, is similar to
>> what we were doing with 5.4. If you could make the state machine in
>> HTTP.pm deal with metadata split across non-blocking reads,
>> definitely go for it.
>>
>> The call to checkTimers in your loop, if included, should probably
>> just go into idleStreams. To be frank, I'm a little uncomfortable
>> with he whole notion of a recursive select loop. If it's working,
>> it's because of luck rather than a careful and deliberate
>> understanding of the reentrancy issues. Adding checkTimers to that
>> loop has the potential to increase our risk.
>>
>> --Vidur
>>
>> Triode wrote:
>>
>>>> Dave Cohen wrote some real non-blocking HTTP code that we can get
>>>> in here.
>>>>
>>>> The hack that I did was just that - it was meant to alleviate audio
>>>> skipping.
>>>
>>>
>>>
>>> Dan,
>>>
>>> Does Dave's code cover all of the http code - including readMetaData
>>> (which is called from sysread and is also blocking!).
>>>
>>> I notice that someone has already started to report this as a
>>> problem...
>>>
>>> Anyway, as a quick hack on your hack(!), the attached attempts to
>>> avoid the blocking sysread in all cases but the one when metadata is
>>> read. It moves the select back into the HTTP->convert() function.
>>> At present this is a mini idle loop which includes calling timers -
>>> if you don't like this due to the possibility of recursion (not sure
>>> it could harm?), it could simply include the select call for a short
>>> timeout - say 0.1
>>>
>>> I think the right answer with Metadata is to read chucks as normal
>>> from the tcp session and then parse off the meta data from the start
>>> if we expect it - do you have this covered, or is it worth me
>>> looking at it?
>>>
>>> Adrian
>>>
>>> ------------------------------------------------------------------------
>>>
>>>
>>>

Triode
2005-03-11, 09:08
The case I was trying to catch was animation whilst calling HTTP->content() [say from user interface]

What about:

1) Putting a semaphore inside CheckTimers so that it is does not do anything if it is already being run.

2) Removing the can_read and changing the loop in HTTP->content() to call idleStreams with a defined timeout - say 0.1 [otherwise
will spin at high rate!]

3) Changing idleStreams to include a call to check timers (protected by the semaphore in CheckTimers) and also add an optional
calling parameter such that a select time can be set - so the new HTTP->convert() does not spin at a high rate?

[and looking at the parseMetaData bit after this!]

[I'm assuming perl can handle a reentrant call to CheckTimers which returns at the first line if the semaphore is set - getting
beyond my perl knowledge of reentracy issues, but this is a call with no params to function which will not alter any local variables
until it gets to them?]

I can knock something up for you to critique...! Unless you can see a clear problem?

Adrian

----- Original Message -----
From: "Vidur Apparao" <vidur (AT) slimdevices (DOT) com>
To: "Slim Devices Developers" <developers (AT) lists (DOT) slimdevices.com>
Sent: Friday, March 11, 2005 3:47 PM
Subject: Re: [Developers] Re: Remote streaming- serverblocks inProtocols::HTTP::sysread


>
> Well...the potential recursion comes from any call to idleStreams (and they are peppered around the code). The call stack is most
> likely to include either a socket handler function or a timer callback before the call to idleStreams, and the call to idleStreams
> could result in a further call to a socket handler or timer callback. HTTP->content() is not the only place where we may run into
> a reentrancy issue, so putting a semaphore around just that method isn't the right fix. Making the entire codebase reentrant safe
> is...but that's a huge undertaking. :-)
>
> It seems like adding the checkTimers() call increases risk, but doesn't specifically help the animation chunkiness while
> streaming - the streaming code doesn't call HTTP->content(). Unless you have a specific case we should consider that benefits from
> adding the checkTimers() call, it seems like removing can_read() and making readMetaData non-blocking are the best options to get
> back to and improve upon 5.4 behavior.
>
> --Vidur
>
> Triode wrote:
>
>> Vidur,
>>
>> OK - I think the potential recursion comes from a timer calling HTTP->convert which then checked timers and calls another timer
>> which also does HTTP->convert.
>>
>> So assuming we want a simple solution, I would propose to put check timers into idleStreams and also put a semaphor on it to
>> ensure it is not called if HTTP->convert is called from inside a timer.
>>
>> Does this sound reasonable? If so I can probably come up with some code rapidly for all but the readMetaData bit - which would
>> get us back to where 5.4 was?
>>
>> Adrian
>> ----- Original Message ----- From: "Vidur Apparao" <vidur (AT) slimdevices (DOT) com>
>> To: "Slim Devices Developers" <developers (AT) lists (DOT) slimdevices.com>
>> Sent: Friday, March 11, 2005 3:11 PM
>> Subject: Re: [Developers] Re: Remote streaming - serverblocks inProtocols::HTTP::sysread
>>
>>
>>>
>>> Adrian,
>>>
>>> I think you're correct in identifying the can_read call for the chunky animation behavior. Since the calling code should be able
>>> to deal with a 0 readlength (and $! set to EWOULDBLOCK), it seems unnecessary.
>>>
>>> The blocking code in readMetaData, while not ideal, is similar to what we were doing with 5.4. If you could make the state
>>> machine in HTTP.pm deal with metadata split across non-blocking reads, definitely go for it.
>>>
>>> The call to checkTimers in your loop, if included, should probably just go into idleStreams. To be frank, I'm a little
>>> uncomfortable with he whole notion of a recursive select loop. If it's working, it's because of luck rather than a careful and
>>> deliberate understanding of the reentrancy issues. Adding checkTimers to that loop has the potential to increase our risk.
>>>
>>> --Vidur
>>>
>>> Triode wrote:
>>>
>>>>> Dave Cohen wrote some real non-blocking HTTP code that we can get in here.
>>>>>
>>>>> The hack that I did was just that - it was meant to alleviate audio skipping.
>>>>
>>>>
>>>>
>>>> Dan,
>>>>
>>>> Does Dave's code cover all of the http code - including readMetaData (which is called from sysread and is also blocking!).
>>>>
>>>> I notice that someone has already started to report this as a problem...
>>>>
>>>> Anyway, as a quick hack on your hack(!), the attached attempts to avoid the blocking sysread in all cases but the one when
>>>> metadata is read. It moves the select back into the HTTP->convert() function. At present this is a mini idle loop which
>>>> includes calling timers - if you don't like this due to the possibility of recursion (not sure it could harm?), it could simply
>>>> include the select call for a short timeout - say 0.1
>>>>
>>>> I think the right answer with Metadata is to read chucks as normal from the tcp session and then parse off the meta data from
>>>> the start if we expect it - do you have this covered, or is it worth me looking at it?
>>>>
>>>> Adrian
>>>>
>>>> ------------------------------------------------------------------------
>>>>
>>>>
>>>>

vidurapparao
2005-03-11, 09:34
Adrian,

Sounds good - post a patch and I'll discuss with Dan. A reentrancy check
in checkTimers should be fine - since we're using single-threaded Perl,
it doesn't have to be a true semaphore.

--Vidur

Triode wrote:

> The case I was trying to catch was animation whilst calling
> HTTP->content() [say from user interface]
>
> What about:
>
> 1) Putting a semaphore inside CheckTimers so that it is does not do
> anything if it is already being run.
>
> 2) Removing the can_read and changing the loop in HTTP->content() to
> call idleStreams with a defined timeout - say 0.1 [otherwise will spin
> at high rate!]
>
> 3) Changing idleStreams to include a call to check timers (protected
> by the semaphore in CheckTimers) and also add an optional calling
> parameter such that a select time can be set - so the new
> HTTP->convert() does not spin at a high rate?
>
> [and looking at the parseMetaData bit after this!]
>
> [I'm assuming perl can handle a reentrant call to CheckTimers which
> returns at the first line if the semaphore is set - getting beyond my
> perl knowledge of reentracy issues, but this is a call with no params
> to function which will not alter any local variables until it gets to
> them?]
>
> I can knock something up for you to critique...! Unless you can see a
> clear problem?
>
> Adrian
>
> ----- Original Message ----- From: "Vidur Apparao"
> <vidur (AT) slimdevices (DOT) com>
> To: "Slim Devices Developers" <developers (AT) lists (DOT) slimdevices.com>
> Sent: Friday, March 11, 2005 3:47 PM
> Subject: Re: [Developers] Re: Remote streaming- serverblocks
> inProtocols::HTTP::sysread
>
>
>>
>> Well...the potential recursion comes from any call to idleStreams
>> (and they are peppered around the code). The call stack is most
>> likely to include either a socket handler function or a timer
>> callback before the call to idleStreams, and the call to idleStreams
>> could result in a further call to a socket handler or timer callback.
>> HTTP->content() is not the only place where we may run into a
>> reentrancy issue, so putting a semaphore around just that method
>> isn't the right fix. Making the entire codebase reentrant safe
>> is...but that's a huge undertaking. :-)
>>
>> It seems like adding the checkTimers() call increases risk, but
>> doesn't specifically help the animation chunkiness while streaming -
>> the streaming code doesn't call HTTP->content(). Unless you have a
>> specific case we should consider that benefits from adding the
>> checkTimers() call, it seems like removing can_read() and making
>> readMetaData non-blocking are the best options to get back to and
>> improve upon 5.4 behavior.
>>
>> --Vidur
>>
>> Triode wrote:
>>
>>> Vidur,
>>>
>>> OK - I think the potential recursion comes from a timer calling
>>> HTTP->convert which then checked timers and calls another timer
>>> which also does HTTP->convert.
>>>
>>> So assuming we want a simple solution, I would propose to put check
>>> timers into idleStreams and also put a semaphor on it to ensure it
>>> is not called if HTTP->convert is called from inside a timer.
>>>
>>> Does this sound reasonable? If so I can probably come up with some
>>> code rapidly for all but the readMetaData bit - which would get us
>>> back to where 5.4 was?
>>>
>>> Adrian
>>> ----- Original Message ----- From: "Vidur Apparao"
>>> <vidur (AT) slimdevices (DOT) com>
>>> To: "Slim Devices Developers" <developers (AT) lists (DOT) slimdevices.com>
>>> Sent: Friday, March 11, 2005 3:11 PM
>>> Subject: Re: [Developers] Re: Remote streaming - serverblocks
>>> inProtocols::HTTP::sysread
>>>
>>>
>>>>
>>>> Adrian,
>>>>
>>>> I think you're correct in identifying the can_read call for the
>>>> chunky animation behavior. Since the calling code should be able to
>>>> deal with a 0 readlength (and $! set to EWOULDBLOCK), it seems
>>>> unnecessary.
>>>>
>>>> The blocking code in readMetaData, while not ideal, is similar to
>>>> what we were doing with 5.4. If you could make the state machine in
>>>> HTTP.pm deal with metadata split across non-blocking reads,
>>>> definitely go for it.
>>>>
>>>> The call to checkTimers in your loop, if included, should probably
>>>> just go into idleStreams. To be frank, I'm a little uncomfortable
>>>> with he whole notion of a recursive select loop. If it's working,
>>>> it's because of luck rather than a careful and deliberate
>>>> understanding of the reentrancy issues. Adding checkTimers to that
>>>> loop has the potential to increase our risk.
>>>>
>>>> --Vidur
>>>>
>>>> Triode wrote:
>>>>
>>>>>> Dave Cohen wrote some real non-blocking HTTP code that we can get
>>>>>> in here.
>>>>>>
>>>>>> The hack that I did was just that - it was meant to alleviate
>>>>>> audio skipping.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Dan,
>>>>>
>>>>> Does Dave's code cover all of the http code - including
>>>>> readMetaData (which is called from sysread and is also blocking!).
>>>>>
>>>>> I notice that someone has already started to report this as a
>>>>> problem...
>>>>>
>>>>> Anyway, as a quick hack on your hack(!), the attached attempts to
>>>>> avoid the blocking sysread in all cases but the one when metadata
>>>>> is read. It moves the select back into the HTTP->convert()
>>>>> function. At present this is a mini idle loop which includes
>>>>> calling timers - if you don't like this due to the possibility of
>>>>> recursion (not sure it could harm?), it could simply include the
>>>>> select call for a short timeout - say 0.1
>>>>>
>>>>> I think the right answer with Metadata is to read chucks as normal
>>>>> from the tcp session and then parse off the meta data from the
>>>>> start if we expect it - do you have this covered, or is it worth
>>>>> me looking at it?
>>>>>
>>>>> Adrian
>>>>>
>>>>> ------------------------------------------------------------------------
>>>>>
>>>>>
>>>>>

Triode
2005-03-11, 10:45
Vidur,

Patch for review.

Adrian
----- Original Message -----
From: "Vidur Apparao" <vidur (AT) slimdevices (DOT) com>
To: "Slim Devices Developers" <developers (AT) lists (DOT) slimdevices.com>
Sent: Friday, March 11, 2005 4:34 PM
Subject: Re: [Developers] Re:Remote streaming- serverblocks inProtocols::HTTP::sysread


>
> Adrian,
>
> Sounds good - post a patch and I'll discuss with Dan. A reentrancy check
> in checkTimers should be fine - since we're using single-threaded Perl,
> it doesn't have to be a true semaphore.
>
> --Vidur
>
> Triode wrote:
>
>> The case I was trying to catch was animation whilst calling
>> HTTP->content() [say from user interface]
>>
>> What about:
>>
>> 1) Putting a semaphore inside CheckTimers so that it is does not do
>> anything if it is already being run.
>>
>> 2) Removing the can_read and changing the loop in HTTP->content() to
>> call idleStreams with a defined timeout - say 0.1 [otherwise will spin
>> at high rate!]
>>
>> 3) Changing idleStreams to include a call to check timers (protected
>> by the semaphore in CheckTimers) and also add an optional calling
>> parameter such that a select time can be set - so the new
>> HTTP->convert() does not spin at a high rate?
>>
>> [and looking at the parseMetaData bit after this!]
>>
>> [I'm assuming perl can handle a reentrant call to CheckTimers which
>> returns at the first line if the semaphore is set - getting beyond my
>> perl knowledge of reentracy issues, but this is a call with no params
>> to function which will not alter any local variables until it gets to
>> them?]
>>
>> I can knock something up for you to critique...! Unless you can see a
>> clear problem?
>>
>> Adrian
>>
>> ----- Original Message ----- From: "Vidur Apparao"
>> <vidur (AT) slimdevices (DOT) com>
>> To: "Slim Devices Developers" <developers (AT) lists (DOT) slimdevices.com>
>> Sent: Friday, March 11, 2005 3:47 PM
>> Subject: Re: [Developers] Re: Remote streaming- serverblocks
>> inProtocols::HTTP::sysread
>>
>>
>>>
>>> Well...the potential recursion comes from any call to idleStreams
>>> (and they are peppered around the code). The call stack is most
>>> likely to include either a socket handler function or a timer
>>> callback before the call to idleStreams, and the call to idleStreams
>>> could result in a further call to a socket handler or timer callback.
>>> HTTP->content() is not the only place where we may run into a
>>> reentrancy issue, so putting a semaphore around just that method
>>> isn't the right fix. Making the entire codebase reentrant safe
>>> is...but that's a huge undertaking. :-)
>>>
>>> It seems like adding the checkTimers() call increases risk, but
>>> doesn't specifically help the animation chunkiness while streaming -
>>> the streaming code doesn't call HTTP->content(). Unless you have a
>>> specific case we should consider that benefits from adding the
>>> checkTimers() call, it seems like removing can_read() and making
>>> readMetaData non-blocking are the best options to get back to and
>>> improve upon 5.4 behavior.
>>>
>>> --Vidur
>>>
>>> Triode wrote:
>>>
>>>> Vidur,
>>>>
>>>> OK - I think the potential recursion comes from a timer calling
>>>> HTTP->convert which then checked timers and calls another timer
>>>> which also does HTTP->convert.
>>>>
>>>> So assuming we want a simple solution, I would propose to put check
>>>> timers into idleStreams and also put a semaphor on it to ensure it
>>>> is not called if HTTP->convert is called from inside a timer.
>>>>
>>>> Does this sound reasonable? If so I can probably come up with some
>>>> code rapidly for all but the readMetaData bit - which would get us
>>>> back to where 5.4 was?
>>>>
>>>> Adrian
>>>> ----- Original Message ----- From: "Vidur Apparao"
>>>> <vidur (AT) slimdevices (DOT) com>
>>>> To: "Slim Devices Developers" <developers (AT) lists (DOT) slimdevices.com>
>>>> Sent: Friday, March 11, 2005 3:11 PM
>>>> Subject: Re: [Developers] Re: Remote streaming - serverblocks
>>>> inProtocols::HTTP::sysread
>>>>
>>>>
>>>>>
>>>>> Adrian,
>>>>>
>>>>> I think you're correct in identifying the can_read call for the
>>>>> chunky animation behavior. Since the calling code should be able to
>>>>> deal with a 0 readlength (and $! set to EWOULDBLOCK), it seems
>>>>> unnecessary.
>>>>>
>>>>> The blocking code in readMetaData, while not ideal, is similar to
>>>>> what we were doing with 5.4. If you could make the state machine in
>>>>> HTTP.pm deal with metadata split across non-blocking reads,
>>>>> definitely go for it.
>>>>>
>>>>> The call to checkTimers in your loop, if included, should probably
>>>>> just go into idleStreams. To be frank, I'm a little uncomfortable
>>>>> with he whole notion of a recursive select loop. If it's working,
>>>>> it's because of luck rather than a careful and deliberate
>>>>> understanding of the reentrancy issues. Adding checkTimers to that
>>>>> loop has the potential to increase our risk.
>>>>>
>>>>> --Vidur
>>>>>
>>>>> Triode wrote:
>>>>>
>>>>>>> Dave Cohen wrote some real non-blocking HTTP code that we can get
>>>>>>> in here.
>>>>>>>
>>>>>>> The hack that I did was just that - it was meant to alleviate
>>>>>>> audio skipping.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Dan,
>>>>>>
>>>>>> Does Dave's code cover all of the http code - including
>>>>>> readMetaData (which is called from sysread and is also blocking!).
>>>>>>
>>>>>> I notice that someone has already started to report this as a
>>>>>> problem...
>>>>>>
>>>>>> Anyway, as a quick hack on your hack(!), the attached attempts to
>>>>>> avoid the blocking sysread in all cases but the one when metadata
>>>>>> is read. It moves the select back into the HTTP->convert()
>>>>>> function. At present this is a mini idle loop which includes
>>>>>> calling timers - if you don't like this due to the possibility of
>>>>>> recursion (not sure it could harm?), it could simply include the
>>>>>> select call for a short timeout - say 0.1
>>>>>>
>>>>>> I think the right answer with Metadata is to read chucks as normal
>>>>>> from the tcp session and then parse off the meta data from the
>>>>>> start if we expect it - do you have this covered, or is it worth
>>>>>> me looking at it?
>>>>>>
>>>>>> Adrian
>>>>>>
>>>>>> ------------------------------------------------------------------------
>>>>>>
>>>>>>
>>>>>>

Dan Sully
2005-03-11, 10:59
* Triode shaped the electrons to say...

>Vidur,
>
>Patch for review.

Looks good. Adrian - go ahead and check this in.

-D
--
There is no emergency. Nothing to see here. Move along.

vidurapparao
2005-03-11, 10:59
OK. Looks good - check it in (assuming you've done some basic testing).
I still have issues with idleStreams() as a whole, but it's been there
for a while and your change (with the reentrancy check) does not make it
substantially worse.

Thanks,
--Vidur

Triode wrote:

> Vidur,
>
> Patch for review.
>
> Adrian
> ----- Original Message ----- From: "Vidur Apparao"
> <vidur (AT) slimdevices (DOT) com>
> To: "Slim Devices Developers" <developers (AT) lists (DOT) slimdevices.com>
> Sent: Friday, March 11, 2005 4:34 PM
> Subject: Re: [Developers] Re:Remote streaming- serverblocks
> inProtocols::HTTP::sysread
>
>
>>
>> Adrian,
>>
>> Sounds good - post a patch and I'll discuss with Dan. A reentrancy
>> check in checkTimers should be fine - since we're using
>> single-threaded Perl, it doesn't have to be a true semaphore.
>>
>> --Vidur
>>
>> Triode wrote:
>>
>>> The case I was trying to catch was animation whilst calling
>>> HTTP->content() [say from user interface]
>>>
>>> What about:
>>>
>>> 1) Putting a semaphore inside CheckTimers so that it is does not do
>>> anything if it is already being run.
>>>
>>> 2) Removing the can_read and changing the loop in HTTP->content() to
>>> call idleStreams with a defined timeout - say 0.1 [otherwise will
>>> spin at high rate!]
>>>
>>> 3) Changing idleStreams to include a call to check timers (protected
>>> by the semaphore in CheckTimers) and also add an optional calling
>>> parameter such that a select time can be set - so the new
>>> HTTP->convert() does not spin at a high rate?
>>>
>>> [and looking at the parseMetaData bit after this!]
>>>
>>> [I'm assuming perl can handle a reentrant call to CheckTimers which
>>> returns at the first line if the semaphore is set - getting beyond
>>> my perl knowledge of reentracy issues, but this is a call with no
>>> params to function which will not alter any local variables until it
>>> gets to them?]
>>>
>>> I can knock something up for you to critique...! Unless you can see
>>> a clear problem?
>>>
>>> Adrian
>>>
>>> ----- Original Message ----- From: "Vidur Apparao"
>>> <vidur (AT) slimdevices (DOT) com>
>>> To: "Slim Devices Developers" <developers (AT) lists (DOT) slimdevices.com>
>>> Sent: Friday, March 11, 2005 3:47 PM
>>> Subject: Re: [Developers] Re: Remote streaming- serverblocks
>>> inProtocols::HTTP::sysread
>>>
>>>
>>>>
>>>> Well...the potential recursion comes from any call to idleStreams
>>>> (and they are peppered around the code). The call stack is most
>>>> likely to include either a socket handler function or a timer
>>>> callback before the call to idleStreams, and the call to
>>>> idleStreams could result in a further call to a socket handler or
>>>> timer callback. HTTP->content() is not the only place where we may
>>>> run into a reentrancy issue, so putting a semaphore around just
>>>> that method isn't the right fix. Making the entire codebase
>>>> reentrant safe is...but that's a huge undertaking. :-)
>>>>
>>>> It seems like adding the checkTimers() call increases risk, but
>>>> doesn't specifically help the animation chunkiness while streaming
>>>> - the streaming code doesn't call HTTP->content(). Unless you have
>>>> a specific case we should consider that benefits from adding the
>>>> checkTimers() call, it seems like removing can_read() and making
>>>> readMetaData non-blocking are the best options to get back to and
>>>> improve upon 5.4 behavior.
>>>>
>>>> --Vidur
>>>>
>>>> Triode wrote:
>>>>
>>>>> Vidur,
>>>>>
>>>>> OK - I think the potential recursion comes from a timer calling
>>>>> HTTP->convert which then checked timers and calls another timer
>>>>> which also does HTTP->convert.
>>>>>
>>>>> So assuming we want a simple solution, I would propose to put
>>>>> check timers into idleStreams and also put a semaphor on it to
>>>>> ensure it is not called if HTTP->convert is called from inside a
>>>>> timer.
>>>>>
>>>>> Does this sound reasonable? If so I can probably come up with
>>>>> some code rapidly for all but the readMetaData bit - which would
>>>>> get us back to where 5.4 was?
>>>>>
>>>>> Adrian
>>>>> ----- Original Message ----- From: "Vidur Apparao"
>>>>> <vidur (AT) slimdevices (DOT) com>
>>>>> To: "Slim Devices Developers" <developers (AT) lists (DOT) slimdevices.com>
>>>>> Sent: Friday, March 11, 2005 3:11 PM
>>>>> Subject: Re: [Developers] Re: Remote streaming - serverblocks
>>>>> inProtocols::HTTP::sysread
>>>>>
>>>>>
>>>>>>
>>>>>> Adrian,
>>>>>>
>>>>>> I think you're correct in identifying the can_read call for the
>>>>>> chunky animation behavior. Since the calling code should be able
>>>>>> to deal with a 0 readlength (and $! set to EWOULDBLOCK), it seems
>>>>>> unnecessary.
>>>>>>
>>>>>> The blocking code in readMetaData, while not ideal, is similar to
>>>>>> what we were doing with 5.4. If you could make the state machine
>>>>>> in HTTP.pm deal with metadata split across non-blocking reads,
>>>>>> definitely go for it.
>>>>>>
>>>>>> The call to checkTimers in your loop, if included, should
>>>>>> probably just go into idleStreams. To be frank, I'm a little
>>>>>> uncomfortable with he whole notion of a recursive select loop. If
>>>>>> it's working, it's because of luck rather than a careful and
>>>>>> deliberate understanding of the reentrancy issues. Adding
>>>>>> checkTimers to that loop has the potential to increase our risk.
>>>>>>
>>>>>> --Vidur
>>>>>>
>>>>>> Triode wrote:
>>>>>>
>>>>>>>> Dave Cohen wrote some real non-blocking HTTP code that we can
>>>>>>>> get in here.
>>>>>>>>
>>>>>>>> The hack that I did was just that - it was meant to alleviate
>>>>>>>> audio skipping.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Dan,
>>>>>>>
>>>>>>> Does Dave's code cover all of the http code - including
>>>>>>> readMetaData (which is called from sysread and is also blocking!).
>>>>>>>
>>>>>>> I notice that someone has already started to report this as a
>>>>>>> problem...
>>>>>>>
>>>>>>> Anyway, as a quick hack on your hack(!), the attached attempts
>>>>>>> to avoid the blocking sysread in all cases but the one when
>>>>>>> metadata is read. It moves the select back into the
>>>>>>> HTTP->convert() function. At present this is a mini idle loop
>>>>>>> which includes calling timers - if you don't like this due to
>>>>>>> the possibility of recursion (not sure it could harm?), it could
>>>>>>> simply include the select call for a short timeout - say 0.1
>>>>>>>
>>>>>>> I think the right answer with Metadata is to read chucks as
>>>>>>> normal from the tcp session and then parse off the meta data
>>>>>>> from the start if we expect it - do you have this covered, or is
>>>>>>> it worth me looking at it?
>>>>>>>
>>>>>>> Adrian
>>>>>>>
>>>>>>> ------------------------------------------------------------------------
>>>>>>>
>>>>>>>
>>>>>>>

Triode
2005-03-11, 14:09
Just committed a slightly revised version of this which avoids an undef warning.

However I've found it doesn't cover all cases as far as the scrolling is concerned as many times HTTP->content is called from within
a timer callback!

How you do feel about me adding a second timer queue for animations which are able to run inside a non animation timer function.
Essentially checkTimers would have two semaphores to check - first it would check the high priority semaphone and dispatch any
(one?) timer on the high priority queue, then it would check the normal semaphone before being able to dispatch any normal timers.

Adrian
----- Original Message -----
From: "Dan Sully" <dan (AT) slimdevices (DOT) com>
To: "Slim Devices Developers" <developers (AT) lists (DOT) slimdevices.com>
Sent: Friday, March 11, 2005 5:59 PM
Subject: [Developers] Re: Re:Remote streaming-serverblocksinProtocols::HTTP::sysread


>* Triode shaped the electrons to say...
>
>>Vidur,
>>
>>Patch for review.
>
> Looks good. Adrian - go ahead and check this in.
>
> -D
> --
> There is no emergency. Nothing to see here. Move along.
>

Dan Sully
2005-03-11, 14:15
* Triode shaped the electrons to say...

>Just committed a slightly revised version of this which avoids an undef
>warning.
>
>However I've found it doesn't cover all cases as far as the scrolling is
>concerned as many times HTTP->content is called from within a timer
>callback!
>
>How you do feel about me adding a second timer queue for animations which
>are able to run inside a non animation timer function. Essentially
>checkTimers would have two semaphores to check - first it would check the
>high priority semaphone and dispatch any (one?) timer on the high priority
>queue, then it would check the normal semaphone before being able to
>dispatch any normal timers.

Go for it.

-D
--
Sir, are you classified as human?
Uhh, negative, I am a meat popsicle.

kdf
2005-03-12, 02:11
Quoting Triode <triode1 (AT) btinternet (DOT) com>:

> Just committed a slightly revised version of this which avoids an undef
> warning.
>
for some reason, the SB2 is no longer scrolling. I haven't tested softsqueeze
yet. This is with FW5, svn 2442.

-kdf

kdf
2005-03-12, 02:45
Quoting kdf <slim-mail (AT) deane-freeman (DOT) com>:

> Quoting Triode <triode1 (AT) btinternet (DOT) com>:
>
> > Just committed a slightly revised version of this which avoids an undef
> > warning.
> >
> for some reason, the SB2 is no longer scrolling. I haven't tested
> softsqueeze
> yet. This is with FW5, svn 2442.
>

nevermind. must have just been tempermental. after a few restarts and updates
to find which revision caused it, it went away.

-kdf

kdf
2005-03-12, 02:52
After a restart of the server, all is well. After a period of time, however,
the SB no longer scrolls. I've been trying to figure out why I'm reading 10%
signal strength so I'm going in to settings->information->player info a fair
bit. After being in there, scrolling no longer happens. resetting SB2 doesn't
help. I have to restart the server.

I'm not sure yet if the settings menu is what is causing it to stop but its the
only thing I've done that manages to always have no scrolling after.

-kdf

Triode
2005-03-12, 04:50
If you run with --d_time do you get any "firing timer" messages when before and after the stuck mode?

Looks like we do need to allow animations whilst in the middle of a timer callback, but useful to check.

Adrian
----- Original Message -----
From: "kdf" <slim-mail (AT) deane-freeman (DOT) com>
To: "Slim Devices Developers" <developers (AT) lists (DOT) slimdevices.com>
Sent: Saturday, March 12, 2005 9:52 AM
Subject: [Developers] scrolling on SB2


>
> After a restart of the server, all is well. After a period of time, however,
> the SB no longer scrolls. I've been trying to figure out why I'm reading 10%
> signal strength so I'm going in to settings->information->player info a fair
> bit. After being in there, scrolling no longer happens. resetting SB2 doesn't
> help. I have to restart the server.
>
> I'm not sure yet if the settings menu is what is causing it to stop but its the
> only thing I've done that manages to always have no scrolling after.
>
> -kdf
>

kdf
2005-03-12, 04:58
Quoting Triode <triode1 (AT) btinternet (DOT) com>:

> If you run with --d_time do you get any "firing timer" messages when before
> and after the stuck mode?
>
> Looks like we do need to allow animations whilst in the middle of a timer
> callback, but useful to check.

they are flying by at lightning speed before, and even as I stare at a now
static display. The trigger seems to be when I go into
settings->information->player information and navigate to the MAC address line.
That line doens't scroll, and nothing scrolls ever after. I havne't managed
to trigger this any other way so far.

-kdf

Triode
2005-03-12, 05:11
And to be certain I am clear on this - this scolling stops on SBG when you do this on SB2?

Doesn't seem to do this for me on Softsqueeze and SBG - I do see the Softsqueeze display resetting scrolling on Softsqueeze too
frequently when displaying the mac address..

I'll look at the code around player settings...

Adrian

----- Original Message -----
From: "kdf" <slim-mail (AT) deane-freeman (DOT) com>
To: "Slim Devices Developers" <developers (AT) lists (DOT) slimdevices.com>
Sent: Saturday, March 12, 2005 11:58 AM
Subject: Re: [Developers] scrolling on SB2


> Quoting Triode <triode1 (AT) btinternet (DOT) com>:
>
>> If you run with --d_time do you get any "firing timer" messages when before
>> and after the stuck mode?
>>
>> Looks like we do need to allow animations whilst in the middle of a timer
>> callback, but useful to check.
>
> they are flying by at lightning speed before, and even as I stare at a now
> static display. The trigger seems to be when I go into
> settings->information->player information and navigate to the MAC address line.
> That line doens't scroll, and nothing scrolls ever after. I havne't managed
> to trigger this any other way so far.
>
> -kdf
>

Triode
2005-03-12, 06:51
Kdf,

There's a call to update in Slim::Buttons::Information::updateSignalStrength

This wrecks scrolling whenever the menu mode is in settings->information->player information (even if a screensaver becomes active
and hence you are not looking at any player information) [I am hoping this is what you are seeing!]

Given the way the screensavers update the screen, it seems to work for me if the update call is removed (this function is then
responsible for ensuring there is an up to date view of signal strength, but not forcing a screen redraw.) Does this work for you
on all players?

Adrian

----- Original Message -----
From: "Triode" <triode1 (AT) btinternet (DOT) com>
To: "Slim Devices Developers" <developers (AT) lists (DOT) slimdevices.com>
Sent: Saturday, March 12, 2005 12:11 PM
Subject: Re: [Developers] scrolling on SB2


> And to be certain I am clear on this - this scolling stops on SBG when you do this on SB2?
>
> Doesn't seem to do this for me on Softsqueeze and SBG - I do see the Softsqueeze display resetting scrolling on Softsqueeze too
> frequently when displaying the mac address..
>
> I'll look at the code around player settings...
>
> Adrian
>
> ----- Original Message -----
> From: "kdf" <slim-mail (AT) deane-freeman (DOT) com>
> To: "Slim Devices Developers" <developers (AT) lists (DOT) slimdevices.com>
> Sent: Saturday, March 12, 2005 11:58 AM
> Subject: Re: [Developers] scrolling on SB2
>
>
>> Quoting Triode <triode1 (AT) btinternet (DOT) com>:
>>
>>> If you run with --d_time do you get any "firing timer" messages when before
>>> and after the stuck mode?
>>>
>>> Looks like we do need to allow animations whilst in the middle of a timer
>>> callback, but useful to check.
>>
>> they are flying by at lightning speed before, and even as I stare at a now
>> static display. The trigger seems to be when I go into
>> settings->information->player information and navigate to the MAC address line.
>> That line doens't scroll, and nothing scrolls ever after. I havne't managed
>> to trigger this any other way so far.
>>
>> -kdf
>>

kdf
2005-03-12, 12:35
Quoting Triode <triode1 (AT) btinternet (DOT) com>:

> And to be certain I am clear on this - this scolling stops on SBG when you do
> this on SB2?

it stops on SB2 when I got into settings ->playerinfo->mac address, ,on the SB2.
I'm not really sure it has anything to do with your scrolling code. In every
other case, its working fine. :)


> Doesn't seem to do this for me on Softsqueeze and SBG - I do see the
> Softsqueeze display resetting scrolling on Softsqueeze too
> frequently when displaying the mac address..
>
> I'll look at the code around player settings...

hopefully you will have hardware to play with soon.
-kdf

kdf
2005-03-12, 12:43
Quoting Triode <triode1 (AT) btinternet (DOT) com>:

> Kdf,
>
> There's a call to update in Slim::Buttons::Information::updateSignalStrength
>
> This wrecks scrolling whenever the menu mode is in
> settings->information->player information (even if a screensaver becomes
> active
> and hence you are not looking at any player information) [I am hoping this
> is what you are seeing!]
>
> Given the way the screensavers update the screen, it seems to work for me if
> the update call is removed (this function is then
> responsible for ensuring there is an up to date view of signal strength, but
> not forcing a screen redraw.) Does this work for you
> on all players?
>
thanks, that seems to be fixing it. so, calling a client->update() specifically
has now become a bad thing? I'm confused on that one.

-kdf

Triode
2005-03-12, 12:48
Kdf,

Well it wrecks the scrolling stuff as it updates the screen with a non scrolled version.

This code is doing it all the time that the current menu is set to settings->information->player information.

Assuming we can rely on screensave to always update the screen, I think we should remove update from here?

Adrian
----- Original Message -----
From: "kdf" <slim-mail (AT) deane-freeman (DOT) com>
To: "Slim Devices Developers" <developers (AT) lists (DOT) slimdevices.com>
Sent: Saturday, March 12, 2005 7:43 PM
Subject: Re: [Developers] scrolling on SB2


> Quoting Triode <triode1 (AT) btinternet (DOT) com>:
>
>> Kdf,
>>
>> There's a call to update in Slim::Buttons::Information::updateSignalStrength
>>
>> This wrecks scrolling whenever the menu mode is in
>> settings->information->player information (even if a screensaver becomes
>> active
>> and hence you are not looking at any player information) [I am hoping this
>> is what you are seeing!]
>>
>> Given the way the screensavers update the screen, it seems to work for me if
>> the update call is removed (this function is then
>> responsible for ensuring there is an up to date view of signal strength, but
>> not forcing a screen redraw.) Does this work for you
>> on all players?
>>
> thanks, that seems to be fixing it. so, calling a client->update() specifically
> has now become a bad thing? I'm confused on that one.
>
> -kdf
>

kdf
2005-03-12, 12:57
Quoting Triode <triode1 (AT) btinternet (DOT) com>:

> Kdf,
>
> Well it wrecks the scrolling stuff as it updates the screen with a non
> scrolled version.
>
> This code is doing it all the time that the current menu is set to
> settings->information->player information.
>
> Assuming we can rely on screensave to always update the screen, I think we
> should remove update from here?

hrm. strangely this seems to permanently stop scrolling, even after I left that
mode. Without it, it is working fine. This concerns me becuase there are
several cases where $client->update is used to force an update of the line
text. If using this now stops scrolling for good, I expect some plugins will
cause problems.

-kdf