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] Fix CPVRTimers::GetRecordingTimerForRecording to respect timer'… #15443

Merged
merged 1 commit into from Feb 6, 2019

Conversation

@ksooo
Copy link
Member

commented Feb 6, 2019

…s start and stop padding time + optimize to check epg event uids if present.

Fixes #15421

@Jalle19 mind doing a quick review?

@ksooo ksooo added this to the Leia 18.1-rc1 milestone Feb 6, 2019
@ksooo ksooo requested a review from Jalle19 Feb 6, 2019
@ksooo ksooo added the Type: Fix label Feb 6, 2019
@Jalle19
Jalle19 approved these changes Feb 6, 2019
xbmc/pvr/timers/PVRTimers.cpp Outdated Show resolved Hide resolved
…s start and stop padding time + optimize to check epg event uids if present.
@ksooo ksooo force-pushed the ksooo:pvr-fix-recording-isinprogress branch from 2525bc6 to f232674 Feb 6, 2019
@ksooo

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2019

jenkins build this please

@ksooo ksooo merged commit 56d92c5 into xbmc:master Feb 6, 2019
1 check passed
1 check passed
default You're awesome. Have a cookie
Details
@ksooo ksooo deleted the ksooo:pvr-fix-recording-isinprogress branch Feb 6, 2019
@linknetx

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

Did this fix find it's way into the last nightly build KodiSetup-20190206-7001c652-master-x64 ?

If it did then this hasn't fixed the issue for this setup:

Kodi 18.0
TVMosaic 1.0.0 17051
TVMosaic PVR addon 4.7.0

The current recording status still doesn't show up in the recordings list.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

Yes the fix went in and it definitely fixes this for me (pvr.hts). Maybe we've missed an edge case? Maybe tvmosaic behaving different wrt start and end times reported for recordings and timers... That could make a difference.

IIRC, you are a developer. How about debugging this by yourself?

@linknetx

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

Am I a developer, afraid not. I just do some coding in my spare time for my own projects normally in C#. My skills in C++ are somewhat limited and that particular segment of code is a stretch too far!

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

I could provide you with a special build with additional logging to try to identify the problem this way? Would you have some spare time for this? You are using Windows, right?

@linknetx

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

@linknetx

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

@ksooo

By hit and miss it works with this part of the code commented out:

/* && timersEntry->m_iClientChannelUid == recording.ChannelUid() */

in CPVRTimers::GetRecordingTimerForRecording:

if (timersEntry->IsRecording() && !timersEntry->IsTimerRule() && timersEntry->m_iClientId == recording.ClientID() /* && timersEntry->m_iClientChannelUid == recording.ChannelUid()*/ )

And then it uses the alternative section of code to return the timersEntry based on matching start and end times.

Not sure why this works but hope that it helps with sorting out the issue.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

Let's see what comes out of the extened logging session. Here is the link for the test build.

http://mirrors.kodi.tv/test-builds/windows/win64/KodiSetup-20190207-1b9cbdf4-pvr-fix-isrecording-x64.exe

Please enable PVR component logging, start a recording, goto recordings window (verify that the red dot is not where it should be), and post the debug log, then.

@linknetx

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

OK, done that, and can confirm that the red dot is not showing in recordings list on the recording item.

Here is the log:

kodi.log

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

Sorry, you have not enabled debug logging. So, log contains nothing of interest.

Goto settings -> system -> logging. enable logging AND component logging for pvr

@linknetx

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

OK, try again:

kodi.log

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

OK, I now know what goes wrong. Unfortunately, this cannot work if the pvr addon (pvr.dvblink in your case) does not supply the channel uid with its recordings. This is an API change rquested some years(!) ago, btw.

=> https://github.com/kodi-pvr/pvr.dvblink/blob/master/src/DVBLinkClient.cpp#L1456

You should file an issue for pvr.dvblink. Please note that it is no bug that the adodn does not implement this; the channel uid is not mandatory. But if this is not done, some (non crucial) Kodi features, like the "red dot" simply cannot work. So, you should kindly ask the adonn dev to implement this, pointing out what the consequence of the missing addon feature is.

EDIT: Now that I know the root cause, it's sure that the "red dot" in the recordings window never worked with pvr.dvblink. For pvr.hts (used by the submitter of the issue fixed by this PR), however, I had introduced a regression which is fixed now.

@linknetx

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

OK, thanks for your help,

Just so that I'm clear about this does this need fixing in both the pvr.dvblink server and the pvr.dvblink client?

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

The addon needs to be fixed.
Not sure what you mean with pvr.dvblink client - if you mean Kodi core code changes, then the answer is no.

@linknetx

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

so would this also require corresponding changes on the dbvlink/tvmosaic backend server

Sorry, don't know .

@linknetx

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

OK, thanks again, I'll refer it to the pvr.dvblink developers.

@linknetx

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

@ksooo

I've put in a request for DVBLogic to implement the ChannelUid so hopefully this should get fixed eventually.

Since the ChannelUid is optional and disables one of the features of Kodi, namely the red dot recording icon, would it be better to have a general solution where the feature works without the optional ChannelUid but uses it when available.

I'm sure you must have considered matching channel names for the case where the ChannelUid is invalid and rejected it for a valid reason:

if (timersEntry->IsRecording() &&
!timersEntry->IsTimerRule() &&
timersEntry->m_iClientId == recording.ClientID() &&
(recording.ChannelUid() == PVR_CHANNEL_INVALID_UID) ?
timersEntry->ChannelName() == recording.m_strChannelName :
timersEntry->m_iClientChannelUid == recording.ChannelUid())

One reason for rejecting it would be that no channel names are available (is that possible?) but that can be checked for:

