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

kill global g_graphicsContext, g_textureManager, and g_LargeTextureManager #13724

Merged
merged 5 commits into from Apr 6, 2018

Conversation

@FernetMenta
Copy link
Member

commented Apr 2, 2018

  • move GraphicsContext from guilib to windowing
  • make GraphicsContext a member of CWinSystemBase
  • kill g_textureManager
  • kill g_LargeTextureManager

There is more work to be done in this area. All remaining dependencies from windowing to guilib have to be cut.

@FernetMenta FernetMenta changed the title kill global g_graphicsContext kill global g_graphicsContext, g_textureManager, and g_LargeTextureManager Apr 2, 2018
@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2018

due to segfault on exit and failing test suit I had to kill 2 more globals

@FernetMenta FernetMenta force-pushed the FernetMenta:gfxcontext branch from aaef466 to 03e0da8 Apr 2, 2018
#include "ServiceBroker.h"
#include "URL.h"

CGUIComponent::CGUIComponent()
{
m_pWindowManager.reset(new CGUIWindowManager());
m_pTextureManager.reset(new CGUITextureManager());
m_pLargeTextureManager.reset(new CGUILargeTextureManager());

This comment has been minimized.

Copy link
@garbear

garbear Apr 2, 2018

Member

if m_pWindowManager->Initialize() doesn't depend on these then they should happen after m_pWindowManager is initialized

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Apr 3, 2018

Author Member

I dropped m_pWindowManager->Initialize() from ctor. Putting behaviour in ctor is not a good style anyway.


m_pWindowManager.reset();
m_pTextureManager.reset();
m_pLargeTextureManager.reset();

This comment has been minimized.

Copy link
@garbear

garbear Apr 2, 2018

Member

Can you deinitialize these in reverse order?

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Apr 2, 2018

Author Member

if this matters, something is really wrong and we should fix the root cause.

This comment has been minimized.

Copy link
@garbear

garbear Apr 2, 2018

Member

If we reverse them, it will show us if anything is wrong so we can fix it.

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Apr 2, 2018

Author Member

deinit is reverse order is happy path. doing it differently challenges the system more.

This comment has been minimized.

Copy link
@garbear

garbear Apr 2, 2018

Member

huh? isn't fixing the system worth the effort?

This comment has been minimized.

Copy link
@garbear

garbear Apr 3, 2018

Member

I don't understand, isn't RAII-inspired order, where resources are unacquired in the opposite order, better than what is here now?

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Apr 3, 2018

Author Member

Right, uninit in reverse order is better in general. But the order should not matter here. If it matters I want to know and fix the actual issue.

This comment has been minimized.

Copy link
@garbear

garbear Apr 3, 2018

Member

It matters from a maintainability standpoint. It's easier for a programmer to verify the order is reversed properly than try to reason about the intention of breaking a convention.

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Apr 3, 2018

Author Member

Ok, then I will remove those lines completely. The only reason they are pointers is to avoid includes in header file

This comment has been minimized.

Copy link
@garbear

garbear Apr 3, 2018

Member

Nonexistent code is easiest to maintain :)

@FernetMenta FernetMenta force-pushed the FernetMenta:gfxcontext branch from df66d14 to a8b9b6a Apr 3, 2018
@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2018

jenkins build this please

@FernetMenta FernetMenta force-pushed the FernetMenta:gfxcontext branch 3 times, most recently from c41b8f7 to 033bf12 Apr 3, 2018
@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2018

jenkins build this please

@FernetMenta FernetMenta force-pushed the FernetMenta:gfxcontext branch from 033bf12 to 8cb650b Apr 3, 2018
@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2018

jenkins build this please

@@ -1089,7 +1089,7 @@ float CGraphicContext::GetFPS() const
{
if (m_Resolution != RES_INVALID)
{
RESOLUTION_INFO info = g_graphicsContext.GetResInfo();
RESOLUTION_INFO info = CServiceBroker::GetWinSystem().GetGfxContext().GetResInfo();

This comment has been minimized.

Copy link
@afedchin

afedchin Apr 3, 2018

Member

looks like it call to itself. I think it may be simplified by calling GetResInfo. isn't?

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Apr 3, 2018

Author Member

right, thanks.

@FernetMenta FernetMenta force-pushed the FernetMenta:gfxcontext branch from 8cb650b to ef9fae4 Apr 3, 2018
@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2018

jenkins build this please

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2018

@DaveTBlake why don't you try your testbuild proposal on this one.

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Apr 3, 2018

Sure @FernetMenta ping me once it is on the mirrors and I will happily test it on Windows

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Apr 3, 2018

@FernetMenta I have tried the gfxContext build that is on the mirrors http://mirrors.kodi.tv/test-builds/windows/win64/KodiSetup-20180403-ef9fae4c-gfxcontext-x64.exe. It does not work, crashing before showing the splash screen.

@FernetMenta FernetMenta force-pushed the FernetMenta:gfxcontext branch from ef9fae4 to 29a95c7 Apr 4, 2018
@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2018

This branch did not include afedchin's fix. I rebased. Please try again.

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Apr 4, 2018

I think jenkins failed to build the wrong version. I have queued a win64 test build for it

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Apr 4, 2018

@FernetMenta happy to confirm that win64 build works in simple testing - no crashes on entry or exit.

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Apr 4, 2018

jenkins build this please

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Apr 4, 2018

Since this seems to cure the crash on exit that master currently has in Win64 it would be nice to merge this @FernetMenta

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2018

your button

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Apr 6, 2018

@FernetMenta could you rebase this please

@afedchin

This comment has been minimized.

Copy link
Member

commented Apr 6, 2018

@DaveTBlake woulkd you like to merge this? it's possible without rebase.

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Apr 6, 2018

Merge without rebase is a new concept for me @afedchin , what happens to the conflicts?

@a1rwulf

This comment has been minimized.

Copy link
Member

commented Apr 6, 2018

@DaveTBlake If you "git merge" you'll get a conflict like you do when "git rebase". I've never tried to use this github web resolver and I'm not sure we want it, although the conflicts seem to be just in the header section.
The conflict resolution afaik is then part of the merge commit.

@Rechi

This comment has been minimized.

Copy link
Member

commented Apr 6, 2018

Don't resolve the conflicts in the merge commit, just wait till this PR is rebased.

@FernetMenta FernetMenta force-pushed the FernetMenta:gfxcontext branch from 29a95c7 to c39b350 Apr 6, 2018
@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2018

rebased

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Apr 6, 2018

Retested on win64, seems fine, no crashing on start or exit.
Will let jenkins complete building (again?) before merge

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Apr 6, 2018

Odd, the win64 build failed yet was fine when jenkins created the test build. One of those things?

@DaveTBlake DaveTBlake merged commit 9dd23cb into xbmc:master Apr 6, 2018
1 check failed
1 check failed
default Sorry, building this PR failed. Please check the logs for the errors.
Details
@Rechi Rechi added this to the Leia 18.0-alpha2 milestone Apr 6, 2018
This was referenced Jul 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.