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 trac17374: Settings deinitialized too early on app exit. #11849

Merged
merged 1 commit into from Mar 15, 2017

Conversation

@ksooo
Copy link
Member

commented Mar 13, 2017

This fixes http://trac.kodi.tv/ticket/17374

Settings are cleared too early on app exit => https://github.com/xbmc/xbmc/blob/master/xbmc/pvr/PVRManager.cpp#L531 does not get the right settings values on app shutdown => wake up command gets not executed.

Bug was introduced by a fix I did some time ago, to only call wake up command on app exit, not on every pvr manager restart (which can happen quite some time during app lifetime).

@Montellese @FernetMenta I moved deinit of settings in CApplication::Cleanup() to a later point in time, namely to CServiceManager::Deinit(), after PVR mananger shutdown. Is this okay? Runtime test went fine.

Copy link
Member

left a comment

Not sure if this is safe. Kodi registers a lot of callbacks into settings and as a result settings has to be cleared down as long as those objects exists.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Mar 14, 2017

Those kind of issues occur because settings are used as global variables. Have a look at AE:
https://github.com/xbmc/xbmc/blob/master/xbmc/cores/AudioEngine/Engines/ActiveAE/ActiveAE.cpp#L2544

It loads all settings into a member every time OnChange is fired by settings.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2017

Those kind of issues occur because settings are used as global variables.

These are actually very valid points. I will try to find a small pragmatic fix for Krypton and do it right (settings changes callback) for Leia.

@ksooo ksooo force-pushed the ksooo:trac17374 branch from b257618 to d151731 Mar 14, 2017
@ksooo

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2017

@FernetMenta I now implemented the settings listener approach you suggested - for now for the settings used during app exit (and causing the bug due to this). Good to go now? I will take care of the other settings used by PVR in another PR.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Mar 15, 2017

@ksooo for v17 is may be ok but for master you might want to make it more robust:

  • PVRManager is global and g_PVRManager.SetPowerManagementEnabled can be called from everywhere by any other component. By doing this sync to settings is lost. It is not clear that those are only supposed to be called on settings change
  • thread safety? maybe not an issue for the wakeup related settings but should be considered when looking at the rest.
@ksooo

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2017

Fair enough. I will change the implementation to address your comments.

@ksooo ksooo force-pushed the ksooo:trac17374 branch from d151731 to 83958c1 Mar 15, 2017
@ksooo

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2017

@FernetMenta I addressed your latest comments re thread safety and sync. Good to go now?

const std::string &settingId = setting->GetId();
if (settingId == CSettings::SETTING_EPG_DAYSTODISPLAY)
{
m_addons->SetEPGTimeFrame(static_cast<const CSettingInt*>(setting)->GetValue());

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Mar 15, 2017

Member

@ksooo don't get me wrong, I don't want to be picky and I don't block this in any way. Just sharing ideas. This has been like this before, so no regression.
Hijacking the cb thread (in most cases gui thread) having it call into addons is probbly not the safest way to handle callbacks. AE for example just sets an event on the callback and the AE thread does the actual work:
https://github.com/xbmc/xbmc/blob/master/xbmc/cores/AudioEngine/Engines/ActiveAE/ActiveAE.cpp#L2616

PVRManager has its own thread as well and can do this work by itself.

Again, just want to share ideas

This comment has been minimized.

Copy link
@ksooo

ksooo Mar 15, 2017

Author Member

@ksooo don't get me wrong, I don't want to be picky and I don't block this in any way. Just sharing ideas. This has been like this before, so no regression.
Hijacking the cb thread (in most cases gui thread) having it call into addons is probbly not the safest way to handle callbacks.

All fine. Thanks for the feedback. As you said, no regression here. I will address this in another PR. I guess there are even more places in PVR using this anti pattern. ;-)

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2017

jenkins build this please

@ksooo ksooo merged commit 77f3bff into xbmc:master Mar 15, 2017
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
jenkins4kodi You did a great job. Have a cookie.
Details
@ksooo ksooo deleted the ksooo:trac17374 branch Mar 15, 2017
@Rechi Rechi added this to the L 18.0-alpha1 milestone Mar 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.