[Suggestion] feat: roll our own Pixel-to-integer conversions

[FreezyLemon]

This is not one of my "let's remove dependencies because I want fewer dependencies" PRs.

When porting av-metrics to v_frame 0.5, I noticed a performance regression that I couldn't resolve.

related code:

// file: av_metrics/src/video/psnr.rs

/// Calculate the squared error for a `Plane` by comparing the original (uncompressed)
/// to the compressed version.
fn calculate_plane_total_squared_error<T: Pixel>(plane1: &Plane<T>, plane2: &Plane<T>) -> f64 {
    plane1
        .data
        .iter()
        .zip(plane2.data.iter())
        .map(|(a, b)| (i32::cast_from(*a) - i32::cast_from(*b)).unsigned_abs() as u64)
        .map(|err| err * err)
        .sum::<u64>() as f64
}

in v_frame 0.3, these integer casts were implemented in-crate and infallible. Now we use the num-traits code and could use .to_i32().expect("pixel fits into i32"), but this is slower for some reason.

Adding a simple Pixel::as_u16() function into the trait fixes this immediately for a very small amount of boilerplate. My question now: Do we expect downstream users to use the generic math from num-traits? T: Pixel implies you can do a lot of math on T, + - * / % | & ! are all implemented for it via num_traits::PrimInt. But is this actually useful for crate users?

For example, (i32::cast_from(*a) - i32::cast_from(*b)).unsigned_abs() as u64 could be reimplemented with math over T as something like

let abs_diff = if a > b { a - b } else { b - a };
abs_diff.to_u64().unwrap()

but in the end, you'd always need to convert back to a concrete type anyways. And it can be a real problem to not know the real size of the type. Can you sum in T or do you need to convert to u64? Can you square in T or do you need to widen the type? etc.

ASM code does transmutation based on byte width anyways, and byte_data() is still available for the case that u16 is too large for what you'd want. So it feels a bit difficult to justify keeping all the num-traits traits there IMHO.

[codecov]

Codecov Report

❌ Patch coverage is 14.28571% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.66%. Comparing base (d3176bc) to head (91f7ddb).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/pixel.rs	0.00%	6 Missing ⚠️
src/pixel_ext.rs	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #72      +/-   ##
==========================================
- Coverage   99.53%   97.66%   -1.87%     
==========================================
  Files           7        9       +2     
  Lines         639      643       +4     
==========================================
- Hits          636      628       -8     
- Misses          3       15      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[FreezyLemon]

Already found some problems with this while trying it on av-metrics.. Let me make this a draft for now.

[shssoichiro]

I think the AsPrimitive trait from num-traits works for any upcasting (via as_()), but I don't think it's implemented for downcasting, which would have to go through checking that the conversion is valid. 🤔 Although we could implement wrappers around it for downcasting, perhaps. Somehow.

[FreezyLemon]

I almost thought we could get away with the std traits Into,TryInto etc... It works for the simple cases but any TryInto/TryFrom will have the problem that the associated error type may be anything (and might not be unwrappable etc.).

Might still be good enough if we omit the fallible conversions.. that would mean:

For any type >= u16 or >= i32 (infallible):

let x: u16 = t.into();

For u8, i16, etc. (fallible):

// intermediary:
let x: i32 = t.into();
let x = i16::try_from(x).expect("fits into i16");

I think T -> u8 is going to be common enough that the meh ergonomics could become a problem.

AsPrimitive might just be better suited for this after all, if we can make the infallible conversions available downstream. I'll take another look

[FreezyLemon]

What a pain. I can't really find a sweet spot here.

Pixel -> integers:

• Into<i32> etc. seem good (for types >= u16). Callsites can be a bit boilerplatey if you need to do <T as Into<u16>>::into(t) but otherwise it's the standard (and std) way of doing things
• fallible conversions (into u8, i8, i16, isize¹) are a bit more complicated:
  • Do we want to truncate (== t as u8) or convert fallibly (== u8::try_from(t))? Maybe expose both?
  • Truncating conversions could be provided by num_traits::AsPrimitive<u8>
  • u8: TryFrom<T>/T: TryInto<u8> is really annoying to use, because the related error type has no trait bounds. u8::try_from(t).unwrap() is not possible unless the callsite includes a trait bound on the error type: where <u8 as TryFrom<T>>::Error: std::error::Error. This is "infectious" to all downstream code and generally horrible.
  • Alternatively, rolling our own try_into is possible

Integers -> Pixel:

• From<u8> is trivially possible.
• Anything else has the same questions: Truncating (some_i32 as u8/u16) or fallible (u8/u16::try_from(some_i32))? Or both?
  • especially annoying for u16 inputs because it's a common case (consider if byte_width == 1 { ... } else { ... }) and needs to be performant. The other types (especially u32/i32 and larger) are not quite as performance-critical
• There is no perfect equivalent to num_traits::AsPrimitive<u8> in the other direction (FromPrimitive works differently and seems to not be a perfect fit, especially for performance-sensitive applications)
• TryFrom/TryInto still has the same problem in the other direction: error types are basically unusable without a ton of trait bounds at callsites.

Maybe I need to take another look at some of the common usage patterns or something, I don't think we can make everyone happy here. Or maybe we need to add more unsafe code for performance-sensitive stuff... We often assume that size_of::<T>() for T: Pixel can only be 1 (u8) or 2 (u16), so if we codify it in the API, we might be able to do more with unsafe code.

Footnotes

1. isize is technically allowed to be i16 so From<u16> for isize is not implemented. ↩

[FreezyLemon]

I missed that it's actually very possible to add a trait bound to the error type of TryFrom/TryInto 🤦