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

[Roland Scheidegger]

Am 12.10.2015 um 21:41 schrieb 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).
> 
Albeit I have to say the rounding of the gallium implementation (outside
of clamping to non-infinite) isn't quite ideal neither - round to
nearest, away from zero if I see that right (this doesn't match d3d10
neither, which for float->float conversions requires round toward zero,
but unlike the infinity issue tests are ok with this).
So, I dunno how this should be handled. Maybe we really need two versions...

Roland