[addons] Thick cleaning up of the C++ API #20710
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AlwinEsch
added
Type: Feature
AlwinEsch
requested review from
garbear,
arnova,
lrusak,
ksooo and
phunkyfish
This was referenced Dec 21, 2021
ksooo
reviewed
Dec 21, 2021
| @@ -337,6 +337,7 @@ class ATTR_DLL_LOCAL CAddonBase | |||
| /// | |||
| CAddonBase() | |||
| { | |||
| CPrivateBase::m_interface->toAddon->create = nullptr; | |||
There was a problem hiding this comment.
This looks strange. Where is this set and to what function?
There was a problem hiding this comment.
Is complete unused on C++, only added to have in "C" ABI based addon case available, if becomes needed.
Was come to it as a destroy in structure, but no create and if a "C" based class handling comes somewhere, it looks more clean 😅.
There was a problem hiding this comment.
Okay then, I was just wondering whether this is intended.
AlwinEsch
force-pushed
the
change-API
branch
from
5658239
to
699ac3c
Compare
This was referenced Dec 21, 2021
This was referenced Dec 28, 2021
In the following changes, a min version increase is necessary, so it makes sense to remove this now because all add-ons need an update anyway.
This has not been used for some time and since the following changes result in a MIN version increase, it can be removed.
In ADDON_Create there were previously two values which are not used, they are removed because a MIN version increase is required. Part of the previous cleanup of the API.
With the addition of "using namespace" in function, the code becomes cleaner because the line breaks are no longer necessary.
The important reason for this is to have "C" ABI more correctly, where its handling pointer is passed in every function call. Previously this was stored in a structure on the add-on side, which is not really clean. It is thought to have cleaner add-ons that are purely based on "C". There will also be some commits to make it even more correct.
With this, the addon function pointers are set in the structure using typedefs, this makes it cleaner and easier to use in other places in the planned sandbox. These typedef's are only used for calls from Kodi to addon, in the other way addon to Kodi they stay in the old way.
Previously, it was transferred to all types in one function using a `void*`, but since such use can be very dangerous and it does not work in the sandbox, it is divided into separate functions here. On the C++ side it remains as before, since it is easier to handle in a class.
This makes a big change, with which the instance function structures are stored in Kodi's `IAddonInstanceHandler` class and a structure with union's instead of "void*" is transferred in" C ". The background is easier to expand and can also be used on the add-on side when creating an instance. The added class `IInstanceInfo` can then be used to query the values. This makes it easier for extensions because the values in the CreateInstance function do not always have to be changed and updates with changes are required for all add-ons.
The ADDON_HANDLE is only used in PVR, since it should not be used any longer, it has now been moved to "kodi/c-api/addon-instance/pvr/pvr_defines.h" and renamed to PVR_HANDLE.
In the correct "C" ABI these are not necessary, since a `struct` must already be at begin at the positions used. Hence, it can be removed and make the code cleaner.
This revises the places where functions are located, so some of the things are moved from General.h to AddonBase.h and these then use the namespace "kodi :: addon", in the background it is more suitable to the Python addon system, where the here in the C++ moved, already in Python at self.Addon. In addition, the new structure AddonToKodiFuncTable_kodi_addon has been added to make changes easier and not have to tackle the MIN version in the future. Unfortunately, the parts in headers had to be moved a bit, which also made it a bit bigger in commit.
The string value kodi_base_api_version stored in the "C" structure is never used and is hereby removed as an adjustment.
With this, own functions are inserted independently of "kodi::addon" in order to call up settings in Kodi using the IAddonInstanceHandler class. At the moment they still use the same functions as the functions in "kodi::addon::" using the "settings.xml" of the addon. In the near future these new functions will be revised (only in Kodi's side) to allow addon instances to have their own settings (e.g. in PVR to use several backends in one addon). The background is when this new support is introduced into Kodi, to avoid that all addons need an update again.
At the moment this call still returns the conventional add-on user class, in future changes the instance will be assigned its own user path, e.g. in the case of planned PVR add-ons which can connect different backends using different settings. As a note on this and previous commits, the add-on documentation will be introduced and revised in a separate request. To do it in these commits would take a long time to get correct.
Previously the values were set as `void*`, since this is a bit ugly, it is replaced here with `const KODI_ADDON_BACKEND_HDL`. In principle, this could be done at any point, but it would extremely blown the request. Since this can also be done separately (without addon updates), it is only related to AddonBase.h here.
The background is to be more in the "C" format style.
Related to changes before. WARNING: All addons need a update about!
AlwinEsch
force-pushed
the
change-API
branch
from
3bcf37b
to
a2b0a7a
Compare
|
The orgy is through so far, apart from image decoder addons are all now with requests. I update to imagedecoder addons after this one and the other one (#20508) in Kodi. Otherwise it would be double the work. The tests so far were OK on my Linux. |
garbear
approved these changes
Dec 29, 2021
|
Also done few tests within Windows and was working too. About int CAudioEncoder::Encode(int nNumBytesRead, uint8_t* pbtStream)
{
return m_ifc->kodi_addoninstance_audioencoder_h->kodi_addon_audioencoder_encode_v1(m_addonInstance, pbtStream, nNumBytesRead);
}The names are autogenerated and as This both are they primary parts to know and the |
|
Can someone else look over the request and approve, want to make sure that no bad part is overseen. |
ksooo
approved these changes
Dec 29, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Since the following requests make it necessary to re-create all add-ons (not absolutely necessary with regard to C++17), I have made a few changes here to clean up, partly to get them in the sandbox (not directly in contact with it) and to have it prepared for a feature on PVR addons to have multiple instances.
For a review, it is better to use the individual commits, as they are only indirectly related and could also come independently (only all breaks the API 😏).
I still have to change and test all addons, so I can't say 100% whether it's running correctly, especially with game addons because there it doesn't have instance support.
Motivation and context
How has this been tested?
What is the effect on users?
Screenshots (if appropriate):
Types of change
Checklist: