Skip to content

Jpeg encoder complete rewrite #2120

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

Merged

Conversation

br3aker
Copy link
Contributor

@br3aker br3aker commented May 15, 2022

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)

New architecture

Jpeg encoder now have a brand new architecture which allows us to define encoding 'configs' like this:

// YCbCr 4:1:0
new JpegFrameConfig(
    JpegColorSpace.YCbCr,
    JpegEncodingColor.YCbCrRatio410,
    new JpegComponentConfig[]
    {
        new JpegComponentConfig(id: 1, hsf: 4, vsf: 2, quantIndex: 0, dcIndex: 0, acIndex: 0),
        new JpegComponentConfig(id: 2, hsf: 1, vsf: 1, quantIndex: 1, dcIndex: 1, acIndex: 1),
        new JpegComponentConfig(id: 3, hsf: 1, vsf: 1, quantIndex: 1, dcIndex: 1, acIndex: 1),
    },
    yCbCrHuffmanConfigs,
    yCbCrQuantTableConfigs)

Which allow us to define any allowed subsampling setup or even a 'custom' scenario with any number of components if user needs to. This API is internal for now - user can only choose encoding mode via enum which we had a long time ago. Maybe one day we can make it public - who knows.

Due to this change: #1713 is resolved and #1476 is resolved.

It would also be fairly easy to implement optimized huffman tables or progressive encoding with current architecture.

API changes

  1. All of these encoding modes are implemented now:
YCbCrRatio420 = 0,
YCbCrRatio444 = 1,
YCbCrRatio422 = 2,
YCbCrRatio411 = 3,
YCbCrRatio410 = 4,
Luminance = 5,
Rgb = 6,
Cmyk = 7,
Ycck = 8,
  1. Grayscale encoding now has a vectorized color conversion which gained a lot of performance.

3. I've removed Ycck encoding simply because it's not popular and we didn't support it anyway (we still can decode YccK though). This issue is partially done: #808

  1. Added non-interleaved encoding for any color type.

Performance

Encoder got a bit slower but I think new architecture's worth it - we were really fast before, now we are still fast enough.

Main branch

Method TargetColorSpace Quality Mean
Benchmark Luminance 75 6.923 ms
Benchmark Rgb 75 12.116 ms
Benchmark YCbCrRatio420 75 6.484 ms
Benchmark YCbCrRatio444 75 8.263 ms
Benchmark Luminance 90 7.046 ms
Benchmark Rgb 90 13.349 ms
Benchmark YCbCrRatio420 90 6.714 ms
Benchmark YCbCrRatio444 90 9.198 ms
Benchmark Luminance 100 9.057 ms
Benchmark Rgb 100 19.099 ms
Benchmark YCbCrRatio420 100 10.194 ms
Benchmark YCbCrRatio444 100 15.372 ms

PR

Method TargetColorSpace Quality Mean
Benchmark Luminance 75 4.618 ms
Benchmark Rgb 75 12.543 ms
Benchmark YCbCrRatio420 75 6.639 ms
Benchmark YCbCrRatio444 75 8.590 ms
Benchmark Luminance 90 5.456 ms
Benchmark Rgb 90 13.447 ms
Benchmark YCbCrRatio420 90 7.218 ms
Benchmark YCbCrRatio444 90 9.150 ms
Benchmark Luminance 100 6.731 ms
Benchmark Rgb 100 19.831 ms
Benchmark YCbCrRatio420 100 10.541 ms
Benchmark YCbCrRatio444 100 15.345 ms

@br3aker
Copy link
Contributor Author

br3aker commented May 15, 2022

Need to write a ton of tests now....

MultiplyToAverage(sourceRow, averageMultiplier);

// copy to the first 8 slots
sourceRow.Slice(0, packedWidth).CopyTo(this.ColorBuffer.DangerousGetRowSpan(i / factors.Height));
Copy link
Contributor Author

@br3aker br3aker May 15, 2022

Choose a reason for hiding this comment

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

This is one of the reasons 420 subsampling encoding got slower - we absolutely need to move ready-to-encode color strides to the first 8 indices. This potentially can be resolved at the TPixel -> RGB conversion step - we can rearrange color strides before doing subsampling but it's a very tricky thing to do, I've decided to make this PR as small as possible.

Copy link
Contributor

@gfoidl gfoidl left a comment

Choose a reason for hiding this comment

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

Half way viewed (have to leave now, will continue later).

Copy link
Contributor

@gfoidl gfoidl left a comment

Choose a reason for hiding this comment

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

Finished my review -- didn't look at test code (due to missing knowledge there).

Looks good 👍🏻

@brianpopow
Copy link
Collaborator

Hmm, this failed test passes on my local machine with .net6 and fails only on macos on .net7. Wondering if this is the same problem as in #2117.

I have not seen issue #2117 on mac yet, only on windows so far, but it could be related.

I will restart the CI to see, if it was a fluke or always happens.

@br3aker
Copy link
Contributor Author

br3aker commented May 22, 2022

I will restart the CI to see, if it was a fluke or always happens.

Looks like it's not a fluke. I'll start writing tests this week for this PR and try to add debug checks for memory access, maybe it's related to some out of bounds index access.

@br3aker br3aker changed the title Jpeg encoder complete rewrite [WIP] Jpeg encoder complete rewrite Jun 28, 2022
@br3aker
Copy link
Contributor Author

br3aker commented Aug 7, 2022

Finally had a moment to finish this:

  1. All colors are supported for encoding
  2. Implemented non-interleaved encoding aka a separate scan for each component
  3. Added tests and removed obsolete benchmark tests - we have proper benchmarks for that
  4. Restored ProfilingSandbox.cs

@JimBobSquarePants @brianpopow

@br3aker br3aker changed the title [WIP] Jpeg encoder complete rewrite Jpeg encoder complete rewrite Aug 7, 2022
@br3aker
Copy link
Contributor Author

br3aker commented Aug 7, 2022

Looks like Skew tests fails on net7 is still a thing. Everything else should be working just fine. I've added tests for all new output color types with a very precise tolerance.

New encoder architecture mirrors decoder architecture. It should allow us to write encoding expansions like optimized huffman table generation or progressive encoding.

P.S.
Please be advised that this PR is a breaking change due to new jpeg output color type enum.

@JimBobSquarePants
Copy link
Member

Very exciting! Will review this asap.

@br3aker
Copy link
Contributor Author

br3aker commented Aug 10, 2022

Looks like I've messed up linked issues, here's the complete list of what's resolved by this PR:

  1. Speed Up Jpeg Encoder Color Conversion #1476 - all color conversions now support scalar/vector/avx paths
  2. Feature request: allow Color Space choices in JpegEncoder #808 - all output color types are supported
  3. Jpeg encoder writes zeroed chrominance quantization table for grayscale images and RGB encoding #1713 - only required quantization table(s) is(are) written
  4. Opening and saving jpeg using CMYK color space drastically changes image's color #1238 - CMYK input jpeg now should be resaved to CMYK output jpeg if not explicitely overriden in encoder settings

Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

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

This all looks awesome. Thanks again for such a fantastic addition to the library!

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.

4 participants