Gstreamer video recorder: add python bindings#1518
Open
asierfernandez wants to merge 36 commits intonvidia-holoscan:mainfrom
Open
Gstreamer video recorder: add python bindings#1518asierfernandez wants to merge 36 commits intonvidia-holoscan:mainfrom
asierfernandez wants to merge 36 commits intonvidia-holoscan:mainfrom
Conversation
Contributor
Greptile SummaryThis PR adds Python bindings for Confidence Score: 5/5Safe to merge; all previously flagged P0/P1 issues are resolved and only minor P2 style findings remain. All substantive prior review concerns (pydoc wiring, storage default mismatch, ArgumentTypeError propagation, CuPy import messaging) are addressed. Remaining findings are a typo in the README and a missing changelog entry — neither blocks correctness or functionality. applications/gstreamer/gst_video_recorder/python/metadata.json (version/changelog mismatch), applications/gstreamer/gst_video_recorder/README.md (double-space typo) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph Pattern["Pattern Source (--source pattern)"]
PAT[PatternGeneratorOp\ngradient / checkerboard / colorbars]
end
subgraph V4L2["V4L2 Source (--source v4l2)"]
V4L[V4L2VideoCaptureOp]
FC[FormatConverterOp\nrgba8888 → rgba8888]
V4L --> FC
end
subgraph Recorder
GST[GstVideoRecorderOp\nC++ or Python binding]
end
PAT -->|output / input| GST
FC -->|tensor / input| GST
GST -->|GStreamer pipeline| MP4[output.mp4]
subgraph Build
CPP_BUILD["--language cpp\nHOLOHUB_BUILD_CPP"]
PY_BUILD["--language python\nHOLOHUB_BUILD_PYTHON"]
end
Reviews (14): Last reviewed commit: "Clarify CuPy requirement in Python gst_v..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCondenses gst_video_recorder into C++ and Python subtrees, makes Python subdirs conditional, adds pybind11 Python bindings and pydoc for GstVideoRecorderOp, implements a Python gst_video_recorder application and tests, moves C++ app into a cpp subtree with tests, updates metadata/README and adds gstreamer tools to deps. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
operators/gstreamer/python/gst_video_recorder_op_pydoc.hpp (1)
50-66: Tighten the duplicated parameter text.The
framerateandblockdocs repeat the same guidance, andoutput.mp4should be formatted consistently as a string literal.📝 Proposed doc cleanup
framerate : str - Video framerate as fraction or decimal. - Formats: "30/1", "30000/1001", "29.97", "60" - ``"30/1"`` or ``"30000/1001"``, and decimal values such as ``"29.97"``. - Special: ``"0/1"`` for live mode (no framerate control, process frames as fast as they come) + Video framerate as a fraction or decimal, for example ``"30/1"``, + ``"30000/1001"``, ``"29.97"``, or ``"60"``. + Special value ``"0/1"`` enables live mode with no framerate control. Default value is ``"30/1"``. max_buffers : int Maximum number of buffers to queue (0 = unlimited) Default value is ``10``. block : bool - Whether ``push_buffer()`` should block when the internal queue is full. - Whether push_buffer() should block when the queue is full (true = block, false = non-blocking, may drop/timeout) + Whether ``push_buffer()`` should block when the queue is full + (``True`` = block, ``False`` = non-blocking, may drop/timeout). Default value is ``True``. filename : str Output video filename. - If no extension is provided, ``".mp4"`` is automatically appended - Default value is ``output.mp4``. + If no extension is provided, ``".mp4"`` is automatically appended. + Default value is ``"output.mp4"``.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/gstreamer/python/gst_video_recorder_op_pydoc.hpp` around lines 50 - 66, Consolidate and tighten the duplicated parameter descriptions: for framerate remove the repeated sentence so it reads one concise description listing accepted formats (e.g., "30/1", "30000/1001", "29.97", "60"), mention special "0/1" for live mode, and state the default "30/1"; for block keep a single clear sentence like whether push_buffer() blocks when the internal queue is full (True = block, False = non-blocking) and the default True; for filename ensure the default value is formatted as a string literal "output.mp4" and note that ".mp4" will be appended if no extension is provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@operators/gstreamer/python/gst_video_recorder_op.cpp`:
- Around line 24-30: Include and use the pydoc header so Python help() exposes
docs: add an `#include` for "gst_video_recorder_op_pydoc.hpp" in
gst_video_recorder_op.cpp, then wire the pydoc symbols into the pybind11 class
and method definitions by passing the doc string identifiers to the py::class_
constructor and the .def() calls for initialize and setup (referencing the pydoc
symbols from gst_video_recorder_op_pydoc.hpp when constructing py::class_ and
when defining Operator::initialize and Operator::setup).
---
Nitpick comments:
In `@operators/gstreamer/python/gst_video_recorder_op_pydoc.hpp`:
- Around line 50-66: Consolidate and tighten the duplicated parameter
descriptions: for framerate remove the repeated sentence so it reads one concise
description listing accepted formats (e.g., "30/1", "30000/1001", "29.97",
"60"), mention special "0/1" for live mode, and state the default "30/1"; for
block keep a single clear sentence like whether push_buffer() blocks when the
internal queue is full (True = block, False = non-blocking) and the default
True; for filename ensure the default value is formatted as a string literal
"output.mp4" and note that ".mp4" will be appended if no extension is provided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 622137e4-9a51-4af5-828f-481915dc0d8a
📒 Files selected for processing (4)
operators/gstreamer/CMakeLists.txtoperators/gstreamer/python/CMakeLists.txtoperators/gstreamer/python/gst_video_recorder_op.cppoperators/gstreamer/python/gst_video_recorder_op_pydoc.hpp
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
asierfernandez
force-pushed
the
feature/gstreamer-python-binding
branch
from
22fdf1d to
93498ff
Compare
Contributor
Author
|
Hi @nvidia-holoscan/holohub @GilShpr this is ready for review. |
Contributor
|
Thanks @asierfernandez for contributing the Required before merge:
We need this structure and parity with the C++ recorder to review and merge. |
bhashemian
added
the
Action Required by Author
Contributor
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
bhashemian
requested changes
Apr 21, 2026
Member
bhashemian
left a comment
bhashemian
left a comment
There was a problem hiding this comment.
Thanks, @GilShpr, for the review.
@asierfernandez, could you please address the feeback so that we can proceed with this PR? Just want to reiterate that there is a requirement for any new operators to have a sample application to test the implemented functionality. Thanks!
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a pybind11-based Python extension module for the existing GStreamer video recorder operator so it can be constructed/configured directly from Python within the HoloHub Python build.
Changes:
- Added pybind11 bindings exposing
GstVideoRecorderOpin a new_holoscan_gstreamer_bridgePython module. - Added pydoc strings for the Python-facing API.
- Wired the new Python bindings into the GStreamer operator build when
HOLOHUB_BUILD_PYTHONis enabled.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| operators/gstreamer/python/gst_video_recorder_op_pydoc.hpp | Adds Python docstrings for the GstVideoRecorderOp binding. |
| operators/gstreamer/python/gst_video_recorder_op.cpp | Implements the pybind11 module and trampoline constructor for GstVideoRecorderOp. |
| operators/gstreamer/python/CMakeLists.txt | Builds the pybind11 module for the holoscan_gstreamer_bridge target. |
| operators/gstreamer/CMakeLists.txt | Adds the python/ subdirectory to the build under HOLOHUB_BUILD_PYTHON. |
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@applications/gstreamer/gst_video_recorder/cpp/CMakeLists.txt`:
- Around line 43-84: Remove stale OUTPUT_FILE before running the recorder test
by ensuring the gst_video_recorder_test clears the previous file; update the
COMMAND for the test that defines gst_video_recorder_test (which uses
$<TARGET_FILE:gst-video-recorder> and OUTPUT_FILE/RECORDING_DIR) to first remove
${OUTPUT_FILE} (e.g. run "rm -f ${OUTPUT_FILE} && ..." or create a small setup
test that runs "rm -f ${OUTPUT_FILE}" and make gst_video_recorder_test DEPEND on
it) so subsequent CTest runs won't be masked by stale test_output.mp4.
In `@applications/gstreamer/gst_video_recorder/python/gst_video_recorder.py`:
- Around line 1-396: The file fails Black formatting; run the Black
auto-formatter on the module (e.g., the gst_video_recorder.py that defines
parse_pattern, parse_v4l2_pixel_format, parse_key_value_properties,
PatternGeneratorOp, GstVideoRecorderApp, build_arg_parser, validate_args, main)
and commit the resulting changes so the file adheres to Black's style (use black
. or black <file> and then add/commit the reformatted file); no code logic
changes are required—only reformat and push.
- Around line 353-359: The CLI adds a "--storage" argument in
parser.add_argument defaulting to 1 (device/CUDA) which forces optional CuPy to
be present; change the default to 0 (host memory) so the no-args pattern and
CMake smoke tests work without CuPy, i.e., update the parser.add_argument call
for "--storage" to default=0 and adjust any tests or docs to pass or mention
"--storage 1" when device/CUDA is required; ensure references to the storage
choice in code (e.g., logic that checks storage value) remain consistent.
- Around line 364-375: validate_args currently only checks numeric args and
leaves string flags empty; add non-empty validation for args.output,
args.encoder, and args.device in validate_args so blank strings fail fast. For
each of args.output, args.encoder, args.device, check that the value is not None
and that str(value).strip() is non-empty and raise SystemExit with a clear
message (e.g., "--output cannot be empty", "--encoder cannot be empty",
"--device cannot be empty") when invalid; keep these checks alongside the
existing width/height/framerate validations in the validate_args function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef7d84d9-4042-477f-b66c-cdfbece43c2c
📒 Files selected for processing (10)
applications/gstreamer/gst_video_recorder/CMakeLists.txtapplications/gstreamer/gst_video_recorder/README.mdapplications/gstreamer/gst_video_recorder/cpp/CMakeLists.txtapplications/gstreamer/gst_video_recorder/cpp/gst_video_recorder.cppapplications/gstreamer/gst_video_recorder/cpp/gst_video_recorder_v4l2.yamlapplications/gstreamer/gst_video_recorder/cpp/metadata.jsonapplications/gstreamer/gst_video_recorder/python/CMakeLists.txtapplications/gstreamer/gst_video_recorder/python/gst_video_recorder.pyapplications/gstreamer/gst_video_recorder/python/metadata.jsonapplications/gstreamer/install_deps.sh
✅ Files skipped from review due to trivial changes (5)
- applications/gstreamer/install_deps.sh
- applications/gstreamer/gst_video_recorder/cpp/gst_video_recorder.cpp
- applications/gstreamer/gst_video_recorder/README.md
- applications/gstreamer/gst_video_recorder/cpp/metadata.json
- applications/gstreamer/gst_video_recorder/python/metadata.json
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
applications/gstreamer/gst_video_recorder/python/gst_video_recorder.py (2)
254-255: Dead branch:argparsechoices=("pattern", "v4l2")already rejects other values.This
elsecan only be reached if someone mutatesargs.sourceafter parsing. Safe to drop, or demote to anassertto document the invariant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/gstreamer/gst_video_recorder/python/gst_video_recorder.py` around lines 254 - 255, The explicit RuntimeError branch after parsing is dead because argparse enforces choices=("pattern","v4l2"); remove the else branch (or replace it with an assert) to document the invariant instead of raising a runtime error—locate the code that checks self.args.source (the block that currently ends with raise RuntimeError(f"unsupported source '{self.args.source}'")) and either delete that else branch or change it to assert self.args.source in ("pattern","v4l2"), preserving surrounding logic and comments.
157-181: Color bars: replace the per-column Python loop with a vectorized fill.For 1920-wide frames this is 1920 Python iterations per frame through NumPy/CuPy fancy indexing. A single
repeat/broadcast produces the same result in one op. Strictly an optimization — no behavior change expected.♻️ Proposed refactor
- frame = self.xp.empty((self.height, self.width, RGBA_CHANNELS), dtype=self.xp.uint8) - bar_width = self.width // SMPTE_COLOR_BARS - if bar_width == 0: - bar_width = 1 - - for x in range(self.width): - bar_idx = x // bar_width - if bar_idx >= SMPTE_COLOR_BARS: - bar_idx = SMPTE_COLOR_BARS - 1 - frame[:, x, :] = colors[bar_idx] - return frame + bar_width = max(1, self.width // SMPTE_COLOR_BARS) + xs = self.xp.arange(self.width) + bar_idx = self.xp.minimum(xs // bar_width, SMPTE_COLOR_BARS - 1) + row = colors[bar_idx] # shape (width, 4) + return self.xp.broadcast_to(row, (self.height, self.width, RGBA_CHANNELS)).copy()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/gstreamer/gst_video_recorder/python/gst_video_recorder.py` around lines 157 - 181, The _colorbars method currently fills frame per-column with a Python loop causing O(width) Python iterations; replace that loop with a vectorized operation using self.xp (NumPy/CuPy) to build a 1D array of bar indices of length self.width via repeat or tile based on bar_width and SMPTE_COLOR_BARS, then index into colors and broadcast/reshape to assign the entire frame in one operation (use colors[bar_indices] then reshape or expand_dims to set frame[:, :, :] accordingly), preserving RGBA_CHANNELS and edge handling where bar_idx >= SMPTE_COLOR_BARS by clamping the bar indices to SMPTE_COLOR_BARS-1.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@applications/gstreamer/gst_video_recorder/python/gst_video_recorder.py`:
- Around line 370-373: The validation rejects an explicit --count 0 even though
the code treats 0 as the sentinel for "unlimited" (see args.count,
_source_condition_args and CountCondition); update the check to accept 0 as a
valid value (i.e., allow args.count == 0 or 1 <= args.count <= 1_000_000_000) so
an explicit --count 0 maps to the unlimited sentinel, and keep or adjust the
error message to mention that 0 represents unlimited if it remains used in
user-facing output.
- Around line 87-110: The __init__ currently declares allocator=None but
swallows it locally so callers passing allocator have no effect; either remove
the allocator parameter from gst_video_recorder.PatternGeneratorOp.__init__
signature (since pattern mode uses NumPy/CuPy and doesn't need a Holoscan
allocator) and also remove allocator=allocator where PatternGeneratorOp is
instantiated, or explicitly forward allocator to the base class by passing
allocator through to super().__init__(..., allocator=allocator, **kwargs) so the
allocator is registered with the operator; update the instantiation sites (e.g.,
where PatternGeneratorOp(...) is called) accordingly to avoid duplicate/unused
parameters.
---
Nitpick comments:
In `@applications/gstreamer/gst_video_recorder/python/gst_video_recorder.py`:
- Around line 254-255: The explicit RuntimeError branch after parsing is dead
because argparse enforces choices=("pattern","v4l2"); remove the else branch (or
replace it with an assert) to document the invariant instead of raising a
runtime error—locate the code that checks self.args.source (the block that
currently ends with raise RuntimeError(f"unsupported source
'{self.args.source}'")) and either delete that else branch or change it to
assert self.args.source in ("pattern","v4l2"), preserving surrounding logic and
comments.
- Around line 157-181: The _colorbars method currently fills frame per-column
with a Python loop causing O(width) Python iterations; replace that loop with a
vectorized operation using self.xp (NumPy/CuPy) to build a 1D array of bar
indices of length self.width via repeat or tile based on bar_width and
SMPTE_COLOR_BARS, then index into colors and broadcast/reshape to assign the
entire frame in one operation (use colors[bar_indices] then reshape or
expand_dims to set frame[:, :, :] accordingly), preserving RGBA_CHANNELS and
edge handling where bar_idx >= SMPTE_COLOR_BARS by clamping the bar indices to
SMPTE_COLOR_BARS-1.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 59d374ba-667e-471e-9b90-45465d58ec3d
📒 Files selected for processing (3)
applications/gstreamer/gst_video_recorder/README.mdapplications/gstreamer/gst_video_recorder/python/CMakeLists.txtapplications/gstreamer/gst_video_recorder/python/gst_video_recorder.py
✅ Files skipped from review due to trivial changes (1)
- applications/gstreamer/gst_video_recorder/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- applications/gstreamer/gst_video_recorder/python/CMakeLists.txt
Contributor
Author
|
OMG! Bots never stop finding issues ;-) Thanks @GilShpr and @bhashemian for your comments. I included @GilShpr requests changes. I can also finish fixing issues detected by bots, even they don't have a big impact. Currently with my laptop I can not test gpu path neither |
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
applications/gstreamer/gst_video_recorder/python/gst_video_recorder.py (2)
91-114:⚠️ Potential issue | 🟡 Minor
allocatorkwarg is still accepted and silently discarded.
allocator=Noneis consumed by the explicit keyword and never forwarded tosuper().__init__, but the caller on line 229 passesallocator=allocator. Either drop the parameter (pattern mode uses NumPy/CuPy directly and doesn't need a Holoscan allocator) and removeallocator=allocatorfrom the instantiation site, or forward it explicitly:super().__init__(fragment, *args, allocator=allocator, **kwargs).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/gstreamer/gst_video_recorder/python/gst_video_recorder.py` around lines 91 - 114, The __init__ accepts an allocator kwarg but currently swallows it (allocator=None) and doesn't forward it to the base constructor; update the constructor to either remove the allocator parameter or forward it to super: change the __init__ signature handling so allocator is passed through in the call to super().__init__ (i.e., call super().__init__(fragment, *args, allocator=allocator, **kwargs)) so the allocator passed by callers (see instantiation site that uses allocator=allocator) is respected; ensure no other code assumes allocator was consumed and keep existing storage_type/xp logic intact.
375-378:⚠️ Potential issue | 🟡 Minor
--count 0is still rejected despite0being the "unlimited" sentinel.
_source_condition_argson line 207 treatscount > 0as "bounded" andcount == 0as "unlimited", and--helpadvertises "default: unlimited", but an explicit--count 0still errors with"must be between 1 and 1000000000". Accept0as an explicit alias for unlimited or make the message mention it.Proposed fix
- if args.count is None: + if args.count is None or args.count == 0: args.count = 0 - elif not (1 <= args.count <= 1_000_000_000): - raise SystemExit("--count must be between 1 and 1000000000") + elif not (1 <= args.count <= 1_000_000_000): + raise SystemExit("--count must be 0 (unlimited) or between 1 and 1000000000")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/gstreamer/gst_video_recorder/python/gst_video_recorder.py` around lines 375 - 378, The check rejects an explicit --count 0 even though 0 is intended as the "unlimited" sentinel used by _source_condition_args; update the validation around args.count so that 0 is accepted as a valid value (or update the error text to mention 0 as unlimited). Specifically, replace the current range check on args.count in gst_video_recorder.py (the block that sets args.count when None and then raises SystemExit) with logic that allows args.count == 0 or 1 <= args.count <= 1_000_000_000, and ensure the SystemExit message reflects that 0 is accepted as "unlimited" if you choose to keep the error path.
🧹 Nitpick comments (1)
applications/gstreamer/gst_video_recorder/python/gst_video_recorder.py (1)
189-196: Deadelsebranch afterparse_patternvalidation.
parse_patternalready rejects anything outside{0, 1, 2}at CLI parse time, soself.patterncan never reach theelsefallback. Drop the redundant branch to keep the control flow honest.Proposed fix
- if self.pattern == 0: - frame = self._gradient() - elif self.pattern == 1: - frame = self._checkerboard() - elif self.pattern == 2: - frame = self._colorbars() - else: - frame = self._gradient() + if self.pattern == 0: + frame = self._gradient() + elif self.pattern == 1: + frame = self._checkerboard() + else: # self.pattern == 2 + frame = self._colorbars()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/gstreamer/gst_video_recorder/python/gst_video_recorder.py` around lines 189 - 196, The final else branch is redundant because parse_pattern enforces pattern ∈ {0,1,2}; update the branch in the method that selects frames (the block using self.pattern and calling _gradient, _checkerboard, _colorbars) to remove the else fallback and leave an explicit if / elif / elif chain, and add an assertion (e.g., assert self.pattern in (0,1,2)) or a defensive ValueError immediately before selection to document the invariant enforced by parse_pattern; refer to parse_pattern, self.pattern, and the helper methods _gradient, _checkerboard, _colorbars when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@applications/gstreamer/gst_video_recorder/python/gst_video_recorder.py`:
- Around line 91-114: The __init__ accepts an allocator kwarg but currently
swallows it (allocator=None) and doesn't forward it to the base constructor;
update the constructor to either remove the allocator parameter or forward it to
super: change the __init__ signature handling so allocator is passed through in
the call to super().__init__ (i.e., call super().__init__(fragment, *args,
allocator=allocator, **kwargs)) so the allocator passed by callers (see
instantiation site that uses allocator=allocator) is respected; ensure no other
code assumes allocator was consumed and keep existing storage_type/xp logic
intact.
- Around line 375-378: The check rejects an explicit --count 0 even though 0 is
intended as the "unlimited" sentinel used by _source_condition_args; update the
validation around args.count so that 0 is accepted as a valid value (or update
the error text to mention 0 as unlimited). Specifically, replace the current
range check on args.count in gst_video_recorder.py (the block that sets
args.count when None and then raises SystemExit) with logic that allows
args.count == 0 or 1 <= args.count <= 1_000_000_000, and ensure the SystemExit
message reflects that 0 is accepted as "unlimited" if you choose to keep the
error path.
---
Nitpick comments:
In `@applications/gstreamer/gst_video_recorder/python/gst_video_recorder.py`:
- Around line 189-196: The final else branch is redundant because parse_pattern
enforces pattern ∈ {0,1,2}; update the branch in the method that selects frames
(the block using self.pattern and calling _gradient, _checkerboard, _colorbars)
to remove the else fallback and leave an explicit if / elif / elif chain, and
add an assertion (e.g., assert self.pattern in (0,1,2)) or a defensive
ValueError immediately before selection to document the invariant enforced by
parse_pattern; refer to parse_pattern, self.pattern, and the helper methods
_gradient, _checkerboard, _colorbars when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e30679d4-29a4-420b-b31d-8a05a8595651
📒 Files selected for processing (7)
applications/gstreamer/gst_video_recorder/README.mdapplications/gstreamer/gst_video_recorder/cpp/CMakeLists.txtapplications/gstreamer/gst_video_recorder/python/CMakeLists.txtapplications/gstreamer/gst_video_recorder/python/gst_video_recorder.pyoperators/gstreamer/python/CMakeLists.txtoperators/gstreamer/python/gst_video_recorder_op.cppoperators/gstreamer/python/gst_video_recorder_op_pydoc.hpp
🚧 Files skipped from review as they are similar to previous changes (4)
- applications/gstreamer/gst_video_recorder/README.md
- operators/gstreamer/python/CMakeLists.txt
- applications/gstreamer/gst_video_recorder/python/CMakeLists.txt
- applications/gstreamer/gst_video_recorder/cpp/CMakeLists.txt
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
GilShpr
requested changes
Apr 22, 2026
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
bhashemian
reviewed
Apr 22, 2026
Member
bhashemian
left a comment
bhashemian
left a comment
There was a problem hiding this comment.
Thanks @asierfernandez for this contribution. I’ve added a few inline comments that would be great to address.
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
Contributor
Author
|
Thanks @bhashemian @GilShpr for the detailed review and for taking the time to go through this so carefully. Also, sorry for the multiple re-review requests on this PR. I hope this round is finally the last one, but if there is still anything that does not quite line up, I will be happy to fix it. |
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
Signed-off-by: Asier Fernandez <asier.fernandez@tecnalia.com>
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.
Added python bindings for gstreamer video recorder operator
Summary by CodeRabbit
New Features
Documentation
Tests
Chores