Opened 9 years ago

Closed 9 years ago

#560 closed defect (fixed)

Intra transform skipping semantic issue with high bit-depth

Reported by: pandrivon Owned by: bbross
Priority: minor Milestone: D7
Component: Text Version: D7 (I1003) d2
Keywords: Cc: bbross, wjhan, jct-vc@…

Description

As of Draft 7 (d2),
"
-If transSkipFlag is equal to 1, the following applies.
1.The variable shift is derived as follows:
–If cIdx is equal to 0,
shift = 13 – BitDepthY (8 269)
–Otherwise,
shift = 13 – BitDepthC
"
However, current Draft 7 states that BitDepthY/C could be set up to 14 begetting potential negative shift definition.

Attachments (3)

transform_skip.patch (3.0 KB) - added by cuilinglan 9 years ago.
The modification for section 8.6.4.doc (27.5 KB) - added by cuilinglan 9 years ago.
modification over spec and software
transform_skip+transformshift_overflow.patch (4.6 KB) - added by pandrivon 9 years ago.
transform skip patch + overflow transformShift fix + typo fix

Download all attachments as: .zip

Change History (13)

comment:1 Changed 9 years ago by DefaultCC Plugin

  • Cc bbross wjhan jct-vc@… added

comment:2 Changed 9 years ago by bbross

  • Milestone set to D7

In HM7.0, I found the following:
Void TComTrQuant::xTransformSkip( Pel* piBlkResi, UInt uiStride, Int* psCoeff, Int width, Int height )
{

assert( width == height );
UInt uiLog2TrSize = g_aucConvertToBit[ width ] + 2;

#if FULL_NBIT

UInt uiBitDepth = g_uiBitDepth;

#else

UInt uiBitDepth = g_uiBitDepth + g_uiBitIncrement;

#endif

UInt iTransformShift = MAX_TR_DYNAMIC_RANGE - uiBitDepth - uiLog2TrSize;
Int j,k;
for (j = 0; j < height; j++)
{

for(k = 0; k < width; k ++)
{

psCoeff[j*height + k] = piBlkResi[j * uiStride + k] << iTransformShift;

}

}

}

Where MAX_TR_DYNAMIC_RANGE is defined as 15 and uiLog2TrSize is always equal to 2 since transform skip can only be applied to 4x4 intra blocks.
15-2=13 and consequently HM and draft text match.

The question is, how does this goes together with the restriction in bit_depth_luma/chroma_minus8 semantics?
"bit_depth_luma/chroma_minus8 shall be in the range of 0 to 6, inclusive."

Changed 9 years ago by cuilinglan

Changed 9 years ago by cuilinglan

modification over spec and software

comment:3 Changed 9 years ago by cuilinglan

Thanks for pointing out this problem. We have modified the spec and the corresponding code as in the attachments.
The negative shift value is allowed by simply modifying the direction of shifting.

comment:4 Changed 9 years ago by pandrivon

Thanks for both patches (Draft and HM).
Btw, the problem could be broadened like mentioned Benjamin as :
iTransformShift = MAX_TR_DYNAMIC_RANGE - uiBitDepth - uiLog2TrSize;
and there are plenty of combinations where iTransformShift can be negative when uiBitDepth>10. Please note that we get a misnamed variable that is Unsigned Integer UInt iTransformShift in xQuant(), xDequant(),(no more issue with the provided patch in xTransformSkip(), xITransformSkip()) but we have an Integer Int iTransformShift in xRateDistOptQuant() and setErrScaleCoeff() thus wrapping errors may occur with negative values in xQuant(), xDequant().
Please correct me if I'm wrong, but it seems that this negative value if UInt iTransformShift is converted to Int (suggestion) in xQuant() and xDeQuant() is not a big deal as derived values used for shifting are always positive though (like min(iQBits)=10 or min(iQBitsC)=3 or min(qBits8)=2, note that there is no problem with iShift in xDeQuant()).
Besides the patch fixes the described Transform Skip issue too.
Any comment and correction is welcome.

comment:5 Changed 9 years ago by bbross

  • Component changed from Text to HM
  • Milestone changed from D7 to HM-7.1
  • Version changed from D7 (I1003) d2 to HM-7.0

Text issue is fixed in draft 7 (JCVTC-I1003_d3.doc)

comment:6 Changed 9 years ago by fbossen

Shouldn't there be a rounding offset (0.5) added for the case where a right shift occurs? This would be more consistent with the inverse transform process. Adding the offset would make the transform bypass process equivalent to using a transform equal to
128 * [ 1 0 0 0; 0 1 0 0; 0 0 1 0; 0 0 0 1]

Changed 9 years ago by pandrivon

transform skip patch + overflow transformShift fix + typo fix

comment:7 Changed 9 years ago by pandrivon

I think that you are right Frank.
I have just posted a new patch with:
+ previous transform_skip.patch
+ your suggestion
+ my suggestion (UInt iTransformShift --> Int iTransformShift for xQuant/xDequant)
+ a typo fox
over HM7.1rc1

comment:8 Changed 9 years ago by fbossen

  • Component changed from HM to Text
  • Milestone changed from HM-7.1 to D7
  • Version changed from HM-7.0 to D7 (I1003) d2

SW fixed in r2483

comment:9 Changed 9 years ago by pandrivon

As both D7 and SW are now fixed, I think that all points addressed by this ticket are fixed thus ticket should be closed.

comment:10 Changed 9 years ago by bbross

  • Resolution set to fixed
  • Status changed from new to closed

Correct. Text issue is fixed in draft 7 (JCVTC-I1003_d3.doc)

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

  • Benjamin Bross(Owner, Subscriber, Participant)
  • Frank Bossen(Participant)
  • jct-vc@…(Subscriber)
  • karl.sharman@…(Always)
  • Karsten Suehring(Always)
  • Pierre Andrivon(Reporter, Participant)
  • Woo-Jin Han(Subscriber)