Skip to content

fix(chat): fill the video-message preview instead of double-applying its ratio - #256

Merged
github-actions[bot] merged 2 commits into
iebb:masterfrom
TiaraBasori:fix/video-note-preview-ratio
Oct 24, 2026
Merged

github-actions[bot] merged 2 commits into
iebb:masterfrom
TiaraBasori:fix/video-note-preview-ratio

Conversation

@TiaraBasori

@TiaraBasori TiaraBasori commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

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 CameraPreview in its own AspectRatio(controller.value.aspectRatio), but the camera package already wraps its texture in an AspectRatio built 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 read controller.value once inside a LayoutBuilder, 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.

  • The outer ratio is gone. A pure videoNotePreviewAspectRatio(CameraValue) returns exactly the ratio CameraPreview puts on its own box — same applicable-orientation chain (recording → paused preview → locked capture → device), same landscape ? aspectRatio : 1 / aspectRatio arithmetic against previewSize.width / previewSize.height — and null while the camera has not reported a preview size, where the package draws a bare Container and the recorder falls back to filling its window instead of inventing a ratio.
  • VideoNotePreviewBox wraps that footprint in a ValueListenableBuilder<CameraValue> over the controller (a ValueNotifier<CameraValue>), so a recording/lock/paused/device-orientation change with an unchanged window size rebuilds the outer cover-fit box too, not only the inner CameraPreview.
  • The footprint is scaled to cover the recorder's area with BoxFit.cover and a hard clip when it crops. A recorder must fill its window with live pixels, so the preview never letterboxes and never stretches.
  • The package's own AspectRatio stays inside a tight SizedBox, 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.high maps to a 1280x720 capture on both platforms, _finishRecording pops the path untouched, and sendVideoNote sends it without a crop — the circle is only a UI mask. #260 carries the native square crop on the existing mithka/media_editor channel.

Telegram API Terms classification

  • Compliant/unchanged: this change complies with applicable Telegram API Terms, preserves Telegram's basic functionality, and does not introduce behavior covered by section 1.4.
  • Section 1.4 exception: the entire feature is guarded by allowTelegramTosViolations (I_DONT_FUCKING_CARE_ABOUT_TOS), disabled in official builds, and available only in an explicitly enabled self-build.
  • Disallowed or unresolved: this PR must not be approved or merged until its classification and implementation are resolved.

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)

  • I reviewed the diff for correctness, unintended behavior, secrets, and unrelated changes.
  • Formatting passed, or I recorded why it is inapplicable: dart format --output=none --set-exit-if-changed lib/chat/video_note_recorder_view.dart test/video_note_recorder_test.dart.
  • Analysis passed, or I recorded why it is inapplicable: flutter analyze.
  • Relevant tests passed, or I recorded why they are inapplicable; name the commands and test cases below. Quality CI runs flutter test --concurrency=4.
  • I checked affected build/platform paths and applicable localization/generated files; list the evidence below.
  • I completed the Terms classification above and verified that official builds cannot enable section 1.4 exceptions.

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).

  • Format: /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.
  • Analysis: /tmp/flutter-3.44.2/bin/flutter analyze lib/chat/video_note_recorder_view.dart test/video_note_recorder_test.dart → No issues found!.
  • Tests: /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 real CameraController and the real CameraPreview through a fake CameraPlatform, and the ratio assertions read tester.getSize(find.byType(CameraPreview)) plus the RenderAspectRatio dry 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 with portraitUp: the package's arithmetic (landscape ? ratio : 1/ratio with ratio = 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 the CameraPreview box) 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).
    • Geometry cases keep their previous coverage (cover-fit vs letterbox, square sensor, large iPad viewports in both orientations, the existing gesture case).
  • Mutation checks, quoted: restoring the old landscape == portrait ? 1/ratio : ratio arithmetic fails a portrait previewSize covers the box at the package ratio at test/video_note_recorder_test.dart:248 with Expected: <1.7777…> Actual: <0.5625>, and the real-box case at :610 with the same numbers; replacing the ValueListenableBuilder with a plain Builder fails recording a different orientation rebuilds the footprint at :610 with Expected: <1.7777…> Actual: <0.5625>. Both mutations were reverted and the suite re-run green.
  • Localization/generated files: not applicable — no string and no assets/l10n or generated file is touched.
  • Build/platform paths: no native file, no dependency change, no platform channel change. Verified at widget level against the real 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, head f925933e):

198    6  lib/chat/video_note_recorder_view.dart
669    0  test/video_note_recorder_test.dart

@github-actions

github-actions Bot commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Automated PR review

Head: f925933e0fe6630733b2bcfa7a0e23099a68cb59 · master: 0c616aac9ede554c8ec564b8c8dd03b0eb6d2069
LLM recommendation: needs_human. This is not a GitHub approval.
Auto-merge: blocked. Only iebb can authorize the current head.

Review unavailable: Waiting for the exact-head Quality run before using the free-model quota.

