Skip to content

fix(tab5): match camera geometry to PPA scale - #49

Merged
vincentkoc merged 5 commits into
mainfrom
fix/tab5-camera-quantized-geometry
Sep 12, 2026
Merged

fix(tab5): match camera geometry to PPA scale#49
vincentkoc merged 5 commits into
mainfrom
fix/tab5-camera-quantized-geometry

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Sep 9, 2026

Copy link
Copy Markdown
Member

Problem

The Tab5 camera transform calculated JPEG dimensions independently of the PPA's
four-bit fractional scale. With the rotated 1280x720 sensor image and
maxWidth: 640, it described 640x1136 pixels while the PPA wrote only 630x1120.
The larger output pitch and packed-image size included an unwritten margin.
The allocation is zero-initialized; this is not a demonstrated heap disclosure.

Change

  • Choose one uniform scale rounded down to a multiple of 1/16.
  • Derive the rotated output dimensions from that exact scale and use them for
    PPA pitch, packed RGB byte count, JPEG encoding and response metadata.
  • Preserve the width ceiling, no upscaling, pixel bounds, cache-aligned
    allocation, blocking transform and cleanup.

Stacked on #48
at 5e66310797e87d35684d8e61b9dba324c5b9cb66. No SDK, managed-component,
configuration, tint, warm-up, capture, audio or provider changes.

Validation

  • The actual production transform failed on the base with:
    packed image 640x1136 differs from PPA-written extent 630x1120.
  • All six camera test methods pass with ASan/UBSan and compiler warnings treated
    as errors. The PPA fixture writes only its quantized extent using the configured
    pitch; coverage includes native/minimum scales, all right-angle rotations,
    width/pixel bounds and allocation/transform failure cleanup.
  • git diff --check passes.
  • Fresh P2 autoreview and independent final diff review found no actionable
    P0-P2 issues in the frozen four-file candidate.

The normal CI run, attempt 1
passed all five jobs at head b05b470fabf54fbc9aac687488eafb76e5fc2814,
including Tab5 and Waveshare. Tab5 firmware and symbols uploaded successfully;
only the firmware artifact was downloaded for verification.

Tab5 artifact 10121391198 is 2,898,286 bytes and matches GitHub's SHA256
abcea107a6d2ec580784933de9574796f45d29097156303117eec23c297a5bb4.
All 11 internal checksums, manifest file sizes/hashes and four offset-mapped
images passed verification. The manifest identifies compiled test merge
2e75363465cbbe39f7e3ff777db3ffe54864151b; its immutable commit has the expected
PR48 base and this PR's head as parents, with the same tree as this head.

Generated configuration and the complete parsed dependency lock match the
previously tested 079791093d6dddb2eced111da3a3685ecb995fe1 image. There is no new
dependency drift, including dl_fft, and the existing SDK, SDIO and camera
patch records match. The SDK remains v5.5.5-648-g362a1776ec2-dirty, qualified
by the existing tracked SPM/NVS patches, not pristine release 5.5.5. The retained
post-Ninja camera mapper/object evidence is unchanged. Symbols were not
downloaded and object bytes were not independently inspected.

Bounded Live Validation

The same compiled 2e75363465cbbe39f7e3ff777db3ffe54864151b image completed
bounded keyless boot/reconnect validation: 150.994 seconds of capture, with
stability windows of 60.148 and 50.054 seconds. No parser, assert, panic, SDIO
or NVS I/O fault was observed in that capture. These windows are not endurance
qualification. An earlier framing-unknown capture remains unresolved; this
change does not claim a framing/parser fix.

One actual camera.snap succeeded on September 9, 2026 at 20:49:46.898 UTC,
producing a 630x1120 JPEG of 46,818 bytes. Direct comparison of the retained
079 and new images showed that the old black right/bottom borders and noisy
lower-right strip were absent in the new image. This is a verified result for
that capture, not a guarantee for every operating condition.

The green/dim cast remains. Color/exposure quality, orientation, physical
privacy-indicator presentation and sensor power-off remain unqualified.
External screen footage was not readable enough to establish UI or privacy
indicator proof. No provider or Talk calls were made in these runs, so voice
and complete media behavior are not qualified by this evidence.

Cleanup completed with a normal exit and complete receipts; test-owned
processes and capture handles were released. Those trials did not change
source or CI.

Landing

Accept this bounded geometry repair on its actual-owner regression and exact
historical image evidence. Landing uses a merge commit after predecessor
#48, with an exact cumulative
tree check that preserves the accepted privacy and packaging repairs and the
qualified Tab5 image pin. Tree equivalence is not new hardware coverage.
The current test merge has main cf600d8206b4bc2e7694787fb12508bc9ab96b63
and unchanged head b05b470fabf54fbc9aac687488eafb76e5fc2814 as parents.
Its tree 53e178f13c884415fb08b9032fbc7d79cb6b7ba1 equals that main plus
the frozen geometry delta, including the already-merged ordinary packaging
correction. The inherited packaging finding is therefore resolved in the
landing tree without rewriting the original signed geometry head.
The color and media limitations above remain open; this does not authorize
a release or establish end-to-end voice qualification.

@clawsweeper

clawsweeper Bot commented Sep 9, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 9, 2026
@clawsweeper

clawsweeper Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codex review: blocked before merge. Reviewed September 12, 2026, 12:10 PM ET / 16:10 UTC (Revision 6).

ClawSweeper review

What this changes

The branch aligns Tab5 JPEG dimensions with hardware scaling and includes stacked camera compatibility patches, media diagnostics, packaging verification, and regression tests.

Merge readiness

Blocked before merge - 4 items remain

The geometry repair remains necessary and has credible bounded hardware proof. The previously reported packaging regression still exists in the pinned head; the stated dependency landing plan addresses it, but has not yet been incorporated.

Priority: P2
Reviewed head: b05b470fabf54fbc9aac687488eafb76e5fc2814

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) Strong bounded camera evidence supports the repair, but the existing packaging regression limits the cumulative patch's readiness.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (live_output): The captured body connects the production camera.snap transform to the identified Tab5 image and an observed 630x1120 JPEG with the previous edge artifacts absent. This supports the bounded geometry repair, without extending that proof to a future cumulative tree, full camera quality, or voice behavior.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Verified Sufficient (live_output): The captured body connects the production camera.snap transform to the identified Tab5 image and an observed 630x1120 JPEG with the previous edge artifacts absent. This supports the bounded geometry repair, without extending that proof to a future cumulative tree, full camera quality, or voice behavior.
Evidence reviewed 9 items Current main still needs the geometry repair: At fetched main, transform_camera_frame independently aligns output dimensions to eight pixels and then derives floating-point scales. The PR instead derives the dimensions and scale from the same integer number of sixteenths.
SDK dependency signal and production repair: The changed transform directly calls ppa_do_scale_rotate_mirror. It uses one quantized scale for both axes and matching dimensions for allocation, output pitch, packed data size, and response metadata; the capture caller retains its existing admission and cleanup boundaries.
Authoritative PPA scaling implementation: The pinned SDK truncates fractional scales and calculates rotated output extents from those quantized values, supporting the production repair's calculation.
Findings 1 actionable finding [P1] Preserve resolved-lock verification for unpatched camera builds
Security None None.

How this fits together

The Tab5 board handles camera requests by capturing sensor frames, rotating and scaling them with the pixel-processing accelerator, and encoding a JPEG. Firmware packaging separately verifies the SDK and camera-component inputs used to build the device image.

flowchart LR
  A[Camera request and width limit] --> B[Sensor capture]
  B --> C[Quantized hardware scaling]
  C --> D[Packed pixels and JPEG dimensions]
  D --> E[JPEG response]
  F[SDK and component inputs] --> G[Firmware verification]
  G --> B
Loading

Before merge

  • Preserve resolved-lock verification for unpatched camera builds (P1) - This call also runs when camera_compat is false, but its verifier unconditionally requires esp_video 2.4.1, fixed hashes, and the mapper layout. The application permits ^2.1, so an otherwise valid ordinary build resolving another compatible version fails packaging without opting into the repair. The prior finding remains in this head; carry forward the separate unpatched verifier already implemented in fix(tab5): align CSI input with ISP output #48 while retaining explicit repair pins.
  • Resolve merge risk (P1) - Ordinary Tab5 builds resolving a valid camera component other than the repair's exact pin currently fail packaging without opting into that repair.
  • Resolve merge risk (P1) - The conflicting cumulative landing tree has not been verified; preservation of the accepted packaging correction, privacy safeguards, and qualified firmware-image pin remains an integration requirement.
  • Complete next step (P2) - After fix(tab5): align CSI input with ISP output #48 lands, integrate its packaging correction, resolve this branch's conflicts, and verify the cumulative tree preserves the accepted privacy repairs and qualified Tab5 image pin.

Findings

  • [P1] Preserve resolved-lock verification for unpatched camera builds — scripts/package_firmware.py:225-230
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Introduced code growth Production +480 net lines; tests +1,501 net lines The cumulative stack justifies growth through camera compatibility and diagnostics; these counts exclude documentation and workflow changes.

Root-cause cluster

Relationship: canonical
Canonical: #49
Summary: This PR owns the geometry correction within a stack of distinct Tab5 repairs.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge-risk options

Maintainer options:

  1. Carry forward the accepted dependency repair (recommended)
    Land the predecessor first, then integrate its ordinary-packaging correction and verify the cumulative tree preserves accepted safeguards and build pins.

Technical review

Best possible solution:

Retain the shared quantized geometry calculation while carrying forward the predecessor's resolved-lock packaging verification, privacy repairs, and qualified build inputs.

Do we have a high-confidence way to reproduce the issue?

Yes: the production calculation and pinned SDK establish the geometry mismatch, while the packaging call chain proves rejection of otherwise valid alternative camera versions. This review did not execute firmware or tests.

Is this the best way to solve the issue?

Yes for the geometry calculation; the cumulative branch also needs the predecessor's existing packaging correction before it is a safe landing candidate.

Full review comments:

  • [P1] Preserve resolved-lock verification for unpatched camera builds — scripts/package_firmware.py:225-230
    This call also runs when camera_compat is false, but its verifier unconditionally requires esp_video 2.4.1, fixed hashes, and the mapper layout. The application permits ^2.1, so an otherwise valid ordinary build resolving another compatible version fails packaging without opting into the repair. The prior finding remains in this head; carry forward the separate unpatched verifier already implemented in fix(tab5): align CSI input with ISP output #48 while retaining explicit repair pins.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.97

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning medium; reviewed against 6b63951f0ac5.

Labels

Label justifications:

  • P2: The central repair addresses a bounded Tab5 camera geometry defect with recorded hardware impact.
  • merge-risk: 🚨 compatibility: The pinned head imposes repair-specific camera identity requirements on ordinary firmware packaging.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (live_output): The captured body connects the production camera.snap transform to the identified Tab5 image and an observed 630x1120 JPEG with the previous edge artifacts absent. This supports the bounded geometry repair, without extending that proof to a future cumulative tree, full camera quality, or voice behavior.
  • proof: sufficient: Contributor real behavior proof is sufficient. The captured body connects the production camera.snap transform to the identified Tab5 image and an observed 630x1120 JPEG with the previous edge artifacts absent. This supports the bounded geometry repair, without extending that proof to a future cumulative tree, full camera quality, or voice behavior.

Evidence

What I checked:

  • Current main still needs the geometry repair: At fetched main, transform_camera_frame independently aligns output dimensions to eight pixels and then derives floating-point scales. The PR instead derives the dimensions and scale from the same integer number of sixteenths. (examples/m5stack-tab5-room-node/components/tab5_room_board/tab5_room_board.c:950, 6b63951f0ac5)
  • SDK dependency signal and production repair: The changed transform directly calls ppa_do_scale_rotate_mirror. It uses one quantized scale for both axes and matching dimensions for allocation, output pitch, packed data size, and response metadata; the capture caller retains its existing admission and cleanup boundaries. (examples/m5stack-tab5-room-node/components/tab5_room_board/tab5_room_board.c:951, b05b470fabf5)
  • Authoritative PPA scaling implementation: The pinned SDK truncates fractional scales and calculates rotated output extents from those quantized values, supporting the production repair's calculation. (components/esp_driver_ppa/src/ppa_srm.c:235, 362a1776ec21)
  • Prior packaging finding remains: Ordinary Tab5 packaging calls verify_camera_patch with patched=false, but inspect_component still requires version 2.4.1, fixed component and manifest hashes, and the mapper layout. The application's main/idf_component.yml permits esp_video ^2.1. The previously reviewed SHA and current checkout are identical. (scripts/package_firmware.py:225, b05b470fabf5)
  • Dependency already supplies the narrow correction: fix(tab5): align CSI input with ISP output #48 remains open. Its current source separates ordinary resolved-lock integrity verification from explicit repair verification, preserving strict repair pins. This PR's body explicitly requests landing after that predecessor. (scripts/package_firmware.py:229, 620a13e63168)
  • Captured hardware proof: The supplied body snapshot, sourceRevision 9b88245cf08ffcfb2a3f86cbc2b4b3cbfe2ab64421699e514d817a87e08c5c52, records camera.snap at 2026-09-09T20:49:46.898Z on compiled image 2e75363: a 630x1120 JPEG of 46,818 bytes, with the earlier edge artifacts absent. It identifies the artifact hash and explicitly limits color, privacy-indicator, endurance, and voice claims. These are retained contributor observations, not a new reviewer execution. (b05b470fabf5)

Likely related people:

  • Vincent Koc: Raw commit 3269794 adds examples/m5stack-tab5-room-node/components/tab5_room_board/tab5_room_board.c:720 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 32697940a996; files: examples/m5stack-tab5-room-node/components/tab5_room_board/tab5_room_board.c)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Incorporate the predecessor's ordinary-packaging correction and retain coverage for valid alternative resolved versions and strict repair rejection.
  • Verify the resolved cumulative tree preserves accepted privacy safeguards and the qualified firmware-image pin.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (5 earlier review cycles)
  • reviewed 2026-09-09T19:27:07.573Z sha b05b470 :: blocked before merge. :: none
  • reviewed 2026-09-09T19:39:47.484Z sha b05b470 :: blocked before merge. :: none
  • reviewed 2026-09-09T19:50:32.257Z sha b05b470 :: needs maintainer review before merge. :: none
  • reviewed 2026-09-09T20:58:16.583Z sha b05b470 :: needs changes before merge. :: none
  • reviewed 2026-09-12T16:01:49.001Z sha b05b470 :: blocked before merge. :: [P1] Preserve resolved-lock verification for unpatched camera builds

@clawsweeper clawsweeper Bot added the proof: sufficient Contributor real behavior proof is sufficient. label Sep 9, 2026
@vincentkoc
vincentkoc changed the base branch from fix/tab5-csi-post-isp-format to main September 12, 2026 15:57
@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 12, 2026
@vincentkoc
vincentkoc marked this pull request as ready for review September 12, 2026 16:06
@vincentkoc
vincentkoc merged commit 94461a6 into main Sep 12, 2026
16 of 17 checks passed
@vincentkoc
vincentkoc deleted the fix/tab5-camera-quantized-geometry branch September 12, 2026 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant