HTSP v26 support #257

Glenn-1990 · 2016-12-13T18:31:47Z

Htsp v26 support.

Edit: removed WIP

ksooo · 2016-12-15T08:28:33Z

The pvr addon capabilities are requested by Kodi before we can determine the htsp version. That's why there is a setting for now.

The setting should not be nessecary. Yes, on first call of getAddonCapabilites we are not connected (most probably), but addon capabilites are re-read directly after the addon state changes to CONNECTED. => https://github.com/xbmc/xbmc/blob/master/xbmc/pvr/addons/PVRClients.cpp#L1536 If this does not work, it's broken.

ksooo · 2016-12-15T09:32:50Z

"GetTimerTypes" does several htsp version checks, but this is somehow broken.
Htsp version is not determined yet when calling this function. The connection state is still "PVR_CONNECTION_STATE_CONNECTING" then.

Are you sure it does not work? GetTimerTypes should be called not only once, but on demand, for instance everytime timer settings dialog is opened, and there is no caching of the types in Kodi PVR core. So, it should work. However, if there is a call to GetTimerTypes that comes to "early", this could be a bug in Kodi PVR core which has no serious consequences, though. I will look inti this.

Glenn-1990 · 2016-12-15T17:02:37Z

@ksooo Good to know that GetTimerTypes gets called every time, that pointed me to problem and solution ;-)

Is rereading the addon settings after connecting a new thing? I always remembered that this was a limitation, I'll see if I can get it working without the setting then :-)

ksooo · 2016-12-15T17:17:12Z

So, both "problems" solved?

Rereading add-on capabilities (not settings, that's different!) is there since day 1 of async connect, means since very early Krypton alphas.

ksooo · 2016-12-15T18:38:22Z

Just looked it up in the code: both addon capabilities and timer types are reread if connection state changes to CONNECTED. So, timer types are not always fetched from the backend, like I said earlier, but only on (re)connect, exactly like addon capabilities.

Glenn-1990 · 2016-12-15T19:36:07Z

Oops, closed by accident.

@ksooo Both issues resolved, this async stuff is really an improvement ;-)

Jalle19 · 2016-12-20T10:07:48Z

@Glenn-1990 the "delete channels without services" can be dropped now that you did tvheadend/tvheadend@e73a8b4 right?

Glenn-1990 · 2016-12-20T16:35:49Z

@Jalle19 indeed ;-)

ksooo · 2016-12-24T22:14:24Z

@piotrasd may i kindly ask you not to hijack a random PR for completely unrelated feature requests. Thank you.

Glenn-1990 · 2016-12-26T19:04:45Z

@ksooo @Jalle19 Updated some commits

Jalle19 · 2017-01-04T16:54:00Z

src/HTSPConnection.cpp

