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.

Sign up for GitHub

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

Jump to bottom

Kill global CStereoscopicsManager #13743

Merged
merged 1 commit into from
Apr 14, 2018
Merged

Kill global CStereoscopicsManager #13743

merged 1 commit into from
Apr 14, 2018

Conversation

garbear
Copy link
Member

@garbear garbear commented Apr 6, 2018

This PR kills the CStereoscopicsManager singleton, required for the login window refactor.

This replaces #13658 using the architecture added by fernet in #13722.

How Has This Been Tested?

Tested on Windows with a 3D tv. Not only does Kodi not crash, but 3D movies still work!

Testing for boot issues needed on other platforms. I think I saw that we have an army of testers in a slack channel?

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@garbear garbear added Type: Improvement non-breaking change which improves existing functionality v18 Leia labels Apr 6, 2018
@garbear garbear added this to the Leia 18.0-alpha2 milestone Apr 6, 2018
@garbear garbear mentioned this pull request Apr 6, 2018
Closed
@DaveTBlake
Copy link
Member

Tested on Windows with a 3D tv. Not only does Kodi not crash, but 3D movies still work!

Well that was the only platform I could help with, so we are good. Not sure the "testing army" have been recruited just yet, but it would be good to shape that out for quick sanity check type cross platform tests.

Given the discussion part of this PR has been done elsewhere I suggest that you set off some test builds, and post links - easier it is made the more likely people can help. Although it only touches 14 files, so it is not a mass change like the eariler global killing.

DaveTBlake
DaveTBlake approved these changes Apr 7, 2018
View reviewed changes
Copy link
Member

@DaveTBlake DaveTBlake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but see if you can find someone that uses stereoscopics to test

FernetMenta
FernetMenta reviewed Apr 7, 2018
View reviewed changes
xbmc/cores/VideoPlayer/VideoPlayer.cpp Outdated
@@ -3674,7 +3674,10 @@ bool CVideoPlayer::OpenVideoStream(CDVDStreamInfo& hint, bool reset)
hint.stills = true;

if (hint.stereo_mode.empty())
hint.stereo_mode = CStereoscopicsManager::GetInstance().DetectStereoModeByString(m_item.GetPath());
{
const CStereoscopicsManager &stereoscopicsManager = CServiceBroker::GetGUI()->GetStereoscopicsManager();

This comment was marked as spam.

Sign in to view

FernetMenta
FernetMenta reviewed Apr 7, 2018
View reviewed changes
xbmc/guilib/GUIComponent.cpp Outdated
@@ -41,12 +42,19 @@ CGUIComponent::~CGUIComponent()
void CGUIComponent::Init()
{
m_pWindowManager->Initialize();
m_stereoscopicsManager.reset(new CStereoscopicsManager(CServiceBroker::GetSettings(),
CServiceBroker::GetDataCacheCore(),

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

FernetMenta
FernetMenta reviewed Apr 7, 2018
View reviewed changes
xbmc/guilib/StereoscopicsManager.cpp Outdated
CStereoscopicsManager::CStereoscopicsManager(void)
CStereoscopicsManager::CStereoscopicsManager(CSettings &settings,
CDataCacheCore &dataCacheCore,
CGUIWindowManager &windowManager) :

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

FernetMenta
FernetMenta reviewed Apr 7, 2018
View reviewed changes
xbmc/guilib/GUIComponent.cpp Outdated
CServiceBroker::RegisterGUI(this);
}

void CGUIComponent::Deinit()
{
CServiceBroker::UnregisterGUI();

m_stereoscopicsManager.reset();

This comment was marked as spam.

Sign in to view

FernetMenta
FernetMenta reviewed Apr 7, 2018
View reviewed changes
xbmc/settings/DisplaySettings.cpp Outdated
@@ -761,24 +762,28 @@ void CDisplaySettings::SettingOptionsScreensFiller(SettingConstPtr setting, std:

void CDisplaySettings::SettingOptionsStereoscopicModesFiller(SettingConstPtr setting, std::vector< std::pair<std::string, int> > &list, int &current, void *data)
{
const CStereoscopicsManager &stereoscopicsManager = CServiceBroker::GetGUI()->GetStereoscopicsManager();

This comment was marked as spam.

Sign in to view

FernetMenta
FernetMenta reviewed Apr 7, 2018
View reviewed changes
xbmc/guilib/StereoscopicsManager.cpp Outdated
CSettings::SETTING_VIDEOSCREEN_STEREOSCOPICMODE
};

m_settings.GetSettingsManager()->RegisterCallback(this, settingSet);

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

FernetMenta
FernetMenta reviewed Apr 7, 2018
View reviewed changes
xbmc/guilib/StereoscopicsManager.h Outdated

RENDER_STEREO_MODE GetNextSupportedStereoMode(const RENDER_STEREO_MODE &currentMode, int step = 1) const;
std::string DetectStereoModeByString(const std::string &needle) const;
RENDER_STEREO_MODE GetStereoModeByUserChoice(const std::string &heading = "") const;

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Apr 8, 2018
@jenkins4kodi jenkins4kodi removed the Rebase needed PR that does not apply/merge cleanly to current base branch label Apr 8, 2018
@garbear
Copy link
Member Author

garbear commented Apr 8, 2018

@FernetMenta addressed your comments. Another round of review?

@garbear
Copy link
Member Author

garbear commented Apr 13, 2018 •
edited
Loading

Tested on Linux, Kodi starts successfully. Test on OSX in progress. That should cover most platforms.

jenkins build this please.

EDIT: OSX starts fine.

@garbear garbear merged commit 3b3e411 into xbmc:master Apr 14, 2018
@garbear garbear deleted the stereoscopicsmanager branch April 14, 2018 09:44
@popcornmix
Copy link
Member

Tried this on Pi (with a master build). When video stops the GUI remains in 3D mode.
With the PR reverted the GUI returns to 2D mode after playback stops.
Working log: http://paste.debian.net/1020646/
Not working log: http://paste.debian.net/1020647/

@da-anda
Copy link
Member

da-anda commented Apr 16, 2018

if that's the case, then StereoscopicsManager is likely not receiving the GUI_MSG_PLAYBACK_STOPPED message. Maybe it's not correctly registered as observer when initialized?

@popcornmix
Copy link
Member

I can confirm that CStereoscopicsManager::OnMessage is never called now (not for start of playback or stop).

FernetMenta
FernetMenta reviewed Apr 17, 2018
View reviewed changes
xbmc/guilib/GUIComponent.cpp
@@ -41,12 +43,18 @@ CGUIComponent::~CGUIComponent()
void CGUIComponent::Init()
{
m_pWindowManager->Initialize();
m_stereoscopicsManager->Initialize();

//! @todo This is something we need to change

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@afl1 afl1 mentioned this pull request May 11, 2018
Closed
@Memphiz Memphiz mentioned this pull request Mar 27, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FernetMenta FernetMenta FernetMenta approved these changes

@DaveTBlake DaveTBlake DaveTBlake approved these changes

Assignees
No one assigned
Labels
Component: Profiles Type: Improvement non-breaking change which improves existing functionality v18 Leia
Projects
None yet
Milestone
Leia 18.0-alpha2
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@garbear @DaveTBlake @popcornmix @da-anda @FernetMenta @jenkins4kodi