Skip to content

RFE: encoder: add stride & various raw input formats support #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

RFE: encoder: add stride & various raw input formats support #11

wants to merge 18 commits into from

Conversation

elmarco
Copy link

@elmarco elmarco commented Feb 12, 2025

The input format is not necessarily RGB or RGBA, and doing a pre-pass conversion can be quite costly (adding about 15-20% of total time from empirical study)

For simplicity reasons, I made sure not to break the existing API.

Those changes don't seem to affect the encoder performance in a significant way.

@elmarco
Copy link
Author

elmarco commented Feb 13, 2025

See also: Devolutions/IronRDP#670

@elmarco
Copy link
Author

elmarco commented Mar 18, 2025

@aldanor hi, wdyt?

@elmarco
Copy link
Author

elmarco commented Apr 15, 2025

@aldanor ping :thanks:

@elmarco
Copy link
Author

elmarco commented May 9, 2025

@aldanor are you still maintaining the crate? thanks

@Joshix-1
Copy link

The api feels kinda confusing too me. It would be cool to control Input format and output format independently. I have the use case that I have an image with RGBA data where I know that the image has no translucency, so I want to save it as RGB. I don't know how I would do this with this PR. Which is why I created #15

Would be cool if it was possible to control the output format independently like in my pr.

And I don't really understand the stride argument. That should maybe be documented better.

@elmarco
Copy link
Author

elmarco commented May 24, 2025

The api feels kinda confusing too me. It would be cool to control Input format and output format independently. I have the use case that I have an image with RGBA data where I know that the image has no translucency, so I want to save it as RGB. I don't know how I would do this with this PR. Which is why I created #15

You can use the RawChannels::Rgbx or Bgrx, or Xbgr etc.. from this PR for that.

Would be cool if it was possible to control the output format independently like in my pr.

Well, you can't really mix input and output formats freely. You need alpha channel input for alpha channel output. And QOI has only two output formats, rgb and rgba...

And I don't really understand the stride argument. That should maybe be documented better.

https://learn.microsoft.com/en-us/windows/win32/medfound/image-stride

@Joshix-1
Copy link

You can use the RawChannels::Rgbx or Bgrx, or Xbgr etc.. from this PR for that.

True, didn't think about that. But what if someone wants to add an alpha channel?

You need alpha channel input for alpha channel output.

Adding an alpha channel with default values of 255 would be imho fine and not unexpected.

@elmarco
Copy link
Author

elmarco commented May 27, 2025

You need alpha channel input for alpha channel output.

Adding an alpha channel with default values of 255 would be imho fine and not unexpected.

Well, this use case goes beyond the typical simple encoding imo.

@Joshix-1
Copy link

Joshix-1 commented May 28, 2025 •
edited
Loading

Why is RawChannels::bytes_per_pixel not public? There should either be another Encoder constructor without a stride argument or bytes_per_pixel should be public. Otherwise the Encoder::new_raw API is imho too clunky to use if you have no stride.

@elmarco
Copy link
Author

elmarco commented May 28, 2025

Why is RawChannels::bytes_per_pixel not public? There should either be another Encoder constructor without a stride argument or bytes_per_pixel should be public. Otherwise the Encoder::new_raw API is imho too clunky to use if you have no stride.

We could make stride an Option. Alternatively, we could have an EncoderBuilder, but this might be overkill.

@Joshix-1
Copy link

Stride could even be Option<NonZeroUsize>, but Option<usize> would be fine as well

Joshix-1
Joshix-1 approved these changes Jun 4, 2025
View reviewed changes
Copy link

@Joshix-1 Joshix-1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the EncoderBuilder. It really improves the usability. Have a few nitpics

src/encode.rs Outdated
if stride * (height - 1) as usize + width as usize * raw_channels.bytes_per_pixel() < size {
return Err(Error::InvalidImageLength { size, width, height });
}
if guess_stride && size != width as usize * height as usize * raw_channels.bytes_per_pixel()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

width as usize * raw_channels.bytes_per_pixel() is a common element in the if statements, maybe that could be put in a variable

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

width_usize ? hmm, I don't know if this is a common pattern. I am a bit unsure.

@Joshix-1 Joshix-1 mentioned this pull request Jul 1, 2025
Open
elmarco and others added 12 commits July 22, 2025 11:32
@elmarco
Fork the project for now
47f6a43 
Unfortunately, Ivan Smirnov the developper and maintainer of the "qoi"
crate is not working actively on the project for the past 3-4 months.

aldanor#14

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
@elmarco
bench: fix short must be unique
76ae676 
Argument short must be unique

	-s is already in use

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
@elmarco
Fix fork doc link
41ddc07 
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
@elmarco
Merge pull request #2 from elmarco/bench-fix
f2680d2 
bench: fix short must be unique
@elmarco
Fix decoding to larger output buffer
ab7f97e 
`decode_impl_stream` expects the output buffer to be of the size of the
decoded content.

Fixes:
aldanor#18

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
@elmarco
Merge pull request #3 from elmarco/issue-18
48eae1c 
Fix decoding to larger output buffer
@elmarco
Fix fork references
39adb4a 
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
@elmarco
Fix fuzz build
f88e71a 
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
@elmarco
Fix clippy warning in fuzz
0bacf9c 
warning: deref on an immutable reference

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
@elmarco
Merge pull request #4 from elmarco/fuzz
75c7f18 
Fix fuzz build
@Martomate @elmarco
decode: fix transparent background bug
bd54802 
This PR fixes qoi-rust#16 where the instructions [RUN 1, RGB r g b,
INDEX 53] produced [Black, rgb, Transparent] instead of [Black, rgb,
Black]. 53 is the hash of (0, 0, 0, 255) and in the special case where
the first instruction is RUN it needs to store the color in the index so
that future INDEX instructions can fetch the value. This is only needed
if the first instruction is RUN.

Since this is only needed for the first instruction one could probably
handle this edgecase before the loop. If that is something we want, we
can either modify this PR or you can make a new one with a better
solution.

[ fix rebase conflict, update commit message ]
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
@elmarco
decode: handle the initial index edge-case before the loop
f8805ca 
Minor improvement over the previous commit.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
elmarco and others added 6 commits July 22, 2025 13:51
@elmarco
Merge pull request #5 from elmarco/issue-16
09dadb0 
decode: fix transparent background bug
@elmarco
Add a comment
2154a60 
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
@elmarco
Fix a typo
083b143 
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
@elmarco
encoder: add stride & various raw input formats support
742a571 
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
@elmarco
Add some encoder tests
5ff97e0 
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
@elmarco
Add more flexible EncoderBuilder
9b0e41c 
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
elmarco added a commit to elmarco/qoi-rust that referenced this pull request Jul 22, 2025
@elmarco
Merge pull request #6 from elmarco/raw
a24a542 
 RFE: encoder: add stride & various raw input formats support aldanor#11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Joshix-1 Joshix-1 Joshix-1 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@elmarco @Joshix-1 @Martomate