Basic metadata-only image parsing API by xakep139 · Pull Request #292 · SixLabors/ImageSharp

xakep139 · 2017-08-10T08:10:26Z

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 practise as demonstrated in the repository. These follow strict Stylecop rules 👮.
I have provided test coverage for my change (where applicable)

Description

Implemented feature request #258

codecov · 2017-08-10T08:24:23Z

Codecov Report

Merging #292 into master will decrease coverage by 0.02%.
The diff coverage is 69.49%.

@@            Coverage Diff             @@
##           master     #292      +/-   ##
==========================================
- Coverage   80.66%   80.63%   -0.03%     
==========================================
  Files         512      513       +1     
  Lines       20141    20199      +58     
  Branches     2197     2206       +9     
==========================================
+ Hits        16246    16287      +41     
- Misses       3218     3228      +10     
- Partials      677      684       +7

Impacted Files	Coverage Δ
...geSharp/Formats/Jpeg/PdfJsPort/PdfJsJpegDecoder.cs	`83.33% <0%> (-16.67%)`	⬇️
...geSharp/Formats/Jpeg/GolangPort/OrigJpegDecoder.cs	`55.55% <0%> (-44.45%)`	⬇️
src/ImageSharp/Formats/Png/PngDecoder.cs	`100% <100%> (ø)`	⬆️
src/ImageSharp/Formats/Gif/GifDecoder.cs	`100% <100%> (ø)`	⬆️
src/ImageSharp/Image/Image.FromStream.cs	`57.57% <100%> (+2.73%)`	⬆️
src/ImageSharp/Formats/Jpeg/JpegDecoder.cs	`100% <100%> (ø)`	⬆️
src/ImageSharp/Formats/Bmp/BmpDecoder.cs	`100% <100%> (ø)`	⬆️
src/ImageSharp/Formats/PixelTypeInfo.cs	`100% <100%> (ø)`
src/ImageSharp/Image/Image.Decode.cs	`75% <50%> (-2.78%)`	⬇️
src/ImageSharp/Formats/Png/PngDecoderCore.cs	`69.51% <57.69%> (-0.68%)`	⬇️
... and 2 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 f49bd9b...d2c0338. Read the comment docs.

xakep139 · 2017-08-10T09:11:47Z

tests/ImageSharp.Tests/TestFileSystem.cs

        public static TestFileSystem Global { get; } = new TestFileSystem();

-        public static void RegisterGloablTestFormat()
+        public static void RegisterGlobalTestFormat()


xakep139 · 2017-08-10T09:11:56Z

tests/ImageSharp.Tests/TestFormat.cs

        public static TestFormat GlobalTestFormat { get; } = new TestFormat();

-        public static void RegisterGloablTestFormat()
+        public static void RegisterGlobalTestFormat()


xakep139 · 2017-08-10T09:12:18Z

tests/ImageSharp.Tests/TestFormat.cs


+            public int DetectPixelSize(Configuration configuration, Stream stream)
+            {
+                throw new NotImplementedException();


Not implemented for test format

xakep139 · 2017-08-11T01:14:29Z

src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs

                if (h == 3 || v == 3)
                {
-                    throw new ImageFormatException("Lnsupported subsampling ratio");
+                    throw new ImageFormatException("Unsupported subsampling ratio");


xakep139 · 2017-08-11T01:17:41Z

tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs

+
+        [Theory]
+        [InlineData(TestImages.Gif.Cheers, 8)]
+        [InlineData(TestImages.Gif.Giphy, 8)]


For these two GIFs Image.GetPixelFormatSize() returns 32 which is not correct

xakep139 · 2017-08-11T01:20:28Z

tests/ImageSharp.Tests/Image/ImageLoadTests.cs

        [Fact]
        public void LoadFromStream()
        {
-             Image<Rgba32> img = Image.Load<Rgba32>(this.DataStream);


Indentation here

antonfirsov · 2017-08-13T23:30:24Z

@xakep139 nice job!
I wonder if instead of having a dedicated DetectPixelSize() method, wouldn't it be better to introduce and return a PixelTypeInfo object instead? We could bridge it to our PixelFormats later.

xakep139 · 2017-08-18T02:07:10Z

@antonfirsov I apologize for delayed response. Your idea is pretty good, I'll implement it.
Initially I thought about detecting TPixel, but some bpp's couldn't be converted into it.

xakep139 · 2017-08-24T10:12:33Z

@antonfirsov do you have other suggestions?

antonfirsov · 2017-08-24T10:24:14Z

tests/ImageSharp.Tests/TestImages.cs

            public const string Powerpoint = "Png/pp.png";
            public const string SplashInterlaced = "Png/splash-interlaced.png";
            public const string Interlaced = "Png/interlaced.png";
+            public const string Palette8Bpp = "Png/Palette-8bpp.png";


Unix systems are case-sensitive, so you have to rename this to palette-8pp.png, to match the filename. There is a test failure on travis because of that.

fixed in d6a2d40

antonfirsov · 2017-08-24T10:28:50Z

@xakep139 no, the PixelTypeInfo thing is the only one, but we are releasing our first beta soon, which means that we need to be very strict defining our public API. I think we need to implement this with the PixelTypeInfo stuff now, because there is no chance to change it later.

@JimBobSquarePants any suggestions to manage this?

CLAassistant · 2017-09-26T09:59:48Z

All committers have signed the CLA.

xakep139 · 2017-09-26T10:28:58Z

@antonfirsov I've synced the branch with upstream

antonfirsov · 2017-09-26T19:48:54Z

@xakep139 thanks! (And also thanks for your patience!)

Someone has to refactor your work to the PixelTypeInfo style so we can merge it. I'll try to be that someone ...
Can I push right to your branch?

xakep139 · 2017-09-27T03:29:37Z

@antonfirsov, yes, sure

denisivan0v · 2018-01-17T10:41:05Z

@antonfirsov @JimBobSquarePants Me and @xakep139 are working on the same project and now we're extremely need an API that would provide base image information without decoding it.
So, I've made changes that was mentioned in #292 (comment):

introduced base IImage interface (as it was requested in interfaces for Image #430)
introduced IImageInfoDetector interface (following to this Basic metadata-only image parsing API #292 (comment))
Image.DetectPixelType method expanded to Image.Identify method that returns IImage

Hope it will also help to resolve #430.

I'd be much appreciated for any suggestions and comments.

antonfirsov · 2018-01-17T12:25:42Z

@denisivan0v thanks for joining in!

I really hope we'll be able to figure out how to provide the feature you need. But let's start with a very important disclaimer:
For the ImageSharp team, maintaining a consistent & clean API has a higher priority than individual features! So expect us to be very strict at the review process.

The major issue is that ImageSharp has not been designed to support metadata-only scenarios. I understand however that many users need it, and I think it's worth for us to find a way for supporting them, so they don't have to look for other libraries. (@JimBobSquarePants do you agree?)

However, to avoid the design lavine I foresee here, this feature set should be limited and/or isolated at this point, with future-proof API modifications only.

antonfirsov · 2018-01-17T12:39:42Z

src/ImageSharp/Image/IImage.cs

+        /// <summary>
+        /// Gets information about pixel.
+        /// </summary>
+        PixelTypeInfo PixelType { get; }


I suppose this value holds the native pixel type in the stream, after Identify().
What's the deal with Image.Load<TPixel>()? What if typeof(TPixel) ≠ the native pixel type in the stream ?

That situation will happen more often than not since 3 out of 4 of our currently supported image formats are commonly RGB.

If it's going to be part of the interface then both The property and its type should be renamed as it's just confusing now. What would the equivalent information be called in other libraries?

However, if we rename it to something like OriginalPixelType, while keeping it in IImage means that we added another source stream specific, metadata-like property to the Image<T> class, which is mostly intended to represent raw image/pixel data.

My suggestion:
Let's name it OriginalPixelType and move it into the ImageMetaData class.

Image shouldn't have any knowledge of its original loaded data.. this is why we removed the original image format from Image I think we need this API to return an ImageMetadata object instead that exposes all this info and not the based IImage interface.

Okkay, let's sum up the situation, so we can figure it out:

@tocsoft Not sure if I'm comfortable with this fact, but we already have MetaData exposed on Image<TPixel>.

We need to include Width+Height (or maybe SixLabors.Size?) into the metadata-only result. Currently it's not exposed on ImageMetadata.

@denisivan0v solved this by defining ImageInfo : IImage as the metadata-only result

But maybe these are just different things, and sharing a common interface is an LSP violation.

@denisivan0v @xakep139 do you actually need the ImageInfo/Image<T> polymorphism? Eg: do you want to mix them in heterogenous containers? For me it feels like an unrealistic scenario.

@JimBobSquarePants I'm not sure if I'm following you correctly.
Do you suggest to keep the PixelType property on the IImage interface + keep it in sync with TPixel in case of Image<TPixel>? Or the opposite: always exposing the raw type after Image.Load<TPixel>() regardless of TPixel?

@antonfirsov The former.

For Image<TPixel> the PixelType property represents TPixel

For ImageInfo the PixelType property represents bits per pixel of the source image.

Keeping it in sync for Image<TPixel> is simple since it can be calculated in the constructor.

Makes sense, and looks like being in sync with my vision about the role of PixelTypeInfo as a bridge between TPixel and raw formats, and future pixel-type agnostic Image API-s. (System.Drawing is decoding images into their native formats, we might implement this feature with IImage.)

However I'm still unsure if the result of Identify() is actually an image. Maybe we should rename to IImageInfo, and reserve IImage for future use cases:
IImage should represent an image that actually has pixels, so it could be used with processors, .Mutate(), .Clone() etc.

I think we should implement the following hierarchy in long term:

// Has Dimensions + PixelTypeInfo + Metadata public interface IImageInfo { ... } // Also has pixels, could be used with ImageProcessor-s. public interface IImage : IImageInfo { ... } internal class ImageInfo : IImageInfo { ... } public class Image<TPixel> : IImage { ... }

Yeah, let's do the rename. 👍

Great design review, thanks!

I also wasn't fully satisfied that Image<T> and internal ImageInfo shares the same IImage interface. But IImageInfo and suggested hierarchy solved the problem and made things clear.

Done in 61c9caf.

JimBobSquarePants

Overall this is very good! 🥇

Just some minor changes and I'm happy for it to go in. Thanks for persisting with us! 😄

JimBobSquarePants · 2018-01-17T13:08:12Z

src/ImageSharp/Image/IImage.cs

+        /// <summary>
+        /// Gets information about pixel.
+        /// </summary>
+        PixelTypeInfo PixelType { get; }


That situation will happen more often than not since 3 out of 4 of our currently supported image formats are commonly RGB.

If it's going to be part of the interface then both The property and its type should be renamed as it's just confusing now. What would the equivalent information be called in other libraries?

JimBobSquarePants · 2018-01-17T13:09:31Z

src/ImageSharp/Image/IImage.cs

+    public interface IImage
+    {
+        /// <summary>
+        /// Gets information about pixel.


Lets flesh out the property descriptors here. This doesn't tell me anything and wont help with docs.

Done in 61c9caf.

JimBobSquarePants · 2018-01-17T13:11:08Z

src/ImageSharp/Formats/Gif/GifDecoderCore.cs

            {
                Width = BitConverter.ToInt16(this.buffer, 0),
                Height = BitConverter.ToInt16(this.buffer, 2),
+                BitsPerPixel = (this.buffer[4] & 0x07) + 1,  // The lowest 3 bits represent the bit depth minus 1


Was hoping this wouldn't trip you up. Good research. 👍

Actually, this was initially done by @xakep139 👍

Done in f8573e3#diff-7ae85077a97a9237d124f0ddfbe4752c

JimBobSquarePants · 2018-01-17T13:12:40Z

src/ImageSharp/Formats/Gif/GifDecoderCore.cs


                this.ReadFrameIndices(imageDescriptor, indices);
-                this.ReadFrameColors(indices, localColorTable ?? this.globalColorTable, imageDescriptor);
+                this.ReadFrameColors(ref image, ref previousFrame, indices, localColorTable ?? this.globalColorTable, imageDescriptor);


Image<T> and ImageFrame<T> are already reference types?

I had to remove TPixel type parameter from this class and move it Decode method. So Image<TPixel> image and ImageFrame<TPixel> previousFrame fields also removed and became local variables in Decode<TPixel> method. They marked with ref because they are reassigning down the call stack.

JimBobSquarePants · 2018-01-17T13:18:22Z

src/ImageSharp/Image/ImageInfo.cs

+
+namespace SixLabors.ImageSharp
+{
+    internal sealed class ImageInfo : IImage


Please add xml docs

Done in 61c9caf

JimBobSquarePants · 2018-01-17T13:22:09Z

src/ImageSharp/Formats/PixelTypeInfo.cs

+namespace SixLabors.ImageSharp.Formats
+{
+    /// <summary>
+    /// Stores information about pixel.


We need a better description than this.

I don't like the name. RawPixelTypeInfo or RawPixelFormatInfo perhaps? Naming is hard!

Whatever we choose the property name on the interface should match.

@JimBobSquarePants I proposed this class and naming in order to make it extendable in a way like this:

public class PixelTypeInfo { public System.Type PixelType { get; } public int BitsPerPixel { get; } internal PixelTypeInfo(int bitsPerPixel, System.Type pixelType = null) { this.BitsPerPixel = bitsPerPixel; } public static PixelTypeInfo Create<TPixel>() where TPixel : IPixel<TPixel> => new PixelTypeInfo(Unsafe.SizeOf<TPixel>(), typeof(TPixel)); }

This will enable integrating it deeper with our pixel type infrastructure, and provide non-generic pixel-type related API-s. As a bridge between native (image format/stream bound) pixel type data and our internal pixel types, this class will not necessarily represent "raw"/native pixel type information only.

You'd need to keep it in sync with changes to TPixel in that case and the PR utilizes it purely for the raw format. It can be one or the other, not both.

See this #292 (comment).

JimBobSquarePants · 2018-01-17T22:24:02Z

tests/ImageSharp.Tests/TestUtilities/ReferenceCodecs/SystemDrawingReferenceDecoder.cs

 namespace SixLabors.ImageSharp.Tests.TestUtilities.ReferenceCodecs
 {
-    public class SystemDrawingReferenceDecoder : IImageDecoder
+    using SixLabors.ImageSharp.MetaData;


Why were the namespaces altered in this class?

That wasn't my intension. I guess it's because of my incorrect tooling settings (I'm using JetBrains Rider on Mac). Reverted it in 61c9caf.

denisivan0v · 2018-01-18T07:28:03Z

@antonfirsov @JimBobSquarePants huge thanks for your review and suggestions! I'm going to make changes asap to reduce time to the next review cycle.

JimBobSquarePants · 2018-01-19T07:39:17Z

tests/ImageSharp.Tests/ImageSharp.Tests.csproj

  </ItemGroup>
  <ItemGroup>
    <PackageReference Include="CoreCompat.System.Drawing" Version="1.0.0-beta006" />
+    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.5.0" />


Do we need this?

This enables unit tests execution in JetBrains Rider, but it's not necessary.

Let's keep it then. I want everyone to be able to test the project in their desired IDE.

JimBobSquarePants · 2018-01-19T12:39:47Z

Many, many thanks @xakep139 and @denisivan0v ! 🥇

This is an incredibly valuable addition to the library and we're so happy you stuck with us to get it merged in!

Add an equivalent for System.Drawing.Image.GetPixelFormatSize() (#258)

f8573e3

xakep139 commented Aug 10, 2017

View reviewed changes

xakep139 commented Aug 11, 2017

View reviewed changes

merge with master

a56893a

Nikita Balabaev added 3 commits August 18, 2017 09:45

fixes on code review

1d81aed

sync with upstream

47454de

update packages

ebe664c

antonfirsov reviewed Aug 24, 2017

View reviewed changes

Nikita Balabaev added 3 commits August 24, 2017 17:36

fix test image filename

d6a2d40

sync with upstream

75a89d6

fix bugs after merge

93419ab

remove duplicates in .csproj files

4a59c6f

xakep139 added 5 commits September 27, 2017 16:58

Merge branch 'master' into master

e1dcdef

Merge branch 'master' into master

b5855a0

Merge branch 'master' into master

79e2523

Merge branch 'master' into master

d0e802d

Merge branch 'master' into master

d204e88

denisivan0v added 3 commits January 17, 2018 16:59

Merge branch 'master' of https://github.com/xakep139/ImageSharp

52af4e3

codestyle fixes

b435f5e

codestyle fix

9f719d5

antonfirsov reviewed Jan 17, 2018

View reviewed changes

JimBobSquarePants requested changes Jan 17, 2018

View reviewed changes

JimBobSquarePants reviewed Jan 17, 2018

View reviewed changes

antonfirsov changed the title ~~Add an equivalent for System.Drawing.Image.GetPixelFormatSize() (#258)~~ Basic metadata-only image parsing API Jan 18, 2018

denisivan0v added 2 commits January 18, 2018 17:13

bugfix in GifDecoderCore.Identity

e88e138

changes on code review

61c9caf

JimBobSquarePants reviewed Jan 19, 2018

View reviewed changes

JimBobSquarePants merged commit 61c9caf into SixLabors:master Jan 19, 2018

JimBobSquarePants mentioned this pull request Jan 19, 2018

Is there any equivalent for System.Drawing.Image.GetPixelFormatSize()? #258

Closed

4 tasks

antonfirsov mentioned this pull request Feb 8, 2018

Allow decoders to load metadata only #460

Closed

4 tasks

denisivan0v mentioned this pull request Apr 18, 2018

Disposing data chunk instead of explicit returning it to ArrayPool in PngDecoderCore.Identify #534

Merged

4 tasks

Uh oh!

Conversation

xakep139 commented Aug 10, 2017

Prerequisites

Description

Uh oh!

codecov bot commented Aug 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xakep139 Aug 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov commented Aug 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xakep139 commented Aug 18, 2017

Uh oh!

xakep139 commented Aug 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov commented Aug 24, 2017

Uh oh!

CLAassistant commented Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xakep139 commented Sep 26, 2017

Uh oh!

antonfirsov commented Sep 26, 2017

Uh oh!

xakep139 commented Sep 27, 2017

Uh oh!

denisivan0v commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antonfirsov commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JimBobSquarePants Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JimBobSquarePants left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 10, 2017 •

edited

Loading

xakep139 Aug 11, 2017 •

edited

Loading

antonfirsov commented Aug 13, 2017 •

edited

Loading

CLAassistant commented Sep 26, 2017 •

edited

Loading

denisivan0v commented Jan 17, 2018 •

edited

Loading

antonfirsov commented Jan 17, 2018 •

edited

Loading

antonfirsov Jan 17, 2018 •

edited

Loading

JimBobSquarePants Jan 17, 2018 •

edited

Loading

antonfirsov Jan 18, 2018 •

edited

Loading

xakep139 Jan 19, 2018 •

edited

Loading

JimBobSquarePants Jan 17, 2018 •

edited

Loading

antonfirsov Jan 17, 2018 •

edited

Loading