Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers: video: add format utilities to improve usability #79476

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions doc/releases/migration-guide-4.1.rst
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,11 @@ Video
The new ``video-controls.h`` source now contains description of each control ID to help
disambiguating.

* The ``video_pix_fmt_bpp()`` function was returning a byte count, this got replaced by
``video_bits_per_pixel()`` which return a bit count. For instance, invocations such as
``pitch = width * video_pix_fmt_bpp(pixfmt)`` needs to be replaced by an equivalent
``pitch = width * video_bits_per_pixel(pixfmt) / 8``.

Watchdog
========

Expand Down
3 changes: 3 additions & 0 deletions doc/releases/release-notes-4.1.rst
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,9 @@ Drivers and Sensors
* Changed :file:`include/zephyr/drivers/video-controls.h` to have control IDs (CIDs) matching
those present in the Linux kernel.

* Changed ``video_pix_fmt_bpp()`` returning the byte count and only supports 8-bit depth,
into ``video_bits_per_pixel()`` returning the bit count and supports any color depth.

* Watchdog

* Wi-Fi
Expand Down
4 changes: 2 additions & 2 deletions drivers/video/video_mcux_csi.c
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ static inline void video_pix_fmt_convert(struct video_format *fmt, bool isGetFmt
break;
}

fmt->pitch = fmt->width * video_pix_fmt_bpp(fmt->pixelformat);
fmt->pitch = fmt->width * video_bits_per_pixel(fmt->pixelformat) / 8;
}
#endif

Expand All @@ -132,7 +132,7 @@ static int video_mcux_csi_set_fmt(const struct device *dev, enum video_endpoint_
{
const struct video_mcux_csi_config *config = dev->config;
struct video_mcux_csi_data *data = dev->data;
unsigned int bpp = video_pix_fmt_bpp(fmt->pixelformat);
unsigned int bpp = video_bits_per_pixel(fmt->pixelformat) / 8;
status_t ret;
struct video_format format = *fmt;

Expand Down
4 changes: 2 additions & 2 deletions drivers/video/video_mcux_mipi_csi2rx.c
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ static int mipi_csi2rx_update_settings(const struct device *dev, enum video_endp
return -ENOTSUP;
}

bpp = video_pix_fmt_bpp(fmt.pixelformat) * 8;
bpp = video_bits_per_pixel(fmt.pixelformat);
sensor_byte_clk = sensor_pixel_rate * bpp / drv_data->csi2rxConfig.laneNum / 8;

ret = clock_control_get_rate(drv_data->clock_dev, drv_data->clock_root, &root_clk_rate);
Expand Down Expand Up @@ -224,7 +224,7 @@ static int mipi_csi2rx_get_frmival(const struct device *dev, enum video_endpoint

static uint64_t mipi_csi2rx_cal_frame_size(const struct video_format *fmt)
{
return fmt->height * fmt->width * video_pix_fmt_bpp(fmt->pixelformat) * 8;
return fmt->height * fmt->width * video_bits_per_pixel(fmt->pixelformat);
}

static uint64_t mipi_csi2rx_estimate_pixel_rate(const struct video_frmival *cur_fmival,
Expand Down
2 changes: 1 addition & 1 deletion drivers/video/video_stm32_dcmi.c
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ static int video_stm32_dcmi_set_fmt(const struct device *dev,
{
const struct video_stm32_dcmi_config *config = dev->config;
struct video_stm32_dcmi_data *data = dev->data;
unsigned int bpp = video_pix_fmt_bpp(fmt->pixelformat);
unsigned int bpp = video_bits_per_pixel(fmt->pixelformat) / 8;

if (bpp == 0 || (ep != VIDEO_EP_OUT && ep != VIDEO_EP_ALL)) {
return -EINVAL;
Expand Down
153 changes: 120 additions & 33 deletions include/zephyr/drivers/video.h
Original file line number Diff line number Diff line change
Expand Up @@ -812,104 +812,191 @@ void video_closest_frmival_stepwise(const struct video_frmival_stepwise *stepwis
void video_closest_frmival(const struct device *dev, enum video_endpoint_id ep,
struct video_frmival_enum *match);

/* fourcc - four-character-code */
#define video_fourcc(a, b, c, d) \
((uint32_t)(a) | ((uint32_t)(b) << 8) | ((uint32_t)(c) << 16) | ((uint32_t)(d) << 24))

/**
* @defgroup video_pixel_formats Video pixel formats
* The @c | characters separate the pixels, and spaces separate the bytes.
* The uppercase letter represents the most significant bit.
* The lowercase letters represent the rest of the bits.
* @{
*/

/**
* @name Bayer formats
* @brief Four-character-code uniquely identifying the pixel format
*/
#define VIDEO_FOURCC(a, b, c, d) \
((uint32_t)(a) | ((uint32_t)(b) << 8) | ((uint32_t)(c) << 16) | ((uint32_t)(d) << 24))

/**
* @brief Convert a four-character-string to a four-character-code
*
* Convert a string literal or variable into a four-character-code
* as defined by @ref VIDEO_FOURCC.
*
* @param str String to be converted
* @return Four-character-code.
*/
#define VIDEO_FOURCC_FROM_STR(str) VIDEO_FOURCC((str)[0], (str)[1], (str)[2], (str)[3])

/**
* @name Bayer formats (R, G, B channels).
*
* The full color information is spread over multiple pixels.
*
* @{
*/

/** BGGR8 pixel format */
#define VIDEO_PIX_FMT_BGGR8 video_fourcc('B', 'G', 'G', 'R') /* 8 BGBG.. GRGR.. */
/** GBRG8 pixel format */
#define VIDEO_PIX_FMT_GBRG8 video_fourcc('G', 'B', 'R', 'G') /* 8 GBGB.. RGRG.. */
/** GRBG8 pixel format */
#define VIDEO_PIX_FMT_GRBG8 video_fourcc('G', 'R', 'B', 'G') /* 8 GRGR.. BGBG.. */
/** RGGB8 pixel format */
#define VIDEO_PIX_FMT_RGGB8 video_fourcc('R', 'G', 'G', 'B') /* 8 RGRG.. GBGB.. */
/**
* @verbatim
* | Bbbbbbbb | Gggggggg | Bbbbbbbb | Gggggggg | Bbbbbbbb | Gggggggg | ...
* | Gggggggg | Rrrrrrrr | Gggggggg | Rrrrrrrr | Gggggggg | Rrrrrrrr | ...
* @endverbatim
*/
#define VIDEO_PIX_FMT_BGGR8 VIDEO_FOURCC('B', 'A', '8', '1')

/**
* @verbatim
* | Gggggggg | Bbbbbbbb | Gggggggg | Bbbbbbbb | Gggggggg | Bbbbbbbb | ...
* | Rrrrrrrr | Gggggggg | Rrrrrrrr | Gggggggg | Rrrrrrrr | Gggggggg | ...
* @endverbatim
*/
#define VIDEO_PIX_FMT_GBRG8 VIDEO_FOURCC('G', 'B', 'R', 'G')

/**
* @verbatim
* | Gggggggg | Rrrrrrrr | Gggggggg | Rrrrrrrr | Gggggggg | Rrrrrrrr | ...
* | Bbbbbbbb | Gggggggg | Bbbbbbbb | Gggggggg | Bbbbbbbb | Gggggggg | ...
* @endverbatim
*/
#define VIDEO_PIX_FMT_GRBG8 VIDEO_FOURCC('G', 'R', 'B', 'G')

/**
* @verbatim
* | Rrrrrrrr | Gggggggg | Rrrrrrrr | Gggggggg | Rrrrrrrr | Gggggggg | ...
* | Gggggggg | Bbbbbbbb | Gggggggg | Bbbbbbbb | Gggggggg | Bbbbbbbb | ...
* @endverbatim
*/
#define VIDEO_PIX_FMT_RGGB8 VIDEO_FOURCC('R', 'G', 'G', 'B')

/**
* @}
*/

/**
* @name RGB formats
* Per-color (R, G, B) channels.
* @{
*/

/** RGB565 pixel format */
#define VIDEO_PIX_FMT_RGB565 video_fourcc('R', 'G', 'B', 'P') /* 16 RGB-5-6-5 */
/**
* 5 red bits [15:11], 6 green bits [10:5], 5 blue bits [4:0].
* This 16-bit integer is then packed in big endian format over two bytes:
*
* @verbatim
* 15.....8 7......0
* | RrrrrGgg gggBbbbb | ...
* @endverbatim
*/
#define VIDEO_PIX_FMT_RGB565X VIDEO_FOURCC('R', 'G', 'B', 'R')

/**
* 5 red bits [15:11], 6 green bits [10:5], 5 blue bits [4:0].
* This 16-bit integer is then packed in little endian format over two bytes:
*
* @verbatim
* 7......0 15.....8
* | gggBbbbb RrrrrGgg | ...
* @endverbatim
*/
#define VIDEO_PIX_FMT_RGB565 VIDEO_FOURCC('R', 'G', 'B', 'P')

/** XRGB32 pixel format */
#define VIDEO_PIX_FMT_XRGB32 video_fourcc('B', 'X', '2', '4') /* 32 XRGB-8-8-8-8 */
/**
* The first byte is empty (X) for each pixel.
*
* @verbatim
* | Xxxxxxxx Rrrrrrrr Gggggggg Bbbbbbbb | ...
* @endverbatim
*/
#define VIDEO_PIX_FMT_XRGB32 VIDEO_FOURCC('B', 'X', '2', '4')

/**
* @}
*/

/**
* @name YUV formats
* Luminance (Y) and chrominance (U, V) channels.
* @{
*/

/** YUYV pixel format */
#define VIDEO_PIX_FMT_YUYV video_fourcc('Y', 'U', 'Y', 'V') /* 16 Y0-Cb0 Y1-Cr0 */

/** XYUV32 pixel format */
#define VIDEO_PIX_FMT_XYUV32 video_fourcc('X', 'Y', 'U', 'V') /* 32 XYUV-8-8-8-8 */
/**
* There is either a missing channel per pixel, U or V.
* The value is to be averaged over 2 pixels to get the value of individual pixel.
*
* @verbatim
* | Yyyyyyyy Uuuuuuuu | Yyyyyyyy Vvvvvvvv | ...
* @endverbatim
*/
#define VIDEO_PIX_FMT_YUYV VIDEO_FOURCC('Y', 'U', 'Y', 'V')

/**
* The first byte is empty (X) for each pixel.
*
* @verbatim
* | Xxxxxxxx Yyyyyyyy Uuuuuuuu Vvvvvvvv | ...
* @endverbatim
*/
#define VIDEO_PIX_FMT_XYUV32 VIDEO_FOURCC('X', 'Y', 'U', 'V')

/**
* @}
*/

/**
* @name JPEG formats
* @name Compressed formats
* @{
*/

/** JPEG pixel format */
#define VIDEO_PIX_FMT_JPEG video_fourcc('J', 'P', 'E', 'G') /* 8 JPEG */

/**
* @}
* Both JPEG (single frame) and Motion-JPEG (MJPEG, multiple JPEG frames concatenated)
*/
#define VIDEO_PIX_FMT_JPEG VIDEO_FOURCC('J', 'P', 'E', 'G')

/**
* @}
*/

/**
* @brief Get number of bytes per pixel of a pixel format
* @brief Get number of bits per pixel of a pixel format
*
* @param pixfmt FourCC pixel format value (@ref video_pixel_formats).
*
* @param pixfmt FourCC pixel format value (\ref video_pixel_formats).
* @retval 0 if the format is unhandled or if it is variable number of bits
* @retval bit size of one pixel for this format
*/
static inline unsigned int video_pix_fmt_bpp(uint32_t pixfmt)
static inline unsigned int video_bits_per_pixel(uint32_t pixfmt)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need migration guide for the dropped API

Copy link
Contributor

@ngphibang ngphibang Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is not an API. It's just an utility / helper function. Do we also need migration guide for this ?

Copy link
Contributor

@ngphibang ngphibang Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps I misunderstood ... It seems all functions in a .h are considered as APIs.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything that's part of a public header and not explicitly marked as internal is to be considered de-facto API :) (see e.g. https://docs.zephyrproject.org/latest/doxygen/html/group__video__interface.html#gabc1c6fd4e13480b269cac9f224ee1c5b)
You may want to review the current API and look for those utilities you folks would like to mark as internal (but that would still require a heads-up to users in the form of a migration guide so that they are aware they might want to move away from using the now internal API).
FWIW it looks to me as if it would be fine to actually keep this as a public API though, it's a utility that might be useful for any user, not just in-tree drivers..?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it should not be internal and can be used by any user. Thanks

{
switch (pixfmt) {
case VIDEO_PIX_FMT_BGGR8:
case VIDEO_PIX_FMT_GBRG8:
case VIDEO_PIX_FMT_GRBG8:
case VIDEO_PIX_FMT_RGGB8:
return 1;
return 8;
case VIDEO_PIX_FMT_RGB565:
case VIDEO_PIX_FMT_YUYV:
return 2;
return 16;
case VIDEO_PIX_FMT_XRGB32:
case VIDEO_PIX_FMT_XYUV32:
return 4;
return 32;
default:
/* Variable number of bits per pixel or unknown format */
return 0;
}
}

/**
* @}
*/

#ifdef __cplusplus
}
#endif
Expand Down
4 changes: 1 addition & 3 deletions samples/drivers/video/capture/src/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -144,9 +144,7 @@ int main(void)
#endif

if (strcmp(CONFIG_VIDEO_PIXEL_FORMAT, "")) {
fmt.pixelformat =
video_fourcc(CONFIG_VIDEO_PIXEL_FORMAT[0], CONFIG_VIDEO_PIXEL_FORMAT[1],
CONFIG_VIDEO_PIXEL_FORMAT[2], CONFIG_VIDEO_PIXEL_FORMAT[3]);
fmt.pixelformat = VIDEO_FOURCC_FROM_STR(CONFIG_VIDEO_PIXEL_FORMAT);
}

LOG_INF("- Video format: %c%c%c%c %ux%u", (char)fmt.pixelformat,
Expand Down
Loading