[drok]

Greetings all,

The GUI of XBMC is presently quite a heavy load on the CPU. I intend to fix this, by re-designing the interaction between renderer and back-end data sources, using an event-driven infrastructure. It's a large undertaking, and I'd like to avoid duplication of effort, request feedback and help with some items.

Current State

I have written the underlying event-driven infrastructure already, and plan to get it included into debian, as it is generic and can be used by many other projects, as a library. It includes extensive unit tests, using CPPUNIT framework. It also includes a C++ parser/lexer marrying the event-driven code with an interpreted grammar. I have now started to integrate it into xbmc. This infra is effectively a number of C++ templates around two main classes: Notifying<type> and Notified<type>. These two are smart wrappers around basic types like int, bool, CStdString, std::list, std:et, and represent data sources and data sinks respectively. The "smart" part is that they handle dependencies between sources and sinks transparently, along with managing the lifecycles of both sources and sinks (ie, when a source/sink is constructed/destroyed, the dependents are notified appropriately).

There are examples of use in the library code itself, which will be called notified (ie, libnotified).

I have started implementing the plan outlined below, and I intend on publishing the sources once I have them cleaned up enough to compile again. To this end I have set up a launchpad PPA for libnotified and hope this can be kept as an external library, and branched xbmc to http://github.com/drok/xbmc for this development.
I expect a pull request (xbmc/notified running usably) will be ready in 2-months time or so.

Plan

Underlying datasources will change minimally to replace return by value to return by (smart) reference. Basically API that currently returns bool or int will return Notifying<bool>& or Notifying<int>& respectively. Member variables of data sources (ie, g_application.m_pPlayer->m_dBitrate, g_settings.m_IsPlayingVideo, etc) will change from their basic types to Notifying<basic type>, but the way they are used by their objects stays the same - minimal source changes.

The GUI elements will change more:

The CGUIControl::UpdateVisibility() api is gone. A new api function OnNotify() is added, which will be called when any dependent data of Notified<type> member variables is changed (animation data is one of the dependencies, of course). In this call, the Control can queue itself for rendering if it is presently visible and contents have changed.

The Renderer will only draw controls listed on the render queue, instead of polling the entire list of controls in the skin.

Functions like g_infoManager.GetBool(), etc no longer be called at render time, but only at skin load time or other significant events, typically due to user input. That call will return a Notifying<bool> which will be assigned to a member Notified<bool>. This assignment creates the relationship between the datasources and the CGUIControl object. The data may change, (and CGUIControl OnNotify()'ed), but the dependency relationship does not.

The GUIInfoManager.cpp will be replaced by 3 others, a Parser grammar, Lexer vocabulary and a new CPP that marries the Parser/Lexer. This will allow much easier maintenance of available keywords, and add a freebie feature - logical and arithmetic expressions (more complex expressions that supported currently, such as ((a || b) && (!c || d) && (player.chapter > 2)), or ( (a + b) * c / d > 4 ? foo : bar ) ).
For instance, a call like GUInfoManager::GetInt( "player.chapter / player.chaptercount > 0.5 ? foo : bar )" ) will return a Notifying<int>& reference, which will represent this arithmetic relationship between the dependent source variables chapter, chaptercount, foo and bar. If this were the Label property of a GUIControl, the GUIControl would be OnNotify()'ed whenever the result of that expression changes.

The Lexer Vocabulary will look similar to:

Code:

<player>{
hasvideo      {  V(bool, g_application.IsPlayingVideo()); }
recording     {  V(bool, g_application.m_pPlayer->IsRecording()); }
progress      {  V(int, g_application.GetPercentage()); }
volume        {
    InternalVariable<CStdString> v(new CNConvert_OneOp("%2.1f dB",
           g_settings.m_nVolume * g_settings.m_dynamicCompression));
                V(CStdString,   v);
... ( more commands )
}

This snippet supports the player.hasvideo, player.recording, player.progress, and player.volume skin variables:

I will need help with:

• Building on Windows/OSX/Others
  • Flex/Bison code needs to be built on these platforms (specifically ylwrap is not available due to not using automake)
    
  • The Posix threads need to be resolved on windows (perhaps with the pthreads-win32 library)
• Code / Architecture reviews
  
• Writing Unit Tests
  
• Lots and lots of testing (including valgrind leak and race condition verification).
  

It's a big change to be sure. I'd appreciate feedback.

[pieh]

Hi, thanks for bringing this discussion up. Event driven infomanager is something we all want (and need) to have.

From this code snippet it seems like with Your idea still need to check each frame for values of each info to determine weather they changed (and going further to notify controls that depend on given info). We already do check if any property of control changed since last render to know if we need re-render given control (speaking very briefly, check "dirty regions" keywords for more info).

I'm also not sure if we really need to employ additional dependency to get job done. What do You actually need from that lib? We really need to avoid adding deps if possible - it surely doesn't help portability.

Now, I must say I didn't really do research in this area but I was seeing it this way (just a concept that sits in my head waiting on its turn):

• determine which info we know when they change (f.e. we will know when "videoplayer.title" will change, but not when "system.cpufrequency" - we still need to probe that one)
  
• create or expand+reuse json AnnouncementManager
  
• create dependency relations between concrete info and CGUIInfoLabel, InfoBool instances
  
• announce events that will invalidate stored info (f.e. starting/stoping/changing currently played item will invalidate videoplayer/musiplayer properties).
  
• update linked CGUIInfoLabel, InfoBool with new values

Don't get me wrong, I'm not against Your idea (it looks like You already spend some time on research so You propably see bigger picture than me). What I (and propably other folks) would like to know is what are advantages of Your approach (not counting event-driven model and extended syntax) that would "justify" additional dependency and rather big change in code.

PS. One of things I learnt here (or still am learning) is that overengineering is not good way to get things done

[jfcarroll]

A few things:

1) Can you switch to UnitTest++ (or provide a good case for using CPPUNIT) as there are already tests in the codebase that use it (see xbmc/threads/test).

2) We are having discussions about restructuring the code a bit post-eden (including the possibility of using a dependency injection design pattern). The event loop, which is now a million lines long (give or take ) and full of specific actions, needs to be written in terms of abstractions (yet to be defined). It sounds like you're going that way - but I haven't looked at the code yet. I'd like to make sure your implementation fits this direction (as long as all the other devs are on board - of course ... I'm a junior guy around here ).

