[PATCH xserver 3/3] render: Fix default picture format initialization

[Keith Packard]

Adam Jackson <ajax at redhat.com> writes:

> "depth" for a picture format is the sum of bits of a/r/g/b, and not x.
> The default format list was creating an x8r8g8b8 format at depth 32,
> which is wrong. Likewise, servers supporting depth 30 would get an
> x8r8g8b8 format at depth 30, which is nonsense.

Hrm. It's not total nonsense, it's just not obviously the right idea.
You *can* draw r8g8b8 to a depth 30 object, but you probably meant to
use r10g10b10 instead.

I wonder if we should be adding both depth 32 and depth 24 888 options
at the top of this function? I fear losing a format that existing
applications are using. If we added both, I'd be less concerned about
compatibility.

>          switch (bpp) {
>          case 16:
>              /* depth 12 formats */
> -            if (pDepth->depth >= 12) {
> -                addFormat(formats, &nformats, PICT_x4r4g4b4, pDepth->depth);
> -                addFormat(formats, &nformats, PICT_x4b4g4r4, pDepth->depth);
> -            }
> +            addFormat(formats, &nformats, PICT_x4r4g4b4, 12);
> +            addFormat(formats, &nformats, PICT_x4b4g4r4, 12);
>              /* depth 15 formats */
> -            if (pDepth->depth >= 15) {
> -                addFormat(formats, &nformats, PICT_x1r5g5b5, pDepth->depth);
> -                addFormat(formats, &nformats, PICT_x1b5g5r5, pDepth->depth);
> -            }
> +            addFormat(formats, &nformats, PICT_x1r5g5b5, 15);
> +            addFormat(formats, &nformats, PICT_x1b5g5r5, 15);
>              /* depth 16 formats */
> -            if (pDepth->depth >= 16) {
> -                addFormat(formats, &nformats, PICT_a1r5g5b5, pDepth->depth);
> -                addFormat(formats, &nformats, PICT_a1b5g5r5, pDepth->depth);
> -                addFormat(formats, &nformats, PICT_r5g6b5, pDepth->depth);
> -                addFormat(formats, &nformats, PICT_b5g6r5, pDepth->depth);
> -                addFormat(formats, &nformats, PICT_a4r4g4b4, pDepth->depth);
> -                addFormat(formats, &nformats, PICT_a4b4g4r4, pDepth->depth);
> -            }
> +            addFormat(formats, &nformats, PICT_a1r5g5b5, 16);
> +            addFormat(formats, &nformats, PICT_a1b5g5r5, 16);
> +            addFormat(formats, &nformats, PICT_r5g6b5, 16);
> +            addFormat(formats, &nformats, PICT_b5g6r5, 16);
> +            addFormat(formats, &nformats, PICT_a4r4g4b4, 16);
> +            addFormat(formats, &nformats, PICT_a4b4g4r4, 16);

You can't just blindly add formats that the driver might not
support. Depth really means the bits supported by the driver; bits
outside of depth may not be present in the hardware. If you have a
driver which also supports depth 16 but only advertises depth 12, that
driver has a bug. Same comment on the 32bpp changes.

-- 
-keith
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <https://lists.x.org/archives/xorg-devel/attachments/20180222/60885e81/attachment.sig>