Repository navigation
fix(chat): fill the video-message preview instead of double-applying its ratio - #256
Conversation
Automated PR reviewHead: Review unavailable: Waiting for the exact-head Quality run before using the free-model quota. 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. |
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. |
iebb
left a comment
There was a problem hiding this comment.
[P2] Preserve the camera texture's orientation-dependent ratio, not the window's ratio.
In video_note_recorder_view.dart, _preview passes landscape: viewport.width > viewport.height to videoNotePreviewFootprint, then tightly constrains CameraPreview to that footprint. But CameraPreview orients its texture using the camera value's recording/paused/locked/device orientation, not the window's dimensions (upstream implementation: https://github.com/flutter/packages/blob/main/packages/camera/camera/lib/src/camera_preview.dart). A landscape Stage Manager window while the camera remains portrait therefore forces the portrait preview into the landscape-sized box; the inner AspectRatio cannot override tight constraints. BoxFit.cover then uniformly scales an already distorted child.
Please derive the footprint from the actual preview/camera orientation and cover-fit it without changing that aspect ratio, or explicitly apply any required rotation. Add an integration-level widget regression using the camera platform/controller/CameraPreview path with camera orientation disagreeing with viewport orientation, including the locked recording orientation. The current new tests exercise only the geometry helper and encode the desired footprint; they cannot detect texture stretching. This is source-traced review evidence, not a claim of device QA.
60af414 to
0e01bea
Compare
|
Rebased onto current master at head The [P2] fix itself is unchanged from the previous revision: the footprint follows |
iebb
left a comment
There was a problem hiding this comment.
Re-reviewed head 0e01bea. The window/camera orientation distinction is improved, but two production boundaries remain. [P2] The helper at lib/chat/video_note_recorder_view.dart:121-138 does not match the pinned camera 0.12.0+2 package for a portrait previewSize: CameraValue.aspectRatio is width/height, and CameraPreview uses landscape ? aspectRatio : 1/aspectRatio, without the extra portrait comparison. For previewSize=720x1280 and portraitUp, the package chooses 16:9 but this helper/footprint forces 9:16. The new tests assert the helper's expected number rather than comparing the actual CameraPreview box. [P2] _preview at 453-483 reads controller.value once in LayoutBuilder, but does not listen to the controller. A device/locked/paused orientation update while the Stage Manager window stays the same size rebuilds the inner CameraPreview, not the tight outer footprint; the ratios can diverge again. Please derive the same ratio as the pinned package and rebuild the outer footprint on camera-value changes, with a real CameraPreview/fake CameraPlatform or controller regression changing orientation without resizing the window (also cover portrait previewSize and recording/locked/paused orientation). These are static current-source/package-contract traces, not a newly executed device reproduction. Genuine improvement; keep open for the focused revision.
|
Addressed at head The helper now matches the pinned package exactly. The outer footprint now listens to the controller. The preview is a public The tests compare the actual box, not the helper's arithmetic. A fake
Mutation checks, quoted: with the old The PR body's line counts and evidence now come from |
460b8c8 to
b2e73bc
Compare
|
Source review of b2e73bc: the current preview-ratio/orientation listener changes address the earlier framing findings and look net positive. Before an exact-head approval/publication, please update this branch against current master 0c616aa so ordinary pull_request Quality attests the new head/base merge. The last green Quality run still attests base 3c9fcb1. The connected maintainer account cannot update this contributor branch (GitHub rejects update-branch permission); no gates were bypassed. Physical camera framing remains a device QA item. |
…its ratio The preview was sized from the window's orientation while the camera package sizes its own box from the camera's applicable orientation (recording, paused preview, locked capture, or device). The two disagree on a portrait sensor in a landscape Stage Manager window, so a tight constraint forced a portrait texture into a landscape-sized box and BoxFit.cover then scaled an already distorted child. The footprint now follows the ratio the preview box itself uses (videoNotePreviewAspectRatio), which covers the window without ever scaling an axis the texture does not carry. The ratio is derived from the same orientation chain the package applies, including the locked recording orientation.
b2e73bc to
f925933
Compare
iebb
left a comment
There was a problem hiding this comment.
Reviewed exact head f925933 under the maintainer’s explicit delegated review authority. This is a genuine net-positive presentation fix. The prior requested changes are resolved: the ratio follows the pinned CameraPreview orientation arithmetic (including portrait previewSize), and ValueListenableBuilder updates the outer cover-fit footprint when camera state changes without a window resize. The real CameraController/CameraPreview fake-platform regressions cover these boundaries. Capture, send behavior, dependencies and native code are unchanged; Terms classification is compliant/unchanged and the contribution checklist is complete. Verified ordinary PR Quality run 40063038085 passed for tested merge ba7c3b3 with parents live master 0c616aa and this exact head; no unresolved review threads. CI evidence, not a claim of locally rerun tests or physical-camera verification. This approval supersedes my earlier change requests on older heads. Publication remains subject to every guarded workflow recheck.
|
Merged the exact validated result |
Change
On iPad the live video-message preview — the round-recording camera reached from the composer's voice affordance — showed the sensor frame at the wrong ratio: letterboxed in a box that did not match the picture, and visibly wrong under Stage Manager or split view.
The recorder wrapped
CameraPreviewin its ownAspectRatio(controller.value.aspectRatio), but the camera package already wraps its texture in anAspectRatiobuilt from the camera's applicable orientation. Two consequences: the ratio was applied twice, and the outer copy had no portrait inversion, so its box was landscape while the inner preview was portrait. That outer copy also readcontroller.valueonce inside aLayoutBuilder, so a recording, lock or device-orientation change never rebuilt it at all.Affected platform: every platform the video-message recorder runs on (Android and iOS); the iPad is where the disagreement becomes obvious.
videoNotePreviewAspectRatio(CameraValue)returns exactly the ratioCameraPreviewputs on its own box — same applicable-orientation chain (recording → paused preview → locked capture → device), samelandscape ? aspectRatio : 1 / aspectRatioarithmetic againstpreviewSize.width / previewSize.height— andnullwhile the camera has not reported a preview size, where the package draws a bareContainerand the recorder falls back to filling its window instead of inventing a ratio.VideoNotePreviewBoxwraps that footprint in aValueListenableBuilder<CameraValue>over the controller (aValueNotifier<CameraValue>), so a recording/lock/paused/device-orientation change with an unchanged window size rebuilds the outer cover-fit box too, not only the innerCameraPreview.BoxFit.coverand a hard clip when it crops. A recorder must fill its window with live pixels, so the preview never letterboxes and never stretches.AspectRatiostays inside a tightSizedBox, where it resolves to the box it is given, so exactly one scale reaches the texture. The control overlay, the shutter disc, the round buttons and every gesture are untouched.Known gap, deliberately not in this PR: the recorded file is still 16:9, not square.
ResolutionPreset.highmaps to a 1280x720 capture on both platforms,_finishRecordingpops the path untouched, andsendVideoNotesends it without a crop — the circle is only a UI mask. #260 carries the native square crop on the existingmithka/media_editorchannel.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:
Presentation only. The change decides how the existing camera texture fills the recorder's box; capture resolution, the recorded file, and the send path are unchanged, so what Telegram receives is identical. It issues no request and touches no message data.
Basic checks (every PR)
dart format --output=none --set-exit-if-changed lib/chat/video_note_recorder_view.dart test/video_note_recorder_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).
/tmp/flutter-3.44.2/bin/dart format --output=none --set-exit-if-changed lib/chat/video_note_recorder_view.dart test/video_note_recorder_test.dart→Formatted 2 files (0 changed), exit 0./tmp/flutter-3.44.2/bin/flutter analyze lib/chat/video_note_recorder_view.dart test/video_note_recorder_test.dart→No issues found!./tmp/flutter-3.44.2/bin/flutter test --no-pub -j 1 test/video_note_recorder_test.dart→ 22/22 passed. The suite drives a realCameraControllerand the realCameraPreviewthrough a fakeCameraPlatform, and the ratio assertions readtester.getSize(find.byType(CameraPreview))plus theRenderAspectRatiodry layout instead of the helper's own return value, so a texture-stretching box would fail them.a portrait previewSize covers the box at the package ratio— 720x1280 withportraitUp: the package's arithmetic (landscape ? ratio : 1/ratiowithratio = 720/1280) gives 16:9, and the box the widget actually occupies measures 16:9. This is the exact fixture the earlier revision got wrong (it forced 9:16).a landscape previewSize keeps the package ratio— 1280x720 in a portrait window: box stays 9:16.locked, paused and recording orientations each win over the device one— each input flips the measured box, not just the helper value.recording a different orientation rebuilds the footprint— the controller's value changes orientation while the window size is untouched, and the outer footprint (and therefore theCameraPreviewbox) flips ratio. This is the second boundary: it fails if the box is not rebuilt on camera-value changes.no preview size yet fills the window instead of inventing a ratio—previewSize == null→ the viewport, no throw (the package's own getter force-unwraps it).landscape == portrait ? 1/ratio : ratioarithmetic failsa portrait previewSize covers the box at the package ratioattest/video_note_recorder_test.dart:248withExpected: <1.7777…> Actual: <0.5625>, and the real-box case at:610with the same numbers; replacing theValueListenableBuilderwith a plainBuilderfailsrecording a different orientation rebuilds the footprintat:610withExpected: <1.7777…> Actual: <0.5625>. Both mutations were reverted and the suite re-run green.assets/l10nor generated file is touched.CameraPreview; whether the framing looks right with a physical sensor on an iPad is a device judgement, and the change deliberately keeps every control in the same place so that check is about framing alone.Line counts (
git diff --numstat origin/master...HEAD, headf925933e):