Skip to content

Conversation

br3aker
Copy link
Contributor

@br3aker br3aker commented Nov 27, 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

Right now this PR exist purely to test crashes during testing, work is still in progress.

This PR consists of a lot of small fixes to the jpeg codebase: general cleanup, redundant/unused code removal and small performance tweaks via removing unnecessary checks.

@codecov
Copy link

codecov bot commented Nov 27, 2021

Codecov Report

Merging #1853 (7d524bd) into master (3a3059e) will decrease coverage by 0%.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #1853    +/-   ##
=======================================
- Coverage      87%     87%    -1%     
=======================================
  Files         937     935     -2     
  Lines       48868   48751   -117     
  Branches     6097    6081    -16     
=======================================
- Hits        42750   42586   -164     
- Misses       5111    5154    +43     
- Partials     1007    1011     +4     
Flag Coverage Δ
unittests 87% <100%> (-1%) ⬇️

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

Impacted Files Coverage Δ
src/ImageSharp/Formats/Jpeg/Components/Block8x8.cs 98% <ø> (+21%) ⬆️
...rc/ImageSharp/Formats/Jpeg/Components/Block8x8F.cs 91% <ø> (+5%) ⬆️
...rmats/Jpeg/Components/Decoder/SpectralConverter.cs 100% <ø> (ø)
...ents/Decoder/ColorConverters/JpegColorConverter.cs 94% <100%> (+<1%) ⬆️
...g/Components/Decoder/JpegComponentPostProcessor.cs 100% <100%> (+7%) ⬆️
...eg/Components/Decoder/SpectralConverter{TPixel}.cs 100% <100%> (ø)
...mats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs 96% <100%> (ø)
...nents/Encoder/LuminanceForwardConverter{TPixel}.cs 100% <100%> (ø)
.../Components/Encoder/RgbForwardConverter{TPixel}.cs 100% <100%> (ø)
...onents/Encoder/YCbCrForwardConverter420{TPixel}.cs 73% <100%> (ø)
... and 7 more

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 3a3059e...7d524bd. Read the comment docs.

@@ -1,23 +0,0 @@
// Copyright (c) Six Labors.
Copy link
Contributor

Choose a reason for hiding this comment

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

GenericBlock8x8.Generated.tt is an empty file -- should it be deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should, looks like I forgot about it, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might be a github diff bug or something but I couldn't find GenericBlock8x8.Generated.tt file in my repo:

image

Copy link
Member

Choose a reason for hiding this comment

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

Should be under Format\Jpeg\Components

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah but screen above is Format\Jpeg\Components, here's another one directly from VS:
image

Sorry for my agnorance but what those .Generated.tt files mean exactly? First time encountering those.

Copy link
Member

Choose a reason for hiding this comment

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

They're T4 templates generating partials of the named types. If you expand them you see the generated file. e.g. Block8x8F.Generated.cs.

That empty one looks like a leftover from previous cleanup and is safe to delete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Completely missed your response - thanks for clarifying .tt stuff! GenericBlock8x8.Generated.tt seems to be deleted now (I did nothing after this review, looks like a diff tool bug or whatever).

@br3aker br3aker force-pushed the dp/jpeg-decoder-cleanup branch from 57db167 to 717b166 Compare November 30, 2021 09:37
@br3aker br3aker marked this pull request as ready for review November 30, 2021 10:57
@br3aker
Copy link
Contributor Author

br3aker commented Nov 30, 2021

Current progress is big enough for review, there's still some work left - I will work on it as a separate PR.

@br3aker br3aker changed the title [WIP] Jpeg codebase cleanup Jpeg codebase cleanup Nov 30, 2021
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.

Nice job! I've been reviewing as you've been working through. Looks much better.

@br3aker br3aker deleted the dp/jpeg-decoder-cleanup branch November 30, 2021 12:03
@br3aker
Copy link
Contributor Author

br3aker commented Nov 30, 2021

Wow that was fast, thanks!

@JimBobSquarePants
Copy link
Member

I've been struggling to find something useful to do so have been focusing on keeping up with PRs.

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