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]

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

[kartben]

@josuah will you be coming back to this?

[josuah]

Yes absolutely. As soon as the "must finish before end of the year" race is over if possible.

[josuah]

force-push:

• rebased on main
• added a release note and migration guide