Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More fixes for live tv tsb and seeks #82

Merged
merged 1 commit into from Jan 8, 2019

Conversation

@mlburgett
Copy link
Contributor

commented Jan 6, 2019

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

Also, remove some outdated functions.

More fixes for live tv tsb and seeks
@mlburgett mlburgett force-pushed the mlburgett:master branch from 8302ee0 to 730f089 Jan 6, 2019
Copy link

left a comment

Because of the incremental nature of the slip file, the actual buffer start time is something like

min (0, now() % (g_timeShiftBufferSeconds/3) * g_timeShiftBufferSeconds /3- g_timeShiftBufferSeconds)

Basing things on the g_timeShiftBufferSeconds could cause unexpected blocks to be returned.

@mlburgett

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2019

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

This comment has been minimized.

Copy link

commented Jan 7, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jan 7, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jan 7, 2019

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

This comment has been minimized.

Copy link

commented Jan 7, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Jan 7, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2019

That sounds like it could be helpful.

@emveepee

This comment has been minimized.

Copy link

commented Jan 7, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2019

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.

@sub3 sub3 merged commit 4d6df2f into kodi-pvr:master Jan 8, 2019
2 checks passed
2 checks passed
continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mlburgett

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

Thanks for the merge, sub3!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.