Stream v3 - #14042
Open
shimonewman wants to merge 196 commits into
Open
Stream v3#14042shimonewman wants to merge 196 commits into
shimonewman wants to merge 196 commits into
Conversation
# Conflicts: # app/config/collections.php # app/controllers/api/video.php # app/init.php # app/workers/transcoding.php # composer.lock # src/Appwrite/Event/Transcoding.php # src/Appwrite/Utopia/Response.php # tests/e2e/Services/Video/VideoCustomServerTest.php
routes renaming permissions
routes renaming permissions
routes renaming permissions
routes renaming permissions
- Added new environment variables for managing video processing intervals and retention. - Enhanced video rendition creation to prevent duplicates based on profile and output. - Introduced a sweeper task for cleaning up stale video resources. - Updated Docker configuration to include new environment variables and volumes for video processing. - Improved error handling for video states, including handling of aborted and error statuses. - Added tests to ensure proper functionality of new features and error handling.
- Increased maximum coroutines for video processing from 1 to 4 to improve concurrency. - Updated HLS master and subtitle templates for better formatting and compliance with standards. - Enhanced subtitle management to ensure uploaded tracks take precedence over auto-extracted ones. - Improved error handling and validation in video and subtitle creation processes. - Added new methods for managing video job directories and ensuring proper cleanup of stale resources. - Updated documentation to reflect changes in subtitle handling and video processing logic.
- Updated the CleanStaleVideosResources class to clarify the handling of encode aborts and resource cleanup. - Enhanced the Videos worker to implement a compare-and-swap mechanism for updating video rendition statuses, ensuring concurrent processes do not interfere. - Improved error handling logic to prevent overwriting aborted or errored states during video processing. - Added checks to ensure that workspace cleanup only occurs for created paths, preventing potential conflicts with concurrent operations.
…to 'pending' - Changed the status terminology for video renditions from 'waiting' to 'pending' across various modules and documentation to better reflect the current processing state. - Updated related comments and tests to ensure consistency with the new status terminology. - Enhanced the response models to include 'pending' in the list of valid statuses for video and subtitle processing.
- Removed the video package from composer.json. - Updated the source URL for the video package in composer.lock to use HTTPS instead of Git protocol. - Adjusted the content hash in composer.lock to reflect the changes.
…onment variables for console profile and growth endpoint
- Introduced new video codecs: `h264`, `hevc`, and `vp9`, with default set to `h264`. - Updated video profiles to include codec information. - Added error handling for unsupported codecs and outputs. - Enhanced API endpoints to filter profiles by codec. - Updated documentation to reflect codec options and usage. - Added tests for codec functionality and profile creation with different codecs.
Co-authored-by: Cursor <cursoragent@cursor.com>
- Revised descriptions in the videos-codecs configuration to clarify the role of the `enabled` key. - Updated create-profile and create-rendition documentation to specify that only enabled codecs are accepted. - Enhanced error handling in the API to reject disabled codecs during profile and rendition creation. - Added unit tests to ensure disabled codecs are omitted from the enabled list.
- Updated error handling to allow rendition creation while the source is in `pending` or `downloading` states, with encoding starting once the source is ready. - Revised documentation to clarify the conditions under which renditions can be created and the implications of source statuses. - Improved assertions in the codebase to reflect new logic for handling video source statuses during rendition creation. - Added unit tests to validate the new behavior for rendition creation and source status checks.
…ledCount - Updated method names from `enqueue` to `publish` and `enqueueMany` to `publishMany` for clarity. - Introduced a new method `getFailedCount` to return the count of failed messages, currently returning 0. - Adjusted the class to align with the new method signatures and improve readability.
- Removed outdated comments and unnecessary error handling related to the creation of video renditions. - Streamlined the process for handling video statuses during rendition creation, focusing on clarity and maintainability. - Ensured that the code aligns with the latest logic for managing video source states.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
🔴 Tier D · Serious problems
Adds a Videos service for transcoding Storage files into HLS, DASH, and CMAF, with encoding profiles, subtitles, sprite timelines, playback APIs, realtime events, and usage accounting. It wires workers, storage, response models, documentation, tests, and a local player demo; source downloads now use isolated per-job workspaces. The latest commits also seed profiles through organization project creation and add selectable profile checkboxes to the demo. Latest changes: The newest commits seed default video profiles when creating organization projects, replace the demo's profile-name input with codec-filtered checkboxes and refresh controls, and update its bootstrap identifiers and JWT.
Fix with agent prompt### Issue 1
tmp/video-player.html:713-720
**Render profile names as text rather than HTML**
Profile names accept arbitrary text through the profiles API, so a name such as `<img src=x onerror=...>` executes when an authenticated viewer loads this list; the title attribute is also unescaped. Please construct these elements with DOM APIs and assign names via `textContent`, so profile data cannot steal the viewer's JWT or perform authenticated requests.
### Issue 2
tmp/video-player.html:740-742
**Discard profile responses for a previously selected codec**
Changing codecs quickly can let an older request finish last and overwrite the current codec's profile list. Upload then queues renditions for those stale profile IDs while playback filters by the newly selected codec; please guard responses against the current codec/request and prevent submitting stale selections while loading.
### Issue 3
app/config/collections/projects.php:2692-2695
**Create the new collections for existing projects during migration**
These collections are created by project creation, but no migration creates them for existing projects. Upgraded projects therefore fail video queries/writes against missing collections; please migrate all seven collections and seed their profiles as well.
### Issue 4
docker-compose.override.yml:6-7
**Keep development builds usable without an SSH agent**
Unconditionally forwarding `default` requires a valid SSH_AUTH_SOCK, so the documented `docker compose up --build` fails on development machines without a running SSH agent. Make forwarding optional or remove the requirement; the locked dependencies use HTTPS source URLs.
### Issue 5
docs/services/videos.md:1-3
**Document the explicit create-source step and retained tracks**
Following this workflow creates renditions that remain pending because creating a video no longer probes or downloads it; POST /source is required. The next paragraph also says uploads replace extracted tracks and source updates retrigger extraction, whereas the implementation retains extracted tracks and the video source cannot be replaced.
### Issue 6
src/Appwrite/Platform/Modules/Videos/Workers/Videos.php:1052-1058
**Do not mark subtitle extraction complete before it succeeds**
A worker interruption or transient database/storage failure during extraction leaves `subtitlesExtracted` permanently true, so subsequent rendition and timeline jobs skip the missing tracks. Please distinguish an in-progress claim from successful completion and allow failed or interrupted extraction to retry.
### Issue 7
src/Appwrite/Platform/Modules/Videos/Module.php:13-14
**Retain recovery for interrupted rendition jobs**
Removing the cleanup task also removes recovery for renditions left `started`, `ended`, or `uploading` by a worker crash or container restart. Encode redelivery rejects every non-pending row, and `finally` cannot clean up a killed process, so these renditions stay stuck and their downloaded workspaces remain on the persistent volume; please retain stale-encode recovery even without shared sources.
### Issue 8
tests/e2e/Services/Videos/VideosCustomServerTest.php:302-305
**Check the actual job workspace in the cleanup regression test**
The new worker downloads under `jobs/{jobId}/in/`, but this assertion checks the old shared `/source` path, which is never created anymore. The test still passes if job cleanup is removed entirely; please wait for and verify removal of the actual timeline workspace.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.📂 Walkthrough · 25
⏳ Still open from earlier reviews · 6
Reviewed the commits since |
![hansi-codes[bot]](https://avatars.githubusercontent.com/in/5046327?s=60&v=4){kind=small}
Comment on lines
+2692
to
+2695
| 'videos' => [ | ||
| '$collection' => ID::custom(Database::METADATA), | ||
| '$id' => ID::custom('videos'), | ||
| 'name' => 'Videos', |
There was a problem hiding this comment.
Create the new collections for existing projects during migration
These collections are created by project creation, but no migration creates them for existing projects. Upgraded projects therefore fail video queries/writes against missing collections; please migrate all seven collections and seed their profiles as well.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/config/collections/projects.php
Line: 2692-2695
Comment:
**Create the new collections for existing projects during migration**
These collections are created by project creation, but no migration creates them for existing projects. Upgraded projects therefore fail video queries/writes against missing collections; please migrate all seven collections and seed their profiles as well.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟠 Major · bug · Reply if this doesn't apply.
Comment on lines
+6
to
+7
| ssh: | ||
| - default |
There was a problem hiding this comment.
Keep development builds usable without an SSH agent
Unconditionally forwarding default requires a valid SSH_AUTH_SOCK, so the documented docker compose up --build fails on development machines without a running SSH agent. Make forwarding optional or remove the requirement; the locked dependencies use HTTPS source URLs.
Prompt To Fix With AI
This is a comment left during a code review.
Path: docker-compose.override.yml
Line: 6-7
Comment:
**Keep development builds usable without an SSH agent**
Unconditionally forwarding `default` requires a valid SSH_AUTH_SOCK, so the documented `docker compose up --build` fails on development machines without a running SSH agent. Make forwarding optional or remove the requirement; the locked dependencies use HTTPS source URLs.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟠 Major · bug · Reply if this doesn't apply.
Comment on lines
+1
to
+3
| The Videos service allows you to transcode video files stored in Appwrite Storage into adaptive streaming formats. Upload a video to a bucket, create a video resource from it, then request renditions at the quality levels you need — Appwrite packages them as HLS, DASH, or CMAF and serves the manifests and segments for playback. You can also attach subtitle tracks and generate sprite timelines for player scrubbing. | ||
|
|
||
| On create (and when the source file is updated), Appwrite probes the source and automatically registers **text-based** embedded subtitle streams (for example SubRip, ASS, or `mov_text`) as ready subtitle tracks. Image-based streams such as PGS or VobSub are skipped. Uploaded WebVTT or SubRip files always win for the same language code: creating or updating an uploaded track replaces any auto-extracted track with that code. |
There was a problem hiding this comment.
Document the explicit create-source step and retained tracks
Following this workflow creates renditions that remain pending because creating a video no longer probes or downloads it; POST /source is required. The next paragraph also says uploads replace extracted tracks and source updates retrigger extraction, whereas the implementation retains extracted tracks and the video source cannot be replaced.
Prompt To Fix With AI
This is a comment left during a code review.
Path: docs/services/videos.md
Line: 1-3
Comment:
**Document the explicit create-source step and retained tracks**
Following this workflow creates renditions that remain pending because creating a video no longer probes or downloads it; POST /source is required. The next paragraph also says uploads replace extracted tracks and source updates retrigger extraction, whereas the implementation retains extracted tracks and the video source cannot be replaced.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟡 Minor · documentation · Reply if this doesn't apply.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
✨ Benchmark resultsComparing
Per-scenario breakdown & investigation detailsMetrics below reflect the current branch (after). Δ P95 compares against the base.
Top API waits (after)
|
…p configuration - Eliminated unused environment variables related to video processing, including intervals for stuck video sweeps and retention settings. - Removed outdated error handling and status checks from the video source management logic, streamlining the codebase. - Updated documentation to reflect changes in video source and rendition handling, ensuring clarity on the current implementation.
Comment on lines
+1052
to
+1058
| $claimed = $dbForProject->updateDocuments( | ||
| 'videos', | ||
| new Document(['subtitlesExtracted' => true]), | ||
| [ | ||
| Query::equal('$id', [$video->getId()]), | ||
| Query::equal('subtitlesExtracted', [false]), | ||
| ] |
There was a problem hiding this comment.
Do not mark subtitle extraction complete before it succeeds
A worker interruption or transient database/storage failure during extraction leaves subtitlesExtracted permanently true, so subsequent rendition and timeline jobs skip the missing tracks. Please distinguish an in-progress claim from successful completion and allow failed or interrupted extraction to retry.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Appwrite/Platform/Modules/Videos/Workers/Videos.php
Line: 1052-1058
Comment:
**Do not mark subtitle extraction complete before it succeeds**
A worker interruption or transient database/storage failure during extraction leaves `subtitlesExtracted` permanently true, so subsequent rendition and timeline jobs skip the missing tracks. Please distinguish an in-progress claim from successful completion and allow failed or interrupted extraction to retry.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟡 Minor · error-handling · Reply if this doesn't apply.
Comment on lines
+13
to
+14
| $this->addService('http', new Http()); | ||
| $this->addService('workers', new Workers()); |
There was a problem hiding this comment.
Retain recovery for interrupted rendition jobs
Removing the cleanup task also removes recovery for renditions left started, ended, or uploading by a worker crash or container restart. Encode redelivery rejects every non-pending row, and finally cannot clean up a killed process, so these renditions stay stuck and their downloaded workspaces remain on the persistent volume; please retain stale-encode recovery even without shared sources.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Appwrite/Platform/Modules/Videos/Module.php
Line: 13-14
Comment:
**Retain recovery for interrupted rendition jobs**
Removing the cleanup task also removes recovery for renditions left `started`, `ended`, or `uploading` by a worker crash or container restart. Encode redelivery rejects every non-pending row, and `finally` cannot clean up a killed process, so these renditions stay stuck and their downloaded workspaces remain on the persistent volume; please retain stale-encode recovery even without shared sources.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟡 Minor · error-handling · Reply if this doesn't apply.
Comment on lines
+302
to
+305
| $video = $this->waitForVideoProbed($videoId); | ||
| $this->assertGreaterThan(0, $video['duration']); | ||
| $this->assertGreaterThan(0, $video['width']); | ||
| $this->assertFileDoesNotExist($this->tmpSourcePath($videoId)); |
There was a problem hiding this comment.
Check the actual job workspace in the cleanup regression test
The new worker downloads under jobs/{jobId}/in/, but this assertion checks the old shared /source path, which is never created anymore. The test still passes if job cleanup is removed entirely; please wait for and verify removal of the actual timeline workspace.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/e2e/Services/Videos/VideosCustomServerTest.php
Line: 302-305
Comment:
**Check the actual job workspace in the cleanup regression test**
The new worker downloads under `jobs/{jobId}/in/`, but this assertion checks the old shared `/source` path, which is never created anymore. The test still passes if job cleanup is removed entirely; please wait for and verify removal of the actual timeline workspace.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟡 Minor · testing · Reply if this doesn't apply.
Comment on lines
+713
to
+720
| <label title="${meta || name}"> | ||
| <input | ||
| type="checkbox" | ||
| data-profile-id="${profile.$id}" | ||
| data-profile-name="${name.replace(/"/g, '"')}" | ||
| ${checked ? 'checked' : ''} | ||
| /> | ||
| ${name}${meta ? ` <span class="muted">(${meta})</span>` : ''} |
There was a problem hiding this comment.
Render profile names as text rather than HTML
Profile names accept arbitrary text through the profiles API, so a name such as <img src=x onerror=...> executes when an authenticated viewer loads this list; the title attribute is also unescaped. Please construct these elements with DOM APIs and assign names via textContent, so profile data cannot steal the viewer's JWT or perform authenticated requests.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tmp/video-player.html
Line: 713-720
Comment:
**Render profile names as text rather than HTML**
Profile names accept arbitrary text through the profiles API, so a name such as `<img src=x onerror=...>` executes when an authenticated viewer loads this list; the title attribute is also unescaped. Please construct these elements with DOM APIs and assign names via `textContent`, so profile data cannot steal the viewer's JWT or perform authenticated requests.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🔴 Critical · security · Reply if this doesn't apply.
Comment on lines
+740
to
+742
| const codec = selectedCodecId(); | ||
| const list = await api(`/videos/profiles?codec=${encodeURIComponent(codec)}`); | ||
| availableProfiles = (list.profiles || []).map((p) => ({ |
There was a problem hiding this comment.
Discard profile responses for a previously selected codec
Changing codecs quickly can let an older request finish last and overwrite the current codec's profile list. Upload then queues renditions for those stale profile IDs while playback filters by the newly selected codec; please guard responses against the current codec/request and prevent submitting stale selections while loading.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tmp/video-player.html
Line: 740-742
Comment:
**Discard profile responses for a previously selected codec**
Changing codecs quickly can let an older request finish last and overwrite the current codec's profile list. Upload then queues renditions for those stale profile IDs while playback filters by the newly selected codec; please guard responses against the current codec/request and prevent submitting stale selections while loading.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟡 Minor · concurrency · Reply if this doesn't apply.
| "renditionId": "6abe2ab3a3ffdbb8659e", | ||
| "bucketId": "6abe2ab2763263b328f4", | ||
| "hlsUrl": "http:\/\/localhost\/v1\/videos\/6abe2ab39eeeec1a9ac3\/outputs\/hls\/master.m3u8?project=6abe2ab19e49995dec17", | ||
| "jwt": "eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJwcm9qZWN0SWQiOiI2YWJlMmFiMTllNDk5OTVkZWMxNyIsInVzZXJJZCI6IjZhYmUyYWMyMjhjMmJmOGY1YTgxIiwic2Vzc2lvbklkIjoiNmFiZTJhYzI3NDhmMGIxYTU0NmUiLCJleHAiOjE3OTA4NDg1ODJ9.S3LVe8lYbw4YwhP1KyrIw8YJ5luBibeQcOcptLrYPQU" |
This branch has not been deployed
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
3 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.
No description provided.