diff --git a/mobile/lib/providers/asset_viewer/download.provider.dart b/mobile/lib/providers/asset_viewer/download.provider.dart index 25db76b077..95dfed0011 100644 --- a/mobile/lib/providers/asset_viewer/download.provider.dart +++ b/mobile/lib/providers/asset_viewer/download.provider.dart @@ -3,7 +3,6 @@ import 'dart:async'; import 'package:background_downloader/background_downloader.dart'; import 'package:hooks_riverpod/hooks_riverpod.dart'; import 'package:immich_mobile/models/download/download_state.model.dart'; -import 'package:immich_mobile/models/download/livephotos_medatada.model.dart'; import 'package:immich_mobile/services/download.service.dart'; class DownloadStateNotifier extends StateNotifier { @@ -17,9 +16,10 @@ class DownloadStateNotifier extends StateNotifier { taskProgress: {}, ), ) { - _downloadService.onImageDownloadStatus = _downloadImageCallback; - _downloadService.onVideoDownloadStatus = _downloadVideoCallback; - _downloadService.onLivePhotoDownloadStatus = _downloadLivePhotoCallback; + // TODO(agg23): These callbacks were overriden and never actually ran. Fix/remove as necessary + // _downloadService.onImageDownloadStatus = _downloadImageCallback; + // _downloadService.onVideoDownloadStatus = _downloadVideoCallback; + // _downloadService.onLivePhotoDownloadStatus = _downloadLivePhotoCallback; _downloadService.onTaskProgress = _taskProgressCallback; } @@ -42,6 +42,7 @@ class DownloadStateNotifier extends StateNotifier { } // Download live photo callback + // ignore: unused_element void _downloadLivePhotoCallback(TaskStatusUpdate update) { _updateDownloadStatus(update.task.taskId, update.status); @@ -50,8 +51,6 @@ class DownloadStateNotifier extends StateNotifier { if (update.task.metaData.isEmpty) { return; } - final livePhotosId = LivePhotosMetadata.fromJson(update.task.metaData).id; - _downloadService.saveLivePhotos(update.task, livePhotosId); _onDownloadComplete(update.task.taskId); break; @@ -61,12 +60,12 @@ class DownloadStateNotifier extends StateNotifier { } // Download image callback + // ignore: unused_element void _downloadImageCallback(TaskStatusUpdate update) { _updateDownloadStatus(update.task.taskId, update.status); switch (update.status) { case TaskStatus.complete: - _downloadService.saveImageWithPath(update.task); _onDownloadComplete(update.task.taskId); break; @@ -76,12 +75,12 @@ class DownloadStateNotifier extends StateNotifier { } // Download video callback + // ignore: unused_element void _downloadVideoCallback(TaskStatusUpdate update) { _updateDownloadStatus(update.task.taskId, update.status); switch (update.status) { case TaskStatus.complete: - _downloadService.saveVideo(update.task); _onDownloadComplete(update.task.taskId); break; diff --git a/mobile/lib/providers/infrastructure/action.provider.dart b/mobile/lib/providers/infrastructure/action.provider.dart index ed62b9a0e8..d1b2c3af21 100644 --- a/mobile/lib/providers/infrastructure/action.provider.dart +++ b/mobile/lib/providers/infrastructure/action.provider.dart @@ -1,7 +1,6 @@ import 'dart:async'; import 'package:auto_route/auto_route.dart'; -import 'package:background_downloader/background_downloader.dart'; import 'package:flutter/material.dart'; import 'package:hooks_riverpod/hooks_riverpod.dart'; import 'package:immich_mobile/constants/enums.dart'; @@ -10,7 +9,6 @@ import 'package:immich_mobile/domain/models/asset/base_asset.model.dart'; import 'package:immich_mobile/domain/models/asset_edit.model.dart'; import 'package:immich_mobile/domain/services/asset.service.dart'; import 'package:immich_mobile/domain/services/remote_album.service.dart'; -import 'package:immich_mobile/models/download/livephotos_medatada.model.dart'; import 'package:immich_mobile/providers/asset_viewer/asset_viewer.provider.dart'; import 'package:immich_mobile/providers/backup/asset_upload_progress.provider.dart'; import 'package:immich_mobile/providers/infrastructure/album.provider.dart'; @@ -23,7 +21,6 @@ import 'package:immich_mobile/providers/user.provider.dart'; import 'package:immich_mobile/providers/websocket.provider.dart'; import 'package:immich_mobile/routing/router.dart'; import 'package:immich_mobile/services/action.service.dart'; -import 'package:immich_mobile/services/download.service.dart'; import 'package:immich_mobile/services/foreground_upload.service.dart'; import 'package:immich_mobile/utils/semver.dart'; import 'package:immich_mobile/widgets/asset_grid/delete_dialog.dart'; @@ -48,7 +45,6 @@ class ActionNotifier extends Notifier { final Logger _logger = Logger('ActionNotifier'); late ActionService _service; late ForegroundUploadService _foregroundUploadService; - late DownloadService _downloadService; late AssetService _assetService; ActionNotifier() : super(); @@ -58,29 +54,6 @@ class ActionNotifier extends Notifier { _foregroundUploadService = ref.watch(foregroundUploadServiceProvider); _service = ref.watch(actionServiceProvider); _assetService = ref.watch(assetServiceProvider); - _downloadService = ref.watch(downloadServiceProvider); - _downloadService.onImageDownloadStatus = _downloadImageCallback; - _downloadService.onVideoDownloadStatus = _downloadVideoCallback; - _downloadService.onLivePhotoDownloadStatus = _downloadLivePhotoCallback; - } - - void _downloadImageCallback(TaskStatusUpdate update) { - if (update.status == TaskStatus.complete) { - _downloadService.saveImageWithPath(update.task); - } - } - - void _downloadVideoCallback(TaskStatusUpdate update) { - if (update.status == TaskStatus.complete) { - _downloadService.saveVideo(update.task); - } - } - - void _downloadLivePhotoCallback(TaskStatusUpdate update) async { - if (update.status == TaskStatus.complete) { - final livePhotosId = LivePhotosMetadata.fromJson(update.task.metaData).id; - unawaited(_downloadService.saveLivePhotos(update.task, livePhotosId)); - } } List _getRemoteIdsForSource(ActionSource source) { diff --git a/mobile/lib/repositories/download.repository.dart b/mobile/lib/repositories/download.repository.dart index c578746a4c..c66313e176 100644 --- a/mobile/lib/repositories/download.repository.dart +++ b/mobile/lib/repositories/download.repository.dart @@ -31,6 +31,10 @@ class DownloadRepository { void Function(TaskProgressUpdate)? onTaskProgress; + // #29900: `onLivePhotoDownloadStatus` is called before the DB has been updated, causing a race between the two Live Photo tasks + // This callback instead listens directly to DB updates + void Function(TaskRecord)? onLivePhotoRecordComplete; + DownloadRepository() { _downloader.registerCallbacks( group: kDownloadGroupImage, @@ -49,6 +53,10 @@ class DownloadRepository { taskStatusCallback: (update) => onLivePhotoDownloadStatus?.call(update), taskProgressCallback: (update) => onTaskProgress?.call(update), ); + + _downloader.database.updates + .where((record) => record.group == kDownloadGroupLivePhoto && record.status == TaskStatus.complete) + .listen((record) => onLivePhotoRecordComplete?.call(record)); } Future> downloadAll(List tasks) { diff --git a/mobile/lib/services/download.service.dart b/mobile/lib/services/download.service.dart index 3f2c36fa7e..4518e82e7a 100644 --- a/mobile/lib/services/download.service.dart +++ b/mobile/lib/services/download.service.dart @@ -1,6 +1,8 @@ +import 'dart:async'; import 'dart:io'; import 'package:background_downloader/background_downloader.dart'; +import 'package:collection/collection.dart'; import 'package:flutter/services.dart'; import 'package:hooks_riverpod/hooks_riverpod.dart'; import 'package:immich_mobile/models/download/livephotos_medatada.model.dart'; @@ -21,11 +23,26 @@ class DownloadService { void Function(TaskStatusUpdate)? onLivePhotoDownloadStatus; void Function(TaskProgressUpdate)? onTaskProgress; + /// Active Live Photo IDs undergoing saving + final Set _savingLivePhotoIds = {}; + DownloadService(this._fileMediaRepository, this._downloadRepository) { _downloadRepository.onImageDownloadStatus = _onImageDownloadCallback; _downloadRepository.onVideoDownloadStatus = _onVideoDownloadCallback; _downloadRepository.onLivePhotoDownloadStatus = _onLivePhotoDownloadCallback; _downloadRepository.onTaskProgress = _onTaskProgressCallback; + _downloadRepository.onLivePhotoRecordComplete = _onLivePhotoRecordComplete; + + unawaited(_savePreviouslyCompletedLivePhotos()); + } + + Future _savePreviouslyCompletedLivePhotos() async { + // Specifically fetch Live Photo video components only, as to not double fetch assets + final records = await _downloadRepository.getLiveVideoTasks(); + final completedIds = records.map((record) => LivePhotosMetadata.fromJson(record.task.metaData).id).toSet(); + for (final id in completedIds) { + await _saveLivePhotos(id); + } } void _onTaskProgressCallback(TaskProgressUpdate update) { @@ -33,18 +50,33 @@ class DownloadService { } void _onImageDownloadCallback(TaskStatusUpdate update) { + if (update.status == TaskStatus.complete) { + unawaited(_saveImageWithPath(update.task)); + } + onImageDownloadStatus?.call(update); } void _onVideoDownloadCallback(TaskStatusUpdate update) { + if (update.status == TaskStatus.complete) { + unawaited(_saveVideo(update.task)); + } + onVideoDownloadStatus?.call(update); } void _onLivePhotoDownloadCallback(TaskStatusUpdate update) { + // Saving relies on mutliple tasks, so we must fetch them from the DB and thus rely on DB events + // rather than this callback onLivePhotoDownloadStatus?.call(update); } - Future saveImageWithPath(Task task) async { + void _onLivePhotoRecordComplete(TaskRecord record) async { + final livePhotosId = LivePhotosMetadata.fromJson(record.task.metaData).id; + await _saveLivePhotos(livePhotosId); + } + + Future _saveImageWithPath(Task task) async { final filePath = await task.filePath(); final title = task.filename; final relativePath = Platform.isAndroid ? 'DCIM/Immich' : null; @@ -65,7 +97,7 @@ class DownloadService { } } - Future saveVideo(Task task) async { + Future _saveVideo(Task task) async { final filePath = await task.filePath(); final title = task.filename; final relativePath = Platform.isAndroid ? 'DCIM/Immich' : null; @@ -83,14 +115,21 @@ class DownloadService { } } - Future saveLivePhotos(Task task, String livePhotosId) async { + Future _saveLivePhotos(String livePhotosId) async { final records = await _downloadRepository.getLiveVideoTasks(); - if (records.length < 2) { + final imageRecord = _findTaskRecord(records, livePhotosId, LivePhotosPart.image); + final videoRecord = _findTaskRecord(records, livePhotosId, LivePhotosPart.video); + + if (imageRecord == null || videoRecord == null) { return false; } - final imageRecord = _findTaskRecord(records, livePhotosId, LivePhotosPart.image); - final videoRecord = _findTaskRecord(records, livePhotosId, LivePhotosPart.video); + // Write semaphore for this `livePhotoId` + if (!_savingLivePhotoIds.add(livePhotosId)) { + return false; + } + + final title = imageRecord.task.filename; final imageFilePath = await imageRecord.task.filePath(); final videoFilePath = await videoRecord.task.filePath(); @@ -98,14 +137,14 @@ class DownloadService { final result = await _fileMediaRepository.saveLivePhoto( image: File(imageFilePath), video: File(videoFilePath), - title: task.filename, + title: title, ); return result != null; } on PlatformException catch (error, stack) { // Handle saving MotionPhotos on iOS if (error.code.startsWith('PHPhotosErrorDomain')) { - final result = await _fileMediaRepository.saveImageWithFile(imageFilePath, title: task.filename); + final result = await _fileMediaRepository.saveImageWithFile(imageFilePath, title: title); return result != null; } _log.severe("Error saving live photo", error, stack); @@ -125,6 +164,7 @@ class DownloadService { } await _downloadRepository.deleteRecordsWithIds([imageRecord.task.taskId, videoRecord.task.taskId]); + _savingLivePhotoIds.remove(livePhotosId); } } @@ -133,8 +173,8 @@ class DownloadService { } } -TaskRecord _findTaskRecord(List records, String livePhotosId, LivePhotosPart part) { - return records.firstWhere((record) { +TaskRecord? _findTaskRecord(List records, String livePhotosId, LivePhotosPart part) { + return records.firstWhereOrNull((record) { final metadata = LivePhotosMetadata.fromJson(record.task.metaData); return metadata.id == livePhotosId && metadata.part == part; }); diff --git a/mobile/test/providers/infrastructure/action_provider_test.dart b/mobile/test/providers/infrastructure/action_provider_test.dart index be13e58f73..7f5c8d4ec2 100644 --- a/mobile/test/providers/infrastructure/action_provider_test.dart +++ b/mobile/test/providers/infrastructure/action_provider_test.dart @@ -12,7 +12,6 @@ import 'package:immich_mobile/providers/infrastructure/asset.provider.dart'; import 'package:immich_mobile/providers/infrastructure/asset_viewer/asset.provider.dart'; import 'package:immich_mobile/providers/user.provider.dart'; import 'package:immich_mobile/services/action.service.dart'; -import 'package:immich_mobile/services/download.service.dart'; import 'package:immich_mobile/services/foreground_upload.service.dart'; import 'package:mocktail/mocktail.dart'; @@ -20,8 +19,6 @@ class MockActionService extends Mock implements ActionService {} class MockAssetService extends Mock implements AssetService {} -class MockDownloadService extends Mock implements DownloadService {} - class MockForegroundUploadService extends Mock implements ForegroundUploadService {} class MockUserService extends Mock implements UserService {} @@ -67,7 +64,6 @@ void main() { overrides: [ actionServiceProvider.overrideWithValue(actionService), assetServiceProvider.overrideWithValue(assetService), - downloadServiceProvider.overrideWithValue(MockDownloadService()), foregroundUploadServiceProvider.overrideWithValue(MockForegroundUploadService()), currentUserProvider.overrideWith((ref) => CurrentUserProvider(userService)), ],