[addons][inputstream][videocodec] rework documentations, fix "C" ABI and fix memleaks #17438
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: Cleanup
AlwinEsch
changed the title
13 tasks
AlwinEsch
force-pushed
the
rework-inputstream
branch
from
71607fe
to
d9c8c51
Compare
13 tasks
phunkyfish
added
the
PR Cleanup: Recent
13 tasks
jenkins4kodi
added
the
Rebase needed
AlwinEsch
force-pushed
the
rework-inputstream
branch
4 times, most recently
from
4130195
to
60275e4
Compare
13 tasks
|
Is now cleaned up by add of code from other requests, further is on #17501 another separated part created. After them is in I look to take some documentation parts where become no change from API here. |
AlwinEsch
force-pushed
the
rework-inputstream
branch
from
60275e4
to
b6beef0
Compare
AlwinEsch
removed
the
Rebase needed
AlwinEsch
force-pushed
the
rework-inputstream
branch
from
b6beef0
to
9f179f5
Compare
jenkins4kodi
added
the
Rebase needed
AlwinEsch
force-pushed
the
rework-inputstream
branch
from
9f179f5
to
398d91c
Compare
AlwinEsch
force-pushed
the
rework-inputstream
branch
from
56b1a34
to
7ee2b54
Compare
Before it contains a C++ part where not match API. Also id the C++ addon call changed to use std::vector
Before was there the complete structure has return value where makes harder in unused cases and not match a correct "C" ABI.
…C" style Before was a pure "C" compile not possible, which shows that this ABI is not set correctly. The interface must be sure that it is only in pure "C"! This change only in relation to inputstream, all other types and positions also require some changes.
This is added to prevent memleaks and to use stack memory. This becomes on next changes improved and done by a C++ structure inside Headers.
This is done to confirm a pure "C" within interface and to allow use for other languages where need a "C" as base. This changes are the initial part, there still few included headers where use C++! This becomes improved on following changes, for here to have the other commits more clean and see if relate to "C" between Kodi or to "C++" on addon.
…TREAM_PROPERTY This is added to translate the "C" structure INPUTSTREAM_PROPERTY to C++ class InputstreamProperty. This added to make access easier and more secure against leaks.
…PUTSTREAM_CAPABILITIES This is added to translate the "C" structure INPUTSTREAM_CAPABILITIES to C++ class InputstreamCapabilities. This added to make access easier and more secure against leaks.
…M_INFO This is added to translate the "C" structure INPUTSTREAM_INFO to C++ class InputstreamInfo. This added to make access easier and more secure against leaks.
…A and INPUTSTREAM_CONTENTLIGHT_METADATA This to have them more clean and to match more the interface.
…AM_TIMES This is added to translate the "C" structure INPUTSTREAM_TIMES to C++ class InputstreamTimes. This added to make access easier and more secure against leaks.
This separate the Kodi TimingConstants.h to use a own on addons where his text begin with "STREAM_..." now and them with "DVD_..." stays on Kodi itself. There are few Pro's and Contra's about but for wanted API better to have. +++: It stays in pure "C" for them and as base for other languages and the "C++" also on new header +++: For the time where the headers comes into a own dev-kit and no more in Kodi source +++: No file outside of the addon header placed in Kodi ---: On changed by them in Kodi must be them on addon headers also by value changes the header updated.
…ders dir This add the header to the kodi-dev-kit folder to have them all together and prepared for a independent dev kit, further to confirm more the "C" use between Kodi and addon.
Further is his "C" code on "kodi/c-api/addon-instance/inputstream/stream_codec.h" now. This to have them more clean as it only relate to inputstream and codec addon instances and not to other addon types.
There is the "C" part about moved to "kodi/c-api/addon-instance/inputstream/stream_crypto.h" and parts renamed to have more clean. Further is the C++ helper class "kodi::addon::StreamCryptoSession" added to use C++ on addons about.
…ts to addon dev-kit There becomes the parts where was previous on Kodi's videoplayer moved to the addon dev kit and much cleaned to confirm the "C" ABI style. This change is not nice but needed for this: - The dev-kit should be soon independent of Kodi (trademark, other languages, smaller) - Before was there some C++ parts inside where breaks a safe ABI use between addon and Kodi - Have the header places for addons better sorted and more clean in view
Before was for streams always Kodi's C++ based structure used where not match a "C" ABI for the addons.
This are no more used now by addons itself and only be used on videoplayer itself.
This to cleanup and to confirm "C" safe ABI and base for other languages. Also is on the C++ header a clang cleanup done.
Class to handle the "C" structure VIDEOCODEC_INITDATA in C++.
This makes the documentation about inputstream and videocodec to style needed for binary addons and to show correct on Website. There still some parts where need improved and a bit more detailed text, but think to make indpendent then as it not touch API itself.
This increase all changed addon instance versions. PVR included as it use a the new DEMUX_PACKET structure now.
AlwinEsch
force-pushed
the
rework-inputstream
branch
from
7ee2b54
to
7c132fd
Compare
Rechi
reviewed
Oct 29, 2020
| @@ -0,0 +1,18 @@ | |||
| --- a/xbmc/addons/kodi-dev-kit/include/kodi/c-api/addon-instance/inputstream/stream_constants.h | |||
There was a problem hiding this comment.
Seems to come in on rebase, Sorry!
Becomes today kicked out and think to add also on .gitignore a /*.diff below of /*.patch to prevent it on base folder in future.
13 tasks
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
There are some changes that fix Memleaks that I noticed on the addons.
This is ensured by the fact that the "C" structures at the input stream are not transferred to the addon but are managed using C++ classes, which are then used by the addon.
In addition, some things are simplified because the data does not have to be used on the addon using "C".
The other, and most importantly with Memleaks, is that the interface to inputstream and videocodec is now 100% "C" and no "C++" stuff is given between Kodi and addon.
I will go through this change step by step on all addon types, on the one hand to make feature requests in this regard (where users also requested a usable "C") and to be really sure that there are no creeping errors in the "C" ABI.
In the end (other requests), a "C" ABI test system is brought to Kodi to ensure that nothing wrong gets into it.
Regarding documentation, you can see here how it looks:
There are certainly a lot of grammatical errors and also certainly wrong descriptions, if someone could look over it and my miserable English would be correct 😀.
Also added on first commit the support about BlockSize where on stream reads is required.
This is still WIP and certain things are not yet correct.
As a note: I have adapted the changes to our code guidelines.
EDIT
I forgot a part of the codec where the "C" structures are in their own memory (to simplify API changes, like on c162382)
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of change
Checklist: