drivers: video: add format utilities to improve usability

[josuah]

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!

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.

[loicpoulain]

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

[josuah]

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!

[josuah]

force-push: CI fixes
force-push: rebase

[josuah]

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]

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

[josuah]

force-push:

• split formats out of <zephyr/drivers/video.h> to allow it being included in non-C files, as well as allow the list to grow without cluttering video.h, now containing no more "database-like" definitions.
• _BPP is replaced by _BITS as _BPP could mean Bytes Per Pixel or Bits Per Pixel
• Introduce a video_bits_per_pixel(pixfmt) using the _BITS marcros.
• Fix copyrights (presence only to files changed >50%)

This also propagate the changes to the various files involved.
I can do another variant where the changes are only adding the extra macros/functions, and letting each vendor import the changes progressively.

[josuah]

force-push:

• Do not split the formats to a separate header for now (only ~150 lines), also to avoid having too many things to review at once. See drivers: video: controls: rename all CIDs to Linux counterpart #78802 (review) (but glad to reopen a separate PR that takes more time to discuss this!)

[ngphibang]

Since #76475 is merged, you will need some rebase. Overall, LGTM.

[josuah]

force-push:

• Removing PRIvfmt as discussed
• Rebase on top of upstream/main after Improve NXP CSI and MIPI_CSI2Rx drivers #76475 got merged
• Replace new invocation of video_pix_fmt_bpp() by video_bits_per_pixel()
• Fix line added in commit 1 and removed in commit 2

About the naming of video_bits_per_pixel(), I would have be glad to keep video_pix_fmt_bpp(), but I feared that keeping the same function name and changing the meaning (byte -> bit) could cause issue. Glad to use some other name!

[josuah]

I documented the BGR565-LE instead of RGB565-LE, but let's resolve the RGB565 discussion first...
#79996

[josuah]

Nevermind, this is the RGB565, even if R is on the right in the MSB first representation, it is at BIT(0), and seems to be what is expected.

[ngphibang]

I think this is BGR565-LE. Per my understanding, for RGB565, R always needs to be presented first.
In BE, it begins at bit 15 while in LE it begins at bit 8 (because of byte order). I don't understand why here, R begin at bit 4 (in the middle of the byte) ?

[ngphibang]

Endianess is about the byte address order (a chunk of 8 bits), not about the bit order (MSB or LSB first), neither a chunk of 4 bits.

To get it simple, I think we can begin with BE first. An RGB565 in BE is represented as two bytes with the most significant byte stored first in memory address.

• RRRRRGGG(1st byte in memory) then GGGBBBBB(2nd byte in memory)

At this point, I think we all agree with this. Then, LE is basically just reversing the byte order (only for a multi-byte value), i.e. lower significant bytes get lower addresses. So it becomes:

• GGGBBBBB(1st byte in memory) then RRRRRGGG(2nd byte in memory)

[josuah]

What I documented here follows Libav and the LWN article, from where I had the representation:

If representing the format in MSBit first, B is on the left.
Big-endian, MSBit first:
15 14 13 12 11 10 9 8 7 6 5 4 3 2 1 0
B b b b b G g g g g g R r r r r

little-endian, MSBit first:
7 6 5 4 3 2 1 0 15 14 13 12 11 10 9 8
g g g R r r r r B b b b b G g g

THIS ⬆️ MIGHT BE WRONG

If representing the format in LSBit first, R is on the left. This representation is confusing when mixing bit-endianness and byte endianness.

big-endian, LSBit first:
0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
r r r r R g g g g g G b b b b B

little-endian, LSBit first:
8 9 10 11 12 13 14 15 0 1 2 3 4 5 6 7
g g G b b b b B r r r r R g g g

At this point, I think we all agree with this.

I am trying to understand what the major implementations did agree... and they seem to differ? I am really not sure as they are around since a lot of time. ^_^' Can we settle that the hardware is right to break the tie?

Looking at #79996 (comment) it looks like red in MSBit position wins.

A test pattern generator looks "ok" (stripes of color) even with the wrong color order, and the LWN article is just text, not code that was checked against hardware. Some deeply anchored confusion in tech it seems!

Thank you for your patience and sorry for the confusion.

Committing the fix.

[ngphibang]

Yes, I believe strongly that the LWN article is WRONG. Personally, If I need a reference, I am usually based on the kernel document. The kernel document of the latest Linux release describes the same layout about the RGB565-LE as I pointed in above comment:

https://www.kernel.org/doc/html/v6.11/userspace-api/media/v4l/pixfmt-rgb.html

[josuah]

force-push:

• Remove the _BITS macros as discussed here
• Typo fixes as discussed here
• Fix an inversion in a bayer format documentation
• Add extra description of the RGB565 with the bit position to avoid ambiguity as discussed here (glad to make further edits if this is not fully resolved!)
• Add the RGB565X format for the sole purpose of having the big-endian version at sight on the documentation.
• Other typo fixes.

[josuah]

force-push:

• Doc fix, I did not spot it from running make -C doc doxygen!

[kartben]

need migration guide for the dropped API

[ngphibang]

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

[ngphibang]

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

[kartben]

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..?

[ngphibang]

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