src/cdef_apply.rs: cleanup
#733
Conversation
| current_block_84 = 17075014677070940716; | ||
| st_y = false; | ||
| } else { | ||
| current_block_84 = 17728966195399430138; | ||
| st_y = true; |
There was a problem hiding this comment.
the issue #700 mentions "cleaning up the state machine control flow". Is this sufficient?
It's not quite the goto that C inserts but I'm assuming that this results in a direct branch in practice.
There was a problem hiding this comment.
As long as the current_block_* ugliness is gone and it's semantically equivalent to the C, it should be good. Let me check more closely.
There was a problem hiding this comment.
ee538a8 does exactly that. None of the other commits touch the control flow.
There was a problem hiding this comment.
So ee538a8 looks like good itself; it translates the state machines as they were transpiled quite nicely. The original gotos in the C source are different, though, and I think some of them got translated into extra, large ifs and stuff like that. Ideally I think we want to match the C as reasonably as possible, though your way is a very clean solution, too. So for this PR, it's definitely good, a significant improvement. We should look into matching the C gotos structure as much as possible, though. It doesn't have to match completely, but following its general structure is good, as it helps a lot with any backports.
| let f: *mut Rav1dFrameContext = (*tc).f as *mut Rav1dFrameContext; | ||
| let bitdepth_min_8 = if 16 == 8 { | ||
| 0 as c_int | ||
| let f: *mut Rav1dFrameContext = tc.f as *mut Rav1dFrameContext; |
There was a problem hiding this comment.
I didn't really want to add more to this PR but I think at this point this could be a proper reference. #728 gets its hands on a &Rav1dFrameContext
Yeah, stacked PRs work great! I've been using them a lot. As long as the stack doesn't grow super long or branch a lot (a linear stack is usually easier for me). |
kkysen
left a comment
kkysen
left a comment
There was a problem hiding this comment.
Reviewed most of it, but I'm still checking some more things, like the goto state machine.
| current_block_84 = 17075014677070940716; | ||
| st_y = false; | ||
| } else { | ||
| current_block_84 = 17728966195399430138; | ||
| st_y = true; |
There was a problem hiding this comment.
As long as the current_block_* ugliness is gone and it's semantically equivalent to the C, it should be good. Let me check more closely.
folkertdev
force-pushed
the
cdef-apply-cleanup
branch
from
5b1f305 to
bf9a325
Compare
Co-authored-by: Khyber Sen <kkysen@gmail.com>
folkertdev
force-pushed
the
cdef-apply-cleanup
branch
from
c16440b to
f9118b7
Compare
| slice::from_raw_parts_mut( | ||
| *dst.offset(y_stride as isize) as *mut BD::Pixel, | ||
| (-2 * y_stride) as usize, | ||
| ), | ||
| slice::from_raw_parts( | ||
| *src.offset((7 as isize * y_stride) as isize) as *const BD::Pixel, | ||
| (-2 * y_stride) as usize, | ||
| ), | ||
| (-2 * y_stride) as usize, | ||
| slice::from_raw_parts_mut(dst[0].offset(y_stride), len), | ||
| slice::from_raw_parts(src[0].offset(7 * y_stride), len), |
There was a problem hiding this comment.
I think these (other similar ones, too) were mistakenly mistranslated before, as src and dst are offset first and then dereferened, vs. dereferenced/[0]ed first, and then offset. Your way looks correct so all is good here. I'll add to the PR description that this bug is fixed as well. And thanks for catching and fixing this.
kkysen
left a comment
kkysen
left a comment
There was a problem hiding this comment.
This basically all LGTM now besides dst should be an array in fn backup2lines and one other suggestion (though not too important).
Co-authored-by: Khyber Sen <kkysen@gmail.com>
Besides cleanup, this also fixes bugs in
slice::from_raw_parts(_mut)calls inBD::pixel_copys introduced during bitdepth deduplication.can be reviewed commit-by-commit.
these PRs get pretty large and have many unrelated changes. Would stacked PRs easier to review?
I'll leave some specific notes/questions in inline comments.