Support for XMP metadata #1918

ynse01 · 2022-01-02T13:48: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 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.

src/ImageSharp/Formats/Gif/GifDecoderCore.cs

src/ImageSharp/Formats/Gif/Sections/GifXmpApplicationExtension.cs

src/ImageSharp/Formats/Png/PngDecoderCore.cs

src/ImageSharp/Formats/Webp/WebpDecoderCore.cs

JimBobSquarePants · 2022-01-03T01:24:38Z

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.

src/ImageSharp/Metadata/Profiles/XMP/XmpProfile.cs

JimBobSquarePants

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

src/ImageSharp/Formats/Png/PngDecoderCore.cs

src/ImageSharp/Formats/Png/PngEncoderCore.cs

src/ImageSharp/Metadata/Profiles/XMP/XmpProfile.cs

antonfirsov · 2022-01-03T16:32:54Z

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 · 2022-01-06T21:44:51Z

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

codecov · 2022-01-07T10:58:55Z

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

@@           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.

antonfirsov

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

src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs

src/ImageSharp/Formats/Jpeg/JpegEncoderCore.cs

JimBobSquarePants

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

src/ImageSharp/Formats/Png/PngEncoderCore.cs

…harp into read-xmp-from-webp

JimBobSquarePants · 2022-01-11T01:32:08Z

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

ynse01 added 3 commits January 1, 2022 17:12

All image formats round trip

b8bed9f

More robust equals implementation

fa3e6ff

Fix build on .NET core 2.1

32aa75b

gfoidl reviewed Jan 2, 2022

View reviewed changes

Performance improvements

06beffb

antonfirsov reviewed Jan 3, 2022

View reviewed changes

src/ImageSharp/Metadata/Profiles/XMP/XmpProfile.cs Outdated Show resolved Hide resolved

src/ImageSharp/Metadata/Profiles/XMP/XmpProfile.cs Outdated Show resolved Hide resolved

JimBobSquarePants reviewed Jan 3, 2022

View reviewed changes

ynse01 and others added 7 commits January 3, 2022 20:34

Use memory allocator in Gif encoder

f451013

Skip over iTXT header

f64f2cc

Read only API for XmpProfile

ded8b20

Delete XElementEqualityComparer.cs

12263f9

Gif: Check on frame count before writing repeat count

b443e82

Merge branch 'master' into read-xmp-from-webp

3d981fe

Don't throw when Position is out of range for BufferedReadStream

559edde

ynse01 added 2 commits January 7, 2022 11:33

Fix casing of test images file names

760785e

Fix casing for Webp test image

e3ef796

antonfirsov reviewed Jan 7, 2022

View reviewed changes

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

ynse01 and others added 3 commits January 7, 2022 20:20

Prevent memory allocations

1ee34ba

Renaming the hasProfile fields

f356c61

Merge branch 'master' into read-xmp-from-webp

12cd6bb

JimBobSquarePants requested changes Jan 9, 2022

View reviewed changes

src/ImageSharp/Formats/Gif/GifEncoderCore.cs Outdated Show resolved Hide resolved

src/ImageSharp/Formats/Png/PngEncoderCore.cs Outdated Show resolved Hide resolved

ynse01 added 5 commits January 9, 2022 16:06

Correct missing frame counter check

dd31636

Another allocation optimization

d879dd8

Merge branch 'read-xmp-from-webp' of https://github.com/ynse01/ImageS…

de9a645

…harp into read-xmp-from-webp

Explicitly control lifetime of memory owner

c05fd21

Zero out the iTxt header

c654079

ynse01 and others added 2 commits January 10, 2022 06:29

Use memory allocator for all text chunks

4904f32

Merge branch 'master' into read-xmp-from-webp

8630d19

JimBobSquarePants merged commit dc79124 into SixLabors:master Jan 11, 2022

brianpopow mentioned this pull request Jan 19, 2022

WebP: Missing features #1802

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for XMP metadata #1918

Support for XMP metadata #1918

ynse01 commented Jan 2, 2022 •

edited

Loading

JimBobSquarePants commented Jan 3, 2022

JimBobSquarePants left a comment

antonfirsov commented Jan 3, 2022

ynse01 commented Jan 6, 2022

codecov bot commented Jan 7, 2022 •

edited

Loading

antonfirsov left a comment

JimBobSquarePants left a comment •

edited

Loading

JimBobSquarePants commented Jan 11, 2022

Support for XMP metadata #1918

Support for XMP metadata #1918

Conversation

ynse01 commented Jan 2, 2022 • edited Loading

Prerequisites

Description

JimBobSquarePants commented Jan 3, 2022

JimBobSquarePants left a comment

Choose a reason for hiding this comment

antonfirsov commented Jan 3, 2022

ynse01 commented Jan 6, 2022

codecov bot commented Jan 7, 2022 • edited Loading

Codecov Report

antonfirsov left a comment

Choose a reason for hiding this comment

JimBobSquarePants left a comment • edited Loading

Choose a reason for hiding this comment

JimBobSquarePants commented Jan 11, 2022

ynse01 commented Jan 2, 2022 •

edited

Loading

codecov bot commented Jan 7, 2022 •

edited

Loading

JimBobSquarePants left a comment •

edited

Loading