Repository navigation
fix(video-note): crop recorded notes to the centred square - #260
TiaraBasori wants to merge 3 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Automated PR reviewHead: Review unavailable: Model claimed ready despite incomplete source context. Remaining gates
No PR code runs with the model key or write token. Free-only routing; provider/rate-limit failures never authorize a merge. Existing review findings must be resolved, not hidden by a new recommendation. |
iebb
left a comment
iebb
left a comment
There was a problem hiding this comment.
Current-head review: square video-note export is a legitimate improvement, but the production iOS exporter in ios/Runner/AppDelegate.swift:cropVideoNote has no compiled/native execution evidence. The new Dart tests mock the channel or pin source strings; they do not execute AVAssetExportSession, the preferredTransform/translation, output verification or audio handling. The PR explicitly acknowledges no Swift/Kotlin compilation, so the current green Linux Quality run is not platform validation.
Before merging, please provide credential-free hosted iOS build coverage and native export regressions using real synthetic input files for 0/90/180/270-degree orientations, square output, duration and retained audio. Cover route cancellation/timeout with a late exporter completion so temporary output is not left unowned. Android currently answers unsupported and sends the original; retain that clearly documented partial-platform scope rather than claiming Android square capture is fixed. This remains open for validation/revision, not rejected as an invalid improvement.
bacb5c8 to
4cdca94
Compare
|
Addressed one boundary at head Route cancellation with a late exporter completion (fixed). Hosted iOS build + native export regressions — not delivered, and here is why. There is an existing macOS XCTest target ( Android scope is unchanged and still documented as partial: Format |
23af1c8 to
e095919
Compare
|
Native regression coverage added at head What landed. What the hosted build actually proved. I built a credential-free simulator workflow on the fork ( The RunnerTests bundle then failed at compile time on one thing of mine, now fixed at this head: a synchronous test calling the main-actor-isolated What is still not proven, stated plainly. The hosted |
iebb
left a comment
iebb
left a comment
There was a problem hiding this comment.
Re-reviewed e095919. Moving the square into the layer transform and adding 0/90/180/270/square geometry cases are useful progress; the author's distinction between compilation and export evidence is correct. A remaining [P2] cleanup boundary is visible in VideoNoteCropService: abandonInFlight clears the only reference then waits at most five seconds; when that wait times out, no continuation remains to reclaim a still-later successful export. Also crop marks a result delivered before videoNoteUploadPath finishes its awaited file checks; if the recorder is disposed in that gap, both dispose and the !mounted continuation find _inFlight null and the accepted-but-unsent crop is orphaned. Please keep per-request ownership until the caller either hands the file to the send path or explicitly abandons it, and attach cleanup to the actual eventual platform completion (not just a five-second wait). Add late-beyond-cleanup-timeout and dispose-between-service-delivery/recorder-handoff regressions that assert temp files are removed while accepted send files survive. Native evidence remains incomplete: the app build step in fork run 39632511955 succeeded on an earlier head, but its XCTest compile failed on main-actor isolation; the new @mainactor head's tests and real AVAssetExportSession rotation/audio/duration exports still have not passed. Please retain the iOS/native gate and the honest Android-unsupported scope. This is current-source review plus observed hosted-run evidence, not a new local/device reproduction. Keep this legitimate fix open.
|
Addressed at head The native evidence you asked for existed and failed — fork run Root fix (iOS). Real export regressions, not source pins: I am not claiming a local pass: this box has no Apple toolchain, and I will not pretend a compile. A fresh credential-free run was dispatched against Dart ownership (the [P2] cleanup boundary).
Format Android is unchanged and still answers unsupported, with the Dart side sending the original recording; I am not claiming a square capture there. |
537d469 to
5701d7b
Compare
|
The native evidence you asked for now exists and passes, so I am not asking you to take it on faith. Fork run 39698978524 dispatched against The three cases that failed on
That is real executor evidence on a simulator, not a source pin. It is still not a physical-camera recording, and the PR does not claim one. The Dart [P2] from the last review is unchanged from my previous comment and green here (26/26 in |
e426e2c to
489b73f
Compare
TiaraBasori
left a comment
TiaraBasori
left a comment
There was a problem hiding this comment.
Addressed review 5604259229 on top of the current rebase head 489b73f — the new head is 9867001, an incremental fix that keeps the per-request ownership and iOS isolation already on the branch and closes the two races you described precisely.
Cleanup is bound to the actual platform completion, not the wait. Each export now gets a VideoNoteCropTicket (from VideoNoteCropService.start). The platform future's own whenComplete is the single unconditional reclaim hook: it runs whenever the export truly finishes, marks the request settled, and — if the request is abandoned and unaccepted — deletes its temp file then. abandon and the global sweep only bound how long they wait; they never cancel or detach the reclaim. The reclaim itself is memoized on the request (one future, not parallel unawaited deletions), so the abandon, the resolve, the sweep and the platform completion all race the same deletion instead of firing their own.
The delivery flag is set only after the caller truly has the file. videoNoteUploadPath now resolves through the ticket: the request is only marked accepted after the cropped path is non-null, differs from the recording, exists on disk, and the recording it replaces has been deleted. Before that point the request stays unaccepted, so a recorder disposed in the gap is seen by cleanup exactly as "delivered but not handed off" and its own crop is reclaimed — and only its own, because:
The recorder no longer sweeps every in-flight export. _finishRecording threads its ticket out via onStarted: and keeps it on the state; dispose and the !mounted branch abandon exactly that ticket. A second live recorder handing its crop to the send path is untouched by another route being torn down.
Regressions (test/video_note_crop_test.dart, 31/31 passing):
late beyond cleanup timeout is still reclaimed by the platform itself— the crop times out,abandon(timeout: Duration.zero)returns with nothing reclaimed, and the platform completes with the temp path long afterwards. Asserts the temp file is then deleted (by the platform completion, not the wait) while a second, accepted send file survives the same late reclaim. Fails on the pre-fix head.dispose between delivery and handoff keeps the accepted send file/… alive— the two halves of the gap you named: the disposed flow's own crop is reclaimed (temp deleted) and the accepted flow's file survives, driven through the ticketvideoNoteUploadPathhands out, withpumpEventQueueletting the file IO land.a surrendered ticket does not touch another recorder's accepted crop— two concurrent tickets; abandoning the dying one deletes only its temp. This is the mutation the old globalabandonInFlightsweep failed: it took the live recorder's accepted file too.
Source pins updated to match: _finishRecording contains no abandonInFlight, dispose uses the state's own ticket, and accept runs only after the recording it replaces is deleted.
Android remains unsupported, unchanged and documented — cropVideoNote still answers unsupported on Android and the Dart side sends the uncropped recording; VideoNoteCropper.kt keeps the exact MediaExtractor→MediaCodec pipeline and per-device hazards for the implementation that replaces it. No media3/transformer dependency was added.
Diff against origin/master is unchanged in scope: MainActivity.kt, VideoNoteCropper.kt, AppDelegate.swift, RunnerTests.swift, video_note_crop_service.dart, video_note_recorder_view.dart, video_note_crop_test.dart.
The camera records 16:9 while the recorder preview and the received bubble both show the centred square, so nearly half of every uploaded note was pixels no client draws. iOS re-encodes the recording to that square through AVAssetExportSession with the layer transform carrying both the preferredTransform and the crop offset (setCropRectangle applies in the track's natural space and misplaces a 180-degree recording), and verifies the output - square within 1px, duration within 0.25s, audio track kept - before returning it. Android answers `unsupported` and sends the recording unchanged: a hand-written MediaCodec crop is only correct per vendor (decoder layout, chroma alignment, encoder input format, PTS) and cannot be validated on hardware here, so it stays documented as unlanded rather than shipping a transcode that corrupts uploads. A crop that outlives its route no longer leaves an unowned temporary: the export stays registered until it settles, and the recorder reclaims its file when it is disposed mid-export. The one file a caller accepted is forgotten at that moment, so cleanup can never delete a note that is already being sent.
VideoNoteCropService now tracks each export as a request instead of one "last export" slot: crop() leaves the produced file owned by the service, accept() hands it to the caller once the send path has taken it, and abandonInFlight() attaches deletion to the platform future itself, so an export that lands after the bounded cleanup wait gives up is still reclaimed. The recorder accepts the crop only after the recording it replaces has been deleted, and a disposed recorder drops the crop it resolved as well. iOS: centredSquareCrop is nonisolated, so the plain XCTest class no longer routes synchronous asserts through the main-actor dispatcher, and the export is extracted as a nonisolated async function the app and the tests share. The XCTest suite now drives real AVAssetExportSession exports of synthetic AVAssetWriter inputs for 0/90/180/270 degrees, asserting square output, duration and surviving audio. The 270-degree case's expectation was corrected (the maths was right; the assertion was not). Android still answers unsupported and the Dart side sends the original.
…it over The review's P2 flagged ownership leaking across the service/recorder gap: a recorder disposed between the service's delivery (crop resolved) and the recorder's handoff (file verified + recording deleted + accepted for sending) could leave an accepted-but-unsent crop that neither the global sweep nor the !mounted continuation touched, and the 5-second wait could outlive the reclaim of a late platform success. Boundaries, precisely closed here, building on 489b73f's per-request ownership and its iOS isolation: * A live crop handle is now a per-request ticket (VideoNoteCropTicket) — not a single global "last export" slot. The service keeps the file until the caller either accepts it for the send path or explicitly abandons it. Cleanup rides the platform future's own whenComplete (the final reclaim is unconditional on that completion), so an export finishing long after the bounded wait gives up is still reclaimed. * The reclaim is memoized on the request: abandon / resolve / the global sweep / the platform completion all race the same single deletion future instead of each firing an unawaited one. The delivery flag (accepted) is only set after the file exists and the recording it replaces is gone (videoNoteUploadPath), so a recorder disposed between delivery and handoff still sees an unaccepted request. * The recorder no longer sweeps every in-flight export. videoNoteUploadPath threads its ticket through onStarted=, _finishRecording keeps the ticket, and dispose / the !mounted branch abandon exactly that ticket — so a second live recorder's accepted send file survives. The file-content effects are those already reviewed: a 60s 720p export bounded at 15s (timeout → send the recording), and a verified square output within 1px and 0.25 s of the source; out-of-scope failures fall back. Regression coverage, barrier-driven where needed: * "late beyond cleanup timeout is still reclaimed by the platform itself" — the crop timed out, cleanup already returned with Duration.zero, and the platform completed with a temp path long afterwards. The temp is deleted by the true completion; a second accepted send file is asserted to survive the late reclaim. Fails without the whenComplete-attached reclaim. * "dispose between delivery and handoff keeps (accepted) send file" — two ticks: a disposed flow's own crop is reclaimed (temp deleted, service delivery happened but handoff never completed), and an accepted flow's file survives that same gap (new VideoNoteCropTicket scope). An awaited-accept hook barriers drive delivery→handoff while the ticket is abandoned, and pumpEventQueue runs the file IO. * "a surrendered ticket does not touch another recorder's accepted crop" — two concurrent tickets; the dying one's ticket abandon deletes only the dying temp. Fails with the old global abandonInFlight sweep. Android still deliberately answers unsupported and the Dart side sends the uncropped recording — 44% wide on 1280×720, MediaExtractor → MediaCodec YUV scope documented for a future change, no bytes touched.
9867001 to
3570f37
Compare
Change
Round video messages are recorded at
ResolutionPreset.high— 1280×720 on both platforms — while the recorder preview and the received message bubble only ever show the centred square. So every uploaded note carried roughly 44% of its pixels in bands no client draws, and the file differed from what a client that records square produces.The recorder now crops the finished recording to its centred square through the existing
mithka/media_editorchannel, and pops the cropped path. A crop failure of any kind — missing editor, platform error, malformed reply, 15s timeout — falls back to the recording, so a note is never lost to a failed re-encode, and nothing is shown to the user.Crop ownership is per request, and it survives the whole service→recorder→send chain:
VideoNoteCropService.starthands back aVideoNoteCropTicketthat owns the export's file, the delivery flag is only set aftervideoNoteUploadPathhas verified the file and deleted the recording it replaces, and the reclaim is attached to the platform future's own completion (memoized per request) — so an export that finishes long after the five-second cleanup wait gave up is still deleted, while a file the send path accepted is never touched. The recorder keeps its own ticket and abandons exactly that one on dispose or on an unmounted finish, so a route being torn down cannot reclaim a second live recorder's crop.Affected platforms: iOS crops the file. Android still sends the recording uncropped — see below.
_finishRecording, afterstopVideoRecording()and before the screen pops, and only there: cancelling a recording still just stops and deletes the temp file.videoNoteUploadPath, so the fallbacks are tested without a platform channel; the channel contract is tested against a mock messenger.iOS.
AVAssetExportSessionatHighestQualitywith a video composition whose single layer instruction maps the centred display-space square onto a squarerenderSize. A camera recording carries its rotation inpreferredTransform— a portrait note is encoded 1280×720 and shown 720×1280 — so the square is computed in display space and the layer transform composes the preferred transform with a translation of the square's origin; the rotation and the crop are therefore one operation, and a rotated recording is neither upright-shifted nor off-centre.setCropRectangleis deliberately not used: it applies in the track's natural space, which leavespreferredTransform's translation pointing at the uncropped frame and pushes a 180° recording out of the render.The export is verified before its path is returned: the output must be square within 1px and within 0.25s of the source, and a mismatch deletes the output and reports an error (which the caller treats as "send the recording").
Android does not ship a transcode, deliberately. This app carries no media3/transformer dependency, so a square crop means a hand-written
MediaExtractor→MediaCodecdecode → centred plane copy →MediaCodecencode →MediaMuxerpipeline. That pipeline is only correct when, for each device, the decoder layout (planar I420 vs semi-planar NV12/NV21,rowStride/sliceHeightdiffering from the frame size, U/V sharing one buffer), the chroma 2×2 alignment of the crop origin, the encoder's accepted input format, and PTS monotonicity plus codec-config extraction are all handled. Getting any of it wrong produces a note that looks fine as a thumbnail and is green, torn, or silent in playback — worse than a note that is merely wide. None of it can be compiled or run on hardware here, socropVideoNoteanswersunsupported, Dart falls back, andVideoNoteCropper.ktrecords the exact pipeline and the per-device hazards for the implementation that replaces it. Android notes keep their 16:9 framing until then, and no Dart change is needed when they stop.Telegram API Terms classification
allowTelegramTosViolations(I_DONT_FUCKING_CARE_ABOUT_TOS), disabled in official builds, and available only in an explicitly enabled self-build.Reason and affected Telegram behavior:
Local media preparation only. The file is re-framed on the device before the existing send path receives it; no request is issued by the crop, no message data is read, and read status, typing status, last seen, self-destructing content, and actions taken on the user's behalf are unaffected.
Basic checks (every PR)
dart format --output=none --set-exit-if-changed lib/chat/video_note_crop_service.dart lib/chat/video_note_recorder_view.dart test/video_note_crop_test.dart.flutter analyze.flutter test --concurrency=4.Commands, results, platform/build/UI evidence, and reasons for inapplicable checks:
/tmp/flutter-3.44.2 is the toolchain Quality CI pins (Flutter 3.44.2).
flutter analyze→ no issues.--no-pub -j 1, one file per invocation):test/video_note_crop_test.dart(new) → 31/31 passed. Channel contract: the payload is{path: …}oncropVideoNote, a returned path is accepted, andMissingPluginException,PlatformException, a null reply, an empty path (which never reaches the channel), an echoed path, and a timeout all resolve tonull. Route-cancellation ownership: a crop whose caller gave up keeps its export registered, so a completion that lands afterwards has its file deleted by the reclaim attached to the platform future (expect(lateFile.existsSync(), isFalse)), while a crop that completed inside the window hands the path to its caller and leaves the file alone. New per-request-ticket regressions (review 5604259229):late beyond cleanup timeout is still reclaimed by the platform itself(crop timed out,abandon(timeout: Duration.zero)already returned, platform completes with the temp path long afterwards → temp deleted by the true completion while a second, accepted send file is asserted to survive the same late reclaim);dispose between delivery and handoff keeps the accepted send file/… alive(a recorder disposed between the service delivering the crop and the send path taking it has exactly its own crop reclaimed — temp deleted — while the accepted flow's file survives the same gap, driven through the ticket thatvideoNoteUploadPathhands out); anda surrendered ticket does not touch another recorder's accepted crop(two concurrent tickets; abandoning the dying one deletes only its own temp, which the old globalabandonInFlightsweep used to take the live one's file too).isSquareVideoNote: 720×720 and 721×720 are square within the tolerance, 1280×720 and 720×1280 are not, and 0 or negative dimensions are not.videoNoteUploadPath: the cropped path wins and the recording is deleted; a null crop, a throwing crop, a cropped path that does not exist, and a crop equal to the recording all keep the recording (and never delete the file being sent). Source pins: the crop is reached from_finishRecordingand not from_cancelRecording, the popped result uses the resolved path, the recorder threads its own ticket (no global sweep in_finishRecording), andacceptruns only after the recording it replaces is deleted.test/video_note_recorder_test.dart→ passed.test/video_trim_service_test.dart→ 2/2 passed (the existingtrimVideocontract still holds after the channel handler was restructured into aswitch/when).test/telegram_ai_surface_style_test.dart→ passed (it source-pins the recorder file).cropVideoNotecase to the existingmithka/media_editorchannel inAppDelegate.swift; Android adds the same case answeringunsupported. No dependency, Gradle, or pubspec change; no native build file touched; no string added or changed, andpython3 tool/check_l10n_strings.pyreports none.ios/RunnerTests/RunnerTests.swift, wired into theRunnerscheme), so the natural place for 0/90/180/270-degree synthetic-input cases is aRunnerTestscase that builds a small H.264 fixture, runscropVideoNoteand asserts the square/duration/audio result — but this environment has no Apple toolchain (swiftcandxcodebuildare both absent) and cannot run that target. A credential-free simulator workflow was prepared for the fork to run it; GitHub refuses to run any workflow onTiaraBasori/mithka(HTTP 422 Actions has been disabled for this repositoryon dispatch, no run created by a push trigger, whileactions/permissionsstill reportsenabled: true, allowed_actions: all), and the fork has no self-hosted macOS runner. The probe was removed and the fork's master restored. So: the exporter's orientation/audio behaviour stays unproven on hardware, the Dart-side fallback and the output verification are what keep a bad native result from being sent, and that is the accurate state of this PR.AVAssetExportPresetHighestQualityaccepts HEVC/HDR camera output — is unverified; every such failure falls back to the recording instead of failing the send.Line counts (
git diff --numstat origin/master...HEAD):31 19 android/app/src/main/kotlin/ad/neko/mithka/MainActivity.kt,52 0 android/app/src/main/kotlin/ad/neko/mithka/VideoNoteCropper.kt,313 25 ios/Runner/AppDelegate.swift,674 1 ios/RunnerTests/RunnerTests.swift,307 0 lib/chat/video_note_crop_service.dart,141 2 lib/chat/video_note_recorder_view.dart,595 0 test/video_note_crop_test.dart.Section 1.4 exception checks
Complete every item for an exception. For a compliant/unchanged PR, write
Not applicablehere.Not applicable — compliant/unchanged classification; no guarded feature is added or touched.
--dart-define=I_DONT_FUCKING_CARE_ABOUT_TOS=false, the feature is absent and ordinary Telegram behavior remains intact.--dart-define=I_DONT_FUCKING_CARE_ABOUT_TOS=true, the feature works through the guarded entry points in a self-build; I included relevant test and build/UI evidence.falseand have no option that enables it. All store versions remain compliant with applicable Telegram API Terms.Guard locations and evidence for each flag variant:
Not applicable.
Agentic reviewer confirmation
Agentic code reviewer only: record this confirmation in the PR review or check these boxes before approval.