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

Merged

Conversation

josuah
Copy link
Collaborator

@josuah josuah commented Oct 6, 2024

Introduce several enhancements for dealing with the video API:

  • Documentation of formats inspired from this LWN article [EDIT: this representation is convenient, but note that the bit packing of RGB565, RGB555, RGB565X, RGB555X are wrong in it].

note: this image is wrong for RGB565 format!
scrot_20241006_203817_655x344

But in a text-based format to be easy to maintain and colorblindness-friendly, and easy to grep through[EDIT: fixed version below]:

* | Bbbbbbbb | Gggggggg | Bbbbbbbb | Gggggggg | Bbbbbbbb | Gggggggg | ...
* | Gggggggg | Rrrrrrrr | Gggggggg | Rrrrrrrr | Gggggggg | Rrrrrrrr | ...
*
* | gggBbbbb | RrrrrGgg | ...
*
* | Xxxxxxxx Rrrrrrrr Gggggggg Bbbbbbbb | ...
  • Add a printf() formatter like stdint.h's PRIu32: printf("video format: " PRIvfmt, PRIvfmt_arg(&fmt));
    This is present elsewhere on Zephyr

  • Convert video_fourcc() to VIDEO_FOURCC() and make it visible to the documentation, now that it is used outside of the header itself.

  • Add a VIDEO_FOURCC_FROM_STR() to easily convert a string from i.e. Kconfig or the devicetree.

Looking forward to your feedback and advises.

@josuah josuah added Enhancement Changes/Updates/Additions to existing features area: Video Video subsystem labels Oct 6, 2024
@zephyrbot zephyrbot added the area: Samples Samples label Oct 6, 2024
@zephyrbot zephyrbot requested review from kartben and nashif October 6, 2024 18:52
/**
* @brief Print formatter to use in printf() style format strings.
*
* This prints a video format in the form of "{width}x{height} {fourcc}" format
Copy link
Collaborator

Choose a reason for hiding this comment

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

From macro below, should be "{fourcc} {width}x{height}" ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right, I changed it after seeing how v4l2-ctl and the examples show the formats but forgot to update thedoc. I will adjust it along with other doc fixups.
Thank you for pointing it!

@loicpoulain loicpoulain self-requested a review October 11, 2024 20:04
@josuah josuah force-pushed the pr-video-format-utilities branch from 29a4758 to 6c21f48 Compare October 14, 2024 11:40
loicpoulain
loicpoulain previously approved these changes Oct 14, 2024
@josuah josuah force-pushed the pr-video-format-utilities branch 3 times, most recently from c021247 to d541c10 Compare October 14, 2024 21:53
@josuah
Copy link
Collaborator Author

josuah commented Oct 14, 2024

force-push: CI fixes
force-push: rebase

@josuah josuah force-pushed the pr-video-format-utilities branch from d541c10 to a9587f1 Compare October 15, 2024 16:14
@josuah
Copy link
Collaborator Author

josuah commented Oct 15, 2024

force-push: use _BPP instead of _BITS_PER_PIXEL following this new function naming:
https://github.com/zephyrproject-rtos/zephyr/pull/76475/files#diff-70c4de6949b97c13ac893b4bfb16fbcc433593fbfa96ec8947bb5cb83c4d04bcR835-R852

@josuah
Copy link
Collaborator Author

josuah commented Oct 21, 2024

force-push: split <zephyr/drivers/video-formats.h> into <zephyr/drivers/video/formats.h>, like it is done for all other Zephyr driver includes: include/zephyr/drivers

@zephyrbot zephyrbot requested a review from MaureenHelm October 21, 2024 10:03
@josuah josuah force-pushed the pr-video-format-utilities branch from 75345f9 to ac056cf Compare October 21, 2024 10:08
ngphibang
ngphibang previously approved these changes Jan 15, 2025
* 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``.
Copy link
Member

@cfriedt cfriedt Jan 18, 2025

Choose a reason for hiding this comment

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

Please use BITS_PER_BYTE throughout this change.

Copy link
Member

@cfriedt cfriedt left a comment

Choose a reason for hiding this comment

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

Please use BITS_PER_BYTE instead of /8 or *8

@josuah
Copy link
Collaborator Author

josuah commented Jan 18, 2025

force-push:

  • rebase on main
  • use / BITS_PER_BYTE instead of / 8 when relevant (thank you @cfriedt!)

I also applied BITS_PER_BYTE here location hoping that is what was meant.

@josuah josuah force-pushed the pr-video-format-utilities branch from 6ce200d to f1bc836 Compare January 19, 2025 17:46
cfriedt
cfriedt previously approved these changes Jan 19, 2025
kartben
kartben previously approved these changes Jan 20, 2025
doc/releases/migration-guide-4.1.rst Show resolved Hide resolved
@josuah josuah dismissed stale reviews from kartben and cfriedt via 7e1e702 January 20, 2025 13:03
@josuah josuah force-pushed the pr-video-format-utilities branch from f1bc836 to 7e1e702 Compare January 20, 2025 13:03
kartben
kartben previously approved these changes Jan 21, 2025
cfriedt
cfriedt previously approved these changes Jan 21, 2025
@kartben
Copy link
Collaborator

kartben commented Jan 23, 2025

@josuah can you please rebase? random CI glitch causing a job to be stuck and can't seem to be able to re-trigger it manually. Thanks!

Introduce a new text documentation of the video formats, describing
every bit, gives the position of the most significant bit, outline how
it is cut in bytes, and show the pixel segmentation.

Extra care is taken for the RGB565 which requies paying attention to the
bit endianess as well as the byte endianess.

Signed-off-by: Josuah Demangeon <me@josuah.net>
Rename video_fourcc() to VIDEO_FOURCC(), differing from the Linux
implementation, but more compliant with the coding style.

Also introduce a VIDEO_FOURCC_FROM_STR() to generate a FOURCC format
code out of a 4-characters string.

Signed-off-by: Josuah Demangeon <me@josuah.net>
This was present in the form of video_pix_fmt_bpp() inside ST and NXP
drivers, and was returning the number of bytes, which does not allow
support for 10-bits, 4-bits or non-byte aligned video formats.
The helper leverages the VIDEO_PIX_FMT_*_BITS macros.

Signed-off-by: Josuah Demangeon <me@josuah.net>
The previous commit introduces video_bits_per_pixel() and converts
all invocation relying on video_pix_fmt_bpp(), 3rd-party users will
need to do the same on their code, and this is not a 100% drop-in API,
so better document it on the release notes.

Signed-off-by: Josuah Demangeon <me@josuah.net>
@josuah josuah dismissed stale reviews from cfriedt and kartben via a35b5c6 January 23, 2025 17:13
@josuah josuah force-pushed the pr-video-format-utilities branch from 7e1e702 to a35b5c6 Compare January 23, 2025 17:13
@josuah
Copy link
Collaborator Author

josuah commented Jan 23, 2025

Yes of course!

Copy link
Collaborator

@kartben kartben left a comment

Choose a reason for hiding this comment

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

Yes of course!

Thanks! Didn't expect it to dismiss existing approvals as I don't think there were any conflicts but, anyway, refresh +1 :)

@kartben kartben merged commit 9cebc19 into zephyrproject-rtos:main Jan 24, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: C++ area: Process area: Samples Samples area: Video Video subsystem Enhancement Changes/Updates/Additions to existing features platform: NXP Drivers NXP Semiconductors, drivers platform: STM32 ST Micro STM32 Release Notes To be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants