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

Support frames metadata for Identify #2363

Merged
merged 20 commits into from
Mar 1, 2023

Conversation

IldarKhayrutdinov
Copy link
Contributor

@IldarKhayrutdinov IldarKhayrutdinov commented Feb 21, 2023

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 for #2286

  1. Add reading of frames metadata to TiffMetadata for Identify.
  2. Add InkSet property to TiffFrameMetadata, for identification of CMYK images.
  3. Minor deep clonig fixes for TiffMetadata.

/cc @brianpopow

Update.

This PR has been expanded to add a new IReadOnlyList<ImageFrameMetadata> FrameMetadataCollection to ImageInfo and updated the gif decoder to also populate this property.

@IldarKhayrutdinov
Copy link
Contributor Author

I’m sorry I’m late.
I still need to add more tests.

@JimBobSquarePants I make without a global change of API, affecting only tiff metadata. At the moment, it is relevant only for tiff images. There is a way to add frames metadata into ImageMetadata.

/// <value>
/// The frames.
/// </value>
public IReadOnlyList<TiffFrameMetadata> Frames { get; set; } = Array.Empty<TiffFrameMetadata>();
Copy link
Member

Choose a reason for hiding this comment

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

If you look at how this is done with our Gif format you will see that the individual frame metadata is not a property of the main image one.

They're stored in a dictionary within individual ImageFrameMetadata instances.

Copy link
Member

Choose a reason for hiding this comment

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

Just realized what you mean by this.

I've pulled your code down and will work on a refactor to add the info to ImageInfo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've pulled your code down and will work on a refactor to add the info to ImageInfo

yes, this way looks more mature

@JimBobSquarePants JimBobSquarePants changed the title TIFF: Support frames metadata for Identify Support frames metadata for Identify Feb 26, 2023
@JimBobSquarePants JimBobSquarePants marked this pull request as ready for review February 26, 2023 11:11
@JimBobSquarePants
Copy link
Member

@IldarKhayrutdinov I've updated your PR to add support for Gif also. Thanks for making a start on this, it made the feature much, much easier to implement!

@JimBobSquarePants JimBobSquarePants added formats:gif area:metadata breaking Signifies a binary breaking change. labels Feb 26, 2023
@JimBobSquarePants JimBobSquarePants added this to the 3.0.0 milestone Feb 26, 2023
/// </summary>
/// <param name="minCodeSize">Minimum code size of the data.</param>
/// <param name="length">The resulting index table length.</param>
public void SkipIndices(int minCodeSize, int length)
Copy link
Member

Choose a reason for hiding this comment

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

Gif doesn't tell you in advance the length of the combined LZW segment blocks. You have to read each one which must be decoded to read the next one.

// Interpret the code
if (code > availableCode || code == endCode)
{
break;
Copy link
Member

Choose a reason for hiding this comment

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

This break here is the killer. A lot of properties we require for each loop follow.

/// <summary>
/// Extension methods for <see cref="ImageFrameCollection{TPixel}"/>.
/// </summary>
public static class ImageFrameCollectionExtensions
Copy link
Member

Choose a reason for hiding this comment

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

This is required because ImageFrameCollection<TPixel> implements both IEnumerable<ImageFrame> via inheritance and IEnumerable<ImageFrame<TPixel>>. You have to awkwardly cast to use Select and Where etc.

Select can easily be pollyfilled but an attempt to polyfill Where or other members of System.Linq will clash to it's easier to add a single AsEnumerable<TPixel> method which can then be chained against for the System.Linq methods.

@IldarKhayrutdinov
Copy link
Contributor Author

@IldarKhayrutdinov I've updated your PR to add support for Gif also. Thanks for making a start on this, it made the feature much, much easier to implement!

I don't have time, thanks for finishing!

/// <summary>
/// 32 bits per pixel. One byte for each color channel.
/// </summary>
Bit32 = 32,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added value for 32 bit

Copy link
Member

Choose a reason for hiding this comment

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

Ace thanks!
I've also added Bit64 and wired the encoder to default to Bit32 if that is given.

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 think this is good to go!

@JimBobSquarePants JimBobSquarePants merged commit 6ec1692 into SixLabors:main Mar 1, 2023
@IldarKhayrutdinov IldarKhayrutdinov deleted the tiff-frames-meta branch March 1, 2023 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants