Read creation date from XMP sidecar too

[martyone]

Related to issue #682 . I was dealing with inconsistent video time handling among cameras and apps I use to manage photos. After all I resorted to fix the times manually and store the corrected times in sidecar files.

[bpatrik]

Thank you for your addition!

There as a bigger MetadataLoader.ts change with one the latest PR that conflicts with the PR. Can you please resolve the conflict?

[martyone]

Rebased.

The line endings in MetadataLoader.ts seems to have changed to CRLF by mistake meanwhile, hence the added commit to fix those. I don't like stealing the authorship this way, so please consider applying the fix for line endings on your own and I can rebase again.

Not sure why the test fails.

BTW master seems completely broken to me now, complaining about nonexistent column media.metadataCreationdateoffset IIRC, so I couldn't really test my rebased changes locally.

[grasdk]

Rebased.

The line endings in MetadataLoader.ts seems to have changed to CRLF by mistake meanwhile, hence the added commit to fix those. I don't like stealing the authorship this way, so please consider applying the fix for line endings on your own and I can rebase again.

Not sure why the test fails.

BTW master seems completely broken to me now, complaining about nonexistent column media.metadataCreationdateoffset IIRC, so I couldn't really test my rebased changes locally.

That was probably my mistake with CRLF. I switched between WSL and pure Windows. I did the other big PR with MetadataLoader. Thanks for fixing it.

This includes the new optional value and column creationDateOffset, which you should also read from the sidecar if available.
I guess there is actually some potential additional refactoring to be done in MetadataLoader, so sidecar data and embedded data use the same logic, but different sources.

The tests as such should not be broken, although there is a problem with SearchManager tests that I'm trying to reproduce locally.

@martyone: Perhaps you could add a test asset and a test in MetaDataLoader.spec.ts that will show that your commit is working as intended. I think there are currently zero tests about sidecar data.

[martyone]

Expanded to other time values as well.

I had some sidecars where some of the namespaces were not used, which threw exceptions in expressions like if ((sidecarData as SideCar).xmp.Rating !== undefined) .... As a result, the rest of the block with metadata processing was jumped over, possibly ignoring other available metadata. Hence the commit to uniformly check for all scopes.

I am not sure whether it was really that one of those namespaces were truly missing from the sidecar or it was the case that "xap" was present instead of "xmp" (an issue I only discovered later) but anyway I think it's good to support minimal sidecars too.

I do not see creationDateOffset in my sidecars. I need to think about tests yet.

[martyone]

Fixed linter issues

[grasdk]

I do not see creationDateOffset in my sidecars.

Sounds like you need a rebuild of the whole project in your work space ¯_(ツ)_/¯

I need to think about tests yet.

Sure. TDD is often hard, but in the end it saves you time, even if you have made a simple change. :)

If you care, I would suggest:

• Add some minimized files to test/backend/assets, maybe use a sub-folder for good measure like test/backend/assets/sidecar
• Copy and adapt some tests already in /test/backend/unit/model/threading/MetaDataLoader.spec.ts to work with your new test material.

I wouldn't mind helping with that if you can post some test-data here.

Regards :)

[martyone]

I wouldn't mind helping with that if you can post some test-data here.

Yeah that would be nice - I wasn't able to run the tests, so please see the WIP commit. The expected JSONs are missing.

I thought about preparing assets addressing particular issues to test, but after all I think its better to work with real world data (as closely as possible), each of which may suffer from more than one issue, so I think it's better not to aim for descriptive test case name etc. and simply iterate over all and check for stable results.

[grasdk]

@martyone added a PR to your branch that finalizes the nice test-material and tests you prepared:
martyone#1

Did not rebase, so there is still a bit of work left. Also borrowed a bit of code from: #839 for fixing the keyword reading.

[martyone]

@grasdk Rebased, squashed your JSONs addition with my commit. I omitted the fixes for keyword reading as those seem to be already in master.

Sadly I cannot test it right now. I did just blindly edit it. Maybe you could test it? It would be nice to have it merged soon - rebasing/resolving conflicts in these has been quite painful and I did it a number of times already.

[grasdk]

You moved some functions to Utils.ts. Understandable, but it doesn't build. I pulled your latest branch and fixed that locally, but currently have trouble running some of the tests from the last merge. Will update tomorrow I think.

[grasdk]

@martyone . Made another PR to your branch: martyone#2. The tests run on my machine after a complete rebuild: rm -rf node_modules/ && npm install && npm run build && npm run build-backend && npm run run-dev

[martyone]

I had to move it somewhere during the rebase to be able to accommodate my rebased commit to use that also from loadVideoMetadata.

Merged your commit, thank you.

[grasdk]

@martyone what's your take on sidecars in general:

1. If sidecar exists, only use it, not embedded metadata
2. If metadata exists, only use it, not sidecar
3. Merge embedded and sidecar. Embedded has precedence
4. Merge embedded and sidecar. Sidecar takes precedence.

I believe I read a comment from @bpatrik in favor of option 1. I think I agree, but in that case I will do some extra work after this is merged, so all the other fields relevant to pigallery2 can be read from sidecars.

[martyone]

@grasdk I would prefer option 4. Option 1 would be good for someone willing to hide some of the embedded metadata, which I consider a special case, which could be also achieved with option 4 by clearing the respective fields in the sidecar (field present, value empty). So option 4 seems more versatile to me. I don't really see a use case for options 2 and 3.

[rhatguy]

I would prefer option 4 also. In practice not all metadata gets written to sidecar immediately. I think most metadata could by synced by exiftool, but I'd prefer not to have to go through that extra step.

[bpatrik]

@martyone what's your take on sidecars in general:

1. If sidecar exists, only use it, not embedded metadata

2. If metadata exists, only use it, not sidecar

3. Merge embedded and sidecar. Embedded has precedence

4. Merge embedded and sidecar. Sidecar takes precedence.

I believe I read a comment from @bpatrik in favor of option 1. I think I agree, but in that case I will do some extra work after this is merged, so all the other fields relevant to pigallery2 can be read from sidecars.

I also prefer 4. Reasoning: There is almost always some data in the embedded, so lets use it if exists. i do not see a reason why not. If you take the effort to create a sidecar, then lets use whatever is in there.

[bpatrik]

Thank you for all this effort.
I'm starting to get lost in the state of the MetadataLaoder, but I will just trust the tests, that things will keep working :)