[x265] [PATCH] fix PSYVALUE shift overflow, Issue #180 [OUTPUT CHANGE on 12bpp]

[Steve Borho]

On 09/15, chen wrote:
> 
> 
> 
> At 2015-09-15 03:29:02,"Steve Borho" <steve at borho.org> wrote:
> >On 09/14, chen wrote:
> >> 
> >> 
> >> 
> >> At 2015-09-14 12:29:42,"Steve Borho" <steve at borho.org> wrote:
> >> >On 09/11, Min Chen wrote:
> >> >> # HG changeset patch
> >> >> # User Min Chen <chenm003 at 163.com>
> >> >> # Date 1442002697 18000
> >> >> # Node ID f520fd29f3d71d495e08fe96df917489348c377b
> >> >> # Parent  137854992fc614bdd6c446852528e24ed52c9991
> >> >> fix PSYVALUE shift overflow, Issue #180 [OUTPUT CHANGE on 12bpp]
> >> >> ---
> >> >>  source/common/quant.cpp |    2 +-
> >> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >> >> 
> >> >> diff -r 137854992fc6 -r f520fd29f3d7 source/common/quant.cpp
> >> >> --- a/source/common/quant.cpp	Fri Sep 11 15:18:14 2015 -0500
> >> >> +++ b/source/common/quant.cpp	Fri Sep 11 15:18:17 2015 -0500
> >> >> @@ -588,7 +588,7 @@
> >> >>  #define UNQUANT(lvl)    (((lvl) * (unquantScale[blkPos] << per) + unquantRound) >> unquantShift)
> >> >>  #define SIGCOST(bits)   ((lambda2 * (bits)) >> 8)
> >> >>  #define RDCOST(d, bits) ((((int64_t)d * d) << scaleBits) + SIGCOST(bits))
> >> >> -#define PSYVALUE(rec)   ((psyScale * (rec)) >> (2 * transformShift + 1))
> >> >> +#define PSYVALUE(rec)   ((psyScale * (rec)) >> X265_MAX(0, (2 * transformShift + 1)))
> >> >
> >> >this prevents a negative value, but it seems like we want really large
> >> >psy values to be clamped to a large positive value instead of 0.
> >> 
> >> 
> >> I think we need check its logic, and choice better way.
> >> 
> >> 
> >> the psyScale is Q.8 x Q.8 = Q.16, it compare to (X << scaleBits) directly, means shift factor must be (16 - 2T - 1) = (15 - 2T) = scaleBits, it is equal unit
> >> In here, the transformShift is NEGTIVE, the scaleBits will be 15 - 2x(-1) = 17, or call Q.17 format, so we need shift left 1 bit on PSYVALUE.
> >> I test some sequence, the result very nearly, so I just simplest way to limit it to Zero.
> >
> >I see, it clamping the downshift to 0 so it doesn't become a negative
> >shift.
> >
> >If this is only a problem at high bit depths, can we make the macro
> >different for each bit depth so this overhead is avoided for 8bit
> >encodes?
> >
> Since we use C++ template to duplicated rdoQuant, it always constant shift.

but is the max always required?

-- 
Steve Borho