Improve YUV to RGB Performance III

[jnnks]

This extends the effort from the previous two PRs.
Two methods have been added, which implement a parallel approach to write_rgb8_scalar and write_rgb8_f32x8, without rayon ;)

f32x8

Before
test convert_yuv_to_rgb_1920x1080  ... bench:   4,425,583.45 ns/iter (+/- 471,817.41)
test convert_yuv_to_rgb_512x512    ... bench:     588,151.05 ns/iter (+/- 144,696.95)

After
test convert_yuv_to_rgb_1920x1080  ... bench:   1,291,415.29 ns/iter (+/- 182,767.64)
test convert_yuv_to_rgb_512x512    ... bench:     241,422.93 ns/iter (+/- 19,608.33)

scalar

Before
test convert_yuv_to_rgb_1920x1080  ... bench:   8,984,152.55 ns/iter (+/- 1,119,268.56)
test convert_yuv_to_rgb_512x512    ... bench:   1,122,767.55 ns/iter (+/- 62,880.50)

After
test convert_yuv_to_rgb_1920x1080  ... bench:   3,457,987.00 ns/iter (+/- 2,434,315.60)
test convert_yuv_to_rgb_512x512    ... bench:     444,836.04 ns/iter (+/- 315,727.15)

Please inspect

[ralfbiedert]

Thanks for the follow up! I'd generally be open to adding std only threading support, but some thoughts:

• _par should not be the default, writing RGB, this should be an opt-in; either a decoder option, another methods on the YUV, or an actual option struct. I'm not sure yet which one I'd like more.
• In any case, I'd like to quote Perlis here "one man’s constant is another man’s variable"; the number of threads should be configurable somehow, not a constant
• Broader question: wouldn't it make sense to rather allow callers to "bring their own threading"? Imagine YUV had a method called .split_n() that gave you n views of the original YUV (preferably each just being the same struct type as the parent again), so you could just call write_rgb on these from however many threads you wanted.

[jnnks]

_par should not be the default, ...

Ok, I defaulted to write_rgb8_f32x8_par just for the benchmarks.

I like the idea with .split_n(). One of the issues I have with this impl is the weird offset calculations (eg. let base_y = (2 * (y + offset) + 1) * strides.0;). We can clean that up nicely and concentrate the index math into one place.

[jnnks]

First impl for DecodedYUV::split.

Some thoughts:

• I used split<const N: usize> to return an array instead of an iterator. This feels a bit more in line with the idea of using this for multi threading, where the number of threads would be const as well.
• Internally this is implemented with chunks iterators and the array is built from those iterators. This can certainly be improved later on. The last chunk will be larger than the others if the buffer cannot be split evenly.
• split returns (&[u8], &[u8], &[u8]). I suppose YUVSlices would be better?
• I am confused about DecodedYUV, YUVSource and YUVBuffer. Where should this impl live?

[ralfbiedert]

Hey, so sorry, I somehow missed your latest update!

I used split to return an array instead of an iterator. This feels a bit more in line with the idea of using this for multi threading, where the number of threads would be const as well.

Personally, I don't mind this being a const parameter, but I'd assume some folks would like this to be configurable (e.g., detect numcpus). For now const is fine though.

split returns (&[u8], &[u8], &[u8]). I suppose YUVSlices would be better?

Yes, I think each one being YUVSlices would be nicer to use.

I am confused about DecodedYUV, YUVSource and YUVBuffer. Where should this impl live?

You mean where split should live? To be honest I'd have to play with this a bit myself, but we can refactor that later.

Let me know if there's anything I can help with.

[jnnks]

I am using clippy 0.1.87 (17067e9ac6 2025-05-09) and there are other warnings. The one the CI complained about is not one of those:

error: unnecessary semicolon
   --> openh264/src/decoder.rs:391:14
    |
