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

[Adam Jackson]

On Fri, 2018-02-23 at 15:57 -0800, Keith Packard wrote:

> > You might be right that we _should not_ expose formats not present in
> > the hardware, though Render has kinda already lost that fight by making
> > a1 and a4 mandatory. But the Render code today, and forever, is making
> > the assertion that the code above it _will_ handle these formats.
> 
> We just need to make sure we don't promise to store bits beyond the
> depth of the drawable.

So let's again consider:

  pict format:
format id:0x2b
type: Direct
depth:30
alpha: 0 mask 0x0
red:  16 mask 0xff
green: 8 mask 0xff
blue:  0 mask 0xff

What exactly does an 'x' channel mean in Render? Is it "alpha wired to
1.0"? If so, is that true for writes as well as reads? If so, is there
any GL hardware that's going to let you write to only six bits of .x?

We also have:

  pict format:
format id:0x27
type: Direct
depth:32
alpha: 0 mask 0x0
red:  16 mask 0xff
green: 8 mask 0xff
blue:  0 mask 0xff
  pict format:
format id:0x2a
type: Direct
depth:24
alpha: 0 mask 0x0
red:  16 mask 0xff
green: 8 mask 0xff
blue:  0 mask 0xff

If you're saying both "the format promises not to write to bits beyond
the given depth" and "x is a=1.0", then 0x27 might write to the x bits,
but 0x2a would not. I'm reasonably confident that's not what glamor
implements since we're never calling glColorMask; I'm fairly sure
that's not what fb/pixman implements either.

- ajax
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Michel Dänzer]

On 2018-02-24 12:58 AM, Keith Packard wrote:
> Michel Dänzer  writes:
> 
>> On 2018-02-22 10:53 PM, Adam Jackson wrote:
>>> "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.
>>
>> Actually, the former isn't wrong, and the latter might not be either, I
>> think. These are the constraints:
>>
>> * The sizes of a/x+r+g+b must equal bpp
>> * The sizes of a+r+g+b must be <= depth (or == ?)
> 
> We could make it ==, but we currently make it <= so that you can have
> r8g8b8 on depth 32, and r4g4b4 on depth 15 and 16.

Those break the first constraint above, is it not valid? So the only
constraint is

 The sizes of a+r+g+b must be <= depth

?

If so, I don't understand what the x component formats are good for...


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Keith Packard]

Adam Jackson  writes:

> What would this mean? If I point this picture at a depth-30 pixmap, is
> the intent really "draw into this as though it was x8r8g8b8"? Is there
> a world where that's useful?

I agree that it probably isn't useful, but we need to be careful with
the condition to not exclude *useful* formats, like r8g8b8 on depth 32,
or r5g5b5 on depth 16. Of course, the real fear is deleting a format
that an existing app is relying on...

> All the 16bpp formats say they have "depth 16", regardless of a/x.

I think that's because you don't have a depth 15 visual.

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Keith Packard]

Michel Dänzer  writes:

> On 2018-02-22 10:53 PM, Adam Jackson wrote:
>> "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.
>
> Actually, the former isn't wrong, and the latter might not be either, I
> think. These are the constraints:
>
> * The sizes of a/x+r+g+b must equal bpp
> * The sizes of a+r+g+b must be <= depth (or == ?)

We could make it ==, but we currently make it <= so that you can have
r8g8b8 on depth 32, and r4g4b4 on depth 15 and 16.

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Keith Packard]

Adam Jackson  writes:

> This is at the bottom of fbPictureInit. This is code that every driver
> already runs. The loop will find that a pixmap of depth 16 has 16 bits
> per pixel, and since that's larger than 12, it will add x4r4g4b4.

But, a pixmap of depth 15 or 16 cannot support r5g6b5 formats, and that's what
the tests do -- eliminate offering formats which require more bits than
the depth of the drawable.

> You might be right that we _should not_ expose formats not present in
> the hardware, though Render has kinda already lost that fight by making
> a1 and a4 mandatory. But the Render code today, and forever, is making
> the assertion that the code above it _will_ handle these formats.

We just need to make sure we don't promise to store bits beyond the
depth of the drawable.

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Adam Jackson]

On Thu, 2018-02-22 at 16:52 -0800, Keith Packard wrote:

> >  switch (bpp) {
> >  case 16:
> >  /* depth 12 formats */
> > -if (pDepth->depth >= 12) {
> > -addFormat(formats, , PICT_x4r4g4b4, 
> > pDepth->depth);
> > -addFormat(formats, , PICT_x4b4g4r4, 
> > pDepth->depth);
> > -}
> > +addFormat(formats, , PICT_x4r4g4b4, 12);
> > +addFormat(formats, , PICT_x4b4g4r4, 12);
> >  /* depth 15 formats */
> > -if (pDepth->depth >= 15) {
> > -addFormat(formats, , PICT_x1r5g5b5, 
> > pDepth->depth);
> > -addFormat(formats, , PICT_x1b5g5r5, 
> > pDepth->depth);
> > -}
> > +addFormat(formats, , PICT_x1r5g5b5, 15);
> > +addFormat(formats, , PICT_x1b5g5r5, 15);
> >  /* depth 16 formats */
> > -if (pDepth->depth >= 16) {
> > -addFormat(formats, , PICT_a1r5g5b5, 
> > pDepth->depth);
> > -addFormat(formats, , PICT_a1b5g5r5, 
> > pDepth->depth);
> > -addFormat(formats, , PICT_r5g6b5, pDepth->depth);
> > -addFormat(formats, , PICT_b5g6r5, pDepth->depth);
> > -addFormat(formats, , PICT_a4r4g4b4, 
> > pDepth->depth);
> > -addFormat(formats, , PICT_a4b4g4r4, 
> > pDepth->depth);
> > -}
> > +addFormat(formats, , PICT_a1r5g5b5, 16);
> > +addFormat(formats, , PICT_a1b5g5r5, 16);
> > +addFormat(formats, , PICT_r5g6b5, 16);
> > +addFormat(formats, , PICT_b5g6r5, 16);
> > +addFormat(formats, , PICT_a4r4g4b4, 16);
> > +addFormat(formats, , 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.

This is at the bottom of fbPictureInit. This is code that every driver
already runs. The loop will find that a pixmap of depth 16 has 16 bits
per pixel, and since that's larger than 12, it will add x4r4g4b4.

You might be right that we _should not_ expose formats not present in
the hardware, though Render has kinda already lost that fight by making
a1 and a4 mandatory. But the Render code today, and forever, is making
the assertion that the code above it _will_ handle these formats.

- ajax
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Adam Jackson]

On Fri, 2018-02-23 at 10:51 +0100, Michel Dänzer wrote:
> On 2018-02-22 10:53 PM, Adam Jackson wrote:
> > "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.
> 
> Actually, the former isn't wrong, and the latter might not be either, I
> think. These are the constraints:
> 
> * The sizes of a/x+r+g+b must equal bpp
> * The sizes of a+r+g+b must be <= depth (or == ?)

Okay, let's make this concrete. Xvfb at depth 30 gives me these
formats:

  pict format:
format id:0x2b
type: Direct
depth:30
alpha: 0 mask 0x0
red:  16 mask 0xff
green: 8 mask 0xff
blue:  0 mask 0xff
  pict format:
format id:0x2c
type: Direct
depth:30
alpha: 0 mask 0x0
red:   0 mask 0xff
green: 8 mask 0xff
blue: 16 mask 0xff

What would this mean? If I point this picture at a depth-30 pixmap, is
the intent really "draw into this as though it was x8r8g8b8"? Is there
a world where that's useful?

All the 16bpp formats say they have "depth 16", regardless of a/x.

- ajax
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Michel Dänzer]

On 2018-02-22 10:53 PM, Adam Jackson wrote:
> "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.

Actually, the former isn't wrong, and the latter might not be either, I
think. These are the constraints:

* The sizes of a/x+r+g+b must equal bpp
* The sizes of a+r+g+b must be <= depth (or == ?)


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Keith Packard]

Adam Jackson  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, , PICT_x4r4g4b4, pDepth->depth);
> -addFormat(formats, , PICT_x4b4g4r4, pDepth->depth);
> -}
> +addFormat(formats, , PICT_x4r4g4b4, 12);
> +addFormat(formats, , PICT_x4b4g4r4, 12);
>  /* depth 15 formats */
> -if (pDepth->depth >= 15) {
> -addFormat(formats, , PICT_x1r5g5b5, pDepth->depth);
> -addFormat(formats, , PICT_x1b5g5r5, pDepth->depth);
> -}
> +addFormat(formats, , PICT_x1r5g5b5, 15);
> +addFormat(formats, , PICT_x1b5g5r5, 15);
>  /* depth 16 formats */
> -if (pDepth->depth >= 16) {
> -addFormat(formats, , PICT_a1r5g5b5, pDepth->depth);
> -addFormat(formats, , PICT_a1b5g5r5, pDepth->depth);
> -addFormat(formats, , PICT_r5g6b5, pDepth->depth);
> -addFormat(formats, , PICT_b5g6r5, pDepth->depth);
> -addFormat(formats, , PICT_a4r4g4b4, pDepth->depth);
> -addFormat(formats, , PICT_a4b4g4r4, pDepth->depth);
> -}
> +addFormat(formats, , PICT_a1r5g5b5, 16);
> +addFormat(formats, , PICT_a1b5g5r5, 16);
> +addFormat(formats, , PICT_r5g6b5, 16);
> +addFormat(formats, , PICT_b5g6r5, 16);
> +addFormat(formats, , PICT_a4r4g4b4, 16);
> +addFormat(formats, , 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


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Peter Harris]

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

>  formats[nformats].format = PICT_x8r8g8b8;
> -formats[nformats].depth = 32;
> +formats[nformats].depth = 24;

The RENDER extension forces a depth 32 pixmap format. It does not force depth 
24. So with a 16bpp root window, there may not be a depth 24 pixmap format, and 
this picture format would not be usable at all (BadMatch with every possible 
drawable). Also, d32 x8r8g8b8 is used in the wild (eg. it looks like gtk2 
sometimes uses it to avoid manually filling the alpha channel with 0xFF). I 
suspect we want to keep this one as-is.

A "depth 30 x8r8g8b8" probably isn't useful most of the time, but maybe it 
makes sense for PutImage-then-convert-in-the-server an x8r8g8b8 image to 
x2r10g10b10 without needing a second (not-depth-30) pixmap?

Peter Harris
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel