AV1 support #819
AV1 support #819
Conversation
algesten
left a comment
algesten
left a comment
There was a problem hiding this comment.
Hi there! Thanks for this!
Overall this looks good. I left some comments in the code.
I suspect there is room to reduce the number of allocations done by the code (the liberal use of Vec), but I'm not overly worried about that. We can merge this PR and revisit with a follow-up.
| /// Default payload type for VP9 profile 0 RTX. | ||
| pub(crate) const PT_VP9_RTX: Pt = Pt::new_with_value(99); | ||
|
|
||
| /// Default payload type for VP9 profile 0. |
| //c.enable_vp8(true); | ||
| //c.enable_h264(true); | ||
| //c.enable_vp9(true); | ||
| c.enable_av1(true); |
There was a problem hiding this comment.
We shouldn't change the defaults here. Let's have a discussion on whether AV1 should be enabled by default. Ideally if you join us on Discord, otherwise, we can discuss in #541
| payload: Vec<u8>, | ||
| size: usize, | ||
| } | ||
|
|
There was a problem hiding this comment.
In these files we try to follow a pattern of having the main exportables from the mod first in the file. See vp8.rs for instance. Then we have internal structs and helper functions in order of appearance.
| /// - T (4 bits) - OBU Type: This 4-bit field specifies the type of data structure contained in the OBU payload. | ||
| /// - E (1 bit) - OBU Extension Flag: A flag indicatin if the optional obu_extension_header is present. | ||
| /// - S (1 bit) - OBU Has Size Field: A flag indicating whether the obu_size syntax element will be present | ||
| /// - R (1 bit) - OBU Reserved Bits: must be set to 0. The value is ignored by a decoder. |
There was a problem hiding this comment.
The doc here should be in the order the fields are in the ascii art.
| /// 0 1 2 3 4 5 6 7 | ||
| /// +---------------+ | ||
| /// |F| T |S|E|R| | ||
| /// +---------------+ |
There was a problem hiding this comment.
Why is there whitespace after |R|?
| obu.obu_type() != OBU_TYPE_TILE_LIST && | ||
| obu.obu_type() != OBU_TYPE_PADDING { | ||
| parsed_obus.push(obu); | ||
| } |
There was a problem hiding this comment.
I suspect it would be better to have an enum.
enum ObuType {
TemporalDelimiter = 2,
TileList = 8
…
}That would allow you to add logic to the type itself.
impl ObuType {
fn include_in_parsed(&self) -> bool {
matches!(self, Self::TemporalDelimiter | Self::TileList | …)
}
}There was a problem hiding this comment.
Maybe bike shed the name on the logic. include_in_packetized or something?
| return; | ||
| } | ||
|
|
||
| let mut packets: Vec<Packet> = vec![]; |
There was a problem hiding this comment.
This looks highly reusable if we store it on the struct and clear between uses. That way we can avoid new allocations for every frame.
|
Add depacketizer to fuzz src/_internal_test_exports/fuzz.rs let codec = match rng.u8(4)? { |
|
Fix CI, just ping here with me or algesten so we can trigger new runs. |
|
Might be worth looking into this impl as well: https://patchwork.ffmpeg.org/project/ffmpeg/patch/[email protected]/ |
|
Might be worth creating a pcap from a couple of other packetizer implementations as well to find differences. |
|
I think this is a good start, it needs some performance tweaks, tests and more checking against the spec. Ideally cross tests agains other implementations as well. |
|
I will fix the CI issues and comments on weekend, will be busy next few days @xnorpx is there an automatic way of fixing lint errors in Rust or I need to fix them manually? |
cargo clippy --fix can do some. I tend to use llms for clippies as it usually just follow directions and usually do the correct thing. |
|
You can check the cargo clippy and format commands in the ci job here: https://github.com/algesten/str0m/blob/main/.github/workflows/cargo.yml So you can run it locally. Only thing is snowflake that might need some manual editing and then make sure cargo fmt does not mess it up. |
|
Maybe also just copy all unittests that pion has as well |
|
Hi @Ramil980, how did you capture pcap data? |
|
Hi @sr1990, I used this guide: https://webrtchacks.com/capture-and-replay-streams-with-video-replay/ |
4 participants
This PR introduces AV1 RTP packetizer and depacketizer support to str0m and enables AV1 in SDP by default
The packetizer is based on the libwebrtc implementation while the depacketizer is adapted from the pion (Go) implementation.
Unit tests for both the packetizer and depacketizer were adapted from libwebrtc, and AV1 support has been verified end-to-end using the chat example.
This is my first time writing code in Rust, so I’d really appreciate any feedback or suggestions 🙂