APV versioning changes by dkozinski · Pull Request #97 · AcademySoftwareFoundation/openapv

dkozinski · 2025-05-23T12:48:08Z

No description provided.

Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>

- Implement versioning functions with conditional compilation - Added oapv_version to generate version string using OAPV_VERSION_MAJOR, OAPV_VERSION_MINOR, and OAPV_VERSION_PATCH. - Added oapv_libversion function to return library version using OAPV_LIB_VERSION_MAJOR, OAPV_LIB_VERSION_MINOR, and OAPV_LIB_VERSION_PATCH. Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>

jamrial · 2025-05-23T19:02:21Z

src/oapv.c

+#else
+
 static char *oapv_ver = "0.1.13.1";
+static char *oapv_libver = "0.1.13.1";


Why major 0, and why making it match the project version?

I guess that you're trying to change the major, minor, and patch meanings of version.
It should be discussed carefully because the version numbers are already used many O/S and applications.
We need to have strong reason and evidence to change the number dramatically, if we have to change it.

It was decided in #91 (comment), and is already implemented. This PR at first reverted it by accident, but it was then rectified.

- Changed the version in library_version.txt to v1.0.0 - Updated the version definition in oapv.c. Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>

…c/oapv_version.h.in file Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>

jamrial · 2025-05-24T23:08:21Z

pkgconfig/oapv.pc.in

 Description: Advanced Professional Video Codec

-Version: @PROJECT_VERSION_MAJOR@.@PROJECT_VERSION_MINOR@.@PROJECT_VERSION_PATCH@
+Version: @LIB_VERSION_MAJOR@.@LIB_VERSION_MINOR@.@LIB_VERSION_PATCH@


No, this one is fine staying as project version. Pkg-config is usually about a package, and not necessarily a specific library within it.

Not always. In the FFmpeg project, the version in the .pc files of individual libraries specifies the versions of the libraries, for example:

libavfilter.pc

... Name: libavfilter Description: FFmpeg audio/video filtering library Version: 11.0.100 ...

libavcodec.pc

... Name: libavcodec Description: FFmpeg codec library Version: 62.3.101 ...

libavcodec.so -> libavcodec.so.62.3.101 libavcodec.so.62 -> libavcodec.so.62.3.101 libavcodec.so.62.3.101

libavfilter.so -> libavfilter.so.11.0.100 libavfilter.so.11 -> libavfilter.so.11.0.100 libavfilter.so.11.0.100

jamrial · 2025-05-24T23:09:44Z

inc/oapv_version.h.in

+// Project version
+#define OAPV_VERSION_MAJOR @VERSION_MAJOR@
+#define OAPV_VERSION_MINOR @VERSION_MINOR@
+#define OAPV_VERSION_PATCH @VERSION_PATCH@


Wasn't it decided that the header would only have the library version defines?

Wasn't it decided that the header would only have the library version defines?

At the moment, we still need this. The function oapv_version() returns the project version and requires the definitions OAPV_VERSION_MAJOR, OAPV_VERSION_MINOR, and OAPV_VERSION_PATCH.
Currently, we still need two versions of the function.
aopv_version() is used by the reference applications and returns the project version.
The other is oapv_libversion(), which returns the library version and could be used for integration with other projects like FFmpeg.

We will consider simplify it.

Can't you add those defines in a different, also generated but not installed header?

Also, oapv_version() and oapv_libversion() should ideally return a const char*, or a strdup() output that the user needs to free otherwise.

Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>

jamrial · 2025-05-26T22:09:11Z

src/oapv.c

+static char *oapv_libver = "1.0.0";
+
 char * oapv_version() { return oapv_ver; }
+char * oapv_libversion() { return oapv_libver; }


I think this would be better as

unsigned oapv_libversion(void) { return (OAPV_LIB_VERSION_MAJOR << 16) | (OAPV_LIB_VERSION_MINOR << 8) | OAPV_LIB_VERSION_PATCH; }

And then adding the following to oapv_version.h

#define OAPV_LIB_MAJOR(v) (((v) >> 16) & 0xFF) #define OAPV_LIB_MINOR(v) (((v) >> 8) & 0xFF) #define OAPV_LIB_PATCH(v) ( (v) & 0xFF)

This is a lot more library user friendly than a string for runtime version checks.
oapv_version() can remain a string just fine, but it should be const char*.

- Updated the oapv_version and oapv_libversion functions to return unsigned integers instead of strings Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>

jamrial · 2025-05-27T16:29:46Z

inc/oapv_config.h

-// #define OAPV_LIBVERSION_HEADER // defined if the file oapv_libversion.h exists
-// #define OAPV_EXPORT_HEADER     // defined if the file oapv_exports.h exists
+#define OAPV_VERSION_HEADER // defined if the file oapv_libversion.h exists
+#define OAPV_EXPORT_HEADER  // defined if the file oapv_exports.h exists


What is the point of these defines? Why would those headers not exist if they are unconditionally installed?

Without this you'd be able to remove the ifdeffery in src/oapv.c and allow oapv_version() to safely return a const char*. And the reason a const char* is better for it than an int is because ideally you'd fetch the project version using git describe --tags first, and if not possible (Like in a tarball snapshot), then version.txt would be parsed.

kpchoi · 2025-05-27T23:03:10Z

inc/oapv.h

+ * openapv project version
 *****************************************************************************/
-OAPV_EXPORT char *oapv_version();
+OAPV_EXPORT unsigned oapv_version();


Suggest to use unsigned int instead of just unsigned here

kpchoi · 2025-05-27T23:03:19Z

inc/oapv.h

+/*****************************************************************************
+ * openapv library version
+ *****************************************************************************/
+OAPV_EXPORT unsigned oapv_libversion();


Suggest to use unsigned int instead of just unsigned here

kpchoi · 2025-05-27T23:08:11Z

src/oapv.c

+unsigned oapv_version() { 
+    return OAPV_GET_VERSION(0, 1, 13);
+}
+
+unsigned oapv_libversion() { 
+    return OAPV_GET_VERSION(1, 0, 0);
+}
+
+#endif


I don't think we need to have project version and library version separately.
It could lead confusing in communications.
I believe the project version and library version should be same.
Android platform and other 3rd-parties are already thinking like that. it is diffcult to change version numbering meaning at this moment.

Android platform and other 3rd-parties are already thinking like that. it is diffcult to change version numbering meaning at this moment.

They are already linking to soversion 1. This PR is not changing that.

Project and library/API version need to be separate to properly signal new symbols and exported defines in public headers as they are introduced. Project version increases are arbitrary and don't signal actual changes to library users.

@dkozinski @jamrial
Version numbering is the important. DO NOT try to decide it without maintainer's decision.
As I emphasized, it should be aligned to the market requirement.
And, I don't still fully understand any special reason why project and library version should be different in this project.

- Refactor versioning strategy based on feedback from maintainer Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>

src/oapv.c

CMakeLists.txt

Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>

- Changed CPACK_PACKAGE_VERSION to use VERSION_APISET, VERSION_MAJOR, VERSION_MINOR, and VERSION_PATCH for better version management. - Updated oapv.pc.in to reflect the new versioning format. Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>

Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>

…version code to application Signed-off-by: kp5.choi@samsung.com <kp5.choi@samsung.com>

app/oapv_app_util.h

inc/oapv.h

Signed-off-by: kp5.choi@samsung.com <kp5.choi@samsung.com>

…ead of version.txt - Removed the function for retrieving version from git tags and version.txt. - Added checks to ensure the existence of oapv.h and extract version information using regex. - Updated project versioning to use values defined in oapv.h. Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>

jamrial · 2025-06-05T14:27:30Z

src/oapv.c


-static char *oapv_ver = "0.1.13.1";
-char * oapv_version() { return oapv_ver; }
+char *oapv_version(void)


Can't this be made a const char* instead? With CMake it's trivial to generate the "0.1.13.1" string at configure time, but i assume the issue is with Android?
As is, this looks potentially racy. If it can't be const, then it should allocate a buffer and return it, and the user should then free it.

Also, an int version is still extremely useful for runtime checks, so please add it as a separate function, like unsigned int oapv_version_int()

Is macro OAPV_VER_SET from inc/oapv.h is sufficient in this case?

To generate the int to be returned? Yes

unsigned int oapv_version_int() { return OAPV_VER_SET(OAPV_VER_APISET, OAPV_VER_MAJOR, OAPV_VER_MINOR, OAPV_VER_PATCH); }

Without a function like this, API users that want the version in integer form would need to parse the output of oapv_version()

I recommend to use OAPV_VER_NUM constant defined in oapv.h, which is the integer type version number.

That's for compile time checks. A function is needed for runtime checks.

I see it was added and then removed because of this comment. It would have been nice to wait a bit instead of being so hasty. Now it will need to be added as a separate PR.

That's for compile time checks. A function is needed for runtime checks.

I see it was added and then removed because of this comment. It would have been nice to wait a bit instead of being so hasty. Now it will need to be added as a separate PR.

I hope that the PR #104 can be an answer of your considerations.

The function may return not just the version number but also an error code, which is a negative value, despite the return type being unsigned, and as such can't be checked at all. For all the API user knows, it just returned a valid version number they can parse with OAPV_VER_GET_*.

The function may return not just the version number but also an error code, which is a negative value, despite the return type being unsigned, and as such can't be checked at all. For all the API user knows, it just returned a valid version number they can parse with OAPV_VER_GET_*.

Again, oapv_version() has changed to support your comments.
Check the PR #105 .

- Removed the inclusion of oapv_version.h in oapv.h. - Changed the return type of oapv_version function from char* to const char*. - Deleted the oapv_version.h.in file as it is no longer needed. - Updated CMakeLists.txt to remove references to the version header file. Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>

kpchoi

LGTM

cpncf · 2025-06-05T23:30:38Z

It has been long discussion here.
I'd like to strongly recommend to merge this PR of the current version as soon as possible.

…ed int Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>

dariusz-f

LGTM

…gned int -The function was redundant because there is a macro OAPV_VER_SET that does this Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>

* Added reading library version from file in CMakeLists.txt Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com> * Refactor version functions - Implement versioning functions with conditional compilation - Added oapv_version to generate version string using OAPV_VERSION_MAJOR, OAPV_VERSION_MINOR, and OAPV_VERSION_PATCH. - Added oapv_libversion function to return library version using OAPV_LIB_VERSION_MAJOR, OAPV_LIB_VERSION_MINOR, and OAPV_LIB_VERSION_PATCH. Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com> * Updated library version to 1.0.0 - Changed the version in library_version.txt to v1.0.0 - Updated the version definition in oapv.c. Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com> * Update CPack version settings to use LIB_VERSION and added missing inc/oapv_version.h.in file Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com> * Exposed oapv_libversion() finction Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com> * Refactor version handling to use unsigned integers - Updated the oapv_version and oapv_libversion functions to return unsigned integers instead of strings Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com> * Update versioning for project and library - Refactor versioning strategy based on feedback from maintainer Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com> * Removed obsolete library version checks from CMakeLists.txt Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com> * Added comment to oapv_version function for Android versioning Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com> * Update versioning scheme in CMakeLists.txt and pkgconfig file - Changed CPACK_PACKAGE_VERSION to use VERSION_APISET, VERSION_MAJOR, VERSION_MINOR, and VERSION_PATCH for better version management. - Updated oapv.pc.in to reflect the new versioning format. Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com> * Refactor versioning messages and declarations Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com> * refactoring code to avoid a symbol conflict. moved version string conversion code to application Signed-off-by: kp5.choi@samsung.com <kp5.choi@samsung.com> * the candidate of consensus Signed-off-by: kp5.choi@samsung.com <kp5.choi@samsung.com> * refactor CMakeLists.txt: Extract version information from oapv.h instead of version.txt - Removed the function for retrieving version from git tags and version.txt. - Added checks to ensure the existence of oapv.h and extract version information using regex. - Updated project versioning to use values defined in oapv.h. Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com> * Further refactoring related to version handling - Removed the inclusion of oapv_version.h in oapv.h. - Changed the return type of oapv_version function from char* to const char*. - Deleted the oapv_version.h.in file as it is no longer needed. - Updated CMakeLists.txt to remove references to the version header file. Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com> * Added oapv_version_int function that returns the version as an unsigned int Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com> * Removed oapv_version_int function that returns the version as an unsigned int -The function was redundant because there is a macro OAPV_VER_SET that does this Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com> --------- Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com> Signed-off-by: kp5.choi@samsung.com <kp5.choi@samsung.com> Co-authored-by: kp5.choi@samsung.com <kp5.choi@samsung.com>

dkozinski added 2 commits May 23, 2025 14:23

Added reading library version from file in CMakeLists.txt

8ebc1ef

Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>

dkozinski requested review from dariusz-f and kpchoi May 23, 2025 12:48

jamrial reviewed May 23, 2025

View reviewed changes

dkozinski added 2 commits May 24, 2025 22:35

Updated library version to 1.0.0

184fd24

- Changed the version in library_version.txt to v1.0.0 - Updated the version definition in oapv.c. Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>

Update CPack version settings to use LIB_VERSION and added missing in…

ae2736a

…c/oapv_version.h.in file Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>

jamrial reviewed May 24, 2025

View reviewed changes

Exposed oapv_libversion() finction

d00ba13

Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>

jamrial reviewed May 26, 2025

View reviewed changes

Refactor version handling to use unsigned integers

c517562

- Updated the oapv_version and oapv_libversion functions to return unsigned integers instead of strings Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>

jamrial reviewed May 27, 2025

View reviewed changes

kpchoi reviewed May 27, 2025

View reviewed changes

dariusz-f mentioned this pull request May 29, 2025

Versioning issues #99

Closed

Update versioning for project and library

a0af74c

- Refactor versioning strategy based on feedback from maintainer Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>

dariusz-f requested changes Jun 2, 2025

View reviewed changes

src/oapv.c Outdated Show resolved Hide resolved

CMakeLists.txt Outdated Show resolved Hide resolved

dkozinski added 4 commits June 2, 2025 11:50

Removed obsolete library version checks from CMakeLists.txt

80342c7

Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>

Added comment to oapv_version function for Android versioning

1c62d66

Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>

Refactor versioning messages and declarations

9448256

Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>

dariusz-f approved these changes Jun 2, 2025

View reviewed changes

refactoring code to avoid a symbol conflict. moved version string con…

d1e72dd

…version code to application Signed-off-by: kp5.choi@samsung.com <kp5.choi@samsung.com>

dariusz-f requested changes Jun 5, 2025

View reviewed changes

app/oapv_app_util.h Outdated Show resolved Hide resolved

inc/oapv.h Show resolved Hide resolved

kpchoi and others added 2 commits June 5, 2025 20:21

the candidate of consensus

bb44b85

Signed-off-by: kp5.choi@samsung.com <kp5.choi@samsung.com>

jamrial reviewed Jun 5, 2025

View reviewed changes

kpchoi approved these changes Jun 5, 2025

View reviewed changes

kpchoi requested a review from dariusz-f June 5, 2025 23:25

Added oapv_version_int function that returns the version as an unsign…

35ffde0

…ed int Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>

dariusz-f approved these changes Jun 6, 2025

View reviewed changes

Removed oapv_version_int function that returns the version as an unsi…

99ca127

…gned int -The function was redundant because there is a macro OAPV_VER_SET that does this Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>

kpchoi merged commit 1d79983 into main Jun 6, 2025
5 checks passed

Conversation

dkozinski commented May 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kpchoi May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamrial May 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkozinski May 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamrial May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpncf Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kpchoi Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kpchoi left a comment

Choose a reason for hiding this comment

Uh oh!

cpncf commented Jun 5, 2025

Uh oh!

dariusz-f left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

kpchoi May 27, 2025 •

edited

Loading

jamrial May 24, 2025 •

edited

Loading

dkozinski May 25, 2025 •

edited

Loading

jamrial May 27, 2025 •

edited

Loading

cpncf Jun 6, 2025 •

edited

Loading

kpchoi Jun 9, 2025 •

edited

Loading