Skip to content

Conversation

@brianpopow
Copy link
Collaborator

@brianpopow brianpopow commented Oct 31, 2021

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

During profiling I noticed alot of time with lossy decoding is spend in System.Collections. Turns out using Dictionary's for lookup tables was not the best idea. I have changed that now to byte array's. it reduces the lossy decoding time from 645.7 ms to 280.8 ms.

Before:

|                     Method |        Job |              Runtime |             Arguments |        TestImageLossy |        TestImageLossless |       Mean |     Error |  StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|--------------------------- |----------- |--------------------- |---------------------- |---------------------- |------------------------- |-----------:|----------:|--------:|------:|------:|------:|----------:|
|        'Magick Lossy Webp' | Job-OJJMOX |             .NET 5.0 | /p:DebugType=portable | Webp/earth_lossy.webp | Webp/earth_lossless.webp |   106.3 ms |   8.05 ms | 0.44 ms |     - |     - |     - |     25 KB |
|    'ImageSharp Lossy Webp' | Job-OJJMOX |             .NET 5.0 | /p:DebugType=portable | Webp/earth_lossy.webp | Webp/earth_lossless.webp |   645.7 ms | 148.77 ms | 8.15 ms |     - |     - |     - |  2,430 KB |
|     'Magick Lossless Webp' | Job-OJJMOX |             .NET 5.0 | /p:DebugType=portable | Webp/earth_lossy.webp | Webp/earth_lossless.webp |   107.3 ms |   4.08 ms | 0.22 ms |     - |     - |     - |     16 KB |
| 'ImageSharp Lossless Webp' | Job-OJJMOX |             .NET 5.0 | /p:DebugType=portable | Webp/earth_lossy.webp | Webp/earth_lossless.webp |   303.3 ms |  42.16 ms | 2.31 ms |     - |     - |     - |  2,092 KB |
|        'Magick Lossy Webp' | Job-IMQWGF |        .NET Core 3.1 |               Default | Webp/earth_lossy.webp | Webp/earth_lossless.webp |   106.1 ms |   5.20 ms | 0.29 ms |     - |     - |     - |     25 KB |
|    'ImageSharp Lossy Webp' | Job-IMQWGF |        .NET Core 3.1 |               Default | Webp/earth_lossy.webp | Webp/earth_lossless.webp |   748.4 ms |  14.45 ms | 0.79 ms |     - |     - |     - |  2,428 KB |
|     'Magick Lossless Webp' | Job-IMQWGF |        .NET Core 3.1 |               Default | Webp/earth_lossy.webp | Webp/earth_lossless.webp |   105.3 ms |   5.27 ms | 0.29 ms |     - |     - |     - |     15 KB |
| 'ImageSharp Lossless Webp' | Job-IMQWGF |        .NET Core 3.1 |               Default | Webp/earth_lossy.webp | Webp/earth_lossless.webp |   490.3 ms |  20.95 ms | 1.15 ms |     - |     - |     - |  2,092 KB |
|        'Magick Lossy Webp' | Job-ETPBSM | .NET Framework 4.7.2 |               Default | Webp/earth_lossy.webp | Webp/earth_lossless.webp |   105.8 ms |   4.06 ms | 0.22 ms |     - |     - |     - |     32 KB |
|    'ImageSharp Lossy Webp' | Job-ETPBSM | .NET Framework 4.7.2 |               Default | Webp/earth_lossy.webp | Webp/earth_lossless.webp | 1,114.4 ms |  26.50 ms | 1.45 ms |     - |     - |     - |  2,436 KB |
|     'Magick Lossless Webp' | Job-ETPBSM | .NET Framework 4.7.2 |               Default | Webp/earth_lossy.webp | Webp/earth_lossless.webp |   106.0 ms |   0.52 ms | 0.03 ms |     - |     - |     - |     18 KB |
| 'ImageSharp Lossless Webp' | Job-ETPBSM | .NET Framework 4.7.2 |               Default | Webp/earth_lossy.webp | Webp/earth_lossless.webp | 1,728.1 ms |  51.56 ms | 2.83 ms |     - |     - |     - |  9,729 KB |

After:

|                     Method |        Job |              Runtime |             Arguments |        TestImageLossy |        TestImageLossless |       Mean |     Error |  StdDev |    Gen 0 | Gen 1 | Gen 2 | Allocated |
|--------------------------- |----------- |--------------------- |---------------------- |---------------------- |------------------------- |-----------:|----------:|--------:|---------:|------:|------:|----------:|
|        'Magick Lossy Webp' | Job-RHHVLS |             .NET 5.0 | /p:DebugType=portable | Webp/earth_lossy.webp | Webp/earth_lossless.webp |   108.5 ms |  62.82 ms | 3.44 ms |        - |     - |     - |     25 KB |
|    'ImageSharp Lossy Webp' | Job-RHHVLS |             .NET 5.0 | /p:DebugType=portable | Webp/earth_lossy.webp | Webp/earth_lossless.webp |   280.8 ms |  39.87 ms | 2.19 ms | 500.0000 |     - |     - |  2,428 KB |
|     'Magick Lossless Webp' | Job-RHHVLS |             .NET 5.0 | /p:DebugType=portable | Webp/earth_lossy.webp | Webp/earth_lossless.webp |   106.8 ms |   9.31 ms | 0.51 ms |        - |     - |     - |     16 KB |
| 'ImageSharp Lossless Webp' | Job-RHHVLS |             .NET 5.0 | /p:DebugType=portable | Webp/earth_lossy.webp | Webp/earth_lossless.webp |   300.4 ms |  10.50 ms | 0.58 ms |        - |     - |     - |  2,092 KB |
|        'Magick Lossy Webp' | Job-SMYZVS |        .NET Core 3.1 |               Default | Webp/earth_lossy.webp | Webp/earth_lossless.webp |   107.6 ms |  30.85 ms | 1.69 ms |        - |     - |     - |     25 KB |
|    'ImageSharp Lossy Webp' | Job-SMYZVS |        .NET Core 3.1 |               Default | Webp/earth_lossy.webp | Webp/earth_lossless.webp |   287.8 ms |  13.71 ms | 0.75 ms |        - |     - |     - |  2,428 KB |
|     'Magick Lossless Webp' | Job-SMYZVS |        .NET Core 3.1 |               Default | Webp/earth_lossy.webp | Webp/earth_lossless.webp |   106.8 ms |   4.50 ms | 0.25 ms |        - |     - |     - |     15 KB |
| 'ImageSharp Lossless Webp' | Job-SMYZVS |        .NET Core 3.1 |               Default | Webp/earth_lossy.webp | Webp/earth_lossless.webp |   487.9 ms |  16.37 ms | 0.90 ms |        - |     - |     - |  2,092 KB |
|        'Magick Lossy Webp' | Job-VDUYMS | .NET Framework 4.7.2 |               Default | Webp/earth_lossy.webp | Webp/earth_lossless.webp |   105.7 ms |   3.50 ms | 0.19 ms |        - |     - |     - |     32 KB |
|    'ImageSharp Lossy Webp' | Job-VDUYMS | .NET Framework 4.7.2 |               Default | Webp/earth_lossy.webp | Webp/earth_lossless.webp |   558.2 ms |   3.61 ms | 0.20 ms |        - |     - |     - |  2,436 KB |
|     'Magick Lossless Webp' | Job-VDUYMS | .NET Framework 4.7.2 |               Default | Webp/earth_lossy.webp | Webp/earth_lossless.webp |   106.5 ms |  11.57 ms | 0.63 ms |        - |     - |     - |     18 KB |
| 'ImageSharp Lossless Webp' | Job-VDUYMS | .NET Framework 4.7.2 |               Default | Webp/earth_lossy.webp | Webp/earth_lossless.webp | 1,757.6 ms | 170.33 ms | 9.34 ms |        - |     - |     - |  9,729 KB |

@brianpopow brianpopow linked an issue Oct 31, 2021 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Oct 31, 2021

Codecov Report

Merging #1800 (414e4a8) into master (527e0fb) will increase coverage by 0.25%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1800      +/-   ##
==========================================
+ Coverage   87.05%   87.30%   +0.25%     
==========================================
  Files         936      936              
  Lines       47638    47832     +194     
  Branches     6017     6009       -8     
==========================================
+ Hits        41469    41761     +292     
+ Misses       5177     5082      -95     
+ Partials      992      989       -3     
Flag Coverage Δ
unittests 87.30% <100.00%> (+0.25%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/ImageSharp/Formats/Webp/Lossy/LossyUtils.cs 100.00% <100.00%> (ø)
src/ImageSharp/Formats/Webp/WebpLookupTables.cs 98.79% <100.00%> (+0.17%) ⬆️
...mageSharp/Formats/Webp/Lossless/NearLosslessEnc.cs 88.67% <0.00%> (-7.55%) ⬇️
...ageSharp/Formats/Webp/Lossless/PredictorEncoder.cs 98.26% <0.00%> (+9.24%) ⬆️
.../ImageSharp/Formats/Webp/Lossless/LosslessUtils.cs 97.71% <0.00%> (+9.49%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 527e0fb...414e4a8. Read the comment docs.

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.

Yeah much better! It'd be nice to document what each table provides but not a requirement.

@brianpopow brianpopow changed the title WIP: Webp: Use byte arrays instead of Dictionary's for lookups Webp: Use byte arrays instead of Dictionary's for lookups Nov 1, 2021
@brianpopow brianpopow merged commit d021222 into master Nov 1, 2021
@brianpopow brianpopow deleted the bp/avoiddictionarys branch November 1, 2021 08:32
Comment on lines +980 to +985
p[offset - step3] = WebpLookupTables.Clip1[p2 + a3 + 255];
p[offset - step2] = WebpLookupTables.Clip1[p1 + a2 + 255];
p[offsetMinusStep] = WebpLookupTables.Clip1[p0 + a1 + 255];
p[offset] = WebpLookupTables.Clip1[q0 - a1 + 255];
p[offset + step] = WebpLookupTables.Clip1[q1 - a2 + 255];
p[offset + step2] = WebpLookupTables.Clip1[q2 - a3 + 255];
Copy link
Member

Choose a reason for hiding this comment

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

Bit late to the party but:

Do we always shift with the same value for the same lookup? Shouldn't we define a helper method then or at least add an explanatory comment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Bit late to the party but

Maybe I was a bit to quick in merging it.

Do we always shift with the same value for the same lookup?

yes, in C it is defined like this:

const uint8_t* const VP8kclip1 = &clip1[255];

See dec_clip_tables.c in the libwebp source.
I thought there might be a way to do the same in C#, but i could not come up with a better solution then this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will create a follow up PR with helper methods

Copy link
Member

Choose a reason for hiding this comment

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

Maybe you could use a Span that starts at the 255 offset?

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.

WebP - Improve performance

4 participants