Replace the entire scripting engine with one that's code-generated #901

Merged
merged 8 commits into from Sep 9, 2012

Conversation

Projects
None yet
10 participants
Member

jimfcarroll commented Apr 25, 2012

At the core this change adds two things:

  1. A native library that looks like the old python api in xbmc/interfaces/native
  2. A codegenerator that generates all of the C-python code to call the native library in xbmc/interfaces/swig

This is in preparation for a removal of xbmc/interfaces/python

This commit adds an alternate addon library where a codegenerator creates all of the CPython code that bridges a native implementation to Python. This is nowhere near ready but I wanted to get it in front of people.

What you need to do to try it out:

  1. You need SWIG installed. Older versions should work fine.
  2. You need a JRE that can be run from the make command.
  3. configure with --enable-swigaddons

Upon a successful make there will be a directory created with a bunch of generated source code here:

xbmc/interfaces/swig/generatedSWIGBindings

Current limitations:

  1. Callbacks are still under construction but the structure is there.
  2. There is a problem with identifying the differences between Strings and Unicode return values in python. Currently there is a way to override the default on a return value by return value basis but I don't know how many places need it.
  3. It only works on Linux for now.
  4. So far it has very limited testing.

TODO:

  1. work through many more issues
  2. the other two build platforms. Windows will need quite a bit of thought. @wsoltys - let me know what you think.
  3. Callbacks need more work.
  4. Control::addItem - doesn't currently exist.
  5. ListItem::addContextMenuItems is empty - probably shouldn't be.
  6. Dialog::browse is split between browse and browseMultiple due to the dual return types. Need to put these back together for Python.
  7. The PythonMonitor functionality is not available here yet.
  8. Better error messaging/handling when callbacks create an exception.
  9. String/CStdString distinction CStdString=unicode, String=non-unicode return value
  10. abortRequested is wrong
  11. Fix the Makefile for the generator to use mkdir -p
  12. Update the License text
  13. Documentation - use the doxygen comments to populate the doc string capabilities of python
  14. Allow the suppression of the codegenerator warnings.
  15. Reorganize the directories (suggestions welcome).
  • xbmc/interfaces/native -> xbmc/interfaces/legacy
  • move the generator outside of the source tree - probably tools.
  • move the current bindings directory (xbmc/interfaces/swig/bindings) which has the templates for generating the python api bridge (language bindings) into xbmc/interfaces/bindings/python.
  • move the SWIG interface files (*.i) currently in xbmc/interfaces/swig into xbmc/interfaces/apidef
  • move the engine files (that code that ties the language bindings into xbmc. In this case XBPython.xx,XBPyThread.xx, etc) into xbmc/interfaces/engines/python.

this means there will be these new directories:

  • xbmc/interfaces/legacy
  • xbmc/interfaces/apidef
  • xbmc/interfaces/bindings (currently with one subdirectory called 'python').
  • xbmc/interfaces/engines (currently with one subdirectory called 'python').
  • tools/apigenerator

Ideally the engines should be binary addons downloaded when a scripting addon requires it (like the python) - but I'll leave that for later.

I could merge the engines and bindings directories if anyone thinks that's more straightforward.

Then I need to make sure all of the platforms build with the new directory structure.

As usual, feedback is appreciated.

@ghost

ghost commented Apr 25, 2012

then can it at least be split to reflect these two logical bits?

Member

jimfcarroll commented Apr 25, 2012

Good idea.

Member

jimfcarroll commented Apr 25, 2012

Ok. Separated.

Member

jimfcarroll commented Apr 25, 2012

Ok, cleaned up the native addon redundancy. @cptspiff - this should be less redundant redundant

Member

jimfcarroll commented Apr 26, 2012

Thanks @garbear !

Contributor

nuka1195 commented Apr 27, 2012

C1fac67. I don't see a modulexbmcaddon.cpp/h in the list of files. maybe not necessary?

Member

jimfcarroll commented Apr 27, 2012

The ModuleAddonXXXX.h/cpp files only hold the non-class module methods. The addon module doesn't have any. The module definitions are in the xbmc/interfaces/swig directory and end in a '.i' which is a SWIG interface file. There's one per module.

Owner

Montellese commented Aug 12, 2012

Just as an FYI there is a problem in the way I integrated the generation of the SWIG bindings into the VS project. Currently the bindings are generated on every compile but I'm not yet sure why. It should only happen when the .i files have changed or when the generated files don't exist. But there's something screwed up with the VS project file.

Member

jimfcarroll commented Aug 12, 2012

Hey @Montellese,

What do you think of this: http://msdn.microsoft.com/en-us/library/e85wte0k.aspx
and the reference to "Custom Build _Step_s" here: http://msdn.microsoft.com/en-us/library/dd293663.aspx

It looks like you can add a "step" to a build and not necessarily override the entire build rule. I can play with this over next weekend.

Member

jimfcarroll commented Aug 12, 2012

Maybe that's a step in the overall build process. I'm not sure. It does say "The step can specify a command line to execute, any additional input or output files, and a message to display. If MSBuild determines that your output files are out-of-date with regard to your input files, it displays the message and executes the command."

Owner

Montellese commented Aug 12, 2012

I gave it a shot and there are at least two problems:

  1. When I specify a custom build step for the XBMC project all the custom build tools I specified for the .i files aren't triggered anymore. I never used either of those so I don't really know what the problem is.
  2. I don't know how to specify to only trigger the custom build step when one of the header files in xbmc/interfaces/native has changed. It works okwhen I specify a single header file (or multiple) but I didn't get it to work with a wildcard (i.e. $(SolutionDir)....\xbmc\interfaces\native*.h).

The custom build step command could look like this

FOR %%f IN ($(SolutionDir)..\..\xbmc\interfaces\swig\*.i) DO CALL $(SolutionDir)..\BuildDependencies\GenerateSWIGBindings.bat %%~dpf %%~nf

or could be put into another BAT file (slightly modified) and then called from the custom build step.

Nice. Trivial thing for such a huge a huge pull request but the XBMC licenses need to be updated :)

Member

jimfcarroll commented Aug 20, 2012

Ok. That should fix it.

Member

jimfcarroll commented Aug 20, 2012

Thanks everyone for all of your help.

EDIT: Moved the TODO List into the main message at the top. I'll keep it up to date.

Member

jimfcarroll commented Aug 20, 2012

Oh!
8) Update the License text.

Owner

Montellese commented Aug 20, 2012

As already mentioned you'll want 10f8ee48af87a72166d042c6fc4acd058e5a66b9 for win32. Furthermore I still don't have a solution for re-generating the python files if a header file in xbmc/interfaces/native changes. And right now I get an exception as soon as I try to start XBMC. The exception is somewhere in WinMainCRTStartup() (some win32 specific C++ initialization code) specifically in _cinit().

Generally I agree with your re-structuring. Only "apidef" sounds a bit odd to me.

Member

jimfcarroll commented Aug 21, 2012

The crash is unrelated to the nature of my changes. It's because XBPython compilation unit must now be initialized later than the Announcement code and therefore the static call to CAnnounementManager::AddAnnouncer called from the constructor of XBPython (which is called when the XBPython.o compilation unit initializes due to the global g_pythonParser) is executed prior to the static elements (one of which is a std::vector) have been constructed.

The fix is ugly.

Owner

Montellese commented Aug 21, 2012

Yup that fixes the exception. Nice find.

I have one more commit with a few cosmetics in the VS project file: 7f9c655f3e414b30e566fc44ca3e49f1b08d5b4f
These unneeded tags were added while I played around with the different build events etc.

xbmc/addons/Addon.cpp
@@ -449,7 +453,7 @@ void CAddon::SaveSettings(void)
doc.SaveFile(m_userSettingsPath);
CAddonMgr::Get().ReloadSettings(ID());//push the settings changes to the running addon instance
- g_pythonParser.OnSettingsChanged(ID());
+// g_pythonParser.OnSettingsChanged(ID());
@topfs2

topfs2 Sep 1, 2012

Member

Is this broadcasted to the bindings elsewhere?

@jimfcarroll

jimfcarroll Sep 4, 2012

Member

Good catch. This is part of TODO 7 in the list on the main PR message. After thinking about a better way to do this and talking to @amet over the weekend, I'll be putting this back in.

xbmc/interfaces/native/AddonCallback.cpp
+ {
+ if (callback)
+ {
+ if (hasHandler())
@topfs2

topfs2 Sep 1, 2012

Member

I haven't had time to go through all the code yet but I'm guessing this is to handle methods bound to an object? Would it perhaps make sense to use that naming instead? If handler is more accurate thats fine, just something which popped in when I was reading

@jimfcarroll

jimfcarroll Sep 4, 2012

Member

Yes. This is now called just 'Callback' (one of the above commits removes all of the extraneous 'Addon' labels to all of the classes). All of this infrastructure is mainly to support python's inability to deal with calls from arbitrary threads - so we need to shuffle calls over to a thread currently running a script, and wait for that thread to call back into a method we have control over, before we can actually execute the callback.

To do this there's two main abstractions. The first is a 'Callback' which is a root class for a set of templates (CallbackFunction) for binding a call and it's parameters to an object (like you said). Here's an example of how it can be used:

Callback<MyClass, int>* callback = new Callback( myObj, &MyClass::doSomething, 5 );

This assumes that there's a class called MyClass with a method called 'MyClass::doSomething(int).' When you call 'execute' on the callback at some later point then 'myObj->doSomething(5)' will be called.

Member

topfs2 commented Sep 3, 2012

Looks awesome :)

jimfcarroll pushed a commit that referenced this pull request Sep 9, 2012

Jim Carroll
Merge pull request #901 from jimfcarroll/codegenerated-addons
Replace the entire scripting engine with one that's code-generated

@jimfcarroll jimfcarroll merged commit c25b3d2 into xbmc:master Sep 9, 2012

Owner

Memphiz commented Sep 9, 2012

Well fought :D

Contributor

mazkolain commented Oct 2, 2012

Sorry for resurrecting this PR, but I suspect that it broke music playback on an addon I'm working on: Spotimc.

The addon relies on an embedded python server (spawned from the addon itself) to serve music streams to XBMC via http. It worked before this PR was merged, but since then all requests to this embedded server end up on timeout errors.

If I'm not wrong, the python bindings for the xbmc module that were superseded by this PR released the python GIL with CPyThreadState, as you can see on the body of Player_Play:
https://github.com/xbmc/xbmc/blob/Frodo_alpha5/xbmc/interfaces/python/xbmcmodule/player.cpp#L109

I believe that the new implementation does not release the GIL, and these steps could end up causing the issue:

  1. Playback is requested from python to an url of the embedded server.
  2. XBMC opens an http connection to the embedded server.
  3. On another python thread the server should handle the request, but it can't since the python GIL is still owned by (1). As a result the requests always time out.

Hope these statements are true, and are able to explain the issue I'm experiencing.

Thanks in advance.

Member

jimfcarroll commented Oct 2, 2012

Ah. No problem resurrecting the PR. If you're right then the fix is trivial. I didn't really want to release the GIL for short lived method calls that quickly return to python and the Player.play method should kick off the player and return. We can try a fix if you can rebuild the code. Can you add a "DelayedCallGuard" to the top of the playStream method in Player.cpp (you can find an example of a DelayedCallGuard in the Player destructor). Let me know if that resolves your issue and I'll put a fix in.

Contributor

mazkolain commented Oct 5, 2012

Tried rebuilding XBMC, adding the suggested line on top of Player::playStream()'s body, with no results. The function's initial code now looks like this:

void Player::playStream(const String& item, const xbmcgui::ListItem* plistitem, bool windowed)
{
    DelayedCallGuard dc;
    CLog::Log(LOGDEBUG, "in Player::playStream()");
    TRACE;

As you can see, I also tried adding some debug statements here and there, but they won't get printed on the log file. The file I'm supossed to change is /xbmc/interfaces/legacy/Player.cpp, right?

Sorry if I did something wrong, I'm new to this :)

Owner

MartijnKaijser commented Oct 5, 2012

@mazkolain
If you need help please use the XBMC forum.
http://forum.xbmc.org/forumdisplay.php?fid=93

Posting you questions here will send a notification to a lot of people which we rather avoid

Member

jimfcarroll commented Oct 6, 2012

@mazkolain, That's exactly right. You'll need to help me reproduce it. I guess we should move to the Developer forum. I'll look for your post. You'll need to explain how to reproduce it. Thanks. Sorry for the trouble.

Contributor

mazkolain commented Oct 9, 2012

I've moved the discussion of the issue to the XBMC forum as suggested:
http://forum.xbmc.org/showthread.php?tid=142303

Sorry for the noise...

@jimfcarroll we lost the original functionality here, it was supposed to reset subtitle delay to 0 when activating new subtitle.

see here https://github.com/xbmc/xbmc/blob/Eden/xbmc/interfaces/python/xbmcmodule/player.cpp#L509

any reason you removed that?

Should this be playCurrent()?

(sorry for commenting on such a large diff)

@jimfcarroll jimfcarroll deleted the jimfcarroll:codegenerated-addons branch Apr 5, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment