[pvr] Add more complete seek and speed contro for supporting PVR demuxers

[adamsutton]

Allow the SetSpeed and SeekTime internal demuxer methods to be passed directly to PVR addon's that include their own demuxer and can support these methods.

[opdenkamp]

good point. could you remove the bool from the pvr api version of the call, and let a negative offset do a backwards seek, and positive forward (and update the doxy). that way we don't have to change this on the pvr api later.

[FernetMenta]

and let a negative offset do a backwards seek

I think this is not how it works. Time is actually the time in ms player gets from dvdclock. It does not do a relative seek. ffmpeg demuxer substracs a starttime from the timestamps it gets from the stream. This is currently not done in pvr demuxer.

[FernetMenta]

Sorry, talk was about pvr api call. Since we get pts (PCR) from the backends, I would recommend that we use this absolute value for seeking. A relative seek would require to take the buffered packets into account.

[adamsutton]

@opdenkamp I'm not sure why you would want to do that, nor do I think I can do it.

At the moment I've done nothing with the seek API for several reasons:

1. XBMC blocks access in PVR mode to the seek controls so unless its used internally during FF/RWD I don't see a major benefit at this time.
2. I don't yet fully understand, because of the above, how the API is meant to work.

The only reason I even included it was simply for completeness, it seemed better to at least pass the demuxer API fully through to those addon's that have their own demuxers.

And while I don't really understand the need for a backwards flag (as opposed to a simple signed number) I would think it better that we maintain the XBMC internal demuxer API as is through to the addon's rather than having a special API within the PVR framework.

[opdenkamp]

right, that would make things harder indeed. but then we don't need the boolean param, do we?

[opdenkamp]

heh, cross comment :)

[FernetMenta]

but then we don't need the boolean param, do we?

good question, but I have no answer yet. I am not sure if we need a new seek function anyway on the pvr api at this time. We already have SeekLiveStream and SeekRecordedStream.
I see it like adamsutton, it's hard to tell without a a prototype.

[adamsutton]

@FernetMenta of course those functions are only relevant to a subset of the addons. But I'm happy for SeekTime() to be removed as I currently have no use for it even if XBMC-PVR allowed control of it.

As noted I merely included it for completeness with regard to the demuxer API.

For me its more important that I can simply control the speed of output to allow play/pause FF/RWD. Jumping is a nicety that I can live without for now.

The one thing I've discussed with @opdenkamp before and I think you (@FernetMenta) mentioned in the XBMC forum post is how we deal with a reverse path to allow the addons/demuxers to indicate the true speed. E.g. TVH will output a response message (async) to indicate when the speed is actually updated and more importantly what it has actually been set to (since it may limit it, or reject the request).

This mainly relates to things like going off either end of the buffer and while can be handled internally within XBMC it would be nice if (where supported) XBMC allowed feedback from the demuxer as to what speed has actually been selected. But I think that's a problem for later.

[FernetMenta]

@adamsutton why do you want to use the realtime stream and control its speed? What's the difference between playing a recording and playing out of a "timeshift" buffer? I think it's very similar. Maybe it makes more sense to extend the pvr demuxer by adding functions to ask the backends for packets.

[adamsutton]

@FernetMenta right, so now you want to impose a specific model on the backends, I can see that going down well.

TVH is a streaming server that understands to a certain extent the contents of the stream and will play it back accordingly (up to now 1x only) using its own streaming container. It simply isn't designed to be used like a file, nor do we have any intention of making it behave that way. We don't really want to have to produce a specific interface for XBMC, when the one we have already works just fine.

With regard to recordings v timeshift, at the moment there is a big difference, though it may blur in time. Recordings are done to a linear file intended to be kept long term and in a standard file format. The timeshift buffer is a series of discrete time chunks (designed to be shared for efficient storage) stored in TVH's internal format that is essentially the same thing we currently stream live TV in.

A caveat to the above is that if we find XBMC simply cannot deal with this way of working we may have to re-think our position. But that is a last resort and I would like to avoid it.

