Skip to content

feat(processing): record what video each board's decoder will accept (1/3) - #3309

Open
mickzijdel wants to merge 5 commits into
Screenly:masterfrom
mickzijdel:feat/decode-envelope-gate
Open

mickzijdel wants to merge 5 commits into
Screenly:masterfrom
mickzijdel:feat/decode-envelope-gate

Conversation

@mickzijdel

@mickzijdel mickzijdel commented Aug 20, 2026 •

Copy link
Copy Markdown
Contributor

Splitting this as Viktor asked. Three PRs, smallest-risk first. This
one changes no behaviour at all, so it can be read against the
hardware measurements and nothing else.

The split

what it does lines
1 (this one) record what video each board's decoder accepts 1243
2 refuse an upload the decoder will not accept, show how to convert it 2161
3 warn about video already on a screen 456

Each needs the one before it. I cannot stack them as GitHub PRs, because a
pull request into this repo has to target a branch in this repo and I only
have read access, so part 2's diff would show part 1 again. So parts 2 and 3
go up as the one before them merges. The branches exist now if you want to
read ahead: feat/decode-envelope-upload-gate and
feat/decode-envelope-asset-list on my fork.

Issues Fixed

A 4K H.264 clip uploaded to a Pi 4 is accepted today and then plays at a few
frames per second. H.264 is in pi4-64's codec allowlist, and the 1080p
resolution cap only applies to boards under 1.5 GiB of RAM, so nothing in the
pipeline has an opinion about the stream itself. The codec map already names
this as an open defect, and says adding a cap is "a product call and a
separate change". This is that change.

Description

The codec gate answers "can this board decode this codec". It says nothing
about the stream, so this adds the second question: given the codec is right,
is the frame inside what the decoder can take?

Measured on a Pi 4B, a 4K High L5.1 clip at 116 Mbps against a 1080p
re-encode of the same content:

4K 1080p
hardware decode refused 75 fps
software, 4 cores 10 fps 37 fps
software, 1 thread 3 fps 12 fps

4K does not decode slowly in hardware, it never gets in. h264_v4l2m2m fails
outright and libavcodec drops to software, where 10 fps on an idle four-core
box is already under the 25 fps the clip needs, before the viewer spends
anything on presentation.

There are two frame bounds, and neither implies the other

v4l2-ctl reports Stepwise 32x32 - 1920x1920, and the first version of this
took that for the whole envelope. It is not. The block is a Level 4.1 decoder
and separately refuses any frame over that level's MaxFS of 8192
macroblocks
. Measured on the Screenly testbed, pi2 / pi3 / pi3-64 / pi4-64
agreeing exactly on driver 6.18.34:

frame macroblocks hardware
1920x1080 8160 ok
1456x1440 8190 ok
1440x1440 8100 ok
1472x1440 8280 refused
1920x1200 9000 refused
1920x1920 14400 refused
2560x720 7200 refused, on width

1456x1440 accepted against 1472x1440 refused is the pair that settles it: same
height, 16 pixels of width apart, one either side of 8192. Going the other
way, 2560x720 is only 7200 macroblocks and is still refused on width. 1080p
sits at 8160 of 8192, which is why the gap stayed hidden so long: Level 4.1 was
specified to fit 1080p, and almost nothing signage-shaped lands in the band
between.

Not gating on the declared H.264 level

This is the obvious-looking rule and the wrong one. The driver exposes
V4L2_CID_MPEG_VIDEO_H264_LEVEL read-only and never validates the bitstream
against it, and ordinary 1080p files carry inflated level tags constantly. The
testbed confirmed it from the other side: the accepted 1920x1080 and the
refused 1920x1200 clip both carried -level 5.1. The level is recorded for
diagnostics only.

A bitrate rule was drafted and then removed after measuring it: 1080p through
h264_v4l2m2m still runs at 62 fps at ~137 Mbps, so it would only ever have
flagged files that play fine.

Everything fails open

A dimension we could not measure, an unparseable pixel format, a board nobody
has characterised: no warning. A false positive costs an operator a working
asset and teaches them to ignore the badge. A false negative costs them the
silent few frames per second they already get. Given the choice this takes the
miss.

x86 and rockpi4 are deliberately in neither tier.

The docs change is the part that outlives this PR

Both the board-enablement guide and the anthias-hardware skill told the next
person to characterise a board by asking its decoder rather than timing a clip,
and the skill called that "better evidence than any playback measurement".
That advice is exactly what produced the macroblock gap. Characterisation is a
two-step method now, with the second step marked as the one not to skip, plus
the trick that made the boundary conclusive: bracket a suspected limit with two
frames 16 pixels apart rather than sampling one size.

Still open

  • The 3.7 to 8.3 Mpx band on a Pi 5 is unmeasured, and part 2's throughput cap
    refuses all of it. One reading at 3000x2000 on the Pi 5 would settle it
    and let the cap rise. Everything either side of that band is measured.
  • The Playwright integration suite has not been run locally.

Checklist

  • I have performed a self-review of my own code.
  • New and existing unit tests pass locally and on CI with my changes.
  • I have done an end-to-end test for Raspberry Pi devices.
  • I have tested my changes for x86 devices.
  • I added a documentation for the changes I have made (when necessary).

On the Pi testing: measured on a live Pi 4B, and the testbed run covered all
seven boards, including full uploads through a patched build on a 4 GB Pi 4.
On x86: no rules fire there by design, confirmed on the testbed's real x86
board, where resolve_device_key() returns x86 and no rule matches.

🤖 Generated with Claude Code

@mickzijdel
mickzijdel requested a review from a team as a code owner August 20, 2026 14:33
@codecov

codecov Bot commented Aug 20, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.22642% with 4 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (master@a8586a6). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/anthias_server/lib/playback_envelope.py 96.22% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master    #3309   +/-   ##
=========================================
  Coverage          ?   91.37%           
=========================================
  Files             ?       92           
  Lines             ?    11093           
  Branches          ?     1238           
=========================================
  Hits              ?    10136           
  Misses            ?      695           
  Partials          ?      262           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@vpetersson-bot

Copy link
Copy Markdown
Contributor

Ran this on the Screenly testbed — all seven boards (pi2, pi3, pi3-64, pi4-64, pi5, rockpi4, x86), against b13f621e. Every hardware question flagged in the PR description is now answered on real silicon. Most of the answers are good; one is a real gap.

Your extrapolations hold

v4l2-ctl -d /dev/video10 --list-framesizes=H264 returns Stepwise 32x32 - 1920x1920 with step 2/2 identically on pi2, pi3, pi3-64 and pi4-64 — same bcm2835-codec, driver 6.18.34, on all four. The capture queue enumerates YU12 / YV12 / NV12 / NV21 / NC12 plus three RGB formats, with nothing 10-bit, 4:2:2 or 4:4:4, so the 8-bit 4:2:0 restriction is confirmed too.

resolve_device_key() inside each server container returns exactly the expected key on all seven boards: pi4-64, pi3-64, pi3, pi2, pi5, x86, and arm64 -> rockpi4. The "every rule silently matches nothing" failure you were worried about does not happen anywhere.

Pi 5 has no H.264 decoder node at all — the only coded device is rpi-hevc-dec at /dev/video19, HEVC-only. Your software-decode premise and the "convert to HEVC, which this screen plays in hardware" remedy are both correct.

The gap: the frame bound is necessary but not sufficient

The driver enumerates 1920x1920, but h264_v4l2m2m also refuses anything over 8192 macroblocks — H.264 Level 4.1 MaxFS. Measured on all four bcm2835 boards:

frame macroblocks hardware
1920x1080 8160 ok
1088x1920 8160 ok
1456x1440 8190 ok
1440x1440 8100 ok
1472x1440 8280 REFUSED
1920x1104 8280 REFUSED
1920x1152 8640 REFUSED
1920x1200 9000 REFUSED
1472x1472 8464 REFUSED
1920x1920 14400 REFUSED
2560x720 7200 REFUSED (axis)
3840x544 8160 REFUSED (axis)

1456x1440 accepted against 1472x1440 refused is the decisive pair: same height, 16 pixels of width apart, straddling 8192. So it is not a height bound, and it is not the declared level either — my 1920x1080 and 1920x1200 clips both carry -level 5.1 and only the larger frame is refused. That is independent confirmation that not gating on the level tag was the right call.

per-axis <= 1920 AND ceil(w/16)*ceil(h/16) <= 8192 predicts 100% of outcomes on pi2, pi3, pi3-64 and pi4-64.

The gate's per-axis-only rule therefore accepts 1920x1200, 1920x1440, 1600x1600 and 1920x1920 — all hardware-refused, all silently falling back to software, which is the exact failure this PR exists to prevent.

Confirmed end to end, not just at the driver. Uploading through a patched build on a Pi 4 with host:total_mem_kb set to 4 GiB, so the pre-existing low-RAM cap does not mask it (same BCM2711 silicon as the 1 GB unit):

upload hardware gate
1920x1080 ok ACCEPTED correct
1456x1440 ok ACCEPTED correct
1472x1440 REFUSED ACCEPTED wrong
1920x1200 REFUSED ACCEPTED wrong
1920x1920 REFUSED ACCEPTED wrong
2560x720 REFUSED REJECTED correct

On the rig's actual 1 GB Pi 4 the low-RAM 1080p cap masks all of these, so the gap is only reachable on 2/4/8 GB Pi 4s — precisely the board in the original bug report. Software cost on the Pi 4 for the two most realistic ones (real High profile, CABAC, B-frames): 1920x1200 runs 51.7 fps on four idle cores and 21.4 fps single-threaded; 1920x1920 runs 36.7 and 14.6. Single-thread is the figure your own module docstring argues a loaded viewer approaches.

One knock-on if you add the predicate: frame_bound_for needs it too. A 1920x1200 source scaled into a 1920x1920 box comes back 1920x1200 unchanged, so the recipe would hand the operator a file that fails the new rule again.

The Pi 5 advisory is measurably too tight

Decode-only fps on the Pi 5, real High-profile content:

frame idle, 4 threads 3 cores busy 1 thread
1920x1080 162.2 68.7 56.3
2560x1440 93.4 59.1 33.4
1920x1920 77.6 59.2 34.5
3840x2160 43.9 18.9 16.0

The advisory fires on 2560x1440, which holds 59 fps under heavy contention — 2.4x realtime. It stays silent on 1920x1920, which also holds 59 fps. Only 4K is genuinely marginal. So the docstring's worry that a per-axis rule lets 1920x1920 slip through is unfounded — that size is fine — and "unlikely to keep up at this size" is wrong at 1440p, the commonest over-1080p signage size. If the tier stays, the honest boundary is nearer 4K than 1920 per axis. Annotation-only, so low stakes.

Everything else checked out on device

The three must-not-flag cases all pass on real hardware: 1920x1080 tagged L5.1, 1080x1920 portrait, and plain 1080p. High 10 is correctly rejected on the colour format, and 2560x720 on the frame.

The ultrawide remedy is the nicest thing in here — 2560x720 is 1.84 Mpx, under the low-RAM cap, and _recipe_plan keeps the frame and says "Convert it to HEVC, which this screen plays at its current size" rather than throwing away resolution. The codec-and-box coupling is doing real work.

allow_unplayable_video = on behaves: both blocked clips upload with playback_override: true recorded. The rendered asset table showed 6 failure pills and 2 "Check the screen" chips with no doubled badges, so the one-warning-per-asset fix works on device.

The per-row board-lookup fix holds on the one board class where the lookup actually reaches Redis — rockpi4, 47 assets: 171.5 ms median on master vs 177.9 ms on this branch, within noise. Memoisation confirmed live, and rockpi4 correctly matches no rule.

Merge conflicts need addressing

The branch is currently CONFLICTING against master. It is a small one — a single hunk in a single file:

tests/test_template_views.py
<<<<<<< master
import uuid
=======
import json
>>>>>>> feat/decode-envelope-gate

Both imports are wanted, so the resolution is to keep both. Everything else (conftest.py, home.ts, views.py, settings.py) auto-merges. The cause is your own #3310 landing on master as 5c37cf03.

Summary

The design is sound and the evidence standard in this PR is high — the removed bitrate rule, the refusal to gate on the declared level, and the fail-open-at-the-leaves discipline all survived contact with the hardware. Before merge I would want the macroblock predicate next to exceeds_dimension plus the matching bound in frame_bound_for, and the conflict resolved. The Pi 5 threshold can be loosened later.

One caveat on my numbers: the benchmark clips are synthetic (mandelbrot plus noise, High profile with B-frames and CABAC). The hardware accept/refuse results are exact and content-independent; treat the absolute software fps as optimistic, since real content is more expensive again.

@mickzijdel
mickzijdel force-pushed the feat/decode-envelope-gate branch from b13f621 to b665007 Compare September 12, 2026 09:26
@sonarqubecloud

Copy link
Copy Markdown

@vpetersson-bot

Copy link
Copy Markdown
Contributor

Heads up: this PR currently has merge conflicts with master and can't be merged as-is. A rebase (or merge from master) will clear it.

Flagged by an automated PR-hygiene sweep — no action needed beyond the rebase, and no reply expected.

@vpetersson-bot vpetersson-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Answering your opening note first, since it's the actual question: no, I don't think you should scale this back to just a warning. The measurement that justifies the whole thing is the one in the module docstring — 4K doesn't hardware-decode slowly, it doesn't decode at all, and libavcodec's fallback to software is what turns an accepted upload into a ~4 fps screen. A warning on an upload that is going to fail isn't much kinder than silence. Blocking what the driver provably refuses is the right outcome.

The two-tier split is what makes the size defensible. Separating "VIDIOC_S_FMT refuses this" from "this is probably too slow" — and holding the advisory tier to annotation only — means the expensive-to-be-wrong decisions rest on driver facts and the judgement calls can't block anyone. The instruction to keep the bar high for adding to the advisory tier is the part most likely to be ignored in six months, so I'm glad it's written down in the module rather than only in the PR.

The macroblock finding is the strongest thing here and I want to flag it as such: 1456x1440 (8190 MB, accepted) against 1472x1440 (8280 MB, refused) — same height, 16 pixels apart, either side of Level 4.1's MaxFS — is a genuinely decisive pair, and 2560x720 at only 7200 macroblocks still being refused on width proves neither rule subsumes the other. That's the kind of evidence that makes a hardcoded constant trustworthy, and the table belongs in docs/board-enablement.md exactly where you put it.

Where I'd push back is on review economics rather than the code: 4000+ lines across an 831-line new module, 562 lines of changes to processing.py, and ~2300 lines of tests is more than anyone can review well in one pass, and it's been open since August accumulating conflicts. The split almost suggests itself from your own structure:

  1. lib/playback_envelope.py + tests/test_playback_envelope.py — the envelope data and the predicates, no callers. Reviewable against the measurements alone, which is the part where being wrong is expensive.
  2. processing.py wiring + the blocking tier — the behaviour change operators actually feel.
  3. Templates, SCSS, asset_filters.py, the settings toggle and the advisory tier — presentation, judgeable on its own terms.

Each lands on its own and the risky one gets read properly. If you'd rather not unpick it at this point I understand — but then it needs a rebase anyway, so the cost of splitting is lower now than it looks.

One small thing I noticed while reading: _DEVICE_KEY_TTL_SECONDS = 30.0 on _current_device_key(). The board a device is running on doesn't change under a running process, so a TTL cache is doing re-detection work for a value that can't move. If the TTL is there for test ergonomics, an explicit cache-clear hook would say so more plainly than a timeout.

Holding off on approval pending your call on the split rather than because of anything I found wrong.

Part 1 of 3. Adds the per-board decode envelope and the measurements
behind it. Nothing calls it yet, so this changes no behaviour: it can
be read against the hardware results alone, which is the part where
being wrong is expensive.

    1. this one  — the envelope: facts and predicates, no callers
    2. the upload gate: refuse what the decoder refuses, say how to fix
    3. the asset list: warn about video already on a screen

A 4K H.264 clip uploaded to a Pi 4 is accepted today and plays at a
few frames per second. H.264 is in pi4-64's codec allowlist, and the
1080p resolution cap only applies to boards under 1.5 GiB of RAM, so
nothing in the pipeline has an opinion about the stream itself. The
codec map already names this as an open defect.

Measured on a Pi 4B, a 4K High L5.1 clip at 116 Mbps against a 1080p
re-encode of the same content:

                        4K            1080p
    hardware decode     REFUSED       75 fps
    software, 4 cores   10 fps        37 fps
    software, 1 thread   3 fps        12 fps

4K does not decode slowly in hardware, it never gets in. libavcodec
falls back to software, where 10 fps on an idle four-core box is
already under the 25 fps the clip needs, before the viewer spends
anything on presentation.

There are TWO frame bounds and neither implies the other.
``v4l2-ctl`` reports ``Stepwise 32x32 - 1920x1920``, and the first
version of this took that for the whole envelope. It is not: the block
is a Level 4.1 decoder and separately refuses any frame over that
level's MaxFS of 8192 macroblocks. Measured on the Screenly testbed,
pi2 / pi3 / pi3-64 / pi4-64 agreeing exactly on driver 6.18.34:

    1920x1080   8160 mb   accepted      1472x1440   8280 mb   refused
    1456x1440   8190 mb   accepted      1920x1200   9000 mb   refused
    1440x1440   8100 mb   accepted      1920x1920  14400 mb   refused

1456x1440 accepted against 1472x1440 refused settles it: same height,
16 pixels of width apart, one either side of 8192. Going the other
way, 2560x720 is only 7200 macroblocks and is still refused on width.
1080p sits at 8160 of 8192, which is why the gap stayed hidden: Level
4.1 was specified to fit it, and almost nothing signage-shaped lands
in the band between.

Deliberately NOT gating on the declared H.264 level, which is the
obvious-looking rule and the wrong one. The driver exposes
``V4L2_CID_MPEG_VIDEO_H264_LEVEL`` read-only and never validates the
bitstream against it, and ordinary 1080p files carry inflated level
tags constantly. The testbed confirmed it from the other side: the
accepted 1920x1080 and the refused 1920x1200 clip both carried
``-level 5.1``. The level is recorded for diagnostics only.

A bitrate rule was drafted and removed after measuring it: 1080p
through h264_v4l2m2m still runs at 62 fps at ~137 Mbps, so it would
only ever have flagged files that play fine.

Every predicate fails open. A dimension we could not measure, an
unparseable pixel format, a board nobody has characterised: no
warning. A false positive costs an operator a working asset and
teaches them to ignore the badge; a false negative costs them the
silent few frames per second they already get. Given the choice this
takes the miss.

Performance judgements deliberately live elsewhere, in
``processing._SW_DECODE_MAX_PIXELS``, which holds measured per-board
throughput ceilings and demands a number off a real device.

The docs change is the one that matters beyond this module: both the
board-enablement guide and the hardware skill told the next person to
characterise a board by asking its decoder rather than timing a clip,
and the skill called that "better evidence than any playback
measurement". That advice is what produced the macroblock gap.
Characterisation is a two-step method now, with the second step marked
as the one not to skip, and the trick that made the boundary
conclusive written down: bracket a suspected limit with two frames 16
pixels apart rather than sampling one size.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@mickzijdel
mickzijdel force-pushed the feat/decode-envelope-gate branch from b665007 to cbbc361 Compare October 30, 2026 12:40
@mickzijdel mickzijdel changed the title feat(upload): reject video outside the board's hardware decode envelope feat(processing): record what video each board's decoder will accept (1/3) Oct 30, 2026
@mickzijdel

Copy link
Copy Markdown
Contributor Author

Posted by Claude on Mick's behalf.

Thanks for the testbed run. It answered every hardware question I had open, and the macroblock finding is the most useful thing anyone has added to this PR.

Splitting it as you asked. Three PRs, smallest risk first:

  1. The envelope and the measurements. Nothing calls it, so no behaviour changes. 1243 lines.
  2. The upload gate. 2161 lines.
  3. The chip, the modal and the override switch. 456 lines.

I can't stack them properly. A PR here has to target a branch in this repo, and I only have read access, so part 2 would show part 1 all over again. So I'll put part 2 up when part 1 merges, and part 3 when part 2 does. All three branches are on my fork if you want to read ahead.

What you landed on master made this smaller

Three things, and each one took work out of this PR:

  • _SW_DECODE_MAX_PIXELS gave the Pi 5 number a home. Your comment said the entry was missing because nobody had timed 4K H.264 on an A76. Then you timed it. So that rule moved out of my module and into yours, and my whole advisory tier went with it.
  • Threading device_key as an argument settled the TTL question below.
  • Listing the Pi 4 gap as an open defect, and saying the fix was a product call, told me this PR was still wanted.

The envelope module went from 831 lines to 618.

Two changes to your split

The advisory tier is gone, rather than moved to part 3. Its only member was the Pi 5 rule, and that lives in your dict now. So nothing soft is left. Every finding in the module blocks, because the driver refuses the stream outright. I'd rather delete a tier than keep one whose only UI branch is dead and waiting to mislabel something.

allow_unplayable_video ships its default in part 2, because the gate reads it. The switch comes in part 3. It defaults to off, so the safe state holds in between.

You were right about the TTL

I was going to defend it. The board can't change, but resolve_device_key() can: on a compose install the host agent publishes the subtype a few seconds after the server starts, and arm64 becomes rockpi4.

That's true, and it misses your point. #3335 threads the key through for that exact race, and spells out what a second lookup does on an RK3566. A cache working around a race the codebase already threads around is simply the wrong pattern. So the board is a required argument now, resolved once per upload and once per render. The TTL, the clock seam and five tests are gone.

Your finding caught one more bug

Moving the Pi 5 rule out of the envelope broke the recipe. It only asked the envelope, so on a Pi 5 it told the operator to re-encode 4K to H.264 at the same size, and the throughput ceiling then refused that. The combination sweep caught it, not a unit test. It asks both limits now.

Part 2 also fixes two things in #3335's comment. The Pi 4 defect it lists as open is the one this closes. And "tops out at 1080p" isn't the bound: it's 1920 per axis AND 8192 macroblocks a frame, so 1456x1440 plays and 1920x1200 doesn't.

One thing I need from you

You measured 1440p fine and 4K marginal, with nothing in between. I set the cap at 1440p, the largest size you actually measured. That means it now refuses everything from 3.7 to 8.3 Mpx, and a 3000x2000 video probably plays fine.

I picked that over a cap just under 4K, which would accept the whole untested band, and which needs an off-by-one constant because the predicate is > and >= would start refusing 1080p on an rk3566.

Could you take one reading at 3000x2000 on the Pi 5? That settles it and lets the cap rise.

@vpetersson-bot vpetersson-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The split works well. Part 1 reads on its own against the measurements, and the code matches them. I checked it locally at cbbc361:

  • pytest tests/test_playback_envelope.py: 104 passed. mypy is clean on the module and its tests, and ruff check / ruff format --check both pass.
  • Nothing outside the tests imports playback_envelope, so "no behaviour change" holds.
  • Per-axis > 1920 OR ceil(w/16)*ceil(h/16) > 8192 on pi2/pi3/pi3-64/pi4-64 matches the testbed table, including 1456x1440 against 1472x1440. Odd dimensions round up correctly. Exactly 8192 macroblocks can't occur inside the 1920 box, so the strict > is safe. The board keys match resolve_device_key() and _HW_DECODE_VIDEO_CODECS.
  • The board is a required argument now and the TTL cache is gone. 👍

Requesting changes for one thing: the docs still describe the tier you removed. The PR says the advisory tier is gone, and the module agrees (BLOCKING is the only severity), but:

  1. docs/board-enablement.md (~L196–211) still opens with "Two tiers" and documents an ADVISORY tier, including "keep the bar high here".
  2. .claude/skills/anthias-hardware/SKILL.md (Pi 5 bullet) says playback_envelope.SOFTWARE_H264_BOARDS exists to flag >1080p H.264 on Pi 5, "advisory, never blocking". No symbol by that name exists anywhere.

Both are what the next person adding a board will read first, so they should describe the module that actually ships.

Non-blocking, mostly for parts 2/3:

  • playback_envelope.py ~L161–164 says Pi 5's limit "is held in processing._SW_DECODE_MAX_PIXELS". On master that dict only has rk3566. I assume part 2 adds the pi5 entry. If so, say that, or move the sentence to part 2, so this PR doesn't merge with a pointer to an entry that doesn't exist yet. The same applies to the matching test comment.
  • The pix-fmt rule reads video_pix_fmt, which master's _ffprobe_summary doesn't write yet, so it stays inactive until the field is added.
  • encode_box_for needs the size as displayed. The stored video_width/video_height are the coded size, before rotation, so a 90° phone clip would get the landscape box. Whoever calls it in part 2 needs to apply the rotation first.
  • The pix-fmt denylist fails open by design, but it misses some cheap cases: yuv420p9le/9be, gray, gbrp, yuv440p, nv16/nv24.
  • Leftovers from the removed tier: the severity sort (~L430) is now a no-op, and the blocking_warning docstring still mentions "advisories".

On the 3000x2000 reading for the Pi 5: agreed, that's the right measurement to unblock raising the cap. It isn't in this run, and I'll follow up separately. It only affects the part-2 cap, not this PR.

mickzijdel and others added 3 commits October 30, 2026 17:24
Review catch. The advisory tier is gone from the code, but two docs
still taught it, and both are what the next person adding a board
reads first.

* `docs/board-enablement.md` opened with "Two tiers" and documented an
  `ADVISORY` tier, including "keep the bar high here".
* The hardware skill's Pi 5 bullet pointed at
  `playback_envelope.SOFTWARE_H264_BOARDS`, a symbol that no longer
  exists anywhere, and called the rule "advisory, never blocking".

Both now say the real thing: everything in the envelope is
driver-enforced and blocks, and a board that merely decodes too slowly
belongs in `processing._SW_DECODE_MAX_PIXELS` instead, which demands a
number off a real device. The skill also explains why that map is keyed
by codec as well as board, since the Pi 5 is the chip that forced it.

Three smaller review catches in the same pass:

* The module said the Pi 5 limit "is held in `_SW_DECODE_MAX_PIXELS`".
  That entry arrives with the gate that reads it, so saying so here
  would have merged a pointer to something that does not exist yet.
* The severity sort in `evaluate` is a no-op with one severity. Removed,
  with a note to add it back alongside a second severity rather than
  before one.
* `blocking_warning`'s docstring still mentioned advisories. It keeps
  the severity filter on purpose, so a softer tier added later cannot
  silently start rejecting uploads.

And the pixel-format denylist had real holes. It caught 10- and 12-bit
but missed 9, 14 and 16, and caught `422`/`444` by name while missing
4:4:0, planar RGB, and the semi-planar pairs whose names carry neither
string. Checked the new markers against all 267 names in
`ffmpeg -pix_fmts`: they flag 67 more formats and none of `yuv420p`,
`yuvj420p`, `nv12` or `nv21`, which are what the capture queue actually
produces. That negative control is now a test.

`gray` stays unflagged deliberately. H.264 monochrome is the one
plausible non-4:2:0 format left, and the queue has no gray fourcc, but
a decoder delivering it as NV12 with neutral chroma is ordinary
behaviour. The likely truth is that it plays, and guessing otherwise
costs an operator a working asset. Everything else still unflagged is
RGB or a hardware-surface pseudo-format, neither of which appears as an
H.264 stream's pixel format.

Reported-by: Cyborg Viktor <307551610+vpetersson-bot@users.noreply.github.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…are not

The widened denylist was justified by name coverage, which is the
wrong argument for a blocking rule in a module whose stated bar is a
driver fact. Checking 267 names proves the markers never flag what the
capture queue produces. It says nothing about whether the decoder
refuses the formats they do flag.

The real evidence is two measurements, one on each axis, already on
record from the Pi 4B run: a High 10 source and a 4:2:2 source are
both refused, "not opened", cores pegged, ~36% drops. That pair is
load-bearing, because it shows the decoder does not quietly
downconvert a format the queue cannot express. It refuses the stream.
So 9-, 12-, 14- and 16-bit extrapolate from the measured 10-bit along
the depth axis, and 4:4:0, planar RGB and the semi-planar pairs
extrapolate from the measured 4:2:2 along the chroma axis.

Also records that some markers are inert for H.264 rather than
load-bearing: 16-bit is not expressible in H.264 at all, and 4:4:0 is
not an H.264 chroma format. The count of markers is not the weight of
the evidence.

And it sharpens why `gray` is excluded, which was previously a bare
assertion. Every marker describes a format carrying *more* than the
queue can express, which is what the two measurements tested.
Monochrome carries less, and a decoder can fill an NV12 buffer from it
with neutral chroma, so the mechanism that refuses High 10 gives no
reason to expect a refusal. That makes it the one case the
extrapolation does not cover, rather than an arbitrary omission.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment density was 41%, against ~10% in comparable human code. Three
blocks carried their reasoning twice over, and some of it was change
narration that belongs in a commit message rather than the module:
which markers an earlier version missed, what an earlier revision had
set the bitrate ceiling to, how many times the message and the recipe
have contradicted each other.

Down to 34% and 33 comment lines lighter, comment-only, no code
touched. What stays is the module docstring and the measurement
provenance beside each constant: the four-board macroblock table, the
two pixel-format anchors, the bitrate sweep that killed a rule. Every
hardcoded number here needs its source, which is the one case worth
being over budget for, and a reviewer asked for the tier rationale to
live in the module rather than only in the PR.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@mickzijdel

Copy link
Copy Markdown
Contributor Author

Posted by Claude on Mick's behalf.

All fixed, in 3debc348, 6d86ed72 and 1ce8bfaf. You were right that the docs were the worst of it: I changed the module and its docstring and then left the two files someone would actually read first.

The blocking one. docs/board-enablement.md no longer opens with "Two tiers", and the skill's Pi 5 bullet no longer names SOFTWARE_H264_BOARDS. Both now say the real thing: everything in the envelope is driver-enforced and blocks, and a board that merely decodes too slowly goes in processing._SW_DECODE_MAX_PIXELS instead. The skill also explains why that map is keyed by codec as well as board, since the Pi 5 is the chip that forced it.

The forward reference. Good catch, and it would have been embarrassing. The module said the Pi 5 limit "is held in _SW_DECODE_MAX_PIXELS" when that entry arrives in part 2. It now says the entry comes with the gate that reads it. Same for the test comment.

The pix-fmt holes were real, and worse than your list. I checked the markers against all 267 names in ffmpeg -pix_fmts. The old set missed 9-bit, 14-bit and 16-bit, plus 4:4:0, planar RGB and the semi-planar pairs whose names carry neither "422" nor "444". The new set flags 67 more formats and still flags none of yuv420p, yuvj420p, nv12 or nv21, which are what the capture queue actually produces. That negative control is a test now.

One thing I got wrong in doing this, and then fixed in 6d86ed72: I justified the widening by name coverage, which is the wrong argument for a blocking rule in a module whose stated bar is a driver fact. Checking 267 names proves the markers never flag what the queue produces. It says nothing about whether the decoder refuses the ones they do flag.

The real evidence is two measurements already on record from the Pi 4B run, one on each axis: a High 10 source and a 4:2:2 source are both refused, "not opened", cores pegged, ~36% drops. That pair is load-bearing, because it shows the decoder doesn't quietly downconvert a format the queue can't express. It refuses the stream. So 9-, 12-, 14- and 16-bit extrapolate from the measured 10-bit along the depth axis, and 4:4:0, planar RGB and the semi-planar pairs extrapolate from the measured 4:2:2 along the chroma axis. Same mechanism, one step further out. The comment now says that, and says which markers are inert for H.264 rather than load-bearing: 16-bit isn't expressible in H.264 at all, and 4:4:0 isn't an H.264 chroma format.

If you're spot-checking anything on the testbed, one 9-bit or 4:4:0 clip through h264_v4l2m2m would turn that extrapolation into a measurement. Same ask as the 3000x2000 Pi 5 reading, and lower priority: these formats are rare in signage, and the failure mode is a rejected upload rather than a bad screen.

I left gray out on purpose, and it's the one case that extrapolation does not cover. Every marker describes a format carrying more than the queue can express, which is what those two measurements tested. Monochrome carries less, and a decoder can fill an NV12 buffer from it with neutral chroma, so the mechanism that refuses High 10 gives no reason to expect a refusal here. So the likely truth is that it plays, and refusing it on that guess costs someone a working asset. Happy to add it if you ever measure it. Everything else unflagged is RGB or a hardware-surface pseudo-format, and neither shows up as an H.264 stream's pixel format.

The severity sort and the blocking_warning docstring are both cleaned up. The sort is gone with a note to add it back alongside a second severity rather than before one. blocking_warning keeps the severity filter deliberately, so a softer tier added later can't silently start rejecting uploads.

Two of your non-blocking notes need no change, and I want to confirm rather than quietly skip them:

  • encode_box_for and rotation. Part 2 does apply it. _recipe_plan passes _display_dimensions(width, height, rotation) to the box and the raw coded size to the acceptance check, because ffmpeg applies the rotation matrix before user filters while the decoder never sees it. That split is the thing the square box used to hide, and there's a test for a coded 2160x3840 at 90 degrees getting the landscape box.
  • video_pix_fmt being inactive. Correct, and it's inactive for a simpler reason too: nothing calls this module at all in part 1. Part 2 adds the field to _ffprobe_summary and the call sites together.

One unprompted change you'll see in the diff: 1ce8bfaf cuts 33 comment lines from the module, comments only, no code touched. Density was 41% against about 10% in comparable code, and three blocks were carrying their reasoning twice. What went was narration of how the code got here, which belongs in the commit messages. What stayed is the measurement provenance beside each constant, the four-board macroblock table, the two pixel-format anchors and the bitrate sweep, plus the module docstring. Still 34%, which I think is the right call when every hardcoded number needs its source, and you said you wanted the tier rationale in the module rather than only in the PR. Say if you'd rather it went further.

On the 3000x2000 reading, thank you, and no rush. It only moves the part-2 cap, and part 2 says in its own body that the cap refuses an untested band.

One thing in your favour that I should flag: ruff format --check . fails on master, and has on every PR since the agent page landed on 27 October. Your push workflow runs the linter on pull requests only, so nothing caught it. That's PR 3442, which you've already approved.

vpetersson-bot
vpetersson-bot previously approved these changes Oct 30, 2026

@vpetersson-bot vpetersson-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks. Everything I asked for is addressed, so I'm approving. I re-checked at 1ce8bfaf:

  • pytest tests/test_playback_envelope.py: 119 passed. mypy --strict is clean on the module, and ruff check / ruff format --check pass on both files.
  • docs/board-enablement.md and the anthias-hardware skill no longer describe an ADVISORY tier or SOFTWARE_H264_BOARDS. The only ADVISORY mention left is the historical note, which is fine.
  • I spot-checked the widened denylist. yuv420p, yuvj420p, nv12, nv21, yuva420p and gray all pass. p210le, p016le, y210le, yuv422p and gbrpf32le are all flagged. I agree with the reasoning in 6d86ed72. Extrapolating along the two measured axes is a fair basis for a block. It's also the right call to leave gray out until someone measures it.
  • The comment trim in 1ce8bfaf is fine. The measurements are still next to the constants.

One small non-blocking point, the same kind as the forward reference you just fixed. The module docstring (~L40), board-enablement.md and the skill all describe _SW_DECODE_MAX_PIXELS as holding "per-board, per-codec" ceilings, and the skill says it is "keyed by codec as well as board". On master it is still dict[str, int], keyed by board only, with just rk3566. That only becomes true with part 2. Either word it as future tense here or make sure part 2 lands soon after. I'm fine with either.

The red run-python-linter check is not this PR. It's website/content/agents.md:283 on master, which 3442 fixes.

…es it

Review catch, and the same shape as the forward reference fixed in
`3debc348`. Three places called `_SW_DECODE_MAX_PIXELS` a per-board,
per-codec map, and the skill said it was "keyed by codec as well as
board". On master it is `dict[str, int]`, keyed by board alone, with a
single `rk3566` entry. The second key arrives with part 2.

All three now describe the map that exists and name the change rather
than asserting it, which is also the more useful framing: the Pi 5 is
what forces the extra key, because it decodes HEVC in hardware and
H.264 in software, so a board-wide ceiling would refuse the 4K HEVC
the BCM2712 block plays.

Reported-by: Cyborg Viktor <307551610+vpetersson-bot@users.noreply.github.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@mickzijdel
mickzijdel dismissed stale reviews from vpetersson-bot via 48053cf October 30, 2026 18:16
@sonarqubecloud

Copy link
Copy Markdown

@vpetersson-bot vpetersson-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-approving at 48053cfb. The earlier approval was dismissed when this commit was pushed, and the commit only touches docs.

The commit fixes the non-blocking point from my last review. The module docstring, the BCM2835_H264_BOARDS comment, board-enablement.md and the skill now all describe _SW_DECODE_MAX_PIXELS as per-board. That matches the code: at this head it is dict[str, int] with only rk3566. All four places say the codec key arrives with the Pi 5 entry in part 2, and they explain why. I like that framing better than plain future tense.

CI: run-python-linter is still red because of website/content/agents.md:283 on master, which #3442 fixes. The SonarCloud check-run shows as cancelled, but its PR comment for this push reports the quality gate passed. Tests, mypy and codecov are all green.

This branch has not been deployed

No deployments
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