Send the per-file role on episode upload - #1845
Open
martien-wdy wants to merge 6 commits into
Open
Conversation
Convert keeps writing playable MP4s into a separate directory for downloaded episodes. ConvertSourceInPlace is the sanctioned exception: it writes cameras/<source>/playable.mp4 inside the episode itself, for the agent to call while sealing, before the manifest's file list is finalized. The raw segments and index.jsonl are still only ever read, and the output goes through a temporary file plus rename so a crash mid-mux leaves nothing half-written (sealing ignores *.tmp files). The result judgement stays with the caller: BFrames, UndecodedSliceHeaders and SyncSamples report the conditions under which the clip's timing or seekability cannot be vouched for, and the package deliberately does not decide policy for its callers.
The seal now remuxes every camera source into cameras/<source>/playable.mp4 before sealFiles walks the episode, so the derived clip gets a size and SHA-256 entry in the manifest and rides the existing pipeline unchanged: the transfer worker uploads it because it is listed, and commit-time verification covers it for the same reason. The manifest entry carries role "derived" (File.Role, FileRoleDerived) so nothing ever counts it as capture payload; model-input and payload-retention accounting resolve payload bytes through index.jsonl into the raw segments, which are kept byte-for-byte as before. Sealing never fails or waits on the mux. A source whose stream cannot become an honestly timed, seekable clip (B slices, slice headers the muxer cannot parse, no random-access frame, or an outright mux failure) seals without its playable.mp4 and the manifest's playable_notes names the reason; a clip that omitted frames whose bytes were missing gets a note as well. The gates mirror the warnings the episode-playable command prints, hardened, because a manifest vouches for everything it lists. Recovery of interrupted .partial episodes derives the same clips, since it is the other path that seals; the truncated index tails are cut first and the muxer counts a partial trailing line as unusable. Cost: the remux is a copy, not a transcode, so the seal gains one more pass over the camera bytes beside the checksum pass it already makes, and the clip adds roughly the raw stream's size to the episode. That size counts against the local quota through both the manifest total and the disk-usage walk enforceQuota performs.
episode-playable now notices an episode whose camera sources all carry a seal-time cameras/<source>/playable.mp4, says so, and converts nothing, since reconverting would only duplicate bytes the manifest already vouches for. The command remains the path for episodes sealed by older agents and for sources whose seal-time mux was refused. The wendy data command documentation explains the sealed-episode MP4, its manifest listing with role "derived", the playable_notes failure account, and the roughly-raw-stream-sized disk overhead counted against the episode's quota. The data platform demo runbook now points at the sealed clip instead of prescribing the laptop-side conversion.
Problem: a camera producer restart mid-episode splices a new Sequence Parameter Set (SPS) and Picture Parameter Set (PPS) into the raw stream, most likely at a different resolution. The seal-time remux kept only the first SPS/PPS as the MP4's single decoder configuration and silently dropped the changed ones, so every frame after the restart would be decoded against the wrong parameters, while no existing gate (B slices, unparseable slice headers, missing sync samples) fired: the clip sealed into the manifest as a faithful rendering it is not. Cause: muxAnnexBToMP4 treated every repeated parameter set as the identical re-inline an encoder emits before each Instantaneous Decoder Refresh (IDR) frame, without comparing bytes. Solution: count byte-different SPS/PPS units as ParameterSetChanges on the ClipResult (identical repeats stay uncounted), refuse the clip at seal time with a playable_notes entry naming the producer restart, and warn from the episode-playable command for laptop-side conversions. Also adds regression tests for two seal contracts that held under attack: a mux I/O failure never fails the seal and leaves nothing half-written listed or behind, and recovery remuxes from the truncated index instead of reusing a stale pre-crash clip, cleaning up any stray temporary file.
The device manifest marks the seal-time cameras/<source>/playable.mp4 remux with role "derived" and leaves the role empty for capture payload, but buildEpisodeManifest projected only path, size, checksum, media type, format and source id. The distinction died at the wire, so the cloud catalog stored the remux with the camera's source id and video/mp4 media type and it read back as a second capture on that source. Vendor the EpisodeFileRole addition to cloud/data_ingest.proto, regenerate cloudpb, and map data.File.Role onto the enum. Empty maps to CAPTURED, FileRoleDerived to DERIVED. Any other string maps to UNSPECIFIED, which the wire contract defines as captured; the enum has no unknown member and inventing one would force readers to handle a state they cannot act on. Only go/proto/gen/cloudpb/data_ingest.pb.go is regenerated. The committed stubs carry a mix of protoc v7.34.0 and v7.35.1 stamps, so a plain regeneration rewrites 80 files; each file's own committed stamp is restored so the diff holds the intended change alone.
Follows the service-protos review on pull request #38. EpisodeManifest no longer carries org_id or asset_id, so the transfer worker stops sending them. The cloud reads both from the mutual Transport Layer Security (mTLS) client certificate presented on the connection, which it already did; the manifest copy was only ever compared against that certificate and then discarded. Because the identity now travels solely on the certificate, the whole org/asset thread through the upload path is dead and goes with it: ingestClientFactory no longer returns them, and processEpisode, uploadEpisode and buildEpisodeManifest no longer take them. dialFactory still reads ProvisioningInfo, which is what builds the certificate identity header. Byte offsets are unsigned on the wire now. The device keeps working in the signed offsets its file interfaces use and converts at the boundary. A server-reported committed offset above the signed maximum is treated as "nothing committed", so the file restarts from zero rather than wrapping to a negative offset. Regenerated go/proto/gen/cloudpb from the updated contract. Every other generated file's protoc version stamp was restored to its committed value, so the diff carries only real changes.
Contributor
Swift E2E ReviewNo Swift E2E review issues were generated for this run. Report artifact: swift-e2e-tests.gh33393310203.run.0001 |
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
1 participant
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.
Problem
#1842 seals each camera source with a browser-playable
cameras/<source>/playable.mp4and marks it in the device manifest withRole = FileRoleDerived. The device therefore knows the file is an artifact computed from capture payload rather than capture payload itself.buildEpisodeManifestingo/internal/agent/services/data_transfer_worker.gonever sent that. It projected path, size, checksum, media type, format and source id, becauseEpisodeFileManifesthad no role field. The remux reached the cloud carrying the camera'ssource_idand media typevideo/mp4, which on the catalog side is indistinguishable from a second camera capture on that source. A Business Intelligence query summing bytes persource_idroughly doubles every camera source. Only the verbatim manifest inattributes_jsonstill held the truth, and nothing queries into it.Cause
The upload manifest is a projection of the device manifest, and the role was added to the device manifest after the projection was written. The wire had no field to project onto.
Solution
Vendor the
EpisodeFileRoleaddition intoProto/cloud/data_ingest.proto(matching wendylabsinc/service-protos#49 byte for byte, as this repository's copy already did), regeneratecloudpb, and mapdata.File.Roleonto the enum inbuildEpisodeManifest.The mapping keeps one vocabulary across the three layers:
EPISODE_FILE_ROLE_CAPTUREDdata.FileRoleDerivedmaps toEPISODE_FILE_ROLE_DERIVEDEPISODE_FILE_ROLE_UNSPECIFIEDCaptured is sent explicitly rather than left at the default, so a manifest that went through this mapper is distinguishable on the wire from one written by an agent built before the field existed. The unrecognised case only arises when an older agent resumes an upload for an episode a newer build sealed;
UNSPECIFIEDis defined by the wire contract to mean captured, which is the pre-existing meaning, and the enum deliberately has no "unknown" member because no reader could act on one.The protoc version-stamp trap
The committed stubs under
go/proto/genwere generated with a mix of protoc versions: 37 files stampedv7.34.0, 6 stampedv7.35.1, plus oneprotoc-gen-go v1.36.11-develamong 42v1.36.11. Runninggo/scripts/generate-proto.shtherefore rewrites the stamp in 80 files and buries the real change.Each regenerated file's own committed stamp is restored, and any file whose only difference was the stamp is reverted outright. The normalizer was verified as a true no-op: regenerating with no proto change leaves 80 files dirty, and after normalization the tree is clean. This pull request touches exactly one generated file,
go/proto/gen/cloudpb/data_ingest.pb.go, and it contains no stamp lines in its diff.One unrelated pre-existing drift surfaced during that check and is not included here:
go/proto/gen/appspb/v1/sensor_service.pb.gois stale against its own.proto, whose comment was edited from "sensors and camera" to "sensor-read and camera" without regenerating. That belongs in its own change.Verification
CC=/usr/bin/clang go build ./go/...passesgofmt -l go/is emptygo vet ./go/internal/agent/services/... ./go/internal/agent/data/...is cleango test ./go/internal/agent/services/... ./go/internal/agent/data/...passes (okfor both packages)Two new tests:
TestBuildEpisodeManifestCarriesFileRolebuilds a manifest with an index, a raw segment and the derived remux, and asserts the wire carriesCAPTURED,CAPTURED,DERIVEDrespectively.TestEpisodeFileRoleUnknownDegradesToUnspecifiedpins the three mapper cases including the unrecognised one.Dependency order
This branch is cut from
feat/episode-playable-at-seal, so its commits appear in this diff. It must merge after both:data.File.Role)and after https://github.com/wendylabsinc/cloud/pull/463, which persists the role in the catalog. The cloud service should be deployed before devices carrying #1842 start uploading: a derived file that reaches a service without that change is recorded as captured and stays that way.