[Mesa-dev] [PATCH] mesa: Match MESA_FORMAT_B5G6R5 for a shallow pixel format of GL_RGB

[Erik Faye-Lund]

On Wed, Sep 9, 2015 at 12:41 PM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> On Wed, Sep 09, 2015 at 12:09:40PM +0200, Erik Faye-Lund wrote:
>> On Wed, Sep 9, 2015 at 11:25 AM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
>> > On Wed, Sep 09, 2015 at 11:11:59AM +0200, Erik Faye-Lund wrote:
>> >> On Thu, Sep 3, 2015 at 6:05 PM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
>> >> > If the user supplies a pixel format of GL_RGB + GL_UNSIGNED_SHORT_5_6_5
>> >> > and specifies a generic unsized GL_RGB internal format, match that to a
>> >> > texture format of MESA_FORMAT_B5G6R5 if supported by the hardware.
>> >> >
>> >> > Noticed while playing with mesa-demos/teximage:
>> >> >
>> >> >   TexImage(RGB/565 256 x 256): 79.8 images/sec, 10.0 MB/sec
>> >> >   TexSubImage(RGB/565 256 x 256): 3804.9 images/sec, 475.6 MB/sec
>> >> >   GetTexImage(RGB/565 256 x 256): 99.5 images/sec, 12.4 MB/sec
>> >> >
>> >> > becomes
>> >> >
>> >> >   TexImage(RGB/565 256 x 256): 3439.1 images/sec, 429.9 MB/sec
>> >> >   TexSubImage(RGB/565 256 x 256): 3744.1 images/sec, 468.0 MB/sec
>> >> >   GetTexImage(RGB/565 256 x 256): 4713.5 images/sec, 589.2 MB/sec
>> >> >
>> >> > on a puny Baytrail which is still far from what it is capable of. The
>> >> > reason for the disparity is that the teximage demo uses a busy texture
>> >> > which is performs an accelerated pixel conversion from the user's B5G6R5
>> >> > into the native X8B8G8R8. After the patch, no conversion is required
>> >> > allowing use of the blitter and memcpy fast paths.
>> >> >
>> >> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>> >> > ---
>> >> >  src/mesa/main/texformat.c | 2 ++
>> >> >  1 file changed, 2 insertions(+)
>> >> >
>> >> > diff --git a/src/mesa/main/texformat.c b/src/mesa/main/texformat.c
>> >> > index fd9f335..866c7b3 100644
>> >> > --- a/src/mesa/main/texformat.c
>> >> > +++ b/src/mesa/main/texformat.c
>> >> > @@ -114,6 +114,8 @@ _mesa_choose_tex_format(struct gl_context *ctx, GLenum target,
>> >> >     case GL_RGB:
>> >> >        if (type == GL_UNSIGNED_INT_2_10_10_10_REV) {
>> >> >           RETURN_IF_SUPPORTED(MESA_FORMAT_B10G10R10A2_UNORM);
>> >> > +      } else if (type == GL_UNSIGNED_SHORT_5_6_5) {
>> >> > +         RETURN_IF_SUPPORTED(MESA_FORMAT_B5G6R5_UNORM);
>> >>
>> >> Shouldn't this be MESA_FORMAT_R5G6B5_UNORM? AFAICT, the reason for
>> >> using BGR above, is the _REV suffix on the type...
>> >
>> > No, it's the first line that's "wrong" since the B10G10R10A2 matches
>> > GL_BGRA + GL_UNSIGNED_INT_2_10_10_10_REV, GL_RGB implies that the
>> > incoming data only has 3 channels (no alpha at all).
>>
>> Good point about the alpha channel. It sounds like it should be
>> changed to MESA_FORMAT_B10G10R10X2_UNORM instead.
>>
>> But according to src/mesa/main/format_pack.c's
>> pack_ubyte_b10g10r10a2_unorm(), mesa's B10G10R10A2 corresponds to
>> OpenGL's UNSIGNED_INT_2_10_10_10_REV. So I think it matches GL_RGBA,
>> not GL_BGRA. The latter would mean another swizzle, AFAICT.
>
> The mapping is (_mesa_format_from_format_and_type):
>
>    case GL_UNSIGNED_INT_2_10_10_10_REV:
>       if (format == GL_RGB)
>          return MESA_FORMAT_R10G10B10X2_UNORM;
>       if (format == GL_RGBA)
>          return MESA_FORMAT_R10G10B10A2_UNORM;
>       else if (format == GL_RGBA_INTEGER)
>          return MESA_FORMAT_R10G10B10A2_UINT;
>       else if (format == GL_BGRA)
>          return MESA_FORMAT_B10G10R10A2_UNORM;
>       else if (format == GL_BGRA_INTEGER)
>          return MESA_FORMAT_B10G10R10A2_UINT;
>       break;
>
> The trick is that the packed formats are written as lsb first (or it may
> just be native and my lsb bias is showing).
>
>> > i965 know how to do B5G6R5 and not R5G6B5, but for completeness we could
>> > also add RETURN_IF_SUPPORTED(MESA_FORMAT_R5G6B5_UNORM);
>>
>> I think intel-specific hacks (like preferring B5G6R5 over R5G6B5)
>> shouldn't leak into _mesa_choose_tex_format(),
>
> Hah, did you look at _mesa_choose_tex_format()? :)
> I sent an another patch to do the hardware agnostic unswizzled conversions.
>
>> So I think it'd be a
>> good move to add "RETURN_IF_SUPPORTED(MESA_FORMAT_R5G6B5_UNORM);"
>> before the latter return.
>
> Not quite, because the mapping for 565 is:
>
>    case GL_UNSIGNED_SHORT_5_6_5:
>       if (format == GL_RGB)
>          return MESA_FORMAT_B5G6R5_UNORM;
>       else if (format == GL_BGR)
>          return MESA_FORMAT_R5G6B5_UNORM;
>       else if (format == GL_RGB_INTEGER)
>          return MESA_FORMAT_B5G6R5_UINT;
>       break;

Ahh, you're right. A closer reading of the OpenGL 4.5 spec shows that
I get confused by the fact that RGB + 565 matches the encoding for
MESA_FORMAT_B5G6R5_UNORM, not MESA_FORMAT_R5G6B5_UNORM as would seem
intuitive. But of course, this confusion comes from OpenGL's pretty
confused "backwards" formats, where the first channel is in the most
significant bits, not in the least significant bits as mesa's R5G6B5
is defined.

This patch does indeed seem correct to me. Sorry for the noise!