#12 Use exif reader for tiff tags reading #1494

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

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

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

src/ImageSharp/Formats/Tiff/Ifd/DirectoryReader.cs

JimBobSquarePants

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

src/ImageSharp/Formats/Tiff/TiffFrameMetadata.cs

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

src/ImageSharp/Formats/Tiff/TiffFrameMetadataExtensions.cs

@@ -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. 👍

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

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

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

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

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

        }

+        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

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

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

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

        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);

tests/ImageSharp.Tests/Formats/Tiff/TiffMetadataTests.cs

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

src/ImageSharp/Formats/Tiff/Constants/TiffSubfileType.cs

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

src/ImageSharp/Metadata/Profiles/Exif/Tags/ExifTag.Number.cs

+        /// <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

src/ImageSharp/Metadata/Profiles/Exif/Tags/ExifTag.NumberArray.cs

src/ImageSharp/Metadata/Profiles/Exif/Tags/ExifTag.Short.cs

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

src/ImageSharp/Formats/Tiff/TiffDecoderMetadataCreator.cs

@@ -54,15 +54,6 @@ public static ImageMetadata Create(List<TiffFrameMetadata> frames, bool ignoreMe
                        }
                    }

-                    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

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

+
+        // 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

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

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

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

@@ -201,10 +203,14 @@ public void SetValue(TestImageWriteFormat imageFormat)
            IExifValue<Rational[]> latitude = image.Metadata.ExifProfile.GetValue(ExifTag.GPSLatitude);
            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

src/ImageSharp/Formats/Tiff/Ifd/DirectoryReader.cs

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

src/ImageSharp/Formats/Tiff/Ifd/EntryReader.cs

-                    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

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)

#12 Use exif reader for tiff tags reading #1494

#12 Use exif reader for tiff tags reading #1494

Conversation

IldarKhayrutdinov commented Jan 6, 2021 • edited Loading

Prerequisites

Description

brianpopow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimBobSquarePants left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimBobSquarePants Jan 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IldarKhayrutdinov Jan 25, 2021 • edited Loading

Choose a reason for hiding this comment

IldarKhayrutdinov commented Jan 12, 2021 • edited Loading

brianpopow commented Jan 12, 2021 • edited Loading

IldarKhayrutdinov commented Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IldarKhayrutdinov Jan 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IldarKhayrutdinov commented Jan 12, 2021

JimBobSquarePants commented Jan 19, 2021

IldarKhayrutdinov commented Jan 19, 2021

IldarKhayrutdinov commented Jan 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IldarKhayrutdinov Jan 26, 2021 • edited Loading

Choose a reason for hiding this comment

brianpopow left a comment

Choose a reason for hiding this comment

JimBobSquarePants commented Jan 29, 2021

JimBobSquarePants commented Jan 29, 2021

IldarKhayrutdinov commented Jan 29, 2021

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