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 PlaneSlice #1035

Merged
merged 15 commits into from Feb 27, 2019

Conversation

Projects
None yet
4 participants
@rom1v
Copy link
Collaborator

commented Feb 23, 2019

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.

This is basically what I explained in #631 (comment).

Therefore, only expose separate rows in PlaneSlice.

Concretely, here are the changes from the caller point of view:

// get a specific row

// before
let row = plane_slice.as_slice()[y * stride..(y + 1) * stride];
// or if we don't care about the end
let row = plane_slice.as_slice()[y * stride..];

// after
let row = &plane_slice[y];
// get a component value

// before
let value = plane_slice.as_slice()[y * stride + x];

// after
let value = plane_slice[y][x];
// iterate over rows

// before
for row in plane_slice.as_slice().chunks(stride).take(height) {
    let _first_four_values = &row[..4];
}

// after
for row in plane_slice.rows_iter() {
    let _first_four_values = &row[..4];
}

Note that in itself, it does not solve the incompatibility with tiles (PlaneSlice itself still borrows the whole plane primitive slice). This is only the first step to remove all usages of raw slice + stride information, so that we can then replace PlaneSlice by a tile-specific "plane region" structure which does not borrow memory outside the tile (cf #821).

For now, I only submit changes for PlaneSlice. I will submit similar changes for PlaneMutSlice once I get feedbacks on this one (and when it's finished).

EDIT: #1043 does the same for PlaneMutSlice

@lu-zero
Copy link
Collaborator

left a comment

The two unsafe usages could be wrapped in a different way later, the rest looks pretty neat already :)

@coveralls

This comment has been minimized.

Copy link

commented Feb 23, 2019

Coverage Status

Coverage increased (+0.2%) to 82.026% when pulling 993af98 on rom1v:planeslice.23 into 7fd3938 on xiph:master.

@rom1v rom1v force-pushed the rom1v:planeslice.23 branch 2 times, most recently from 5d12ae6 to 739deda Feb 25, 2019

rom1v added some commits Feb 4, 2019

Expose separate rows from PlaneSlice
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.

As a first step, add PlaneSlice methods to access rows separetely.

See <#631 (comment)>.
Add method to create PlaneSlice view from a Plane
Many functions currently using a primitive slice along with the stride
information will accept a Plane(Mut)Slice instead.

As a consequence, we will often need to get a Plane(Mut)Slice instance
without any offset, so add a convenient function which do not require a
&PlaneOffset parameter.
Implement operator[] for PlaneSlice
Implement trait Index to access PlaneSlice rows as if it was a
two-dimensional array.

That way, we can write:

    let row = &plane_slice[5];
    let value = plane_slice[5][3];
Expose a rows iterator for PlaneSlice
Add rows_iter() to iterate over PlaneSlice rows.
Make convert_slice_2d() use raw pointers
The util function convert_slice_2d() operates on a slice with stride
information. It will become incompatible with PlaneSlice which will not
expose a multi-rows slice anymore (see
<#631 (comment)>).

To keep it generic enough, we don't want to use a PlaneSlice wrapper for
every call, so make the function use raw pointers (and unsafe).

Note: this commit changes indentation for unsafe blocks, so the diff is
more understable with "git show -b".
Make run_filter() use raw pointers
The function run_filter() operates on a slice with stride information.

Since it is sometimes called on primitive arrays that do not belong to a
plane, or with "arbitrary" stride (e.g. 1, which is different from the
plane stride), do not make it accept PlaneSlice.

Instead, make it use raw pointers (and unsafe).

@rom1v rom1v force-pushed the rom1v:planeslice.23 branch from 739deda to c10e3e7 Feb 26, 2019

Do not expose multirows slices in PlaneSlice
Remove as_slice() and similar methods from PlaneSlice which expose
slices spanning multiple rows.

@rom1v rom1v force-pushed the rom1v:planeslice.23 branch from c10e3e7 to 993af98 Feb 26, 2019

@tdaede

This comment has been minimized.

Copy link
Collaborator

commented Feb 27, 2019

This looks excellent!

@tdaede

tdaede approved these changes Feb 27, 2019

@lu-zero lu-zero merged commit f3aa26a into xiph:master Feb 27, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 82.026%
Details
type Item = &'a [T];

fn next(&mut self) -> Option<Self::Item> {
let remaining = self.ps.plane.cfg.height as isize - self.ps.y + self.next_row as isize;

This comment has been minimized.

Copy link
@rom1v

rom1v Feb 27, 2019

Author Collaborator

Oops, parentheses missing:

let remaining = self.ps.plane.cfg.height as isize - (self.ps.y + self.next_row as isize);

It's fixed in my PlaneMutSlice pull request #1043.

This comment has been minimized.

Copy link
@tdaede

tdaede Feb 27, 2019

Collaborator

Weird that it didn't get picked up in tests.

@rom1v rom1v referenced this pull request Mar 18, 2019

Merged

Tile encoding #1126

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.