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 <matts...@gmail.com> wrote: >>> On Mon, Oct 12, 2015 at 11:12 AM, Rob Clark <robdcl...@gmail.com> wrote: >>>> On Mon, Oct 12, 2015 at 12:47 AM, Jason Ekstrand <ja...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev