enum Rav1dObuType: make a real enum

[folkertdev]

No description provided.

[kkysen]

Just one little naming thing for one of the variants but otherwise it all LGTM!

[folkertdev]

a general question: how important is the performance of GetBits? its refill method only reads data one byte at a time, which in microbenchmarks is quite inefficient. But maybe in practice this is just not a performance-sensitive part of the code?

This is the logic we've taken from zlib-ng: https://github.com/memorysafety/zlib-rs/blob/main/zlib-rs/src/inflate/bitreader.rs#L103

[kkysen]

I don't believe it's very performance critical. I remember @randomPoison did some benchmarks and I don't think it mattered much, since it's just used to parse the headers. It would be not too hard to swap out the implementation, though, like the approach you took, but we did already make it safe. I also looked into bitvec a bit but the generated code didn't look as optimized. I was also hoping to make APIs like .get_bits const generic and return uN like types, as that also avoids bounds checking for a bunch of arrays. It sucks that Rust doesn't have uN native types, though. I explored that a bit but it wasn't a priority since we already made it safe, but once we finish safety, I'd like to revisit it to squeeze some more performance out.

[folkertdev]

we need to keep the raw integer around for logging later.

It would be nice if rust could represent this in some other way. non_exhaustive seems like it would work here, but that is only an API thing: it does not change the repr. I haven't been able to find an RFC in this direction, though surely one must exist somewhere.

[kkysen]

What do you mean exactly?

[folkertdev]

the problem is this restriction on enums https://doc.rust-lang.org/nomicon/other-reprs.html

Fieldless enums with repr(C) or repr(u*) still may not be set to an integer value without a corresponding variant, even though this is permitted behavior in C or C++. It is undefined behavior to (unsafely) construct an instance of an enum that does not match one of its variants. (This allows exhaustive matches to continue to be written and compiled as normal.)

It would be nice for this sort of low-level code (I've also seen this come up in zlib and ntpd-rs) to opt-in to not having that restriction. Then any match on the type would have to be non-exhaustive, but you could do something like

#[repr(u8, no_niche)]
enum MyEnum {
    Foo,
    Bar,
}

match my_enum {
    Foo => x,
    Bar => y,
    other @ _ => report_error(other as u8), 
}

which could make this work because this mapping is now total

3u8 as MyEnum

there is some talk about range types which maybe would even allow you to write

#[repr(u8)]
enum MyEnum {
    Foo = 1,
    Bar = 2,
    Rest(u8) = 0 | 3..
}

which then can be matched on exhaustively

[kkysen]

Ahh. I do like that last version a lot. Since range/pattern types are generally very useful as well, and it sticks with the requirement then an enum is always one of its variants, which a lot of code relies on I think.

[folkertdev]

I've asked about it on the rust zullip https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/enum.20range.20variants/near/424528113

Looks like this idea does have a lot of overlap with the range types idea, but there are some problems to fix around the representation and guarantees in FFI. Hopefully I've nerdsniped the right people :)

[kkysen]

Whoops, I merged these in the wrong order. Oh well.