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

[WIP] CDEF parallelization #2657

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

master-of-zen
Copy link
Collaborator

@master-of-zen master-of-zen commented Jan 29, 2021

I wish to contribute and join rav1e development in the future, as I'm heavy invested in video encoding/AV1 and currently I'm a rust newbie. I heard that a good starting piece would be CDEF parallelization, and here we begin)

This code tries to use rayon's parallel iterator for CDEF parallelization, only issue atm is that output can't be borrowed as mutable.

error[E0596]: cannot borrow `*output` as mutable, as `Fn` closures cannot mutate their captured variables
   --> src/cdef.rs:614:117
    |
614 | ...<T>, input: &Frame<T>, tb: &TileBlocks,  output: &mut TileMut<'_, T>));
    |                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot borrow as mutable

src/lib.rs Outdated
@@ -56,6 +56,7 @@
#![warn(clippy::needless_continue)]
#![warn(clippy::path_buf_push_overwrite)]
#![warn(clippy::range_plus_one)]
#![feature(type_ascription)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a nightly feature, why do you need it?

Copy link
Collaborator Author

@master-of-zen master-of-zen Jan 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwice tpl: &(usize, usize) in queue errors.


note: see issue #23416 <https://github.com/rust-lang/rust/issues/23416> for more information
help: add `#![feature(type_ascription)]` to the crate attributes to enablerustc(E0658)

Don't know how to get around that

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you do not need to do that, rust is not swift :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad)

}

#[hawktracer(filter_tile)]
pub fn filter_tile<T: Pixel>(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please configure your editor to run rustfmt on save.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

src/cdef.rs Outdated
queue.push((fbx, fby));
}}

queue.par_iter()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The output needs to be part of the queue.

@master-of-zen master-of-zen marked this pull request as draft January 29, 2021 13:56
@master-of-zen
Copy link
Collaborator Author

The big issue with the output being accessible by all threads, I don't know a "no pain" way to do this, the probable solution is using spinlock on output

@negge
Copy link
Collaborator

negge commented Jan 30, 2021

I wish to contribute and join rav1e development in the future, as I'm heavy invested in video encoding/AV1 and currently I'm a rust newbie. I heard that a good starting piece would be CDEF parallelization, and here we begin)

Hi @master-of-zen, thank you for the patch. We would be happy to have you join and contribute to the project.

A lot of coordination and discussion for rav1e happens on freenode IRC in #daala. Feel free to join us there as well.

@coveralls
Copy link
Collaborator

coveralls commented Feb 4, 2021

Coverage Status

Coverage increased (+0.3%) to 83.656% when pulling 52b35fa on master-of-zen:CDEF-paralelization into 2ec4e67 on xiph:master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants