Opened 3 years ago
Closed 3 years ago
#636 closed defect (wontfix)
Editorial modifications from JVET-P0626 create reference to undefined value
Reported by: | fbossen | Owned by: | |
---|---|---|---|
Priority: | minor | Milestone: | |
Component: | spec | Version: | VVC D8 vB |
Keywords: | Cc: | ksuehring, bbross, XiangLi, fbossen, jvet@… |
Description
JVET-P0626 modifies the range of values to which equations (8-134) and (8-144) are applied. With this modification, equations (8-138) and (8-148) can reference undefined values.
For example when nTbW = 8, nTbH = 16, refIdx = 0 and intraPredAngle = 16, equation (8-138) may refer to ref[18]. This happens when x = 7, y = 15:
From equation (8-135) iIdx = 16 * 16 >> 5 = 8
In equation (8-138) x + iIdx + i = 7 + 8 + 3 = 18 which is used to index ref[]
However, equation (8-134) defines ref[i] only up to i=17.
The largest index for which ref[] is defined is refW + refIdx + x = 17, where refW = 2 * 8 = 16, refIdx = 0, and x = 1 (was x = 2 before JVET-P0626 modifications)
It is suggested to revert the JVET-P0626 modifications to avoid referencing undefined values (even though they end up being multiplied by 0).
The application of the JVET-P0626 modifications in SW have led to bug report #635. The modifications are thus also proposed to be reverted in SW.
Change history (12)
comment:1 Changed 3 years ago by afilippov
comment:2 follow-up: ↓ 3 Changed 3 years ago by fbossen
This report is intended to be about the spec.
The non-normative change suggested in JVET-P0626 introduces a reference to an undefined value. This undefined value is then multiplied by 0.
The main question here is whether we are happy with the spec having such quirks.
comment:3 in reply to: ↑ 2 Changed 3 years ago by afilippov
Since multiplication by 0 is well defined, to refer to this reference sample (ref[18] in your example) or not is just an implementation question that can be handled in different ways. Thus, to me, the reported issue is irrelevant to the spec as a result of equation (8-138) is always defined and doesn't depend on a value of this reference sample (ref[18] in your example) that may take an arbitrary value which is called "undefined" in your report.
JVET-P0626 is aimed at removing the redundant operations that forced implementers to perform an extra padding operation. If JVET-P0626 is not applied, it's unclear why the reference sample (ref[18] in your example) should be set to p[ −1 + refW ][ −1 − refIdx ], although any value can be used instead. Thus, the idea to define a variable that can take an arbitrary value without any effect on a result is not quite clear to me.
I don't see any spec related issues. The SW bug mentioned at the end of your report is fixed in the merge request https://vcgit.hhi.fraunhofer.de/jvet/VVCSoftware_VTM/merge_requests/1096 with JVET-P0626 inside.
comment:4 Changed 3 years ago by fbossen
We're talking about an editorial issue here. It has no impact on what implementers can and cannot do.
comment:5 Changed 3 years ago by afilippov
We're talking about an editorial issue here.
According to my understanding, there is no editorial issue here:
- In equation (8-138), coefficient value precedes the reference sample value: ∑ fT[ i ]*ref[ x+iIdx+i ]
- Expressions are evaluated from left to right. Hence, fT[i] is evaluated first.
- In equation (8-138), when i=3, f[ 3 ] is zero, since the subsample offset p = (intraPredAngle*(15+1)) % 32 = (256 % 32) = 0.
- When fT[i] is 0, there is no need to refer to ref[ x+iIdx+i ], taking into account a well-known property of multiplication: 0 * x =0. Multiplication operation is defined in "5.2. Arithmetic operators".
- Hence, it is absolutely unnecessary to refer to ref[x + iIdx + i] = ref[7 + 8 + 3] = ref[ 18 ].
If the order of a multiplier and a multiplicand in (8-138) had been opposite, it wouldn't be that crystal clear. But the current spec with JVET-P0626 is absolutely unambiguous.
It has no impact on what implementers can and cannot do.
I am not very sure about that. The VVC spec. draft before incorporating JVET-P0626 prescribed to set ref[ refW + refIdx + Max( 1, nTbW / nTbH ) * refIdx + 2 ] to specific value p[ −1 + refW ][ −1 − refIdx ]. But in fact, it could take any value in contrast to all other reference samples. However, there were no any clarifiction notes in the spec about that. JVET-P0626 removed this unreasonable restriction that could mislead implementers.
comment:6 Changed 3 years ago by fbossen
- Version set to VVC D7 vB
Some operators such as && and || guarantee that the second operand is not evaluated if the first one takes a given value. This is not the case for operators such as &, |, * and /.
What you are describing is fT[ i ] == 0 ? 0 : ft[ i ] * ref[ x + iIdx + i ]
comment:7 Changed 3 years ago by afilippov
I think spec- and implementation-related questions are mixed up again.
Although there is an informative note in Section 5.1 that "the mathematical operators used in this Specification are similar to those used in the C programming language", I guess implementation-related aspects of the C programming language are irrelevant to the VVC spec draft. Since the VVC spec draft clearly states that "only the externally observable output behaviour is required to conform to the specifications of this Recommendation | International Standard", it is enough for the spec to guarantee that output is always specified. Outputs of all the operations and the entire process described in Section 8.4.5.2.12 are well defined and unambiguous since the C programming language standard clearly defines that "the result of the binary * operator is the product of the operands". The output value of 0 * x is 0 for any x. Thus, output behaviour of fT[ i ] * ref[ x + iIdx + i ] is well defined for any value of ref[ x + iIdx + i ] if fT[ i ] == 0. Since the expression fT[ i ] * ref[ x + iIdx + i ] is evaluated from left to right, it is absolutely unnecessary to refer to ref[ 18 ] in your example if the output value of 0 * ref[ x + iIdx + i ] is known without ref[ x + iIdx + i ]. At least, I couldn't find any relevant constraints / requirement in the VVC spec draft that could force a VVC spec reader to evaluate x in the expression 0 * x. Thus, I don't see any issues in the VVC spec draft with JVET-P0626 incorporated. By the way, using the expression (fT[ i ] == 0) ? 0 : fT[ i ] * ref[ x + iIdx + i ] in formulae (8-138) and (8-148) could exclude any misunderstandings if you see any.
On the other hand, the version of the VVC spec draft before incorporating JVET-P0626 restricted implementation freedom as the value of the reference sample ref[ refW + refIdx + ( Max( 1, nTbW / nTbH ) * refIdx + 2 ] was set to p[ −1 + refW ][ −1 − refIdx ] although ref[ refW + refIdx + ( Max( 1, nTbW / nTbH ) * refIdx + 2 ] can actually take an arbitrary value. Undoubtedly, the result of multiplying 0 by ref[ refW + refIdx + ( Max( 1, nTbW / nTbH ) * refIdx + 2 ] is much more obvious for VVC spec readers than the fact that assigning p[ −1 + refW ][ −1 − refIdx ] to the reference sample ref[ refW + refIdx + ( Max( 1, nTbW / nTbH ) * refIdx + 2 ] is actually optional although this operation is prescribed by the spec.
comment:8 Changed 3 years ago by bbross
- Version changed from VVC D7 vB to VVC D7 vC
comment:9 Changed 3 years ago by bbross
- Version changed from VVC D7 vC to VVC D7 vD
comment:10 Changed 3 years ago by bbross
- Version changed from VVC D7 vD to VVC D7 vE
comment:11 Changed 3 years ago by bbross
- Version changed from VVC D7 vE to VVC D8 vB
comment:12 Changed 3 years ago by bbross
- Resolution set to wontfix
- Status changed from new to closed
I guess two different things are mixed up here, namely (1) removal of redundant padding operations and (2) access to uninitialized memory.
In my opinion, the solution in merge request https://vcgit.hhi.fraunhofer.de/jvet/VVCSoftware_VTM/merge_requests/1096 is the simplest and most straightforward one as
Thus, I don't think JVET-P0626 itself introduces any issues with memory access. Ticket #635 is just a software integration bug that could be easily fixed (valgrind check passed) by initializing left and above reference sample buffers before encoding/decoding. Thus, it is suggested to