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

[pvr] add IsRealTimeStream to api #8896

Merged
merged 1 commit into from Jan 19, 2016
Merged

[pvr] add IsRealTimeStream to api #8896

merged 1 commit into from Jan 19, 2016

Conversation

FernetMenta
Copy link
Contributor

player needs to know if the stream is real-time. currently addons without a demuxer show issues.

@ksooo
Copy link
Member

ksooo commented Jan 17, 2016

You need to bump addons/xbmc.pvr/addon.xml as well.

I assume the actual use case for new CPVRClient::IsRealTimeStream will be added in a follow-up PR?

Will you provide the implementation for the addons as well?

@FernetMenta
Copy link
Contributor Author

You need to bump addons/xbmc.pvr/addon.xml as well.

thanks, will do

I assume the actual use case for new CPVRClient::IsRealTimeStream will be added in a follow-up PR?

yes

Will you provide the implementation for the addons as well?

nope, that's the job of the addon authors. it's not just adding a dummy function. If this does not get implemented correctly, the addon can stay broken as well. a broken addon motivates more for doing a change than a bad working one.

@ksooo
Copy link
Member

ksooo commented Jan 17, 2016

Will you provide the implementation for the addons as well?

nope, that's the job of the addon authors. it's not just adding a dummy function. If this does not get implemented correctly, the addon can stay broken as well. a broken addon motivates more for doing a change than a bad working one.

Don't know whether this approach will work out. We do not have authors (active maintainers) for all addons.

Will you post the change and how to implement it in the PVR addon dev subforum? Otherwise the active maintainers will be lost, I'm afraid.

@FernetMenta
Copy link
Contributor Author

I already posted the api change in the dev section of the forum.

Don't know whether this approach will work out. We do not have authors (active maintainers) for all addons.

Bad times for the addon then. Without an active maintainer a feature dies sooner or later. As mentioned, this is not only adding a dummy. This has big impact on behaviour. I guess you don't want to dig into the details of all addons, try to understand how it works, set up a test environment, and do the implementation?

@ksooo
Copy link
Member

ksooo commented Jan 17, 2016

I already posted the api change in the dev section of the forum.

Cool. Thanks.

Without an active maintainer a feature dies sooner or later.

Just wanted to have raised my hand in beforehand. Imo more than 50% of our addons will die, then. Not that I really care, but...

I guess you don't want to dig into the details of all addons, try to understand how it works, set up a test environment, and do the implementation?

Of course not. :-/

@MartijnKaijser
Copy link
Member

@ksooo same happens with skins ;) with the difference that we are still forced to build the pvr addons ourselves and ship them. I'm already nudging the python add-on devs that similar thing might happen and we will bump ABI in near future.
Sure it sucks but it might get some more attention from people who want to keep using it.

@zag2me
Copy link
Contributor

zag2me commented Jan 18, 2016

I guess this is why TVheadend is having problems with millhouses builds. I'm happy to test as soon as that add-on gets updated.

@FernetMenta
Copy link
Contributor Author

I guess this is why TVheadend is having problems with millhouses builds

nope, see first message:

currently addons without a demuxer show issues.

@FernetMenta
Copy link
Contributor Author

objections if I press the button? addons are branched and can go for the adjustments once this is in.

@ksooo
Copy link
Member

ksooo commented Jan 19, 2016

Go ahead. :-)

@FernetMenta
Copy link
Contributor Author

jenkins build this please

FernetMenta added a commit that referenced this pull request Jan 19, 2016
[pvr] add IsRealTimeStream to api
@FernetMenta FernetMenta merged commit 4bfeb18 into xbmc:master Jan 19, 2016
@FernetMenta FernetMenta deleted the pvr branch January 19, 2016 19:17
@hudokkow hudokkow added this to the Krypton 17.0-alpha1 milestone Jan 19, 2016
@metaron-uk
Copy link
Contributor

Probably this is related to the business of seeking in timeshift recordings / buffers not being able to be filled in VideoPlayer (I was involved with some discussions on this last year). It would be good to get a proper solution for this.

I understand VNSI will set this to 'true' when a timeshifted recording is playing within 10 seconds of 'now', or the stream is 'live'.

I've searched the forum for IsRealTimeStream but can't find the post @FernetMenta mentioned above. Could someone post a link here?

@FernetMenta
Copy link
Contributor Author

It would be good to get a proper solution for this.

no sure what you are trying to say but I consider this as a proper solution. the old code had some advanced settings like "minaudiocachelevel" and "minvideocachelevel" that have disappeared. A real-time stream need special treatment because once playback has started, buffer (caches) won't fill. Stream have to identify if they are real-time or not to be handled properly.

@metaron-uk
Copy link
Contributor

It wasn't intended as a comment on the videoplayer code. There was a check added in Jarvis (Isengard?) at some point which identified a realtime thread by checking to see if the recording currently being played was associated to a currently showing EPG entry. This was inefficient in the way it worked and killed slow frontends (like RPi) that's what I meant about being good to get 'a proper solution'.

@FernetMenta, I'm genuinely interested to find out what you're working on (is it just better managing video player caches, or does it also have something to do with improving skip forward in timeshift recordings). I'd like to find out, but I couldn't find the forum post you referred to.

@FernetMenta
Copy link
Contributor Author

I announced the api change in the forum http://forum.kodi.tv/showthread.php?tid=97764&pid=2217621#pid2217621

@ryangribble
Copy link
Contributor

Hi guys,

I missed this API change till now, just trying to understand how we would properly implement this call in pvr.wmc.

There is no built in demuxer, we pass off a smb:// path to a "live stream" file to Kodi File.Open() function, so as far as I know we don't know whether the user has paused/rewind the live stream or whether it is real time.

How should non demuxer/file based addons implement this?
Same goes for the recent time shift API calls.

In our case isn't it Kodi video player itself that knows what position it is up to in the "live stream" ts file? How could we get at that information from the addon side? Or should there be a way we can tell Kodi that instead of calling this (and the time shift) calls on the addon, it should use its own file player data?

@FernetMenta
Copy link
Contributor Author

How should non demuxer/file based addons implement this?

I have implemented this in particular for those addons not having its own demuxer

we pass off a smb:// path to a "live stream

that sounds like the "otherStreamHack".
https://github.com/xbmc/xbmc/blob/master/xbmc/cores/VideoPlayer/DVDInputStreams/DVDInputStreamPVRManager.cpp#L116
files accessed via this protocol are in general files and we don't treat them as real-time. Kodi won't know unless to signal this info by this new method.
I call this method a hack because it never was implemented properly. Hence you can't expect full functionality as the other methods provide.
Addons that use pvr:// protocol handle files access via Kodi's virtual file system and have control over this.

@ryangribble
Copy link
Contributor

So should we return false on this new call then?

Regarding this "other stream hack" and implementing things properly, I'm not sure where to start, it pre-dates my involvement with this addon and Kodi. If pvr addons shouldn't be passing a file path then maybe the plan should be to deprecate that function.

To me it seems like if Kodi is playing the file it should be able to tell itself about the current position, file start position etc and thus fill in these details on whether live stream or not etc, is that what you mean by "it was never implemented properly"?

So say if we want to make our addon not just hand off the file path two Kodi, what would we need to do? Just send bytes of data to Kodi when it asks for it? Any example addon than works like this (read a file on the backend but provide data to Kodi directly rather than pointing Kodi at the file)

@FernetMenta
Copy link
Contributor Author

There's tree types of PVR addons and they promote their capabilities in the addon capabilities struct.

  • bHandlesDemuxing
    Those have the best control
  • bHandlesInputStream
    still have some control but demuxin is delegated to ffmpeg
  • neither bHandlesDemuxing nor bHandlesInputStream -> other stream hack

The third category was introduced before my time and I agree that it should be depreciated. Even the important and vital functionality required for PVR was delegated to Kodi's generic functions.

I looked into the wmc code and was surprised that it states bHandlesInputStream. Since it handles reading the files itself, you should have no issues in implementing IsRealTimeStream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants