[Mesa-dev] [Bug 84566] Unify the format conversion code

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=84566

--- Comment #37 from Jason Ekstrand <jason at jlekstrand.net> ---
(In reply to Iago Toral from comment #35)
> Jason, you wrote some fast paths in _mesa_format_convert for the cases where
> we can directly pack or unpack to the destination format. In these paths, if
> we have to pack from or unpack to RGBA8888 UINT, you have asserts to make
> sure that we are packing to or unpacking from an integer type. Likewise, if
> we are packing from or unpacking to RGBA8888 UBYTE, you added asserts to
> make sure that we are packing to or unpacking from a non-integer type.
> 
> I am not sure why these were added, but I think we should remove them since
> there are valid conversions that will hit these paths. For example,
> glReadPixels where we want to store the pixel data in
> GL_RGBA/GL_UNSIGNED_INT. In this case, the destination format matches one of
> these fast path (RGBA UINT) but the source format, which is the render
> buffer's format, is MESA_FORMAT_B8G8R8A8_UNORM which is not integer.

The reason for this is that the original set of [un]packing functions only
included functions for [un]packing integer types to/from uint and [un]packing
normalized types to/from ubyte.  Any normalized type can (in theory) be
converted to a float.  The ubyte versions existed to speed things up when the
format was small enough that [un]packing to/from ubyte wouldn't lose any
precision.  In theory, we could also do [un]packing to/from 16 and 32-bit
normalized types and that would be slightly faster than going through float.  I
didn't do this for a couple of reasons

1) Even though autogenerated functions are cheap in the sense that they require
almost no effort to code, they do increase the size of the library.  If every
format had 4 packing and 4 unpacking functions, that makes for a fairly large
amount of code.

2) Most packed unorm formats have fewer than 8 bits per pixel.  The only
exception I can think of off hand is 10-10-10-2.  Therefore, most unorm
conversions can either go through ubyte, will hit the swizzle_and_convert path,
or will have to go through float anyway.  I don't think we really gain much by
having them go through uint

3) For integer types, GL only allows conversion to/from float and to/from other
integer (not unorm) types.  Since all integer types can fit in 32 bits, uint32
is a natural intermediate format (signed integers get sign-extended).

4) Even moreso than with unorm, the vast majority of integer conversions will
go through mesa_swizzle_and_convert.  I belive the only packed integer format
is 10-10-10-2.  We need to have the unpacking functions for the odd conversion
case, but most won't hit it so also allowing packing to/from lower numbers of
bits doesn't gain us much.

> I think we have at least two options:
> 
> 1) Transform the asserts into conditionals. With these we make sure that
> these fast paths are only used when the assertions you have are not
> violated, but we still let _mesa_format_convert use the non-fast paths to
> resolve the conversion in the cases that would hit the assertions. If there
> was a good reason for these assertions to exist then I guess this is the way
> to go.

I think this is the one we want, but I'm not 100% sure I understand what you
mean.

> 2) We simply remove the asserts. I think pack/unpack functions can deal with
> any format type, right? At the moment uint pack/unpack functions seem to be
> an exception since they only support packing to and unpacking from integer
> types, but since these functions are auto-generated it seems we could just
> let them handle other types too. Since in this case we would enable the fast
> path, it would be the most efficient solution.
> 
> What do you think?

Given what I said above, does it make more sense now?  It's quite possible that
I've missed something, but that explains my reasoning.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141024/39642b11/attachment-0001.html>