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: #31
This is what I came up with, http://nopaste.linux-dev.org/?147680. A bit more extensive, and in the blind. I've moved on to refactoring the function entirely. It's gross.
find quote
althekiller Offline
Retired Team-Kodi Member
Posts: 5,002
Joined: May 2004
Reputation: 12
Post: #32
@DBMandrake https://github.com/t-nelson/xbmc/commits...o_in_multi

Give that a try, it's all in the blind, so I may have made a mess. Is there a plugin that presents this behavior I could be testing with? Your logs seem to be testing on a local httpd.
find quote
DBMandrake Offline
Fan
Posts: 412
Joined: Feb 2013
Reputation: 12
Location: UK
Post: #33
(2014-04-09 18:01)althekiller Wrote:  @DBMandrake https://github.com/t-nelson/xbmc/commits...o_in_multi

Give that a try, it's all in the blind, so I may have made a mess. Is there a plugin that presents this behavior I could be testing with? Your logs seem to be testing on a local httpd.

Looks good just glancing at the code. My only concern is whether the recursion could cause any problems. (Is the function re-entrant safe ? Would the recursion cause an additional unwanted simultaneous connection to be made ? etc) A bit of single stepping should answer those questions. I'll do some testing tomorrow.

Some of the addons that are affected can't be named here but I've been told the PBS addon and others are affected too. I can't use the PBS addon in the UK, which is why I set up an apache server on Linux to do my testing across a LAN - it allows me to control the number of simultaneous connections and rule out other intermittent variables such as overloaded servers, Internet speed etc. Testing with an addon on over the internet streams is going to introduce too many variables to reach reliable conclusions.

Once tested working in controlled conditions I will of course test with addons too, although I'm having trouble compiling for OS X which is my main platform at home - for some reason a release build of master crashes when I try to play video (or go to the system information screen) even though a debug build works OK...not sure why. Also seem to be having a lot more trouble building the 32 bit version than 64 bit so will try that this time. (Recompiling all the depends takes forever on my old Mac mini so its slow going...)
(This post was last modified: 2014-04-09 21:42 by DBMandrake.)
find quote
althekiller Offline
Retired Team-Kodi Member
Posts: 5,002
Joined: May 2004
Reputation: 12
Post: #34
On first failure we disable multisession before recursing, so we won't do it more than once. The recursive call will either succeed or disable seeking altogether.

I just randomly tried seeking around in a bunch of different stuff from the PBS addon with master and was unable to get the log line mentioned in the OP. With or without the patch the seek experience was pretty crap. I don't normally stream content though, so maybe this is normal.

I don't have any apple HW available, so can't help you there. I wouldn't bother with release or multiple arch stuff. Just find some config you can build clean master from then grab my branch and see what happens.
find quote
ulion Offline
Team-Kodi Member
Posts: 84
Joined: Dec 2012
Reputation: 0
Post: #35
great to see more peoples can talk about this part of code, it's a little confused, but please do not break things when fixing a problem.

if certain http server does not support multi-session, you can make a PR to introduce some url param (indeed protocol param it is called in xbmc) to let xbmc know that, then it will set the m_multisession to false. you do use an add-on transfer such url to xbmc, don't you?

I didn't realize it will use 3 http sessions, it suppose to use 2 http sessions for the double-buffer function works. why it use 3?

and when the 2nd session failed, it will mark the connection non-multisession.
find quote
althekiller Offline
Retired Team-Kodi Member
Posts: 5,002
Joined: May 2004
Reputation: 12
Post: #36
You can have oldstate, m_oldstate and m_state all in play at once. That's three by my count. What are you implying we're going to break here? I honestly can't repro the original problem. I'm just coding in the blind and hoping for the best. This is one of the last issues to be sorted before moving to RC for Gotham.
find quote
ulion Offline
Team-Kodi Member
Posts: 84
Joined: Dec 2012
Reputation: 0
Post: #37
just realized that I have an enhanced version of this part of code, while it is not in current Gotham repo, let me find where it is...
find quote
ulion Offline
Team-Kodi Member
Posts: 84
Joined: Dec 2012
Reputation: 0
Post: #38
here it is: https://github.com/xbmc/xbmc/pull/4543
(This post was last modified: 2014-04-10 06:54 by ulion.)
find quote
Kib Offline
Team-Kodi Server Dude
Posts: 3,475
Joined: Jan 2010
Reputation: 54
Location: NL
Post: #39
@Althekiller, do you need a webserver with those restrictions somewhere to test with?

Neon skin - v3.0.8 now available for XBMC 13.x (Gotham)
find quote
DBMandrake Offline
Fan
Posts: 412
Joined: Feb 2013
Reputation: 12
Location: UK
Post: #40
(2014-04-09 23:02)althekiller Wrote:  I just randomly tried seeking around in a bunch of different stuff from the PBS addon with master and was unable to get the log line mentioned in the OP. With or without the patch the seek experience was pretty crap. I don't normally stream content though, so maybe this is normal.
It depends on how much bandwidth is available for the stream to pick up quickly from the seek point but I generally saw about a 1-2 second delay on seeking a stream on Frodo before it was playing again. I haven't tried the PBS add-on so can't comment on its performance.
Quote:I don't have any apple HW available, so can't help you there. I wouldn't bother with release or multiple arch stuff. Just find some config you can build clean master from then grab my branch and see what happens.
I built master for 64 bit and it runs without crashing, so something is broken with 32 bit builds on Mac OS - at least with XCode 3.2.6 / Snow Leopard. I'll try your patch on windows today and Mac OS tonight.

(2014-04-10 05:28)althekiller Wrote:  You can have oldstate, m_oldstate and m_state all in play at once. That's three by my count.
Yes it was definitely using up to 3 at a time originally, your initial changes reduced that to two.
Quote:What are you implying we're going to break here? I honestly can't repro the original problem. I'm just coding in the blind and hoping for the best. This is one of the last issues to be sorted before moving to RC for Gotham.
Really glad to hear that a fix for this should go into Gotham before release, that's why I've been working hard to try to pin it down before the release gets too close...

What is your build environment, windows or linux ? If its windows do you have any other box even a Raspberry Pi ? I'm actually running apache on my Raspberry Pi as the test server as I don't have any other Linux box, and I was unable to reproduce the problem properly with apache on windows - apache on windows doesn't use the same forking model as apache on unix so limiting simultaneous connections doesn't seem to work properly. I also tried IIS on windows and although that can limit simultaneous connections XBMC can't parse IIS directory listings so attempting to connect to an http directory hosted by IIS shows no files.

Part of what I'm doing is not just limiting connections - I'm watching the network sockets with netstat on the server as well, that's how I'm verifying how many connections are active at each breakpoint etc...also caching thumbnails has to be turned off in settings to test like this otherwise extra connections will be kept open to generate thumbnails, confusing the issue.
(This post was last modified: 2014-04-10 10:25 by DBMandrake.)
find quote
DBMandrake Offline
Fan
Posts: 412
Joined: Feb 2013
Reputation: 12
Location: UK
Post: #41
(2014-04-10 05:17)ulion Wrote:  great to see more peoples can talk about this part of code, it's a little confused, but please do not break things when fixing a problem.

if certain http server does not support multi-session, you can make a PR to introduce some url param (indeed protocol param it is called in xbmc) to let xbmc know that, then it will set the m_multisession to false. you do use an add-on transfer such url to xbmc, don't you?
The problem exists with any method which requests an http/https stream, not just add-ons. That's why I'm testing/debugging without an add-on to rule out any problems with add-ons. The issue is a fundamental problem with the handling of http streams.

A URL param doesn't solve the issue because in most cases the add-on would not know what the server limits are beforehand - remember in the majority of cases add-on authors do not run the servers their add-ons connect to or have any direct affiliation with them, even for things like iPlayer. Most add-ons are unofficial and made by 3rd parties, often individuals.

The proper fix is for the code to be more robust about detecting a simultaneous connection limit and gracefully falling back to a single session.

(2014-04-10 05:49)ulion Wrote:  just realized that I have an enhanced version of this part of code, while it is not in current Gotham repo, let me find where it is...

(2014-04-10 06:08)ulion Wrote:  here it is: https://github.com/xbmc/xbmc/pull/4543

Argh, things are moving quickly, I haven't even tested althekiller's fix yet and there is another one to try. Smile

I'm going to pull althekiller's fix and your fix into a couple of different test branches on top of master and do some testing today. I will report back what I find.
(This post was last modified: 2014-04-10 10:28 by DBMandrake.)
find quote
DBMandrake Offline
Fan
Posts: 412
Joined: Feb 2013
Reputation: 12
Location: UK
Post: #42
(2014-04-09 18:01)althekiller Wrote:  @DBMandrake https://github.com/t-nelson/xbmc/commits...o_in_multi

Give that a try, it's all in the blind, so I may have made a mess. Is there a plugin that presents this behavior I could be testing with? Your logs seem to be testing on a local httpd.
I used git cherry-pick to take your three commits and apply them to a new branch on top of master.

I've done some testing on this and I'm happy that the first two commits work and are clean enough to be obviously correct but the third commit makes me really nervous.

First commit:

https://github.com/t-nelson/xbmc/commit/...7e3a16bb29

Already tested earlier - this reduces the maximum simultaneous connection attempts from three to two preventing problems with two simultaneous connections, but still has problems with only one simultaneous connection.

Second commit: (applied on top of the first)

https://github.com/t-nelson/xbmc/commit/...1872c25d65

This also works perfectly as expected - with only one simultaneous connection allowed, on failure of a second connection attempt m_multisession is set to false and the recursive call to Seek works to automatically retry the connection without the calling function knowing anything went wrong. (So should be perfectly seamless) The commit is also simple and obvious enough that you can tell by looking at it that it's correct.

Third commit: (on top of the other two)

https://github.com/t-nelson/xbmc/commit/...1a749d2336

It's hard to follow the changes made here and what they're trying to do - were you only wanting to clean up the code but keep the functionality the same ? Because I've immediately noticed that this code behaves quite differently.

More specifically with commit 1 & 2 the connection flow (with two or more connections allowed) is as follows:

On initial startup connection 1 is opened. On first seek connection 2 is opened and starts streaming, connection 1 is left open. (and idle ?)

On second seek connection 1 is closed, connection 3 is opened and starts streaming, connection 2 is left open. (and idle ?)

On third seek connection 2 is closed, connection 4 is opened and connection 3 is left open and idle.

So the two connections "roll over" with the oldest connection of the pair being closed before a new connection is opened. Am I right in thinking this is the correct behaviour to allow the double cache to work properly ?

----

With commit three applied the connection sequence is:

On initial startup connection 1 is opened. On first seek connection 2 is opened and starts streaming, connection 1 is left open. (and idle ?)

On second seek connection 2 is closed, connection 3 is opened and starts streaming, connection 1 is left open. (and idle ?)

On third seek connection 3 is closed and connection 4 is opened, connection 1 is left open.

So the two connections no longer roll over - the initial connection stays open permanently and each seek causes the active connection to immediately close, thus double caching wouldn't work properly ?

So my (non coders) opinion is that commits 1&2 are no brainers and solve the problem completely but commit 3 either needs reworking or leaving out due to risk of additional unintended side effects...

Now on to test ulion's patch.... Smile
(This post was last modified: 2014-04-10 13:11 by DBMandrake.)
find quote
DBMandrake Offline
Fan
Posts: 412
Joined: Feb 2013
Reputation: 12
Location: UK
Post: #43
(2014-04-10 06:08)ulion Wrote:  here it is: https://github.com/xbmc/xbmc/pull/4543
ulion,

I can't seem to compile your pull request. I started with an up to date master branch and did a git cherry-pick to pull in those two specific commits in order (same thing I did successfully with t-nelsons commits) but when I compile I get:

Code:
Error    4    error C2065: 'url' : undeclared identifier    C:\Github\xbmc\xbmc\filesystem\CurlFile.cpp    1227    1    XBMC
Error    5    error C2228: left of '.GetProtocol' must have class/struct/union    C:\Github\xbmc\xbmc\filesystem\CurlFile.cpp    1227    1    XBMC
Error    6    error C2660: 'XFILE::CCurlFile::SetCommonOptions' : function does not take 2 arguments    C:\Github\xbmc\xbmc\filesystem\CurlFile.cpp    1227    1    XBMC
    7    IntelliSense: identifier "url" is undefined    c:\github\xbmc\xbmc\filesystem\curlfile.cpp    1227    31    XBMC

So it seems your changes rely on other changes that are only in your branch ? Or is there a better way for me to test this - do you have a branch for your seek fix that I could pull the entire branch of ? (Although that doesn't solve the issue of merging it into master properly I suppose - it merges without errors but won't compile)

Still finding my way around git so I may have done something wrong. Edit: I just fetched this entire branch and had the same compile error:

https://github.com/ulion/xbmc/tree/fix_c...ure_master

So I can't test your code until it can compile or you let me know what I'm doing wrong.
(This post was last modified: 2014-04-10 12:59 by DBMandrake.)
find quote
wsnipex Offline
Team-Kodi Member
Posts: 3,750
Joined: Jun 2011
Reputation: 92
Post: #44
yes, that looks broken. url gets defined on line 1179, but only if (multisession || m_oldstate)
CURL url(m_url); should probably be moved up or also be defined on line 1226
find quote
ulion Offline
Team-Kodi Member
Posts: 84
Joined: Dec 2012
Reputation: 0
Post: #45
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.
(This post was last modified: 2014-04-10 15:24 by ulion.)
find quote
Post Reply