Implement filecache retry logic

[arnova]

This PR implements retry logic for our filecache. Especially poor (wifi) connections benefit from this. Previously we would simply (silently) abort in case a single Read() failed.

With this we simply retry a read, as long as we have data in our cache. This PR also adjusts Curl's fillbuffer retry logic to properly support Read() retries. This also disables CFileCurl's internal retry logic which could cause long waits (hangs) on exit when .e.g a connection timeout occurred.

[arnova]

jenkins build this please

[arnova]

@Paxxi : Please review/comment

[arnova]

ping @Paxxi. Anyone else against this?

[fritsch]

I don't understand all those FileCache hacks and there are many in that
code. We need to take great care that nothing will hang undefined for 5
seconds - that would be a really bad user experience and if this happens on
every end of a movie or something then we have a big problem.

So @Paxxi please review.

2016-01-09 9:38 GMT+01:00 arnova notifications@github.com:

ping @Paxxi https://github.com/paxxi. Anyone else against this?

—
Reply to this email directly or view it on GitHub
#8723 (comment).

               Key-ID:     0x1A995A9B
               keyserver: pgp.mit.edu

Fingerprint: 4606 DA19 EC2E 9A0B 0157 C81B DA07 CF63 1A99 5A9B

[arnova]

That's why we need to
  properly test this during alpha, so we can iron out any issues.
  The way it is now is really bad for poor (wifi) connections.
  Endusers don't understand why their video aborts while there's
  still plenty of data in the filecache....

On 09/01/16 09:45, Peter Frühberger
  wrote:

I don't understand all those FileCache hacks and there
  are many in that
  code. We need to take great care that nothing will hang undefined
  for 5
  seconds - that would be a really bad user experience and if this
  happens on
  every end of a movie or something then we have a big problem.

  So @paxxi please review.

  2016-01-09 9:38 GMT+01:00 arnova <notifications@github.com>:

  > ping @paxxi <https://github.com/paxxi>. Anyone else
  against this?
  >
  > —
  > Reply to this email directly or view it on GitHub
  >
  <https://github.com/xbmc/xbmc/pull/8723#issuecomment-170210180>.
  >



  -- 
  Key-ID: 0x1A995A9B
  keyserver: pgp.mit.edu
  ==============================================================
  Fingerprint: 4606 DA19 EC2E 9A0B 0157 C81B DA07 CF63 1A99 5A9B
  —
    Reply to this email directly or view
      it on GitHub.

[Paxxi]

Sorry about the radio silence, too many notifications :)
No objections to merging as is, could use an int64_t instead of int to be more consistent with other file access methods but I don't think we're going to need a cache large enough to warrant it.

[arnova]

@Paxxi : No problem and thanks for the review. I think I'll stick to int since that's what the surrounding functions in that area use as well. I'll squash it and I think it's ready to go in then.

[arnova]

@Paxxi: Mind reviewing it again. I simplified the logic somewhat. Thanks!

jenkins build this please

[arnova]

@MilhouseVH : Could you include this in your builds so it gets some proper testing before it's merged into mainline?

[MilhouseVH]

Will do, thanks.

[arnova]

@MilhouseVH : No, thank you ;-)

[MilhouseVH]

This PR is causing a range request failure when attempting to play content from Amazon Prime - see this debug log: http://pastebin.com/raw/EaW1W3np

With this PR, the request to Amazon will fail with:

16:16:46 39677.078125 T:1489114016   DEBUG: Curl::Debug - TEXT: HTTP server doesn't seem to support byte ranges. Cannot resume.
16:16:46 39677.078125 T:1489114016   DEBUG: Curl::Debug - TEXT: Closing connection 0
16:16:46 39677.078125 T:1489114016   ERROR: CCurlFile::FillBuffer - Failed: Requested range was not delivered by the server(33)
16:16:46 39677.078125 T:1489114016   ERROR: AddOnLog: Input Stream: Could not open / parse mpdURL (http://s3.lvlt.dash.us.aiv-cdn.net/d/1$AOAGZA014O5RE,42D4116A/videoquality$1080p/prod/4be9/73d6/2189/47ce-952c-ce46eddf5830/442612cc-c80f-4b78-9953-5f0fcd2b268f_corrected.mpd)

To reproduce you'll need:

• latest Kodi master
• this PR
• inputstream.mpd addon with libwidevinecmd.so (this is @fritsch's build: https://dl.dropboxusercontent.com/u/55728161/inputstream.mpd-1.0.0.zip)
• Amazon Prime Instant add-on, or a derivative
• and of course an Amazon Prime account (month long test account should work)

If you live in the UK you may also need Getflix to access the US as the UK isn't working well at all (probably unrelated to this PR, likely an add-on issue).

I'm in the UK, but get the above curl error when accessing US or DE content (with or without Getflix) while PR8723 is included, Without PR8723, content from the US via Getflix will play normally.

[ghost]

I came over this issue today also with inputstream with @MilhouseVH openelec:

• uncompressed filesize about 5.2mb, gzipped 832k
• after reading the file with Accept-Encoding: gzip once a retry takes place at file position 5.4mb curl range value.
  this is first behind the extracted filesize (???) and secondly the server does not support byte ranges.

My guess was that it is curl, but the user has installed 7.47.1 (the newest version), but good to know that it seems to be something inside kodi - I'm not keen on fixing curl code.

B:t.w: the behaviour was a little bit strange: Download was working once, after that not anymore.
After a Kodi restart it was runnig again once. Then it was not running at all anymore.
Seems to be an uninitialized member - I'm sure valgrind should find it.

[arnova]

Strange I've reviewed the code once again and I can't seem to find which change is responsible for this. @MilhouseVH : You're 100% sure it's this PR causing it and not the e.g. libcurl update?

[MilhouseVH]

@arnova: Absolutely 100% sure, yes.

[MilhouseVH]

To be clear, I stopped adding this PR and the problem went away - Amazon Prime now worked.

In addition, I built master for Ubuntu x86_64 without any other changes and Amazon Prime worked. I then rebuilt with this PR and could then reproduce the range request failure.

[ghost]

just a question: has anything of this development found its way to master?
Note: I haven't pulled this PR!!

Today I have this (and never saw this before):

15:39:17 T:160 DEBUG: Curl::Debug - SSL_DATA_OUT: ����V
15:39:17 T:160 DEBUG: Curl::Debug - HEADER_OUT: POST /WidevineLicenser/WidevineLicenser HTTP/1.1
15:39:17 T:160 DEBUG: Curl::Debug - HEADER_OUT: Host: wvguard.sky.de
15:39:17 T:160 DEBUG: Curl::Debug - HEADER_OUT: Content-Range: bytes 0-/-1
15:39:17 T:160 DEBUG: Curl::Debug - HEADER_OUT: User-Agent: Kodi/17.0-ALPHA1 (Windows NT 10.0; WOW64) App_Bitness/32 Version/17.0-ALPHA1-Git:Unknown
15:39:17 T:160 DEBUG: Curl::Debug - HEADER_OUT: Accept: /
15:39:17 T:160 DEBUG: Curl::Debug - HEADER_OUT: Accept-Charset: UTF-8,*;q=0.8
15:39:17 T:160 DEBUG: Curl::Debug - HEADER_OUT: Content-Length: 2
15:39:17 T:160 DEBUG: Curl::Debug - HEADER_OUT: Content-Type: application/x-www-form-urlencoded
15:39:17 T:160 DEBUG: Curl::Debug - TEXT: upload completely sent off: 2 out of 2 bytes
15:39:17 T:160 DEBUG: Curl::Debug - TEXT: SSLv2, Unknown (23):

where dose the content-range come from? I didn't set it. I stoill have the feeling that there is something uninitialized.

[arnova]

@MilhouseVH : Sure, but in principle if no other problems popup this should be merged in a few weeks. Please ping me in case you want to drop it.

@fritsch : I'm running (XFCE) Mint 17.2, which is based on Ubuntu 14.04-LTS. I though we'd always support the last LTS version of Ubuntu? When did we drop support for it?

@FernetMenta : Thanks for the info.

@mapfau : Nothing has been merged to master yet concerning this or anything else concerning Curl. I'll have a look at your issue.

[fritsch]

@arnova Ubuntu 16.04 LTS is released in 1 months and kodi v17 most likely after it.Kodi v16 supports 14.04 just fine - but sucks VAAPI feature wise.

[ghost]

I'll, because its reproducable on my machine

[arnova]

@mapfau : Looking over the code, I suspect a problem in libcurl itself. I don't see anything (obvious) in our code that could cause this.

[ghost]

@arnova sorry - I passed the headers too late, it was my fault :-(

[arnova]

@mapfau : No problem :)

[arnova]

jenkins build this please

[arnova]

@MilhouseVH : No reports of problems with this? I just added an IO control option to properly handle enabling/disabling retrying inside file implementations when cache is used. The changes are pretty simple/straightforward. Please review, but I don't think they require any additional testing.

[MilhouseVH]

No further reports of any problems after your fix for Amazon. I've tested your latest changes and Amazon is still working.

[arnova]

jenkins build this please

[RubenTeixeira]

I believe this PR broke video queue on android. Behaviour is as follows:
Stream starts, video queue gets to 100% and forward buffer starts filling up, immediatly after, the video queue starts to drop until near 0%, down to a point when it starts filling up again. Then this behaviour will repeat (down,up,down,up).
I'm currently overwelmed with work so i cant post logs right now, but if no one else does in the meanwhile, tomorrow or the day after i will provide some.

[ghost]

@arnova
not sure if it was this or one of the other commits in the last nightly kodi-20160412-6167026-master-armeabi-v7a.apk on android tv when starting a video file (smb library share) the audio kicks in and the screen stays black for around 5 seconds then it shows up video. reverting to kodi-20160410-25dc4bf-master-armeabi-v7a.apk fixes it.

[fritsch]

trac.kodi.tv all debuglog are mandatory

[ghost]

the issue i described is now resolved with the latest nightly.

[RubenTeixeira]

Same! Fixed with latest build.

[arnova]

@RubenTeixeira : Great that it's fixed but please next time don't start blaming (random) commits/PRs unless fairly sure it's the cause. And if not, use the forum instead.

[RubenTeixeira]

@arnova i'm sorry, but from the previous build to the troubling one, the only commit related to buffering was this one.