Kodi Community Forum

Full Version: separate graphics rendering from main thread
You're currently viewing a stripped down version of our content. View the full version with proper formatting.
Pages: 1 2 3
how do you plan to avoid singletons? Aren't they mandatory for self contained components? It's ofc not good to make use of singletons in a way globals have been used and create loads of interdependencies. I'm really interested in the technical aproach you have in mind to learn from it.

F.e. I created the StereoScopicsManager as singelton to have one central class to interact with (a separate static utility class for some functions might have been nicer I have to admit). Ideally it wouldn't have been needed to make it a singleton, but the components involved didn't have enough hooks/signals/etc to implement it completely transparently to any other component (I tried to hook into as many messages as possible). What would I have to do to get rid of the singleton approach here? In a PHP project I would have implemented some signal/slot system and made use of it all over the place, but I'm unable to do this in C++, especially with all the threading stuff.
(2015-10-21, 20:03)da-anda Wrote: [ -> ]how do you plan to avoid singletons? Aren't they mandatory for self contained components?

The way singletons are used in this project is the same as globals: include the header and you are ready to use it from anywhere in the app. That is evil. A good example is settings. Every component can request a setting from every other.
I have changed this for AE (not forced because it is still possible). Only AE does access settings related to AE. If other components like player want to know something about AE, they have to use the interface to AE, not settings.

(2015-10-21, 20:03)da-anda Wrote: [ -> ]Aren't they mandatory for self contained components?

Absolutely not. A singleton means that there can only be one. A self contained component or object can exist multiple times. We could even break settings into peaces. Why do we need a single file for the entire app?

(2015-10-21, 20:03)da-anda Wrote: [ -> ]F.e. I created the StereoScopicsManager as singelton to have one central class to interact with

That needs to be reverted. I intend to use VideoPlayer as the infrastructure for transcoding. If a system has enough power it can run multiple instances of VideoPlayer for serving multiple clients. A singleton would not permit this.[/quote]
(2015-10-21, 21:42)FernetMenta Wrote: [ -> ]
(2015-10-21, 20:03)da-anda Wrote: [ -> ]F.e. I created the StereoScopicsManager as singelton to have one central class to interact with

That needs to be reverted. I intend to use VideoPlayer as the infrastructure for transcoding. If a system has enough power it can run multiple instances of VideoPlayer for serving multiple clients. A singleton would not permit this.

Sorry, not reverted but transformed into something more flexible.
(2015-10-21, 21:42)FernetMenta Wrote: [ -> ]
(2015-10-21, 20:03)da-anda Wrote: [ -> ]how do you plan to avoid singletons? Aren't they mandatory for self contained components?

The way singletons are used in this project is the same as globals: include the header and you are ready to use it from anywhere in the app. That is evil. A good example is settings. Every component can request a setting from every other.
I have changed this for AE (not forced because it is still possible). Only AE does access settings related to AE. If other components like player want to know something about AE, they have to use the interface to AE, not settings.

Ah ok, I see what you mean now. Yes, that's bad. Components should stay within their "domain" and not directly mess with the internal logic of others. So you basically want to apply domain driven design pattern along with other design principels. What about directly accessing and manipulating member variables of "foreign" objects? IMO this also has to die. All class variables should be private and be interacted with only via setters and getters. How should a component know if changing a variable requires additional logic in the other component? Do you agree with this?
(2015-10-22, 09:28)da-anda Wrote: [ -> ]What about directly accessing and manipulating member variables of "foreign" objects? IMO this also has to die. All class variables should be private and be interacted with only via setters and getters. How should a component know if changing a variable requires additional logic in the other component? Do you agree with this?

Setters and getters are already at a very low level. Lets distinguish between a struct like class that's primary purpose is holding data and some object that contributes to behaviour. The first are often encapsulated by the latter and I call them extended state variables in this context.

Quote:How should a component know if changing a variable requires additional logic in the other component?

Encapsulation is key here. The outside world does not know anything about variables of an encapsulated object. And also important that the object should not know anything about the outside other than interfaces. Do you remember my presentation about AE? That showed this concept.
So the answer is: it does not need to know.
(2015-10-21, 21:42)FernetMenta Wrote: [ -> ]I have changed this for AE (not forced because it is still possible). Only AE does access settings related to AE. If other components like player want to know something about AE, they have to use the interface to AE, not settings.

