API Image frames refactor #326

tocsoft · 2017-09-09T10:25:21Z

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 has a few API changed and a significate internal refactor of where pixel data is stored.

refactors the internals of where image data is stored such that all pixel data is now help within ImageFrames, every Image will always have at least one image frame (unless its been disposed).
hides accesses Span apis into the SixLabor.ImageSharp.Advanced namespace as these APIs are "Use at your own risk".
Adds the missing SavePixelData() apis to pair with the LoadPixelData apis to access the raw byte array holding the image pixels data.

moved some extensions into new namespace

TODO: I do not like this, far to flaky.

… and remove the one from Image<TPixel>.

JimBobSquarePants

Looks good to me.

Quick question. Do you think we should seal our Image and ImageFrame classes? I'm thinking of people attempting to build things like integral images.

https://github.com/andrewkirillov/AForge.NET/blob/master/Sources/Imaging/IntegralImage.cs

They could inherit our image class and utilize the available API.

tocsoft · 2017-09-10T11:16:06Z

I think it would be almost impossible at the moment to make that work for the current version... we would have to make a heap more of the internals public.

I feel we should leave it sealed for v1, we can always relax things as people hit the limits of our current implementation, I feel moving over to an interface based version of the API would end up being the better choice than just unsealing Image.

JimBobSquarePants · 2017-09-10T11:46:55Z

@tocsoft I can live with that for now.

antonfirsov · 2017-09-10T14:04:05Z

@JimBobSquarePants I think we should keep our stuff sealed. People should follow practices like composition over inheritance for these cases like integral images.

JimBobSquarePants · 2017-09-11T06:52:19Z

Just noticed this issue, can we handle this better now?

#284

tocsoft · 2017-09-11T07:09:59Z

sort of we would still need and Image<T> to encode from so we could create a temp Image with just the one frame (which due to the fact we store all data in frames now be a relatively cheep operation) and then just encode that... would need to think about the best way though... would probably be better to have configuration on the encoders to let use specify which FrameIndex to treat as the source of data to encode (excluding gif of course)

antonfirsov · 2017-09-11T09:28:03Z

src/ImageSharp/Image/ImageFrameCollection.cs

+        /// <summary>
+        /// Gets the root frame.
+        /// </summary>
+        public ImageFrame<TPixel> RootFrame => this.frames.Count > 0 ? this.frames[0] : null;


Is it valid to have 0 frames? Shouldn't we throw in the constructor instead?

yes you can have zero frames, it will have zero when the ImageFrameCollection as been disposed.

Q1: Shouldn't we throw ObjectDisposedException instead of returning null then?
Q2: Looks like it's possible to dispose an ImageFrameCollection without disposing the underlying image bringing Image<T> into an invalid state. Shouldn't we drop IDisposable from ImageFrameCollection<T> API and dispose frames directly from Image<T>.Dispose()?

yes we should probably throw an ObjectDisposedException.

I don;t think its really an issue that you can dispose of the ImageFrameCollection without disposing of the image. You can do the same for ImageFrame too. I suppose we could have the Image expose and interface of IImageFrameCollection that doesn't support IDisposeable then the users can't call dispose of the collection separately to the Image.

antonfirsov · 2017-09-11T09:35:53Z

src/ImageSharp/Image/ImageFrameCollection.cs

+        /// </summary>
+        /// <param name="index"> The zero-based index at which item should be inserted..</param>
+        /// <param name="frame">The <seealso cref="ImageFrame{TPixel}"/> to insert into the <seealso cref="Image{TPixel}"/>.</param>
+        public void Insert(int index, ImageFrame<TPixel> frame)


Is it really a good idea to expose a mutable List API on ImageFrameCollection? Feels like the API surface is to wide now.
I'd rather keep it as an immuatble collection. If someone wants to implement a gif frame manipulator tool with ImageSharp (looks like a very uncommon use case), he should probably construct new image instances after calculating the frames.

if its immutable then you can't create an animated gif.. also the public api doesn't allow users to specify frames so the only way for a user to go from zero to animated gif is to create a series of images, merge their frames together into a single image instance and then save that.

It's probably matter of API taste, but I would utilize builder pattern instead, having some kind of ImageBuilder object to construct multi-frame images. This looks like a more convenient way to preserve valid object state. Constructing ImageFrame instances of a certain size, and verifying it against the image dimensions when calling ImageFrameCollection.Add() looks a bit clanky to me.

But looks like ImageSharp API follows mutable style on many other places (like ImageMetaData) so maybe it's easier to conform to this style everywhere for 1.0.

How about encapsulating the consistance rules into ImageFrameCollection by hiding ImageFrame<T> constructors and define methods on ImageFrameCollection<T>:

public ImageFrame<T> AppendNewFrame() { } public ImageFrame<T> InsertNewFrame(int index) { }

?

@JimBobSquarePants thoughts?

I had thought about adding those as APIs (well in concept not name, couldn't think of what to call them) but I like it.

This makes me think we should find a way to extend the Mutate() apis to allow us to apply mutations to single frames... might need to extend expand IImageProcessors, as we will need IFrameProcessors as well as IImageProcessors will also have to scope some of the APIs to allow targeting only images and not frames (resize, crop, rotate etc) unless we allow images to have frames with multiple dimensions and handle normalising frame sizes on a per encoder bases instead of during add frame (i.e. update gif to draw based on the first frame dimensions)

@antonfirsov I see where you're going there and it does seem safer. As far as I can see we don't actually new up a frame anywhere within our codebase.

@tocsoft I'd rather we treated images as a whole and avoid adding IFrameProcessor's. Seems like we'd be creating a complexity level that would be difficult to follow.

I think we should leave it as is for now then... we can worry about that later if/when people hit issues with the API.

antonfirsov · 2017-09-11T10:00:25Z

Everything LGTM except the mutable ImageFrameCollection API. It's much easier to maintain a consistent Image<T> state if we close down that list.

tocsoft · 2017-09-12T16:39:08Z

@SixLabors/core so we happy enough now with this to merge into beta?

antonfirsov · 2017-09-12T16:49:08Z

We are really short on time, so OK for now.
In long term I'm planning to suggest deep changes to the frame API, because I'm not happy with the "greatest common divisor" approach we used to pick the gif model as an universal model for Image<TPixel>.
Hope we can go for an early 2.0 ...

tocsoft · 2017-09-12T20:43:49Z

ok merging this.. we can continue any final tweeks on the beta branch

API Image frames refactor

tocsoft and others added 21 commits September 4, 2017 23:09

wip - pixel data only on image frames

07e9f23

fix frame double processing

d8a8211

moved some extensions into new namespace

fix stylecop issues

05bb7c2

Fix AutoOrientProcessor

48fbc4f

Remove Parent property

1b13599

Fix null reference execption

a478aed

TODO: I do not like this, far to flaky.

Use RootFrame for validation

40433d8

exlusivly use frames to access pixel spans

ee60eb3

pass configuation into processors

09a667d

remove IImageFrame interface

31ad615

Minor refactor.

bd0d188

Added new extension method for IConfigurable to get the configuration…

cd75248

… and remove the one from Image<TPixel>.

Silence warnings.

10926cc

Move extension method back to Image<Pixel> to keep it public.

46cf550

Minor refactor and added Guard for null image.

53e1741

Fix recursion plus cleanup

b708773

Adjust for new frame cont in Gif encoder

13b19b9

Undo my silliness

3eb75e6

FrameCollection responsible for adding initial frame

41358ce

expose span APIs on image

176920a

merge in beta-1

c63e0d2

tocsoft requested review from JimBobSquarePants, antonfirsov and dlemstra September 9, 2017 10:25

JimBobSquarePants added 3 commits September 10, 2017 20:27

Fix spelling

22d5163

Remove unused local variables

2dc3c4f

Reuse comparable swap

19b4504

JimBobSquarePants approved these changes Sep 10, 2017

View reviewed changes

antonfirsov reviewed Sep 11, 2017

View reviewed changes

introduce IImageFrameCollectino to hide disposable

db80a7f

tocsoft added 3 commits September 12, 2017 18:50

Merge beta-1 into image-frames

a09cf72

Merge remote-tracking branch 'origin/beta-1' into image-frames

6ecae1b

Merge remote-tracking branch 'origin/beta-1' into image-frames

13a40ae

tocsoft merged commit 6f4144c into beta-1 Sep 12, 2017

tocsoft deleted the image-frames branch September 14, 2017 18:22

antonfirsov pushed a commit to antonfirsov/ImageSharp that referenced this pull request Nov 11, 2019

Merge pull request SixLabors#326 from SixLabors/image-frames

0810141

API Image frames refactor

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API Image frames refactor #326

API Image frames refactor #326

tocsoft commented Sep 9, 2017

JimBobSquarePants left a comment

tocsoft commented Sep 10, 2017 •

edited

Loading

JimBobSquarePants commented Sep 10, 2017

antonfirsov commented Sep 10, 2017

JimBobSquarePants commented Sep 11, 2017

tocsoft commented Sep 11, 2017

antonfirsov Sep 11, 2017

tocsoft Sep 11, 2017

antonfirsov Sep 11, 2017 •

edited

Loading

tocsoft Sep 11, 2017

antonfirsov Sep 11, 2017

tocsoft Sep 11, 2017

antonfirsov Sep 11, 2017

tocsoft Sep 11, 2017

JimBobSquarePants Sep 12, 2017

tocsoft Sep 12, 2017

antonfirsov commented Sep 11, 2017

tocsoft commented Sep 12, 2017

antonfirsov commented Sep 12, 2017

tocsoft commented Sep 12, 2017

API Image frames refactor #326

API Image frames refactor #326

Conversation

tocsoft commented Sep 9, 2017

Prerequisites

Description

JimBobSquarePants left a comment

Choose a reason for hiding this comment

tocsoft commented Sep 10, 2017 • edited Loading

JimBobSquarePants commented Sep 10, 2017

antonfirsov commented Sep 10, 2017

JimBobSquarePants commented Sep 11, 2017

tocsoft commented Sep 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov commented Sep 11, 2017

tocsoft commented Sep 12, 2017

antonfirsov commented Sep 12, 2017

tocsoft commented Sep 12, 2017

tocsoft commented Sep 10, 2017 •

edited

Loading

antonfirsov Sep 11, 2017 •

edited

Loading