Skip to content

Add a metadata service to store file metadata - #31839

Merged
PVince81 merged 2 commits into
masterfrom
feat/metadata-server
Apr 14, 2022
Merged

Add a metadata service to store file metadata#31839
PVince81 merged 2 commits into
masterfrom
feat/metadata-server

Conversation

@CarlSchwan

@CarlSchwanCarlSchwan commented Apr 4, 2022

Copy link
Copy Markdown
Member

@CarlSchwanCarlSchwan added the 2. developing Work in progress label Apr 4, 2022
@CarlSchwanCarlSchwan added this to the Nextcloud 24 milestone Apr 4, 2022
@CarlSchwanCarlSchwan self-assigned this Apr 4, 2022
@CarlSchwan
CarlSchwan marked this pull request as draft April 4, 2022 21:16
@blizzzblizzz mentioned this pull request Apr 7, 2022
@CarlSchwan
CarlSchwanforce-pushed the feat/metadata-server branch from 7879192 to 9586920CompareApril 8, 2022 13:54
@CarlSchwan
CarlSchwan marked this pull request as ready for review April 8, 2022 14:37
Comment threadapps/dav/lib/Connector/Sabre/FilesPlugin.php
Comment threadcore/Application.php Outdated
Comment threadcore/Migrations/Version240000Date20220404230027.php Outdated
Comment threadcore/Migrations/Version240000Date20220404230027.php Outdated
Comment threadlib/private/Metadata/Provider/ExifProvider.php Outdated
Comment threadlib/private/Server.php Outdated
Comment threadlib/private/Metadata/MetaDataManager.php Outdated
Comment threadlib/private/Metadata/MetaDataManager.php Outdated
Comment threadlib/private/Metadata/IMetadataManager.php Outdated
Comment threadlib/private/Metadata/FileMetadataMapper.php Outdated
Comment threadlib/private/Metadata/FileEventListener.php
Comment threadlib/private/Metadata/Provider/ExifProvider.php Outdated
Comment threadlib/private/Files/Cache/Scanner.php Outdated
Comment threadlib/private/Files/Cache/Scanner.php Outdated
Comment threadlib/private/Files/Cache/Scanner.php Outdated
@juliusknorr

Copy link
Copy Markdown
Member

Just curious, is searching for files by metadata something in scope? I could imagine this a useful scenario for future enhancements of the photos app to search by location for example, as with JSON-only metadata db field that would not scale.

@CarlSchwan

Copy link
Copy Markdown
MemberAuthor

Just curious, is searching for files by metadata something in scope? I could imagine this a useful scenario for future enhancements of the photos app to search by location for example, as with JSON-only metadata db field that would not scale.

Unfortunately, this seems to be a bit tricky. If we want to search photos by location in an efficient way we need more than just the simple GPS coordinate but we need to map this to a geographic area. This is a bit tricky to implement but we can inspire outself from something similar I have in Koko: https://invent.kde.org/graphics/koko/-/blob/master/src/reversegeocoder.cpp#L30 (this requires to be done in a background job as loading the data is quite expensive already in pure C/C++ with an optimized kdtree data structure)

This geographic area should probably be stored in a separate table e.g. https://invent.kde.org/graphics/koko/-/blob/master/src/imagestorage.cpp#L132

But the biggest issue is that while this is fairly simple to implement in the one user use-case, for a Nextcloud we need to retrieve the list of locations or the list of photos in one location on a per user basis and this is really tricky unless we limit ourself only to files stored in the user storage (so no group folders, no sharing, no circles ,...). @PVince81@AndyScherzinger Would this is be ok?

In that case, we could adopt the following DB structure for location information.

CREATETABLEoc_file_metadata (
id INTEGER,
group TEXT,
data TEXT, # nullable
foreign INTEGER, # nullable
UNIQUE(id, group)
)
CREATETABLEoc_location (
id INTEGERPRIMARY KEY,
country TEXT,
state TEXT,
city TEXT,
UNIQUE(country, state, city)
)

The PHP API needs to be expanded to describe if the data is directly retrievable in the JSON data column, or to be joined with another table (e.g for locations with the oc_location table). To be able only to retrieve the locations for one user, it should be possible to join with oc_storage and oc_filecache to get only the ids of files from the user. This might still be expensive when a user has a log of photos but I don't really see a better way to do it, unfortunately :(

@AndyScherzinger

Copy link
Copy Markdown
Member

I think the location data needs a more detailed discussion, so from my perspective we can skip that for now. The only thing we "need" for 24 is height/width for the Clients to implement a different layout for their media views. (kinda like a gallery).

So if there if leaving location out doesn't create a future blocker just skip it for now. Because this needs a lot more thinking from my perspective because with with/height it is just extraction+exposure, for anything else it is likely more than that, like "places" (like you also outlined), "people" only other simple info I can come up with would be certain exif data (like exposure, aperture, cam-model, lense) but since we also don't make use of that for now, this can also be skipped.

So from my point of view for v24 - keep it simple by limiting it to width/height only. What do you think @PVince81 ?

@PVince81

Copy link
Copy Markdown
Member

yes, keep it simple for now.

if we need to cache/compute more data for GPS search later, it will be a separate task anyway

@PVince81

Copy link
Copy Markdown
Member

had a quick chat with @CarlSchwan about the location data:

  1. I don't think it's a good idea to link oc_location to oc_file_metadata because the latter table also contains non-GPS data, so it feels a bit weird to link them there

  2. an alternative would be to connect oc_locations with oc_filecache. Since it's a 1-N relationship, we'd need a new table oc_location_mapping or similar to connect: oc_filecache->oc_location_mapping->oc_locations. This way it becomes possible to find all file ids for a specific location

We both agreed that 2) is a better approach and will remove the need to add anything else to oc_file_metadata, thus deblocking this PR.

@tobiasKaminsky

Copy link
Copy Markdown
Member

@CarlSchwan can I test this somehow already?
How can I regenerate metadata for existing images?

Comment threadlib/private/Files/Cache/Scanner.php Outdated
@CarlSchwan

Copy link
Copy Markdown
MemberAuthor

@CarlSchwan can I test this somehow already? How can I regenerate metadata for existing images?

Just run the branch and you should be able to see the metadata with the PROPFIND/SEARCH parameter.

Unfortunately, the metadata regeneration job is not implemented yet and is task for another PR (probably not 24.0.0) so you need to handle the case where the metadata are not available

@tobiasKaminsky

Copy link
Copy Markdown
Member

Unfortunately, the metadata regeneration job is not implemented yet and is task for another PR (probably not 24.0.0) so you need to handle the case where the metadata are not available

So it might be that clients receive e.g. 500 images, but only 200 images have a valid metadata.
Is this something clients can now before?
Media searches are done in batches, so it might be that first batch has valid metadata and thus client shows new view (which uses metadata), but then second batch is without valid metadata and thus the view is mixed up…

@jancborchardt ^ as we spoke in our call about it, but I have no idea how to display this hybrid view

@jancborchardt

Copy link
Copy Markdown
Member

@CarlSchwan@PVince81 btw what is the reason for the metadata capability being optional? Customer requirement (don’t see that in the customer issue), not running on all setups, or something else?
(Just asking cause wouldn’t it be easier and more consistent to just always have it?)

@PVince81

Copy link
Copy Markdown
Member

@CarlSchwan@PVince81 btw what is the reason for the metadata capability being optional? Customer requirement (don’t see that in the customer issue), not running on all setups, or something else? (Just asking cause wouldn’t it be easier and more consistent to just always have it?)

it's enabled by default.
the config option is like for previews to be able to disable it if there are use cases where metadata is not relevant, or for performance concerns

@CarlSchwan

Copy link
Copy Markdown
MemberAuthor

@CarlSchwan@PVince81 btw what is the reason for the metadata capability being optional? Customer requirement (don’t see that in the customer issue), not running on all setups, or something else? (Just asking cause wouldn’t it be easier and more consistent to just always have it?)

Also we want to support in the future the possibility for apps to provide their own metadata extractor and dynamically indicate in the capability that metadata is available for each mimetype

Comment threadlib/private/Metadata/IMetadataProvider.php Outdated

@PVince81PVince81 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

please see my one comment about the interface

Comment threadlib/private/Metadata/IMetadataProvider.php
@CarlSchwan
CarlSchwanforce-pushed the feat/metadata-server branch from 1df6a0a to e5b79f1CompareApril 13, 2022 08:52
@blizzzblizzz mentioned this pull request Apr 13, 2022
@tobiasKaminsky

Copy link
Copy Markdown
Member

@CarlSchwan if there is none metadata, "[ ]" is returned, but with metadata it is:
{"width":960,"height":1280}

Json parser gets a hiccup here, as it expects "{}" in case of empty.

@CarlSchwan
CarlSchwanforce-pushed the feat/metadata-server branch from e5b79f1 to 3b1d4d1CompareApril 13, 2022 11:46
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@CarlSchwan
CarlSchwanforce-pushed the feat/metadata-server branch from 3b1d4d1 to 09bf9cfCompareApril 13, 2022 12:24
@CarlSchwan

Copy link
Copy Markdown
MemberAuthor

@CarlSchwan if there is none metadata, "[ ]" is returned, but with metadata it is: {"width":960,"height":1280}

Json parser gets a hiccup here, as it expects "{}" in case of empty.

@tobiasKaminsky this should be fixed. Strange that the json parser doesn't support [] :(

Comment threadapps/dav/lib/Connector/Sabre/FilesPlugin.php Outdated
Comment threadapps/dav/lib/Files/FileSearchBackend.php
And update autoloader
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@CarlSchwan
CarlSchwanforce-pushed the feat/metadata-server branch from 7fdad3f to 1c7ecfcCompareApril 14, 2022 10:11
@PVince81
PVince81 merged commit 3ca7971 into masterApr 14, 2022
@PVince81
PVince81 deleted the feat/metadata-server branch April 14, 2022 11:39
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2. developingWork in progress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@CarlSchwan@juliusknorr@AndyScherzinger@PVince81@tobiasKaminsky@jancborchardt@icewind1991@miaulalala