From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/fourcc: document format naming scheme Date: Tue, 28 Apr 2026 15:16:43 +1000 Message-ID: In-Reply-To: <20260426151641.93763-1-contact@emersion.fr> References: <20260426151641.93763-1-contact@emersion.fr> <20260426151641.93763-1-contact@emersion.fr> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Typo (line 137 of the diff context):** ``` + * - The first letter indicates the number of planes, channel alignment and + * ordering. It is should not be a letter already used to denote a chann= el. ``` "It is should not be" =E2=86=92 "It should not be" --- **Cb/Cr digit reversal rule is backwards:** ``` + * - The two other digits indicate bits for the Y channel. If the Cb chann= el + * comes before Cr, the digits are reversed. ``` This is wrong when checked against Q410/Q401: - `DRM_FORMAT_Q410` (`'Q','4','1','0'`) =3D YCbCr (Cb before Cr) =E2=80=94 = digits are "10" (normal/unreversed) - `DRM_FORMAT_Q401` (`'Q','4','0','1'`) =3D YCrCb (Cr before Cb) =E2=80=94 = digits are "01" (reversed) So Q410 with Cb-before-Cr has the **normal** digits, not reversed. The rule= is inverted =E2=80=94 it should read: "If the **Cr** channel comes before = Cb, the digits are reversed." --- **Sub-sampling digit scheme doesn't cover NV formats:** ``` + * - The first digit indicates chroma sub-sampling: 0 for 2x2, 2 for 2x1, = 4 for + * none. ``` This accurately describes P-series (P010=E2=86=920=3D2x2, P210=E2=86=922=3D= 2x1) and Q-series (Q410=E2=86=924=3Dnone) formats. However, the NV family = =E2=80=94 the most common multi-planar YUV formats =E2=80=94 uses a complet= ely different scheme where the two trailing digits encode **bits-per-pixel*= * (NV12=3D12bpp, NV16=3D16bpp, NV24=3D24bpp), not "subsampling digit + Y-ch= annel bits". While the "for the most part" qualifier hedges this, it might = be worth explicitly noting that older multi-planar families (NV, YUV/YVU) p= redate and don't follow this convention, to avoid confusion. --- **P030 exception to "Y channel bits" claim:** ``` + * - The two other digits indicate bits for the Y channel. ``` P030 has digits "30" but its Y channel is 10-bit (it's a packed 3=C3=9710-b= it format). P010/P012/P016 all fit the rule correctly. P030 is arguably jus= t a naming oddity but is a notable exception within the very format family = (P-series) that the scheme is supposed to describe. --- **Underscore plane delimiter claim lacks examples:** ``` + * An underscore "_" can be used to delimit planes. ``` I couldn't find any existing format in `drm_fourcc.h` that uses an undersco= re to delimit planes. `YUV420_8BIT` and `YUV420_10BIT` have underscores, bu= t those separate the format type from the bit depth, not planes. If this is= forward-looking guidance, it might be clearer stated as such. --- **Minor observation =E2=80=94 channel list completeness:** The channel definitions are good. "D" for darkness (inverse brightness) is = a nice touch documenting the D1/D2/D4/D8 formats. The single-channel "R" fo= rmats (R1, R2, R4, R8, R10, R12, R16) are covered by listing "R" as red. --- **Overall**: The RGB naming scheme documentation is accurate and useful. Th= e YUV section needs the Cb/Cr reversal rule fixed (it's currently stating t= he opposite of what the code shows) and the typo corrected. Consider acknow= ledging that NV-family formats use a different, older convention. --- Generated by Claude Code Patch Reviewer