[elupus]

For reference on the backward bool. It doesn't mean seek backward, it means seek to the keyframe before specified time, as opposed to after.

//RANT ON
I would really like that we stop extending the API with special case functions. One seek function with a a parameter like SEEK_CUR/SEEK_TIME ... should be more than enough and avoid us having to constantly add new methods. (not that standard HTTP should be able to do seeks based on time so we should support that on a file system level).

Also the split between seek on file and on live tv. is rather silly.. As is the fact that it's globally part of the PVR context without any additional context.
//RANT OFF

[elupus]

Also.. SetSpeed would make more sense as a generic IoControl command....

[adamsutton]

@elupus you guys understand all of this way better than me, the purpose of this PR was simply to allow the PVR demuxer (used by TVH, and VDR?) the same level of control as the other demuxers using the same standard APIs already defined in XBMC.

Special case code has already been added to support the other backends which do not provide a demuxer and therefore require such special cases to handle things.

If there is a better way of doing this then I'm more than happy for someone to point it out and I will be prepared to integrate accordingly.

And as I noted, I'm not going to lose any sleep if you guys reject this and find a better way to do it post Frodo. However not including the support necessary for XBMC to support timeshift for all existing backends (even if its not perfect and needs fixing later) would be a real shame!

[elupus]

Well the way the API is now, i'm not at all opposed to adding these functions. With the following alterations.

• make sure void CPVRClient::SetSpeed(int speed) / SeekTime check the m_pStruct pointer so it's non null. That way we don't crash badly if an API don't implement it.
• Correct backward param doxygen to my description above.
• Correct time doxygen to say "absolute time since stream start", since that is what it is now.

[adamsutton]

@elupus doxy comments updated, you ok with those?

I don't quite follow your point about checking m_pStruct, no other code checks that pointer its pretty much assumed to be correct (not sure it can be NULL). If you're talking about check m_pStruct->SetSpeed, then again this doesn't appear to be the way the PVR addon code is implemented.

it's assumed (enforced?) that all addons must implement all methods, even if they simply stub them out. This is certainly what I've done in the corresponding PR on @opdenkamp's addon repo.

[elupus]

If they stub them it work. But not all other addons have stubs for these
new methods. They are optional so to speak, we should not require stubs.

[adamsutton]

@elupus I'm not really arguing that point per se, its not a bad suggestion.

However if you want to put this requirement on new code, fine, but you really should put the requirement on ALL code, which means significant updates to the PVR code which currently implies all functions are properly stubbed out.

[elupus]

Only new code at the moment to avoid breaking all current addons, but I
fully agree in principle. @opdenkamp will the loader accept missing
dlsym's?

[opdenkamp]

these are set by passing a struct to the add-on, and the add-on then sets the function pointers in that struct (calls get_addon() in xbmc_pvr_dll.h). the struct is memset to 0 before, so if you add the new methods to the end of the struct and check whether it's actually set before using it, then the old add-ons should be happy too without any change.

[adamsutton]

@opdenkamp @elupus I don't think that the way the linkage has been done I can test what you're asking for, I think the code is forcing (part of the loader?) that these values be non-NULL.

Certainly I cannot check that m_pStruct->SeekTime != NULL.

Am I missing something obvious?

[opdenkamp]

Certainly I cannot check that m_pStruct->SeekTime != NULL.

as far as I can see now (but haven't checked thoroughly or tested anything), that's exactly what you need to do (except NULL != 0) and that will work as xbmc memsets the struct with pointers to 0, add-on then sets pointers in that struct, and adding a new entry to the end of the struct won't bother the add-on, as it won't touch that new part.

[adamsutton]

@opdenkamp try it ;) it doesn't work, the way the func ptr linkage is setup the compiler whinges.

[opdenkamp]

right, will have to dig into that as soon as i got time

[ghost]

signoff

[opdenkamp]

merged: 0feee86