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

Support for XMP metadata #1918

Merged
merged 23 commits into from
Jan 11, 2022
Merged

Conversation

ynse01
Copy link
Contributor

@ynse01 ynse01 commented Jan 2, 2022

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 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 fixes #858.

Adding the ability to read and write XMP metadata from the following image formats:

  • GIF
  • JPEG
  • PNG
  • TIFF
  • WEBP

XMP metadata is placed in a dedicated XmpProfile class, which exposes the raw XML data.

Some highlights on the changes:

  • Relies on a solution for bug Don't throw when Position is out of range.  #1907
  • WebP decoder is no longer dependant on the order in which the headers appear in the file. This also exposes EXIF on some images where they where hidden previously.
  • Extended JPEG is supported, where the XMP metadata is larger then 64 kB. Added a test images for this specific case.
  • Support the image that @rickbrew shared
  • GIF does not provide a size of the XMP metadata, so I opted to read in blocks of 256 bytes and combine them later.
  • IGifExtension got a new property, to predict the buffer size required during encoding.
  • Added some test images, to ensure all formats have at least 1 image with XMP metadata. Re-used existing test images where applicable.
  • Unit test for all image formats.

Open points, not part of this review:

  • XmpProfile does not provide any strongly typed classes for the XMP properties.
  • Metadata profiles should use a MemoryAllocator, as these profiles get quite large for realistic images. This is most notable in the GIF decoding. Doing so, causes a breaking change though, as it makes Metadata classes IDisposable and require the constructors to have a Configuration argument.

@JimBobSquarePants
Copy link
Member

Will do a proper review later on but wasn’t specifically planning on supporting this for V2 so depending on the design we might have to make this internal to ensure we have the time to get it right.

Regarding Metadata, we cannot use the allocator since we require the ability to explicitly set profile instances to null.

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.

Lovely bit of work so far but I'm not sure about where we're going with this. Needs more discussion.

src/ImageSharp/Formats/Gif/GifEncoderCore.cs Show resolved Hide resolved
src/ImageSharp/Formats/Gif/GifEncoderCore.cs Outdated Show resolved Hide resolved
src/ImageSharp/Formats/Png/PngDecoderCore.cs Show resolved Hide resolved
src/ImageSharp/Formats/Png/PngEncoderCore.cs Outdated Show resolved Hide resolved
src/ImageSharp/Metadata/Profiles/XMP/XmpProfile.cs Outdated Show resolved Hide resolved
src/ImageSharp/Metadata/Profiles/XMP/XmpProfile.cs Outdated Show resolved Hide resolved
@antonfirsov
Copy link
Member

Metadata profiles should use a MemoryAllocator, as these profiles get quite large for realistic images.

Regarding Metadata, we cannot use the allocator since we require the ability to explicitly set profile instances to null.

The main challenge here is that metadata profiles shall be IDisposable in order to hold allocated buffers. (This way profile setters can dispose them too.) In case we are concerned that profiles (of any type) can get allocation-heavy, we should revisit the API.

@ynse01
Copy link
Contributor Author

ynse01 commented Jan 6, 2022

I had to apply the fix of #1907 to BufferedReadStream also and updated its tests accordingly. Hope this is OK with you guys.

@codecov
Copy link

codecov bot commented Jan 7, 2022

Codecov Report

Merging #1918 (4904f32) into master (7a5cd86) will increase coverage by 0%.
The diff coverage is 92%.

❗ Current head 4904f32 differs from pull request most recent head 8630d19. Consider uploading reports for the commit 8630d19 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master   #1918    +/-   ##
=======================================
  Coverage      88%     88%            
=======================================
  Files         966     968     +2     
  Lines       51321   51564   +243     
  Branches     6397    6428    +31     
=======================================
+ Hits        45185   45403   +218     
- Misses       5076    5091    +15     
- Partials     1060    1070    +10     
Flag Coverage Δ
unittests 88% <92%> (+<1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/ImageSharp/IO/BufferedReadStream.cs 95% <ø> (-1%) ⬇️
.../ImageSharp/Formats/Webp/BitWriter/Vp8BitWriter.cs 93% <41%> (-3%) ⬇️
src/ImageSharp/Formats/Jpeg/JpegEncoderCore.cs 94% <78%> (-1%) ⬇️
src/ImageSharp/Metadata/Profiles/XMP/XmpProfile.cs 82% <82%> (ø)
src/ImageSharp/Formats/Gif/GifDecoderCore.cs 86% <83%> (-2%) ⬇️
src/ImageSharp/Formats/Webp/WebpDecoderCore.cs 76% <90%> (-4%) ⬇️
src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs 89% <94%> (+<1%) ⬆️
...Formats/Gif/Sections/GifXmpApplicationExtension.cs 94% <94%> (ø)
src/ImageSharp/Formats/Png/PngEncoderCore.cs 97% <96%> (-1%) ⬇️
src/ImageSharp/Formats/Gif/GifConstants.cs 100% <100%> (ø)
... and 20 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 eb5c71b...8630d19. Read the comment docs.

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

Some memory optimization suggestions. Commented on Jpeg codec, but applicable to all.

src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs Outdated Show resolved Hide resolved
src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs Outdated Show resolved Hide resolved
src/ImageSharp/Formats/Jpeg/JpegEncoderCore.cs Outdated Show resolved Hide resolved
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.

Just a single missed allocation optimization opportunity and our gif check remaining and this looks good to go in. I've gotta say, I'm super impressed with how quickly you've added this across multiple formats. Really nice 👍

src/ImageSharp/Formats/Gif/GifEncoderCore.cs Outdated Show resolved Hide resolved
src/ImageSharp/Formats/Png/PngEncoderCore.cs Outdated Show resolved Hide resolved
@JimBobSquarePants
Copy link
Member

Let's get this merged 😄 Thanks @ynse01 for the code and everyone else for the excellent reviews. Much appreciated!

@JimBobSquarePants JimBobSquarePants merged commit dc79124 into SixLabors:master Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for XMP metadata
6 participants