[Mesa-dev] [PATCH 3/6] glsl: move half<->float convertion to util

[Roland Scheidegger]

Am 12.10.2015 um 20:33 schrieb Rob Clark:
> On Mon, Oct 12, 2015 at 2:22 PM, Matt Turner <mattst88 at gmail.com> wrote:
>> On Mon, Oct 12, 2015 at 11:12 AM, Rob Clark <robdclark at gmail.com> wrote:
>>> On Mon, Oct 12, 2015 at 12:47 AM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>>>>>> +/**
>>>>>> + * Convert a 2-byte half float to a 4-byte float.
>>>>>> + * Based on code from:
>>>>>> + * http://www.opengl.org/discussion_boards/ubb/Forum3/HTML/008786.html
>>>>>> + */
>>>>>> +static inline float
>>>>>> +_mesa_half_to_float(GLhalfARB val)
>>>>>> +{
>>>>>> +   return half_to_float(val);
>>>>>> +}
>>>>
>>>> This is kind of ugly. How hard would it really be to just replace the uses
>>>> with the new name?  I don't think its used *that* often.
>>>
>>> hmm, ~20-30 call sites.. not impossible to update them all, but I
>>> think it should be a separate patch and then drop the compat shims
>>> rather than one big patch that changes everything..
>>>
>>> We could also keep the _mesa_half_to_float() name.. but I really
>>> wanted to drop the GLhalfARB and not drag along GL typedefs.  If no
>>> one objects to just replacing GLhalfARB with uint16_t.
>>>
>>> I could go either way.
>>
>> Replacing GLhalfARB with uint16_t seems like a good plan.
> 
> ok, then the most straightforward (least churn) approach seems to be
> to keep the _mesa_ prefix but use uint16_t..
> 
>>>>>> +
>>>>>> +#include <stdint.h>
>>>>>> +
>>>>>> +#ifdef __cplusplus
>>>>>> +extern "C" {
>>>>>> +#endif
>>>>>> +
>>>>>> +uint16_t float_to_half(float val);
>>>>>> +float half_to_float(uint16_t val);
>>>>>
>>>>> I think these functions need to be prefixed with something -- util_*
>>>>> or something or just leave them as _mesa_*.
>>>>
>>>> Unfortunately, until is kind of a grab-bag right now.  Some stuff has a
>>>> util_ prefix, some has kept its _mesa_ or u_ prefix depending on whether it
>>>> was copied from Mesa or gallium, and some (hash_table for example) isn't
>>>> prefixed at all.  Personally, I'd go with either util_ (it is a utility
>>>> library) or just keep _mesa_ (this is still the mesa project after all). I'm
>>>> not going to be too insistent though
>>>
>>> the util_ prefix conflicts w/ u_half.h (which appears to implement
>>> basically the same thing in a simpler way, but maybe not compatible
>>> enough to just switch to the gallium implementation?  Otherwise the
>>> easier approach would be to just move the gallium implementation to
>>> global util directory and use that instead)..  I was going to use
>>> convert_ prefix but that also conflicts..
>>
>> Seems valuable to ultimately remove the Gallium implementation as
>> well. Chad did some really tedious work to improve the functions
>> you're moving to round properly.
> 
> hmm, looks like his change was mostly important for compiler (to match
> what intel hw does).. otoh I guess it shouldn't hurt for gallum (which
> looks to be used mostly for texture/format conversion) and would be
> nice to de-duplicate..

I'd be in favor of dropping this implementation over the gallium one,
but not vice versa. I've expressed my concerns over this implementation
before (it also looks more expensive), mainly rounding up float values
to infinity half floats seems like a bad idea. The gallium
implementation has a comment why I think is a bug, mostly because d3d10
mandates such value getting rounded to MAX_HALF_FLOAT instead, gl not
caring about it but at least recommending the same for converting to
fp11 floats. I doubt hw in general rounds such values to infinity (due
to the afromentioned d3d10 requirement, which is actually tested for).

Roland