[pvr] add optional support for playing recordings from epg timeline or epg osd #3900

vkosh · 2013-12-25T22:52:54Z

This allows user to play recording through programme info dialog, which is shown by clicking on programme in the past in epg timeline or epg osd view. It also switches to a recording playback from epg search result if selected programme has one.
Programmes with associated recordings are shown with HasRecording marker (skin.confluence).
To associate recording to a programme pvr client should set recording id for corresponding epg tag. This will turn on epg updates on recordings update.
If pvr client doesn't use epg-recordings linking, then no such updates occured and everything else remains as it is now (views, switching to live stream, etc.).
If pvr backend has ability of recording everything (e.g. network pvr), then pvr client should implement its own recordings update logic to trigger recordings update on each programme end.
More info at http://forum.xbmc.org/showthread.php?tid=179792

xbmc/epg/EpgDatabase.cpp

@@ -79,7 +79,8 @@ bool CEpgDatabase::CreateTables(void)
          "iSeriesId       integer, "
          "iEpisodeId      integer, "
          "iEpisodePart    integer, "
-          "sEpisodeName    varchar(128)"
+          "sEpisodeName    varchar(128), "
+          "sRecordingId    varchar(1024) "


Jalle19 · 2013-12-26T21:55:37Z

In my opinion you should split the changes into a couple of commits. You have pure cosmetics (some minor comment changes), iterator typedefs, CPVRRecordingPtr implementation and the main PR material in one commit now. Splitting it up makes it easier to see what has really been added/changed.

Jalle19 · 2013-12-26T22:07:17Z

May I also suggest that you use std::string instead of CStdString, especially since you already use the former in CPVRRecordingUid. Your current approach makes for implicit casts all over the place.

vkosh · 2013-12-26T23:27:25Z

@Jalle19 thanks for the review. I'll split the commit. As for CStdString - may be it would be better to do in separate PR? I used std::string in new code that doesn't actually affect much on current implementation.

vkosh · 2013-12-27T16:33:57Z

@Jalle19 I fixed the issues you pointed out, reorganized the commits as you suggest, made recording ID of std::string type to avoid implicit casts, and also added "has recording" marker to programme search view. Could you re-review please.

Jalle19 · 2013-12-27T22:37:37Z

Thanks, looks way better now.

I can't really comment on the main implementation since I haven't been able to test this yet, though I did notice that the changes in GUIDialogPVRGuideInfo and GUIWindowPVRCommon are very similar, infact both methods seem to accomplish the exact same thing, with the latter having a more polished implementation. @xhaggi didn't you comment on this a while back?

Anyway, refactoring of that belongs in another PR.

xhaggi · 2013-12-28T10:57:57Z

@Jalle19 thanks for your review, i don't have time to do this now and we need to discuss the entire idea with @opdenkamp.

opdenkamp · 2014-01-02T21:50:43Z

i'll review this later when i got more time. we won't change the interface until the release

MartijnKaijser · 2014-05-21T16:31:58Z

@vkosh please re-base against current master
@xhaggi and @Jalle19 added for review

vkosh · 2014-05-21T23:49:06Z

@MartijnKaijser, @xhaggi, @Jalle19 rebased and refactored.

Jalle19 · 2014-05-22T04:39:29Z

@xhaggi I think we should use the "epgUid" field already present in recordings. I don't think any addons use it at the moment since XBMC itself doesn't use it, but this and the code that gets a recording based on an EPG tag could use it. It would be faster too I suppose.

I can pull this to a local branch and have a look.

Jalle19 · 2014-05-22T04:41:01Z

I meant "and the code that gets a timer based on an EPG tag" earlier.

xbmc/epg/EpgInfoTag.cpp

@@ -55,6 +55,9 @@ CEpgInfoTag::CEpgInfoTag(void) :

  CPVRTimerInfoTagPtr emptyTimer;
  m_timer = emptyTimer;
+
+  CPVRRecordingPtr emptyRecording;
+  m_recording = emptyRecording;


Jalle19 · 2014-05-22T07:56:27Z

Apparently only PVR_TIMER has en epgUid member. I'd like to see it added to PVR_RECORDING too, this way we could set the recording tag for EPG items when recordings are fetched (should do the same for timers instead of the manual lookup like now, but that's for another PR).

xbmc/epg/EpgDatabase.cpp

@@ -71,7 +71,8 @@ void CEpgDatabase::CreateTables(void)
        "iSeriesId       integer, "
        "iEpisodeId      integer, "
        "iEpisodePart    integer, "
-        "sEpisodeName    varchar(128)"
+        "sEpisodeName    varchar(128), "
+        "sRecordingId    text"


xbmc/pvr/recordings/PVRRecordings.cpp

+  PVRRecordingMap::const_iterator it = m_recordings.find(CPVRRecordingUid(iClientId, strRecordingId));
+  if (it != m_recordings.end())
+  {
+    return it->second;


Jalle19 · 2014-05-22T08:47:57Z

I think you'll need to ignore my comments about epgUid earlier, apparently it's used when adding a timer on the backend.

opdenkamp · 2014-05-22T18:15:51Z

correct, backend may want to link the epg data to the timer/recording too

vkosh · 2014-05-25T16:17:36Z

Fixed and rebased. Please re-review.

xhaggi · 2014-05-26T11:57:58Z

@ronie could you please take a look at the confluence part, commit 6f06898

xhaggi · 2014-05-26T12:04:02Z

@vkosh except the small cleanups, it looks good.

Jalle19 · 2014-05-26T13:14:14Z

While looking at some of the recordings related code I'm wondering if we can just change the == operator for recordings to check only the client and recording ID. That way we wouldn't need a separate class for comparing. @opdenkamp do you know off the top of your head why a recording is only considered equal if absolutely everything matches?

ronie · 2014-05-26T15:28:21Z

@ronie could you please take a look at the confluence part, commit 6f06898

looks good to me :-)

…tainer

…r epg osd

…ed recordings in epg timeline view, epg osd and programme search results

vkosh · 2014-05-28T11:47:26Z

Rebased and polished.

@Jalle19 returning to CPVRRecordingsUid with overloaded operators and recordings map implementation. Each recording can be uniquely identified by clientId+recordingId tuple provided by backend, and we need to find recordings by this tuple as fast as we can during pvr loading/updating, because each EPG tag may have associated recording (e.g. my pvr client links all past EPG's to recordings).
Therefore I decided to map recordings by the uid tuple and use std::map for this with tuple keys represented by CPVRRecordingsUid. In this case we have O(log n) complexity for recordings lookup.
The previous implementation uses vector for recordings and it has O(n) complexity for lookup which is worse than O(log n).
So i'd prefer to leave CPVRRecordingsUid implementation as is.

Jalle19 · 2014-05-28T11:48:55Z

You're right, I guess this is fine by me then.

jmarshallnz · 2014-07-11T06:52:21Z

@Jalle19, @xhaggi by the looks this is ready for a button push?

Jalle19 · 2014-07-11T06:54:19Z

Yeah I think so. @xhaggi ?

jmarshallnz · 2014-07-11T06:55:55Z

Cool - it'd be good to get this (plus anything else that hits epg/pvr) in before I push in the stdstring changes, as otherwise they'll all need yet another rebase.

Much easier if I rebase the stdstring stuff than having a bunch of others having to rebase stuff up :)

xhaggi · 2014-07-11T07:51:29Z

everything else is fine by me too.

jmarshallnz · 2014-07-11T08:14:11Z

@xhaggi I agree, but I don't care about it enough to delay this any longer. It can always be cleaned up later if you like :)

jenkins build this please

xhaggi · 2014-07-11T08:17:22Z

sure ;)

[pvr] add optional support for playing recordings from epg timeline or epg osd

xhaggi · 2014-07-12T12:28:33Z

@vkosh and @jmarshallnz we missed syncing the xbmc_epg_types.h with the addon repo.
https://github.com/opdenkamp/xbmc-pvr-addons/blob/master/xbmc/xbmc_epg_types.h

opdenkamp · 2014-07-12T12:36:06Z

what happened to merge windows? this indeed needs to be done, the api version needs to be bumped, add-on versions needs to be bumped, and then it needs to be pulled along with the API change. I bet all PVR add-ons are now crashing.

xhaggi · 2014-07-12T12:51:53Z

i'll do a PR for both

xhaggi · 2014-07-12T13:04:06Z

done. #5012 and opdenkamp/xbmc-pvr-addons#331

Jalle19 reviewed Dec 26, 2013
View reviewed changes

ghost assigned opdenkamp Jan 2, 2014

jmarshallnz reviewed May 22, 2014
View reviewed changes

opdenkamp reviewed May 22, 2014
View reviewed changes

xbmc/pvr/recordings/PVRRecordings.cpp

PVRRecordingMap::const_iterator it = m_recordings.find(CPVRRecordingUid(iClientId, strRecordingId));

if (it != m_recordings.end())

{

return it->second;

This comment was marked as spam.

Sign in to view

vkosh added 6 commits May 28, 2014 14:49

[pvr] refactor: define map type for epg container

685ccf0

[pvr] refactor: use map to improve recording lookup in recordings con…

85f3315

…tainer

[pvr] refactor: use std::string for recording id to avoid implicit casts

fb9b6a8

[pvr] cosmetics: fix minor typos in comments

b26487f

[pvr] add optional support for playing recordings from epg timeline o…

dffecf4

…r epg osd

[confluence] add "has recording" marker to programmes having associat…

31e2567

…ed recordings in epg timeline view, epg osd and programme search results

jmarshallnz added a commit that referenced this pull request Jul 11, 2014

Merge pull request #3900 from vkosh/epg-recording

dd9b215

[pvr] add optional support for playing recordings from epg timeline or epg osd

jmarshallnz merged commit dd9b215 into xbmc:master Jul 11, 2014

Jalle19 mentioned this pull request Jul 15, 2014

[pvr.tvh] link events with recordings and vice versa adamsutton/xbmc-pvr-addons#47

Merged

[pvr] add optional support for playing recordings from epg timeline or epg osd #3900

[pvr] add optional support for playing recordings from epg timeline or epg osd #3900

Conversation

vkosh commented Dec 25, 2013

This comment was marked as spam.

This comment was marked as spam.

Jalle19 commented Dec 26, 2013

Jalle19 commented Dec 26, 2013

vkosh commented Dec 26, 2013

vkosh commented Dec 27, 2013

Jalle19 commented Dec 27, 2013

xhaggi commented Dec 28, 2013

opdenkamp commented Jan 2, 2014

MartijnKaijser commented May 21, 2014

vkosh commented May 21, 2014

Jalle19 commented May 22, 2014

Jalle19 commented May 22, 2014

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

Jalle19 commented May 22, 2014

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

Jalle19 commented May 22, 2014

opdenkamp commented May 22, 2014

vkosh commented May 25, 2014

xhaggi commented May 26, 2014

xhaggi commented May 26, 2014

Jalle19 commented May 26, 2014

ronie commented May 26, 2014

vkosh commented May 28, 2014

Jalle19 commented May 28, 2014

jmarshallnz commented Jul 11, 2014

Jalle19 commented Jul 11, 2014

jmarshallnz commented Jul 11, 2014

xhaggi commented Jul 11, 2014

jmarshallnz commented Jul 11, 2014

xhaggi commented Jul 11, 2014

xhaggi commented Jul 12, 2014

opdenkamp commented Jul 12, 2014

xhaggi commented Jul 12, 2014

xhaggi commented Jul 12, 2014