Added support for loading exif data from PNG "Raw profile type exif" text chunk #1877
Conversation
…profile type exif" text chunk.
|
|
Co-authored-by: Günther Foidl <gue@korporal.at>
Co-authored-by: Günther Foidl <gue@korporal.at>
|
Additional design decision: should the |
Once we have that profile, absolutely. |
…ly used once. Added a missing comment for StringEqualsInsensitive.
Codecov Report
@@ Coverage Diff @@
## master #1877 +/- ##
======================================
- Coverage 87% 87% -1%
======================================
Files 961 962 +1
Lines 51034 51121 +87
Branches 6324 6342 +18
======================================
+ Hits 44863 44925 +62
- Misses 5133 5148 +15
- Partials 1038 1048 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
…ource implementation of Convert.FromHexString(): https://source.dot.net/#System.Private.CoreLib/Convert.cs,c9e4fbeaca708991
| if (name.Equals("Raw profile type exif", StringComparison.OrdinalIgnoreCase) && | ||
| this.TryReadLegacyExifTextChunk(baseMetadata, uncompressed)) | ||
| { | ||
| // Successfully parsed exif data stored as text in this chunk |
There was a problem hiding this comment.
Reverse the condition here so you don't have the odd empty scope.
There was a problem hiding this comment.
I ended up moving this into its own function, TryReadTextChunkMetadata, since I was missing support for loading legacy exif data from uncompressed text chunks, and I also wanted to have an easy spot to add support for other metadata chunks in the future. I suppose I could also just add IPTC support in this PR too if you want, but I figure it's getting somewhat large and it might be better to just make another PR with that feature after this one gets merged.
There was a problem hiding this comment.
Yeah, let's keep IPTC separate for now.
…-enigma/ImageSharp into je/nonstandard-png-exif
There was a problem hiding this comment.
@jubilant-enigma Can you double check that your local fork is using the updated submodules. There shouldn't be a change to them compared to our master.
EDIT. I pulled your fork and did the update myself to fix.
| } | ||
|
|
||
| HexConverter.HexStringToBytes(dataSpan.Slice(0, exifHeader.Length * 2), tempExifBuf); | ||
| if (!tempExifBuf.AsSpan().Slice(0, exifHeader.Length).SequenceEqual(exifHeader)) |
There was a problem hiding this comment.
if (!tempExifBuf.AsSpan(0, exifHeader.Length).SequenceEqual(exifHeader))
| lineSpan = dataSpan.Slice(0, newlineIndex); | ||
| } | ||
|
|
||
| i += HexConverter.HexStringToBytes(lineSpan, exifBlob.AsSpan().Slice(i)); |
There was a problem hiding this comment.
i += HexConverter.HexStringToBytes(lineSpan, exifBlob.AsSpan(i));
| // Skip to the data length | ||
| dataSpan = dataSpan.Slice(4).TrimStart(); | ||
| int dataLengthEnd = dataSpan.IndexOf('\n'); | ||
| int dataLength = ParseInt32(dataSpan.Slice(0, dataSpan.IndexOf('\n'))); |
There was a problem hiding this comment.
int dataLength = ParseInt32(dataSpan.Slice(0, dataLengthEnd));
There was a problem hiding this comment.
Thanks for pointing these issues out @turbedi. Ideally I'll fix them sometime next week when I create a PR for IPTC metadata loading from text chunks.
Sorry about that, thanks for taking care of it! Unfortunately my available free time has been more intermittent than I hoped. |
I don't really have much experience in C# outside of toy projects, so I don't really have a good idea of what best practices are for C# these days.
Some more optimal functions (span related, etc) seemed available with some of the newer .net versions, however, then .netstandard1.3 would produce build errors. Yet that tests don't seem to be run on that target? Not sure what's going on there.
Prerequisites
Description
See #1875.