V3 : Decode Chunky Tile Rows Directly.

[JimBobSquarePants]

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

Fixes #2873
Avoids the integer overflow issue by removing the interim allocation completely and decoding directly to the pixel buffer.

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/ImageSharp/Formats/Tiff/TiffDecoderCore.cs:611

• Corrected spelling from 'Tiff's' to 'TIFFs'.

/// Decodes the image data for TIFFs which arrange the pixel data in tiles and the chunky configuration.

[stefannikolei]

@JimBobSquarePants Look at #2855 i fixed the build there. And just checked it still works

[JimBobSquarePants]

@JimBobSquarePants Look at #2855 I fixed the build there. And just checked it still works

Ah no, it's something else. I've already done that using the following which covers all cases.

if: ${{ contains(matrix.options.os, 'ubuntu') }}

What I'm seeing is a difference in behavior with dotnet build between old builds and now. The older SDKs are no longer installed on the image by default and after installing it seems to throw errors for stuff that did not throw before.

It's a proper head scratcher!

[stefannikolei]

@JimBobSquarePants Look at #2855 I fixed the build there. And just checked it still works

Ah no, it's something else. I've already done that using the following which covers all cases.

if: ${{ contains(matrix.options.os, 'ubuntu') }}

What I'm seeing is a difference in behavior with dotnet build between old builds and now. The older SDKs are no longer installed on the image by default and after installing it seems to throw errors for stuff that did not throw before.

It's a proper head scratcher!

I saw that. But I just triggered a build in my branch and I got no build errors oO

[JimBobSquarePants]

@JimBobSquarePants Look at #2855 I fixed the build there. And just checked it still works

Ah no, it's something else. I've already done that using the following which covers all cases.

if: ${{ contains(matrix.options.os, 'ubuntu') }}

What I'm seeing is a difference in behavior with dotnet build between old builds and now. The older SDKs are no longer installed on the image by default and after installing it seems to throw errors for stuff that did not throw before.
It's a proper head scratcher!

I saw that. But I just triggered a build in my branch and I got no build errors oO

I'm running a build against the legacy release/v3.1.x branch. That targets .NET 6 and 7. The main branch targets .NET 8 and 9 both of which are installed on the image.

[stefannikolei]

I'm running a build against the legacy release/v3.1.x branch. That targets .NET 6 and 7. The main branch targets .NET 8 and 9 both of which are installed on the image.

Ah missed that one.. Was wondering why dotnet-setup was bumped.

Perhaps you can try to not go agains ubuntu-latest instead use ubuntu-22-04 (libgdi was installed on this one) or the version it worked the last time

[JimBobSquarePants]

@stefannikolei it works if you install NET 8 and NET 6!! 🤷I'll just update the script to do that.

[stefannikolei]

@stefannikolei it works if you install NET 8 and NET 6!! 🤷I'll just update the script to do that.

just checked which version of ubuntu was used

But when it works it works. Perhaps ubuntu-latest should not be used for the future. Instead use a explicit version, perhaps those issue won't come then.

[JimBobSquarePants]

@stefannikolei it works if you install NET 8 and NET 6!! 🤷I'll just update the script to do that.

just checked which version of ubuntu was used

But when it works it works. Perhaps ubuntu-latest should not be used for the future. Instead use a explicit version, perhaps those issue won't come then.

I would but they change old images also for some reason I don't understand.

actions/runner-images#11503

I think whatever issues there are have been fixed for .NET 8+ so going forward it hopefully should not be a problem.

[brianpopow]

looks good 👍

[aevitas]

Very much looking forward to these changes!

Curious, why are test runners using versions of .NET that are out of support, but not the most recent version? I could see the regressions argument, but arguably no one should be running EOL versions of .NET to begin with.

[brianpopow]

@aevitas while the image mentioned in Discussion 2870 can be decoded, it still suffers from the same issue mentioned in #2877. I am working on fixing that, too.

[JimBobSquarePants]

@brianpopow do you want to use this branch?

If you can tell me where the issue manifests maybe I can have a look if you’re stuck?

[brianpopow]

@brianpopow do you want to use this branch?

@JimBobSquarePants I would suggest to first merge this branch into main and continue on a new branch, since it is a different issue.

If you can tell me where the issue manifests maybe I can have a look if you’re stuck?

I think, I know what needs to be done. The horizontal predictor is reversed by applying it to each row of the image, but for tiled images, it needs to be applied to each row of the tile.

[brianpopow]

@JimBobSquarePants I think I made a mistake with merging this. I thought this was going into the main branch.

[JimBobSquarePants]

@brianpopow All good. I wanted to target v3 since we are still a long way from a v4 launch. I'll open a second PR for v4.