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

Add XMP sidecar support in backend for video files #761

Merged
merged 7 commits into from
Nov 16, 2023

Conversation

grahamalderson
Copy link
Contributor

This pull request adds xmp sidecar support using exifr in order to gain access to keywords metadata for video files. It can be extended to collect additional data for videos or photos and is a first step towards reducing the number of exif readers per #277 .

This is a first step towards #156
Exifr was chosen per #277
Work accelerated by reviewing exiftool code from @mcdamo on #294

Known limitation of this pull request:
-Keywords for videos do not show up in the info panel but tagged videos are returned when you search for the keyword.

It's unclear to me why the tags do not show up in the info panel on the videos. If you search for keyword:test or keyword:rabbit you will get back the bunny.mp4 video, suggesting they are correctly recorded in the database.

I attempted modification of frontend files to identify video vs photo and load metadata separately for them on the info panel believing that to be the issue, but the problem was deeper. When logged to console in the browser, metadata.keywords was showing as undefined. Given this, it's not clear my modifications to the frontend were even necessary, and I have not included them. I would be happy to look at it further with some pointers on why metadata.keywords is returning undefined on the frontend for videos despite them returning in keyword search results.

@bpatrik
Copy link
Owner

bpatrik commented Nov 15, 2023

Hi,
first thank you for the contribution. This is a long outstanding issue. I wanted to refactor the whole metadata loader class, but never got there.

Unfortunately exifr is not maintained anymore https://github.com/MikeKovarik/exifr (last commit 2 years ago)

I'm still fine with this contribution at this point as it at least move forward the issue.

You only implemented sidecars for videos. They wont be loaded for jpgs. Can you also add that?

Video keywords are not showing up as they ar filtered out at server side:

delete (m as PhotoDTO).metadata.keywords;

@grahamalderson
Copy link
Contributor Author

grahamalderson commented Nov 15, 2023

I missed that exifr was no longer maintained, that's too bad. I had been planning to slowly work through refactoring metadataloader and implement exifr for everything, but I guess that's not a good idea at this point so I won't proceed.

Thanks for the pointer on the contentwrapper, I think I can get that done today or tomorrow.

I'm happy to give sidecars for photos a try in this PR as well. I had been planning to do that in the future. It wasn't clear to me if that was something that was widely used since XMP can be contained in the photo file, so I didn't prioritize it.

If we see different values between the metadata in the photo file and the metadata in the sidecar, which should we prefer? It's less of an issue while I'm just working on keywords right now since we can just concatenate them, but for the next step as sidecar support is extended, what if we see conflicting values?

I'll submit a future PR to extend the sidecar support beyond keywords.

Thanks for building this and the quick response!

Removed metadata.keywords from contentwrapper deletion list
@grahamalderson
Copy link
Contributor Author

@bpatrik Outstanding issues resolved.
-video keywords now show up on frontend
-sidecar support extended to photos same as in videos

This PR is still limited to support for keywords in sidecars, but can be extended further in a future PR

@bpatrik
Copy link
Owner

bpatrik commented Nov 16, 2023

Thank you very much!

It's less of an issue while I'm just working on keywords right now since we can just concatenate them, but for the next step as sidecar support is extended, what if we see conflicting values?

Sidecar should override the photo. I think that is expected if you have an explicit sidecar next to your photo that it is there for a reason and should be used as the authoritative source of truth. If you want to make it even better, you can create a settings for override, skip,merge to support all possible scenarios. But I do not find that necessary for now.

@bpatrik bpatrik merged commit 2f75894 into bpatrik:master Nov 16, 2023
for (const sidecarPath of sidecarPaths) {
if (fs.existsSync(sidecarPath)) {
const sidecarData = exifr.sidecar(sidecarPath);
sidecarData.then((response) => {
Copy link
Owner

Choose a reason for hiding this comment

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

It occurred to me that this is an async call that you are not waiting for.

So there might be some race condition when the sidecar won't be properly loaded.

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