Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Xmp toolkit v2023-12 submodule #3014

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

serghov
Copy link

@serghov serghov commented Jul 10, 2024

This merge request attempts to pull XMP-Toolkit-SDK as a submodule. Due to the complicated and messy build system in XMP-Toolkit-SDK, I avoid using it and instead use the existing in-tree build system with some modifications.

I have made several hacky workarounds:

  1. There is a hardcoded include for /third-party/expat/lib/expat.h in XMP-Toolkit-SDK. Since we pull expat as an external dependency, I had to copy expat.h to a fake third-party/expat/lib/ directory in the build folder at generation time. I am not quite sure if the same will be possible with meson.
  2. MD5Update has a slightly different signature, so I used a const_cast. I expect to get rid of this when I do a refactoring.
  3. SXMPMeta::DeleteNamespace seems to be deprecated in this version, which needs further investigation.

To-do:

  • Add all compile definitions for different platforms that XMP-Toolkit-SDK requires, e.g., WIN_ENV, WIN_UNIVERSAL_ENV, etc.
  • Integrate with meson.
  • Refactor xmp.cpp and all associated files.
  • Fix unit tests
  • Fix github actions to clone with submodules

@serghov serghov marked this pull request as draft July 10, 2024 08:36
@serghov serghov mentioned this pull request Jul 10, 2024
@serghov serghov force-pushed the xmp-toolkit-v2023-12-2 branch 4 times, most recently from 9027573 to 2b479ec Compare July 10, 2024 10:56
@serghov
Copy link
Author

serghov commented Jul 11, 2024

There are some changes in behavior of XMP-Toolkit-sdk in newer versions.

One major change I encountered is setting of aliased properties. Previously as far as I understand sdk did not care about aliasing whatsoever, the newer version do and attempt to handle them according to specifications.

Specific XMP implementations may treat a
property in one namespace as equivalent to a property in another namespace. However, to foster interchange,
applications must always write the standard, “base” form of the property.

XMPSpecificationPart2.pdf page 8

I found this comment in sources

  // According to the XMP specification, Xmp.tiff.ImageDescription is an
  // alias for Xmp.dc.description. Exiv2 treats an alias just like any
  // other property and leaves it to the application to implement specific
  // behaviour if desired.

Handling aliases in exiv2 was left to application developers, which is fine except new versions of XMP-Toolkit-sdk attempt to handle aliases under the hood, although I suspect they might have a bug or two in there. I will try to figure out a solution upstream.

So with this patch exiv2 will automagically set the value of the property alias is pointing to, however this might be considered a breaking change and should probably be discussed.

And because XMP-Toolkit-sdk handles aliases for exiv2 now, we have to keep track of every language we set for a langAlt property and overwrite them whenever necessary.

An alternative array of simple text items. Language alternatives facilitate the selection of a simple text item
based on a desired language. Each array item shall have an xml:lang qualifier. Each xml:lang value shall be
unique among the items.

XmpSpecificationPart1 page 30

@serghov serghov force-pushed the xmp-toolkit-v2023-12-2 branch 5 times, most recently from 6532cd6 to 709ba45 Compare July 17, 2024 12:50
@serghov serghov force-pushed the xmp-toolkit-v2023-12-2 branch from 709ba45 to a3284aa Compare July 17, 2024 13:20
@HTRamsey
Copy link
Contributor

I can offer you one thumbs up if you finish this

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

Successfully merging this pull request may close these issues.

2 participants