intel / tinycbor

Concise Binary Object Representation (CBOR) Library
MIT License
501 stars 187 forks source link

Enhancement: add extra encoding/decoding half precision floating point functions to public API #149

Open phirsov opened 5 years ago

phirsov commented 5 years ago

It's uncommon to operate half precision floating point data on application level, while such format is widely used on transport level to minimize traffic. In such case one can need API for encoding/decoding half-precision floating point data AS more common single or double. Something like this:

CborError cbor_encode_float_as_half_float(CborEncoder *encoder, float value);
CborError cbor_encode_double_as_half_float(CborEncoder *encoder, double value);
CborError cbor_value_get_half_float_as_float(const CborValue *value, float *result);
CborError cbor_value_get_half_float_as_double(const CborValue *value, double *result);
// etc.

There is some related stuff in the private area, and I think, I can make it public.

thiagomacieira commented 5 years ago

Right, half-float is often used for storage only, while all the manipulation is done on single- or double-precision.

I agree on adding those functions, but let's put each pair on a separate .c file from cborparser.c and cborencoder.c. That way, those two remain floating-point-free.

phirsov commented 5 years ago

Partially implemented. Is this OK. or some amendment is needed?

thiagomacieira commented 5 years ago

Yes, it looks very good. I have one overall comment and two very minor. Overall, I think the float API has more value than double. Any system capable of double-precision either already has a library function to convert single to double or the FPU implements it. But there are systems without double support, or at least more costly than float. So let's explore changing the internal function and the API to return float and receive float.

Thanks for the tests, they look nice.

The two minor comments:

  1. Name the two files cborparser_float.c and cborenccoder_float.c please
  2. Add the two files to tst_cpp.
phirsov commented 5 years ago

Look at the improvement made so far. But I'm in doubt now: I have used ldexpf function, which is not accessible in plain old C89 and some antique compilers.

thiagomacieira commented 5 years ago

Let's do bit manipulation directly. I wonder if there's a #define that helps us know that FP is IEEE 754. C++ has that.

phirsov commented 5 years ago

Let's do bit manipulation directly

Do you mean to replace the val = ldexpf(mant + 1024, exp - 25); with val = ((float)mant + 1024) * (float)(1 << (exp - 25)) (NB! in case exp > 25)

phirsov commented 5 years ago
static inline float decode_halff(unsigned short half)
{
    int exp = (half >> 10) & 0x1f;
    int mant = half & 0x3ff;
    float mantf, expf, val;
    if (exp == 0) {
        mantf = mant;
        expf = 1.0f / (1 << 24);
        val = mantf * expf;
    } else if (exp != 31) {
        mantf = mant + 1024.0f;
        expf = exp >= 25 ? 1 << (exp - 25) : 1.0f / (1 << (25 - exp));
        val = mantf * expf;
    } else {
        val = mant == 0 ? INFINITY : NAN;
    }
    return half & 0x8000 ? -val : val;
}
thiagomacieira commented 5 years ago

If that produces proper results, that's fine. I was thinking of converting the uint16_t to uint32_t first, then memcpy from that to a float, instead of doing FP calculations. If we're going to do math, I'd rather use ldexpf, which according to the standard is part of C99.

phirsov commented 5 years ago

So, there are 4 ways with their pro-s and contra-s:

  1. use ldexp: standard-compliant, works on most platforms, but can be expensive;
  2. use ldexpf: needs the C99 compiler, can be less expensive (but still expensive);
  3. use naive math: works without std math lib (yes, sometimes linking without -lm option saves a lot of space), but error-prone and still expensive;
  4. direct bit manipulations: re-shifting mantissa, re-biasing exponent... Error-prone again, cheap for platforms without hardware floating-point math. But what's about platforms with non-IEEE-754 math? Well. we can end with dropping float and double from API at all:
    #ifndef CBOR_NO_PLATFORM_IEEE754_FLOATING_POINT
    CBOR_INLINE_API CborError cbor_value_get_float(CborEncoder *encoder, float *value);
    CBOR_INLINE_API CborError cbor_encode_float(CborEncoder *encoder, float value);
    // ... for half and double
    #else
    // Caveat lector: IEEE754 floating point representation is assumed
    CBOR_INLINE_API CborError cbor_value_get_float(CborEncoder *encoder, uint32_t *value);
    CBOR_INLINE_API CborError cbor_encode_float(CborEncoder *encoder, uint32_t value);
    // ... for half and double
    #endif

What is the preferable way?

thiagomacieira commented 5 years ago

CBOR is defined as carrying IEEE754 binary64, binary32 and binary16 content. So I like your idea.

I don't think we need to provide the uintXX_t version of the APIs for non-IEEE754 systems, though. Let people use cbor_encode_floating_point and cbor_value_get_floating_point (the latter does not exist), which operate on void *.

phirsov commented 5 years ago

OK, but I think, this is the task for separate feature request. As for now, let's roll back to plain old doubles (ldexp, not ldexpf) both for cbor_value_get_half_float_as_float and cbor_value_get_half_float_as_double.

thiagomacieira commented 5 years ago

I don't think we need the ..._as_double API. Conversion from double to float can be done by user code before reaching TinyCBOR API.

phirsov commented 5 years ago

Well, cbor_value_get_half_float_as_float needs a pointer to float, so user should do extra effort. Hope, nobody could try something like:

double x;
/* Oops... I know a guy, who can try it without any doubt */
cbor_value_get_half_float_as_float(cbor_value, (float*)&x);

But in name of API brevity ..._as_double should be eliminated.

thiagomacieira commented 5 years ago

If someone wants to shoot themselves in the foot, who are we to say no? :-)

phirsov commented 5 years ago

Implemented. I'll squash the branch into the single commit before pull request.

thiagomacieira commented 4 years ago

Hello @x448 we have the code to encode and decode half floats. I just didn't want to add an API that uses that.

x448 commented 4 years ago

@thiagomacieira Oops! Deleted. Thanks for letting me know.