#12 Use exif reader for tiff tags reading by IldarKhayrutdinov · Pull Request #1494 · SixLabors/ImageSharp

IldarKhayrutdinov · 2021-01-06T16:42:49Z

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 matches 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

Remove duplicate code, use exif reader for tiff tags reading
Correct TrySetValue method for ExifNumberArray
Minor fixes for exif values and exif reader
Duplicate tags problem report

I was afraid to make a lot of changes to exif classes, so it turned out a little clumsy. Don't hesitate to write comments 😉
Tests are works correctly and successful.
The logic of Exif reading has not changed.
The difficulty was in supporting multi-page Tiff files, where IDF tags and their extended data (> 4 bytes) can be scattered throughout the file in no particular order, and file can be very large.

brianpopow

@IldarKhayrutdinov thanks once again for your help.

brianpopow · 2021-01-10T14:07:58Z

-                return false;
+                // sometimes duplicates appear,
+                // can compare val.Tag == exif.Tag
+                if (val == exif)


this is comparing by reference, this seems like a mistake.

yeah, this is suspicious, but it used to work like this:

ImageSharp/src/ImageSharp/Metadata/Profiles/Exif/ExifReader.cs

Line 156 in 9f54406

if (val == value)

JimBobSquarePants

Thanks! Definitely a step in the right direction! I've picked up on a few things though to revisit.

JimBobSquarePants · 2021-01-08T04:29:02Z

@@ -31,7 +31,7 @@ public static T[] GetArray<T>(this TiffFrameMetadata meta, ExifTag tag, bool opt
        public static bool TryGetArray<T>(this TiffFrameMetadata meta, ExifTag tag, out T[] result)


I'm not a fan of all these extension methods. ExifProfile has strong type (Get/Set)Value methods already.

right, now can remove or simplify these methods

@JimBobSquarePants I got rid of some methods, but most remained, they are useful as wrappers for ExifProfile methods - allow you to cast types for properties

allow you to cast types for properties

I don't understand. The values returned by the existing method are already strong typed to the correct type without the need for internal calls and casting.

For example. This method is called upstream by the following in TiffDecoderMetadataCreator.

byte[] buf = frame.GetArray<byte>(ExifTag.XMP, true); if (buf != null) { tiffMetadata.XmpProfile = buf; }

However, this could easily have been.

IExifValue<byte[]> xmp = frame.ExifProfile.GetValue(ExifTag.XMP); if (xmp != null) { tiffMetadata.XmpProfile = xmp.Value; }

which uses the existing methods removing any code duplication and is better for someone reading the code since it indicates exactly where the metadata is coming from.

I do wish that ExifProfile.GetValue was ExifProfile.TryGetValue but we can't fix that without introducing a breaking change (I could be convinced to do that though).

The same duplication is happening in the resolution getter/setter code.

We have existing methods in jpeg GetExifResolutionValue that demonstrate a much more simple approach using existing APIs.

@JimBobSquarePants yes, you are right, it can be simplified even more, I did it

Ace... I'll give it another review tonight. 👍

JimBobSquarePants · 2021-01-08T04:32:29Z

+
+        private uint gpsOffset = 0;
+
+        public ExifReader(bool isBigEndian, Stream stream)


I think these parameters should be reversed.

yeah, it looks more logical

JimBobSquarePants · 2021-01-08T04:33:16Z

        }

+        public ExifReader(byte[] exifData) =>
+            this.data = new MemoryStream(exifData ?? throw new ArgumentNullException(nameof(exifData)));


What about endianness?

endianness will readed later:

ImageSharp/src/ImageSharp/Metadata/Profiles/Exif/ExifReader.cs

Line 74 in 9f54406

this.isBigEndian = this.ReadUInt16() != 0x4949;

the class turned out to be a little universal, it can make 2 inheritor for tiff and exif, what do you think?

@JimBobSquarePants it seems, worth to make 2 inheritor for tiff and exif, it will look better

@JimBobSquarePants please review again, I split the classes and they seems to be looking better now

JimBobSquarePants · 2021-01-08T04:43:05Z

+
        public override IExifValue DeepClone() => new ExifNumberArray(this);
+
+        private bool SetSingle<T>(T value, Func<T, Number> converter)


TrySetSingle
TrySetArray

These methods return the value for simplicity case construction, they will always return true:

ImageSharp/src/ImageSharp/Metadata/Profiles/Exif/Values/ExifNumberArray.cs

Line 51 in 4676084

return this.SetSingle(val, v => (Number)v);

IldarKhayrutdinov · 2021-01-12T18:40:37Z

@JimBobSquarePants @brianpopow I can't reply to your comments, maybe it's because of Assignees

brianpopow · 2021-01-12T18:44:21Z

@JimBobSquarePants @brianpopow I can't reply to your comments, maybe it's because of Assignees

i dont know why that is. I have removed myself from assignees. Can you try again @IldarKhayrutdinov ?

IldarKhayrutdinov · 2021-01-12T19:39:54Z

strange, it doesn't work

IldarKhayrutdinov · 2021-01-06T16:47:50Z

+        /// <summary>
+        /// Gets the RowsPerStrip exif tag.
+        /// </summary>
+        public static ExifTag<Number> RowsPerStrip { get; } = new ExifTag<Number>(ExifTagValue.RowsPerStrip);


RowsPerStrip dataType is short or long - https://www.awaresystems.be/imaging/tiff/tifftags/rowsperstrip.html

IldarKhayrutdinov · 2021-01-07T08:17:45Z


-                    if (coreMetadata.ExifProfile == null)
-                    {
-                        byte[] buf = frame.GetArray<byte>(ExifTag.SubIFDOffset, true);


now SubIFD and subGps are read to common TiffFrameMetaData.FrameTags list

IldarKhayrutdinov · 2021-01-07T08:22:19Z

+
+        // used for sequential read big values (actual for multiframe big files)
+        // todo: different tags can link to the same data (stream offset) - investigate
+        private readonly SortedList<uint, Action> lazyLoaders = new SortedList<uint, Action>(new DuplicateKeyComparer<uint>());


DuplicateKeyComparer used for possiblity add a duplicate offsets (but tags don't duplicate)

IldarKhayrutdinov · 2021-01-07T11:34:32Z

+            foreach (IExifValue val in values)
            {
-                return false;
+                // sometimes duplicates appear,


note, I added a reproduction of the problem with duplicates, these tests work successfully:

ImageSharp/tests/ImageSharp.Tests/Metadata/Profiles/Exif/ExifProfileTests.cs

Line 207 in 4676084

Assert.Equal(2, image.Metadata.ExifProfile.Values.Count(v => (ushort)v.Tag == 59932));

ImageSharp/tests/ImageSharp.Tests/Metadata/Profiles/Exif/ExifProfileTests.cs

Line 373 in 4676084

Assert.Equal(2, profile.Values.Count(v => (ExifTagValue)(ushort)v.Tag == ExifTagValue.DateTime));

ImageSharp/tests/ImageSharp.Tests/Metadata/Profiles/Exif/ExifProfileTests.cs

Line 493 in 4676084

Assert.Equal(2, profile.Values.Count(v => (ushort)v.Tag == 59932));

IldarKhayrutdinov · 2021-01-07T11:38:01Z

            Assert.Equal(expectedLatitude, latitude.Value);

+            // todo: duplicate tags
+            Assert.Equal(2, image.Metadata.ExifProfile.Values.Count(v => (ushort)v.Tag == 59932));


note: this PR doesn't affected, it was earlier

IldarKhayrutdinov · 2021-01-12T19:42:51Z

strange, it doesn't work

happened!

JimBobSquarePants · 2021-01-19T10:35:36Z

Did you mean to close this? I thought it just needed a little work.

IldarKhayrutdinov · 2021-01-19T10:44:32Z

Did you mean to close this? I thought it just needed a little work.

sorry, I myself did not understand how it happened

IldarKhayrutdinov · 2021-01-25T19:30:21Z

@JimBobSquarePants @brianpopow I pushed some small improvements, please review. Are you changes requested?

JimBobSquarePants · 2021-01-25T23:30:47Z

+        private static ByteOrder ReadByteOrder(Stream stream)
        {
-            if (this.ReadHeader())
+            var headerBytes = new byte[2];


Stackalloc span?

sorry, do you mean doing new Span(stackalloc byte[2]) ?

JimBobSquarePants · 2021-01-25T23:33:34Z

-                    return null;
-            }
-        }
+        protected override void RegisterExtLoader(uint offset, Action reader) => throw new NotImplementedException();


I'm assuming this is a feature waiting to be implemented?

The HeaderReader class use one time to read only header. I changed exception type to NotSupported.
I tried to make fewer changes within the PR.
It was possible to move the stream reading methods into separate classes (as was in tiff), this would remove HeaderReader class and slightly simplify the exif/tiff readers.

brianpopow

I think this can be merged now, but i leave the final verdict to @JimBobSquarePants

JimBobSquarePants · 2021-01-29T20:10:46Z

@brianpopow if you’re happy, I’m happy.

JimBobSquarePants · 2021-01-29T20:11:51Z

Thanks @IldarKhayrutdinov !!

IldarKhayrutdinov · 2021-01-29T20:24:40Z

It was crazy PR 🙃

#12 Use exif reader for tiff tags reading

IldarKhayrutdinov added 2 commits December 27, 2020 10:29

Use ExifProfile for tiff frame metadata

5323bb6

Use exif reader for tiff tags reading, minor fixes for exif values.

d8c5ee5

IldarKhayrutdinov force-pushed the tiff-format branch from d608688 to f9b1a27 Compare January 7, 2021 08:27

Exif reader fixes

acf5f3b

IldarKhayrutdinov force-pushed the tiff-format branch from f9b1a27 to acf5f3b Compare January 7, 2021 11:08

IldarKhayrutdinov marked this pull request as ready for review January 7, 2021 11:39

IldarKhayrutdinov mentioned this pull request Jan 7, 2021

Tiff format support #1456

Closed

17 tasks done

brianpopow self-assigned this Jan 8, 2021

brianpopow requested changes Jan 10, 2021

View reviewed changes

JimBobSquarePants requested changes Jan 10, 2021

View reviewed changes

brianpopow removed their assignment Jan 12, 2021

IldarKhayrutdinov marked this pull request as draft January 12, 2021 19:28

IldarKhayrutdinov marked this pull request as ready for review January 12, 2021 19:29

IldarKhayrutdinov commented Jan 12, 2021

View reviewed changes

brianpopow added the formats:tiff label Jan 14, 2021

IldarKhayrutdinov closed this Jan 19, 2021

IldarKhayrutdinov reopened this Jan 19, 2021

Exif/Tiff readers improvements

4676084

IldarKhayrutdinov requested review from JimBobSquarePants and brianpopow January 25, 2021 19:20

JimBobSquarePants reviewed Jan 25, 2021

View reviewed changes

Use directly ExifProfile methods instead TiffFrameMetadataExtensions

1ef78ce

Change exception type

327de4e

IldarKhayrutdinov requested a review from JimBobSquarePants January 26, 2021 07:53

brianpopow and others added 2 commits January 29, 2021 19:32

Merge branch 'tiff-format' into tiff-format

f67398b

entries.FrameTags => entries.ExifProfile

3505322

brianpopow approved these changes Jan 29, 2021

View reviewed changes

JimBobSquarePants approved these changes Jan 29, 2021

View reviewed changes

JimBobSquarePants merged commit 1498dfa into SixLabors:tiff-format Jan 29, 2021

IldarKhayrutdinov deleted the tiff-format branch January 29, 2021 20:14

JimBobSquarePants added a commit that referenced this pull request Feb 17, 2021

Merge pull request #1494 from IldarKhayrutdinov/tiff-format

ae14b08

#12 Use exif reader for tiff tags reading

JimBobSquarePants added a commit that referenced this pull request Feb 17, 2021

Merge pull request #1494 from IldarKhayrutdinov/tiff-format

9c62ab1

#12 Use exif reader for tiff tags reading

		@@ -31,7 +31,7 @@ public static T[] GetArray<T>(this TiffFrameMetadata meta, ExifTag tag, bool opt
		public static bool TryGetArray<T>(this TiffFrameMetadata meta, ExifTag tag, out T[] result)


		private uint gpsOffset = 0;

		public ExifReader(bool isBigEndian, Stream stream)


		public override IExifValue DeepClone() => new ExifNumberArray(this);

		private bool SetSingle<T>(T value, Func<T, Number> converter)

Conversation

IldarKhayrutdinov commented Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Prerequisites

Description

Uh oh!

brianpopow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JimBobSquarePants left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JimBobSquarePants Jan 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IldarKhayrutdinov Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

IldarKhayrutdinov commented Jan 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brianpopow commented Jan 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IldarKhayrutdinov commented Jan 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IldarKhayrutdinov Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

IldarKhayrutdinov commented Jan 6, 2021 •

edited

Loading

JimBobSquarePants left a comment •

edited

Loading

JimBobSquarePants Jan 26, 2021 •

edited

Loading

IldarKhayrutdinov Jan 25, 2021 •

edited

Loading

IldarKhayrutdinov commented Jan 12, 2021 •

edited

Loading

brianpopow commented Jan 12, 2021 •

edited

Loading

IldarKhayrutdinov commented Jan 12, 2021 •

edited

Loading

IldarKhayrutdinov Jan 7, 2021 •

edited

Loading

IldarKhayrutdinov Jan 26, 2021 •

edited

Loading