refactor: metadata extraction #12359
Conversation
| // make sure dates comes from sidecar | ||
| const sidecarDate = firstDateTime(sidecarTags as Tags, EXIF_DATE_TAGS); | ||
| if (sidecarDate) { | ||
| for (const tag of EXIF_DATE_TAGS) { | ||
| delete mediaTags[tag]; | ||
| } |
There was a problem hiding this comment.
It doesn't seem desirable that it'll use a lower-priority date-time tag from the sidecar over a higher-priority one in the original.
There was a problem hiding this comment.
This is actually very much on purpose. We allow changing the date time in immich and we write that to a sidecar file. We always write DateTimeOriginal. Images with higher priority fields, like SubSecDateTimeOriginal would be impossible to overwrite.
There was a problem hiding this comment.
I see, but there are also tags below DateTimeOriginal that would get used instead of it. It's confusing behavior.
There was a problem hiding this comment.
An interesting option would be to write both DateTimeOriginal and a custom tag like ImmichDateTimeOriginal. That tag would be the highest priority during metadata extraction if it exists. Probably out of scope for this PR, though.
There was a problem hiding this comment.
Nah, I think tags are confusing enough and the point is to be interoperable. We could potentially only remove tags from the sidecar is we find DateTimeOriginal in the sidecar. At the moment the only tag with higher priority than that is the sub-second one.
There was a problem hiding this comment.
For me at least, if you have a reliable date time tag in the xmp file we should use that always. I agree that maybe we should potentially ignore the low quality ones like CreateDate though.
| return null; | ||
| }) as Promise<ImmichTags | null>; | ||
| return {}; |
There was a problem hiding this comment.
Not sure how to feel about this. An empty object implies that it read the file successfully and there was just nothing there. If there was an issue reading it (permissions, etc.), it's not the same as it having no metadata.
There was a problem hiding this comment.
The confusing part is probably the service should catch and ignore the error, not the repository. We don't actually ever differentiate or do anything in the failure case anyways, so it doesn't matter much if it is here or in the service.
There was a problem hiding this comment.
Yeah, it'd be nicer for the empty object fallback to live in the service.
There was a problem hiding this comment.
I have a list of things I am working through with regards to the metadata service file. I can make a note to move the try/catch/suppression to the service and write some additional tests for those scenarios in another PR.
jrasm91
force-pushed
the
refactor/metadata-extraction
branch
from
ef8e88c
to
2f5519f
Compare
jrasm91
force-pushed
the
refactor/metadata-extraction
branch
from
2f5519f
to
ad5d735
Compare
ImmichTagsandReverseGeocodeResultfrom the repositories