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

Expose separate rows from PlaneMutSlice #1043

Merged
merged 16 commits into from Feb 28, 2019
Merged

Expose separate rows from PlaneMutSlice #1043

merged 16 commits into from Feb 28, 2019

Conversation

@rom1v
Copy link
Collaborator

@rom1v rom1v commented Feb 27, 2019

This PR is to PlaneMutSlice what #1035 was to PlaneSlice.

It is written over #1040 (so ignore the first commit).

@rom1v rom1v force-pushed the planemutslice branch 3 times, most recently from 4d940ba to 633fe00 Feb 27, 2019
@tdaede
Copy link
Collaborator

@tdaede tdaede commented Feb 27, 2019

Landed the cdef fix, needs one more rebase sorry ;_;

@rom1v rom1v force-pushed the planemutslice branch 3 times, most recently from b05dc55 to db53522 Feb 27, 2019
@coveralls
Copy link

@coveralls coveralls commented Feb 27, 2019

Coverage Status

Coverage increased (+0.7%) to 83.295% when pulling 7ccf959 on rom1v:planemutslice into 924a999 on xiph:master.

src/plane.rs Outdated
// cannot directly return self.ps.row(row) due to lifetime issue
let range = self.ps.slice_range(row);
Some(&self.ps.plane.data[range])
let range = PlaneSlice::slice_range(&self.plane, self.x, self.y);
Copy link
Collaborator

@tdaede tdaede Feb 27, 2019

Would it make more sense for slice_range to be a method on plane?

Copy link
Collaborator Author

@rom1v rom1v Feb 27, 2019

Yes, you're right. Now that it is refactored that way, it is not related to PlaneSlice anymore.

assert!(dst.len() >= (ysize - 1) * out_stride + xsize);
assert!(input.len() >= ((ysize + 3) * in_stride + xsize + 4) as usize);
cdef_filter_block(dst.as_mut_ptr(),
assert!(out_slice.rows_iter().len() >= (8 * by >> ydec) + ysize);
Copy link
Collaborator

@tdaede tdaede Feb 27, 2019

Is there a more direct way to get len() than producing an iterator first?

Copy link
Collaborator Author

@rom1v rom1v Feb 27, 2019

Currently, no, but we can add rows_count() (or similar) on PlaneMutSlice.

I did not do that because it's only used in assert!().

Copy link
Collaborator

@tdaede tdaede Feb 27, 2019

OK, we can skip it.

Copy link
Collaborator

@lu-zero lu-zero Feb 28, 2019

The assert could be a debug_assert!() in case.

Copy link
Collaborator Author

@rom1v rom1v Feb 28, 2019

When should we call assert!() vs debug_assert!() in rav1e?

Here, we call an unsafe code using raw pointers, which is undefined if this assertion fails.

Copy link
Collaborator

@lu-zero lu-zero Feb 28, 2019

Ideally we can move to debug_assert!() all the cases in which:

  • We want to document the behavior, but
  • We have tests to make sure we do not break it
  • The code isn't user-facing. When it is, I'd keep the assert and document when it could panic.

rom1v added 16 commits Feb 28, 2019
Make RowsIter depend on Plane instead of PlaneSlice, so that we can
reuse it for PlaneMutSlice.
Currently, many functions access rectangular regions of planes (spanning
multiple rows) via a primitive slice to the whole plane along with the
stride information.

This strategy is not compatible with tiling, since this borrows the
memory belonging to other tiles.

Like for PlaneSlice, add methods to PlaneMutSlice to access rows
separately.

See <xiph#631 (comment)>.
Implement traits Index and IndexMut to access PlaneMutSlice rows as if
it was a two-dimensional array.

Thay way, we can write:

    plane_slice[8][8] = 1;
    let row = &mut plane_slice[5];
    row[3] = 42;
Add rows_iter() and rows_iter_mut() to iterate over PlaneMutSlice rows.
This will help to use Plane and PlaneSlice in tests.
The function required a multirows slice with stride information, which
will become incompatible with PlaneMutSlice.

It was already unsafe, so just replace the slice parameter by a raw
pointer.
The number of rows to copy in the bottom was incorrect (it used yorigin,
which is the number of rows in the "top padding").
Add unit test to make sure we will not break Plane::pad() when we change
its implementation not to use PlaneMutSlice.
We have access to the whole underlying buffer from there.

This avoids to call methods returning multirows mutable slices from the
PlaneMutslice, so we can then remove these methods.
Remove as_mut_slice() and similar methods from PlaneMutSlice which
expose slices spanning multiple rows.
@rom1v rom1v force-pushed the planemutslice branch from db53522 to 7ccf959 Feb 28, 2019
tdaede
tdaede approved these changes Feb 28, 2019
Copy link
Collaborator

@lu-zero lu-zero left a comment

Let's land it!

@tdaede tdaede merged commit 7ccee27 into xiph:master Feb 28, 2019
3 checks passed
@rom1v rom1v mentioned this pull request Mar 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants