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

[bugzilla-daemon at freedesktop.org]

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

--- Comment #39 from Iago Toral <itoral at igalia.com> ---
(In reply to Jason Ekstrand from comment #37)
> (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 see. I think the problem here is that we check for pack/unpack fast paths
before we try _mesa_swizzle_and_convert, so when either the source or the
destination are RGBA ubyte/uint/float we will always try the direct pack/unpack
path.

I understand this makes sense since directly packing or unpacking should be
slightly more efficient if we can do it, but if we do not want to have
pack/unpack conversion functions for all types, then we need to make sure that
in some cases we fall back to other paths (maybe _mesa_swizzle_and_convert if
they are array types) instead of asserting. That would be solution 1) below.

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

>From your explanations I think this is what we should do too. For example, when
the source type is RGBA ubyte, we will do something like this:

if (src_array_format.as_uint == RGBA8888_UBYTE.as_uint &&
    !_mesa_is_format_integer_color(dst_format) {
   // Call pack function here
}

instead of:

if (src_array_format.as_uint == RGBA8888_UBYTE.as_uint) {
    assert(!_mesa_is_format_integer_color(dst_format));
   // Call pack function here
}

That way, if dst is an integer type we will fall back to
_mesa_swizzle_and_convert (if both src and dst are array types) or, as last
resort, to the src->rgba->dst conversion path.

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

I think so, thanks for the detailed explanation. I had not considered the
increased size of the library if we auto-generate uint pack/unpack functions
and was wondering why we would not do that. Also, it is true that in most cases
we should be able to handle integer types via swizzle and convert, we just need
to make sure that we fall back to that path.

-- 
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/7f247c3d/attachment.html>