Conversation
|
Have you done any testing of this? In theory it sounds fine, but it would be great to have some hard numbers on files that previously collided that were resolved by this approach. Also, all stash-boxes need to purge fingerprints based on this, otherwise they'll be left with stale hashes. Ideally stash-box should reject hash submissions from old stash versions when the duration is <= 150s. EDIT: The way I would test this is find someone with a lot of short duration videos, have them generate phashes the old way, then generate phashes the new way. Then hand the list of scene_id,old_hash,new_hash to claude or whatever and have it map out the collisions on distance 8/6/4. That should quickly give you an idea if accuracy has improved or not. |
| // Short videos use fewer frames: sampled close together they are near-identical, | ||
| // and the repetitive sprite that results hashes alike across unrelated videos. | ||
| // See https://github.com/stashapp/stash/issues/3722. | ||
| func spriteColumns(duration float64) int { |
There was a problem hiding this comment.
Is it spriteColumns if it determins both row and column count? Sounds more like grid size or grid dimension.
There was a problem hiding this comment.
True enough, can change up the name.
|
I just tested it with a few random sub 150 second clips and it seemed okay. I could try and get a copy of someone's database who has a bunch of short clip for better testing data. |
I strongly dislike the forced deletion of existing pHash fingerprints outright. This would effectively render many scenes on FansDB stash-box dead (it's the only reliable parameter we have for matching content) and would require purging them not just fingerprints unless new ones are resubmitted from existing users. Considering that scene tagger is the only way Stash allows submitting fingerprints I don't see a case where many of them get back to us. I feel like the old fingerprints should only be removed when new one is generated and the user is prompted to resumit it back to the stash-box instances for which user has StashID connected. |
|
So, I was mostly just basing this off of WP but the comment is several years old and I not sure if FansDB was active then. I'm open to discussion on how the handle the situation again since that comment is rather old. I'm not too fond of keeping bad data but I'm honestly not sure of the "best" way to handle it. |
|
@InfiniteStash So, I did some more testing with another, larger, DB of short clips and most of the logic is fine. The only real thing I can't test is the regeneration stuff. I currently don't have any files to compare so unless someone like @DogmaDragon is willing to spin up another version and take down the data I'm not quite sure how to do it. |
|
Back in the day I was stoked about FansDB coming online and convinced that this short video phash issue would be a major problem. So I was motivated to spend all that time researching and testing to mitigate it. At the time Stash-box code was regularly going 6+ months between minor updates, so I aimed to try and solve everything solely on the Stashapp side. Maybe these days a combined approach would be more useful. Per @InfiniteStash , I'd hope that by now an admin should be able to query the FansDB back-end and get some real metrics on whether this did turn out to be an actual problem that needs to be solved, or if it's just a minor annoyance like in StashDB. It's hard to believe all the time I invested hand coding Stashapp/box back in the day and how it would take a fraction of the time now. But for all my PR's that got abandoned after I gave up, this was the one I had hoped would be the most useful for the future of Stash. |
|
Rather than forking PHASH just for <=150s videos, I'm looking into if there's improvements that can be made to PHASH as a whole. Introducing a PHASHv2 or whatever makes the migration path clearer, and means we don't have to purge the old ones. |
|
I mean, if we want to keep it really simple we could just not delete old PHashes and then going forward all submissions to fansDB will be the new ones but it allows backwards compatibility. I still don't love the idea of keeping bad data but I also don't want to leave a bunch of users high and dry. |
I don't really see it as bad data. It's valid, but limited use data since it can only be used by people who already have this pHash.
Not deleting is also not ideal. Sure it offers backwards compatabily, but it prevents forwards compatability as new users won't be able to find existing scenes since they will only have the new pHash. I have no idea how to handle this technologically, but effectively what I would want is for everyone to be offered to re-generate and re-submit and then replace old pHash with new in a single task, similar in how other maintenence tasks work. This would preserve backwards compatability and enable user friendly option for forwards compatability. Directing people to a simple button that runs a task is a lot easier than requiring people to go scene by scene to rescrape and then submit newly generated pHash fingerprints. |
Description
This PR expands the PHash calculation logic to be based on scene length. Shorter scene have less frames and the PHashs can get mucked up due to it. This also adds migration that will delete all PHashs for any scene that is less than 2.5 minutes and require the user to recalculate them.
Related Issue
closes #3722
supersedes: #4074
Testing
Ran a dev env with a few files that are less than 2.5 mins with phash. Upgraded to the new version and saw that the appropraite phashes were deleted and anything greater than 2.5 mins wasn't touched.
Screenshots
N/A
Checklist
AI Usage Disclosure
Claude Code Opus drafted it up.
Additional Context