[QA only, do not merge] Combine video edit/crop PRs #15090, #15147, #15180 - #15409
Draft
claude[bot] wants to merge 8 commits into
Draft
[QA only, do not merge] Combine video edit/crop PRs #15090, #15147, #15180#15409claude[bot] wants to merge 8 commits into
claude[bot] wants to merge 8 commits into
Conversation
… into qa/video-edit-combined
…qa/video-edit-combined
Combines the VIDEO_EDIT trim/crop feature with the asset-based video metadata approach from #15147/#15180. #15090 implemented the same VIDEO_EDIT node feature (VideoTrim/VideoCrop/LoadVideo edit widget, DynamicCombo codec, io.VideoEdit type) essentially identically to #15147, so those files merged cleanly with no functional duplication. Conflict resolution: - comfy_api/latest/_input/video_types.py: trivial docstring-only conflict on VideoInput.as_cropped(); kept the more descriptive docstring from #15090, function body identical on both sides. - server.py: reverted to master's version, dropping #15090's competing `GET /video_metadata` endpoint (and the resolve_view_media_path refactor that existed solely to support it). That endpoint was #15090's alternative way of sourcing video metadata for the frontend widget (live ffprobe via HTTP), which is superseded here by #15147/#15180's asset system_metadata extraction on ingest/scan. Nothing else in the tree referenced this route, so dropping it is safe. No other unique, non-conflicting content from #15090 was discarded.
Re-applies the /video_metadata route from PR #15090 (078f0f5), dropped during the merge in favor of the assets-based metadata approach. It does not conflict with that work at the route level and closes a cosmetic gap (fps/file-size estimation) for the VIDEO_EDIT trim/crop widget during QA. Implemented as a self-contained handler (duplicating /view's minimal filename-resolution logic for blake3 asset hashes vs. on-disk paths) rather than refactoring /view's already-reviewed inline logic into a shared helper, to keep this QA-only change isolated and low-risk.
# Conflicts: # comfy_api/latest/_input_impl/video_types.py
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
2 participants
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.
Requested by Christian Byrne · Slack thread
What this is
A QA/testing-only combination branch. It merges three open video-editing PRs together so their functionality can be tested as a whole in one place (and so CI / a preview environment can run against the combined result). This branch is not intended to be merged into
master. Please close/leave this PR unmerged once QA is done; it should be treated as disposable.Source PRs combined
feat/asset-video-media-metadata— extracts video metadata into assetsystem_metadataon ingest/scan. Foundational; merged first.feat/asset-video-metadata— adds theVIDEO_EDITinput type and trim/crop nodes on top of the asset-based metadata from feat(assets): extract video metadata into system_metadata on ingest and scan #15180. Merged second, cleanly, with no conflicts (it's a strict superset of feat(assets): extract video metadata into system_metadata on ingest and scan #15180 at the file/hunk level).feat/video-edit-input— an earlier/alternative implementation of the sameVIDEO_EDITtrim/crop feature, but sourcing video metadata from a newGET /video_metadataserver endpoint instead of the asset system. Per its relationship to feat: VIDEO_EDIT input type with asset-backed video metadata #15147 (feat: VIDEO_EDIT input type with asset-backed video metadata #15147's own description calls itself "an alternative solution for feat: VIDEO_EDIT input type for video trim/crop rich widgets #15090"), the two are competing approaches to the same problem, not additive.How the #15090 vs #15147 overlap was resolved
Merging #15090 on top of #15147+#15180 produced only one real conflict, plus one competing implementation to reconcile:
Kept:
system_metadataextraction on ingest/scan, and theVIDEO_EDITinput type /VideoTrim/VideoCrop/LoadVideonode implementation.VideoTrim/VideoCrop/LoadVideo/io.VideoEditcode turned out to be almost byte-identical to feat: VIDEO_EDIT input type with asset-backed video metadata #15147's own copy of the same feature (both branches clearly built the same node-level implementation), socomfy_extras/nodes_video.py,comfy_api/latest/_io.py,comfy_api/latest/_input_impl/video_types.py, andcomfy_api/latest/_util/*merged automatically with no duplication.comfy_api/latest/_input/video_types.py(VideoInput.as_cropped): kept the more descriptive docstring; the function body was identical on both sides.Dropped:
GET /video_metadataendpoint inserver.py(and theresolve_view_media_pathrefactor of/viewthat existed solely to support it). This was feat: VIDEO_EDIT input type for video trim/crop rich widgets #15090's alternative way of getting metadata to the frontend widget (a live ffprobe-style HTTP probe), which is superseded here by feat: VIDEO_EDIT input type with asset-backed video metadata #15147/feat(assets): extract video metadata into system_metadata on ingest and scan #15180's approach of extracting metadata into the asset'ssystem_metadataat ingest/scan time. Nothing else in the tree calls this route, so removing it is safe.server.pyon this branch is otherwise identical tomaster.No other unique, non-conflicting content from #15090 was found to drop — the diff surface between #15090 and #15147 was limited to the metadata-sourcing mechanism described above plus unrelated drift from the two branches being based on different points in
master's history.Sanity checks performed (no full CI available locally)
python -m py_compileon every file touched by the merge (app/assets/scanner.py,app/assets/services/{ingest,media_metadata,video_metadata}.py,comfy_api/latest/_input/video_types.py,comfy_api/latest/_input_impl/video_types.py,comfy_api/latest/_io.py,comfy_api/latest/_util/__init__.py,comfy_api/latest/_util/video_types.py,comfy_extras/nodes_video.py,server.py, and the new/changed test files) — all clean.server.pyandapp/assets/api/routes.py— none (the competing/video_metadataroute was removed).node_iddefinitions incomfy_extras/nodes_video.py— none (SaveWEBM,SaveVideo,CreateVideo,GetVideoComponents,LoadVideo,VideoSlice,VideoTrim,VideoCropeach appear once).tests-unit/assets_test/services/test_video_metadata.py(the unit tests added by feat(assets): extract video metadata into system_metadata on ingest and scan #15180/feat: VIDEO_EDIT input type with asset-backed video metadata #15147 for the merged metadata-extraction code): 13 passed.tests-unit/assets_test/suite: the metadata/scanning/ingest tests pass; the remaining failures/errors in that directory are pre-existing integration tests that boot a full ComfyUI subprocess and need the full dependency stack (torch, etc.) not installed in this sandbox — unrelated to this merge.comfy_api_test/video_types_test.py/video_bit_depth_test.pycouldn't be executed here (missingtorchin this minimal environment) but passpy_compile.