Gotham - seeking on some http streams broken

  Thread Rating:
  • 0 Votes - 0 Average
  • 1
  • 2
  • 3
  • 4
  • 5
Post Reply
althekiller Offline
Team-XBMC Developer
Posts: 5,001
Joined: May 2004
Reputation: 12
Post: #16
There's only one point that there are three CReadStates in play there, lines 1172-1210. Give this patch a try: http://nopaste.linux-dev.org/?147629. It eliminates the third CReadState instance, but I didn't really check for repercussions.
find quote
ngoal Offline
Junior Member
Posts: 8
Joined: Feb 2013
Reputation: 0
Post: #17
How would I go about adding this patch to the android version
find quote
Martijn Offline
Team-XBMC
Posts: 10,966
Joined: Jul 2011
Reputation: 158
Location: Dawn of time
Post: #18
(2014-04-08 19:32)ngoal Wrote:  How would I go about adding this patch to the android version

Magic

Always read the XBMC online-manual, FAQ and search the forums before posting.
Do NOT e-mail Team-XBMC members asking for support. Read/follow the forum rules.
For troubleshooting and bug reporting, make sure you read this first

For your mediacenter artwork go to
[Image: fanarttv.png]
find quote
althekiller Offline
Team-XBMC Developer
Posts: 5,001
Joined: May 2004
Reputation: 12
Post: #19
(2014-04-08 19:32)ngoal Wrote:  How would I go about adding this patch to the android version

Patience.
find quote
DBMandrake Offline
Fan
Posts: 354
Joined: Feb 2013
Reputation: 8
Location: UK
Post: #20
(2014-04-08 17:55)althekiller Wrote:  There's only one point that there are three CReadStates in play there, lines 1172-1210. Give this patch a try: http://nopaste.linux-dev.org/?147629. It eliminates the third CReadState instance, but I didn't really check for repercussions.
Thanks, I'll do some testing of your patch tomorrow versus disabling multisession entirely as in my hack.

(2014-04-08 19:32)ngoal Wrote:  How would I go about adding this patch to the android version
Sorry, can't help you there. I have no android devices and know nothing about how to compile an android version of XBMC. I linked to the source code of the patch on github above so anyone who can compile the android version could easily compile that version.
(This post was last modified: 2014-04-08 23:26 by DBMandrake.)
find quote
ngoal Offline
Junior Member
Posts: 8
Joined: Feb 2013
Reputation: 0
Post: #21
Dbmandrake thanks for all the hard work. Hopefully your effort ends up in the next beta.
find quote
DBMandrake Offline
Fan
Posts: 354
Joined: Feb 2013
Reputation: 8
Location: UK
Post: #22
(2014-04-08 17:55)althekiller Wrote:  There's only one point that there are three CReadStates in play there, lines 1172-1210. Give this patch a try: http://nopaste.linux-dev.org/?147629. It eliminates the third CReadState instance, but I didn't really check for repercussions.
Just tried this and it does reduce maximum simultaneous connection to two which certainly improves matters, (but isn't as robust as turning multisession off entirely with limited connections available) but in the process its reverting a change deliberately made in ulion's original commit. I'm trying to understand what the benefit of having up to three simultaneous connections is, why it was done, and what the drawback of limiting it to two connections might be ?

I'm wondering if the best compromise is to limit it to a maximum of two simultaneous connections as per your patch, (allowing the benefits of multiple connections in most cases - whatever those benefits might be) AND adding a check where the first time an additional connection for a seek fails m_multisession is set to false to gracefully fall back to no multisession and the seek is re-tried without multisession. (And multisession remains disabled for the rest of the playback for that particular stream)

Sound reasonable ? We want it to be as robust as possible when servers limit the number of available simultaneous connections to one or two. (It's also a bit unfair on the servers even if they allow it that a single client playing a video may be consuming up to three connection slots for the duration of playback when extra connections are not really needed except when seeking. With lots of XBMC clients that quickly mounts up to a large number of open connections per server and could cause server load issues when Gotham goes into general release)
(This post was last modified: 2014-04-09 11:36 by DBMandrake.)
find quote
DBMandrake Offline
Fan
Posts: 354
Joined: Feb 2013
Reputation: 8
Location: UK
Post: #23
A bit more digging and I think the main problem comes down to a logical error in this section of CCurlFile::Seek:

https://github.com/xbmc/xbmc/blob/master...1196-L1207

Code:
long response = m_state->Connect(m_bufferSize);
  if(response < 0 && (m_state->m_fileSize == 0 || m_state->m_fileSize != m_state->m_filePos))
  {
    m_seekable = false;
    if(m_multisession && m_oldState)
    {
      delete m_state;
      m_state = m_oldState;
      m_oldState = oldstate;
    }
    return -1;
  }
This is where a new connection is opened during a seek - if it fails m_seekable is set to false and the stream is no longer seekable. However this decision does not take into account or keep track of how many other sessions may already be open for this stream due to previous seeks.

So for example where two simultaneous connections are allowed, on the initial connection one session is opened successfully and the video plays, on the first seek a second connection is opened successfully and the seek succeeds (with the original connection still left open) on the second seek attempt a third connection is opened and fails (either immediately due to an http error code or due to a timeout depending on how the server limits connections) which then causes the if statement to be true, thus setting m_seekable to false and returning -1 to the caller.

The bug is that even if we already have two sessions open for the stream, (which is more than ample to seek with) just because the third connection fails we give up and decide the stream is unseekable when we should just disable multisession entirely, or drop back to one less session. So it should be setting m_multisession false (and doing any necessary cleanup and re-trying the request on an existing handle) in this if statement, not setting m_seekable to false and giving up. (Or at least not unless this is the only session open)

I'm still studying it to see if I can find a simple and not too invasive fix that would allow multisession to still work when possible but fall back properly when additional sessions fail...
(This post was last modified: 2014-04-09 14:44 by DBMandrake.)
find quote
Kib Offline
Team XBMC
Posts: 3,194
Joined: Jan 2010
Reputation: 53
Location: NL
Post: #24
Could you not instead of setting m_seekable to false, check if m_multisession is true, and set to false instead?
If not true keep old way of working?

I'm not a coder so ignore if this is useless Smile

Neon skin - v3.0.7 now available for XBMC 13 (Gotham)
find quote
DBMandrake Offline
Fan
Posts: 354
Joined: Feb 2013
Reputation: 8
Location: UK
Post: #25
(2014-04-09 14:43)Kib Wrote:  Could you not instead of setting m_seekable to false, check if m_multisession is true, and set to false instead?
If not true keep old way of working?
That by itself wouldn't be enough - the connection for the seek failed to return any data so as well as marking m_multisession false (so we don't try to use extra sessions for the next seek) we need to find the handle of a previous curl session and reuse it to retry the request, or close a previous idle connection handle then open a new one to keep the connection count down. (It's still not clear to me exactly how the code works - I'm doing a lot of single stepping while monitoring the network connections at the server end to get to grips with it Smile )
Quote:I'm not a coder so ignore if this is useless Smile
I'm not a coder either, I just pretend to be occasionally... Tongue
(This post was last modified: 2014-04-09 14:51 by DBMandrake.)
find quote
wsnipex Online
Team-XBMC packaging monkey
Posts: 3,476
Joined: Jun 2011
Reputation: 84
Post: #26
I guess we need to keep track of how many concurrent connections are allowed with a counter and a max of sessions.
We shouldn't even set multisession to false in case a 3rd connection fails, only if a 2nd fails.
find quote
DBMandrake Offline
Fan
Posts: 354
Joined: Feb 2013
Reputation: 8
Location: UK
Post: #27
(2014-04-09 15:13)wsnipex Wrote:  I guess we need to keep track of how many concurrent connections are allowed with a counter and a max of sessions.
We shouldn't even set multisession to false in case a 3rd connection fails, only if a 2nd fails.
There is a maximum of three sessions now due to the following code rotating stuff through m_state, m_oldstate, and oldstate:

https://github.com/xbmc/xbmc/blob/master...1172-L1185

So we can tell how many connections we already have open by checking m_oldstate and oldstate != NULL ? As you say a counter to keep track of the previously "probed" number of connections may still be needed, so that when a 3rd connection fails we make a note that up to two is ok, or if a 2nd connection fails just disable multisession altogether. As there are only three possibilities (1, 2 or 3 simultaneous connections allowed, as the code never goes above 3) it could just be special cased with another flag (m_multisession3 ?) instead of a counter.
(This post was last modified: 2014-04-09 15:30 by DBMandrake.)
find quote
althekiller Offline
Team-XBMC Developer
Posts: 5,001
Joined: May 2004
Reputation: 12
Post: #28
Well we need the patch I posted regardless as there's the possibility to leak oldstate on that silly early return. I agree that disabling multi session if a second connection fails is probably the right way to go.
find quote
DBMandrake Offline
Fan
Posts: 354
Joined: Feb 2013
Reputation: 8
Location: UK
Post: #29
(2014-04-09 15:40)althekiller Wrote:  Well we need the patch I posted regardless as there's the possibility to leak oldstate on that silly early return. I agree that disabling multi session if a second connection fails is probably the right way to go.
Ah good, so you think limiting it to a maximum of two as per your patch is preferable, and that there's no good reason to use as many as three ?

If we can do that, and put in a check that detects a failed 2nd session and disables multisession that would make the problem MUCH simpler and cleaner to solve.

I'll go back to your patch as a base and see if I can build graceful fall-back to a single session on top of that.
find quote
DBMandrake Offline
Fan
Posts: 354
Joined: Feb 2013
Reputation: 8
Location: UK
Post: #30
(2014-04-09 15:40)althekiller Wrote:  Well we need the patch I posted regardless as there's the possibility to leak oldstate on that silly early return. I agree that disabling multi session if a second connection fails is probably the right way to go.
Might have something here, only about 3 lines changed beyond what you changed, and it seems to work, but I need someone that understands the code properly to sanity check it: Smile

https://github.com/DBMandrake/xbmc/commi...7426ee1344

Complete version:

https://github.com/DBMandrake/xbmc/blob/...1195-L1208

The rationale behind the change is when the session has failed, if (m_multisession && m_oldState) is true we're in multisession and we are the second session attempt (m_oldState exists due to the previously opened session) so we set m_multisession=false and exit with return code -1 but we do NOT set m_seekable=false. (That is now in an else statement)

However if (m_multisession && m_oldState) is false, we have either already disable multisession for some reason, or we are the first session attempt. In either case a failed connection marks m_seekable=false and returns.

In my brief testing with two simultaneous connections available it will use up to two connections as per your patch and seek normally, however in the case where I limit it to only one simultaneous connection (where it would previously fail to seek on a wmv or play at all on an mkv) it seems that when we set m_multisession=false and return with -1 the calling function calls our seek function a second time it succeeds this time as m_multisession is now false.

So it doesn't seem to be necessary to write any re-try code into this function to try the connection again as the caller simply calls the seek function again ? Looks promising anyway for such a small change - we get to make use of two sessions where servers allow it but fall back transparently to a single session (and still with an ability to seek) when a server only allows one simultaneous connection attempt. I'll do plenty more testing on it of course to check for corner cases.
(This post was last modified: 2014-04-09 17:17 by DBMandrake.)
find quote
Post Reply