Skip to content

Conversation

@brianpopow
Copy link
Collaborator

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

This PR adds support for decoding tiff image with ccitt T.6 fax compression (some call the compression fax4).

For the specification see pdf T-REC-T.6-198811 in the Tiff folder.

@codecov
Copy link

codecov bot commented Aug 22, 2021

Codecov Report

Merging #1747 (89548fe) into master (5d82124) will increase coverage by 0.00%.
The diff coverage is 86.41%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #1747    +/-   ##
========================================
  Coverage   84.44%   84.44%            
========================================
  Files         836      841     +5     
  Lines       36748    37006   +258     
  Branches     4306     4343    +37     
========================================
+ Hits        31032    31251   +219     
- Misses       4883     4903    +20     
- Partials      833      852    +19     
Flag Coverage Δ
unittests 84.44% <86.41%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...ompression/Decompressors/DeflateTiffCompression.cs 96.00% <ø> (ø)
...ff/Compression/Decompressors/LzwTiffCompression.cs 100.00% <ø> (ø)
...mpression/Decompressors/PackBitsTiffCompression.cs 90.90% <ø> (ø)
...mageSharp/Formats/Tiff/TiffDecoderOptionsParser.cs 64.41% <66.66%> (+0.04%) ⬆️
...pression/Decompressors/ModifiedHuffmanBitReader.cs 73.33% <73.33%> (ø)
...ompression/Decompressors/CcittReferenceScanline.cs 78.43% <78.43%> (ø)
...on/Decompressors/ModifiedHuffmanTiffCompression.cs 93.33% <81.81%> (-6.67%) ⬇️
...iff/Compression/Decompressors/T6TiffCompression.cs 85.24% <85.24%> (ø)
...mats/Tiff/Compression/Decompressors/T6BitReader.cs 88.52% <88.52%> (ø)
...mats/Tiff/Compression/Decompressors/T4BitReader.cs 95.48% <91.15%> (+1.18%) ⬆️
... and 11 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 5d82124...89548fe. Read the comment docs.

@brianpopow brianpopow requested a review from a team August 23, 2021 10:42
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.

I won't lie, I did not read the specification. 😝

Can't see any code issues though, just a nice to have and a question.

{
internal enum CcittTwoDimensionalCodeType
{
None = 0,
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to have enumerations like this documented even if they're internal.

/// <returns>The value read.</returns>
protected uint ReadValue(int nBits)
{
Guard.MustBeGreaterThan(nBits, 0, nameof(nBits));
Copy link
Member

Choose a reason for hiding this comment

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

How often is this called. The Guard might be a performance killer

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

its called very often, its DebugGuard now

/// <summary>
/// Class to handle cases where TIFF image data is compressed using CCITT T6 compression.
/// </summary>
internal class T6TiffCompression : TiffBaseDecompressor
Copy link
Member

Choose a reason for hiding this comment

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

If this can be sealed let's make a habit of doing so.

/// <returns>Position of b1.</returns>
public int FindB1(int a0, byte a0Byte)
{
if (this.scanLine.IsEmpty)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

Suggested change
if (this.scanLine.IsEmpty)
if (this.IsEmpty)

if the property is at hands?

/// <returns>Position of b1.</returns>
public int FindB2(int b1)
{
if (this.scanLine.IsEmpty)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

brianpopow and others added 3 commits August 24, 2021 12:20
@brianpopow brianpopow added this to the 2.0.0 milestone Aug 24, 2021
@brianpopow brianpopow merged commit ce97da5 into master Aug 24, 2021
@brianpopow brianpopow deleted the bp/tiff-fax4 branch August 24, 2021 11:22
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