3) Can you point me directly to some examples of how your framework works (maybe the tests) as I'm really short on time this week but I'd like to see what your talking about.

[drok]

@pieh, there is no polling whatsoever in the code snipped I showed above.

Dependency on yet another library is up to the xbmc development team of course. The library is only composed of 4 header files, which define the needed class templates, and a function library containing 2 or 3 functions that allow propagation of user events along the same data dependency paths as the 'assignment' and 'disconnect/destruction' events generated and handled by the templates.

My recommendation is for creating a dependency as opposed to copying the 4 header files. The package I release contains a comprehensive test suite, which is run automatically when the package is created. If any test fails, the package won't build. So, if you depend on the package, you can be assured that it passes all the tests. If you copy header files into the xbmc project, you're exposed to the risk of bugs introduced by modifications to these header files.

I will be publishing the source in tgz snapshot format, to make use of the test-suite interlock I mentioned above (ie, every tgz passes all known test suite tests). I won't be publishing a git version, as I feel the code released that way cannot be guaranteed to be tested equally well).

Additionally, there are a few further performance/resource improvements that I'm planning to the library, and the easiest way to pick them up is to add a dependency to "notified == 1.*". When the API changes, I will increment the major revision number.

@jfcarroll: the xbmc code base need not test the Notified library. The implementation of the event-driven code can be tested with any suite. None is built into the libnotified-dev package I suggested as dependency. Further, there is a comparison between cppunit and unittest++ at unittest++ vs cppunit. I myself picked cppunit because of it's an xUnit framework, while unittest++ is not, and there is a standard xUnit framework for Python, PyUnit. Since they're both xUnit, integrating the Python and C++ and C test suites should be straightforward. The notified library will be released at the PPA, please see the Notified-Tests.cpp test suite contained in the source package for examples. The PPA contains an August version of the lib currently, but it's enough to get an understanding of the use scenarios, even if some of the features I've added since are not there yet.

Allow me to more clearly show how the assignment event gets propagated, this time with the Parser/Lexer layer stripped off. The code in the "Notifying" column below comes from the Notified test suite, and I've commented verbosely what is happening at every line.

Code:

Trivial C++ implement| event-driven implementation| Commentary
---------------------+----------------------------+-----------------------------------------------------------
int a = 1;           |Notifying<int> a(1);        | declare 'a' integer, init with 1                            
int b = 2;           |Notifying<int> b(2);        | declare 'b' integer, init with 2                            
int c = a + b;       |Notifying<int> c = a + b;   | declare 'c' integer and:                                    
                     |                            |                                                             
                     |                            |     in the plain version:                                    
                     |                            |     init the value of c to sum of a+b                        
                     |                            |                                                             
                     |                            |     in the Notifying version:                                
                     |                            |     set c to depend on a and b, summed                      
                     |                            |                                                             
int plain_c;         |Notified<int> notified_c(r);| declare 'c' integer. This would be the a member variable    
                     |                            |      of a GUIControl.                                        
                     |                            |     in Notifying version,                                    
                     |                            |     the object 'r' will be OnNotify()'ed                    
                     |                            |     whenever the value of c changes,                        
                     |                            |     or when any of its the dependent vars                    
                     |                            |     is destroyed due to going out of scope                  
                     |                            |                                                              
plain_c = c;         |notified_c = c;             | assign to the GUIcontrol variable:                          
                     |                            |     in the plain version, the value of c calculated above.  
// true:             |// true:                    |     in the Notifying version, a reference to c.              
ASSERT(plain_c == 3);|ASSERT(notified_c == 3);    |     notified_c will be poked with OnNotify whenever          
                     |                            |     c changes, which is poked and re-evaluated when          
                     |                            |     either a or b change.                                    
                     |                            |                                                              
                     |                            | Change the value of 'a' *AND* :                              
a = 9;               |a = 9;                      |     The Notify version uses a smart assignment operator      
                     |                            |     to notify a's dependents (ie, 'c'), of a need            
plain_c = c = a + b; |                            |     to reevaluate. 'c' handles that event by                
                     |                            |     recomputing a+b, storing the result in c, and            
                     |                            |     notify its upstream dependents (notified_c).            
                     |                            |     'notified_c' is poked, and it will call                  
                     |                            |     the OnNotify() method of the 'r' object                  
                     |                            |                                                              
                     |                            |     The plain version has to poll the 'a' and 'b'            
                     |                            |     variabled and add them. this would happen at            
                     |                            |     Render time.                                            
                     |                            |     The Notifying version doens't need to poll,              
                     |                            |     as the event was fully handled at "a=9" above.          
                     |                            |     At that time, the GUIControl would mark itself          
                     |                            |     'dirty' and queue itself to the render queue            
                     |                            |     if it determined that the new value of c (11)            
                     |                            |     needs to be rendered.                                    
                     |                            |                                                              
// true:             |// true:                    | Access GUIControl member:                                    
ASSERT(plain_c == 11)|ASSERT(notified_c == 11);   |     In the Notifying version,                                
                     |                            |     this actually dereferences a pointer to c                
                     |                            |     and returns the current value of c                      
                     |                            |     which was updated above at "a=9",                        
                     |                            |     due to an update event automatically                    
                     |                            |     fired by the assignment operator
                     |                            |     function Notifying<> operator=(Notifying&)

[davilla]

any build time tests will fail when cross-compiling

[drok]

any build time tests will fail when cross-compiling

That's a good point, and one I hadn't considered.

Fortunately launchpad does not cross compile. My package got built for both i386 and amd64 with the tests enabled; most likely on hosts of those respective architectures.

Fortunately the debian build infrastructure (debhelper, which is used in the build recipe for debian packages) is cross-compiling aware and skips testing when cross compiling.

But what is a good way to test when cross compiling? Should a target-built testsuite be included in the package, to be run at install time, and refuse install if it fails ? I suppose a target filesystem could be cross built, and even install-time scripts would not run on the target.

It would be nice to not install broken code in the cross-compiling corner-case. Are there other ways my code might end up being delivered broken-on-arrival to end users?

A pkg-config configuration is included in libnotified-dev, so one would only need to call PKG_CHECK_MODULES(notified) in the configure script to get the correct CFLAGS and LIBS - at least on unixy systems.

[spiff]

The dep won't kill us but we will have to stuff a copy in there no matter (most likely through our de downloader). We build on many a platform without central software management systems.

oh, And great initiative!

[Walter Sobchak]

Is this some kind of a listener pattern? Where the skin registers itself as a listener for CPUINFO changes?

Coming from an OSGi background I would just register an event listener with the EventAdmin for the topic "CPUINFO/Speed" to get informations about changes in CPU Speed. The CPU Info publisher would then just propagate this event every sec or so and the event handlers would do whatever they like to do (mark their control dirty, so the gfx layer repaints them).

edit: Ah, ok. After checking out your code I can see its indeed the classical listener pattern. However, I do not like the public Notify member vars in the demo and mich prefer the "White Board Pattern" for listeners, as described. Having to register yourself with some eventmanager and deregister yourself always leads to leaks if not done properly.

[drok]

Is this some kind of a listener pattern?

[...]

Having to register yourself with some eventmanager and deregister yourself always leads to leaks if not done properly.

I assume you're refering to Listener vs. Whiteboard as described at osgi.org...whiteboard.pdf. I'm not familiar with osgi but the whiteboard design pattern described in this paper does resemble the design of my Notifying, Notified classes, while the EventManager code follows the Listener design.

The EventManager class is used for bundling of related low level events into higher level events (eg. in the context of a playlist, a "advancing to next track" event would be equivalent to a sequence of lower level events such as "approach end current track", "load new track", "start cross fade", "unload last track"). I've only spent enough effort for a proof of concept on this class (and used a listener pattern), and will likely redesign it before I release the libnotify library.

However, the Notifying<>InternalVariable<>Notified classes do not have explicit registration/unregistration, as the lifecycle of sources and listeners is managed internally and privately in the assignment operator as well as the destructor. The subscribe()/unsubscribe() methods, which manage registration between sources and listeners will be private in the released library. These classes follow a design similar to the Whiteboard pattern, I believe.

I've paid special attention to memory management so it should not be possible to create a memory leak. If there are code paths that lead to leaks, I'd appreciate your pointing them out (a test case to demostrate a leak, in form of a function for Notified-Tests.cpp would be most appreciated). I will eliminate any memory leaks that exist in this library. I use valgrind for finding leaks, and gcov for verifying test coverage.

There are some lifecycle testcases in the suite already, and I am adding more.

From your comment, I assume you're referring to the demo code in demo_producer.cpp, which makes use of the EventManager facility (ie, event bundling). I suggest checking out notified_demo.cpp, which contains only Whiteboard code patterns (as I have described two posts ago), in a somewhat simple application. Also the test suite (Notified-Tests.cpp) is a great source of example code using the Notifying/Notified classes. Note that the test suite does not contain any EventManager test code, as I don't intend to keep the EventManager design in its current proof-of-concept form.

I think the functionality of the EventManager class can be achieved with a cleaner design than the proof of concept code. I will add test code when I find a better design for this class.

Thank you kindly for reviewing my code. I'm open to suggestions for improvement. It is my hope that this library will be useful in a wide range of applications, beyond xbmc, and beyond UI's.

Question: While I arrived at this design without being aware of the OSGI paper, does OSGI have copyright/patent/whatever claims on designs of this nature ?

[Walter Sobchak]

First of all you can read the OSGi license here:
http://www.osgi.org/Download/Release4V43?info=nothanks
Its more or less the MIT license I think.

The beauty of the Whiteboard pattern is that the listeners do not need to know anything about the possible producer(s). They can just sit (idle) in the system until a producer finds them (in OSGi through the ServiceRegistry) and use them.
The listeners do not have to register themself on every possible producer.

I will have to look closer to your code if it really resembles the whiteboard pattern.

Beside that I would love to see some OSGi-fication of the xbmc codebase for the benefits of addons and skins. But overall the xbmc is already a very fine piece of software!

[drok]

First of all you can read the OSGi license here:
http://www.osgi.org/Download/Release4V43?info=nothanks
Its more or less the MIT license I think.
[...]
Beside that I would love to see some OSGi-fication of the xbmc codebase for the benefits of addons and skins. But overall the xbmc is already a very fine piece of software!

The license you linked says, among others, "You are not authorized to create any derivative work of the Specification", "... granted a limited (without the right to sublicense)". Quite unlike MIT. Anyway, my code is not using anything from OSGI, neither code nor specifications, and the design pattern is more or less textbook architecture.

While you may be able to find design similarities between OSGI designs and libnotified, this does not amount to "OSGi-fication of the xbmc codebase". I was not aware of OSGi when I started the project of converting the xbmc GUI to event driven. My effort is to produce a free and open source library, free of affiliations.

My code is licensed with a "do what you want with it" kind of license.

[Robotica]

Hi Drok,

It would be great if you pull this of! Are you stranded or still busy?