fix: Prevent duplicates and clean up remaining near duplicates

lib/Classifiers/AbstractTaskProcessingClassifier.php

@@ -51,6 +51,17 @@ public function classify(array $queueFiles): void {
 			return;
 		}
 
+		$filesToProcess = $this->filterQueueFiles($queueFiles);
+		if (count($filesToProcess) !== count($queueFiles)) {
+			$skipped = array_values(array_filter($queueFiles, static fn (QueueFile $qf): bool => !in_array($qf, $filesToProcess, true)));
+			$this->logger->debug('Skipping ' . count($skipped) . ' already processed files from ' . $this->getModelName() . ' queue');
+			$this->dropFromQueue($skipped);
+		}
+		$queueFiles = $filesToProcess;
+		if (count($queueFiles) === 0) {
+			return;
+		}
+
 		$storageId = $queueFiles[0]->getStorageId();
 		$rootId = $queueFiles[0]->getRootId();
 		$userId = $this->findUserForStorage($storageId, $rootId);
@@ -88,6 +99,17 @@ public function classify(array $queueFiles): void {
 		$this->dropFromQueue($queueFiles);
 	}
 
+	/**
+	 * Hook for subclasses to skip queue files that don't need to be processed (again).
+	 * Files that are filtered out are removed from the queue.
+	 *
+	 * @param list<QueueFile> $queueFiles
+	 * @return list<QueueFile>
+	 */
+	protected function filterQueueFiles(array $queueFiles): array {
+		return $queueFiles;
+	}
+
 	private function findUserForStorage(int $storageId, int $rootId): ?string {
 		$mounts = array_values(array_filter(
 			$this->userMountCache->getMountsForStorageId($storageId),

lib/Classifiers/TaskProcessing/ImageFaceRecognitionClassifier.php

@@ -11,7 +11,13 @@
 
 use OCA\Recognize\Classifiers\AbstractTaskProcessingClassifier;
 use OCA\Recognize\Classifiers\Images\ClusteringFaceClassifier;
+use OCA\Recognize\Db\FaceDetectionMapper;
+use OCA\Recognize\Db\QueueFile;
+use OCA\Recognize\Service\QueueService;
 use OCA\Recognize\TaskProcessing\ImageFaceRecognitionTaskType;
+use OCP\Files\Config\IUserMountCache;
+use OCP\TaskProcessing\IManager as ITaskProcessingManager;
+use Psr\Log\LoggerInterface;
 
 final class ImageFaceRecognitionClassifier extends AbstractTaskProcessingClassifier {
 	public const MIN_FACE_RECOGNITION_SCORE = 0.5;
@@ -24,11 +30,40 @@ final class ImageFaceRecognitionClassifier extends AbstractTaskProcessingClassif
 	public const MAX_OVERLAP_NEW_CLUSTER = 0.1;
 	public const MIN_OVERLAP_EXISTING_CLUSTER = 0.5;
 
+	public function __construct(
+		LoggerInterface $logger,
+		ITaskProcessingManager $taskProcessingManager,
+		IUserMountCache $userMountCache,
+		QueueService $queue,
+		private FaceDetectionMapper $faceDetections,
+	) {
+		parent::__construct($logger, $taskProcessingManager, $userMountCache, $queue);
+	}
+
 	protected function getTaskTypeId(): string {
 		return ImageFaceRecognitionTaskType::ID;
 	}
 
 	protected function getModelName(): string {
 		return ClusteringFaceClassifier::MODEL_NAME;
 	}
+
+	/**
+	 * Skip files that already have face detections, so that re-crawling doesn't
+	 * create a second set of detections for the same faces.
+	 *
+	 * @param list<QueueFile> $queueFiles
+	 * @return list<QueueFile>
+	 */
+	protected function filterQueueFiles(array $queueFiles): array {
+		try {
+			$processed = array_flip($this->faceDetections->findFileIdsWithDetections(
+				array_map(static fn (QueueFile $qf): int => $qf->getFileId(), $queueFiles),
+			));
+		} catch (\OCP\DB\Exception $e) {
+			$this->logger->warning('Could not look up existing face detections; scheduling all files', ['exception' => $e]);
+			return $queueFiles;
+		}
+		return array_values(array_filter($queueFiles, static fn (QueueFile $qf): bool => !isset($processed[$qf->getFileId()])));
+	}
 }

lib/Db/FaceDetectionMapper.php

@@ -132,6 +132,31 @@ public function findByFileId(int $fileId): array {
 		return $this->findEntities($qb);
 	}
 
+	/**
+	 * Which of the given files already have at least one face detection (for any user)
+	 *
+	 * @param list<int> $fileIds
+	 * @return list<int>
+	 * @throws \OCP\DB\Exception
+	 */
+	public function findFileIdsWithDetections(array $fileIds): array {
+		$found = [];
+		foreach (array_chunk(array_values(array_unique($fileIds)), 1000) as $chunk) {
+			$qb = $this->db->getQueryBuilder();
+			$qb->selectDistinct('file_id')
+				->from('recognize_face_detections')
+				->where($qb->expr()->in('file_id', $qb->createNamedParameter($chunk, IQueryBuilder::PARAM_INT_ARRAY)));
+			$result = $qb->executeQuery();
+			/** @var list<array{file_id: int|string}> $rows */
+			$rows = $result->fetchAll();
+			$result->closeCursor();
+			foreach ($rows as $row) {
+				$found[] = (int)$row['file_id'];
+			}
+		}
+		return $found;
+	}
+
 	/**
 	 * @throws \OCP\DB\Exception
 	 * @return \OCA\Recognize\Db\FaceDetection[]

lib/Migration/RemoveDuplicateFaceDetections.php

@@ -30,58 +30,184 @@
 use OCP\Migration\IRepairStep;
 use Psr\Log\LoggerInterface;
 
+/**
+ * Removes face detections that describe the same face in the same file for the same user.
+ *
+ * Processing a file more than once (e.g. after a recrawl) yields a second set of detections
+ * whose bounding boxes differ only slightly from the first one, so detections are considered
+ * duplicates when their bounding boxes overlap almost completely, not only when they are identical.
+ */
 final class RemoveDuplicateFaceDetections implements IRepairStep {
+	/**
+	 * Minimum intersection over union of two bounding boxes to consider them the same face.
+	 * Two different faces in the same image practically never overlap this much.
+	 */
+	public const MIN_IOU = 0.9;
+
+	private const DELETE_CHUNK_SIZE = 1000;
+
+	/**
+	 * Approximate number of detections loaded into memory at once
+	 */
+	private int $batchSize = 10000;
 
 	public function __construct(
 		private IDBConnection $db,
 		private LoggerInterface $logger,
 	) {
 	}
 
+	/**
+	 * @internal Only meant for tests
+	 */
+	public function setBatchSize(int $batchSize): void {
+		$this->batchSize = max(1, $batchSize);
+	}
+
 	public function getName(): string {
 		return 'Remove duplicate face detections';
 	}
 
 	public function run(IOutput $output): void {
 		try {
-			$selectQuery = $this->db->getQueryBuilder();
-			$selectQuery
-				->select('file_id', 'user_id', 'x', 'y', 'height', 'width')
-				->selectAlias($selectQuery->func()->min('id'), 'min_id')
-				->from('recognize_face_detections')
-				->groupBy('file_id', 'user_id', 'x', 'y', 'height', 'width');
-
-			$result = $selectQuery->executeQuery();
-
-			$deleteQuery = $this->db->getQueryBuilder();
-			$deleteQuery->delete('recognize_face_detections')
-				->andWhere(
-					$deleteQuery->expr()->neq('id', $deleteQuery->createParameter('min_id')),
-					$deleteQuery->expr()->eq('file_id', $deleteQuery->createParameter('file_id')),
-					$deleteQuery->expr()->eq('user_id', $deleteQuery->createParameter('user_id')),
-					$deleteQuery->expr()->eq('x', $deleteQuery->createParameter('x')),
-					$deleteQuery->expr()->eq('y', $deleteQuery->createParameter('y')),
-					$deleteQuery->expr()->eq('height', $deleteQuery->createParameter('height')),
-					$deleteQuery->expr()->eq('width', $deleteQuery->createParameter('width'))
-				);
-
-			while (
-				/** @var array{min_id: int, file_id: int, user_id: string, x: float, y: float, height: float, width: float} $row */
-				$row = $result->fetch()
-			) {
-				$deleteQuery->setParameter('min_id', $row['min_id'], IQueryBuilder::PARAM_INT);
-				$deleteQuery->setParameter('file_id', $row['file_id'], IQueryBuilder::PARAM_INT);
-				$deleteQuery->setParameter('user_id', $row['user_id']);
-				$deleteQuery->setParameter('x', $row['x']);
-				$deleteQuery->setParameter('y', $row['y']);
-				$deleteQuery->setParameter('height', $row['height']);
-				$deleteQuery->setParameter('width', $row['width']);
-				$deleteQuery->executeStatement();
+			$removed = 0;
+			$lastFileId = -1;
+			do {
+				$upperFileId = $this->findBatchUpperBound($lastFileId);
+				$idsToDelete = $this->findDuplicateIds($lastFileId, $upperFileId);
+				foreach (array_chunk($idsToDelete, self::DELETE_CHUNK_SIZE) as $chunk) {
+					$deleteQuery = $this->db->getQueryBuilder();
+					$deleteQuery->delete('recognize_face_detections')
+						->where($deleteQuery->expr()->in('id', $deleteQuery->createNamedParameter($chunk, IQueryBuilder::PARAM_INT_ARRAY)));
+					$deleteQuery->executeStatement();
+				}
+				$removed += count($idsToDelete);
+				$lastFileId = $upperFileId;
+			} while ($upperFileId !== null);
+			if ($removed > 0) {
+				$output->info('Removed ' . $removed . ' duplicate face detections');
+				$this->logger->info('Removed ' . $removed . ' duplicate face detections');
 			}
-			$result->closeCursor();
 		} catch (\Throwable $e) {
 			$output->warning('Failed to automatically remove duplicate face detections for recognize.');
 			throw new \Exception('Failed to automatically remove duplicate face detections', previous: $e);
 		}
 	}
+
+	/**
+	 * Returns the file id that ends the batch of roughly batchSize detections starting after $lastFileId,
+	 * or null if the remaining detections fit into one batch. Uses the index on file_id.
+	 *
+	 * @throws \OCP\DB\Exception
+	 */
+	private function findBatchUpperBound(int $lastFileId): ?int {
+		$qb = $this->db->getQueryBuilder();
+		$qb->select('file_id')
+			->from('recognize_face_detections')
+			->where($qb->expr()->gt('file_id', $qb->createNamedParameter($lastFileId, IQueryBuilder::PARAM_INT)))
+			->orderBy('file_id')
+			->setFirstResult($this->batchSize)
+			->setMaxResults(1);
+		$result = $qb->executeQuery();
+		$fileId = $result->fetchOne();
+		$result->closeCursor();
+		return $fileId === false ? null : (int)$fileId;
+	}
+
+	/**
+	 * Finds duplicates among the detections with $lastFileId < file_id <= $upperFileId (no upper limit if null).
+	 * Whole files are always contained in one batch, so all detections of a (file, user) group are compared.
+	 *
+	 * @return list<int>
+	 * @throws \OCP\DB\Exception
+	 */
+	private function findDuplicateIds(int $lastFileId, ?int $upperFileId): array {
+		$selectQuery = $this->db->getQueryBuilder();
+		$selectQuery
+			->select('id', 'file_id', 'user_id', 'x', 'y', 'height', 'width', 'cluster_id')
+			->from('recognize_face_detections')
+			->where($selectQuery->expr()->gt('file_id', $selectQuery->createNamedParameter($lastFileId, IQueryBuilder::PARAM_INT)));
+		if ($upperFileId !== null) {
+			$selectQuery->andWhere($selectQuery->expr()->lte('file_id', $selectQuery->createNamedParameter($upperFileId, IQueryBuilder::PARAM_INT)));
+		}
+		$result = $selectQuery->executeQuery();
+
+		// Group in PHP instead of relying on the database's sort order, which depends on the collation of user_id
+		/** @var array<string, list<array{id: int, x: float, y: float, width: float, height: float, priority: int}>> $groups */
+		$groups = [];
+		while (
+			/** @var array{id: int|string, file_id: int|string, user_id: string, x: float|string, y: float|string, height: float|string, width: float|string, cluster_id: int|string|null}|false $row */
+			$row = $result->fetch()
+		) {
+			$groups[$row['file_id'] . '/' . $row['user_id']][] = [
+				'id' => (int)$row['id'],
+				'x' => (float)$row['x'],
+				'y' => (float)$row['y'],
+				'width' => (float)$row['width'],
+				'height' => (float)$row['height'],
+				'priority' => self::getPriority($row['cluster_id']),
+			];
+		}
+		$result->closeCursor();
+
+		$idsToDelete = [];
+		foreach ($groups as $group) {
+			array_push($idsToDelete, ...self::findDuplicatesInGroup($group));
+		}
+		return $idsToDelete;
+	}
+
+	/**
+	 * Ranks which detection to keep among duplicates: one assigned to a cluster (the user's person assignment),
+	 * then a rejected one (cluster_id = -1, so the user's rejection is retained), then an unclustered one.
+	 */
+	private static function getPriority(int|string|null $clusterId): int {
+		if ($clusterId === null) {
+			return 0;
+		}
+		return (int)$clusterId === -1 ? 1 : 2;
+	}
+
+	/**
+	 * Given all detections of one user in one file, returns the ids of those that duplicate another one.
+	 * Detections that are assigned to a cluster are preferred, then rejected ones, then older ones.
+	 *
+	 * @param list<array{id: int, x: float, y: float, width: float, height: float, priority: int}> $detections
+	 * @return list<int>
+	 */
+	private static function findDuplicatesInGroup(array $detections): array {
+		if (count($detections) < 2) {
+			return [];
+		}
+		usort($detections, static fn (array $a, array $b): int => [$b['priority'], $a['id']] <=> [$a['priority'], $b['id']]);
+		$kept = [];
+		$duplicates = [];
+		foreach ($detections as $detection) {
+			foreach ($kept as $keptDetection) {
+				if (self::isSameFace($detection, $keptDetection)) {
+					$duplicates[] = $detection['id'];
+					continue 2;
+				}
+			}
+			$kept[] = $detection;
+		}
+		return $duplicates;
+	}
+
+	/**
+	 * @param array{x: float, y: float, width: float, height: float, id?: int, priority?: int} $a
+	 * @param array{x: float, y: float, width: float, height: float, id?: int, priority?: int} $b
+	 */
+	public static function isSameFace(array $a, array $b): bool {
+		$intersectionWidth = min($a['x'] + $a['width'], $b['x'] + $b['width']) - max($a['x'], $b['x']);
+		$intersectionHeight = min($a['y'] + $a['height'], $b['y'] + $b['height']) - max($a['y'], $b['y']);
+		$intersection = max(0.0, $intersectionWidth) * max(0.0, $intersectionHeight);
+		$union = $a['width'] * $a['height'] + $b['width'] * $b['height'] - $intersection;
+		if ($union <= 0.0) {
+			// Degenerate boxes without area: only identical ones are duplicates
+			return abs($a['x'] - $b['x']) < 1e-9 && abs($a['y'] - $b['y']) < 1e-9
+				&& abs($a['width'] - $b['width']) < 1e-9 && abs($a['height'] - $b['height']) < 1e-9;
+		}
+		return $intersection / $union >= self::MIN_IOU;
+	}
 }

lib/TaskProcessing/TaskResultListener.php

@@ -197,10 +197,25 @@ private function applyFaceResults(array $fileIds, array $results): void {
 		// The mount tables are read for every file in this batch, so refresh them once here
 		// rather than on every lookup.
 		$this->userMountCache->clear();
+		// Files may have been processed by more than one task, e.g. when a recrawl was
+		// triggered while tasks were still pending. Only the first result is stored, as
+		// re-detected faces differ slightly in their coordinates and would end up as duplicates.
+		try {
+			$alreadyProcessed = array_flip($this->faceDetections->findFileIdsWithDetections($fileIds));
+		} catch (\Throwable $e) {
+			// Storing possible duplicates is better than losing the results; the
+			// RemoveDuplicateFaceDetections repair step can clean them up later
+			$this->logger->warning('Could not look up existing face detections', ['exception' => $e]);
+			$alreadyProcessed = [];
+		}
 		foreach ($fileIds as $i => $fileId) {
 			if (!isset($results[$i])) {
 				continue;
 			}
+			if (isset($alreadyProcessed[$fileId])) {
+				$this->logger->debug('File ' . $fileId . ' already has face detections, discarding new face results');
+				continue;
+			}
 			$raw = (string)$results[$i];
 			$userIds = $this->getUsersWithFileAccess($fileId);
 			foreach (explode("\n", $raw) as $line) {