H264InputMixin and H264DecoderModule - #3308
Open
leshy wants to merge 13 commits into
Open
Conversation
…d video
Ports declared in a mixin are collected like any other (annotations merge
across the MRO), and a port is fed through its transport — so a mixin can
add an H.264 video input and inject decoded frames into the host's own
image port, indistinguishable from wire traffic. The image port ducks
both ways: an In (retrofit an existing consumer) or an Out (a standalone
decoder). Both users are one declaration each:
class VideoMarkerDetectionModule(H264InputMixin, MarkerDetectionStreamModule):
config: VideoMarkerDetectionModuleConfig
class H264DecoderModule(H264InputMixin, Module):
config: H264InputConfig
color_image: Out[Image]
Decoding never skips — H.264 reference frames don't survive that — but
only the newest picture reaches the consumer, at decode_hz. Take the
mixin for a single consumer; keep the standalone decoder when several
share one decode, since the mixin's decoder is per-module.
MarkerDetectionStreamModule's port-count guard loosens to what the code
actually requires (one Out; inputs are wired by name), so auxiliary mixin
inputs are legal.
leshy
requested review from
Dreamsorcerer,
mustafab0,
paul-nechifor and
spomichter
as code owners
July 31, 2026 11:00
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #3308 +/- ##
==========================================
+ Coverage 75.34% 75.36% +0.02%
==========================================
Files 1149 1151 +2
Lines 110476 110613 +137
Branches 10007 10041 +34
==========================================
+ Hits 83234 83363 +129
- Misses 24378 24384 +6
- Partials 2864 2866 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
leshy
changed the title
leshy
marked this pull request as draft
July 31, 2026 11:09
…rker The mixin carried four `type: ignore`s because it declared no relationship to the module it lives in. Type it as a Module under TYPE_CHECKING and the host's `start`, `register_disposable` and `config` all resolve; at runtime it stays a plain object, or it would be collected as a module in its own right. The decoder is narrowed at its one use site, so the decode result is checked as list[VideoFrame] rather than Any. `_decoder` keeps a runtime-safe annotation deliberately: class annotations are evaluated to collect ports, so naming an `av` type there would turn an optional dependency into a hard import. dedicated_worker moves onto the mixin. It was set on H264DecoderModule, so VideoMarkerDetectionModule — the retrofit case the mixin exists for — was silently sharing a worker while doing H.264 decode. The mixin leads the MRO, so a host that wants otherwise still wins by setting it on itself.
ModuleBase already declares the type; every other module that opts in writes the bare assignment.
Measured, not assumed: H.264 decode is ~0.34 ms/frame at 720p and ~2.5 ms at 1080p, and the bgr24 conversion only runs at decode_hz. A 30 fps stream therefore costs 1-8% of one core. PyAV drops the GIL for both the decode and the reformat, so neither half of dedicated_worker's rationale — CPU and GIL contention — applies. This also restores H264DecoderModule to the shared pool; it claimed a process on the same bad assumption.
h264_decode() is now the whole implementation — an operator from a
CompressedVideo observable to an Image one, with the decoder living per
subscription. Attaching it is all the module layer does, and both ways to
attach share it: the mixin subscribes into the host's own color_image In,
H264DecoderModule publishes on an Out.
Three things fall out:
- The mixin declares color_image itself. Annotations merge across the
MRO, so it is the consumer's own port, not a name looked up through
getattr. The publish/transport.publish ducking goes with it.
- throttle_first replaces the hand-rolled _last_fed bookkeeping.
- Decode is testable without a Module, so the tests now run a real
encoded stream through the operator instead of a stubbed decoder.
Fixes a latent corruption: the pipeline subscribed through observable(),
which is latest-wins backpressured and drops intermediate messages. A
dropped packet costs every frame until the next keyframe. It reads from
pure_observable() now and throttles after decode, never before.
The decoder was guessing at a rate on the consumer's behalf. Emit every decoded frame instead and let consumers thin the stream: observable() is latest-wins backpressured, and memory2 transforms cover the rest. Cheap enough to be uncontroversial: an Image over a decoded frame is 0.6 us and shares the buffer, so frames a consumer ignores cost only the bgr24 conversion the decode needed anyway (~1% of a core at 720p, ~3.4% at 1080p for a 30 fps stream). Takes the config with it. H264InputConfig existed only to carry the knob, and VideoMarkerDetectionModuleConfig only to compose it with the host's — so the mixin now needs no config at all, and its user is one line.
color_image was hardcoded once the mixin started declaring the port itself — the earlier image_port string was configurable, this was not. An overridable image_in property restores that without the string: a host whose image In is named otherwise overrides it and gets a typed port back, not a getattr. The declaration moves under TYPE_CHECKING so the mixin expects the host's port rather than contributing one. Class annotations inside that guard never reach __annotations__, so a renamed host no longer inherits a stray color_image it would have to leave unwired.
H264InputMixin typed itself as a Module under TYPE_CHECKING and stayed a plain object at runtime, purely so the blueprint scanner wouldn't collect it. That is a workaround in the wrong place: the mixin genuinely is a Module, it just has no graph of its own to run in. Subclass Module for real and give the scanner the rule instead — a *Mixin is not deployable, same spirit as the leading-underscore skip already there. Nothing else in the registry ends in Mixin, so the rule is inert today and does the right thing for the next one. Checked before trusting it: with the mixin registered, all 195 blueprint and blueprint-kwargs checks still passed, so the entry was cosmetic rather than breaking — the cost was a module in the registry that would fail on deploy, having no image port of its own.
Not part of this branch — the blueprint scanner walks the filesystem, so regenerating in a checkout with untracked modules picks them up. It points at dimos/navigation/path_heading/module.py, which nothing on this branch ships, so the entry would fail to import for anyone else.
leshy
marked this pull request as ready for review
July 31, 2026 13:01
leshy
changed the title
leshy
changed the title
Decoding a codec produces the same pixels the encoder had — no meaning is extracted, so perception was the wrong shelf. dimos/stream is the reactive media package: video_provider already exposes video sources as observables and audio/ is a reactive node graph, so a video/ sibling is where an rx decode operator belongs. It also gives the three copies of this decode somewhere neutral to converge. The memory2 transform in go2/dds/video.py and the eventual transport codec under protocol/pubsub can both import from stream/video; neither could reasonably have imported from perception.
github-actions
Bot
added
the
ready-to-merge
github-actions
Bot
added
ready-to-merge
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a mixin that gives any module with an
In[Image]an H.264videoinput, decoding internally and feeding the frames into that image port's own transport, downstream code cannot tell them from wire traffic.Ports declared in a mixin are collected like any other, so adapting an existing consumer is one declaration:
The image port ducks both ways - an
In(retrofit an existing consumer) or anOut(a standalone decoder), which is allH264DecoderModuleis.Which one to use
The mixin is one module fewer in the graph and transport, but each video-capable module owns its own decoder. Decoding is extremely cheap though. Before running dedicated decoder module, ensure that transport costs due to higher traffic don't overwhelm per-module decoding costs 0.3-1ms 2-4% of one core