refactor!: update Pixel subtraits

[FreezyLemon]

Supersedes #72 which has become a bit of a mess.

I went to the std documentation to collect the traits implemented on u8/u16:

• arithmetic: !, +, -, *, /, %
• bitwise: |, &, ^, <<, >>
• comparison: PartialEq, Eq, PartialOrd, Ord
• formatting: {} {:?} {:o} {:x} {:X} {:e} {:E} {:b}
• conversions: various (from/into, tryfrom/tryinto, FromStr)
• common misc: Clone, Default, Copy, Hash, Sum, Step, Product
• marker (nightly): ZeroablePrimitive, SimdElement, RangePattern, AtomicPrimitive,
• nightly/wip misc: NumBufferTrait, FunnelShift, Distribution
• misc internals: DisjointBitOr, CarrylessMul, CarryingMulAdd

It's notable that a lot of operations on integers (checked_*, wrapping_*, unsigned_abs, abs_diff, log, pow etc. etc. etc.) don't live in a trait but are just type-inherent methods. Even num_traits doesn't expose most of these, maybe because std has steadily been growing over the years.

I see two main problems with keeping arithmetics/bitwise operations "in" Pixel:

• maintenance burden, i.e. keeping up with std/num_traits adding new stuff
• questionable usability of arithmetics given type size limitations (how often do you actually want to work in a u8? any sum will quickly overflow)

So I suggest dropping arithmetic and bitwise operations from the Pixel trait entirely. This requires users to convert to some concrete numeric type before "working" on pixels.

Because there is exactly one of each TryFrom, TryInto, From and Into required by T: Pixel, types can be fully inferred even without outside "help":

let a = pix.into();                // always u16
let b = pix.try_into().unwrap();   // always u8
let pix = T::from(x);              // always u8
let pix = T::try_from(y).unwrap(); // always u16

If you want to widen a Pixel into an integer, you can do u64::from(pix.into()) (which is infallible so no panics ever). If you want to narrow an integer into Pixel, you have to do it twice though: Pixel::try_from(my_u64.try_into().expect("fits into u16")).expect("Pixel is u16").

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.39%. Comparing base (b8f193c) to head (f961865).

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #77      +/-   ##
==========================================
- Coverage   99.40%   99.39%   -0.01%     
==========================================
  Files           7        7              
  Lines         675      666       -9     
==========================================
- Hits          671      662       -9     
  Misses          4        4              

☔ 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.

[shssoichiro]

I want to take a look at how this feels in a real project, e.g. zoomvtools, before I accept it. That way I can understand the implications of the usability changes.

[shssoichiro]

In some places it seems slightly better and in some places slightly worse.

Before:

impl<T> Pixel for T
where
    T: ...
{
    #[inline(always)]
    fn from_u16_or_max_value(value: u16) -> Self {
        Self::from(value).unwrap_or_else(|| {
            // If conversion fails (shouldn't happen with our inputs), fallback to max
            Self::max_value()
        })
    }

    #[inline(always)]
    fn from_u32_or_max_value(value: u32) -> Self {
        Self::from(value).unwrap_or_else(|| {
            // If conversion fails (shouldn't happen with our inputs), fallback to max
            Self::max_value()
        })
    }
}

After:

impl<T> Pixel for T
where
    T: ...
{
    #[inline(always)]
    fn from_u16_or_max_value(value: u16) -> Self {
        match size_of::<T>() {
            1 => T::from(value.clamp(0, u8::MAX as u16) as u8),
            2 => semisafe_res_unwrap(T::try_from(value)),
            _ => unreachable!(),
        }
    }

    #[inline(always)]
    fn from_u32_or_max_value(value: u32) -> Self {
        match size_of::<T>() {
            1 => T::from(value.clamp(0, u8::MAX as u32) as u8),
            2 => semisafe_res_unwrap(T::try_from(value.clamp(0, u16::MAX as u32) as u16)),
            _ => unreachable!(),
        }
    }
}

Before:

for (f, p) in f_src.iter_mut().zip(p_src.iter()) {
    *f = p.to_f32().expect("fits in f32");
}

After:

for (f, p) in f_src.iter_mut().zip(p_src.iter()) {
    // u16::from doesn't work
    let p: u16 = (*p).into();
    *f = p as f32;
}

Before:

pub fn blend_plane<T: Pixel>(
    dst: &mut [T],
    src: &[T],
    reference: &[T],
    dst_stride: usize,
    src_stride: usize,
    ref_stride: usize,
    width: usize,
    height: usize,
    time256: i32,
) {
    for y in 0..height {
        for x in 0..width {
            let value =
                (<T as num_traits::AsPrimitive<u32>>::as_(*semisafe_get(src, y * src_stride + x))
                    * (256 - time256) as u32
                    + <T as num_traits::AsPrimitive<u32>>::as_(*semisafe_get(
                        reference,
                        y * ref_stride + x,
                    )) * time256 as u32)
                    >> 8;
            *semisafe_get_mut(dst, y * dst_stride + x) = T::from_u32_or_max_value(value);
        }
    }
}

After:

pub fn blend_plane<T: Pixel>(
    dst: &mut [T],
    src: &[T],
    reference: &[T],
    dst_stride: usize,
    src_stride: usize,
    ref_stride: usize,
    width: usize,
    height: usize,
    time256: i32,
) {
    for y in 0..height {
        for x in 0..width {
            let value = {
                let src: u16 = (*semisafe_get(src, y * src_stride + x)).into();
                let ref_: u16 = (*semisafe_get(reference, y * ref_stride + x)).into();
                (src as u32 * (256 - time256) as u32 + ref_ as u32 * time256 as u32) >> 8
            };
            *semisafe_get_mut(dst, y * dst_stride + x) = T::from_u32_or_max_value(value);
        }
    }
}

I haven't found any places where it becomes a blocker. I guess I'm leaning toward merge, but it is a large breaking change.

[FreezyLemon]

Thanks for taking the time to give it a shot!

I agree, some places are worse than before, notably the T::from and T::try_from semantics feel less concise. But I can't think of a great way to fix this, pixel values for HBD will inherently be > u8::MAX and I wouldn't know how to statically guarantee that

if size_of::<T>() == 2 {
    T::try_from(my_u16).expect("Pixel is u16")
}

becomes something like an infallible From. FWIW, u16::TryFrom<u16> is infallible (duh) and a pretty thin implementation, so I doubt it's ever really going to have a panic / branch anywhere.

Oh, and one small FYI: No need to do let x: u16 = something().into();, there is only Into<u16>. If you want to do a oneliner you could even do let src = u32::from((*semisafe_get(...)).into()); in that example.

[FreezyLemon]

On second look.. should we maybe add helpers like from_uXY_or_clamped() to Pixel too? Could help downstream...

Maybe later. I don't really want to make this PR bigger than it needs to be.