Remaining gates

  • Waiting for iebb to approve this exact current head.
  • A reviewer still requests changes; obtain a superseding review or dismiss it explicitly.
  • The latest exact-head Quality PR run must pass.
  • All current head checks must complete successfully (no skipped/neutral substitutes).

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.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ⚠️ Failed 2026-10-18T19:31:10.237219Z 541cf4a PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@iebb iebb left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@TiaraBasori
TiaraBasori force-pushed the fix/video-note-preview-ratio branch 2 times, most recently from 60af414 to 0e01bea Compare October 21, 2026 10:07
@TiaraBasori

Copy link
Copy Markdown
Contributor Author

Rebased onto current master at head 0e01bea so the branch builds against the recorder chrome master landed meanwhile (test/video_note_recorder_test.dart gained a widget case for it, and _preview's neighbours moved). The rebase resolved to master's test block plus this branch's geometry cases; flutter test --no-pub -j 1 test/video_note_recorder_test.dart → 15/15, format Formatted 2 files (0 changed), analyze No issues found!. The PR body's line counts and the test counts now come from git diff --numstat origin/master...HEAD at this head.

The [P2] fix itself is unchanged from the previous revision: the footprint follows videoNotePreviewAspectRatio(value) — the same orientation chain CameraPreview uses (recording → paused preview → locked capture → device) — instead of viewport.width > viewport.height, so a portrait texture is never forced into a landscape-sized box and BoxFit.cover never scales an axis the texture does not carry. The six videoNotePreviewAspectRatio cases and a portrait camera in a landscape window keeps its full frame pin it; the mutation check (landscape branch reduced to landscapeLeft only) still fails locked and recording orientations win over the device one.

@iebb iebb left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@TiaraBasori

Copy link
Copy Markdown
Contributor Author

Addressed at head 8be7d05c.

The helper now matches the pinned package exactly. videoNotePreviewAspectRatio(CameraValue) reproduces CameraPreview's own arithmetic and nothing else: the applicable-orientation chain (recording → paused preview → locked capture → device) and landscape ? aspectRatio : 1 / aspectRatio against previewSize.width / previewSize.height. The extra portrait comparison is gone, so the finding's fixture — previewSize = 720x1280, portraitUp — now yields 16:9, as the package does, not 9:16. When previewSize is null the helper returns null and the recorder fills its window: the package draws a bare Container there, and inventing 1:1 was my own invention, not the package's. sensorIsPortrait is gone with it.

The outer footprint now listens to the controller. The preview is a public VideoNotePreviewBox that wraps the whole footprint in a ValueListenableBuilder<CameraValue> over the controller (the controller is a ValueNotifier<CameraValue>), so a recording/lock/paused/device-orientation change with an unchanged window size rebuilds the tight outer box, not only the inner CameraPreview. _preview only measures the viewport now.

The tests compare the actual box, not the helper's arithmetic. A fake CameraPlatform drives a real CameraController and the real CameraPreview; assertions read tester.getSize(find.byType(CameraPreview)) and the RenderAspectRatio dry layout. Sixteen widget cases plus the three geometry/ratio groups, 22/22:

  • a portrait previewSize covers the box at the package ratio — 720x1280 portraitUp: measured box 16:9 (the number the previous revision got wrong).
  • a landscape previewSize keeps the package ratio, locked, paused and recording orientations each win over the device one — each input flips the measured box.
  • recording a different orientation rebuilds the footprint — the controller's value changes orientation while the window size is untouched; the outer box and the CameraPreview box flip. This is your second boundary.
  • no preview size yet fills the window instead of inventing a ratio — previewSize == null → the viewport, no throw.
  • The previous cover-fit cases (square sensor, large iPad viewports both ways, cover vs letterbox) are kept.

Mutation checks, quoted: with the old landscape == portrait ? 1/ratio : ratio arithmetic, a portrait previewSize covers the box at the package ratio fails at test/video_note_recorder_test.dart:248 with Expected: <1.7777…> Actual: <0.5625>, and the real-box case at :610 with the same numbers. Replacing the ValueListenableBuilder with a plain Builder fails the orientation-change case at :610 with Expected: <1.7777…> Actual: <0.5625>. Both reverted; 22/22 green. Format Formatted 2 files (0 changed), analyze No issues found!.

The PR body's line counts and evidence now come from git diff --numstat origin/master...HEAD at this head.

@TiaraBasori
TiaraBasori force-pushed the fix/video-note-preview-ratio branch 2 times, most recently from 460b8c8 to b2e73bc Compare October 24, 2026 02:01

iebb commented Oct 24, 2026

Copy link
Copy Markdown
Owner

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.
@TiaraBasori
TiaraBasori force-pushed the fix/video-note-preview-ratio branch from b2e73bc to f925933 Compare October 24, 2026 04:36

@iebb iebb left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions
github-actions Bot merged commit ba7c3b3 into iebb:master Oct 24, 2026
9 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

Merged the exact validated result ba7c3b3b7d267c3b6f61eb8dbdab8a875cb8afe1 into master after rechecking iebb's current-head approval. Head f925933e0fe6630733b2bcfa7a0e23099a68cb59, prior master 0c616aac9ede554c8ec564b8c8dd03b0eb6d2069. Full merged-result Quality run 40063038085 passed before publication; this job verified its attestation and did not execute PR code. Post-merge Quality has been requested separately; this comment does not claim it has passed.

@iebb iebb mentioned this pull request Oct 24, 2026
7 of 11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants