Fix for Ticket #311 Scaling List Bugs

#311

• Wrong scaling list sizes used. -> bug fix by changing uiLog2TrWidth-1 to uiLog2TrWidth, and uiLog2TrHeight-1 to uiLog2TrHeight
• Scaling factor incorrect for dependent quantization. -> bug fix by removing to change shift value
• 32-bit variable overflow in dependent quantization. -> it may not be happened in my understanding, because qIdx and invQScale are well clipped in a predefined range "qIdx" is related to "TransCoeffLevel" in VVC working draft

It is a requirement of bitstream conformance that the value of dec_abs_level[ n ] shall be constrained such that the corresponding value of TransCoeffLevel[ x0 ][ y0 ][ cIdx ][ xC ][ yC ] is in the range of CoeffMin to CoeffMax, inclusive.

The variables CoeffMin and CoeffMax specifying the minimum and maximum transform coefficient values are derived as follows: CoeffMin = −( 1 << 15 ) (7 117) CoeffMax = ( 1 << 15 ) − 1 (7 118)

and invQScale is a 8-bits integer

nextCoef = ( nextCoef + scaling_list_delta_coef + 256 ) % 256

so the result "nomTCoeff" won't be overflow. Could you provide an example for the overflow case?

• Transform skip only applies to luma. -> bug fix by modifying useTransformSkip to useTransformSkip = tu.mtsIdx==MTS_SKIP && isLuma(compID);
• Pred matrix ID delta adjusted for non-existent 2x2 luma lists. -> we have a encoder normative constraint defined in the proposed text

If sizeId is equal to 1, the value of refMatrixId shall not be equal to 0 or 3. Otherwise, iIf sizeId is less than or equal to 5, the value of scaling_list_pred_matrix_id_delta[ sizeId ][ matrixId ] shall be in the range of 0 to matrixId, inclusive. Otherwise (sizeId is equal to 6), the value of scaling_list_pred_matrix_id_delta[ sizeId ][ matrixId ] shall be in the range of 0 to matrixId / 3, inclusive.

mentioned in merge request !594 (closed)

gcc fails with

/home/bboe201/jenkins/workspace/VVCSoftware_VTM_mr/source/Lib/CommonLib/Slice.cpp: In member function ‘void ScalingList::checkPredMode(uint32_t, uint32_t)’:
/home/bboe201/jenkins/workspace/VVCSoftware_VTM_mr/source/Lib/CommonLib/Slice.cpp:2241:120: error: suggest parentheses around ‘&&’ within ‘||’ [-Werror=parentheses]
     if ((sizeId == SCALING_LIST_64x64 && ((listId % 3) != 0 || (predListIdx % 3) != 0)) || (sizeId == SCALING_LIST_2x2 && ((listId % 3) == 0) || (predListIdx % 3) == 0))
                                                                                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors

added 1 commit

• 3dec60c7 - fix gcc fail error

Compare with previous version

1. I believe you found all the same changes I found, so that looks fine.
2. Looks fine to me.
3. See example below.
4. My merge request had one additional change for this at QuantRDOQ.cpp, Line 579. If you agree, you might as well fix that one here as well.
5. Ok... I don't necessarily agree, but if that's the way you proposed it and you don't want to change it, then that is fine for now. The behavior seems inconsistent with the handling of the missing chroma lists at the 64x64 level (e.g. 64x64 luma deltas just skip over the missing chroma positions, different from how you propose these 2x2 chroma deltas). And it wastes bits to do it this way (a 2x2 list delta of "2" can point to a non-existent luma list so you have to use the value "3" instead and waste bits). If you need me to submit a contribution at the next meeting to remove this inefficiency, then I can do that.

Example of overflow for 3:

I believe there are worse examples, but here's the first one I hit with my testing. Let me know if you see anything invalid about the example.

4x4 TU
QP  = 53
levelScale = 40
scalingfactor = 255

invQScale = 40 x 255 = 10200
shift = -3  which increases invQScale to 81600

level = -13662     
qIdx  = -27324   valid coefficient in the range of CoeffMin to CoeffMax
	

nomTCoeff = ( qIdx * invQScale + add ) >> ( (shift < 0) ? 0 : shift )
nomTCoeff = qIdx * invQScale = -2229638400

This exceeds the 32-bit signed range -2147483648 to 2147483647

The value wraps around given the limited range of nomTCoeff
nomTCoeff = 2065328896 

The final result is a positive dequant coefficient (32767), completely opposite of correct result (-32768).

Regarding to 3

• I see, from your example, it truly will cause overflow bug in this case. But it won't be hit in CTC, right? If i use 10-bits samples to do the dequantization, the shift number should be -1 (in your example, shift number is -3), or i am misunderstanding something. I agree that the method you proposed to further type-cast qIdx and invQScale into (int64_t) can solve this problem.

Regarding to 5

• I agree that your method can save some bits, but with this modification, the spec will become more complicated. We need to add some if, else condition, to tell decoder how to deal with different situations. However, in the original design, the decoder still won't crash in 2x2 chroma scaling matrices case, because we have a normative encoder constraint to make sure that 2x2 chroma scaling matrices won't refer 2x2 luma scaling matrices. And we are ok, if you want to submit a new proposal to do this modification.

Regarding to 3

Yes, my example used bitdepth of 8. The software still needs to work correctly for 8-bit encoding.

I don't think scaling lists are enabled in CTC. So, you are correct, this has no impact there.

Correct, I also don't believe it's possible to overflow with 10-bit bitdepth.

You are also extremely unlikely to hit these extreme cases in normal situations. But the software still needs to handle corner cases correctly.

Thanks, Brian

Thanks for the bug fixes. There seem different ways to fix issues. May we merge the agreed items and bring proposals on uncommon/normative parts for the next meeting?

Hi Xiang, Chen-Yen,

Yes, I'm OK to use Chen-Yen's fixes.

For #3, I would suggest fixing the overflow somehow. If you are OK with my fix, Chen-Yen can you add it to your merge request?

I mentioned the additional fix for #4 at QuantRDOQ.cpp, Line 579. If you agree, Chen-Yen can you include that fix in your merge request too?

I'm OK to not change anything for #5.

Thanks, Brian

added 1 commit

• cc58e563 - related to ticket #311 bugfix

Compare with previous version

Please don't use the shared user for comments

Hi, Xiang, Brian, Karsten

As discussed, i have committed a new version code to fix #3 (overflow issue), and #4 (bug in QuantRDOQ.cpp)

Sorry, i accidentally used the shared user for the above comments

added 97 commits

• cc58e563...ca678b5b - 96 commits from branch jvet:master
• 0a4c160c - bugfix for ticket #311

Compare with previous version

merged

mentioned in commit 65a4a484

mentioned in merge request !838 (merged)