Opened 8 years ago

Closed 7 years ago

#1212 closed enhancement (wontfix)

SAD and HAD called during Inter where pixel values may be out of range, negative or exceed upper limit

Reported by: yetish Owned by: karlsharman
Priority: minor Milestone: HM-16.5
Component: HM Version: HM-12.1
Keywords: Cc: fbossen, ksuehring, davidf, jct-vc@…


Minor Bug

SAD and HAD called during Inter where pixel values may be out of range, negative or exceed upper limit of given bit depth due to TComYuv::removeHighFreq(...)

Within TEncSearch::xMotionEstimation(..), when bBi is enabled, TcomYuv::removeHighFreq(...) is called which attempts to average out the Dst with the Src pix values as follows:

pDst[x ] = (pDst[x ]<<1) - pSrc[x ] ;

However, in not all cases are they similar values, and so it is possible to that a negative values or that exceeding the bit-depth range would be stored.

Previously, in Ticket 175 the issue was raised that clipping was hindering performance and hence the code checks if the flag for disabling clip has been enabled.


The issue with clipping is it only stops if it exceeds the bit-depth range and does not handle if the value is negative.

As a consequence, when SAD or Hadamard Distortion Metrics are called within TcomRdCost, block pairs may contain invalid pixel values, resulting in lower or highter distortion scores including exceeding the maximum for the given bit depth. As such, for those block pairs that are incorrectly scored, this can affect the choices undertaken by the encoder.

Taking observations of when Dst is assigned values that are invalid, test conditions were produced. Src would be assessed against and upper and lower condition in order to decide average out Dst with Src.

The condition are linear equations, adding minimal additional complexity. The lower limit test is y > 2x + 1, where x is Dst and Src is y, the upper condition is y <= 2x – 2BitDepth.

int Dst2 = pDst[x]<<1;
pDst[x ] = pSrc[x ] > (Dst2 + 1) && pSrc[x] <= Dst2-iMaxValBitDepthY ?

Dst2 - pSrc[x ] : pDst[x ];

where iMaxValBitDepthY is defined prior to the loop as:

int iMaxValBitDepthY = 2<<g_bitDepthY;

This can be extended to the chroma parts in a similar fashion.

(Before Loop)

Added to checking pSrc
int iMaxValBitDepthC = 2<<g_bitDepthC;

(Within Loop)

Fix - Ensures Pixel Values are within Range
int DstU2 = pDstU[x]<<1;
int DstV2 = pDstV[x]<<1;

pDstU[x ] = pSrcU[x ] > (DstU2 + 1) && pSrcU[x] <= DstU2-iMaxValBitDepthC ?
DstU2 - pSrcU[x ] : pDstU[x ];

pDstV[x ] = pSrcV[x ] > (DstV2 + 1) && pSrcV[x] <= DstV2-iMaxValBitDepthC ?
DstV2 - pSrcV[x ] : pDstV[x ];

The solution was tested on the first three frames of an 8 bit, 10 bit and 12 bit video, where no subsequent invalid pixel values captured after new Dst value has been assigned within TcomYuv::removeHighFreq(...).

Below is a table of a series of brief tests conducted with the differences between original and that with the modified removeHighFreq() method. A short GOP of 5 was used.

Video Resolution BitDepth Frames Bit-Rate Y-PSNR Time
RaceHorses 416x240 8 30 1.04% -0.0579 0.49%
OldTown 1920x1080 10 20 0.07% -0.0008 -0.48%
Traffic 2560x1600 12 10 0.13% 0.001 -0.24%

Change History (5)

comment:1 Changed 8 years ago by DefaultCC Plugin

  • Cc fbossen ksuehring davidf jct-vc@… added

comment:2 Changed 8 years ago by fbossen

  • Type changed from defect to enhancement

comment:3 Changed 8 years ago by ksuehring

  • Milestone HM-13.0 deleted

comment:4 Changed 7 years ago by ksuehring

  • Milestone set to HM-16.5
  • Owner set to karlsharman
  • Status changed from new to assigned

comment:5 Changed 7 years ago by karlsharman

  • Resolution set to wontfix
  • Status changed from assigned to closed

The JCTVC previously decided that clipping could be
disabled at this point for the benefit of run-time
knowing that clipping was useful. Clipping has not
been removed from the code, just disabled.

This patch proposes to add a modified form of clipping
back into the code. This seems to be opposite to the
previous direction of the JCTVC, and therefore any
such changes should be discussed by the committee.

Note: See TracTickets for help on using tickets.

This list contains all users that will be notified about changes made to this ticket.

These roles will be notified: Reporter, Owner, Subscriber, Participant

  • David Flynn(Subscriber)
  • Frank Bossen(Subscriber, Participant)
  • jct-vc@…(Subscriber)
  • Karl Sharman(Owner, Participant)
  • karl.sharman@…(Always)
  • Karsten Suehring(Subscriber, Participant, Always)
  • Yetish Joshi(Reporter)