EXIF Support for PNG's #616

brianpopow · 2018-06-16T14:34:11Z

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

This MR adds reading and writing eXIf chunks for PNG files. The basic difference between JPG's and PNG's EXIF is, that PNG does not include the Exif code (e. g. "Exif00") in the exif chunk.

I had to change the ExifWriter a little for that. The GetData method now has a parameter, which indicates, if the Exif Code should be included (defaults to true).

Please review the changes and let me know what you think.

…present (SixLabors#611)

…er reloading it, if the profile is still there (SixLabors#611)

… the ExifChunk - changed the ExifWriter GetData, to support the special case for PNG's not inlcluding the 'Exif' Code (SixLabors#611)

codecov · 2018-06-16T15:26:50Z

Codecov Report

Merging #616 into master will increase coverage by <.01%.
The diff coverage is 96.05%.

@@            Coverage Diff             @@
##           master     #616      +/-   ##
==========================================
+ Coverage   89.78%   89.79%   +<.01%     
==========================================
  Files         889      889              
  Lines       37529    37614      +85     
  Branches     2462     2468       +6     
==========================================
+ Hits        33697    33777      +80     
- Misses       3127     3133       +6     
+ Partials      705      704       -1

Impacted Files	Coverage Δ
...rocessing/Processors/Transforms/AutoOrientTests.cs	`100% <ø> (ø)`	⬆️
...Formats/Jpeg/Components/Decoder/ProfileResolver.cs	`100% <ø> (ø)`	⬆️
...ts/ImageSharp.Tests/MetaData/ImageMetaDataTests.cs	`100% <ø> (ø)`	⬆️
src/ImageSharp/MetaData/Profiles/ICC/IccProfile.cs	`90% <ø> (-0.55%)`	⬇️
.../ImageSharp.Tests/Formats/Png/PngChunkTypeTests.cs	`100% <100%> (ø)`	⬆️
...rc/ImageSharp/MetaData/Profiles/Exif/ExifReader.cs	`70.93% <100%> (-2.27%)`	⬇️
...ImageSharp/MetaData/Profiles/Exif/ExifConstants.cs	`100% <100%> (ø)`	⬆️
...c/ImageSharp/MetaData/Profiles/Exif/ExifProfile.cs	`87.2% <100%> (ø)`	⬆️
...rc/ImageSharp/MetaData/Profiles/Exif/ExifWriter.cs	`84.82% <100%> (+0.32%)`	⬆️
src/ImageSharp/Formats/Png/PngEncoderCore.cs	`85.5% <100%> (+0.63%)`	⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2105e4...6c777d4. Read the comment docs.

JimBobSquarePants · 2018-06-17T06:40:09Z

I'll leave this one for @dlemstra to review but I'm thinking we should make the EXIF00 bytes the sole responsibility of the jpeg format if it's unused in png.

dlemstra · 2018-06-17T07:55:10Z

src/ImageSharp/MetaData/Profiles/Exif/ExifProfile.cs

        /// <returns>The <see cref="T:byte[]"/></returns>
-        public byte[] ToByteArray()
+        public byte[] ToByteArray(bool includeExifIdCode = true)
        {
            if (this.values == null)


You should also check if this.data starts with the exif id and skip the first 6 bytes when includeExifIdCode is false.

good point, i missed that one. I have added a unit test for that case.

dlemstra · 2018-06-17T07:56:35Z

src/ImageSharp/MetaData/Profiles/Exif/ExifWriter.cs

+            int i = 0;
+            if (includeExifIdCode)
+            {
+                result[0] = (byte)'E';


You could also increment i when you set the values so you don't need the duplicate code block in the else part.

i have changed that

…d unit test for that

JimBobSquarePants · 2018-06-17T15:53:21Z

src/ImageSharp/MetaData/Profiles/Exif/ExifProfile.cs

+            }
+
+            // The EXIF ID code: ASCII "Exif" followed by two zeros.
+            byte[] exifCode = { 69, 120, 105, 102, 0, 0 };


Can we avoid this allocation per iteration. Do we know yet whether that identifier is specific to jpeg?

i have changed that.

I am not sure if its jpeg specific or other formats which support Exif also use this. I only can tell for sure that PNG does not use it. Maybe @dlemstra knows more about this?

…thExifIdCode()

JimBobSquarePants · 2018-06-21T06:42:10Z

src/ImageSharp/MetaData/Profiles/Exif/ExifProfile.cs

        /// <returns>The <see cref="T:byte[]"/></returns>
-        public byte[] ToByteArray()
+        public byte[] ToByteArray(bool includeExifIdCode = true)


I want to move the Exif identifier out of the ExifProfile as that's defined as part of the App1 segment itself in section 4.7.2 of the specification.

http://www.exif.org/Exif2-2.PDF

Ideally we should pass the marker to the ToByteArray method when exporting. The ExifProfile constructor should have an overload accepting the marker.

ok, i will do that on the weekend.

Fantastic, thanks!

# Conflicts: # src/ImageSharp/MetaData/Profiles/Exif/ExifWriter.cs

…gExif

…er defined in Jpeg ProfileResolver

…rom one exifprofile to another

…r FontShapesAreRenderedCorrectlyAlongACirclePath"

JimBobSquarePants · 2018-06-23T14:55:59Z

src/ImageSharp/MetaData/Profiles/Exif/ExifProfile.cs

@@ -5,6 +5,7 @@
 using System.Collections.Generic;
 using System.IO;
 using System.Linq;
+using SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder;


The ExifProfile needs to be format agnostic.

JimBobSquarePants · 2018-06-23T14:57:15Z

src/ImageSharp/MetaData/Profiles/Exif/ExifProfile.cs

@@ -239,7 +235,8 @@ public void SetValue(ExifTag tag, object value)
        /// Converts this instance to a byte array.
        /// </summary>
        /// <param name="includeExifIdCode">Indicates, if the Exif ID code should be included.
-        /// This Exif ID code should not be included in case of PNG's. Defaults to true.</param>
+        /// The Exif Id Code is part of the JPEG APP1 segment. This Exif ID code should not be included in case of PNG's.
+        /// Defaults to true.</param>
        /// <returns>The <see cref="T:byte[]"/></returns>
        public byte[] ToByteArray(bool includeExifIdCode = true)


Pass through the code as a ReadonlySpan<bye> copying that to the output array.

# Conflicts: # tests/ImageSharp.Tests/Processing/Processors/Transforms/AutoOrientTests.cs

JimBobSquarePants · 2018-07-08T14:10:10Z

@brianpopow Looks great to me 👍 thanks for persevering!

@dlemstra Would you be able to cast your eye over it just to be certain I haven't missed anything?

dlemstra · 2018-07-08T14:55:47Z

src/ImageSharp/Formats/Jpeg/JpegEncoderCore.cs

+            int bytesToWrite = remaining > MaxBytesApp1 ? MaxBytesApp1 : remaining;
+            int app1Length = bytesToWrite + 2;
+
+            // write the app1 header


Can you please cleanup these redundant comments? The method below the comment is self explanatory.

i have removed the unnecessary comments

dlemstra · 2018-07-08T14:59:00Z

src/ImageSharp/MetaData/Profiles/Exif/ExifProfile.cs

+        /// Extends the profile with additional data.
+        /// </summary>
+        /// <param name="bytes">The array containing addition profile data.</param>
+        public void Extend(byte[] bytes)


I wonder if this logic should be handled inside the ExifProfile class. It feels like this should be handled inside the decoder itself.

while this extending is also something jpeg specific and therefore should be handled by the decoder, i could not figure out a way to change it without making it more complicated. The problem is, that we do not know in advance how many APP1 marker there will be for very large EXIF data.

@dlemstra: i have moved the extending of the exif data to the jpeg decoder now, please let me know if you think this is a viable solution.

dlemstra · 2018-07-08T14:59:47Z

src/ImageSharp/MetaData/Profiles/Exif/ExifProfile.cs

        /// <returns>The <see cref="T:byte[]"/></returns>
-        public byte[] ToByteArray()
+        public byte[] ToByteArray(ReadOnlySpan<byte> exifIdCode = default)


Feels a bit odd that we need to pass in the extra data. Can this not be handled inside the encoder/decoder?

yes i think you are right, it feels a bit odd. This is jpeg specific and should be handled by the encoder/decoder

dlemstra · 2018-07-08T15:00:15Z

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

@@ -79,6 +79,7 @@ public List<ExifValue> ReadValues()

            if (this.ReadString(4) == "Exif")
            {
+                // two zeros are expected to follow the Exif Id code


Do we really need this comment?

dlemstra · 2018-07-08T15:03:37Z

src/ImageSharp/Formats/Jpeg/JpegEncoderCore.cs

@@ -639,7 +671,7 @@ private void WriteExifProfile(ExifProfile exifProfile)
        /// </exception>
        private void WriteIccProfile(IccProfile iccProfile)
        {
-            // Just incase someone set the value to null by accident.
+            // Just in-case someone set the value to null by accident.


Can you remove this comment instead? Not every JPEG file has an IccProfile.

…is is jpeg specific and should be handled by the jpeg encoder

…e will be skipped when setting the ExifProfile

JimBobSquarePants · 2018-07-25T06:12:31Z

@brianpopow I'm going to have a look at this PR this evening and fix the merge conflicts and any final tweaks, thanks for being so patient with us and helping out! 👍

JimBobSquarePants · 2018-08-04T20:17:25Z

Ok merging this now as it all looks good to me. Thanks @brianpopow for adding this, much appreciated! 👍

EXIF Support for PNG's

brianpopow added 6 commits June 12, 2018 20:27

added EXIF chunk type and setting ExifProfile, if eXIf chunk data is …

cd44c22

…present (SixLabors#611)

writes the eXIf chunk to the stream during encoding (SixLabors#611)

39e3122

if ignoreMetadata is set, EXIF chunk will be ignored

89f60fc

added unit test which writes a png with a Exif Profile and checks aft…

4ed1df8

…er reloading it, if the profile is still there (SixLabors#611)

- no longer exposing raw exif data byte's, using ToByteArray to write…

820f30c

… the ExifChunk - changed the ExifWriter GetData, to support the special case for PNG's not inlcluding the 'Exif' Code (SixLabors#611)

all Exif tests which write and load images are now testing png and jpg

34e2443

JimBobSquarePants requested a review from dlemstra June 17, 2018 06:38

dlemstra reviewed Jun 17, 2018

View reviewed changes

brianpopow and others added 3 commits June 17, 2018 13:05

removed code duplication setting the byte order marker

9e6c3e5

ToByteArray skips the first 6 bytes if includeExifCode is false, adde…

fd02990

…d unit test for that

Merge branch 'master' into feature/pngExif

5594177

JimBobSquarePants added the formats:png label Jun 17, 2018

JimBobSquarePants reviewed Jun 17, 2018

View reviewed changes

avoiding unnecessary allocation of the exif id byte array in StartsWi…

e26ea49

…thExifIdCode()

tocsoft added this to the 1.0.0-rc1 milestone Jun 17, 2018

Merge branch 'master' into feature/pngExif

8a2c8d9

brianpopow changed the title ~~[WIP] EXIF Support for PNG's~~ EXIF Support for PNG's Jun 20, 2018

Merge branch 'master' into feature/pngExif

bc9ba35

JimBobSquarePants requested changes Jun 21, 2018

View reviewed changes

brianpopow changed the title ~~EXIF Support for PNG's~~ [WIP] EXIF Support for PNG's Jun 21, 2018

brianpopow added 6 commits June 23, 2018 08:33

Merge remote-tracking branch 'upstream/master' into feature/pngExif

48178b8

# Conflicts: # src/ImageSharp/MetaData/Profiles/Exif/ExifWriter.cs

Merge remote-tracking branch 'origin/feature/pngExif' into feature/pn…

5a7926d

…gExif

removed ExifIdCode from ExifConstants and ExifProfile, using ExifMark…

a0a0142

…er defined in Jpeg ProfileResolver

using Span CopyTo() in the ExifProfile Constructor to copy the data f…

88e41ea

…rom one exifprofile to another

Merge remote-tracking branch 'upstream/master' into feature/pngExif

9d81e7c

fixed submodule merge mistake, setting it to "add reference output fo…

53336ea

…r FontShapesAreRenderedCorrectlyAlongACirclePath"

JimBobSquarePants reviewed Jun 23, 2018

View reviewed changes

brianpopow added 2 commits July 7, 2018 21:57

added unit test for writing large exif profiles

685b5c5

Merge remote-tracking branch 'upstream/master' into feature/pngExif

ff160a7

# Conflicts: # tests/ImageSharp.Tests/Processing/Processors/Transforms/AutoOrientTests.cs

JimBobSquarePants approved these changes Jul 8, 2018

View reviewed changes

brianpopow changed the title ~~[WIP] EXIF Support for PNG's~~ EXIF Support for PNG's Jul 8, 2018

dlemstra requested changes Jul 8, 2018

View reviewed changes

brianpopow and others added 12 commits July 9, 2018 19:22

removed unnecessary comments

a5d576e

removed ExifIdCode parameter from ExifProfile.ToByteArray, because th…

35aaaee

…is is jpeg specific and should be handled by the jpeg encoder

to keep the ExifReader free from jpeg specific stuff, the Exif Id Cod…

cb6f4a0

…e will be skipped when setting the ExifProfile

Merge branch 'master' into feature/pngExif

7d7e518

skipping exif id code before extending the exif profile

c4e81de

added support for large exif profiles for the old GolangJpegDecoderCore

a995021

Merge branch 'master' into feature/pngExif

8834f15

moved extending the ExifProfile to the jpeg decoder

efd0288

Merge branch 'master' into feature/pngExif

7f21da4

Merge branch 'master' into feature/pngExif

7e8b464

Merge branch 'master' into feature/pngExif

9aba104

Merge branch 'master' into feature/pngExif

21a7c78

Merge remote-tracking branch 'upstream/master' into feature/pngExif

659bc7c

JimBobSquarePants modified the milestones: 1.0.0-rc1, 1.0.0-beta5 Aug 4, 2018

Align ICC and EXIF API.

6c777d4

JimBobSquarePants added the metadata:exif label Aug 4, 2018

JimBobSquarePants merged commit 67b2e83 into SixLabors:master Aug 4, 2018

antonfirsov pushed a commit to antonfirsov/ImageSharp that referenced this pull request Nov 11, 2019

Merge pull request SixLabors#616 from brianpopow/feature/pngExif

97df31f

EXIF Support for PNG's

brianpopow deleted the feature/pngExif branch March 23, 2020 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EXIF Support for PNG's #616

EXIF Support for PNG's #616

brianpopow commented Jun 16, 2018

codecov bot commented Jun 16, 2018 •

edited

Loading

JimBobSquarePants commented Jun 17, 2018

dlemstra Jun 17, 2018 •

edited

Loading

brianpopow Jun 17, 2018

dlemstra Jun 17, 2018

brianpopow Jun 17, 2018

JimBobSquarePants Jun 17, 2018

brianpopow Jun 17, 2018

JimBobSquarePants Jun 21, 2018 •

edited

Loading

brianpopow Jun 21, 2018

JimBobSquarePants Jun 22, 2018

JimBobSquarePants Jun 23, 2018

JimBobSquarePants Jun 23, 2018

JimBobSquarePants commented Jul 8, 2018

dlemstra Jul 8, 2018

brianpopow Jul 9, 2018

dlemstra Jul 8, 2018

brianpopow Jul 9, 2018

brianpopow Jul 13, 2018

dlemstra Jul 8, 2018

brianpopow Jul 9, 2018

dlemstra Jul 8, 2018

dlemstra Jul 8, 2018

JimBobSquarePants commented Jul 25, 2018

JimBobSquarePants commented Aug 4, 2018 •

edited

Loading

EXIF Support for PNG's #616

EXIF Support for PNG's #616

Conversation

brianpopow commented Jun 16, 2018

Prerequisites

Description

codecov bot commented Jun 16, 2018 • edited Loading

Codecov Report

JimBobSquarePants commented Jun 17, 2018

dlemstra Jun 17, 2018 • 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

JimBobSquarePants Jun 21, 2018 • 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

JimBobSquarePants commented Jul 8, 2018

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

JimBobSquarePants commented Jul 25, 2018

JimBobSquarePants commented Aug 4, 2018 • edited Loading

codecov bot commented Jun 16, 2018 •

edited

Loading

dlemstra Jun 17, 2018 •

edited

Loading

JimBobSquarePants Jun 21, 2018 •

edited

Loading

JimBobSquarePants commented Aug 4, 2018 •

edited

Loading