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

Well-defined lazy initialization for get_intra_edges #3277

Merged
merged 2 commits into from
Oct 27, 2023

Conversation

barrbrain
Copy link
Collaborator

@barrbrain barrbrain commented Oct 26, 2023

Inspired by #3276. Introduce new types to capture the requirements of intra edge buffers and avoid undefined behavior.
Analysis from @kornelski quoted below:

// left pixels are ordered from bottom to top and right-aligned
let (left, not_left) = edge_buf.data.split_at_mut(2 * MAX_TX_SIZE);
let (top_left, above) = not_left.split_at_mut(1);

Right-aligned pixels suggests that not all of the left pixels are always initialized, so get_intra_edges was returning edge_buf without initializing all of its elements. That's technically UB, and an example of a safety comment being misleading about safety.

I don't think I could simply change edge_buf to be a contiguous slice of pixels (of variable width) or something like a PlaneRegion, because in dispatch_predict_intra there's assembly that seems to rely on edge_buf having a particular layout. It's a bit scary to change edge_buf layout, since code elsewhere has assumptions like let edge_ptr = edge_buf.data.as_ptr().offset(2 * MAX_TX_SIZE as isize) as *const _;.

So if edge_buf can't be contiguous, it can't have uninitialized elements left, so I've just initialized it. AFAIK that's 256 zeroed bytes. I took it out of get_intra_edges to avoid initializing it every time inside the function, so the initialization can be amortized in the outer scope.

I think longer term it could be worth trying to make edge_buf a newtype or a contiguous array, and change assembly to match, but that's a bit more than I can do now.

@barrbrain barrbrain mentioned this pull request Oct 26, 2023
@barrbrain barrbrain force-pushed the intra-edges branch 6 times, most recently from e9a9cdf to bf464ea Compare October 27, 2023 05:59
@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

Attention: 22 lines in your changes are missing coverage. Please review.

Files Coverage Δ
src/api/lookahead.rs 42.68% <100.00%> (+0.46%) ⬆️
src/asm/shared/predict.rs 99.36% <100.00%> (+<0.01%) ⬆️
src/asm/shared/transform/inverse.rs 100.00% <100.00%> (ø)
src/asm/shared/transform/mod.rs 100.00% <100.00%> (ø)
src/asm/x86/predict.rs 98.89% <100.00%> (-0.01%) ⬇️
src/context/block_unit.rs 93.72% <100.00%> (-0.01%) ⬇️
src/encoder.rs 86.92% <100.00%> (-0.01%) ⬇️
src/partition.rs 77.36% <100.00%> (+1.90%) ⬆️
src/predict.rs 94.86% <100.00%> (-0.03%) ⬇️
src/rdo.rs 85.18% <100.00%> (+0.01%) ⬆️
... and 3 more

... and 1 file with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@barrbrain barrbrain changed the title WIP: Well-defined lazy initialization for get_intra_edges Well-defined lazy initialization for get_intra_edges Oct 27, 2023
@barrbrain barrbrain self-assigned this Oct 27, 2023
Equivalent to composing Aligned::new() with MaybeUninit::uninit_array(),
which is currently only available in nightly rustc.
Always write to the top-left so the initialized area is contiguous.

Introduce new types to make a safe rebinding pattern ergonomic:

  let mut edges: IntraEdgeBuffer<T> = Aligned::uninit_array();
  let edges: IntraEdge<T> = get_intra_edges(&mut edges, ...);
  predict_intra(&edges, ...);

IntraEdgeBuffer holds the aligned array for initialization.
IntraEdge holds references to the initialized slices and ensures the
layout required by the intra-prediction assembly.

Support passing pre-initialized data for tests and benchmarks.

Since MaybeUninit::write_slice() is only available in nightly rustc,
use std::mem::transmute() with copy_from_slice() in get_intra_edges().
@barrbrain barrbrain marked this pull request as ready for review October 27, 2023 19:21
@barrbrain barrbrain removed their assignment Oct 27, 2023
@barrbrain barrbrain requested a review from lu-zero October 27, 2023 19:24
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.

Probably fine, I'm debating if it is better to hide the buffer allocation in the function and return (buffer, ref).

@barrbrain barrbrain merged commit efa9f8c into xiph:master Oct 27, 2023
23 of 25 checks passed
@barrbrain barrbrain deleted the intra-edges branch October 27, 2023 23:51
@barrbrain barrbrain mentioned this pull request Oct 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants