Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for encoding and decoding IPTC metadata #1174

Merged
merged 18 commits into from
Apr 19, 2020
Merged

Conversation

brianpopow
Copy link
Collaborator

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 PR adds support for reading and writing IPTC metadata in JPEG files. The actual IPTC data parsing is kindly borrowed from Magick.Net.
Description of parsing APP13 Marker can be found here App13 Marker

@brianpopow brianpopow requested a review from a team April 16, 2020 09:47
{
Guard.NotNull(encoding, nameof(encoding));

if (!this.IsRepeatable(tag))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dlemstra i have changed the original code, so it is now possible to add some entrys more than once. If you think that would be useful for Magick.Net, too, i can change that over there, once this PR is done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be awesome if you could also get this merged into Magick.NET. Make sure you reference this issue then: dlemstra/Magick.NET#262. And maybe the API then also needs this: public IEnumerable<IptcValue> GetValues(IptcTag tag)?

@codecov
Copy link

codecov bot commented Apr 16, 2020

Codecov Report

Merging #1174 into master will increase coverage by 0.05%.
The diff coverage is 87.41%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1174      +/-   ##
==========================================
+ Coverage   82.42%   82.47%   +0.05%     
==========================================
  Files         684      687       +3     
  Lines       29564    29871     +307     
  Branches     3324     3378      +54     
==========================================
+ Hits        24369    24637     +268     
- Misses       4507     4534      +27     
- Partials      688      700      +12     
Flag Coverage Δ
#unittests 82.47% <87.41%> (+0.05%) ⬆️
Impacted Files Coverage Δ
src/ImageSharp/Metadata/Profiles/IPTC/IptcValue.cs 66.66% <66.66%> (ø)
src/ImageSharp/Formats/Jpeg/JpegEncoderCore.cs 94.61% <85.71%> (-0.61%) ⬇️
src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs 92.87% <92.85%> (-0.01%) ⬇️
...c/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs 93.75% <93.75%> (ø)
...eSharp/Metadata/Profiles/IPTC/IptcTagExtensions.cs 96.49% <96.49%> (ø)
...Formats/Jpeg/Components/Decoder/ProfileResolver.cs 100.00% <100.00%> (ø)
src/ImageSharp/Metadata/ImageMetadata.cs 100.00% <100.00%> (ø)
...c/ImageSharp/Metadata/Profiles/Exif/ExifProfile.cs 88.88% <100.00%> (+0.12%) ⬆️
... and 1 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 424d388...7fb1612. Read the comment docs.

@brianpopow
Copy link
Collaborator Author

A thing i was unsure about was, if i should follow the specification strictly and cap some IPTC values accordingly. For example the Category should only be max 3 characters long according to the spec. Here is an overview of the tags with the max length ExifTool

It seems no other tool which supports IPTC does honor those max length values, for example irfanview.

…Value now returns a list of entrys instead of just one
@JimBobSquarePants
Copy link
Member

I'm curious about all the values being strings. It looks to me that there are different value types in the specification. Perhaps we should refactor to allow safely using things like DateTime?

@JimBobSquarePants
Copy link
Member

A thing i was unsure about was, if i should follow the specification strictly and cap some IPTC values accordingly.

@brianpopow Do you mean writing as well as reading? I'd like to follow the spec for writing if possible.

@brianpopow
Copy link
Collaborator Author

A thing i was unsure about was, if i should follow the specification strictly and cap some IPTC values accordingly.

@brianpopow Do you mean writing as well as reading? I'd like to follow the spec for writing if possible.

I mean only when the user adds new values to the profile. Enforcing the strict length rules would also prevent to write a app marker which exceeds the maximum length.
On the other hand it may be inconvenient for user who are not aware of the size limits. Maybe an additional bool parameter strict for the SetValue method (which would default to true)?

@brianpopow
Copy link
Collaborator Author

I'm curious about all the values being strings. It looks to me that there are different value types in the specification. Perhaps we should refactor to allow safely using things like DateTime?

If you look at page 32, ReleaseDate Property, they state the format should be CCYYMMDD

@JimBobSquarePants
Copy link
Member

If you look at page 32, ReleaseDate Property, they state the format should be CCYYMMDD

@brianpopow How do you intend on enforcing that though?

@JimBobSquarePants
Copy link
Member

Maybe an additional bool parameter strict for the SetValue method (which would default to true)?

I'd consider that yes. Would it be hard to implement?

@brianpopow
Copy link
Collaborator Author

Maybe an additional bool parameter strict for the SetValue method (which would default to true)?

I'd consider that yes. Would it be hard to implement?

No, i dont think its hard. The only think im unsure yet is how to deal with the datetime fields.

/// </summary>
/// <param name="tag">The tag of the iptc value.</param>
/// <param name="dateTime">The datetime.</param>
public void SetDateTimeValue(IptcTag tag, DateTime dateTime)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

/// </summary>
ReleaseDate = 30,

/// <summary>
/// Release time, not repeatable.
/// Release time. Format should be HHMMSS±HHMM,
/// e.g. "090000-0500" for 9 o'clock New York time (five hours behind UTC).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use <example> for block examples. Nest in summary to ensure they show up.

https://docs.microsoft.com/en-us/dotnet/csharp/codedoc#example

Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great; Well done!

@JimBobSquarePants JimBobSquarePants merged commit eb312e0 into master Apr 19, 2020
@JimBobSquarePants JimBobSquarePants deleted the bp/iptc branch April 19, 2020 23:51
@brianpopow brianpopow mentioned this pull request Apr 20, 2020
@JimBobSquarePants JimBobSquarePants added this to the 1.0.0-rc1 milestone Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants