-
Notifications
You must be signed in to change notification settings - Fork 259
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
[WIP. DON'T MERGE] Search all intra modes in lookahead #1650
base: master
Are you sure you want to change the base?
Conversation
src/api/mod.rs
Outdated
let intra_cost = intra_mode_set | ||
.iter() | ||
.map(|&luma_mode| { | ||
let mut plane_after_prediction_region = plane_after_prediction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tile and the region seem to stay the same, is there a reason why they are in the iteration instead of out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Borrowck reasons. I need both mutable and immutable access in this closure, and if there's a mutable borrow outside of the closure I can't get the immutable one inside.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The underlying issue is that get_satd
accepts PlaneRegion
and PlaneRegionMut
is not a PlaneRegion
.
src/api/mod.rs
Outdated
height: IMPORTANCE_BLOCK_SIZE, | ||
}); | ||
|
||
get_satd( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our intra pruning during RDO actually uses get_sad, not get_satd. I think satd is actually better, but maybe that could be a cause of the PSNR vs PSNR-HVS difference?
Can you try rerunning this on master, which now uses satd for intra pruning? |
Since its impact on enc time is very small but giving significant gain > 2%, we can consider enabling this PR at speed 2 (or <= 2)? |
1da81b1
to
fe648e4
Compare
It's become worse, actually (unless I rebased something wrong): https://beta.arewecompressedyet.com/?job=speed2-2019-10-04_114925-45585ee&job=all-intra-modes-importance-speed2-2019-10-04_115057-fe648e4 |
Instead of just DC, try all of them and use the minimal intra-cost.
AWCY on
--speed 2
shows good PSNR improvement.Since this seems to significantly shift the importance, the segment ID heuristic needs to be updated. This is what I got so far: AWCY on default speed with the updated heuristic, however, as can be seen by the AWCY on default speed of the updated heuristic vs. segment ID RDO it can be considerably improved. I guess I'll try to do this throughout my remaining time.
cc #1643