video: improve the capture sample #96072
Conversation
zephyrbot
requested review from
JarmouniA,
avolmat-st,
kartben,
loicpoulain,
nashif and
ngphibang
|
Once this is reviewed, it should become easy to convert every other sample to the same layout. Maybe further making some sort of common code used by the samples, which is configuring the video drivers out of Kconfig values using the video API, just for the sake of samples. |
There was a problem hiding this comment.
Pull Request Overview
Refactors the video capture sample and introduces a comprehensive video test pattern validation test. The changes improve code organization, error handling, and provide proper test coverage for video capture functionality.
- Split monolithic main function into modular setup functions for better maintainability
- Added comprehensive test for video test pattern validation using CIELAB color analysis
- Improved error handling and proper return codes throughout video capture sample
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| samples/drivers/video/capture/src/main.c | Refactored into modular functions with improved error handling |
| samples/drivers/video/capture/src/check_test_pattern.h | Removed (functionality moved to test) |
| tests/drivers/video/test_pattern/src/main.c | New comprehensive test with CIELAB color validation |
| tests/drivers/video/test_pattern/testcase.yaml | Test configuration for video pattern testing |
| tests/drivers/video/test_pattern/prj.conf | Basic project configuration |
| tests/drivers/video/test_pattern/boards/native_sim_native_64.conf | Platform-specific test configuration |
| tests/drivers/video/test_pattern/Kconfig | Configuration options for video testing |
| tests/drivers/video/test_pattern/CMakeLists.txt | CMake build configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
josuah
force-pushed
the
pr_video_sample_improvement
branch
3 times, most recently
from
c23d99b to
0e5bde7
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Here is the video test that ran and passed on CI: #INFO - 711/896 native_sim/native/64 drivers.dma.chan_blen_transfer FILTERED (runtime filter)
#INFO - 712/896 native_sim/native/64 drivers.stepper.stepper_api.adi_tmc2209_work_q PASSED (native 0.011s <host>)
INFO - 713/896 native_sim/native/64 drivers.video.test_pattern PASSED (native 0.007s <host>)
#INFO - 714/896 native_sim/native/64 drivers.stepper.stepper_api.adi_tmc2209 PASSED (native 0.012s <host>)
#INFO - 715/896 native_sim/native/64 drivers.gpio.hogs PASSED (native 0.006s <host>) |
josuah
force-pushed
the
pr_video_sample_improvement
branch
from
0e5bde7 to
3236b02
Compare
|
| #endif | ||
|
|
||
| if (strcmp(CONFIG_VIDEO_PIXEL_FORMAT, "")) { | ||
| if (CONFIG_VIDEO_FRAME_HEIGHT > 0) { |
There was a problem hiding this comment.
Behavior would change I think like that since fmt.width / fmt.height used at line 183 should be normally updated with request given by CONFIG_VIDEO_FRAME_WIDTH / HEIGHT.
There was a problem hiding this comment.
You probably meant this (185 now)?
Yes I see they need to be updated before this if() happen.
There was a problem hiding this comment.
I moved these lines to app_get_video_info() to preserve the order.
if (CONFIG_VIDEO_FRAME_HEIGHT > 0) {
fmt->height = CONFIG_VIDEO_FRAME_HEIGHT;
}
if (CONFIG_VIDEO_FRAME_WIDTH > 0) {
fmt->width = CONFIG_VIDEO_FRAME_WIDTH;
}
if (strcmp(CONFIG_VIDEO_PIXEL_FORMAT, "") != 0) {
fmt->pixelformat = VIDEO_FOURCC_FROM_STR(CONFIG_VIDEO_PIXEL_FORMAT);
} There was a problem hiding this comment.
Hum, I still think that this bunch of code should be done before the set selection since this is the width / height we want to give to the set_selection (compose). Did I missed something, or maybe this is fixed in another commit later on ?
|
Thanks @josuah for this work. I haven't commented on the last commit since previous comments regarding crop/compose would make the commit change again, however I think the approach is very good. I was also wondering if you could have a tester such as a transform element. Just an idea but basically, inserting an element between the receiver and the app, and this element would perform the test, avoiding to have test implemented into the test app. Something like: would become Test test transform would be checking the buffers given by the receiver, checking them or performing some hash or so on it for example before given them to the appli, which doesn't potentially even have any idea that there is something checking in the middle. Since we will probably tend to have more and more stuff like that, starting to get some encoder for example, why not having some debug / test purpose elements as well. |
|
Great thanks for @josuah to clean-up this sample. I used this sample for testing recently, and would like to share my experience. This sample uses a simple As the zephyr video subsystem evolves, we may build more and more complicated pipelines. Do you think it's possible to make this sample more flexible? |
|
Sorry @CkovMk I also missed your message.
Yes absolutely, not only the sample, page 27 of this slide, libMP is introduced: And here is a video about it: But in the meantime, there is also more complex hardware coming with video encoder and internal ISP. But for an external ISP, I suspect this would be only devicetree-level interconnection of devices? With a single input stream, arriving onto the video capture chip, the application would not be having more devices involved. With the current APIs, it is possible to have the application configure the format of the ISP, and the ISP configure the format of the sensor. Though, if it is arbitrary application/user choice for both, agreed, this needs to have both devices configured from |
| #endif | ||
|
|
||
| if (strcmp(CONFIG_VIDEO_PIXEL_FORMAT, "")) { | ||
| if (CONFIG_VIDEO_FRAME_HEIGHT > 0) { |
There was a problem hiding this comment.
Hum, I still think that this bunch of code should be done before the set selection since this is the width / height we want to give to the set_selection (compose). Did I missed something, or maybe this is fixed in another commit later on ?
josuah
force-pushed
the
pr_video_sample_improvement
branch
2 times, most recently
from
566e08e to
ddaabc4
Compare
Add an always-on CID for enabling the test pattern, which makes it possible to use it in tests enabling the test first enabling the pattern. Signed-off-by: Josuah Demangeon <me@josuah.net>
When possible, use static initializer instead of memset() and initialization at runtime. Fixes a bug where uninitialized memory of caps would be used, in case drivers do not initialize them like they should. Signed-off-by: Josuah Demangeon <me@josuah.net>
Increase log verbosity to print one line per captured frames and make it like in the README by default. This avoids giving the impression that the capture sample does not work. Users desiring to get better performance can disable logging or reduce verbosity from INF to WRN. Signed-off-by: Josuah Demangeon <me@josuah.net>
When possible, make use of C if statements instead of preprocessor #ifdef. This still allow conditional compilation by letting the compiler garbage-collect unused code. Signed-off-by: Josuah Demangeon <me@josuah.net>
Use an "app_" prefix for all functions local to the application. This avoids accidentally using "display_" or "video_" prefix that are reserved for the respective area. Signed-off-by: Josuah Demangeon <me@josuah.net>
josuah
force-pushed
the
pr_video_sample_improvement
branch
from
ddaabc4 to
0564078
Compare
Use the same error handling implementation as everywhere else in the
video area and the majority of Zephyr:
ret = error();
if (ret < 0) {
return ret;
}
Signed-off-by: Josuah Demangeon <me@josuah.net>
josuah
force-pushed
the
pr_video_sample_improvement
branch
from
0564078 to
ea3fa88
Compare
josuah
force-pushed
the
pr_video_sample_improvement
branch
from
ea3fa88 to
684bbf4
Compare
|
Another force-push for the compliance check (probably cocci rule unable to make sense out of he |
|
You might be interested in seeing the new I hope this suits you well. |
| int "Value used for the test pattern menu control" | ||
| default 1 | ||
| help | ||
| Some hardware support different typtes of test patteren in different order in the menu. |
There was a problem hiding this comment.
nit: typo Some drivers support different types of test patterns and/or in a different order
There was a problem hiding this comment.
Good catch >_<
And agreed on better wording.
Turn the test part of the sample code into a dedicated test into the tests/ directory instead of just samples/. Signed-off-by: Josuah Demangeon <me@josuah.net>
Move blocks of code into individual functions to make the sample more modular, component-based. Signed-off-by: Josuah Demangeon <me@josuah.net>
Centralize exit of all helper functions and display a common message that indicates clearly that the sample is not blocking on any function but instead stopped running. Signed-off-by: Josuah Demangeon <me@josuah.net>
Allow the display to also use if() instead of #if by leveraging the DEVICE_DT_GET_OR_NULL() that permit display_dev to always be defined. The compiler will const-fold all the unused variables and functions. Signed-off-by: Josuah Demangeon <me@josuah.net>
josuah
force-pushed
the
pr_video_sample_improvement
branch
from
684bbf4 to
66ef2fa
Compare
|
7 participants
I am proposing a lot of changes, with the goal to let reviewers decide which ones are relevant, essential, and which one are un-necessary or unwanted, and then I can apply the decisions.
For now, only tested with
zephyr,video-sw-generatoronnative_sim:Then
west flashto see the results.