Skip to content
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.

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

Fix RgbScalar #2416

Merged
merged 12 commits into from
Mar 27, 2023
Merged

Conversation

stefannikolei
Copy link
Contributor

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

I ran the tests with SSE Disabled. And I found this bug...

Before fixing it I had 151 failing tests after it only 53 tests are failing. On a quick check all other failures are 'Image difference is over threshold!'

@brianpopow
Copy link
Collaborator

We need a test to cover this issue. I will try to help here.

There are more color converter path's which are not covered by tests: ColorConverters. We need not add more unit tests there (@stefannikolei: not necessary you, btw).

@brianpopow
Copy link
Collaborator

I have added tests now to cover all path for JpegColorConverterBase

I think its ready for review now.

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

LGTM for the fix.

I wonder now however, why do we have CmykArm64, while we don't have YCbCrArm64. Doesn't CmykArm64 and CmykVector have essentially the same performance?

@stefannikolei
Copy link
Contributor Author

LGTM for the fix.

I wonder now however, why do we have CmykArm64, while we don't have YCbCrArm64. Doesn't CmykArm64 and CmykVector have essentially the same performance?

Ycbcr is implemented in #2417

You can check the performance in the prs. All have benchmarks attached

@antonfirsov
Copy link
Member

antonfirsov commented Mar 27, 2023

Ycbcr is implemented in #2417

Ah for some reason I thought that was already merged 😄 With those numbers it makes sense thanks!

@antonfirsov antonfirsov merged commit 2ba22e4 into SixLabors:main Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants