pulsejet / memories

Fast, modern and advanced photo management suite. Runs as a Nextcloud app.
https://memories.gallery
GNU Affero General Public License v3.0
3.11k stars 83 forks source link

Would you accept a pull request for Face Recognition integration? #146

Closed matiasdelellis closed 1 year ago

matiasdelellis commented 1 year ago

First of all, thank you very much for the application. Love it. 😄

On the other hand, I develop an unofficial facial recognition application for Nextcloud.

It has much more development, but it will never be accepted as an official part since they prefer the automatic approach beyond the quality of the results, but I don't want to make a controversy about it either. I am writing here to see if you would be willing to accept a PR adding support for this application.

Now i'm doing tests, just modifying the backend queries to reuse the frontend.

imagen imagen

However ideally i should add a new (and hidden by default) view, since the way it works is different (It is oriented to the faces, to avoid errors). imagen

pulsejet commented 1 year ago

Of course! In fact your app was my first choice for the integration, but feature parity with Photos had higher priority (which we almost have now).

There are just a few requirements before this can get merged:

  1. As you already noted, this needs to fit well into the TimelineQuery architecture. This should generally be pretty easy; I guess you already did most of this part.
  2. Basic feature parity with the recognize integration. So the face previews should work right (center the face), and things like renaming / merging (if you support this) face clusters from memories itself. Unfortunately, recognize uses a terrible webdav API to handle all this, so the two options we have are either having a similar webdav API for facerecognition, or duplicating some UI parts to call the existing API. I strongly prefer the latter since webdav sucks.
  3. We do need to support both backends. This is very easy -- if recognize is enabled with face recognition enabled, show it as People. If recognize isn't enabled or it's face feature is disabled, show faceregnition as People if enabled. If both apps are enabled, just have two tabs, People (recognize) and People (faces) (subtitle if possible). People might want to run both apps and compare the results (I do).

If you're going to work on this, do let me know in advance of your plan. That way I can avoid conflicting refactors.

matiasdelellis commented 1 year ago

Hi @pulsejet Thank you very much for allowing me to work on it... 😄

We agree on everything... Nextcloud took the unix saying, "everything is a file" to the extreme "everything is a dav entry". 🙈 😅

About 1.. I was pleasantly surprised how you implemented this backend, very efficient, and easy to extend.. 😄 As you say, it was the easy part and now I have something functional.

Now I stop hardcoding my own database, and try to add another TimelineQueryPersons to allow both applications independently. I could have added it to the same file, but I would be left with a lot of conditionals.

I'll see what I can reuse without recreating everything.

About 2...

Basic feature parity with the recognize integration.

Ok.. There are some things that are different, but we will have the same characteristics. 😉

So the face previews should work right (center the face)

Yes.. I like how you solved this. 😉

and things like renaming / merging (if you support this) face clusters from memories itself. Unfortunately, recognize uses a terrible webdav API to handle all this, so the two options we have are either having a similar webdav API for facerecognition, or duplicating some UI parts to call the existing API. I strongly prefer the latter since webdav sucks.

The philosophy of the application is that the clusters of faces are independent, and clusters with the same name are the same person. Beyond the technical, all this part is already solved using the side bar (See third image of the first comment..), and maybe in the implementation for this application we can simplify and stay with that.

About 3: We do need to support both backends. This is very easy -- if recognize is enabled with face recognition enabled, show it as People. If recognize isn't enabled or it's face feature is disabled, show faceregnition as People if enabled. If both apps are enabled, just have two tabs, People (recognize) and People (faces) (subtitle if possible).

Simply agree. 👍🏻

People might want to run both apps and compare the results (I do).

Just these days I'm testing it, and please, believe me I'm trying to be impartial. I still have to finish it, but see that.

To compare graphically, you can see these files.

If you're going to work on this, do let me know in advance of your plan. That way I can avoid conflicting refactors.

The only plan is to reuse as many components as possible. I'm still learning the code base.

Thanks again,

illnesse commented 1 year ago

Welp, time to try out facerecognition i guess! I'm already looking forward to your implementation in memories. Just a minor thing, i probably wouldn't rely on face classifier active toggle to display the people (recognize) navigation but just check count of clusters/faces in the db instead. I sometimes turn classification off when I get annoyed at recognize bugs 😏

rhatguy commented 1 year ago

Great to see these two projects coming together! Cheering you guys on!

pulsejet commented 1 year ago

The philosophy of the application is that the clusters of faces are independent, and clusters with the same name are the same person. Beyond the technical, all this part is already solved using the side bar (See third image of the first comment..), and maybe in the implementation for this application we can simplify and stay with that.

Fine by me as long as the UX stays easy-to-use.

Just these days I'm testing it, and please, believe me I'm trying to be impartial. I still have to finish it, but see that.

To compare graphically, you can see these files.

Interesting findings! I'm a bit surprised - used to think face recognition was a "solved" problem to a large extent; so TIL something.

jacotec commented 1 year ago

I was running @matiasdelellis face regognition before I have switched to Recognize just because it's perfectly embedded in Photos and Memories.

Mathias' face recognition is by far much much better than Recognize! I was shocked how many dozens (or even more) topics I have from the same person ... and gave up to merge them all to the same person.

After using Memories now for 3 weeks ... my two cents: This world does not need this original super slow "Photo" crap. And if Memories and Face Recogition are pairing, nobody needs Recognize as well :-P

@matiasdelellis The backdraw is that you need some advanced level to get this DLIB stuff installed. No chance for people running NC at a managed provider not having SSL/root access to the server. That prevents a simple installation from the app store. Do you see any chance to bundle the DLIB requirements into your app so for these people the installation is as simple as for Recognize? Otherwise @pulsejet must give the users the option which one to use, so users without root access can continue to use recognize and the geeks can use Face Recognition.

@matiasdelellis Another thing ... I know this will kill the name of your app ;-) ... but the object and landscape detection feature of recognize is pretty nice. I guess that's something you can also do (if you want) ... and I guess much better?

pulsejet commented 1 year ago

After using Memories now for 3 weeks ... my two cents: This world does not need this original super slow "Photo" crap. And if Memories and Face Recogition are pairing, nobody needs Recognize as well :-P

(If I'd be Frank Karlitschek I'd fire the team responsible for photos and hire @pulsejet ... ah no, that's Musk behavior ... so at least send my guys for a seminar to him to learn how to code efficiently)

That's a bit harsh. In all fairness, I don't believe there's anyone working on Photos full time, and this isn't surprising. I doubt anybody buys Nextcloud Enterprise for the Photos app, so it doesn't make any money. Honestly it's even a good thing -- I'd much rather have the Nextcloud team focus on performance and sync bugs than simpler things like a photos app, which can be handled by the community ;)

@matiasdelellis The backdraw is that you need some advanced level to get this DLIB stuff installed. No chance for people running NC at a managed provider not having SSL/root access to the server. That prevents a simple installation from the app store. Do you see any chance to bundle the DLIB requirements into your app so for these people the installation is as simple as for Recognize? Otherwise @pulsejet must give the users the option which one to use, so users without root access can continue to use recognize and the geeks can use Face Recognition.

I'm a bit partial towards recognize's approach to this problem. Basically call an external binary (node in recognize's case) and interact with it, so you can be completely independent of PHP (so no more issues with version etc either). That's also exactly what memories does with exiftool, and that solves a lot of problems.

@matiasdelellis Another thing ... I know this will kill the name of your app ;-) ... but the object and landscape detection feature of recognize is pretty nice. I guess that's something you can also do (if you want) ... and I guess much better?

+1 for this, even though I find this feature quite useless in practice :p

matiasdelellis commented 1 year ago

Hi @jacotec Thank you for your words... They help the spirit.. 😄

However, I agree with @pulsejet that we can't demand more from the people of Nextcloud. It is a very large product, which cannot satisfy all users. I am aware from minute one of the recognize errors, but I was never critical about it because we can't spit on the ceiling. All the errors that we can point out can be improved.

Does this report ring a bell? https://github.com/matiasdelellis/facerecognition/issues/114

We had the same problems. And the problem resulted in us using a model not recommended by the dlib developer himself. They now have the same problem, they chose the wrong tools...

And so it happens with many things, now they realized that they have to control the size of the images, or that it is relevant to expose the thershold configuration..

Now Face Recognition works better, but it's just that we have much more development, or just experience.. 😉

Quietly they can improve it... It's just a shame that they have announced it without even testing it in a real instance. 😞

@matiasdelellis Another thing ... I know this will kill the name of your app ;-) ... but the object and landscape detection feature of recognize is pretty nice. I guess that's something you can also do (if you want) ... and I guess much better? +1 for this, even though I find this feature quite useless in practice :p

I tried it a while ago, and the result, at least personally, was disappointing. I didn't test it again, because I'm against using nextcloud tags for that... 😞

I'm a bit partial towards recognize's approach to this problem. Basically call an external binary (node in recognize's case) and interact with it, so you can be completely independent of PHP (so no more issues with version etc either). That's also exactly what memories does with exiftool, and that solves a lot of problems.

Well, originally it used an external command.

...and now we also have an external model.

The problem is that when scaling the application, it is impossible to group the faces using only php, and giving an external program access to the database to do that would not be highly recommended. 😞

How long does it take recognize to clustering 5000 faces? At least a minute? Face Recognition does it in 5 seconds. Imagine with 200 thousand faces.. 🙈

matiasdelellis commented 1 year ago

p.s: when I published the app for the first time, the nextcloud guys announced Mail the next day, and to promote the app, they showed a video of how facial recognition was bad for users' privacy. 🤦🏻

At least it seems they changed their minds.. 😅

jacotec commented 1 year ago

That's a bit harsh. In all fairness, I don't believe there's anyone working on Photos full time, and this isn't surprising.

You're right. Reading it again after coming home, you're correct. I've edited it (however it's in your quote ;-) ). I did not want to offend anyone ... it's just that your is sooo much better and faster.

jacotec commented 1 year ago

@matiasdelellis Another thing ... I know this will kill the name of your app ;-) ... but the object and landscape detection feature of recognize is pretty nice. I guess that's something you can also do (if you want) ... and I guess much better? +1 for this, even though I find this feature quite useless in practice :p

I tried it a while ago, and the result, at least personally, was disappointing. I didn't test it again, because I'm against using nextcloud tags for that... 😞

That's a really good point ... my system is flooded with tags and it's even hard to find the tags I've defined for use. Maybe it'd make sense if the classification is really good ... Recognize does not the best job here.

The best approach would be to keep the classifications into the memories database to not flood the system with tags. But I think I'm shooting over the target right now ;-)

HyCore commented 1 year ago

You're right. Reading it again after coming home, you're correct. I've edited it (however it's in your quote ;-) ). I did not want to offend anyone ... it's just that your is sooo much better and faster.

true, crazy fast. i'm looking forward to remove completely the photo app embed into nextcloud and replace it with memory. just waiting for all the feature being added :D (ability the share multiple image in a click without having to create album, having access to the "shared with you" tab, and some other that i don't have in mind yet)

rhatguy commented 1 year ago

I'll second everyone else's comments here. I'm not currently running facerecognition because recognize and memories were so easy to setup. I thought I'd finally found a web based method of tagging I could live with. Unfortunately I've been so displeased with the quality of the matching (clustering people who are not the same person) that I've almost given up on recognize. Its also been running on my server for 1-2 weeks now. I recently increased the cron job size to 500 per cron run, but now nextcloud is complaining that sometimes cron jobs haven't executed for up to 4 hours (because recognize is taking so long to run through 500 pictures). This is on a dual Xeon Intel 6132 server, but it seems limited to 1 thread so recognition across my entire picture portfolio has taken forever and seems to have quite a few more days to go before it completes.

I ran one of the earlier version of facerecognition and I remember being impressed by the quality of the matches, however I couldn't deal with the user interface at that time. Memories now has the UI nailed so I'm looking forward to trying facerecognition WITH memories again in the future.

My preferred option is still to leverage my existing face tags I have in the pictures EXIF metadata (tagged with Picasa and Digikam) but it seems most software these days focuses on new web based tagging mechanisms and not reading existing data from pictures. I wish more tools would focus on portable metadata (hopefully stored in EXIF or sidecars) so as photo management tools come and go we could continue to leverage the metadata those tools have created. I can't complain much though because I haven't dedicated the time to work on a way to import that data into memories either. #127

jacotec commented 1 year ago

@matiasdelellis @pulsejet So, is this something you're gonna do to integrate together in Memories?

After looking closer into my "object detection" results, I've switched it off and hit the button to delete all the classification tags. The results are really not good. If this here becomes a project somewhere in the future, I'd do the same with Face Recognition. The clustering errors are insane. If I know that Mathias' Face recognition will find it's way into Memories one day, I'll switch that off in Recognize right now.

jacotec commented 1 year ago

@pulsejet It's awesome to see the integration with Face Recognition! But how does this work? There is nothing in your changelog how this should be enabled. I've disabled the "Recognize" App and Reenabled "Face Recognition". In my personal settings in "Face Recognition" I see the persons, but Memories (4.9.2) still does not have the "Persons" Menu ...?

pulsejet commented 1 year ago

@jacotec are you on the beta channel of face recognition? Only the latest beta is integrated.

jacotec commented 1 year ago

@pulsejet Awesome, that did the trick!

matiasdelellis commented 1 year ago

Probably, today I will publish a new version without the beta label..

I had things pending to fix, but it is better that they use what there is that works well instead of the previous version that fails only due missing css.. 🙄

Thanks for all,