feat: Live Photo .mov via explicit file_sizes (live_video_original)#465
Open
epheterson wants to merge 5 commits into
Open
feat: Live Photo .mov via explicit file_sizes (live_video_original)#465epheterson wants to merge 5 commits into
epheterson wants to merge 5 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds Live Photo pairing support so that when users request original photo downloads, the paired Live Photo video (.mov) is also downloaded, addressing issue #199 (Live Photos missing the video component).
Changes:
- Update photo task orchestration to append a
live_video_originaldownload task when appropriate (Live Photo +originalrequested). - Prevent
live_video_*keys from being accepted as user-configurablefile_sizesto avoid duplicate task creation. - Add dedicated unit tests covering Live Photo pairing behavior and failure modes; temporarily pin
icloudpyto a fork/branch that exposes the needed keys.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/album_sync_orchestrator.py |
Auto-collects an additional download task for live_video_original when original is requested and the asset is a Live Photo. |
src/config_parser.py |
Filters live_video_* keys out of user-configurable file_sizes validation to avoid duplicates and improve logging for invalid options. |
tests/test_live_photo_pair_download.py |
Introduces test coverage for Live Photo .mov pairing and related edge cases. |
requirements.txt |
Temporarily switches icloudpy dependency to a git branch with Live Photo support. |
When a photo is a Live Photo and the user has requested the 'original' file_size, the orchestrator collects a second download task for the paired .mov. Live Photos now round-trip intact (HEIC + paired MOV land together in the destination) instead of dropping the video half. Has no effect on plain stills (the 'live_video_original' key is absent in photo.versions for them) or when the user did not request 'original'. Failure to read photo.versions is non-fatal — the original-still task is still emitted. Also filters live_video_* keys out of the user-facing file_sizes validation (these are internal — surfaced by the auto-append, not for direct user configuration). A user who adds live_video_original to file_sizes would silently get duplicate download tasks for the same path; this PR makes them get a clear 'not a valid option' log line. Dependency: requires icloudpy with the live_video_* keys in PHOTO_VERSION_LOOKUP and the item_type property — submitted as a companion PR at mandarons/icloudpy. requirements.txt temporarily pins the fork branch. Once that PR merges and a new icloudpy release ships, please bump requirements.txt back to a version pin (e.g. icloudpy==0.9.0) — happy to follow up. Tests: - 5 in tests/test_live_photo_pair_download.py: * Live Photo with 'original' yields two tasks (still + .mov) * still photo yields one task * Live Photo with only medium/thumb does NOT append .mov * photo.versions raising is non-fatal * None result from collect_download_task is skipped Closes mandarons#199.
…gs_list) F841: tasks was bound but never read; the test asserts on fake_collect.call_args_list directly. Drop the assignment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…lution Two Copilot review items: 1. **Silent exception swallow.** ``_collect_photo_download_tasks`` was catching ``photo.versions`` read failures with a bare ``except Exception: pass``, making Live Photo pairing failures completely silent. Operators investigating "why is the .mov missing for this photo?" had no signal. Now logs at DEBUG (visible when troubleshooting; no noise at default INFO level). 2. **MagicMock class pollution.** ``test_versions_access_failure_is_non_fatal`` was doing ``type(photo).versions = property(...)`` which monkey-patches the global ``MagicMock`` class and leaks a raising ``versions`` property into every later test that touches ``MagicMock().versions``. Replaced with a dedicated ``_BrokenPhoto`` stub class. Also added ``test_versions_access_failure_logs_debug`` to lock in the observability requirement -- if a future refactor silences the DEBUG log again, this test fails. Verified on python:3.10 docker mirroring CI: ruff clean, 441 passed, 100.00% coverage. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
epheterson
force-pushed
the
feat/photos-live-photo-pair-download
branch
from
32e1a81 to
f5c7e51
Compare
|
This probably also can be configurable with If you want .mov - you can add |
Switch from auto-appending the paired .mov (when "original" was requested) to the maintainer-preferred explicit model: add live_video_original (or live_video_medium/thumb) to photos.filters.file_sizes to download it. It flows through the normal download loop; non-Live-Photos lack the version and are skipped quietly (DEBUG, not WARNING, to avoid spam on large libraries). - config_parser: stop filtering live_video_* out of valid file_sizes - album_sync_orchestrator: drop the auto-append special-case - photo_download_manager: log a missing live_video_* version at DEBUG - docs: note live_video_original in README + config.yaml - tests updated for the explicit model Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Thanks for the steer this is the right way to do it! Happy to help add features, and don't mind adjusting to fit into the existing design. |
epheterson
changed the title
….MP4) The live_video_* versions are QuickTime/MP4 movies, but the filename builder only remapped the extension for original_alt, so the paired video inherited the still's extension (e.g. IMG_1234__live_video_original__<id>.HEIC) -- a video wearing a .HEIC extension, which every image tool rejects. Map the live_video_* versions to their real container extension via the iCloud UTI (defaulting to MOV). generate_photo_path also renames an existing mislabeled '...__live_video_*__<id>.HEIC' in place to the corrected path, so upgrading doesn't re-download every paired video or leave the old file behind. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Live Photos store the paired
.movas a separate version (live_video_original, pluslive_video_medium/live_video_thumb) which icloudpy 0.9.0 surfaces viaPhotoAsset.PHOTO_VERSION_LOOKUP(icloudpy#139). This PR makes those versions selectable like any other size.Opt-in via the existing surface: add
live_video_original(and/or_medium/_thumb) tophotos.filters.file_sizes:live_video_*version logs at DEBUG, not WARNING, to avoid spam).validate_file_sizesnow accepts thelive_video_*keys.Reworked from the original auto-append approach to this explicit
file_sizesmodel per review feedback — keepsfile_sizesas the single surface for "which variants to pull" rather than adding implicit behavior. Closes #199.