struct Rav1dFrameContext_lf::cdef_lpf_line: Make ptrs into offsets

[randomPoison]

I'm getting a couple of weird test failures on this, but I'm putting it up in a PR now so that others can review.

[fbossen]

I'm getting a couple of weird test failures on this, but I'm putting it up in a PR now so that others can review.

Could it be that cdef_line_buf is a vector of u8 and that some offsets are meant to be applied to vectors of BD::Pixel which may be 1 or 2 bytes wide?

[randomPoison]

In theory that's handled in the PR. The logic for initializing cdef_lpf_line is in decode.rs, it's been updated to use offsets calculated in pixels, which are then used in bitdepth-dependent code to offset into a pixel array.

Also the errors I'm seeing are actually panics for out of bounds indexing, but it's happening because the slice being indexed into is empty. This happens in backup2lines, which is being passed all of cdef_line_buf for failing slice in question. I didn't actually change anything about how backup2lines is being called in this PR, so I'm not sure why that buffer is now sometimes empty. My guess was that it was related to the mem::take I'm now doing, but I don't see how the function in question can get called while we've moved the field out of the struct🤔

Looks like there might also be a segfault happening? That could be related to byte vs pixel offsets if we're calculating a pointer incorrectly and then passing it to asm.

[fbossen]

Looks like there might also be a segfault happening? That could be related to byte vs pixel offsets if we're calculating a pointer incorrectly and then passing it to asm.

Yes, I am seeing segfaults. However, those seem to happen only when multiple threads are used.

[randomPoison]

Well none of the code I touch in this PR is used when single threaded, I believe. Or at least we only initialize cdef_lpf_line if there is more than one task context, i.e. if we have multiple threads. I was only able to get tests to hit that code when testing with multiple threads.

[kkysen]

Also the errors I'm seeing are actually panics for out of bounds indexing, but it's happening because the slice being indexed into is empty

I didn't look at the code really, but maybe this is because of the mem::take somehow?

[randomPoison]

Yeah that's my current theory, but I can't tell how that's possible. It'd require that rav1d_cdef_brow is called from rav1d_copy_lpf, and I can't see how that could happen.

[kkysen]

Is there concurrent access to the Vec?

[fbossen]

Looks like multiple threads can call filter_sbrow_deblock_rows / rav1d_filter_sbrow_deblock_rows / rav1d_copy_lpf concurrently.

[randomPoison]

I hope there's not concurrent access because we're not doing anything to synchronize access within those functions 😰 @fbossen are concurrent calls also acting on the same data? I was assuming each task had its own data since there's no synchronization happening.

[kkysen]

Maybe the offsets split the Vec into sections where different sections are mutated concurrently, but each section is not mutated concurrently?

[fbossen]

I hope there's not concurrent access because we're not doing anything to synchronize access within those functions 😰 @fbossen are concurrent calls also acting on the same data? I was assuming each task had its own data since there's no synchronization happening.

cdf_line_buf is used as a collection of smaller buffers of size 4 * stride. These smaller buffers can be concurrently written to, but there is no overlap/conflict in the store operations.

[kkysen]

The uses of f in fn backup_lpf that prevent us from &mut f.lf.cdef_line_buf borrowing are:

• frame_hdr:
• dsp: *const Rav1dDSPContext: this is a raw pointer, so we can ignore borrowck issues for now. Eventually, it can be made a global with a OnceCell/AtomicPtr/etc.
• resize_step: [c_int; 2]: Copy
• resize_start: [c_int; 2]: Copy
• f.bitdepth_max: c_int: Copy

Thus, we can change fn backup_lpf to this:

unsafe fn backup_lpf<BD: BitDepth>(
    c: &Rav1dContext,
    frame_hdr: &Rav1dFrameHeader,
    dsp: &Rav1dDSPContext,
    resize_step: [c_int; 2],
    resize_start: [c_int; 2],
    bitdepth_max: c_int,

That should fix the borrowck issues while keeping the Vec in place so that concurrent modifications to different sections of it work. Hopefully that works.

This is a bit unwieldy passing all of the arguments separately, but I think it's fine. It also reduces the number of frame_hdr .unwrap()s, too, as it's already .unwrap()ed in the caller.

[randomPoison]

Looks like passing in the variables separately worked 🎉

[kkysen]

LGTM! Good to merge once #777 is merged (after #777 is rebased on #780).