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

WIP: Undo horizontal prediction for each tile row in case of tiled tiff's #2878

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

brianpopow
Copy link
Collaborator

@brianpopow brianpopow commented Feb 2, 2025

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 fixes issue: #2877: The horizontal predictor needs to be reversed for each tile row in case of tiled images.

Note: This is still work in progress.

@@ -712,6 +713,13 @@ private void DecodeTilesChunky<TPixel>(
{
Span<byte> uncompressedPixelRow = uncompressedPixelBufferSpan.Slice(uncompressedPixelBufferOffset, bytesToCopy);
tileBufferSpan.Slice(tileBufferOffset, bytesToCopy).CopyTo(uncompressedPixelRow);

// Undo the horziontal predictor for each tile row.
if (this.Predictor == TiffPredictor.Horizontal)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@JimBobSquarePants I am not sure, if this is the best approach to undo the predictor here. I tried to do this in HorizontalPredictor.Undo(), but could not figure out a way without replicating the logic to iterate over the tiles in DecodeTilesChunky(). Maybe you have a better idea howto do this?

Copy link
Member

Choose a reason for hiding this comment

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

So, are you writing the row, then undoing the write?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It currently works like this:

  1. For each Tile: Decompress tile data
  2. For each row in tile: Undo horizontal predictor
  3. When done with all tiles: decode color

The horizontal predictor is used with lzw and deflate compression to achieve better compression. It assumes that neighboring pixel values do not change much and therefore only stores the difference between two consecutive pixels instead the actual pixel values.

Because it is tight to lzw and deflate compression, it feels to me this should be the concern of the decompressor and not be handled outside (which is the case for none tiled images).

I will try to gather more tiled image test cases for all the different color formats and then see if I can figure out a way how I can move the undoing of the predictor inside the uncompressing method.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I understand.

I'm just trying to read through TiffLibrary and LibTiff.NET to see how it works there as a reference. I agree the decompressor feels like it should be responsible though.

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.

2 participants