-
Notifications
You must be signed in to change notification settings - Fork 22
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
struct Rav1dFrameContext_lf::cdef_lpf_line
: Make ptrs into offsets
#779
Conversation
Could it be that |
In theory that's handled in the PR. The logic for initializing 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 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. |
Well none of the code I touch in this PR is used when single threaded, I believe. Or at least we only initialize |
I didn't look at the code really, but maybe this is because of the |
Yeah that's my current theory, but I can't tell how that's possible. It'd require that |
Is there concurrent access to the |
Looks like multiple threads can call |
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. |
Maybe the offsets split the |
|
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 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 aOnceCell
/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.
Rav1dFrameContext_lf::cdef_lpf_line
: Make ptrs into offsetsstruct Rav1dFrameContext_lf::cdef_lpf_line
: Make ptrs into offsets
73689be
to
1d83d68
Compare
e41e8ec
to
eae6239
Compare
Looks like passing in the variables separately worked 🎉 |
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.
1d83d68
to
89acd73
Compare
eae6239
to
e1c517e
Compare
89acd73
to
285a342
Compare
e1c517e
to
325b0dd
Compare
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.