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] Rework PVR component inter-dependencies #15707

Merged
merged 17 commits into from Mar 14, 2019

Conversation

@ksooo
Copy link
Member

commented Mar 9, 2019

"Inspired" by the large amount of rare but varying deadlocks among PVR components I finally moved my ass and I defined a dependency graph for the PVR components and their subcomponents.

Before, every PVR component called into every other PVR component. Even the subcomponents called each other in any order. It was all a big mess (and I was part of building this mess, yes).

Now, we have a clear picture:

pvr-component-deps

This PR strictly implements this dependency tree. No more, no less. No functional restrictions due to the refactoring, no new features, but massive reduction in deadlock risk.

I tried to slice this PR down into reviewable pieces. Each commit removes exactly one wrong dependency.

I have tested this for quite some time now on Android and macOS. Works without any issues.

@Jalle19 I really would appreciate your feedback on the code.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2019

@MilhouseVH could you include this PR into your builds to get some feedback from runtime testers?

@ksooo ksooo force-pushed the ksooo:pvr-component-deps branch 4 times, most recently from 0c08945 to 9efb6ee Mar 9, 2019

* @brief Remove a channel from all non-system groups.
* @param channel The channel to remove.
*/
void RemoveFromAllGroups(const CPVRChannelPtr& channel);

This comment has been minimized.

Copy link
@AlwinEsch

AlwinEsch Mar 9, 2019

Member

Your changes looks good, only a question about CPVRChannelPtr and std::shared_ptr<CPVRChannel>, on some placed you have changed them to the std:: and like here you use CPVR...Ptr again, When changing this accidentally went down with switch?

This comment has been minimized.

Copy link
@ksooo

ksooo Mar 9, 2019

Author Member

Formerly I preferred the typedef (xyzPtr), if I write new code today (or change a line containing such a type), I use/change this to use std::xyz.

I will change this line accordingly. Thanks. It happened, because I just moved the whole method declaration from public to private section.

@@ -65,6 +84,10 @@ CGUIWindowPVRSearchBase::CGUIWindowPVRSearchBase(bool bRadio, int id, const std:
{
}

CGUIWindowPVRSearchBase::~CGUIWindowPVRSearchBase()

This comment has been minimized.

Copy link
@AlwinEsch

AlwinEsch Mar 9, 2019

Member

Is there anything planned for here?

This comment has been minimized.

Copy link
@ksooo

ksooo Mar 9, 2019

Author Member

If the compiler generates the dtor inline, or you define it yourself inline, then you would need to include "EpgSearchfilter.h" in the header, otherwise the compiler complains about the incomplete type when it tries to destruct the std::unique_ptr<CPVREpgSearchFilter> member which I have introduced with this PR.

Screenshot 2019-03-09 at 23 45 56

@ksooo ksooo force-pushed the ksooo:pvr-component-deps branch from 9efb6ee to e6bf163 Mar 9, 2019

@ksooo ksooo force-pushed the ksooo:pvr-component-deps branch from e6bf163 to e2fe62d Mar 10, 2019

@kib

This comment has been minimized.

Copy link
Member

commented Mar 10, 2019

@ksoo a very tiny suggestion: the accompanying picture would be a lot easier to read when you use a colored cell instead of very similar looking symbols + and x

example:
54072771-c979e700-427f-11e9-9434-f8f13606bdb7

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Mar 10, 2019

@kib yep, thanks. Much better.

@kib

This comment has been minimized.

Copy link
Member

commented Mar 10, 2019

54072771-c979e700-427f-11e9-9434-f8f13606bdb7 (1)

@pkerling
Copy link
Member

left a comment

Wow, very nice! 👍

Some stylistic suggestions from me and a few minor questions. I didn't mark every instance if a specific pattern occurred multiple times.

Show resolved Hide resolved xbmc/pvr/channels/PVRChannel.cpp Outdated
Show resolved Hide resolved xbmc/pvr/channels/PVRChannelGroupsContainer.cpp Outdated
Show resolved Hide resolved xbmc/pvr/recordings/PVRRecording.cpp Outdated
Show resolved Hide resolved xbmc/pvr/recordings/PVRRecordings.cpp Outdated
Show resolved Hide resolved xbmc/interfaces/json-rpc/PVROperations.cpp Outdated
Show resolved Hide resolved xbmc/pvr/channels/PVRChannelGroups.cpp Outdated
Show resolved Hide resolved xbmc/pvr/timers/PVRTimersPath.cpp Outdated
Show resolved Hide resolved xbmc/pvr/timers/PVRTimersPath.cpp Outdated
Show resolved Hide resolved xbmc/pvr/timers/PVRTimersPath.cpp Outdated
Show resolved Hide resolved xbmc/pvr/timers/PVRTimersPath.cpp Outdated
@ksooo

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2019

@pkerling thanks for the review. All very valid points.

@Jalle19
Copy link
Member

left a comment

Brain started hurting about a quarter way through the diff 😆

Show resolved Hide resolved xbmc/pvr/PVRGUIActions.cpp

@ksooo ksooo force-pushed the ksooo:pvr-component-deps branch from 0f67d4a to 51df381 Mar 11, 2019

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2019

Brain started hurting about a quarter way through the diff

Keep going. Your feedback is very valuable and much appreciated, as always.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2019

jenkins build this please

@ksooo ksooo force-pushed the ksooo:pvr-component-deps branch from 51df381 to 7ff9294 Mar 12, 2019

@ksooo ksooo added the v18 Leia label Mar 13, 2019

@ksooo ksooo added this to the Leia 18.2-rc1 milestone Mar 13, 2019

ksooo added some commits Feb 15, 2019

[PVR] PVR inter component dependencies: channel can hold epg member; …
…no expensive lookups needed to obtain epg for a channel.
[PVR] Fix PVR inter component dependencies: remove CFileItem and CFil…
…eList from PVR core. Those are GUI classes using global graphics mutex, which easily can lead to deadlocks when used inside PVR core.

@ksooo ksooo force-pushed the ksooo:pvr-component-deps branch from 7ff9294 to ec1ecb1 Mar 14, 2019

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2019

Heads up: If nobody objects I'm going to merge this tonight. Milhouse build testers did not find anything, all code review feedback got incorporated.

@ksooo ksooo merged commit d68bd9b into xbmc:master Mar 14, 2019

1 check passed

default You're awesome. Have a cookie
Details

@ksooo ksooo deleted the ksooo:pvr-component-deps branch Mar 14, 2019

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.