Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for #1601, #1611 and #1629 and possibly #1608 #1633

Merged
merged 1 commit into from
Sep 5, 2019

Conversation

xiphmont
Copy link
Contributor

@xiphmont xiphmont commented Sep 5, 2019

This patch set attempts to both improve cooperation between LRF and
tiling setup, but also eliminate several illegal configurations.

The patch strategy is to consistently eliminate evaluation of
'stretch' superblocks in RDO of LRUs for the loop restoration filter.
This primarily affects the case where a tile consists of
primarily/exclusively SBs that belong to an LRU stretched from some
other tile.

@xiphmont xiphmont changed the title WIP: Fix for #1601, #1611 and #1629 and possible #1608 WIP: Fix for #1601, #1611 and #1629 and possibly #1608 Sep 5, 2019
@coveralls
Copy link
Collaborator

coveralls commented Sep 5, 2019

Coverage Status

Coverage increased (+0.1%) to 71.095% when pulling aa12165 on xiphmont:lrf-tiling-fixes into cc170c2 on xiph:master.

@xiphmont
Copy link
Contributor Author

xiphmont commented Sep 5, 2019

lrf-tiling-baseline@2019-09-05T03:38:41.579Z -> lrf-tiling-fixes@2019-09-05T04:28:25.579Z

PSNR | PSNR Cb | PSNR Cr | PSNR HVS | SSIM | MS SSIM | CIEDE 2000
-0.2642 | 0.1970 | 0.1539 | -0.0703 | -0.3476 | -0.0846 | -0.1653

In fact, the strategy of the fix is suboptimal; Ignoring all stretch in LRF RDO is leaving lots of bits ont he table, especially at 720p. I'll need to differentiate between stretching in the same tile and into another tile after all.

OTOH, correcting stretch handling is offsetting the loss in other ways for now. In the interests of getting the other devs unstuck, I'd like to land this (it is hopefully correct, just not as efficient as it could be) and then handle improvement in another PR.

@xiphmont xiphmont force-pushed the lrf-tiling-fixes branch 3 times, most recently from 2bdc019 to 9a84b92 Compare September 5, 2019 06:33
@xiphmont xiphmont changed the title WIP: Fix for #1601, #1611 and #1629 and possibly #1608 Fix for #1601, #1611 and #1629 and possibly #1608 Sep 5, 2019
@xiphmont
Copy link
Contributor Author

xiphmont commented Sep 5, 2019

Yes, this should land if tests pass. I'll continue refining in the next PR. Reviewers have at.

This patch set attempts to both improve cooperation between LRF and
tiling setup, but also eliminate several illegal configurations.

The patch strategy is to consistently eliminate evaluation of
'stretch' superblocks in RDO of LRUs for the loop restoration filter.
This primarily affects the case where a tile consists of
primarily/exclusively SBs that belong to an LRU stretched from some
other tile.
Copy link
Collaborator

@lu-zero lu-zero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine.

&lrf_input,
pli,
);
// no aelative cost differeneces to different
Copy link
Member

@tmatth tmatth Sep 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: no relative cost differences

None
} else {
let x = (sbo.0.x >> self.rp_cfg.sb_shift) - if x_stretch { 1 } else { 0 };
let y = (sbo.0.y >> self.rp_cfg.sb_shift) - if y_stretch { 1 } else { 0 };
Copy link
Member

@tmatth tmatth Sep 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just {x,y}_stretch as i32 (or another int type) instead of the conditional blocks?

@YaLTeR
Copy link
Collaborator

YaLTeR commented Sep 5, 2019

Found a desync:

#[cfg_attr(feature = "decode_test", interpolate_test(aom, "aom"))]
#[cfg_attr(feature = "decode_test_dav1d", interpolate_test(dav1d, "dav1d"))]
fn fuzz_desync_1(decoder: &str) {
  let w = 11;
  let h = 256;
  let speed = 10;
  let q = 200;
  let limit = 2;
  let min_keyint = 2;
  let max_keyint = 4;
  let low_latency = true;
  let bitrate = 16384;

  let mut dec = get_decoder::<u8>(decoder, w, h);
  dec.encode_decode(
    w,
    h,
    speed,
    q,
    limit,
    8,
    Default::default(),
    min_keyint,
    max_keyint,
    low_latency,
    bitrate,
    1,
    1,
  );
}
---- test_encode_decode::fuzz_desync_1_dav1d stdout ----
Encoding 11x256 speed 10 quantizer 200 bit-depth 8 bitrate 16384
Encoded packet 0
Decoding frame 0
Decoded. -> 0
Retrieving frame
Retrieved.
thread 'test_encode_decode::fuzz_desync_1_dav1d' panicked at 'assertion failed: `(left == right)`

Diff < left / right > :
 [
<    129,
>    121,
     255,
     101,
     0,
     6,
 ]

', src/test_encode_decode/mod.rs:145:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

---- test_encode_decode::fuzz_desync_1_aom stdout ----
Encoding 11x256 speed 10 quantizer 200 bit-depth 8 bitrate 16384
Encoded packet 0
Decoding frame 0
Decoded. -> 0
Retrieving frame
Retrieved.
thread 'test_encode_decode::fuzz_desync_1_aom' panicked at 'assertion failed: `(left == right)`

Diff < left / right > :
 [
<    129,
>    121,
     255,
     101,
     0,
     6,
 ]

', src/test_encode_decode/mod.rs:145:5

On master this causes the debug assert from #1611.

@xiphmont
Copy link
Contributor Author

xiphmont commented Sep 5, 2019

YaLTer: reproduced. I'm on it.

@xiphmont
Copy link
Contributor Author

xiphmont commented Sep 5, 2019

The two desyncs you found appear to have nothing to do with LRF work. The first one that you deleted occurs on master predating all LRF commits. The one above appears to have popped up on master after the last LRF commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants