kodi-pvr / pvr.nextpvr

Kodi's NextPVR client addon
GNU General Public License v2.0
22 stars 33 forks source link

More fixes for live tv tsb and seeks #82

Closed mlburgett closed 5 years ago

mlburgett commented 5 years ago

Rework time/buffer housekeeping so we know (approximately where the start/end of the tsb is.

Also, remove some outdated functions.

mlburgett commented 5 years ago

Not sure what you're referring to in the code, but on the client side of the socket, the slip file(s) are presented as a single virtual file. All the issues about which bits are in which slip file are hidden from the client side. @sub3, do you see an issue with how g_timeShiftBufferSeconds is used?

emveepee commented 5 years ago

Not sure what you're referring to in the code, but on the client side of the socket, the slip file(s) are presented as a single virtual file. All the issues about which bits are in which slip file are hidden from the client side. @sub3, do you see an issue with how g_timeShiftBufferSeconds is used?

I think sub starts streaming for the top of the first available not from the g_timeShiftBufferSeconds point when those arg! lines are shown in web.log I posted on the forum

2018-12-30 10:30:48.079 [DEBUG][26] Got request: Range: bytes=29097984-29130752-924
2018-12-30 10:30:48.079 [DEBUG][26] RollingFile.Seek(29097984) 2018-12-30 10:30:48.079 [DEBUG][26] arg! our live buffer doesn't go back this far.... 2018-12-30 10:30:48.079 [DEBUG][26] Physical file length: 2109360 2018-12-30 10:30:48.079 [DEBUG][26]

262330500 C:\Temp\live-TV Guide-3740-57-10.ts C:\Temp\live-TV Guide-3740-57-9.ts C:\Temp\live-TV Guide-3740-57-8.ts C:\Temp\live-TV Guide-3740-57-7.ts 372515 0

2018-12-30 10:30:48.079 [DEBUG][26] about to read 32768 from location 11564 (current length = 2109360) 2018-12-30 10:30:48.079 [DEBUG][26] Physical file length: 2109360

location = shown is the next 32768 boundary. Each file is 1/3 of the time, but the head doesn't get removed until the tail can get 1200 seconds.

Martin

sub3 commented 5 years ago

I wont pretend to have a full understanding of how the everything works in these newer versions of the NextPVR addon. I've been lucky enough to be able to spend my time on other stuff.

From a live tv client perspective though - the default slip buffer size on the server is 20 minutes. In the first 20 minutes of live tv, the size of the available slip data will be the number of seconds that live tv has been running. After 20 minutes, at least 20 minutes of data will be available to the client at all times. At times there is more data available, but this is hidden from the client.

ie, the slip buffer is 1200 seconds (20*60). The server implements this as a rolling set of four files, each containing up to 400 seconds. 3 files of 400 seconds = 1200, and the fourth (most recent file) is always somewhere between 0 and 400 seconds as it grows to completion. When the fourth files hits 400 seconds, a new file is created, and the oldest deleted.

sub3 commented 5 years ago

I guess the issue is really how to translate "seconds" into "seek offset", since there is no super easy way for the client to know precisely what time represents which portion of the virtual file, and it's probably easy for it to guess at a bitrate and could guess for too far back, into an area that the application no longer has.

I'm not 100% what it does in this scenario - as Martin says, it might currently not seek at all, rather than just go back as far as it can.

I don't think there is necessarily anything that can be done in the client to avoid this happening, and it might depend on a change in the backend to seek back as far as it can in this scenario.

emveepee commented 5 years ago

If my logic is wrong and we want to be accurate right now It is possible to get the slip file for epg mode with method=channel.stream.info and the sid. If that could be extended to regular live tv it could work. The offset value is actually better because the addon doesn't implement the Kodi time skipping option

mlburgett commented 5 years ago

@Sub3, that sounds reasonable. There's always some risk of trying to jump back too far, since we don't know exactly where the oldest data available is. I'm using a bytes per second average created over the life of the session, and assuming the back end will always have at least slip seconds worth of data available (once slip seconds has elapsed.) The actual seeking is accomplished by calculating a 32768 byte aligned block to request that (assuming it's within bounds) contains the offset from 0 being seeked to.

Previous version only recently limited skipping forward, and didn't limit skipping back. This version clips seeks to our calculated tsb start + 1 second and the end - 1 second.

sub3 commented 5 years ago

In TimeshiftBuffers.cpp, search for 'parse response header'. I could add another parameter here from the backend, to tell you the earliest offset that can be safely seeked to. You'd have to handle it not being there though, and still do best endeavours at estimating how far back to seek - since some users will continue using older versions.

mlburgett commented 5 years ago

That sounds like it could be helpful.

emveepee commented 5 years ago

I looked over the code and

  1. my major concern is I don't understand the need for TSBTimerProc() in it's own thread. We already have a control function GetStreamTimes() that Kodi polls every second paused or not. Right now some checks are in GetStartTime(); but the "timer" proc could be called in an different way

  2. The code cleanup for removing unnecessary functions is good but it would have been nice to see it in a PR of its own for easy acceptance.

  3. The cleanup PR could also include the virtual PauseStream() function with no change to TimeShift and session files except for the base class needed to support future Pausing.logic changes. Again an easy acceptance

  4. Minor point but In my testing because of time needed to setup a tuner a recording the buffer actually starts here https://github.com/kodi-pvr/pvr.nextpvr/blob/master/src/buffers/TimeshiftBuffer.cpp#L137 There can be can be 2-8 seconds lost in my setup more but IPTV users can lose more. .

mlburgett commented 5 years ago

In my testing GetStreamTimes() was called ~3 times a second. Since we don't really need/want other than per second resolution, this appears to be unnecessary work, so now GetStreamTimes() can just return values already calculated.

sub3 commented 5 years ago

Ok, this is definitely an improvement on what we had previously. There is still further room for improvement, but lets get this latest code out in the hands of other beta testers.

mlburgett commented 5 years ago

Thanks for the merge, sub3!