Fix desync when using multiple segments #1977
Conversation
src/encoder.rs
Outdated
| ac: &[i16], | ||
| alpha: i16, | ||
| rdo_type: RDOType, | ||
| need_recon_pixel: bool, | ||
| ) -> (bool, ScaledDistortion) { | ||
| let qidx = get_qidx(fi, ts, cw, tx_bo); |
There was a problem hiding this comment.
I have tested your code to check whether the qidx obtained here (before your fix) matches to the qidx passed at line 1129 (after your fix).
Like using either of below:
//debug_assert!(qidx == qidx2); // ==> Does not work well in rust native code level debug in my VS Code, so // using below
if qidx != qidx2 {
let _test = true;
}
Then, I am seeing that it consistently mismatches at plane 1 and 2.
The desync starts from the first 1,1 block position (i.e. MI grid) of luma 4x4 block.
Before your fix, instead of passing qidx directly, "cw.bc.blocks.set_segmentation_idx(tile_bo, bsize, sidx);" is used to set (or rememeber) the seg_id at MI grid, but someho chroma blocks fail to retrieve that info, possibly because of using incorrect offset in block context array.
There was a problem hiding this comment.
The problem is that this didn't use tile_partition_bo, see the commit message for details.
There was a problem hiding this comment.
The problem is that this didn't use tile_partition_bo, see the commit message for details.
As you exactly stated, I realized that using the existing parameter 'tile_partition_bo' to get qidx, i.e. doing: let qidx = get_qidx(fi, ts, cw, tile_partition_bo); fixes it as well.
Because for sub8x8 bsize, tile_partition_bo already always point to the last sub 8x8 partition when chroma block is encoded.
However, I believe you didn't like fetching seg_id from block context and wanted passing qidx (since it is already available from caller functions, write_tx_blocks() and write_tx_tree()).
As in
ycho@a8f10af
| // For example in 4:2:0, four 4x4 luma partitions share one 4x4 chroma block, | ||
| // this block is part of the last 4x4 partition, but its `tx_bo` offset | ||
| // matches the offset of the first 4x4 partition. | ||
| tx_bo: TileBlockOffset, |
There was a problem hiding this comment.
Yes, this is really correct comment! The correct seg_id used by chroma 4x4 block is the seg_id for last luma 4x4.
| ac: &[i16], | ||
| alpha: i16, | ||
| rdo_type: RDOType, | ||
| need_recon_pixel: bool, | ||
| ) -> (bool, ScaledDistortion) { | ||
| let qidx = get_qidx(fi, ts, cw, tile_bo); |
There was a problem hiding this comment.
So, for chroma block with sub8x8 partition, instead of tile_bo it should be the last sub8x8 luma block.
smarter
force-pushed
the
segment-desync-2
branch
2 times, most recently
from
ab85545
to
f787da8
Compare
They both had a parameter named `tile_bo` but used it to mean different things, and confusingly `write_tx_blocks` passed its `tile_bo` parameter as argument to another parameter of `encode_tx_block` called `tile_partition_bo`. Rename `tile_bo` to `tx_bo` in `encode_tx_block` since that's the name used for the value passed to it by `write_tx_blocks`.
The segmentation index is a partition-level flag, but in `encode_tx_block` we were looking it up using the offset of the current transform block. This _almost_ works but can be wrong for chroma blocks because the transform block offset might not be contained in the partition (see added documentation for an example). We could fix this locally in `encode_tx_block` by passing `tile_partition_bo` instead of `tx_bo` to `get_qidx`, but it's simpler to just have `qidx` be a parameter of `encode_tx_block` since the caller already knows what the correct `qidx` is anyway. This commit only reenables segmentation at speed 0 since it heavily impacts speed right now, see FIXME comment. objective-1-fast results at speed 0: PSNR | PSNR HVS | SSIM | MS SSIM | CIEDE 2000 -1.0540 | -1.4265 | -3.8863 | -3.9374 | -4.1611 Looks good, except dark70p hates this for some reason: PSNR | PSNR HVS | SSIM | MS SSIM | CIEDE 2000 6.6386 | 5.6340 | 3.6653 | 3.9391 | 0.0501
smarter
force-pushed
the
segment-desync-2
branch
from
f787da8
to
768d7b2
Compare
No description provided.