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

KHR_XMP #1553

Merged
merged 56 commits into from
Aug 15, 2020
Merged

KHR_XMP #1553

merged 56 commits into from
Aug 15, 2020

Conversation

emilian0
Copy link
Contributor

@emilian0 emilian0 commented Feb 1, 2019

KHR_xmp draft.
TODOes (feel free to update the list):

  • Should we just remove the enumeration and allow any glTF node to reference XMP packets?->YES
  • Agree on location of XMP packets (top level or under asset). -> under asset
  • Clarify asset is the place where to reference a packet that applies to the entire gltf.
  • Consider constraining to no more than one XMP packet reference per node(to avoid inconsistencies).
  • Talk about proposal of a unique glTF packet per glTF.
  • XMP [qualifiers section 6.4] (https://wwwimages2.adobe.com/content/dam/acom/en/devnet/xmp/pdfs/XMP%20SDK%20Release%20cc-2016-08/XMPSpecificationPart1.pdf)
    -> Talked to XMP chair regarding qualifiers, no need for adding them to the extension.
  • Adjust schema
  • Sketchfab approval
  • Test extension with Adobe Dimension.
  • Address concerns regarding compatibility with JSONLD
  • Address concerns regarding overriding rules
  • Test extension implementations
    • Exported glb from Adobe Dimension - imported in Babylon: asset metadata displays correctly.
    • Test extension with 3D e-commerce CLI tool (in progress)

@andreasplesch
Copy link
Contributor

andreasplesch commented Feb 1, 2019

Some quick thoughts:

  • what is the difference between ordered and unordered array: both are Arrays in the schema
  • dc:date will be popular and used as creation date. But its definition is fuzzy. Where is the 'event in the life cycle' defined ?
  • perhaps make dc namespace a key:
{
    "meshes": [
      {
        extensions : {
            "KHR_XMP": {
                "dc": {
                    "contributor" : [ "Creator1Name", "[email protected]", "Creator3Name<[email protected]>"],
                    "coverage" : "Bay Area, California, United States",
                    "creator" : [ "CreatorName", "[email protected]"],
...

the colon is more of a XML convention, I believe. Are colons a problem in js property names ? Probably not, but unusual.

  • add mechanism on how to add other namespaces, and clarify that only XMP namespaces are allowed if so. If only dc is included in the extension, say so, and/or rename to KHR_XMPDC
  • other XMP namespaces seem to have more complex data types, requiring object key-value values. Make the effort of translating now for the ones which are applicable ?
  • follow more closely XMP standard roles of properties: https://github.com/adobe/xmp-docs/blob/master/XMPNamespaces/dc.md, for example the standard points out that the creator should not be included as contributor; potentially just make XMP definitions authoritative by reference, explicitly. Table descriptions would be just summaries.

@donmccurdy
Copy link
Contributor

donmccurdy commented Feb 1, 2019

add mechanism on how to add other namespaces, and clarify that only XMP namespaces are allowed if so. If only dc is included in the extension, say so, and/or rename to KHR_XMPDC

Assuming the DC namespace is sufficient for this extension (seems likely, but let's allow a little time for more feedback) I think that renaming the extension to KHR_xmp_dc and removing the dc: prefixes may be about right. Future extensions could follow that pattern, although it's unclear whether more XMP metadata is needed at this time.

In my opinion, this extension should follow the pattern of KHR_lights_punctual in creating a top-level array that nodes point into. Given a file containing 100 meshes with two unique licenses, licenses should not be copied 100x.

XMP applies to several glTF concepts: asset, scene, mesh, material, image, texture.

If we're specifying attachment points here, texture seems redundant when image is available. Should node be included as well?

@lexaknyazev
Copy link
Member

perhaps make dc namespace a key

Following our recent discussions on extension design, I'd suggest against using nested objects.

@andreasplesch
Copy link
Contributor

I think nodes as a grouping mechanism would definitely benefit from being targeted. In fact, everywhere where extras is allowed this extension could apply. Otherwise, extras may be used instead by authors.

If attributing many meshes or images or materials is a concern, then being able to reference the same set of metadata becomes necessary. Perhaps by a short name rather than index but this is not important.

@emilian0
Copy link
Contributor Author

emilian0 commented Feb 1, 2019

@andreasplesch .

  • unordered vs ordered. ordered is when the order in the array matters (for instance creators are specified in order of importance). I tried to clarify that in the schema description but it is not enforced by the schema.
  • dc:date, I agree with you. The original specs for the (dc namespace)[https://github.com/adobe/xmp-docs/blob/master/XMPNamespaces/dc.md] isn't very clear either.
    The xmp namespace resolves that by adding the create and modify date. Should we consider adding the xmp namespace as well?

@andreasplesch @donmccurdy: I think restricting the extension makes sense if we only care about dc. I think we should talk more about this though. I wish I could cherry-pick properties from different namespaces (for instance, referencing the comment above, drop dc:date in favor of xmp:CreateDate xmp:ModifyDate). I also wonder if, in the future, we might need a glTF namespace.

@donmccurdy I am concerned about potential repetitions as well. Adding an indirection as you suggested could help. I almost wonder if we should take all these XMPs out of the json and place them in a buffer, worth considering?

@emackey
Copy link
Member

emackey commented Feb 2, 2019

Some random thoughts, take or leave:

  • Existing known metadata systems all place the metadata in the asset object and its children. Some exporters even place asset at the start of the JSON, to make it more user-discoverable. I think it's worth keeping with this convention, and making this an extension on assets. This would make it possible to keep all licensing info at the top of the file. It would also mean it needs a way of referencing arbitrary places in the glTF file, and for that I would suggest a list of JSON pointer strings per block of metadata (so one metadata block could reference several different images, and another block could reference others, etc.)

  • Might be nice to avoid the dc: XML-style prefixes/namespaces, and just cherry-pick the fields that make sense for glTF.

@andreasplesch
Copy link
Contributor

andreasplesch commented Feb 2, 2019

The problem with cherry-picking properties from namespaces or within a namespace is that the extension becomes decoupled from XMP. It becomes questionable to even keep XMP in the extension name as it would be just XMP inspired. But perhaps a home grown metadata system is what is best suited.

From what I read on XMP, there are general requirements for XMP conforming file readers and writers. For example, custom properties need to be allowed and preserved when writing back a file with embedded XMP. One idea of XMP seems to be that it should be possible to use a generic XMP reader to extract metadata from a file (a jpg or pdf), perhaps after some preprocessing. This is rdf based, and there is a rdf json encoding: https://dvcs.w3.org/hg/rdf/raw-file/default/rdf-json/index.html . But JSON-LD seems to supercede rdf-json. There is probably no appetite to explore RDF or JSON-LD.

@emilian0
Copy link
Contributor Author

emilian0 commented Feb 3, 2019

I side with @andreasplesch regarding cherry picking. Either we embrace XMP or we work on a simpler custom metadata extension. An in-between solution might bring more burden than good.

My opinion is that there is value in aligning glTF metadata to XMP to avoid another custom metadata language (this way XMP metadata can be transferred to a glTF, can be read and interpreted in the same way as for other types of assets and can be passed around).
We could change the specs to clarify that any XMP property can be encoded in a glTF. We can also add guidelines to clarify how to specify important metadata concepts (for instance use xmp:CreateDate to encode the creation date).
Perhaps this is worth discussing in a working group call before we make changes to the specs?

@emackey
Copy link
Member

emackey commented Feb 4, 2019

I don't have experience with XMP. Would current XMP parsers/readers be able to read from JSON inside the proposed extension here?

@andreasplesch
Copy link
Contributor

https://www.adobe.com/products/xmp.html is the main portal for XMP. That would be a question for the XMP community. There is a seemingly inactive forum and a SDK which is probably used by most XMP using applications. To me it looks the SDK is XML focused but the standard separates an abstract data model from serialization. meaning there is an opening for JSON serialization.

@andreasplesch
Copy link
Contributor

Some more digging found exiftool at https://sno.phy.queensu.ca/~phil/exiftool/ and https://github.com/mattburns/exiftool.js which apparently is popular metadata parser, and can deal with XMP.

@emilian0
Copy link
Contributor Author

emilian0 commented Feb 6, 2019

I don't have experience with XMP. Would current XMP parsers/readers be able to read from JSON inside the proposed extension here?

no current XMP parsers that I am aware of. But talking to the XMP community there appear to be an active effort to serialize XMP as JSON (ISO/PWI 16684-3).

@emilian0
Copy link
Contributor Author

emilian0 commented Feb 6, 2019

You probably noticed that we introduced a hack to the language alternative used to encode dc:rights so that it can be used to include spdx tags.
I suggest to remove the hack and add a glTF namespace and the glTF:SPDX property (that is the appropriate mechanism to extend xmp metadata).
Please let me know if you have concerns, thanks.

@donmccurdy
Copy link
Contributor

donmccurdy commented Feb 7, 2019

I'm still not sure I understand why the dc: namespace is necessary... if we're only using dc: properties, why not name the extension KHR_xmp_dc and drop the prefix? It's highly unusual in JSON, and adding a gltf: namespace exacerbates that... I'm not proposing that we cherry-pick properties, but that we omit what appears to be a redundancy.

I suggest to remove the hack and add a glTF namespace and the glTF:SPDX property (that is the...

If we go this route, consider khr:spdx also. Are XMP parsers case-sensitive? We should be strict about that.

@emilian0
Copy link
Contributor Author

emilian0 commented Feb 7, 2019

@donmccurdy I am leaning towards opening up the specs to allow for any XMP namespace in glTF, not only dc. This way:

  • we can use XMP namespaces/properties that are more appropriate to encode specific metadata ( xmp:CreateDate as an example). dc doesn't seem enough.
  • for tools that use XMP (like Adobe Dn/Pdf) we can transfer the full XMP metadata to/from the exported/imported glTF.

An alternative could be a custom metadata type inspired by dc and XMP (Andreas' comment). That would make the extension simpler and the interop with XMP harder.
Since our tools and formats work with XMP aligning with XMP makes sense to me, I wonder how the rest of the industry feels about it though.

khr:spdx works for me, we should get the blessing from the greater Khronos group.

@donmccurdy
Copy link
Contributor

Ok, if there's a possibility of including other namespaces in this extension, then I have no problem with the dc: prefix for now.

As to whether we should allow any XMP namespace, that's pretty broad yes? Would we normatively reference the XMP spec? And what's the process by which XMP is updated over time? There is definitely a lot of possible XMP metadata with no bearing on a 3D model...

I very much appreciate the prior work of XMP here, and I like the goal of striving for interoperability with it, but we should also recognize that the large majority of glTF i/o tools will not be bundling an XMP parser/writer, and simplicity has some value. :)

I don't have that much preference between gltf: and khr:, actually, just a suggestion.

@lexaknyazev
Copy link
Member

Using XMP as a normative reference would allow us not defining specific properties at all. glTF loaders should just skip those fields they don't understand (like extras).

@andreasplesch
Copy link
Contributor

Other defined XMP namespaces use more complex data types which would need to be converted to json. Ideally, this has been done elsewhere already but seems unlikely. Or the conversion could be left unspecified which would limit interoperability but may be ok within organizations.

@emilian0
Copy link
Contributor Author

Hi @donmccurdy , regarding the process by which XMP is updated over time.
My understanding is that the Khronos group will be in charge of maintaining the khr extension. Updating 3rd party extensions will require opening a conversation with the 3rd party / XMP ISO standard committee.

@andreasplesch I agree regarding type conversion. I took a look at the data types. Most of them can be boiled down to text(that includes url, GUID, Date). The XMP spec describes additional formatting for each type of text data, I would keep that out of the glTF schema, though, and just refer to it.
There are also simple properties like boolean, integer, real, array of text.
Language alternative and Resource Ref are non trivial. Language alternative is already in this pull request, we should add ResourceRef.
I believe that is it, anything else I am missing?

@lexaknyazev
Copy link
Member

Updating 3rd party extensions will require opening a conversation with the 3rd party / XMP ISO standard committee.

I think a KHR extension could just point to the relevant ISO standard so that updates are handled implicitly. Let's discuss it on the nearest call.

@@ -9,6 +9,7 @@
* [KHR_materials_pbrSpecularGlossiness](2.0/Khronos/KHR_materials_pbrSpecularGlossiness/README.md)
* [KHR_materials_unlit](2.0/Khronos/KHR_materials_unlit/README.md)
* [KHR_texture_transform](2.0/Khronos/KHR_texture_transform/README.md)
* [KHR_XMP](2.0/Khronos/KHR_XMP/README.md)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depending on when this pull request is merged, we might want to move this to the In progress Khronos and multi-vendor extensions section below - that way the PR can be merged before Khronos ratification is finished.

@emackey
Copy link
Member

emackey commented May 27, 2020

Please add the copyright appendix from the clearcoat extension, at the bottom with a link under the contributors.

@emilian0
Copy link
Contributor Author

emilian0 commented Jun 3, 2020

Hi @donmccurdy please let me know if this is sufficient bd4daa5 , thanks.

@weegeekps
Copy link

weegeekps commented Jun 4, 2020

I just opened a PR in the glTF-Sample-Models repository that contains a real-world example of the Damaged Helmet updated with KHR_xmp metadata.

cc: @emackey as I know you were interested in looking at a real-world example.

@neiltrevett
Copy link
Collaborator

Closing in on complete clarity on preferences around Adobe/ISO linking following working group discussions today. Our two goals are:

  • to not link to specs behind pay walls
  • to snapshot our own copy of the referenced XMP spec for stability and legal clarity

If we link to Adobe XMP materials to avoid the ISO pay wall - would we reference the:

GitHub repo: https://github.com/adobe/xmp-docs - which has a permissive outbound LICENSE.md which would seemingly let us fork a copy for stability - but...

are we actually linking to the Adobe XMP spec (which is presumably the equivalent to the ISO spec): https://wwwimages2.adobe.com/content/dam/acom/en/devnet/xmp/pdfs/XMP%20SDK%20Release%20cc-2016-08/XMPSpecificationPart1.pdf

The copyright notice on that spec prohibits reproduction and transmission - so we would not be able to fork/snapshot that spec without permission from Adobe after all?

Finally, we need to be very precise on what sections of the Adobe materials we are normatively referencing to minimize patent licensing obligations when we ratify the glTF extension spec. Have we done everything we can to precisely define our normative links to be as narrowly defined as possible?

@lrosenthol
Copy link

lrosenthol commented Jun 10, 2020 via email

@donmccurdy
Copy link
Contributor

Should we link to both, then? Even something like:

Specification ISO (Normative) Mirror 1 Mirror 2
XMP ISO-________ Adobe Khronos

@neiltrevett
Copy link
Collaborator

Hi @lrosenthol - firstly - thank you for all the great work you and and Adobe have put into the XMP spec - it is enabling - and we look forward to glTF becoming a part of the XMP family :)

However, the final version of 16684-1 has numerous editorial improvements and the recent
dated revision (16684-1:2019) has technical changes not reflected in the
Adobe document

Thank you for that clarity - which definitely changes the calculus - and I agree in that case we should reference the canonical ISO 16684-1:2019.

Given that many major international standards come from the ISO (eg. ISO
8601 - time & date formatting, ISO 10646 - Unicode, etc) and that those
are the documents that governments and regulated industries around the
world make official - this restriction seems pretty limiting

Khronos loves ISO! In fact we are working to bring glTF into a future PDF version :) I know from experience that we will get some push-back from the glTF community about the paywall - but it is just a minor irritant that we were hoping to avoid all other things being equal.

Have we done everything we can to precisely define our normative links to be as narrowly defined as possible?

This is another excellent reason to refer to the ISO documents, as they are
covered by the ISO IP policy and therefore there are no questions.

Agreed - the ISO license for XMP simplifies the patent license TO the glTF community. My comment was concerning the patent licensing FROM Khronos members. Links to external specs are considered normative when a Khronos specification is ratified under the Khronos IP framework - and so it is good practice to minimize Khronos member patent licensing obligations by precisely defining which sections of a external spec are being made normative - I want to check that we have gone through that exercise before we ratify.
@emilian0 can you confirm/comment?

As an aside - whether we end up referencing the complete ISO spec, or sections of it, the good news is that the Khronos ratification will provide additional patent protection for usage of ISO 16684-1:2019 in the context of glTF.

@neiltrevett
Copy link
Collaborator

Should we link to both, then? Even something like:

Specification ISO (Normative) Mirror 1 Mirror 2
XMP ISO-________ Adobe Khronos

If there are differences between the two specs I don't think we should link to both - it would create obvious legal and technical confusion

@lexaknyazev
Copy link
Member

If there are differences between the two specs I don't think we should link to both - it would create obvious legal and technical confusion

We should evaluate exact differences between Adobe and ISO published versions and see if those differences are relevant for glTF.

I know from experience that we will get some push-back from the glTF community about the paywall - but it is just a minor irritant that we were hoping to avoid all other things being equal.

While the original ISO-branded PDFs are behind the paywall, nothing prevents Khronos (or Adobe) from making and publishing a Markdown gist (without copy-pasting ISO language) that contains all the technical information needed for community implementations.

@neiltrevett
Copy link
Collaborator

While the original ISO-branded PDFs are behind the paywall, nothing prevents Khronos (or Adobe) from making and publishing a Markdown gist (without copy-pasting ISO language) that contains all the technical information needed for community implementations.

Interesting idea - we could include an informative link to the Gist in the spec - and would also help us clarify what sections of the underlying ISO spec need to be normative. @emilian0 what do you think?

@emilian0
Copy link
Contributor Author

Hi @neiltrevett
We reference 16684-1:2019 at the top of the KHR_xmp specs. My understanding is that we are ok keeping the link.

Thanks for bringing up the need for specifying which sections of external specs are normative, no I haven't gone through this exercise yet.
After a quick review of the specs:

  • XMP Core Properties` section references normative aspects of XMP (indeed the core properties definition). I will look up the exact paragraph.
  • We refer to the namespace concept several times pointing to this link. I will look up the exact paragraph in the external specs.
  • Section Transferring and merging metadata refers to the pantry - ingredient mechanism in xmpMM. I suggest to label this paragraph as non normative. The idea of KHR_xmp is to transmit XMP metadata via a glTF, but the actual details of how metadata can be merged or assembled is XMP business not glTF. I would still like to keep this section in the specs as non - normative since it is an important implementation detail.

For each normative reference I can point to the ISO paragraph.
Do we want to point to still point to the Adobe github in addition to that? (I can verify these are aligned)

@lexaknyazev @neiltrevett in regards to
While the original ISO-branded PDFs are behind the paywall, nothing prevents Khronos (or Adobe) from making and publishing a Markdown gist (without copy-pasting ISO language) that contains all the technical information needed for community implementations.
Isn't this what our link to Adobe XMP CoreProperties does?

@emilian0
Copy link
Contributor Author

@neiltrevett @lrosenthol @lexaknyazev @donmccurdy
I went through KHR_xmp and ISO 16684-1:2019 .
I added links to ISO paragraphs.
I removed the definitions of FrameCount, FrameRate, Part, ResourceEvent, ProperName, that don't find correspondence in the ISO specs (but are present in the Adobe links). I believe this change isn't impacting this extension.
Since I can't link directly to ISO paragraphs I preserved the links to the Adobe github specs.
Please let me know if you think this is a reasonable compromise (diffs f25f32d)
Thanks

@neiltrevett
Copy link
Collaborator

@emilian0
I think this is a good compromise - thanks for working through all those links :)

For complete clarity we should include a statement:

The referenced paragraphs of ISO 16684-1:2019 are normative and so are INCLUDED in the Scope of this Specification and may contain contributions from non-members of Khronos that are not covered by the Khronos Intellectual Property Rights Policy. The links to https://github.com/adobe/xmp-docs are purely informative and so are EXCLUDED from the Scope of this Specification, but are provided for convenience. The references to ISO 16684-1:2019 shall remain normative if there are differences to information provided at any informative links.

If you agree I suggest we this either in the copyright / license header.

@emilian0
Copy link
Contributor Author

Thanks @neiltrevett , updated at 8832254

@donmccurdy
Copy link
Contributor

@emilian0 could you rebase this PR and update the status in the spec? Thanks!

@emackey
Copy link
Member

emackey commented Aug 15, 2020

@donmccurdy No need for rebase, right? Let's just merge it and then update the status. I'm not a fan of squash/rebase, it destroys the history of how things evolved.

@donmccurdy
Copy link
Contributor

Hm, I was seeing this, and didn't think to click the dropdown:

Screen Shot 2020-08-14 at 7 05 28 PM

I guess my default is just set to "Rebase"? Note that "Rebase" still keeps history, as opposed to "Squash". But in any case, I'll merge it now. :)

@donmccurdy donmccurdy merged commit db78a82 into master Aug 15, 2020
@donmccurdy donmccurdy deleted the emiliano/KHR_XMP branch August 15, 2020 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension needs discussion Issue or PR requires working group discussion to resolve.
Projects
None yet
Development

Successfully merging this pull request may close these issues.