Santiago de Luxán Hernández (46b543f9) at 20 Jan 16:26
Ok. Results are identical.
Ok. I'm testing the new changes just to be sure. I have marked the MR as a draft until I get the results, which should be identical.
Santiago de Luxán Hernández (46b543f9) at 19 Jan 16:02
subsumed setMIPFlagISPPass into setBestPredModeDCT2 and removed get...
Yeah, I admit our variable names have not always been the most accurate.
Hi! Yes, I was writing a comment about this exactly at this time in MR !2478 (closed). I repeat here my findings:
After performing a more in-detail analysis, I agree with you that there is a bug in the behavior of the code. This is due to two things:
updateISPStatusFromRelCU
. Thus, the change in Line 6313.In any case, it is a minor problem and the change in the results is negligible (some sequences with +0.01% and other with -0.01%, on average it stays in 0.00%).
Hi, Frank. Sorry for my delayed response.
After performing a more in-detail analysis, I agree with you that there is a bug in the behavior of the code. This is due to two things:
updateISPStatusFromRelCU
.I think your proposed solution does not fix the problem. I have prepared MR !2505 (merged) as an alternative. In any case, it is a minor problem and the change in the results is also negligible (some sequences with +0.01% and other with -0.01%, on average it stays in 0.00%).
Results for AI (bit identical with RA)
Y | U | V | EncT | DecT | |
---|---|---|---|---|---|
Class A1 | 0,00% | 0,05% | -0,03% | 100% | 101% |
Class A2 | 0,00% | 0,03% | 0,00% | 100% | 100% |
Class B | 0,00% | 0,01% | 0,02% | 100% | 101% |
Class C | 0,00% | 0,02% | 0,04% | 100% | 100% |
Class E | 0,00% | -0,01% | 0,23% | 100% | 100% |
Overall | 0,00% | 0,02% | 0,05% | 100% | 101% |
Class D | -0,01% | -0,05% | 0,20% | 100% | 102% |
Class F | -0,02% | -0,09% | -0,02% | 100% | 102% |
As reported in MR !2478, there is a minor bug in a section of the ISP encoder related to the updateISPStatusFromRelCU
function. Although it does not have a significant impact, this MR solves it. In particular:
updateISPStatusFromRelCU
ispPredModeVal.bestPredModeDCT2
if the a MIP flag value of 1 was stored in the relCU.The changes in the results are negligible.
Hi, Frank. Thanks for the efforts. I have two questions:
updateISPStatusFromRelCU
, which, unless I'm mistaken, it's the only function where bestPredModeDCT2
is read and used. The 4 possibilities are the following:a) relCU uses MIP and currCU uses MIP
b) relCU uses MIP and currCU does not use MIP
c) relCU does not use MIP and currCU uses MIP
d) relCU does not use MIP and currCU does not use MIP
At the beginning of updateISPStatusFromRelCU
we have the following code:
const int relatedCuIntraMode = ispPredModeVal.bestPredModeDCT2;
double bestNonISPCostRelCU = m_modeCtrl->getBestDCT2NonISPCostRelCU();
double costRatio = bestNonISPCostCurrCu / bestNonISPCostRelCU;
bool bestModeCurrCuIsMip = bestNonISPModeCurrCu.mipFlg;
bool isSameTypeOfMode = bestModeRelCuIsMip == bestModeCurrCuIsMip;
bool bothModesAreAngular = bestNonISPModeCurrCu.modeId > DC_IDX && relatedCuIntraMode > DC_IDX;
bool modesAreComparable =
isSameTypeOfMode
&& (bestModeCurrCuIsMip || bestNonISPModeCurrCu.modeId == relatedCuIntraMode
|| (bothModesAreAngular && abs(relatedCuIntraMode - (int) bestNonISPModeCurrCu.modeId) <= 5));
bestPredModeDCT2
is inside the variable relatedCuIntraMode
, which is only utilized to calculate bothModesAreAngular
and modesAreComparable
. So, for b) and c) the variable isSameTypeOfMode
is false, which implies that modesAreComparable
is false regardless the value of relatedCuIntraMode
. Hence, updateISPStatusFromRelCU
will return false directly and bestPredModeDCT2
is inconsequential.
Then, for d), the introduced change in the condition is not used and therefore relatedCuIntraMode
keeps the same value as without the introduced change.
Finally, for a), isSameTypeOfMode
will be true, so the condition
&& (bestModeCurrCuIsMip || bestNonISPModeCurrCu.modeId == relatedCuIntraMode
|| (bothModesAreAngular && abs(relatedCuIntraMode - (int) bestNonISPModeCurrCu.modeId) <= 5))
will be evaluated. However, since bestModeCurrCuIsMip
is true, the rest of the condition is ignored. Hence, in this case as well there is no change. What do you think?
Ok. You're right.
Thanks.
Ok. Thank you for the change.
Thanks. I would like to suggest having the array named numSubPartsBestModeAltSplit
instead of numSubPartsBestModeAlt
. Otherwise it's not straightforward what the suffix "Alt" implies.
Thanks for the change.
Yes, you are right. After checking it, I see that all these other cases you are mentioning are variables defined in IntraSearch.h. In that case it is only necessary to consider the HOR and VER cases. Hence, the value NUM=2 is the correct one for those situations. How about we add a comment next to the NUM definition to avoid any future problems like with the one with the tracing file? For example:
enum class ISPType : int8_t
{
NONE = -1,
HOR = 0,
VER,
NUM, //In order to count the NONE case as well, use the static inline function to_uint(ISPType t)
RESERVED
};
Ok. It seems that was the only place where ISPType::NUM
is applied. The other code lines where NUM_INTRA_SUBPARTITIONS_MODES
was used have (for loops) been changed to something like this
const auto ispMode: { ISPType::HOR, ISPType::VER }
.
Yeah, I think now it's clearer. Thanks for the change.
For consistency purposes: Since all these elements now appear with "ISPType::" in front of them, "INTRA_SUBPARTITIONS_RESERVED" could also be shortened to "RESERVED".