feat(planecontrol,videodecoder)!: the capture plane owns capture end to end (#753) - #761
feat(planecontrol,videodecoder)!: the capture plane owns capture end to end (#753)#761Ulrond wants to merge 13 commits into
Conversation
Extends planecontrol with a graphics capture surface so an application can
import decoded video frames as GPU textures while the decoder runs
continuously, replacing the per-SoC vendor-private enable-texture paths.
Capture is a routing destination for a decoder's output, which is what this
module already owns, so ICapture is reached through IPlaneControl.getCapture()
on the video plane the decoder would otherwise have been mapped to - the same
idiom as getGraphicsFbProvider(). One published service for the module.
Surface:
ICapture per-plane resource: capabilities, state,
property reads, open()/close()
ICaptureController per-session: start/stop, acquireLatestFrame,
releaseFrame, property writes
ICaptureControllerListener onRingReady, onFrameAvailable
ICaptureEventListener onSystemError, onDecoderDetached, onStateChanged
CaptureCapabilities ring limits, formats, modifiers, ring model
CaptureProperty ring shape property keys
CapturePropertyKVPair capture property key/value pair
VideoFrameView per-frame Dma-Buf addressing
CaptureErrorCode capture error codes
State planecontrol resource lifecycle states
IVideoDecoder is unchanged - the decoder does not know where its output goes.
Capture requires OperationalMode.GRAPHICS_TEXTURE, which videodecoder already
advertises through IVideoDecoderManager.getSupportedOperationalModes().
Frames are NV12 linear with truthful per-plane offsets, importable through
EGL_EXT_image_dma_buf_import without translation. Decode proceeds at full rate
regardless of the consumer's acquire cadence; the all-slots-locked policy is
HFP-declared. An unsupported configuration fails start() with a CaptureErrorCode
rather than silently falling back to plane output.
Names are capture-prefixed where the module already defines the unprefixed
name (Property, PropertyKVPair, Capabilities).
hfp-planecontrol.yaml gains a per-plane captureCapabilities declaration.
Reviewer sign-off is set to recheck for the 14+5 cycle on the new interface.
Ulrond
added
component:planecontrol
There was a problem hiding this comment.
Pull request overview
This PR extends the com.rdk.hal.planecontrol HAL with a new decoded-frame capture surface (ICapture) to support decode-to-texture workflows (DMA-BUF ring suitable for EGL DMA-BUF import) without changing IVideoDecoder.
Changes:
- Adds a new per-plane capture API surface (
ICapture+ controller + listeners) and supporting types (capabilities, properties, error codes, frame view, lifecycle state). - Extends
IPlaneControlwithgetCapture(planeResourceIndex, captureEventListener)to access capture as a PlaneControl sub-resource (Option B). - Updates the PlaneControl HFP, module metadata, and documentation to describe capture destinations and lifecycle.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| planecontrol/metadata.yaml | Updates component scope/notes and resets reviewer sign-off statuses for the new capture review cycle. |
| planecontrol/current/hfp-planecontrol.yaml | Declares per-plane captureCapabilities in the HFP (slot limits, formats/modifiers, ring model, full-ring policy). |
| planecontrol/current/docs/plane_control.md | Documents the new capture requirements, interfaces, and lifecycle/contract. |
| planecontrol/current/com/rdk/hal/planecontrol/IPlaneControl.aidl | Adds getCapture(...) to expose capture per video plane resource. |
| planecontrol/current/com/rdk/hal/planecontrol/ICapture.aidl | Defines the capture resource API (capabilities/state, open/close, property reads). |
| planecontrol/current/com/rdk/hal/planecontrol/ICaptureController.aidl | Defines per-session control (start/stop/acquire/release, property writes). |
| planecontrol/current/com/rdk/hal/planecontrol/ICaptureControllerListener.aidl | Adds callbacks for ring readiness and frame availability. |
| planecontrol/current/com/rdk/hal/planecontrol/ICaptureEventListener.aidl | Adds resource-level callbacks for system errors, decoder detach, and state changes. |
| planecontrol/current/com/rdk/hal/planecontrol/CaptureCapabilities.aidl | Adds the capability parcelable for ring and format/modifier support. |
| planecontrol/current/com/rdk/hal/planecontrol/CaptureProperty.aidl | Adds the capture property key enum (slot/ring/format and dimensions). |
| planecontrol/current/com/rdk/hal/planecontrol/CapturePropertyKVPair.aidl | Adds key/value pair parcelable for atomic multi-property writes. |
| planecontrol/current/com/rdk/hal/planecontrol/CaptureErrorCode.aidl | Adds capture-specific service error codes for start/open/system-error reporting. |
| planecontrol/current/com/rdk/hal/planecontrol/VideoFrameView.aidl | Adds per-frame DMA-BUF addressing parcelable for EGL import. |
| planecontrol/current/com/rdk/hal/planecontrol/State.aidl | Adds capture resource lifecycle state enum (module-scoped). |
| * The bound decoder must have `OperationalMode.GRAPHICS_TEXTURE` selected in its | ||
| * `Property.OPERATIONAL_MODE` before `ICaptureController.start()` is called. Whether | ||
| * that mode is available is advertised by | ||
| * `IVideoDecoderManager.getSupportedOperationalModes()`. |
There was a problem hiding this comment.
Fixed in ac45760 — qualified as videodecoder.OperationalMode.GRAPHICS_TEXTURE, videodecoder.Property.OPERATIONAL_MODE and videodecoder.IVideoDecoderManager.getSupportedOperationalModes() across all the capture AIDL and the module docs, not just this line.
|
|
||
| The `IVideoDecoder` contract is unchanged - the decoder does not know where its output goes. | ||
|
|
||
| Capture requires the decoder to be operating in `OperationalMode.GRAPHICS_TEXTURE`, which is advertised by `IVideoDecoderManager.getSupportedOperationalModes()` and selected through the decoder's `Property.OPERATIONAL_MODE`. A video decoder can be bound to at most one capture session at a time. |
There was a problem hiding this comment.
Fixed in ac45760 — qualified as videodecoder.OperationalMode.GRAPHICS_TEXTURE, videodecoder.Property.OPERATIONAL_MODE and videodecoder.IVideoDecoderManager.getSupportedOperationalModes() across all the capture AIDL and the module docs, not just this line.
planecontrol defines its own Property enum, so an unqualified Property.OPERATIONAL_MODE reads as planecontrol.Property. Qualify the videodecoder-owned OperationalMode, Property and IVideoDecoderManager references throughout the capture documentation.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
planecontrol/current/com/rdk/hal/planecontrol/ICapture.aidl:164
- The crash-cleanup description refers to
ICaptureController.close(), butICaptureControllerhas noclose()method (closing is done viaICapture.close(controller)). This makes the lifecycle contract ambiguous.
* If the client that opened the `ICaptureController` crashes, then the
* `ICaptureController` has `stop()` and `close()` implicitly called to perform clean up.
planecontrol/current/com/rdk/hal/planecontrol/ICaptureEventListener.aidl:41
- Hyphenate “vendor-specific” in the parameter description for consistency and grammar.
* @param[in] errorCode A CaptureErrorCode enum value.
* @param[in] vendorErrorCode A vendor specific error code.
*/
planecontrol/current/com/rdk/hal/planecontrol/State.aidl:22
- The
@briefsays this enum is for a “planecontrol resource instance”, but the following lines clarify it applies specifically to capture resources. Tighten the brief to avoid readers assuming plane resources have a lifecycle state.
* @brief Lifecycle state of a planecontrol resource instance.
…r owns its output format
PlaneType.CAPTURE
-----------------
Capture is its own plane type rather than a capability bolted onto a video
plane. A capture plane is a plane whose destination is the client's texture
instead of the display, so it is discovered and addressed exactly as a display
plane is. It is never composited, so alpha, z-order and display latency do not
apply to it.
The decoder owns format and size
--------------------------------
Format was declared twice - once on the plane, once by the decoder - and
CaptureErrorCode.FORMAT_MISMATCH existed only because the two could disagree.
Format and size are properties of the decoder's output, so they move to
videodecoder and the error class disappears with them.
videodecoder gains CaptureConfig{drmFourcc, drmModifier, width, height}
IVideoDecoderController.setCaptureConfig()
Capabilities.supportedCaptureFourCCs / Modifiers
planecontrol loses CaptureCapabilities.supportedFourCCs / supportedModifiers
CaptureProperty.DRM_FOURCC / DRM_MODIFIER / WIDTH / HEIGHT
CaptureErrorCode.FORMAT_MISMATCH (ordinal 3 left as a gap)
A capture plane consumes what the decoder produces; it does not negotiate a
second format.
Routing is the mode
-------------------
Property.OPERATIONAL_MODE, the OperationalMode enum and
IVideoDecoderManager.getSupportedOperationalModes() are removed. Where a
decoder's frames go follows from how it is wired: mapped to a plane, returned
over onFrameOutput(), or routed to capture by setCaptureConfig(). That call is
the whole of capture-mode selection - a decoder with a configuration applied
emits for capture, one without does not, and it clears on close(). Whether a
decoder supports capture at all is Capabilities.supportedCaptureFourCCs being
non-empty.
Pool, not ring
--------------
The client-visible semantics were never a ring: acquireLatestFrame() takes the
newest Ready buffer and lets older ones go, so nothing is consumed in order.
Renamed throughout to match videodecoder's existing OUTPUT_FRAME_POOL_SIZE:
maxSlotCount/maxSlotSizeBytes -> maxBufferCount
SLOT_COUNT/SLOT_SIZE_BYTES -> BUFFER_COUNT
VideoFrameView.slot -> bufferIndex
stallsWhenRingFull -> stallsWhenPoolExhausted
onRingReady() -> onPoolReady()
sharedRingBuffer and onPoolReady's pool file descriptor are gone. Every frame
carries the descriptors and offsets that address it, so one Dma-Buf carved into
offset-addressed buffers and one Dma-Buf per buffer are served by identical
client code. CaptureCapabilities is now maxBufferCount and
stallsWhenPoolExhausted - the two things a client cannot observe for itself.
Documentation
-------------
video_decoder.md replaces "Operational Modes" with "Output Routing" and gains
"Decode to Texture" covering pixel format versus memory layout, the
vendor-namespaced modifier encoding, and why the choice trades bandwidth
against portability - a GPU that reads the vendor's compressed layout can halve
capture-path bandwidth, while anything touching pixels needs LINEAR, which is
why NV12 + LINEAR is required of every decoder that supports capture.
Reviewer sign-off on both components is set to recheck.
Ulrond
changed the title
Ulrond
added
component:videodecoder
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
planecontrol/current/com/rdk/hal/planecontrol/ICaptureController.aidl:62
- The
start()documentation referencesCaptureProperty.BUFFER_SIZE_BYTES, butCapturePropertycurrently only definesBUFFER_COUNT. This makes the API contract internally inconsistent.
* Reserves a pool of `CaptureProperty.BUFFER_COUNT` buffers of
* `CaptureProperty.BUFFER_SIZE_BYTES` each from the platform's video memory region,
* and wires the bound video decoder's capture output into the pool.
planecontrol/current/com/rdk/hal/planecontrol/IPlaneControl.aidl:259
- This method returns a capture interface for a
PlaneType.CAPTUREplane, but the brief says "for a video plane resource". That wording is inaccurate and may confuse consumers into thinking capture is a subtype of VIDEO planes rather than its own plane type.
* Gets a Capture interface for a video plane resource.
videodecoder/current/com/rdk/hal/videodecoder/IVideoDecoderController.aidl:353
- The EX_ILLEGAL_ARGUMENT documentation refers to
drmFourcc,drmModifier,widthandheightas if they were direct parameters, but this method takes a singlecaptureConfigobject. This is misleading for API consumers and makes the contract harder to interpret.
* @exception binder::Status::Exception::EX_ILLEGAL_ARGUMENT if `drmFourcc` is not in
* `Capabilities.supportedCaptureFourCCs`, `drmModifier` is not in
* `Capabilities.supportedCaptureModifiers`, or `width`/`height` exceed the
* `CodecCapabilities` of the codec this decoder was opened for.
| @@ -50,24 +49,6 @@ interface IVideoDecoderManager | |||
| */ | |||
| IVideoDecoder.Id[] getVideoDecoderIds(); | |||
There was a problem hiding this comment.
Same answer as on Property.OPERATIONAL_MODE — deliberate, marked !, and the capability moves to the capture plane, which owns capture end to end after #753. A client on the old surface pins the frozen videodecoder/0.1.0.0 or 0.2.0.0 snapshots, which are untouched.
| @@ -66,26 +66,6 @@ enum Property { | |||
| */ | |||
| OUTPUT_FRAME_POOL_SIZE = 2, | |||
There was a problem hiding this comment.
Deliberate, and marked ! on both the commit and the PR title. Two things make it a versioned removal rather than silent breakage:
- The ordinal is retired, not reused.
Propertynow runs… 2, 4, 5 …, so no existing member's value shifts and no client misreads a value it still knows. currentis the development surface.videodecoderis pre-baseline at0.2.0.1, and a client needing the old surface pins the frozenvideodecoder/0.1.0.0or0.2.0.0snapshots, which this PR does not touch.
OPERATIONAL_MODE and getSupportedOperationalModes() move to the capture plane, which now owns capture end to end — that is the substance of #753 rather than a side effect of it. Capture is selected by mapping a source to a plane of type CAPTURE, so there is no longer a mode to set on the decoder.
…full A requirements review against the graphics-player specification found one contradiction and four unstated obligations. THE DECODER MUST NOT TRANSFORM THE FRAME ---------------------------------------- CaptureConfig.width and .height read as "the width of the captured frames", which invites a vendor to scale output to them. The specification requires the opposite: frames arrive at the resolution the stream decodes to, in the source colorimetry, with no scaling, rotation, crop, colour conversion, tone-mapping or gamma adjustment. Shape and colour belong to the consumer, which applies them per frame as it textures the frame onto its scene and may change them on any frame. A transform applied in the decoder would have to be undone, and one the consumer cannot undo makes the frame unusable. So the fields are restated as what they are - the maximum dimensions the buffers must accommodate - and VideoFrameView.width/.height now say they report what each frame actually is, which a smaller stream makes differ. HAL.PLANECONTROL.14, HAL.VIDEODECODER.16 FRAME-DROP BUDGET ----------------- No more than one dropped frame per 15 seconds of capture, 144p through 2160p, while the client acquires and releases at the presentation cadence. The capture path is not permitted to lose frames a display plane would have shown. A client that stops releasing is explicitly not covered - that case is CaptureCapabilities.stallsWhenPoolExhausted. HAL.PLANECONTROL.15 PRESENTATION TIME ----------------- Carried unaltered, and stated as the frame's only timing reference. A captured frame goes to the client's scene rather than to a display plane, so the client presents it against the clock its audio path already runs on. HAL.PLANECONTROL.16 CONCURRENCY AND ALPHA --------------------- Capture planes and video planes are independent resources, so a product declaring both runs a capture session alongside a playback session routed to a display plane, up to the decoder count its video decoder profile declares. NV12 and DRM_FORMAT_MOD_LINEAR are the required baseline, not the limit - supportedCaptureFourCCs is an open list, so a product able to emit a format carrying alpha declares it and a client selects it, with no interface change.
Finding — capture does not relay colour metadata it already hasRaising this separately because it is a pre-merge gap with no detectable failure mode, and it is the one thing on this PR I would not merge without a decision on. What is missing. A capture plane specifies the memory layout of the frames it delivers and nothing about how to interpret their pixels. Not in Why it matters. A consumer importing NV12 through Omit them and the driver falls back to its own default, typically BT.601 narrow. HD and UHD content is BT.709 or BT.2020, so the result is shifted colour and crushed or stretched levels. Wrong colour renders a plausible picture, not an error — no return code, no log line, and no application-side test can catch it, because the consumer has no reference to compare against. It surfaces as a subjective PQ complaint late in certification and is expensive to trace back to an unset EGL attribute.
The type already exists one module across.
What is genuinely new: chroma siting. It appears nowhere in To decide
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.
Suppressed comments (5)
videodecoder/current/com/rdk/hal/videodecoder/Property.aidl:69
Property.OPERATIONAL_MODEwas removed from thecurrentvideodecoder property enum but still exists in the releasedvideodecoder/0.2.0.1API. Removing an enum constant/property is a breaking change for any client that reads/writes it; consider deprecating it (documenting it as ignored) or cutting a new versioned release/interface for the updated routing model.
OUTPUT_FRAME_POOL_SIZE = 2,
/**
videodecoder/current/com/rdk/hal/videodecoder/IVideoDecoderManager.aidl:53
getSupportedOperationalModes()was removed from thecurrentIVideoDecoderManager API, but it still exists in the releasedvideodecoder/0.2.0.1interface. This is a breaking interface change for clients built against that release; consider keeping the method (even if deprecated) or introducing a new versioned interface/release rather than removing the contract outright.
IVideoDecoder.Id[] getVideoDecoderIds();
/**
* Gets a Video Decoder interface.
planecontrol/current/com/rdk/hal/planecontrol/ICapture.aidl:198
- The comment says the
ICaptureControllerhasclose()called implicitly on client crash, butclose()is a method onICapture, not onICaptureController. This is confusing for API consumers and should describe the session being implicitly stopped and closed.
* If the client that opened the `ICaptureController` crashes, then the
* `ICaptureController` has `stop()` and `close()` implicitly called to perform clean up.
planecontrol/current/com/rdk/hal/planecontrol/IPlaneControl.aidl:265
- This method returns an
ICapturefor aPlaneType.CAPTUREresource, but the first line of the docstring says “video plane resource”, which is misleading (a capture plane is not a video/display plane).
* Gets a Capture interface for a video plane resource.
videodecoder/current/docs/video_decoder.md:242
- This row implies
IPlaneControl.getCapture()alone opens a capture session “against this decoder”, but capture is actually routed by mapping the decoder’s source to a CAPTURE plane and then openingICaptureon that plane. Clarifying this avoids readers assuming a direct decoder→capture binding API exists in videodecoder.
| **A capture plane** | The client opens a capture session against this decoder through `IPlaneControl.getCapture()`. | No. Frames are consumed through the capture plane. |
|
Resolved in A capture plane was declared twice.
Neither declaration said what the pixels mean.
Chroma siting had no representation anywhere.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
planecontrol/current/com/rdk/hal/planecontrol/IPlaneControl.aidl:265
- The docstring says this returns a Capture interface for a "video plane resource", but this API is specifically for plane resources of type
PlaneType.CAPTURE. Calling it a video plane is misleading for API consumers.
* Gets a Capture interface for a video plane resource.
planecontrol/current/com/rdk/hal/planecontrol/PlaneCapabilities.aidl:37
- This comment states that on
PlaneType.CAPTUREplanes, all non-routing fields (includingmaxWidth/maxHeight,frameWidth/frameHeight, etc.) "shall" be empty/zero/false. However, the product HFP added in this PR populates those fields for the capture plane, which would contradict this API contract. Consider narrowing this requirement to only the truly display/compositing-related fields (alpha/z-order/vsync latency, etc.), or otherwise clarify how these fields are expected to be used for capture planes.
* Every other field of this parcelable shall be EMPTY, ZERO OR FALSE on a capture
* plane - `pixelFormats` and `supportedDynamicRanges` empty, `colorDepth`,
* `maxWidth`, `maxHeight`, `frameWidth`, `frameHeight`, `maxFrameRate` and
* `vsyncDisplayLatency` zero, `supportsAlpha` and `supportsZOrder` false.
| * by field onto GstVideoColorimetry, where siting is a separate concern and | ||
| * GStreamer keeps GstVideoChromaSite as its own type. | ||
| */ | ||
| ChromaSite chromaSite; |
|
Withdrawing my previous comment. It changed The gap raised in #761 (comment) is still open, and the questions in it stand:
Nothing further will be implemented against this until those are agreed. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.
Suppressed comments (4)
planecontrol/current/docs/plane_control.md:68
- This entry says capture capabilities are "for a video plane", but
CaptureCapabilitiesdescribes thePlaneType.CAPTUREcontract. Using "capture plane" here avoids confusion withPlaneType.VIDEO.
| `CaptureCapabilities.aidl` | Parcelable describing capture capabilities for a video plane.|
planecontrol/current/com/rdk/hal/planecontrol/IPlaneControl.aidl:266
- The docstring says "video plane resource", but this method only applies to plane resources of type
PlaneType.CAPTURE(capture planes). This wording is misleading for API consumers.
/**
* Gets a Capture interface for a video plane resource.
*
planecontrol/current/docs/plane_control.md:60
- This table entry describes
ICaptureas for a "video plane"; capture is explicitly for planes of typePlaneType.CAPTURE(capture planes), not video planes.
This issue also appears on line 68 of the same file.
| `ICapture.aidl` | Decoded frame capture interface for a video plane used as a capture destination.|
planecontrol/current/docs/plane_control.md:256
- The section text still says there are "2 types of planes (video and graphics)", but this PR adds
PlaneType.CAPTUREand the table now includes Capture. Update the sentence above this table so the documentation stays consistent.
|-----------|--------------------|
| **Video** |If there is no video to display on a visible plane, then it shall render transparent black. <br>The z-order is dynamic only for video planes.<br> Primary video plane shall always be listed at resource index 0.|
| **Graphics** |When the plane type is GRAPHICS, `getGraphicsFbProvider()` provides graphics frame creation, commit, and destroy operations.|
| **Capture** |The destination is the client's texture rather than the display, so the plane is never composited: alpha, z-order and display latency do not apply.<br>The source is mapped with `setVideoSourceDestinationPlaneMapping()` exactly as it is for a video plane, and that mapping is what routes the source to capture.<br>When the plane type is CAPTURE, `getCapture()` provides decoded frame capture to a Dma-Buf buffer pool.<br>It runs opposite to a graphics plane: a graphics plane carries frames from the client to the display, a capture plane carries decoded frames from the pipeline to the client.<br>Capture planes are listed after graphics planes.|
e5fc087 to
8b72315
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated no new comments.
Suppressed comments (13)
planecontrol/current/hfp-planecontrol.yaml:124
- The capture HFP declares
supportedFourCCsandsupportedModifiersas independent lists, but the capture AIDL surface declares pairedsupportedFormats(FormatLayout[]). This mismatch makes it ambiguous how a product profile maps onto the runtimeCaptureCapabilitiescontract.
# FOURCC codes and modifiers are the kernel's, from include/uapi/drm/drm_fourcc.h.
supportedFourCCs: # DRM_FORMAT_NV12 required on every capture plane
- 0x3231564E # DRM_FORMAT_NV12
supportedModifiers: # DRM_FORMAT_MOD_LINEAR required on every capture plane
- 0x0000000000000000 # DRM_FORMAT_MOD_LINEAR
planecontrol/current/hfp-planecontrol.yaml:131
supportedCodecsusesH264, but thecom.rdk.hal.videodecoder.Codecenum usesH264_AVC. Using a value that doesn't exist in the codec enum will break schema-to-AIDL alignment and makes the profile ambiguous.
- H264
planecontrol/current/com/rdk/hal/planecontrol/capture/CaptureCapabilities.aidl:59
CaptureCapabilitiesrefers toProperty.DRM_FOURCC/Property.DRM_MODIFIER, but this interface exposes format selection viaICaptureController.setFormat(FormatLayout)and there is no suchPropertyhere. This reads like leftover text from an earlier design and can mislead API consumers.
* A client selects one entry and sets `Property.DRM_FOURCC` and
* `Property.DRM_MODIFIER` from it.
planecontrol/current/com/rdk/hal/planecontrol/capture/CaptureCapabilities.aidl:91
- The comment states
Codec.H264_AVCandCodec.AV1are both required, but the product HFP in this PR declares only H.264 for the capture plane and the PR description also calls out H.264 as the required baseline. Please align the contract text so requirements and profiles don't contradict each other.
* `Codec.H264_AVC` and `Codec.AV1` are both required to be present on every
* capture plane. Certification asks for H.264 in one cycle and AV1 in the next,
* and a product serves both, so a client that can negotiate either always has a
* working path.
planecontrol/current/com/rdk/hal/planecontrol/capture/ICapture.aidl:136
ICapture.open()documentation says the client selects the session format viaICaptureController.setProperty(), butICaptureControlleronly exposessetFormat(). This is inconsistent with the actual API surface.
* 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.
planecontrol/current/docs/capture/capture_interface.md:234
- This paragraph refers to
ICaptureController.setProperty(), but the capture controller API usessetFormat()and plane size is set viaIPlaneControl.setProperty(). The current wording doesn't match the implemented AIDL surface.
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 mapped source decoding a codec outside `supportedCodecs` fails there with `CaptureErrorCode.CODEC_NOT_CAPTURABLE`. None of them falls back to plane output.
planecontrol/current/com/rdk/hal/planecontrol/capture/CaptureErrorCode.aidl:70
CaptureErrorCode.FORMAT_UNSUPPORTEDreferencesCaptureCapabilities.supportedFourCCs/supportedModifiers, butCaptureCapabilitiesdefinessupportedFormats(pairedFormatLayout[]). The@seeis currently pointing at non-existent fields.
* @see CaptureCapabilities.supportedFourCCs, CaptureCapabilities.supportedModifiers
*/
planecontrol/current/com/rdk/hal/planecontrol/capture/ICaptureControllerListener.aidl:44
- The
onPoolReady()comment has a broken sentence ("which is what as many buffers...") that obscures the intent. Tightening this wording will make the pool-depth contract clearer.
* 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.
planecontrol/current/hfp-planecontrol.yaml:117
maxBufferCountrefers toCaptureProperty.BUFFER_COUNT, but this PR's capture API doesn't defineCapturePropertyand pool depth is described elsewhere as vendor-calibrated (reported viaonPoolReady()). Keeping this key/comment is likely to confuse product profiles.
This issue also appears on line 120 of the same file.
maxBufferCount: 8 # Maximum pool buffers (CaptureProperty.BUFFER_COUNT)
planecontrol/current/hfp-planecontrol.yaml:126
- These comments reference
CaptureProperty.WIDTH/CaptureProperty.HEIGHT, but capture frame size is controlled via the plane'sProperty.WIDTH/Property.HEIGHT(and there is noCapturePropertytype in this PR).
This issue also appears on line 131 of the same file.
maxFrameWidth: 1920 # Maximum captured frame width (CaptureProperty.WIDTH)
maxFrameHeight: 1080 # Maximum captured frame height (CaptureProperty.HEIGHT)
planecontrol/current/docs/capture/capture_interface.md:79
- This requirement states both
Codec.H264_AVCandCodec.AV1are mandatory for every capture plane, but the capture plane HFP in this PR only declares H.264. Please align the written requirements with the declared product profile (or vice versa) so implementers and validators have one consistent baseline.
| **HAL.PLANECONTROL.CAPTURE.2** | Shall declare in `CaptureCapabilities.supportedCodecs` the codecs whose decoded frames a capture plane can deliver, and shall include `Codec.H264_AVC` and `Codec.AV1`.| Certification asks for H.264 in one cycle and AV1 in the next; a product serves both. A decoder opened for any other codec still decodes and displays normally. |
planecontrol/current/docs/capture/capture_interface.md:302
- This example comment says both H264_AVC and AV1 are required, but the product profile in this PR only declares H264_AVC for capture. The example should match the stated baseline to avoid confusion.
// captureCapabilities.supportedCodecs - H264_AVC and AV1 both required
planecontrol/current/docs/capture/capture_interface.md:39
- This states both
Codec.H264_AVCandCodec.AV1are required on every capture plane, but the capture plane HFP in this PR only declares H.264. Align the narrative description with the baseline requirement so the documentation and profiles don't contradict each other.
`Codec.H264_AVC` and `Codec.AV1` are both required on every capture plane; certification asks for H.264 in one cycle and AV1 in the next, and a product serves both. A decoder opened for a codec outside `supportedCodecs` decodes and displays normally — it just cannot feed a capture plane, and `start()` fails with `CaptureErrorCode.CODEC_NOT_CAPTURABLE` if one is mapped to it.
… mandates nothing (#753) supportedCodecs and supportedFormats state what a product can deliver. Neither carries a required value: a platform that cannot capture a codec or a format does not list it, and a mandate it cannot meet would not change that. setFormat() is therefore required before start(), which fails with INVALID_CONFIGURATION when no pair was selected - there is no default pair left to assume. The HFP is brought to the same shape: supportedFormats replaces the two independent lists, maxBufferCount goes with the field it named, and the capture plane declares routing rather than display geometry.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
planecontrol/current/com/rdk/hal/planecontrol/IPlaneControl.aidl:265
- The docstring says this is for a "video plane resource", but the API is specifically for planes of type
PlaneType.CAPTURE. This wording is misleading for API consumers.
* Gets a Capture interface for a video plane resource.
planecontrol/current/com/rdk/hal/planecontrol/capture/ICaptureControllerListener.aidl:44
- The
onPoolReady()documentation has a malformed sentence about pool depth ("which is what as many buffers...") that is hard to understand and appears to have been accidentally garbled. Clarifying this text will make the contract easier to follow.
* 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.
planecontrol/current/com/rdk/hal/planecontrol/capture/ICapture.aidl:136
ICapture.open()documentation referencesICaptureController.setProperty()and configuring pool depth, but the controller interface exposessetFormat()(and pool depth is not configured via the API). This is misleading and contradicts the actual AIDL surface.
* 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.
planecontrol/current/com/rdk/hal/planecontrol/capture/VideoBufferView.aidl:60
VideoBufferView.bufferIndexdocs say that an index that names no buffer is ignored, butICaptureController.releaseFrame()/acquireLatestFrame()explicitly treat out-of-range indices asEX_ILLEGAL_ARGUMENT. The documentation should match the API contract to avoid clients depending on undefined behaviour.
* An index that names no buffer in the current pool is ignored, which is what
* makes a release arriving after a stop safe.
| import com.rdk.hal.planecontrol.graphics.IGraphicsFbProvider; | ||
| import com.rdk.hal.planecontrol.graphics.IGraphicsFbProviderListener; | ||
| import com.rdk.hal.planecontrol.capture.ICapture; | ||
| import com.rdk.hal.planecontrol.capture.ICaptureEventListener; |
Update — the interface has moved a long way since the last review@ramkumarpattabiraman this is worth a fresh read rather than a diff; the PR description is rewritten to match. Capture and graphics are now subpackages. Capture is reduced to a control surface. boolean setFormat(in FormatLayout format); // one row of supportedFormats
Nothing is mandated any more. Pool depth is the platform's.
Two things the interface never said, both of which a client would have hit:
Both docs now carry a worked example, and the examples are compiled. A scratch harness extracts the fenced C++ from the docs and builds it against the generated headers. It has caught three real defects so far: Requirements are renumbered per package — |
Two vendor questions this interface depends onNeither blocks review of the shape, but both decide whether it can be implemented as written. 1. Broadcom can do
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated no new comments.
Suppressed comments (7)
planecontrol/current/com/rdk/hal/planecontrol/capture/ICapture.aidl:136
- The
open()documentation referencesICaptureController.setProperty(), but the controller interface exposessetFormat()(and frame size is configured viaIPlaneControlproperties). This is misleading for API consumers and should be updated to match the actual contract.
* 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.
planecontrol/current/docs/capture/capture_interface.md:289
- This paragraph refers to
ICaptureController.setProperty(), but the capture controller API usessetFormat(). Frame size is set viaIPlaneControl.setProperty()onProperty.WIDTH/HEIGHT. Update the text to reflect the actual API surface.
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 mapped source decoding a codec outside `supportedCodecs` fails there with `CaptureErrorCode.CODEC_NOT_CAPTURABLE`. None of them falls back to plane output.
planecontrol/current/com/rdk/hal/planecontrol/capture/ICaptureControllerListener.aidl:44
- The
onPoolReady()comment has a broken sentence ("which is what as many buffers as...") that makes the pool-depth contract hard to understand. Please rewrite for clarity.
* 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.
videodecoder/current/docs/video_decoder.md:242
- In the output-routing table, the capture-plane row implies capture is selected only by
getCapture(), but capture also depends on mapping the decoder’s video sink to aCAPTUREplane viaIPlaneControl.setVideoSourceDestinationPlaneMapping(). Clarifying this avoids suggestinggetCapture()alone changes decoder routing.
| **A capture plane** | The client opens a capture session against this decoder through `IPlaneControl.getCapture()`. | No. Frames are consumed through the capture plane. |
planecontrol/current/docs/capture/capture_interface.md:134
- There is a stray comma/period in the requirement text (", .") which reads as a typo.
| **HAL.PLANECONTROL.CAPTURE.2** | Shall declare in `CaptureCapabilities.supportedCodecs` the codecs whose decoded frames a capture plane can deliver, .| The list is what the plane can capture, not what a product must offer. A decoder opened for a codec outside it decodes and displays normally; it just cannot feed this plane. A decoder opened for any other codec still decodes and displays normally. |
planecontrol/current/docs/capture/capture_interface.md:225
- The docs say
releaseFrame()“tolerates unknown indices” and that out-of-pool indices are ignored, but the AIDL contract forICaptureController.releaseFrame()says out-of-pool indices raiseEX_ILLEGAL_ARGUMENT. Please align the documentation with the interface contract (either update docs or update the AIDL contract).
Call `ICaptureController.releaseFrame(bufferIndex)` when the client stops drawing while still holding a buffer. A client drawing continuously has already released through the previous step. The call is idempotent and tolerates unknown indices.
Release is keyed by index because the index is the buffer's identity. An index that names no buffer in the current pool is ignored, which is what makes a release arriving after a stop safe.
planecontrol/current/com/rdk/hal/planecontrol/capture/VideoBufferView.aidl:65
- This comment says out-of-pool buffer indices are ignored to make post-stop releases safe, but
ICaptureController.releaseFrame()is documented as raisingEX_ILLEGAL_ARGUMENTfor indices outside the pool. Please align this description with the interface contract.
* An index that names no buffer in the current pool is ignored, which is what
* makes a release arriving after a stop safe.
|
An alternative to this PR is open as #782, against #781: the same capture interface as a module in its own right, The AIDL types are the same in both — #782 carries them over verbatim with only the package rewritten. What differs is discovery and binding: #782 adds This PR also changes |
Closes #753.
What this adds
A decoded-frame capture surface on
com.rdk.hal.planecontrol, so an application can import decoded video frames as GPU textures (EGL DMA-BUF import) while the decoder runs continuously. Each port does this today through a different vendor-private path, so middleware cannot state a portable contract.Capture is declared, configured and bound entirely on the capture plane.
IVideoDecodercarries nothing about it.Capture is a plane type
PlaneType.CAPTUREsits besideVIDEOandGRAPHICS. The type says where a plane's pixels come from and where they go:VIDEOIPlaneControlGRAPHICSIGraphicsFbProviderCAPTUREICaptureA capture plane runs opposite to a graphics plane: a graphics plane carries frames from the client to the display, a capture plane carries decoded frames from the pipeline to the client. It is never composited, so alpha, z-order, position and display latency have no meaning on it.
This mirrors
getGraphicsFbProvider()exactly — one published service for the module, the capture resource enumerated by the existing plane enumeration. There is noICaptureManagerand noICapture.Id.The mapping is the binding
setVideoSourceDestinationPlaneMapping()maps a source to aCAPTUREplane exactly as it maps one to a display plane, and that mapping is the whole of the binding:The source captured is whatever is mapped to the plane. A source is mapped to one plane at a time, which is what limits a decoder to a single capture session. Because the decoder is named in exactly one place, there is no second place for the two to disagree.
The vendor layer configures whatever the mapped source's decoder requires, over whatever internal path the platform provides.
Interface surface
Everything below is in
com.rdk.hal.planecontrol.capture. Graphics moved tocom.rdk.hal.planecontrol.graphicsin the same pass, leavingcom.rdk.hal.planecontrolfor planes themselves.ICapture.aidlgetCapabilities(),getState(),open()/close()ICaptureController.aidlsetFormat(),start(),stop(),acquireLatestFrame(),releaseFrame()ICaptureControllerListener.aidlonPoolReady(),onFrameAvailable(),onCaptureError()ICaptureEventListener.aidlonSystemError(),onSourceUnmapped(),onStateChanged()CaptureCapabilities.aidlFormatLayout.aidlVideoBufferView.aidlVideoFrameView.aidlState.aidlCLOSED,READY,STARTING,STARTED,STOPPINGCaptureErrorCode.aidlEX_SERVICE_SPECIFICCaptureErrorCode.aidlState.aidlLifecycle:
setVideoSourceDestinationPlaneMapping(source → capture plane)→getCapture(planeIndex, eventListener)→open(controllerListener)→setProperty(WIDTH/HEIGHT/BUFFER_COUNT)→start()→acquireLatestFrame(releaseIndex)→stop()→close(controller).Frame delivery
Addressing is delivered once.
onPoolReady(VideoBufferView[])carries every pool buffer with the file descriptors, offsets, strides, lengths, size, format and modifier that address it. None of that changes during a session, so the client imports each buffer into an EGLImage on receipt.A frame is an index and a time.
VideoFrameViewisbufferIndex+presentationTimeNs, so a frame costs an int and a long on the wire rather than aParcelFileDescriptorarray per frame at 60 Hz.Release and acquire are one call.
acquireLatestFrame(releaseBufferIndex)frees the previous buffer and takes the next in one round trip.VideoFrameView.NO_BUFFERon the first call.releaseFrame()remains for the last frame of a session.The frame returned is the one due for presentation. Audio latency and AV-sync correction are applied by the vendor layer, so a client that draws on receipt is in sync without computing anything. Frames whose presentation time has passed are dropped; frames whose time has not come stay queued.
planeFds[N]/planeOffsets[N]/planeStrides[N]feedEGL_DMA_BUF_PLANE<N>_FD_EXT/_OFFSET_EXT/_PITCH_EXTdirectly — no translation. A client caching EGLImages must key onbufferIndex, never on the file descriptor alone: under a shared-Dma-Buf pool every buffer carries the same descriptor and an fd-keyed cache silently freezes the picture.What a plane declares, what a session configures
CaptureCapabilities—supportedFormats(format and layout as pairs, since a modifier is valid with particular formats),supportedCodecs,maxFrameWidth/maxFrameHeight,resize,stallsWhenPoolExhausted. Nothing here is mandated. A platform that cannot capture a codec or a format does not list it, and a requirement it cannot meet would not change that.The client makes one decision. It picks a row of
supportedFormatsand passes it toICaptureController.setFormat(). That is required beforestart(), which fails withINVALID_CONFIGURATIONotherwise — there is no default pair to assume once nothing is mandated.Frame size is the plane's own
Property.WIDTH/HEIGHT, set throughIPlaneControlthe way any plane's size is. Pool depth is not configured at all: the platform calibrates it from the throughput it can sustain, and the client learns it by counting whatonPoolReady()delivers.resizefalse meansWIDTH/HEIGHTmust equal what the mapped source decodes to, andstart()fails withRESOLUTION_MISMATCHotherwise. Nothing is scaled, rotated, cropped, colour-converted or tone-mapped on this path — shape and colour belong to the consumer and may change on any frame.Errors and lifetime
onCaptureError()reports failures not tied to a single acquire.CaptureErrorCodecoversOUT_OF_MEMORY,SOURCE_NOT_MAPPED,CODEC_NOT_CAPTURABLE,HARDWARE_FAULT,RESOLUTION_MISMATCH,COLOR_CONVERSION_UNSUPPORTED,FORMAT_UNSUPPORTEDandINVALID_CONFIGURATION. Nothing falls back to plane output.plane_control.mdspecifies startup in either order (frames produced beforestart()are discarded, and starting capture may cost a decode interruption while the vendor reconfigures), shutdown in either order, and that imported EGLImages do not survivestop().videodecoder
Its net change from
developis the removal ofOperationalMode,IVideoDecoderManager.getSupportedOperationalModes()andProperty.OPERATIONAL_MODE. Where a decoder's frames go follows from how it is wired — mapped to a display plane, returned overonFrameOutput(), or mapped to a capture plane — so there is no mode to select.video_decoder.mdcarries the routing table.Product profile
hfp-planecontrol.yamldeclares the DPI9 capture plane: FHD, NV12 linear, H264,resize: false, one plane. The interface stays general and the product declaration narrows it, so a later platform declares more without an interface change.Points for reviewers
onStateChangedsits onICaptureEventListener, not the controller listener —videodecoderputs state changes on the event listener and frame callbacks on the controller listener.CaptureErrorCodeis numbered compactly (1–8) rather than preserving holes. The enum is new here and has never shipped.resizeis a capability, not a property. A plane either scales or it does not; that is a hardware fact, not a per-session choice.captureCapabilitiesis declared in the HFP, reachable throughICapture.getCapabilities()—graphicsFbCapabilitiesworks the same way.Governance
New interface on a GREEN component, so reviewer sign-off in
planecontrol/metadata.yamlandvideodecoder/metadata.yamlis set torecheckfor the 14+5 cycle.