lib: pixel: towards gstreamer/ffmpeg/opencv

[josuah]

This is a proposed implementation for:

• lib: pixel: towards gstreamer/ffmpeg/opencv/libcamera #86669

Implementation notes:

• Functions named pixel_<a>_to_<b>() or pixel_<operation>_<a>(), where <a> and <b> are pixel formats name with frame/line/hist/avg suffix (no reliance on <zephyr/drivers/video.h> or <zephyr/drivers/display.h> formats)
• Heavy use of static inline: to allow the compiler to optimize the constants away and build operations that work on whole-lines without function call inside.
• No unit tests yet, as maybe using VIDEO_SW_GENERATOR to build the test data was interesting?

[josuah]

To test the samples on the native simulator:

west build -p -b native_sim samples/lib/pixel/print/ && west flash
west build -p -b native_sim samples/lib/pixel/resize/ && west flash
west build -p -b native_sim samples/lib/pixel/stats/ && west flash
west build -p -b native_sim samples/lib/pixel/stream/ && west flash

[nashif]

west build -p -b native_sim samples/lib/pixel/print/ && west flash

wow, this is nice

[IAmAnkur]

More of a suggestion:
If it is guaranteed that src and dst addresses never overlap, then we are good but for cases where src and dst addresses might overlap, i have seen memcpy leading to pixel data corruption So it is good to either handle such scenario or use safe version of memcpy (memcpy_s) which inherently checks for address overlap.

[josuah]

This function could be called to shrink a buffer in-place, and this would lead to a bug!

[josuah]

memcpy_s() seemed like unsupported, so I switched to memmove() to allow incrementally improve performance as use-cases shows-up. This would only copy 1~3 bytes in typical usage anyway I think.

[IAmAnkur]

Just a suggestion:
Seems like one of the input parameter (video_endpoint_id ep) is not getting used in function, if we do not have plan to use it, it is good to remove it.

[josuah]

There was missing input validation on it!

[ngphibang]

Just a side note. I think all things related to video_endpoint_id should be removed from the video framework. Video port / enpoint is already described in the devicetree and up to now, all the video_endpoint_id parameters passed in video functions are not used at all, just served as a check (which is not necessary). I plan to make a PR to remove them.

[josuah]

A no-op everywhere in current drivers that is true... This PR uses multiple endpoints, should I try to make it single-endpoint (i.e. 1 input device, 1 output device)? Did you already have some plan you wanted to try for this scenario?

[ngphibang]

Yes, I have.

should I try to make it single-endpoint (i.e. 1 input device, 1 output device)?

It depends on the complexity of the real hw device that we should expose it as a single or multiple devices or not. But I think it's not really related to the discussed point here.

In the v4z framework, the video_endpoint_id parameter put in each API function has nothing to do with its meaning. There are current drivers such as mipicsi2rx which also has multiple endpoints : ep_in is the camera sensor and ep_out is the csi (dma) but why it does not need to distinguish the ep direction like the ISP (as in the current PR) ?

So, the difference is not because a device has multiple endpoints (direction) or not, but it's because the ISP (and other devices like the PxP) falls into another catergory which (Linux) calls m2m device. For this kind of devices, the driver needs two separate buffer queues and hence needs to distinguish between input (which Linux falsely call "output") and output (which Linux calls "capture") side. To support this kind of devices, we can do as follow depending on each API:

• get/set_format() : add a "type" (input or output) field in struct video_format
• enqueue/dequeue() : add a "type" field in struct video_buffer
• set_stream() : add a type parameter in the function API. I see that you didn't implement this callback. But streaming need to be done separately for each side.
• set/get_ctrl() : nothing to do as control IDs are already different for input and output (and also, we rarely has to set controls on both input and output)
• set/get/enum_frmival() : no need.

I will push the PR this weekend. I plan to do more reworks in the framework to support m2m device as we also need it for rewriting PxP driver as said in my last comment here.

[josuah]

Thank you for the insight, I understand the underlying challenges a little bit better.

I do my best to make these drivers follow the APIs you introduce. :) Good thing that the PRs got split!

P.S.: m2m reminds of this other PR (few months ago):

• WIP: Hardware Acceleration Driver Class #82939

[josuah]

force-push:

• Switch resize function from memcpy() to memmove() to allow in-place resizing (thank you @IAmAnkur)
• Add missing input validation from video_sw_isp_set_signal (thank you @IAmAnkur)
• Try to address CI issues at the same time, after reproducing them locally

[josuah]

force-push:

• print the performance statistics on the stream sample
• switch samples from printf() to LOG_INF()
• Try to address CI issues at the same time, after reproducing them locally (different issues from last time)

[josuah]

force-push:

• Rebase on latest main (to go past Zephyr 4.1 and base other PRs on this)

[josuah]

force-push:

• More consistent naming scheme for macros (STREAM_XXX_TO_YYY -> XXXSTREAM_TO_YYYSTREAM like function names)
• Improve the doc grouping of functions (this was only documenting a single line)
• Do not expose the single-pixel conversion functions, which:
  • Greatly reduce the number of functions
  • Still permits to convert a single pixel by specifying a width of 1 to the line conversion functions
  • Simplifies handling of formats that are not defined at a single-pixel level (i.e. YUYV)
• Introduce the RGB332 format.
• Simplify how the pixel printing functions are defined
• Rebase on latest main

The global goal is to make defining a new format of operation much less bureaucratic: only a line-conversion function, wrap it in a frame-conversion function (stream), and declare a top-level macro for it.

This also reduces the API surface to just the essential:

• Line conversion functions (the kernel operations that do the heavy lifting)
• Stream-based frame conversion functions (stitching the kernels together)

[josuah]

force-push:

• Enable UTF-8 back again
• Introduce a choice Kconfig to select the printing method (printf, printk, shell_print), including "none" to skip printing (CI timed-out due to large prints)
• Switch docstrings from @defgroup to @copydetails to document functions in batch, it seemed more intuitive and appropriate.

[josuah]

force-push:

• Syntax sugar: pipeline one-liners in C (+ macros)
• Add __weak attribute to line conversion functions to allow optimized version to be opted-in (i.e. SIMD).
• minor bugfixes
• Document the functions used by pipeline module developers
• Add a MAINTAINERS entry to allow notifications (how to create the new area?)

The documentation is somewhat better now, but needs a proper top-level introduction outside of Doxygen.

I hope these force-push are not distracting. I add them for the sake of avoiding extra PRs after the merge, in the meantime that review happens. I'll try to batch the changes more than currently done to reduce CI load and notifications.

[josuah]

Force-push:

• CI bugfixes
• Rebase on main
• Add kernel processing for both convolutional kernels (i.e. sharpen mask) or custom kernels (i.e. "median" for denoising).

Unit test result for gaussian Blur kernel:

Unit test result for median (denoise) kernel:

[josuah]

force-push:

• I could not reproduce job 39768403923 locally: it runs well when I test on this sample platform, so removing it for now. All tests are kept and pass CI.

New features will be proposed on other branches to reduce review burden, now using 1240bfd as stable checkpoint.