after considering your valuable comment, I fix the uiCostHor and uiCostVer to be MAX and remove uiCostHor = UINT64_MAX;
and uiCostVer = UINT64_MAX;
in the below condition checks
I simply report the simulation result of this fix on AI-CTC. anchor is commit 3892d194 and test is commit 3892d194 with MR285.
@fan do you achieve simulation result more?
--------- | sequence | |||||
---|---|---|---|---|---|---|
Class C | BasketballDrill | -0.03% | 0.11% | -0.01% | ||
BQMall | 0.02% | -0.14% | -0.25% | |||
PartyScene | 0.00% | -0.07% | 0.12% | |||
RaceHorses | 0.01% | -0.12% | -0.19% | |||
Class D | BasketballPass | 0.01% | 0.04% | -0.05% | ||
BQSquare | 0.01% | -0.04% | 0.27% | |||
BlowingBubbles | 0.02% | 0.03% | -0.16% | |||
RaceHorses | 0.02% | 0.01% | 0.08% | |||
ClassE | FourPeople | -0.01% | 0.07% | -0.15% | ||
Johnny | -0.02% | -0.16% | -0.03% | |||
KristenAndSara | 0.02% | 0.06% | -0.05% | |||
ClassF | BasketballDrillText | -0.01% | 0.15% | 0.16% | ||
ArenaOfValor | #VALUE! | #VALUE! | #VALUE! | |||
SlideEditing | -0.02% | 0.02% | 0.12% | |||
SlideShow | -0.18% | -0.02% | -0.57% |
average is
Y | U | V | |||
---|---|---|---|---|---|
Class C | 0.00% | -0.06% | -0.08% | ||
Class D | 0.02% | 0.01% | 0.03% | ||
Class E | 0.00% | -0.01% | -0.08% |
this fix is not about decoding-crach or mismatch. if experts didn't want to fix this I will just close.
after removing uiCostVer += tmpCost0; I think uiCostVer and uiCostHor is not needed to be initialized as 0. however, doesn't it seems better that initilizing to be 0 than -1 for unsigned integer variable?.
on second suggestion such as
uint64_t tmpCost0 = UINT64_MAX; uint64_t tmpCost1 = UINT64_MAX;
with removing uiCostHor = UINT64_MAX;
and uiCostVer = UINT64_MAX;
in the below seems have problem in my thought. would you please check below comment.
regarding your suggestion,
I think that initializing the tmpCost0 and tmpCost1 to be MAX can have impact on regular TIMD process. as you can see this, the value of the uiCost is derived as summation of tmpCost0 and tmpCost1. I might be wrong but if tmpCost0 or tmpCost1 is MAX and template is partially available then the cost seems incorrect.
Thanks @fan
as I mentioned, I only did simple simulation. after fan's feedback, I also simulate AI-CTC and it is confirmed that performance difference is observed.
I think this change follows same intension of the original implementation but some corner case seems happened. I will report the simulation result.
@fan, Sure, @karamnaser do you have time to look at this?
@seregin in the simple test, it has no impact and it is observed that bitstreams achieved without this fix are decodable. but potentially if the cost is exactly 0, it could be impact.
@fan would you please look at this change?
for TIMD derivation process of SGPM,
Variable uiCostHor/uiCostVer which has uint64_t type is initialized to be -1. and the vaule of these variables are accumulated with template cost after that.
Hence, if template cost is exactly 0, the cost is still set to be MAX-1. it seems unusual.
BTW, it is observed that RPR simulation with latest modification of this MR run successfully in simple test on Class D. @Kenneth I also agree on your comment.
I think it is ready to be merged.
Thanks a lot!
I have missed DMVD off cases. after considering your explanationm, I also agree RPR handling is still needed in the low-level for DMVD off case. I think your latest suggestion is clear.
regarding the encoder-side, personally I also agree it is not needed to fix but I am not sure there is not further problem caused by not-aligned condition between encoder and decoder.
isn't it safe that encoder is aligned with decoder? either way is OK to me.
can you confirm the modified the MR. I changed the code both for encoder and decoder. If you are sure any problem is not expected without encoder change. I will remove the encoder changes on the MR.
Macro is removed.