if (timersEntry->IsRecording() &&
!timersEntry->IsTimerRule() &&
timersEntry->m_iClientId == recording.ClientID() &&
(recording.ChannelUid() == PVR_CHANNEL_INVALID_UID &&
timersEntry->ChannelName().size() > 0 &&
recording.m_strChannelName.size() > 0) ?
timersEntry->ChannelName() == recording.m_strChannelName :
timersEntry->m_iClientChannelUid == recording.ChannelUid())

Another reason for rejecting it would be if two (or more) channels had identical names, I suppose that's possible, but they would both (or all) have to be currently recording for it to cause a problem.

I think that this minor issue would be better than the current situation where the ChannelUid isn't available. Which is that is a currently recording item shows the delete option in the context menu, which doesn't work and is confusing, rather than the correct stop recording option.

By the way I've tested both of these and they work perfectly.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

Since the ChannelUid is optional and disables one of the features of Kodi, namely the red dot recording icon,

To be accurate, it prevents the red dot only in the recordings window; the dot works everywhere else.

So, it is just a very minor glitch, if at all. As I said, no essential functuonality is bound to presense of channel uid.

Right approach is not to hack around with comparing channel names (which does not work 100%, btw) and stuff, but to make channeluid mandatory for PVR_RECORDING. Especially, as I'm convinced that it is a no-brainer for pvr addon devs to add support for this (they already have that information in the addon).

Problem is, that some addon devs are quite lazy (err, don't have enough free time) and only implement mandatory stuff or do something if users complain.

You see when the comment was added that there is a todo? 3 years ago...
=> https://github.com/kodi-pvr/pvr.dvblink/blame/master/src/DVBLinkClient.cpp#L1455

@linknetx

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

Right approach is not to hack around with comparing channel names and stuff, but to make channeluid mandatory for PVR_RECORDING.

That's a better solution, can that be implemented?

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

That's a better solution, can that be implemented?

Actual work needs to be done by the addon maintainers. There is no way around that.

All I can (need to) do on the kodi side is updating the api documentation (optional -> mandatory) and announcing the API change for v19, making it a bug if the addon does not support PVR_RECORDING.channeluid.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

@ksooo a backend can record some program on a channel that is not synchronized with Kodi. Would this still be possible if PVR_RECORDING.channeluid was mandatory?

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

This is all a bit messy. I just discovered that PVR_TIMER.channeluid is also optional. So, we would also need to make PVR_TIMER.channeluid mandatory. :-(

PVR_RECORDINGs (in difference to PVR_TIMERs) have no state. That's imo the root cause of the "red dot" problem.

To determine a recording's "recording in progress" state (to be able to support the red dot, for instance), one must search for the timer that belongs to a recording . And for a 100% match you need the channel uid both in the timer and the recording, which will obviously not work 100% as long as channel uids are optional. :-/

Please note that it is not enough to only check recording's time and duration against 'now'. Only the state of the related timer currently is a safe indicator that the backend actually is recording this. That's the reason I implemented it the way it is today.

Solution would be another mandatory API change - to inroduce PVR_RECORDING.state (or an in-progress bool).

@linknetx

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

To be accurate, it prevents the red dot only in the recordings window; the dot works everywhere else.

It also doesn't work on the recent recordings on the home screen

So, it is just a very minor glitch, if at all. As I said, no essential functionality is bound to presence of channel uid.

The lack of a red dot is not really the issue, the problem arises when you attempt to delete a recording from the recordings list which is still recording. The context menu gives the delete option which does nothing and since you are not aware that it is still recording, no red dot, it's somewhat confusing. It then involves finding it in the timers list, stopping recording then back to the recordings list to delete it.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

You are right, i overlooked the other places where the problem occurs. Thanks for mentioning.

Tell the dvblogic guys to implement the one liner and we're fine for all pvr.dvblink users.

This pragmatic approach has low effort and high gain and is not a hack.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

The context menu gives the delete option which does nothing

Not sure this is another unrelated problem with the addon, as this works fine with other addons.

@linknetx

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

Not sure this another unrelated problem with the addon, as this works fine with other addons.

This is only if the recording is in progress, it won't delete it whilst still recording. For all completed recordings it deletes correctly.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

This is only if the recording is in progress, it won't delete it whilst still recording. For all completed recordings it deletes correctly.

I understood this in the first place. And as I said, this works with other pvr addons, thus I tend to say, open a ticket for pvr.dvblink (or start using another, better supported pvr addon). ;-)

@linknetx

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

If pvr.dvblink implement the ChannelUid then I will get the red dot in the recordings list and the context menu will give the 'Stop recording' option which works :-)

By better supported I take it you mean TVHeadend, as far as I understand that means a a switch to Linux on the backend, which I don't really want.

Now if TVHeadend could be ported to Windows !!!!

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

If pvr.dvblink implement the ChannelUid then I will get the red dot in the recordings list

Yes.

and the context menu will give the 'Stop recording' option which works

Most probably not. I tried to explain that most probably another fix in pvr.dvblink is needed for this. We're facing two different issues here, thus two fixes are needed.

@linknetx

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

If I implement the hack, which I suggested a few posts previously, to match the channels via Channel Name rather than Channel Uid then I get the red dot and I also get the context menu with the 'Stop Recording' option which works fine. Had it running for a few days and it works every time :-)

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

Channel names can change at any time and nobody guarantees that record's and timer's channel name have to match. They are just strings, API does not require that consistent channel names must be used across timers and recordings. Thus, sorry, cannot accept your hack.

And now, I will end this discussion from my end. Please understand that my time is limited. No bad blood involved, just not enough time.

@linknetx

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

No problem whatsoever, I understand entirely and thanks for your help, no reply needed :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.