391 |             };
    |              ^ help: remove
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_semicolon
    = note: `-D clippy::unnecessary-semicolon` implied by `-D clippy::pedantic`
    = help: to override `-D clippy::pedantic` add `#[allow(clippy::unnecessary_semicolon)]`

error: this could be a `const fn`
   --> openh264/src/decoder.rs:423:5
    |
423 | /     pub unsafe fn raw_api(&mut self) -> &mut DecoderRawAPI {
424 | |         &mut self.raw_api
425 | |     }
    | |_____^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#missing_const_for_fn
    = note: `-D clippy::missing-const-for-fn` implied by `-D clippy::nursery`
    = help: to override `-D clippy::nursery` add `#[allow(clippy::missing_const_for_fn)]`
help: make the function `const`
    |
423 |     pub const unsafe fn raw_api(&mut self) -> &mut DecoderRawAPI {
    |         +++++

error: this could be a `const fn`
   --> openh264/src/encoder.rs:810:5
    |
810 | /     pub unsafe fn raw_api(&mut self) -> &mut EncoderRawAPI {
811 | |         &mut self.raw_api
812 | |     }
    | |_____^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#missing_const_for_fn
help: make the function `const`
    |
810 |     pub const unsafe fn raw_api(&mut self) -> &mut EncoderRawAPI {
    |         +++++

error: unnecessary semicolon
   --> openh264/src/encoder.rs:896:18
    |
896 |                 };
    |                  ^ help: remove
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_semicolon

[jnnks]

The tests are failing because I removed some asserts, which are only valid for YUV422.
I am testing with YUV420 and these asserts would fail.

I would remove the asserts/tests. What do you think?

[ralfbiedert]

The tests are failing because I removed some asserts, which are only valid for YUV422. I am testing with YUV420 and these asserts would fail. I would remove the asserts/tests. What do you think?

I'm a bit confused, OpenH264 is only YUV420; so in other words, our own YUV conversion should only ever consider YUV420, or am I missing something?

The 3 tests that are failing (test_new_y_length_not_matching and co), expect a panic if YUV is not 420, e.g., if length is (20, 4, 5) instead of (20,5,5). That is, for an image of size 10x2 as in the test, one would expect Y to be length 20, and U and V each 5?

If so, isn't that what the test is asserting (thus it's checking for 420, and not for 422)?

[ralfbiedert]

I am using clippy 0.1.87 (17067e9ac6 2025-05-09) and there are other warnings. The one the CI complained about is not one of those:

These should be fixed on the latest master. If you could rebase clippy should pass. If not (or in any case) we probably want to upgrade MSRV slowly anyways as more items become const.

[jnnks]

If you could rebase ...

I merged instead with an embarrassing amount of push --force. Lmk if you want rebase instead

I'm a bit confused, OpenH264 is only YUV420; so in other words, our own YUV conversion should only ever consider YUV420, or am I missing something?

Ah, sorry, my bad. I messed the dimensions in split()

[jnnks]

I am wondering how to continue here.

We can introduce a trait (eg. IntoRGB8) that can make the conversion available and we impl it for DecodedYUV and YUVSlices. For YUVSlices this would be a bit awkward, because we need to offset the target reference outside of the function.

openh264/src/formats/rgb.rs already contains some RGBSlices. Conversion between YUV/RGBSlice would be independent from other parts of the frame and can be parallelized in any flavor. We would then have to join the slices somehow.

Seems to me that YUV/RGBSlices are not used in this repo, but are part of the public API.
How are you using them?
From my POV, the second option makes more sense.

[ralfbiedert]

Hey, apologies again for dropping the ball here for so long. I just tried updating your branch to merge this but got permission errors. I now just pulled in all your changes and pushed manually.

I'm aware this isn't done yet, but I think adding split is a step in the right direction and seems to work already. I'm not sure what the best approach for the conversion logic is, this would need some design work and I'm pretty swamped atm.

Closing this PR now since it got 'merged', but we can continue further design discussion here, or in a new PR.