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

Minor xmp fixes #1009

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chkuendig
Copy link
Contributor

  • Don't write xmp sidecar for adjusted versions (some metadata like orientation only makes sense when applying to l the original file)
  • Some images (screenshots?) have a different 'adjustmentSimpleDataEnc' (plist instead oc compressed JSON) field. This led to a crash
  • One comment had the wrong indents

@AndreyNikiforov
Copy link
Collaborator

  • Some images (screenshots?) have a different 'adjustmentSimpleDataEnc' (plist instead oc compressed JSON) field. This led to a crash

For filenameEnc field we have to evaluate type subfield to get correct encoding. Does adjustmentSimpleDataEnc have type differentiating encoding too? We can use it to support encoding explicitly and skip unsupported types - probably good to do that for all enc fields to avoid breakages when Apple introduce new encoding.

@AndreyNikiforov AndreyNikiforov self-requested a review December 1, 2024 17:25
@@ -970,7 +970,7 @@ def download_photo_(counter: Counter, photo: PhotoAsset) -> bool:
download.set_utime(download_path, created_date)
logger.info("Downloaded %s", truncated_path)

if xmp_sidecar:
if xmp_sidecar and download_size != AssetVersionSize.ADJUSTED:
Copy link
Collaborator

Choose a reason for hiding this comment

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

How many XMP files we need for one asset downloaded in multiple sizes, e.g. original+alternative or original+live? For requested adjusted version we download original if adjusted does not exist yet (and there is complex logic for size fallback) - may be we need digging deeper into this ares? WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it might make sense to think this through better. Maybe only download the xmp for original? Generally since the XMP is based on the asset record, it should only be created once for a specific foto.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If only one xmp per asset, then we need some mechanism to ensure it is indeed once for any number of sizes fetched for asset.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this change. It's not critical compared to the crash when processing screenshots.

@chkuendig
Copy link
Contributor Author

For filenameEnc field we have to evaluate type subfield to get correct encoding. Does adjustmentSimpleDataEnc have type differentiating encoding too? We can use it to support encoding explicitly and skip unsupported types - probably good to do that for all enc fields to avoid breakages when Apple introduce new encoding.

Unfortunately for all fields ending in Enc the type subfield just says ENCRYPTED_BYTES which afaik can be any of the following three (maybe there's more):

  • Base64 encoded bytes for a unicode string
  • Base64 encoded bytes of a binary plist dict
  • Base64 encoded bytes of zlib compressed JSON

The Plist is easy to detect (it has specific header bytes), the other two can only be detected by decoding it (and in theory you could probably have a zlib compressed JSON where all bytes are valid unicode?).

@chkuendig chkuendig force-pushed the xmp-fixes branch 3 times, most recently from aff9bb6 to 8f6cd8e Compare December 21, 2024 21:30
@chkuendig
Copy link
Contributor Author

@AndreyNikiforov I removed the limitation on the AssetVersionSize so this can be unblocked. Maybe long term this is something to revisit, but it doesn't cause any issues as is I think (unlike the crash that's also fixed in this PR).

@AndreyNikiforov
Copy link
Collaborator

@AndreyNikiforov I removed the limitation on the AssetVersionSize so this can be unblocked. Maybe long term this is something to revisit, but it doesn't cause any issues as is I think (unlike the crash that's also fixed in this PR).

Great! Add notes to changelog, pls, so it is clear what is new for the next release

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