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] Document implict call to CloseXXXStream() before OpenXXXStream() #15479

Merged
merged 1 commit into from Feb 17, 2019

Conversation

Projects
None yet
4 participants
@djp952
Copy link
Contributor

commented Feb 10, 2019

Description

The PVR API is implicitly calling CloseLiveStream() / CloseRecordedStream() before calling OpenLiveStream() / OpenRecordedStream(). I believe these implicit actions should not take place on behalf of the PVR Client addon.

Motivation and Context

This is a problem if the PVR client is forced to open a stream prior to the actual call to OpenLiveStream() / OpenRecordedStream(). My specific example is that in order to properly support GetChannelStreamProperties() and GetRecordingStreamProperties(), I have to open the stream. The backend I am working with (HDHomeRun DVR) does not expose the proper MIME type via metadata, and in the case of Recordings, I do not know if the stream will be real-time or not until it's been opened and I have examined the HTTP response headers. (HDHomeRun DVR reports Recordings currently in-progress as real-time, for example).

Moving the point where GetXXXStreamProperties() is called so that it occurs after OpenXXXStream is prohibitive and unnecessary. PVR addons can provide Kodi with the proper properties if they are unknown by opening the stream.

Which leads to why this change: Things fall apart because the stream that was opened in GetXXXStreamProperties() is implicitly closed by PVRClient.cpp, and is then opened again. This is wasteful at best, but at worst the properties may be different on the subsequent open (this is unlikely, but possible).

I see no other instance in PVRClient.cpp where another PVR interface method is implicitly invoked by Kodi. I also think that other addons may run into the same problem with Leia, not being able to properly report the MIME type and/or realtime flag unless they open and examine the stream -- and likely coming up with the same solution.

For now, I can reference count the calls to Open/Close as a workaround, but I would appreciate consideration of this PR for future Kodi 18.x releases as it's not technically 'breaking' in nature. Thank you!

How Has This Been Tested?

Tested on Kodi 18.0 Leia, both at the "18.0-Leia" tag commit as well as the current master branch commit at the time the PR was generated. Windows 10, x86 and x64 (non-UWP/store). I verified that CloseLiveStream()/CloseRecordedStream() was invoked the expected number of times (once). I ran into no issues switching channels or changing from Live to Recorded and vice-versa.

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

@djp952 djp952 changed the title Don't implictly call CloseXXXStream() before calling OpenXXXStream() [PVR] Don't implictly call CloseXXXStream() before calling OpenXXXStream() Feb 10, 2019

@@ -1269,7 +1268,6 @@ PVR_ERROR CPVRClient::OpenRecordedStream(const CFileItem& recordingItem)
}

return DoAddonCall(__FUNCTION__, [this, recording](const AddonInstance* addon) {

This comment has been minimized.

Copy link
@Rechi

Rechi Feb 10, 2019

Member

xbmc/addons/PVRClient.cpp:1270:37: warning: lambda capture 'this' is not used [-Wunused-lambda-capture]

Suggested change
return DoAddonCall(__FUNCTION__, [this, recording](const AddonInstance* addon) {
return DoAddonCall(__FUNCTION__, [recording](const AddonInstance* addon) {

This comment has been minimized.

Copy link
@djp952

djp952 Feb 11, 2019

Author Contributor

I had tried that in response to the Jenkins log, but the [this] capture is needed in those functions. The module will not compile if you remove them. Perhaps some difference across the various compilers here.

This comment has been minimized.

Copy link
@Rechi

Rechi Feb 11, 2019

Member

You only removed it in this lambda capture or also in the other one?
As the compiler warning indicates it's only unused here. If that still gives errors please tell which compiler you are using and post the compile error you get.

@ksooo

This comment has been minimized.

Copy link
Member

commented Feb 10, 2019

This wont definitely not go into v18, as I fear that this will break other addons, which rely on this behavior, which is like this from "day one" of PVR.

@djp952

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

I understand, and I appreciate even considering it. I checked the various PVRs and found at least one that on the surface does rely upon this (argustv). If Kodi never explicitly calls CloseLiveStream() for some reason, there is nothing in the addon to protect itself from multiple calls to OpenLiveStream().

If this behavior was intentional that makes me the outlier (again). I'm fine with cancelling/deleting the PR completely, it's very easy to work around. If the PR dies, may I suggest a tweak to the interface documentation in the header file that says Close will be implicitly called just prior to Open()? If that would be OK, I could also just change this PR to do that instead - some suggested verbiage change only?

Thanks!

@ksooo

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

@djp952 let's keep the implementation as it is and update the API documentation instead.

@djp952 djp952 changed the title [PVR] Don't implictly call CloseXXXStream() before calling OpenXXXStream() [PVR] Document implict call to CloseXXXStream() before OpenXXXStream() Feb 12, 2019

@djp952

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2019

Code changes reverted; remarks added to PVR header file instead. Let me know if you want different wording, or edit to your liking. I've never been accused of being terse but I tried.

@ksooo

ksooo approved these changes Feb 12, 2019

Copy link
Member

left a comment

Thanks.

@ksooo ksooo added this to the Leia 18.2-rc1 milestone Feb 12, 2019

@MartijnKaijser MartijnKaijser merged commit 80dab3a into xbmc:master Feb 17, 2019

1 check was pending

default Found some time, building it now.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.