Skip to content
This repository has been archived by the owner on Dec 31, 2020. It is now read-only.

API/ABI changes review for NVTT #259

Open
lvc opened this issue Apr 24, 2017 · 5 comments
Open

API/ABI changes review for NVTT #259

lvc opened this issue Apr 24, 2017 · 5 comments

Comments

@lvc
Copy link

lvc commented Apr 24, 2017

The review of API/ABI changes for NVTT since 2.0.0 version: https://abi-laboratory.pro/tracker/timeline/nvidia-texture-tools/

Created with the help of open-source abi-tracker tool: https://github.com/lvc/abi-tracker. Hope it will be helpful for users and maintainers of the library.

nvtt-1
nvtt-2

Environment

  • Linux x86_64
  • GCC 4.9
  • NVTT 2.0.0 and higher
@castano
Copy link
Owner

castano commented Sep 4, 2017

This is neat. Is this tracking all exported symbols or only the ones declared in the public headers? In my case the nvtt library exports many private symbols, but applications are only supposed to use the ones declared in the public header 'nvtt.h'.

@lvc
Copy link
Author

lvc commented Sep 26, 2017

Hi,

It checks all headers in the source directory (to check ABI in all shared objects) for now.

I've fixed it to check only one header nvtt.h:

nvtt-3
nvtt-4

Thank you.

@StefanBruens
Copy link
Contributor

StefanBruens commented Feb 6, 2018

Just some idea:
Lets call 2.1.0/2.1.1 "development snapshots". Then provide a "stable" version 2.2.0, meant for everyone upgrading from 2.0.8 or earlier.

The change in 2.1.1 is just a correction of class added in 2.1.0 (78054e9), so no issue for anyone coming from 2.0.8 or earlier.

The new APIs in 2.1.0 are fine for anyone coming from 2.0.8.

Remaining issues: removed API in 2.1.0, namely:

  1. InputOptions::setColorTransform ( enum ColorTransform t )
  2. InputOptions::setLinearTransform ( int channel, float w0, float w1, float w2, float w3 )
  3. InputOptions::setTextureLayout ( enum TextureType type, int width, int height, int depth )

(3) is trivial, as it has been replaced by a new function with an additional (defaulting) parameter:
NVTT_API void setTextureLayout(TextureType type, int w, int h, int d = 1, int arraySize = 1);
Just readd
NVTT_API void setTextureLayout(TextureType type, int w, int h, int d = 1)
and make the old function a wrapper to the new function. As InputOptions has no vtable, order of declaration does not matter.

The first two look a little bit more complicated (commit 1e2567e#diff-48db6077e2f51b018674558733ee4460), but are trivial as well.
setLinearTransform actually never had any effect, so it is fine to replace this one with a stub.
setColorTransform only had an effect with ColorTransform_YCoCg. This can be restored.

Biggest issue:
new virtual method in OutputHandler::endImage()
If an application compiled with the old OutputHandler sets its own outputhandler, two things can happen:

  1. The class adds no virtual methods - a call to endImage() is not backed by any implementation, so we end up with calling (*0)();
  2. The class adds a virtual methods - this ends up in the vtable slot where endImage() is expected. Everytime endImage() is called, a completely different method is executed.

For this issue I see no trivial solution, but to rename the current OutputHandler base class and restore the original one.

StefanBruens added a commit to StefanBruens/nvidia-texture-tools that referenced this issue Feb 6, 2018
Although adding a method argument with default argument keeps API
compatibility, it changes the function type.

Use function overloading instead and implement the old one as a wrapper
to the extended one, supplying the default argument.

Fixes part of castano#259

Signed-off-by: Stefan Brüns <[email protected]>
StefanBruens added a commit to StefanBruens/nvidia-texture-tools that referenced this issue Feb 6, 2018
The linear transform has never been implemented, so just add as a stub
for API/ABI compatibility.

Fixes part of castano#259

Signed-off-by: Stefan Brüns <[email protected]>
StefanBruens added a commit to StefanBruens/nvidia-texture-tools that referenced this issue Feb 6, 2018
The only colortransform actually implemented was the transform from
RGB to YCoCG (ColorTransform_YCoCg).
Restore the full enum for API compatibility, and allow the transform
to be specified using InputOptions::setColortransform.

Fixes part of castano#259

Signed-off-by: Stefan Brüns <[email protected]>
@castano
Copy link
Owner

castano commented Feb 7, 2018

I was hoping 2.1.0 would be an opportunity to do some breaking changes. My plan was to maintain binary compatibility through the revisions of minor version, but allow changes between minor versions. This may be a little unusual, but I'm reserving the major version changes for major rewrites with little to none source code compatibility. If this makes life too hard for distributions, I'm willing to reconsider. Patches like this that improve compatibility at little cost are certainly welcome.

@StefanBruens
Copy link
Contributor

Its somewhat sad this has not been merged prior to the 2.1.2 release.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants