[FFmpeg-devel] [PATCH] Add a gamma flag to exr loader to avoid banding

[Michael Niedermayer]

On Tue, May 06, 2014 at 04:30:16PM -0300, Gonzalo Garramuno wrote:
> On 05/05/14 16:47, Michael Niedermayer wrote:
> >On Mon, May 05, 2014 at 12:52:25PM -0300, Gonzalo Garramuno wrote:
> >>On 05/05/14 12:28, Michael Niedermayer wrote:
> >>>it seems to fail only with --enable-libopencv
> >>That's beyond my knowledge.  So you got the fate tests to pass?
> >its basically the same as before, they pass on some platforms with
> >some configure options but fail on others.
> I replaced the exr_half2float() with one that does not work any
> magic with floating point (it works all in uint).
> Can you give it a try?  Attached is the exr diff and the checksums
> which remain the same.
> 

[...]
> @@ -77,6 +81,7 @@ typedef struct EXRThreadData {
>      uint16_t *lut;
>  } EXRThreadData;
>  
> +
>  typedef struct EXRContext {
>      AVClass *class;
>      AVFrame *picture;

unrelated


> @@ -106,8 +111,83 @@ typedef struct EXRContext {
>      EXRThreadData *thread_data;
>  
>      const char *layer;
> +
> +    float gamma;
> +
> +    uint16_t gamma_table[65536];
> +
>  } EXRContext;
>  

> +// -15 stored using a single precision bias of 127
> +const unsigned int  HALF_FLOAT_MIN_BIASED_EXP_AS_SINGLE_FP_EXP = 0x38000000;
> +// max exponent value in single precision that will be converted
> +// to Inf or Nan when stored as a half-float
> +const unsigned int  HALF_FLOAT_MAX_BIASED_EXP_AS_SINGLE_FP_EXP = 0x47800000;
> +
> +// 255 is the max exponent biased value
> +const unsigned int  FLOAT_MAX_BIASED_EXP = (0xFF << 23);
> +
> +const unsigned int  HALF_FLOAT_MAX_BIASED_EXP = (0x1F << 10);

these should be #define ...


> +
> +/*
> + * Convert a half float as a uint16_t into a full float.
> + *
> + * @param h half float as uint16_t
> + *
> + * @return float value
> + */
> +inline static float

doesnt need to be inline


> +exr_half2float(uint16_t hf)
> +{
> +    unsigned int    sign = (unsigned int)(hf >> 15);
> +    unsigned int    mantissa = (unsigned int)(hf & ((1 << 10) - 1));
> +    unsigned int    exp = (unsigned int)(hf & HALF_FLOAT_MAX_BIASED_EXP);
> +    unsigned int    f;
> +
> +    if (exp == HALF_FLOAT_MAX_BIASED_EXP)
> +    {
> +        // we have a half-float NaN or Inf
> +        // half-float NaNs will be converted to a single precision NaN
> +        // half-float Infs will be converted to a single precision Inf
> +        exp = FLOAT_MAX_BIASED_EXP;
> +        if (mantissa)
> +            mantissa = (1 << 23) - 1;    // set all bits to indicate a NaN
> +    }
> +    else if (exp == 0x0)
> +    {
> +        // convert half-float zero/denorm to single precision value
> +        if (mantissa)
> +        {

    > +           mantissa <<= 1;
> +           exp = HALF_FLOAT_MIN_BIASED_EXP_AS_SINGLE_FP_EXP;
> +           // check for leading 1 in denorm mantissa

> +           while ((mantissa & (1 << 10)) == 0)
> +           {
> +               // for every leading 0, decrement single precision exponent by 1
> +               // and shift half-float mantissa value to the left
> +               mantissa <<= 1;
> +               exp -= (1 << 23);
> +            }
> +            // clamp the mantissa to 10-bits

this could probably be simplified with av_log2(), not sure its worth
teh work considering this isnt speed relevant though

also please keep the formating consistent within a file


> +            mantissa &= ((1 << 10) - 1);
> +            // shift left to generate single-precision mantissa of 23-bits
> +            mantissa <<= 13;
> +        }
> +    }
> +    else
> +    {
> +        // shift left to generate single-precision mantissa of 23-bits
> +        mantissa <<= 13;
> +        // generate single precision biased exponent value
> +        exp = (exp << 13) + HALF_FLOAT_MIN_BIASED_EXP_AS_SINGLE_FP_EXP;
> +    }
> +
> +    f = (sign << 31) | exp | mantissa;
> +

> +    return *((float *)&f);

aliasing violation, undefined behavior
also it would be better if it outputted some kind of int
and float is limited to the pow() codepath


> +}
> +
> +
>  /**
>   * Convert from 32-bit float as uint32_t to uint16_t.
>   *


> @@ -128,6 +208,8 @@ static inline uint16_t exr_flt2uint(uint32_t v)
>      return (v + (1 << 23)) >> (127 + 7 - exp);
>  }
>  
> +
> +
>  /**
>   * Convert from 16-bit float as uint16_t to uint16_t.
>   *

unrelated

and this one seems to pass here for the case that failed previously

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140507/d801d98a/attachment.asc>