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

Cleanup some of the PVR code #3684

Merged
merged 8 commits into from Dec 4, 2013
Merged

Conversation

Jalle19
Copy link
Member

@Jalle19 Jalle19 commented Nov 19, 2013

I've started looking at the PVR subsystem to get a grip on how it works so I can eventually fix some bugs in the future. The changes here were done as a part of that process (and as Linux gitfu training since I've been spoiled by SourceTree on Windows).

The only possible functionality change is the addition of a missing lock in IsConnectedClient(). All other methods that loop on the client map uses a lock so I figured it should be here too.

@ghost ghost assigned opdenkamp Nov 19, 2013
@opdenkamp
Copy link
Member

thanks, couple of comments, and good to go after they are addressed

@Jalle19
Copy link
Member Author

Jalle19 commented Nov 27, 2013

Made QueueJob() private now, care to counter-comment on the other comments?

@Jalle19
Copy link
Member Author

Jalle19 commented Nov 27, 2013

To summarise the comments that were magically lost in the push:

  • IsConnectedClients is called in one place which calls lock() just before, is that a problem?
  • HasEnabledClients() reuses the code from EnabledClientAmount() that's why I changed it. I don't really see why the code shouldn't be reused here.

@Jalle19
Copy link
Member Author

Jalle19 commented Dec 2, 2013

@opdenkamp any update on this?

@@ -95,10 +95,12 @@ bool CPVRClients::IsConnectedClient(int iClientId) const
return GetConnectedClient(iClientId, client);
}

bool CPVRClients::IsConnectedClient(const AddonPtr addon)
bool CPVRClients::IsConnectedClient(const AddonPtr client)

This comment was marked as spam.

This comment was marked as spam.

@opdenkamp
Copy link
Member

f3b283c and 969c337 should be dropped imo

@Jalle19
Copy link
Member Author

Jalle19 commented Dec 3, 2013

Rebased, squashed the last two commits, dropped the commits you wanted.

@Jalle19
Copy link
Member Author

Jalle19 commented Dec 3, 2013

I changed HasTimerSupport() so it doesn't need to grab a lock on its own, should fix any potential deadlocks caused by the added lock in the first commit.

@opdenkamp
Copy link
Member

thanks

opdenkamp pushed a commit that referenced this pull request Dec 4, 2013
@opdenkamp opdenkamp merged commit 1195dee into xbmc:master Dec 4, 2013
@ryangribble
Copy link
Contributor

Hey guys, After getting latest source, i found that the EPG wasnt being loaded for any channels anymore. All the initial channels etc load, but then the scheduled EPG loads just dont happen. Attempting to manually refresh EPG on a channel also fails. When i checkout the commit before this PR things work OK and after taking this PR they dont.

I think it's the QueueJob() function. If you look at the old EPG queue item it never checked for !IsStarted() || JobPending, it only checked JobPending. But the new QueueJob always checks IsStarted as well

@opdenkamp
Copy link
Member

thanks for reporting back, but please report issues like this on the forum

@Jalle19
Copy link
Member Author

Jalle19 commented Dec 5, 2013

Edit: Github is screwing up the diff views for individual commits or something, anyway I'll try to fix this when I get home.

@ryangribble
Copy link
Contributor

I did post this on the forum after Opdenkamp's comment: http://forum.xbmc.org/showthread.php?tid=179678

I fixed it locally by adding another bIgnoreStarted parameter, similar to how you already have bIgnorePending and it's working fine now. I didnt submit a PR myself yet because i dont have a clone for XBMC tree as I mainly work in the pvr addons area, so if you are on it, I will leave it to you :)

Thanks

opdenkamp pushed a commit that referenced this pull request Dec 11, 2013
[pvr] fix broken epg create after (#3684)
@Jalle19 Jalle19 deleted the pvr-small-fixes branch March 5, 2014 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants