Exif reader fixes

5 years ago · afa8a1b8b0
7 changed files with 94 additions and 46 deletions
--- a/src/ImageSharp/Metadata/Profiles/Exif/ExifReader.cs
+++ b/src/ImageSharp/Metadata/Profiles/Exif/ExifReader.cs
@ -5,6 +5,7 @@ using System;
 using System.Buffers.Binary;
 using System.Collections.Generic;
 using System.Collections.ObjectModel;
+using System.Diagnostics;
 using System.IO;
 using System.Runtime.CompilerServices;
 using System.Text;
@ -23,12 +24,17 @@ namespace SixLabors.ImageSharp.Metadata.Profiles.Exif
        private readonly byte[] buf2 = new byte[2];

        // used for sequential read big values (actual for multiframe big files)
-        private readonly SortedList<uint, Action> lazyLoaders = new SortedList<uint, Action>();
+        // 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>());

        private bool isBigEndian;

        private List<ExifTag> invalidTags;

+        private uint exifOffset = 0;
+
+        private uint gpsOffset = 0;
+
        public ExifReader(bool isBigEndian, Stream stream)
        {
            this.isBigEndian = isBigEndian;
@ -91,7 +97,6 @@ namespace SixLabors.ImageSharp.Metadata.Profiles.Exif
            }

            uint ifdOffset = this.ReadUInt32();
-
            this.AddValues(values, ifdOffset);

            uint thumbnailOffset = this.ReadUInt32();
@ -134,43 +139,15 @@ namespace SixLabors.ImageSharp.Metadata.Profiles.Exif

        protected void AddSubIfdValues(List<IExifValue> values)
        {
-            uint exifOffset = 0;
-            uint gpsOffset = 0;
-            foreach (IExifValue value in values)
-            {
-                if (value.Tag == ExifTag.SubIFDOffset)
-                {
-                    exifOffset = ((ExifLong)value).Value;
-                }
-
-                if (value.Tag == ExifTag.GPSIFDOffset)
-                {
-                    gpsOffset = ((ExifLong)value).Value;
-                }
-            }
-
-            if (exifOffset != 0)
-            {
-                this.AddValues(values, exifOffset);
-            }
-
-            if (gpsOffset != 0)
+            if (this.exifOffset != 0)
            {
-                this.AddValues(values, gpsOffset);
+                this.AddValues(values, this.exifOffset);
            }
-        }

-        private static bool IsDuplicate(IList<IExifValue> values, IExifValue value)
-        {
-            foreach (IExifValue val in values)
+            if (this.gpsOffset != 0)
            {
-                if (val == value)
-                {
-                    return true;
-                }
+                this.AddValues(values, this.gpsOffset);
            }
-
-            return false;
        }

        private static TDataType[] ToArray<TDataType>(ExifDataType dataType, ReadOnlySpan<byte> data, ConverterMethod<TDataType> converter)
@ -337,7 +314,6 @@ namespace SixLabors.ImageSharp.Metadata.Profiles.Exif
            }

            uint size = numberOfComponents * ExifDataTypes.GetSize(dataType);
-            object value = null;
            if (size > 4)
            {
                uint newIndex = this.ConvertToUInt32(this.offsetBuffer);
@ -349,29 +325,60 @@ namespace SixLabors.ImageSharp.Metadata.Profiles.Exif
                    return;
                }

+                if (this.lazyLoaders.ContainsKey(newIndex))
+                {
+                    Debug.WriteLine($"Duplicate offset: tag={tag}, size={size}, offset={newIndex}");
+                }
+
                this.lazyLoaders.Add(newIndex, () =>
                {
                    var dataBuffer = new byte[size];
                    this.Seek(newIndex);
                    if (this.TryReadSpan(dataBuffer))
                    {
-                        value = this.ConvertValue(dataType, dataBuffer, numberOfComponents);
-                    }
+                        object value = this.ConvertValue(dataType, dataBuffer, numberOfComponents);

-                    if (exifValue.TrySetValue(value) && !IsDuplicate(values, exifValue))
-                    {
-                        values.Add(exifValue);
+                        this.Add(values, exifValue, value);
                    }
                });
            }
            else
            {
-                value = this.ConvertValue(dataType, this.offsetBuffer, numberOfComponents);
+                object value = this.ConvertValue(dataType, this.offsetBuffer, numberOfComponents);
+
+                this.Add(values, exifValue, value);
+            }
+        }
+
+        private void Add(IList<IExifValue> values, IExifValue exif, object value)
+        {
+            if (!exif.TrySetValue(value))
+            {
+                return;
+            }
+
+            foreach (IExifValue val in values)
+            {
+                // sometimes duplicates appear,
+                // can compare val.Tag == exif.Tag
+                if (val == exif)
+                {
+                    Debug.WriteLine($"Duplicate Exif tag: tag={exif.Tag}, dataType={exif.DataType}");
+                    return;
+                }
            }

-            if (exifValue.TrySetValue(value) && !IsDuplicate(values, exifValue))
+            if (exif.Tag == ExifTag.SubIFDOffset)
+            {
+                this.exifOffset = (uint)value;
+            }
+            else if (exif.Tag == ExifTag.GPSIFDOffset)
+            {
+                this.gpsOffset = (uint)value;
+            }
+            else
            {
-                values.Add(exifValue);
+                values.Add(exif);
            }
        }

@ -387,12 +394,10 @@ namespace SixLabors.ImageSharp.Metadata.Profiles.Exif
            if (this.RemainingLength < length)
            {
                span = default;
-
                return false;
            }

            int readed = this.data.Read(span);
-
            return readed == length;
        }

@ -408,6 +413,11 @@ namespace SixLabors.ImageSharp.Metadata.Profiles.Exif

        private void GetThumbnail(uint offset)
        {
+            if (offset == 0)
+            {
+                return;
+            }
+
            var values = new List<IExifValue>();
            this.AddValues(values, offset);

@ -528,5 +538,19 @@ namespace SixLabors.ImageSharp.Metadata.Profiles.Exif
                ? BinaryPrimitives.ReadInt16BigEndian(buffer)
                : BinaryPrimitives.ReadInt16LittleEndian(buffer);
        }
+
+        /// <summary><see cref="DuplicateKeyComparer{TKey}"/> used for possiblity add a duplicate offsets (but tags don't duplicate).</summary>
+        /// <typeparam name="TKey">The type of the key.</typeparam>
+        public class DuplicateKeyComparer<TKey> : IComparer<TKey>
+            where TKey : IComparable
+        {
+            public int Compare(TKey x, TKey y)
+            {
+                int result = x.CompareTo(y);
+
+                // Handle equality as beeing greater
+                return (result == 0) ? 1 : result;
+            }
+        }
    }
 }
--- a/tests/ImageSharp.Tests/Formats/Tiff/TiffMetadataTests.cs
+++ b/tests/ImageSharp.Tests/Formats/Tiff/TiffMetadataTests.cs
@ -146,6 +146,8 @@ namespace SixLabors.ImageSharp.Tests.Formats.Tiff
                Assert.Equal(10, image.Metadata.VerticalResolution);

                TiffFrameMetadata frame = image.Frames.RootFrame.Metadata.GetTiffMetadata();
+                Assert.Equal(30, frame.FrameTags.Values.Count);
+
                Assert.Equal(32u, frame.Width);
                Assert.Equal(32u, frame.Height);
                Assert.Equal(new ushort[] { 4 }, frame.BitsPerSample);
@ -176,6 +178,8 @@ namespace SixLabors.ImageSharp.Tests.Formats.Tiff
                Assert.Equal(TiffPredictor.None, frame.Predictor);
                Assert.Null(frame.SampleFormat);
                Assert.Equal("This is Авторские права", frame.Copyright);
+                Assert.Equal(4, frame.FrameTags.GetValue<ushort>(ExifTag.Rating).Value);
+                Assert.Equal(75, frame.FrameTags.GetValue<ushort>(ExifTag.RatingPercent).Value);
            }
        }

--- a/tests/ImageSharp.Tests/Metadata/Profiles/Exif/ExifProfileTests.cs
+++ b/tests/ImageSharp.Tests/Metadata/Profiles/Exif/ExifProfileTests.cs
@ -6,6 +6,7 @@ using System.Collections.Generic;
 using System.IO;
 using System.Linq;
 using System.Text;
+
 using SixLabors.ImageSharp.Metadata;
 using SixLabors.ImageSharp.Metadata.Profiles.Exif;
 using SixLabors.ImageSharp.PixelFormats;
@ -14,6 +15,7 @@ using Xunit;

 namespace SixLabors.ImageSharp.Tests
 {
+    [Trait("Profile", "Exif")]
    public class ExifProfileTests
    {
        public enum TestImageWriteFormat
@ -201,10 +203,14 @@ namespace SixLabors.ImageSharp.Tests
            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));
+
            int profileCount = image.Metadata.ExifProfile.Values.Count;
            image = WriteAndRead(image, imageFormat);

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

            // Should be 3 less.
            // 1 x due to setting of null "ReferenceBlackWhite" value.
@ -363,8 +369,14 @@ namespace SixLabors.ImageSharp.Tests
            // Force parsing of the profile.
            Assert.Equal(25, profile.Values.Count);

+            // todo: duplicate tags (from root container and subIfd)
+            Assert.Equal(2, profile.Values.Count(v => (ExifTagValue)(ushort)v.Tag == ExifTagValue.DateTime));
+
            byte[] bytes = profile.ToByteArray();
            Assert.Equal(525, bytes.Length);
+
+            var profile2 = new ExifProfile(bytes);
+            Assert.Equal(25, profile2.Values.Count);
        }

        [Theory]
@ -477,6 +489,9 @@ namespace SixLabors.ImageSharp.Tests
        {
            Assert.NotNull(profile);

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

            foreach (IExifValue value in profile.Values)
--- a/tests/ImageSharp.Tests/Metadata/Profiles/Exif/ExifReaderTests.cs
+++ b/tests/ImageSharp.Tests/Metadata/Profiles/Exif/ExifReaderTests.cs
@ -8,6 +8,7 @@ using Xunit;

 namespace SixLabors.ImageSharp.Tests
 {
+    [Trait("Profile", "Exif")]
    public class ExifReaderTests
    {
        [Fact]
--- a/tests/ImageSharp.Tests/Metadata/Profiles/Exif/ExifTagDescriptionAttributeTests.cs
+++ b/tests/ImageSharp.Tests/Metadata/Profiles/Exif/ExifTagDescriptionAttributeTests.cs
@ -6,6 +6,7 @@ using Xunit;

 namespace SixLabors.ImageSharp.Tests
 {
+    [Trait("Profile", "Exif")]
    public class ExifTagDescriptionAttributeTests
    {
        [Fact]
--- a/tests/ImageSharp.Tests/Metadata/Profiles/Exif/ExifValueTests.cs
+++ b/tests/ImageSharp.Tests/Metadata/Profiles/Exif/ExifValueTests.cs
@ -7,6 +7,7 @@ using Xunit;

 namespace SixLabors.ImageSharp.Tests
 {
+    [Trait("Profile", "Exif")]
    public class ExifValueTests
    {
        private ExifProfile profile;
--- a/tests/ImageSharp.Tests/Metadata/Profiles/Exif/Values/ExifValuesTests.cs
+++ b/tests/ImageSharp.Tests/Metadata/Profiles/Exif/Values/ExifValuesTests.cs
@ -6,6 +6,7 @@ using Xunit;

 namespace SixLabors.ImageSharp.Tests.Metadata.Profiles.Exif.Values
 {
+    [Trait("Profile", "Exif")]
    public class ExifValuesTests
    {
        public static TheoryData<ExifTag> ByteTags => new TheoryData<ExifTag>
@ -94,6 +95,7 @@ namespace SixLabors.ImageSharp.Tests.Metadata.Profiles.Exif.Values
        public static TheoryData<ExifTag> NumberArrayTags => new TheoryData<ExifTag>
        {
            { ExifTag.StripOffsets },
+            { ExifTag.StripByteCounts },
            { ExifTag.TileByteCounts },
            { ExifTag.ImageLayer }
        };
@ -160,6 +162,7 @@ namespace SixLabors.ImageSharp.Tests.Metadata.Profiles.Exif.Values
            { ExifTag.Orientation },
            { ExifTag.SamplesPerPixel },
            { ExifTag.PlanarConfiguration },
+            { ExifTag.Predictor },
            { ExifTag.GrayResponseUnit },
            { ExifTag.ResolutionUnit },
            { ExifTag.CleanFaxData },
@ -208,7 +211,6 @@ namespace SixLabors.ImageSharp.Tests.Metadata.Profiles.Exif.Values
            { ExifTag.ExtraSamples },
            { ExifTag.PageNumber },
            { ExifTag.TransferFunction },
-            { ExifTag.Predictor },
            { ExifTag.HalftoneHints },
            { ExifTag.SampleFormat },
            { ExifTag.TransferRange },