feat(capture)!: capture as a module in its own right, bound to a pipeline source (#781) - #782
Open
Ulrond wants to merge 6 commits into
Open
feat(capture)!: capture as a module in its own right, bound to a pipeline source (#781)#782Ulrond wants to merge 6 commits into
Ulrond wants to merge 6 commits into
Conversation
Capture was nested inside planecontrol on the reasoning that it is a routing destination for a decoder's output, and so belongs beside the routing it is a variant of. That holds only while the destination is a plane. A capture bound to a point with no plane - av_sync, or a source whose frames are not going to a display at all - has no plane to be the destination of, and the model has to invent a blank one to describe it. So capture moves out. It is an output in its own right, bound to a source, rather than a destination within the plane model. A source may have a plane, a capture, both or neither, and none of those is a special case. The interface is carried over unchanged. The ten AIDL files are verbatim from the planecontrol-nested proposal with only the package rewritten to com.rdk.hal.capture, because what changes is where capture lives and what it binds to, not what it does once bound: the ring, the format negotiation and the acquire/release contract are unaffected. The split is clean at the type level, which is the evidence it was separable: the module's only external imports are com.rdk.hal.PropertyValue and com.rdk.hal.videodecoder.Codec. Nothing in capture referred to planecontrol. The per-product declaration moves with it, from captureCapabilities inside hfp-planecontrol.yaml to hfp-capture.yaml, and gains the source list - the points in the pipeline a product can bind a capture to. Requirements are renumbered HAL.CAPTURE.1-11 from HAL.PLANECONTROL.CAPTURE.*. The prose still describes the plane model and is not yet rewritten; the module is structurally correct and the document is not.
…781) Moving capture out of planecontrol took its entry point with it. Discovery, addressing, the source binding and the frame size all came from IPlaneControl, and a module that no longer sits under planecontrol has none of them. This is the upgrade the move exists for. ICaptureManager is the entry point, following the manager pattern every other module uses: a serviceName of CaptureManager, getCaptureIds() and getCapture(). A capture is addressed by its own ICapture.Id rather than by another module's plane index. CaptureSource names the stages a session can bind to - VIDEO_DECODER, VIDEO_SINK, AV_CLOCK - and ICapture.open() takes one. The binding is the session: it names the stage frames are taken from, for the lifetime of that session, and replaces setVideoSourceDestinationPlaneMapping() as the way a capture learns what it is capturing. This is deliberately not common/AVSource: that enum is the input feed, which continues to decide what flows through a stage exactly as before. The two are different axes and a capture only chooses the first. Binding takes a view rather than diverting frames, so anything already consuming the stage is unaffected and a capture can attach to a pipeline already running. CaptureCapabilities gains supportedSources and maxCapturesPerSource to declare what a resource offers and how far a stage fans out; a bind past that limit is refused with SOURCE_UNAVAILABLE, renamed from SOURCE_NOT_MAPPED, which described a mapping that no longer exists. A capture carries its own frame size, because an output in its own right cannot borrow one from a plane it does not have. Property with WIDTH and HEIGHT, and setProperty()/getProperty() on the controller, replace the plane properties - the controller already imported PropertyValue without ever using it. The AIDL no longer refers to planes anywhere. The prose document still does and is rewritten next.
The document still argued the case the move rejected - that capture is a plane whose destination is the client's texture, discovered with IPlaneControl.getCapabilities() and bound with setVideoSourceDestinationPlaneMapping(). None of that is the interface any more. It now describes a capture found through ICaptureManager, addressed by its own ICapture.Id, and bound to a CaptureSource at open(). The binding is the session: it names the stage frames are taken from and lasts until close(). What flows through that stage is still decided by the input feed, which is a different axis a capture neither selects nor changes. Binding takes a view rather than diverting, so anything already consuming the stage is unaffected and a capture can attach to a pipeline already running - with maxCapturesPerSource declaring how far a stage fans out. The plane/image-plane disambiguation section loses half its subject. A capture is no longer a hardware surface, so only the image-plane sense remains: the colour components of one frame, which is where planeFds[] and planeOffsets[] get their name from the EGL import API. The requirements, the resource-management flow, the sequence diagram and the end-to-end walkthrough are rewritten against the real calls. Frame size is set with ICaptureController.setProperty() because a capture carries its own, and onSourceUnmapped becomes onSourceLost - there is no mapping left to undo. Outbound links follow the peer-module convention rather than filesystem paths, and the planecontrol and graphics pages are no longer related reading.
… session (#781) getProperty sat on the controller, which made reading a property require the session. Only the client that opened the session holds a controller, so a property nothing mutates was readable by exactly one caller. It moves to ICapture, where the rest of the observable surface already is - getCapabilities and getState are both readable by any holder of the resource, and a property is the same kind of thing. This follows the pattern the other modules use: IVideoDecoder, IVideoSink and IAudioDecoder all expose getProperty on the resource. setProperty stays on the controller. Mutating the frame size belongs to the client that owns the session, and only while it is READY. Two things the move through the document left behind are fixed with it. Steps 3 and 4 of the resource-management list were the same call - binding and opening became one operation when the binding became the session - so they are merged and the list renumbered. And the System Context flowchart still styled a node called PC, which stopped existing when IPlaneControl became ICaptureManager.
The earlier passes matched whole sentences and missed the places where the same claims were split across lines or phrased differently: capabilities and error codes still described what "this plane" can deliver and what "the mapped source" is decoding, and a diagram still labelled a node with the old model. They now say what they mean under the current interface - a capture resource, and the source a session is bound to. The placeholder API Documentation row goes with them; it promised Doxygen that does not exist.
Frames do not flow through the AV clock. avclock is a timing service - its surface is ClockTime, ClockMode and the playback rate, and it describes itself as audio/video clock synchronization and timing control. Its only mentions of frames are the PTS its linked sink reports, which is timing observed from a frame path running elsewhere. There is nothing at the clock to capture. A capture that wants presentation timing binds to VIDEO_SINK, because the sink is where the clock's synchronisation has already been applied and where frames actually are. That is now said on the value itself, so the next reader does not reach for a clock again. Every remaining value is a stage frames flow through, which is the rule the enum has to hold to.
Ulrond
added
enhancement
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new RDK HAL AIDL module com.rdk.hal.capture that defines a standalone decoded-frame capture API bound to a selectable pipeline stage (e.g. VIDEO_DECODER, VIDEO_SINK), rather than modelling capture as a planecontrol sub-feature.
Changes:
- Adds the new
capture/module structure (AIDL interfaces/types, HFP, module metadata, and module docs). - Defines the capture service entry point (
ICaptureManager) plus session/resource contracts (ICapture,ICaptureController, listeners, capabilities, formats, buffer/frame views). - Documents the end-to-end capture workflow and configuration model in
capture_interface.md.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| capture/metadata.yaml | Adds component metadata for the new capture module. |
| capture/current/CMakeLists.txt | Adds standalone build rules for the capture module’s generated C++ bindings. |
| capture/current/mkdocs.yml | Adds MkDocs configuration for capture module documentation. |
| capture/current/interface.yaml | Declares the AIDL interface build inputs/imports for the capture module. |
| capture/current/hfp-capture.yaml | Adds HAL Feature Profile schema/example for per-product capture capabilities. |
| capture/current/docs/capture_interface.md | Adds the capture module specification and usage walkthrough. |
| capture/current/com/rdk/hal/capture/ICaptureManager.aidl | Defines the module entry point and serviceName. |
| capture/current/com/rdk/hal/capture/ICapture.aidl | Defines capture resource lifecycle and session opening/closing. |
| capture/current/com/rdk/hal/capture/ICaptureController.aidl | Defines per-session configuration and frame acquire/release contract. |
| capture/current/com/rdk/hal/capture/ICaptureEventListener.aidl | Defines capture resource event callbacks (state, source loss, system errors). |
| capture/current/com/rdk/hal/capture/ICaptureControllerListener.aidl | Defines session callbacks for pool delivery and frame availability/errors. |
| capture/current/com/rdk/hal/capture/CaptureCapabilities.aidl | Defines the capability surface (formats, codecs, sources, fan-out, sizing, pool behaviour). |
| capture/current/com/rdk/hal/capture/CaptureErrorCode.aidl | Defines service-specific error codes used by the capture module. |
| capture/current/com/rdk/hal/capture/CaptureSource.aidl | Defines the pipeline stage binding enum. |
| capture/current/com/rdk/hal/capture/Property.aidl | Defines configurable session properties (WIDTH/HEIGHT). |
| capture/current/com/rdk/hal/capture/State.aidl | Defines capture resource/session lifecycle states. |
| capture/current/com/rdk/hal/capture/FormatLayout.aidl | Defines the (fourcc, modifier) format/layout pairing. |
| capture/current/com/rdk/hal/capture/VideoBufferView.aidl | Defines per-pool-buffer DMA-BUF addressing and metadata delivered once per session. |
| capture/current/com/rdk/hal/capture/VideoFrameView.aidl | Defines per-frame identity/timestamp returned from acquireLatestFrame(). |
Suppressed comments (3)
capture/current/com/rdk/hal/capture/ICapture.aidl:209
ICapture.getProperty()is documented as returning null for unknown properties, but the method return type is not marked@nullable. In this repo,getProperty()methods that may return null are consistently annotated@nullable(e.g.videosink/current/com/rdk/hal/videosink/IVideoSink.aidl:81).
* @returns PropertyValue or null if the property key is unknown.
*
* @exception binder::Status::Exception::EX_NONE for success.
* @exception binder::Status::Exception::EX_ILLEGAL_ARGUMENT for invalid property value.
*
* @see ICaptureController.setProperty(), Property
*/
PropertyValue getProperty(in Property property);
capture/current/docs/capture_interface.md:304
- This sequence diagram uses an undeclared participant
PCand calls planecontrol APIs (setVideoSourceDestinationPlaneMapping,setProperty) that do not exist in the standalone capture module. It also showsopen(controllerListener)without the requiredCaptureSourceparameter, so the diagram is both invalid Mermaid and misleading.
Client->>Manager: getCapture(captureId, eventListener)
PC-->>Client: ICapture
Client->>Capture: getCapabilities()
Capture-->>Client: CaptureCapabilities
Client->>PC: setVideoSourceDestinationPlaneMapping(VIDEO_SINK, sinkIndex, capturePlane)
PC-->>Client: true
capture/current/docs/capture_interface.md:286
- This sentence says that a format/modifier outside
CaptureCapabilitiesfails atICaptureController.setProperty(), but format/modifier selection is done viasetFormat()(whilesetProperty()is for WIDTH/HEIGHT). As written, it gives callers the wrong place to expect validation failures.
A format, modifier or frame size outside `CaptureCapabilities` fails at `ICaptureController.setProperty()`, while it is still a configuration error rather than a stream of wrong pixels. A pool the platform's video memory region cannot satisfy fails at `ICaptureController.start()` with `CaptureErrorCode.OUT_OF_MEMORY`, rather than being silently trimmed, and a bound source decoding a codec outside `supportedCodecs` fails there with `CaptureErrorCode.CODEC_NOT_CAPTURABLE`. None of them falls back to display output.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+160
to
+163
| * The client selects the session's format through | ||
| * `ICaptureController.setProperty()` in the `READY` state, before calling | ||
| * `ICaptureController.start()` - the frame format and size it wants, and the depth | ||
| * of the pool that holds them. |
Comment on lines
+24
to
+26
| * Applies to the capture resources reached through `ICaptureManager.getCapture()`. | ||
| * Plane resources themselves are stateless and are not described by this enum. | ||
| * |
Comment on lines
+218
to
+220
| * A format shall be selected before `start()`. There is no default: what a plane | ||
| * can deliver is whatever it declares, so there is no pair the interface could | ||
| * assume on the client's behalf. |
Comment on lines
+42
to
+44
| * The array length is the number of buffers the vendor reserved, which is what | ||
| * as many buffers as the platform calibrated for the throughput it can sustain. The length of this array IS the pool depth - it is not declared anywhere else, because there is nothing for a client to decide before it and nothing to check it against. Where the | ||
| * session left it unset. |
Comment on lines
+66
to
+70
| MW -->|binder| Capture | ||
| Capture --> Plane | ||
| Decoder --> Plane | ||
| Plane --> Pool | ||
| Pool -. imported as GPU textures .-> App |
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
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.
Closes #781.
What this is
Frame capture as a HAL module in its own right,
com.rdk.hal.capture, bound to a stage of the pipeline rather than to a display plane.This is the alternative to #761, which places the same interface inside
planecontrol. Both implement decode-to-texture; they differ in where capture lives and what it binds to.Why capture is not a plane destination
A capture bound to a stage whose frames are not going to a display has no plane to be the destination of, so the plane model has to invent a blank one to describe it. Making capture an output in its own right removes the special case: a source may have a display path, a capture, both or neither.
The split is clean at the type level, which is the evidence it was separable — the module's only external imports are
com.rdk.hal.PropertyValueandcom.rdk.hal.videodecoder.Codec. Nothing in capture referred toplanecontrol.The interface carried over unchanged
The ten AIDL types are verbatim from the
planecontrol-nested proposal with the package rewritten: the ring, the format negotiation and the acquire/release contract are untouched. What changes is where capture lives and what it binds to, not what it does once bound.What the move required
Discovery, addressing, the source binding and the frame size all came from
IPlaneControl, so a module outsideplanecontrolhas none of them:ICaptureManager—serviceName,getCaptureIds(),getSupportedSources(),getCapture()IPlaneControl.getCapture()ICapture.IdCaptureSourcetaken byopen()setVideoSourceDestinationPlaneMapping()Property+setProperty()/getProperty()supportedSources,maxCapturesPerSourcePlaneType.CAPTUREdiscoverySOURCE_UNAVAILABLE,onSourceLost()SOURCE_NOT_MAPPED,onSourceUnmapped()Contract
The binding is the session.
open()names the stage frames are taken from, for that session's lifetime.The input feed decides content. Where a capture attaches and what is playing are different axes — which is why
CaptureSourceis notcommon/AVSource. TheIVideoDecodercontract is unchanged.Binding takes a view, it does not divert, so anything already consuming the stage is unaffected and a capture can attach to a pipeline already running.
maxCapturesPerSourcedeclares the fan-out.Reads from the resource, writes through the session.
getProperty()is onICapturewheregetCapabilities()andgetState()already are, matchingIVideoDecoder,IVideoSinkandIAudioDecoder;setProperty()stays on the controller, which only the opening client holds.CaptureSourcevaluesVIDEO_DECODERandVIDEO_SINK. Every value is a stage frames flow through — the AV clock is not one.avclockis a timing service whose surface isClockTime,ClockModeand playback rate; its only mentions of frames are PTS reported by the linked sink. A capture wanting presentation timing binds toVIDEO_SINK, where the clock's synchronisation has already been applied.Scope
planecontrolandvideodecoderare byte-identical todevelop. The branch addscapture/and touches nothing else, soPlaneTypehas noCAPTUREvalue andhfp-planecontrol.yamlhas nocaptureCapabilities.The document is rewritten against the interface that exists: requirements renumbered
HAL.CAPTURE.1–11, both diagrams and the end-to-end walkthrough against the real calls, and the plane/image-plane disambiguation reduced to the image-plane sense that still applies.