Skip to content

Add Extract/Clone apis for manipulating deconstructing images from from#344

Merged
JimBobSquarePants merged 4 commits intomasterfrom
tocsoft/extract-clone-frames
Sep 21, 2017
Merged

Add Extract/Clone apis for manipulating deconstructing images from from#344
JimBobSquarePants merged 4 commits intomasterfrom
tocsoft/extract-clone-frames

Conversation

@tocsoft
Copy link
Member

@tocsoft tocsoft commented Sep 19, 2017 •
edited
Loading

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 adds the new APIs to Image<TPixel> for cloning and extracting frames from an image.

New APIs

  • Image<TPixel>.Clone(int frameIndex)
  • Image<TPixel>.CloneAs<TPixel2>(int frameIndex)
  • Image<TPixel>.Extract(int frameIndex)

TODO

  • Add Tests
  • Resolve API location
    should these APIs be members of Image<T> of ImageFrameCollection?
    i.e. do you call image.Clone(index) or image.Frames.CloneFrame(index)
  • Resolve API names
    should the apis have the Frame postfix? I feel this depended on if the API lives on the frames collection or the image

resolves #340

@JimBobSquarePants
Copy link
Member

JimBobSquarePants commented Sep 19, 2017

The Frame suffix seems nice and explicit to me but I don't think that the APIs should live in the ImageFrameCollection. (Don't ask me why it just doesn't feel right)

@antonfirsov
Copy link
Member

antonfirsov commented Sep 19, 2017

@JimBobSquarePants I think you have this feeling because all of them are returning an Image<T> instance. This makes them more bound to Image<T> than ImageFrameCollection<T> where everything operates on frame instances only.

@JimBobSquarePants
Copy link
Member

JimBobSquarePants commented Sep 19, 2017

@antonfirsov Yeah, that describes it well.

@tocsoft tocsoft changed the title [WIP] Add Extract/Clone apis for manipulating deconstructing images from from Add Extract/Clone apis for manipulating deconstructing images from from Sep 21, 2017
@antonfirsov
Copy link
Member

antonfirsov commented Sep 21, 2017 •
edited
Loading

@tocsoft All LGTM. Like that we manage the lifecycle of frames through our API now, preserving valid state for Image<T>.

We probably need a gif manipulator example, because the behavior of some functions (eg. AddFrame(otherFrame)) is not trivial now.

@tocsoft tocsoft mentioned this pull request Sep 21, 2017
Open
4 tasks
JimBobSquarePants
JimBobSquarePants approved these changes Sep 21, 2017
View reviewed changes
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.

Very happy with this. It's come together very well!

@JimBobSquarePants JimBobSquarePants merged commit 60b7ab1 into master Sep 21, 2017
@tocsoft tocsoft deleted the tocsoft/extract-clone-frames branch September 21, 2017 17:59
@antonfirsov antonfirsov mentioned this pull request Sep 22, 2017
Merged
4 tasks
antonfirsov pushed a commit to antonfirsov/ImageSharp that referenced this pull request Nov 11, 2019
@JimBobSquarePants
Merge pull request SixLabors#344 from SixLabors/tocsoft/extract-clone…
6724baa 
…-frames

Add Extract/Clone apis for manipulating deconstructing images from from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JimBobSquarePants JimBobSquarePants JimBobSquarePants approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Clone/Extract frames from image

3 participants

@tocsoft @JimBobSquarePants @antonfirsov