@@ -495,6 +495,7 @@ bool CHTSPConnection::SendHello ( void )
 bool CHTSPConnection::SendAuth
  ( const std::string &user, const std::string &pass )
 {
+  uint32_t access, u32;


access should be bool

Jalle19 · 2017-01-04T16:54:26Z

src/HTSPConnection.cpp

+  // <= htsp v25, msg present = no access, msg missing = access
+  // >= htsp v26, msg should be present in any case
+  if (m_htspVersion <= 25) //
+    access = msg ? 0 : 1;


I would return early end skip the else clause, but it's up to you

Jalle19 · 2017-01-04T16:54:35Z

src/HTSPConnection.cpp

+    access = msg ? 0 : 1;
+  else
+  {
+    if (msg == NULL || (!htsmsg_get_u32(msg, "noaccess", &u32) && u32))


Jalle19 · 2017-01-04T16:55:17Z

src/HTSPConnection.cpp

+
+      /* Log received permissions */
+      if (!htsmsg_get_u32(msg, "admin", &u32))
+        Logger::Log(LogLevel::LEVEL_INFO, "admin permissions: %i", u32);


Can you log these lines with two spaces extra indentation, like we do for demuxer streams? I find it makes the log more readable.

Jalle19 · 2017-01-04T16:55:38Z

src/HTSPConnection.cpp

+      if (!htsmsg_get_u32(msg, "streaming", &u32))
+        Logger::Log(LogLevel::LEVEL_INFO, "streaming permissions: %i", u32);
+      if (!htsmsg_get_u32(msg, "dvr", &u32))
+        Logger::Log(LogLevel::LEVEL_INFO, "dvr permissions: %i", u32);


DVR in uppercase, same below for "dvr connection limit"

Jalle19 · 2017-01-04T16:56:00Z

src/HTSPConnection.cpp

+      if (!htsmsg_get_u32(msg, "limitdvr", &u32))
+        Logger::Log(LogLevel::LEVEL_INFO, "dvr connection limit: %i", u32);
+      if (!htsmsg_get_u32(msg, "limitstreaming", &u32))
+        Logger::Log(LogLevel::LEVEL_INFO, "sreaming connection limit: %i", u32);


typo (should be streaming)

Jalle19 · 2017-01-04T16:58:10Z

src/Tvheadend.cpp

+  if (m_conn.GetProtocol() < 26)
+    return PVR_ERROR_NOT_IMPLEMENTED;
+
+  /* Seems a bit odd */


What does this mean?

Kodi calls this function to request the play position, although the play position was already transferred to Kodi as a member of PVR_RECORDING.

Jalle19 · 2017-01-04T16:58:43Z

src/Tvheadend.cpp

@@ -1367,7 +1408,7 @@ bool CTvheadend::Connected ( void )
  m_asyncState.SetState(ASYNC_CHN);

  msg = htsmsg_create_map();
-  if (Settings::GetInstance().GetAsyncEpg())
+  if (Settings::GetInstance().GetAsyncEpg() || m_conn.GetProtocol() >= 26)


You can drop this commit now

Jalle19 · 2017-01-04T16:59:17Z

src/Tvheadend.cpp

@@ -598,6 +602,43 @@ PVR_ERROR CTvheadend::RenameRecording ( const PVR_RECORDING &rec )
  return SendDvrUpdate(m);
 }

+PVR_ERROR CTvheadend::SetPlayCount ( const PVR_RECORDING &rec, int playcount )


You use playCount in another place, I suggest to use it here too for consistency

Jalle19 · 2017-01-04T16:59:38Z

src/Tvheadend.cpp

+  return SendDvrUpdate(m);
+}
+
+PVR_ERROR CTvheadend::SetPlayPosition ( const PVR_RECORDING &rec, int playposition )


You use playPosition when calling this method, I suggest you name this the same for consistency

Glenn-1990 · 2017-01-08T14:40:37Z

@Jalle19 @ksooo Updated and removed WIP

Some fixes are preferable for Krypton, should I separate the fixes in another PR apart from the HTSP v26 stuff?

ksooo · 2017-01-08T14:49:13Z

Lets finalize this PR first before opening one for Krypton, to prevent unneeded resync.

ksooo · 2017-01-08T14:50:04Z

I still had no time to take a closer look at your changes.

ksooo · 2017-01-08T14:50:22Z

Have you seen and addressed my comments?

ksooo · 2017-01-08T15:18:25Z

At least CTvheadend::GetPlayPosition needs still to be fixed.

Will take a closer look at the rest and comment back as soon as i find some time.

Glenn-1990 · 2017-01-08T16:07:43Z

@ksooo Yes I addressed you comments about add-on capabilities and timertypes.

ksooo · 2017-01-08T16:10:48Z

What about getplayposition?

Glenn-1990 · 2017-01-08T17:07:06Z

@ksooo GetPlayPosition() is working (when on htsp v26 or up) but I'm still not sure if this is how it should be. If you could take a look :-)

ksooo · 2017-01-08T17:17:52Z

GetPlayPosition() should not return what came in with rec.iLastPlayedPosition, but obtain and return latest position stored in backend.

Glenn-1990 · 2017-01-08T17:34:01Z

Well as we are working async, rec.iLastPlayedPosition will always hold the correct value. We push an Kodi update every time the position on the backend changes.
https://github.com/kodi-pvr/pvr.hts/blob/master/src/Tvheadend.cpp#L1895

But ofc we could also use our local playposition (from m_recordings)?

ksooo · 2017-01-08T21:34:01Z

We push an Kodi update every time the position on the backend changes.

How often is this in worst case? can this cause performance problems? Not sure we actually should do this, especially as we're getting asked (GetPlayPosition) when kodi needs an update!

But ofc we could also use our local playposition (from m_recordings)?

I think we should go for this. "Feels" better.

Jalle19

Apart from the minor comment this looks good to me

Jalle19 · 2017-01-10T13:58:44Z

src/Tvheadend.cpp

@@ -665,14 +706,20 @@ PVR_ERROR CTvheadend::GetTimerTypes ( PVR_TIMER_TYPE types[], int *size )

  /* PVR_Timer.iPreventDuplicateEpisodes values and presentation.*/
  static std::vector< std::pair<int, std::string> > deDupValues;
-  if (deDupValues.size() == 0)


Is it okay to remove this check? Won't this mean we may accidentally push_back duplicates?

This will not work as we do a version check a few lines below, and htspversion = 0 the first time.
It's working for the other methods, so I guess Kodi will reject duplicates.

IMO it's better to be safe than sorry, we cannot rely on Kodi doing the "right thing". Is there a way to change this to non-static so this thing wouldn't be an issue?

ksooo · 2017-01-10T16:25:28Z

I still want to do a review before we merge this. Will do as soon as I find the time. No need to rush, imo.

Jalle19 · 2017-01-10T19:36:59Z

@ksooo sure

Glenn-1990 · 2017-01-10T20:21:06Z

How often is this in worst case? can this cause performance problems? Not sure we actually should do this, especially as we're getting asked (GetPlayPosition) when kodi needs an update!
@ksooo Only when a user presses "stop" on other clients.

What's the use of "iLastPlayedPosition" as we have "GetPlayPosition()" ? The API is unclear about it. If we don't need "iLastPlayedPosition", the code can be simplified.

ksooo · 2017-01-10T20:33:18Z

What's the use of "iLastPlayedPosition" as we have "GetPlayPosition()" ? The API is unclear about it. If we don't need "iLastPlayedPosition", the code can be simplified.

I have not invented this API, but I guess GetPlayPosition() is there to have a lightweight method to be able to directly and quickly (and frequently?) pull the play position data for a particular recording. We only have bulk query methods otherwise and refreshing data for all recordings (GetRecordings) might be too heavyweight.

iLastPlayedPosition seems to be there for setting/updating the last played position using Add|UpdateRecording which is not a bulk operation and thus fits performance-wise.

Jalle19 · 2017-01-10T21:26:51Z

@Glenn-1990 @ksooo would it perhaps be a good idea to separate the "Fixed" commits to a separate PR while we grind this one?

ksooo · 2017-01-11T08:47:11Z

+1 for doing separate PR for fixes and features. Makes the review easier.

ksooo · 2017-01-25T21:31:07Z

src/client.cpp

@@ -217,6 +217,8 @@ PVR_ERROR GetAddonCapabilities(PVR_ADDON_CAPABILITIES* pCapabilities)
  pCapabilities->bHandlesInputStream         = true;
  pCapabilities->bHandlesDemuxing            = true;
  pCapabilities->bSupportsRecordingEdl       = true;
+  pCapabilities->bSupportsRecordingPlayCount = tvh->GetProtocol() >= 26 ? true : false;
+  pCapabilities->bSupportsLastPlayedPosition = tvh->GetProtocol() >= 26 ? true : false;


these two lines look a little bit overcomplicated. Why not just

pCapabilities->bSupportsRecordingPlayCount = (tvh->GetProtocol() >= 26); pCapabilities->bSupportsLastPlayedPosition = (tvh->GetProtocol() >= 26);

ksooo · 2017-01-25T21:39:38Z

@Glenn-1990 good job, thanks. Could you please add changelog and version bump. We are ready to then, imo.

Jalle19 · 2017-01-25T22:02:04Z

+1 from me too if you add the change log entry etc.

Glenn-1990 · 2017-01-29T19:23:16Z

Changelog added

Jalle19 · 2017-01-30T07:44:54Z

@ksooo good to go?

ksooo · 2017-01-30T08:04:16Z

Yep, good to go.

Does anything speak against a Krypton backport @Glenn-1990?

Jalle19 · 2017-01-30T12:38:39Z

@ksooo Krypton is still unreleased so everything should be backported to it IMO.

ksooo · 2017-01-30T16:15:02Z

@ksooo Krypton is still unreleased so everything should be backported to it IMO.

Yep, but we need to hurry then, because we want to release Krypton really very soon, @Glenn-1990 ;-)

Glenn-1990 · 2017-01-30T16:45:37Z

@ksooo A backport would be nice, maybe someone else can do a runtime test first? Especially the play count and play position stuff.

ksooo · 2017-01-30T17:09:00Z

That's problematic, due to lack of time on my end. If we are not sure that it actually works, no backport now, please.

However, ofc a Krypton PR is welcome. I will test asap.

Glenn-1990 · 2017-01-30T21:58:11Z

Under some circumstances the playposition seems to be wrong as Kodi uses "-1" for fully watched. Only affects htsp v26 and up. I'll come up with a PR, already addressed in the Krypton PR.

Glenn-1990 closed this Dec 15, 2016

Glenn-1990 force-pushed the tvh42 branch from 24ce97d to 3fa771b Compare December 15, 2016 19:29

Glenn-1990 reopened this Dec 15, 2016

Glenn-1990 force-pushed the tvh42 branch from e53bff2 to 5789ccb Compare December 26, 2016 19:02

Jalle19 requested changes Jan 4, 2017

View reviewed changes

Glenn-1990 force-pushed the tvh42 branch from 5789ccb to 852227b Compare January 8, 2017 14:35

Glenn-1990 changed the title ~~[WIP] htsp v26 support~~ HTSP v26 support Jan 8, 2017

Jalle19 approved these changes Jan 10, 2017

View reviewed changes

Glenn-1990 force-pushed the tvh42 branch from 852227b to 8ee66d5 Compare January 10, 2017 20:10

Glenn-1990 force-pushed the tvh42 branch from 8ee66d5 to cf3a05b Compare January 17, 2017 20:02

ksooo approved these changes Jan 25, 2017

View reviewed changes

Glenn-1990 added 6 commits January 29, 2017 20:03

[htsp v26] adapt authentication changes

3e64eb3

[htsp v26] Fixed: dvr start and stop times are set to '0'

8e32bd4

[htsp v26] Hack can be removed now

0f51070

[htsp v26] implement server based playcount and playposition

202e652

[htsp v26] bump supported version

1263478

version bump to 4.0.6

d0e264c

Glenn-1990 force-pushed the tvh42 branch from cf3a05b to d0e264c Compare January 29, 2017 19:21

Jalle19 merged commit b290c8c into kodi-pvr:master Jan 30, 2017

Glenn-1990 mentioned this pull request Jan 30, 2017

Krypton HTSP v26 support #279

Closed

HTSP v26 support #257

HTSP v26 support #257

Conversation

Glenn-1990 commented Dec 13, 2016 • edited Loading

ksooo commented Dec 15, 2016

ksooo commented Dec 15, 2016 • edited Loading

Glenn-1990 commented Dec 15, 2016

ksooo commented Dec 15, 2016

ksooo commented Dec 15, 2016

Glenn-1990 commented Dec 15, 2016

Jalle19 commented Dec 20, 2016

Glenn-1990 commented Dec 20, 2016

ksooo commented Dec 24, 2016

Glenn-1990 commented Dec 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Glenn-1990 commented Jan 8, 2017

ksooo commented Jan 8, 2017

ksooo commented Jan 8, 2017

ksooo commented Jan 8, 2017

ksooo commented Jan 8, 2017

Glenn-1990 commented Jan 8, 2017

ksooo commented Jan 8, 2017

Glenn-1990 commented Jan 8, 2017

ksooo commented Jan 8, 2017

Glenn-1990 commented Jan 8, 2017

ksooo commented Jan 8, 2017 • edited Loading

Jalle19 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksooo commented Jan 10, 2017 • edited Loading

Jalle19 commented Jan 10, 2017

Glenn-1990 commented Jan 10, 2017 • edited Loading

ksooo commented Jan 10, 2017 • edited Loading

Jalle19 commented Jan 10, 2017

ksooo commented Jan 11, 2017

Choose a reason for hiding this comment

ksooo commented Jan 25, 2017

Jalle19 commented Jan 25, 2017

Glenn-1990 commented Jan 29, 2017

Jalle19 commented Jan 30, 2017

ksooo commented Jan 30, 2017 • edited Loading

Jalle19 commented Jan 30, 2017

ksooo commented Jan 30, 2017

Glenn-1990 commented Jan 30, 2017

ksooo commented Jan 30, 2017

Glenn-1990 commented Jan 30, 2017

Glenn-1990 commented Dec 13, 2016 •

edited

Loading

ksooo commented Dec 15, 2016 •

edited

Loading

ksooo commented Jan 8, 2017 •

edited

Loading

ksooo commented Jan 10, 2017 •

edited

Loading

Glenn-1990 commented Jan 10, 2017 •

edited

Loading

ksooo commented Jan 10, 2017 •

edited

Loading

ksooo commented Jan 30, 2017 •

edited

Loading