That doesn't change the fact that you need some place which knows where settings are stored and how to load them. You don't want every component to have to know that the settings are stored in a file called guisettings.xml and that the file is located at location X (which depends on other stuff like what profile you are in, if you're in portable mode etc).
Changing the logic that only component X can access settings belonging to component X and every other component has to go through component X's implementation doesn't change the fact that component X has to get the actual values from somewhere and needs to be able to write them somehow and all components should do it the same way and should write into a single file (obviously this could be changed as well and it would already be supported by the existing settings library).
So as long as there is one settings.xml describing all settings of all components and one guisettings.xml that contains all setting values having a single place responsible for handling the settings and all their values doesn't seem wrong to me. The fact that CSettings includes a lot of other components to access their option fillers etc is bad but splitting this up would have required a complete refactor of our startup behaviour which was out of scope of the settings refactoring.

Without a proper dependency injection framework it can get very tricky to avoid singletons that are used in multiple places.
(2015-10-22, 11:58)Montellese Wrote: [ -> ]
(2015-10-21, 21:42)FernetMenta Wrote: [ -> ]I have changed this for AE (not forced because it is still possible). Only AE does access settings related to AE. If other components like player want to know something about AE, they have to use the interface to AE, not settings.

That doesn't change the fact that you need some place which knows where settings are stored and how to load them. You don't want every component to have to know that the settings are stored in a file called guisettings.xml and that the file is located at location X (which depends on other stuff like what profile you are in, if you're in portable mode etc).
Changing the logic that only component X can access settings belonging to component X and every other component has to go through component X's implementation doesn't change the fact that component X has to get the actual values from somewhere and needs to be able to write them somehow and all components should do it the same way and should write into a single file (obviously this could be changed as well and it would already be supported by the existing settings library).
So as long as there is one settings.xml describing all settings of all components and one guisettings.xml that contains all setting values having a single place responsible for handling the settings and all their values doesn't seem wrong to me. The fact that CSettings includes a lot of other components to access their option fillers etc is bad but splitting this up would have required a complete refactor of our startup behaviour which was out of scope of the settings refactoring.

Without a proper dependency injection framework it can get very tricky to avoid singletons that are used in multiple places.

What is the advantage of having a single file "guisettings"? I often thought that it should be split into more manageable smaller files.
Sure I don't object to that. It's just how it was before I refactored the settings system and I wanted to provide the same functionality afterwards to be able to easier test migration etc. At this point we could considere splitting settings up per component and let every component instantiate their own CSettingsManager instance to read/write/manage their settings.
well sure, the key is encapsulation. There is nothing wrong with public variables as long as the object is interacted with via a defined interface, but unfortunately we don't do this for everything.

Just have a look here: https://github.com/xbmc/xbmc/blob/master....cpp#L1260
The video-infotag should not be altered just for the sake of a certain render/output setting - that's plain wrong. The presentation layer has to take care of this without manipulating persisted values). Once this usually persisted value is changed it's not to be trusted anymore in other code paths and things can go wrong.

And here, even that there is a setter, you can't rely on the value of strPath as you never know if it contains the VFS path or the final file path.
https://github.com/xbmc/xbmc/blob/master...v.cpp#L555
I ran into issues with this when I tried to replace the "choose fanart", "refresh" etc buttons in VideoInfoDialog with a "manage..." button that triggers the according context menu. Context-Menu logic expects the VFS path for cFileItem->getPath() while a manipulated clone of the FileItem is passed on to the VideoInfoDialog. This is just crazy, as the VideoInfoDialog has no way to know it's a library item or not. And the entire library code is stuffed with "if (item->HasVideoInfoTag()) { return item->getVideoInfoTag()->getPath(); } else { item->getPath(); }". WTF.

It's IMO a absolute NO-GO to alter any value of a DB related object for any other purpose than persisting the new information. So if the value should not end up in DB, don't alter it just for display reasons. Rather implement a new method or utility class.
(2015-10-22, 12:19)da-anda Wrote: [ -> ]well sure, the key is encapsulation. There is nothing wrong with public variables as long as the object is interacted with via a defined interface, but unfortunately we don't do this for everything.

Just have a look here: https://github.com/xbmc/xbmc/blob/master....cpp#L1260
The video-infotag should not be altered just for the sake of a certain render/output setting - that's plain wrong. The presentation layer has to take care of this without manipulating persisted values). Once this usually persisted value is changed it's not to be trusted anymore in other code paths and things can go wrong.

And here, even that there is a setter, you can't rely on the value of strPath as you never know if it contains the VFS path or the final file path.
https://github.com/xbmc/xbmc/blob/master...v.cpp#L555
I ran into issues with this when I tried to replace the "choose fanart", "refresh" etc buttons in VideoInfoDialog with a "manage..." button that triggers the according context menu. Context-Menu logic expects the VFS path for cFileItem->getPath() while a manipulated clone of the FileItem is passed on to the VideoInfoDialog. This is just crazy, as the VideoInfoDialog has no way to know it's a library item or not. And the entire library code is stuffed with "if (item->HasVideoInfoTag()) { return item->getVideoInfoTag()->getPath(); } else { item->getPath(); }". WTF.

It's IMO a absolute NO-GO to alter any value of a DB related object for any other purpose than persisting the new information. So if the value should not end up in DB, don't alter it just for display reasons. Rather implement a new method or utility class.


I agree. CFileItem is a perfect example how other OO principles like hierarchy and polymorphism can be violated. When I look at the ridiculous list of IsXYZ methods https://github.com/xbmc/xbmc/blob/master...tem.h#L178 I don't know whether I should cry of laugh Smile
Sure this was not the original author's intent. Others just grew it by adding more and more. When digging into primitive hacks like that: https://github.com/xbmc/xbmc/commit/ba7e...6bb57728f6 it makes me want to spew.
(2015-10-22, 13:01)FernetMenta Wrote: [ -> ]CFileItem is a perfect example how other OO principles like hierarchy and polymorphism can be violated. When I look at the ridiculous list of IsXYZ methods https://github.com/xbmc/xbmc/blob/master...tem.h#L178 I don't know whether I should cry of laugh Smile
agreed. How would you like to get rid of all these "isFoo" methods? In PHP I'd probably check an object against a certain interface implementation if I want to interact with it, like "if ($fileItem instanceof KODI\VFS\Nodes\VideoNodeInterface)" or alike - not sure how this should be done with C++ and missing reflection etc. Would you implement a "getType()" method and compare it to a ENUM like:
Code:
CMetaData metaData;
if (pFileItem->getType() == VFS_NODE_VIDEONODE)
  metaData = pFileItem->getVideoInfoTag();
else if (pFileItem->getType() == VFS_NODE_MUSICNODE)
  metaData = pFileItem->getMusicInfoTag();
else if (pFileItem->getType() == VFS_NODE_PVRNODE)
  metaData = pFileItem->getPVRInfoTag();

Just as an example. For the particular dummy case of the example I'd probably do something entirely different and have all InfoTags share a certain metaDataInterface and simply do a "pFileItem->getMetaData()" and have the according fileItems implement a certain metatag related interface.

The "isFoo" part could then be moved to a utility class if we'd like to keep the convenience of helper methods.
I've talked about the VFS issues with spiff a few days ago because he has to deal with some of that in his VFS binary addons. You can't have a binary addon that implements a specific protocol and then have these hardcoded IsFoo() methods. IMO the main problem lies in the fact that some part of our core code has to know that an item is of type X. If the interfaces and implementations would be better core code shouldn't have to care about any of that.

But we're getting a bit off topic in this thread. This discussion should be done in a separate thread.
(2015-10-22, 13:31)Montellese Wrote: [ -> ]IMO the main problem lies in the fact that some part of our core code has to know that an item is of type X.

Agree, that is another principle we should respect: avoid runtime type information (RTTI).
The approach I like most is this:

1) Main thread owns the main window, but just handles events, does nothing else.
2) GPU rendering runs in its own thread. One thread should suffice, and would it make possible to not use any synchronization for GPU resources, as long as the GPU thread is the only one ever accessing the GPU resources.
3) All other work should be split into as many threads as reasonable. Proper synchronization necessary, of course.

Threads 1) and 2) should not ever wait for any other thread, so both event handling and drawing stays active at all times, to give the user the best possible GUI experience. If a crucial 3) thread gets stuck, the GUI can show a warning, maybe with an option to restart Kodi.
wow, that's look pretty good.
Pages: 1 2 3