Gotham - seeking on some http streams broken

  Thread Rating:
  • 0 Votes - 0 Average
  • 1
  • 2
  • 3
  • 4
  • 5
Post Reply
althekiller Offline
Retired Team-Kodi Member
Posts: 5,002
Joined: May 2004
Reputation: 12
Post: #46
@DBMandrake, I just pushed a fourth commit that should fix the issue you mentioned with the refactor commit.
find quote
DBMandrake Offline
Fan
Posts: 453
Joined: Feb 2013
Reputation: 14
Location: UK
Post: #47
(2014-04-10 15:18)ulion Wrote:  fixed in the PR, you are right, it mixed a little my other modified code, just change the problem line to:

SetCommonOptions(m_state);

would be enough for make it work.
Thanks, I'll have another try at compiling your commits tomorrow and run some tests to see how they compare to althekiller's.

(2014-04-10 15:21)althekiller Wrote:  @DBMandrake, I just pushed a fourth commit that should fix the issue you mentioned with the refactor commit.
Thanks - I've just applied all four commits and compiled on the Mac - the connections do switch around properly now after each seek as they did before the refactor commit.

I'm going to do some real world testing using add-ons on the Mac tonight running this build, then do some more local network tests and single stepping tomorrow on windows to try to verify the changed logic in corner cases to make sure nothing else has been missed, but it's looking promising so far, and I do like the elegance of the recursive call to retry the request without multisession rather than duplicating lots of code. Smile

XBMC 13.2 - Mid 2007 Mac Mini, 4GB, 2TB HD, OS X 10.6.8.
XBMC 13.2 - Raspberry Pi 512MB Model B, 16GB SD, Raspbmc.
(This post was last modified: 2014-04-10 17:53 by DBMandrake.)
find quote
althekiller Offline
Retired Team-Kodi Member
Posts: 5,002
Joined: May 2004
Reputation: 12
Post: #48
Thanks for your efforts DBMandrake. A PR was just posted for the other bug that is holding Gotham off from moving to Release Candidate status. So the sooner we can get this one confirmed, the better.
find quote
althekiller Offline
Retired Team-Kodi Member
Posts: 5,002
Joined: May 2004
Reputation: 12
Post: #49
I've PR'd my attempt at https://github.com/xbmc/xbmc/pull/4546 . Perhaps it will be less arduous to review now.

Edit for dumb forum adding the period to my URL.
(This post was last modified: 2014-04-10 19:57 by althekiller.)
find quote
DBMandrake Offline
Fan
Posts: 453
Joined: Feb 2013
Reputation: 14
Location: UK
Post: #50
(2014-04-10 18:00)althekiller Wrote:  Thanks for your efforts DBMandrake. A PR was just posted for the other bug that is holding Gotham off from moving to Release Candidate status. So the sooner we can get this one confirmed, the better.
Working my way through a series of tests just now. Your refactor (3rd and 4th commits together) seem to have fixed another very minor issue I had noticed but not mentioned with only commits 1&2 applied.

With only 1&2, if a second connection failed the first time you tried to open one (first seek attempt) it did fall back to multisession disabled properly, so in the case of a server that always only supported one connection it worked fine, however if the first attempt to seek and open a second connection succeeded - but then on any subsequent seek attempt a connection failed it did NOT fall back to multisession disabled and video playback was just aborted altogether. It seems fallback only worked on the first seek attempt.

A very unlikely scenario, granted, which is why I didn't originally mention it. To trigger it I had to start with apache set to 2 connections, start playback, seek at least once, then change apache to 1 connection, then seek again - with the old code the playback would then fail, with the refactored code it drops back to multisession disabled and continues playing, and continues to be able to seek Smile I'm assuming there was a logic error in one of the if's in the code you refactored.

I notice that on servers with round robin dns each subsequent connection attempt can be to a different ip address, so if different servers in the round robin list had different connection limits (or one server was overloaded causing a single connection to fail in the round robin list) it could in theory be hit in the real world, so it's nice that this code fixes that too. As robust as possible is always nice.

On to more testing...

XBMC 13.2 - Mid 2007 Mac Mini, 4GB, 2TB HD, OS X 10.6.8.
XBMC 13.2 - Raspberry Pi 512MB Model B, 16GB SD, Raspbmc.
(This post was last modified: 2014-04-11 11:42 by DBMandrake.)
find quote
DBMandrake Offline
Fan
Posts: 453
Joined: Feb 2013
Reputation: 14
Location: UK
Post: #51
(2014-04-10 15:18)ulion Wrote:  fixed in the PR, you are right, it mixed a little my other modified code, just change the problem line to:

SetCommonOptions(m_state);

would be enough for make it work.
I applied the updated versions of your two commits on top of master and it compiled successfully this time. Smile Initial testing shows some problems though.

The first is that it is still attempting to use three simultaneous connections when seeking instead of two. I'm not sure whether it was meant to apply on top of althekiller's first commit which fixed this:

https://github.com/t-nelson/xbmc/commit/...a242489163

...or whether you were meaning to include the same changes in your commit. In single stepping it looks like it's opening the 3rd connection before closing the 1st connection instead of the other way around.

On a server limited to two connections it still tries to open a 3rd connection on the first seek attempt, when that connection fails it's dropping back to multisession disabled which is a bit unnecessary when two connections would be usable.

In some conditions it's also opening two connections during initial playback instead of one of althekiller's version of the fix.

So as it stands just now this patch definitely needs some more work, while althekiller's seems to be ready to go and also looks a bit cleaner and easier to understand to me with a bit less code duplication. It's up to you guys to decide which way to go of course - if you want me to test any further changes to your version of the patch I'm happy to do so.

Edit: a little bit more testing and there might be some sort of race condition causing the 3rd connection overlap. If I set a breakpoint at the start of CCurlFile::Seek and quickly resume the breakpoint when seeking there is a 3rd connection overlap, however if I let it sit at the breakpoint for a few seconds one of the open connections closes (automatically cleaned up ?) and the three connection overlap doesn't occur.

So the behaviour is different when running in real time or when single stepped slowly. Sad Perhaps an explicit m_state->Disconnect(); is missing where one should be ?

XBMC 13.2 - Mid 2007 Mac Mini, 4GB, 2TB HD, OS X 10.6.8.
XBMC 13.2 - Raspberry Pi 512MB Model B, 16GB SD, Raspbmc.
(This post was last modified: 2014-04-11 12:55 by DBMandrake.)
find quote
DBMandrake Offline
Fan
Posts: 453
Joined: Feb 2013
Reputation: 14
Location: UK
Post: #52
(2014-04-10 17:46)DBMandrake Wrote:  I'm going to do some real world testing using add-ons on the Mac tonight running this build,
In reply to myself, I did do some real world testing of althekiller's patch last night on my Mac using various add-ons and did not notice any problems, only improvements.

I monitored outgoing TCP sessions with netstat via ssh to see how many connections were open at once and how the port numbers changed with each seek. From this I could see that in some cases (more often than not) two simultaneous connections were open after first seek and the port numbers rotated as expected, (oldest connection dropped before newest connection opened etc) and seeking worked perfectly. In no instance did I see more than two simultaneous connections. In other cases I could see that only a single connection was open and the port number incremented with each seek - indicating that a fallback to m_multisession=false had occurred, but if I hadn't been monitoring with netstat (or debugging) I would have never known as seeking was still working perfectly with no sign of trouble.

On a rare occasion on overloaded servers that intermittently return a connection failure I did lose the connection altogether on a seek attempt (causing playback to abort) - but there's not much we can do about that, if we're only allowed one connection to the server and have to drop the connection and make a new one to request a new range and the server refuses the new connection due to temporary overloading (or a load balancer problem) we're screwed anyway. The same thing happened without this patch, and I've seen aborted playback on seek plenty of times on Frodo too - the only other alternative is to never allow seeking without multisession which seems too drastic.

You either allow seeking without multisession which means you can always seek on any server that supports http resume regardless of number of connections, but run a small risk of losing the connection on seek if the server returns intermittent connection failures, or you only allow seeking with multisession using a second connection which means you don't drop an old connection until a new one is up and running, but then you can never risk seeking on servers that only allow one connection. If you lost the connection for some other reason you're stuck never being able to jump back to where you were and having to fast forward from the beginning...

Servers that only allow one connection were more common than I expected in my anecdotal testing last night - those servers would have been un-seekable on Frodo, but can now with this patch be seeked, so at least if you did lose your connection for some reason you can jump right back to where you were. (I didn't encounter any servers where I couldn't seek with this patch)

XBMC 13.2 - Mid 2007 Mac Mini, 4GB, 2TB HD, OS X 10.6.8.
XBMC 13.2 - Raspberry Pi 512MB Model B, 16GB SD, Raspbmc.
(This post was last modified: 2014-04-11 13:37 by DBMandrake.)
find quote
ulion Offline
Team-Kodi Member
Posts: 84
Joined: Dec 2012
Reputation: 0
Post: #53
1. for single session server:
backup current state to m_oldState, then init 2nd session to seek, failed, then we mark m_multisession to false, restore the m_oldState to m_state, disconnect it and re-connect to seek

2. for double session server:
backup current state to m_oldState, then init 2nd session to seek, success, then work on the 2 sessions, when do additional seek out of the 2 states, it stack down current 2 states, create a new session (3rd) to seek, then failed, then mark m_multisession to false, but we still have the previous 2 success states, and will keep using max 2 sessions (close oldest state when init new state)

3. server support more sessions, just like above, but the 3rd session always success, then it will close the oldest state after the 3rd session seek done.


so it will try the 3rd session, then it failed, from then on, it only use 2 sessions. don's see how many sessions it used, just see whether it works without problem with all kinds of servers.
find quote
DBMandrake Offline
Fan
Posts: 453
Joined: Feb 2013
Reputation: 14
Location: UK
Post: #54
What is the benefit of trying to open a third connection when two are already open ?

XBMC 13.2 - Mid 2007 Mac Mini, 4GB, 2TB HD, OS X 10.6.8.
XBMC 13.2 - Raspberry Pi 512MB Model B, 16GB SD, Raspbmc.
find quote
ulion Offline
Team-Kodi Member
Posts: 84
Joined: Dec 2012
Reputation: 0
Post: #55
after re-consideration, since when init the 3rd session, we always close the oldState, then indeed we can close it first, to avoid the 3rd session. the 3rd commit in the PR shorten the code and limit max 2 sessions.
find quote
DBMandrake Offline
Fan
Posts: 453
Joined: Feb 2013
Reputation: 14
Location: UK
Post: #56
(2014-04-11 14:06)ulion Wrote:  after re-consideration, since when init the 3rd session, we always close the oldState, then indeed we can close it first, to avoid the 3rd session. the 3rd commit in the PR shorten the code and limit max 2 sessions.
I'll try it out.

Have you finished making changes to this commit as when I tried to cherry pick it I was getting "fatal: bad revision", then found when I refreshed the github page the SHA had changed in the last few minutes. Wink

I have 9789ebcd58169c53f570727ede797e64f93a7dbe.

XBMC 13.2 - Mid 2007 Mac Mini, 4GB, 2TB HD, OS X 10.6.8.
XBMC 13.2 - Raspberry Pi 512MB Model B, 16GB SD, Raspbmc.
(This post was last modified: 2014-04-11 14:21 by DBMandrake.)
find quote
ulion Offline
Team-Kodi Member
Posts: 84
Joined: Dec 2012
Reputation: 0
Post: #57
modify already done, one place replace check m_oldState to check m_multisession, the same effect but more reasonable.
find quote
DBMandrake Offline
Fan
Posts: 453
Joined: Feb 2013
Reputation: 14
Location: UK
Post: #58
Something still not right. I'm still seeing some 3 connection overlaps seeking a wmv, although the overlap only lasts about 5 seconds.

Putting breakpoints on m_state->Disconnect(); and long response = m_state->Connect(m_bufferSize); show that when I seek the Seek function is called three times in a row (presumably seeking to index data and back etc) and the Connect function is called three times with Disconnect never called. If I pause at the breakpoints for a few seconds one of the old connections closes by itself a number of seconds later while still sitting at the breakpoint. So we seem to be missing an explicit Disconnect call before setting up a new connection, unlike althekiller's patch ? (Compare to the extra Disconnect call here: https://github.com/t-nelson/xbmc/blob/b5...1188-L1192 )

Seems to only happen when the Seek function is called multiple times rapidly. For each physical seek of the video the seek function does get called multiple times and the number of times seems to depend on the video container format.

XBMC 13.2 - Mid 2007 Mac Mini, 4GB, 2TB HD, OS X 10.6.8.
XBMC 13.2 - Raspberry Pi 512MB Model B, 16GB SD, Raspbmc.
(This post was last modified: 2014-04-11 15:13 by DBMandrake.)
find quote
althekiller Offline
Retired Team-Kodi Member
Posts: 5,002
Joined: May 2004
Reputation: 12
Post: #59
DBMandrake, thanks so much for the thorough testing! If more users were willing to put this kind of effort into sorting out bugs, xbmc would be a much better application.

Do you have anything further that you need to test with my patch set? If not please comment on github that we appear to be in good shape. I'd like to kick the last beta soon.
find quote
DBMandrake Offline
Fan
Posts: 453
Joined: Feb 2013
Reputation: 14
Location: UK
Post: #60
(2014-04-11 15:36)althekiller Wrote:  Do you have anything further that you need to test with my patch set? If not please comment on github that we appear to be in good shape. I'd like to kick the last beta soon.
No I think I've tested about all that I can and it seems to work perfectly with connection limits of one, two or more in the test scenarios that I can come up with, so I'll add a comment to the pull request to that effect.

In the meantime I'll continue to run a build that includes the commits from your pull request for my normal XBMC install at home for day to day use so if I notice anything else later I'll post back here.

XBMC 13.2 - Mid 2007 Mac Mini, 4GB, 2TB HD, OS X 10.6.8.
XBMC 13.2 - Raspberry Pi 512MB Model B, 16GB SD, Raspbmc.
(This post was last modified: 2014-04-11 17:07 by DBMandrake.)
find quote
Post Reply