-
-
Notifications
You must be signed in to change notification settings - Fork 854
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
Basic metadata-only image parsing API #292
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
@@ -22,7 +22,7 @@ public class TestFileSystem : ImageSharp.IO.IFileSystem | |||
|
|||
public static TestFileSystem Global { get; } = new TestFileSystem(); | |||
|
|||
public static void RegisterGloablTestFormat() | |||
public static void RegisterGlobalTestFormat() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix typo
@@ -23,15 +23,15 @@ public class TestFormat : IConfigurationModule, IImageFormat | |||
{ | |||
public static TestFormat GlobalTestFormat { get; } = new TestFormat(); | |||
|
|||
public static void RegisterGloablTestFormat() | |||
public static void RegisterGlobalTestFormat() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix typo
tests/ImageSharp.Tests/TestFormat.cs
Outdated
return this.testFormat.Sample<TPixel>(); | ||
} | ||
|
||
public int DetectPixelSize(Configuration configuration, Stream stream) | ||
{ | ||
throw new NotImplementedException(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not implemented for test format
@@ -1238,7 +1254,7 @@ private void ProcessStartOfFrameMarker(int remaining) | |||
|
|||
if (h == 3 || v == 3) | |||
{ | |||
throw new ImageFormatException("Lnsupported subsampling ratio"); | |||
throw new ImageFormatException("Unsupported subsampling ratio"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix typo
|
||
[Theory] | ||
[InlineData(TestImages.Gif.Cheers, 8)] | ||
[InlineData(TestImages.Gif.Giphy, 8)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For these two GIFs Image.GetPixelFormatSize()
returns 32
which is not correct
TestFileSystem.Global.AddFile(this.FilePath, this.DataStream); | ||
} | ||
|
||
[Fact] | ||
public void LoadFromStream() | ||
{ | ||
Image<Rgba32> img = Image.Load<Rgba32>(this.DataStream); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation here
@xakep139 nice job! |
@antonfirsov I apologize for delayed response. Your idea is pretty good, I'll implement it. |
@antonfirsov do you have other suggestions? |
tests/ImageSharp.Tests/TestImages.cs
Outdated
@@ -25,6 +25,9 @@ public static class Png | |||
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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in d6a2d40
@xakep139 no, the @JimBobSquarePants any suggestions to manage this? |
@antonfirsov I've synced the branch with upstream |
@xakep139 thanks! (And also thanks for your patience!) Someone has to refactor your work to the |
@antonfirsov, yes, sure |
@xakep139 Sorry for the late response! I gonna deal with this during my ImageSharp hackaton in Feburary. It could be related to #430, because we should probably use the same I would probably move the method internal interface IPixelTypeDetector
{
PixelTypeInfo DetectPixelType(Configuration configuration, Stream stream)
}
public class PngDecoder : IImageDecoder, IPixelTypeDetector
{
...
} @JimBobSquarePants I really think this is a valuable feature, and we should merge this. Thoughts about my ideas? |
@xakep139 @antonfirsov Refresh my memory. What information are we seeking from this API and what is it's use case? |
In our system we should check bit depth of uploaded images, it's technical requirement of another system |
In general it could be usefull to get metadata without loading (decoding) the whole image in memory. E.g. to check image size (width, height), get pixel type (RGB, RGBA, etc.) and bit depth (or a pixel size). |
Thanks @xakep139 @antonfirsov Would we then use reflection to determine which decoders support what we need? |
we would register them the same we we do with the encoders/decoders i.e. we would add |
Actually, I wanted to keep things as dumb as: public static class Image
{
...
public static bool TryDetectPixelType(Configuration config, Stream stream, out PixelTypeInfo pixelTypeInfo)
{
IImageDecoder decoder = DiscoverDecoder(stream, Configuration config);
IPixelTypeDetector detector = decoder as IPixelTypeDetector;
if (detector == null) return false;
pixelTypeInfo = detector.DetectPixelType(config, stream);
return true;
}
...
} I'm afraid that going any further, and exposing |
Yeah let's leave it simple there and try not to expose another public API yet. |
- intoduced base IImage interface - introduced IImageInfoDetector interface - Image.DetectPixelType method expanded to Identify method that returns IImage
@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.
Hope it will also help to resolve #430. I'd be much appreciated for any suggestions and comments. |
@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: 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. |
src/ImageSharp/Image/IImage.cs
Outdated
/// <summary> | ||
/// Gets information about pixel. | ||
/// </summary> | ||
PixelTypeInfo PixelType { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 onImage<TPixel>
. - We need to include Width+Height (or maybe
SixLabors.Size
?) into the metadata-only result. Currently it's not exposed onImageMetadata
.- @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 solved this by defining
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 { ... }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, let's do the rename. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this is very good! 🥇
Just some minor changes and I'm happy for it to go in. Thanks for persisting with us! 😄
src/ImageSharp/Image/IImage.cs
Outdated
/// <summary> | ||
/// Gets information about pixel. | ||
/// </summary> | ||
PixelTypeInfo PixelType { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
src/ImageSharp/Image/IImage.cs
Outdated
public interface IImage | ||
{ | ||
/// <summary> | ||
/// Gets information about pixel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets flesh out the property descriptors here. This doesn't tell me anything and wont help with docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 61c9caf.
@@ -242,6 +295,7 @@ private void ReadLogicalScreenDescriptor() | |||
{ | |||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was hoping this wouldn't trip you up. Good research. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this was initially done by @xakep139 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -327,7 +385,7 @@ private void ReadFrame() | |||
indices = Buffer<byte>.CreateClean(imageDescriptor.Width * imageDescriptor.Height); | |||
|
|||
this.ReadFrameIndices(imageDescriptor, indices); | |||
this.ReadFrameColors(indices, localColorTable ?? this.globalColorTable, imageDescriptor); | |||
this.ReadFrameColors(ref image, ref previousFrame, indices, localColorTable ?? this.globalColorTable, imageDescriptor); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Image<T>
and ImageFrame<T>
are already reference types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/ImageSharp/Image/ImageInfo.cs
Outdated
|
||
namespace SixLabors.ImageSharp | ||
{ | ||
internal sealed class ImageInfo : IImage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add xml docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 61c9caf
namespace SixLabors.ImageSharp.Formats | ||
{ | ||
/// <summary> | ||
/// Stores information about pixel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See this #292 (comment).
namespace SixLabors.ImageSharp.Tests.TestUtilities.ReferenceCodecs | ||
{ | ||
public class SystemDrawingReferenceDecoder : IImageDecoder | ||
using SixLabors.ImageSharp.MetaData; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why were the namespaces altered in this class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@antonfirsov @JimBobSquarePants huge thanks for your review and suggestions! I'm going to make changes asap to reduce time to the next review cycle. |
@@ -17,6 +17,7 @@ | |||
</ItemGroup> | |||
<ItemGroup> | |||
<PackageReference Include="CoreCompat.System.Drawing" Version="1.0.0-beta006" /> | |||
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.5.0" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This enables unit tests execution in JetBrains Rider, but it's not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep it then. I want everyone to be able to test the project in their desired IDE.
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! |
Prerequisites
Description
Implemented feature request #258