Scale Mode

[artnez]

No description provided.

[rafikk]

The deprecated behavior corresponds to fit, I believe.

[rafikk]

A couple of high-level comments:

1. I don't think the current values for crop_mode are sufficiently descriptive or powerful. For instance, my interpretation of the behavior of fill was that it would maintain the aspect ratio and draw outside the requested dimensions. I realize now that the name crop_mode indicates that it fills and crops instead. It seems like we want a more general scaling_mode or image_mode setting that has, for instance, aspect_fill and crop_fill. Take a look at the values for UIViewContentMode in iOS.
2. The scaleWand method shouldn't have any branching based on the crop or image mode. That logic should be managed entirely in getScaledDimensions. I think the drawback is a consequence of the design decision above. There should be a scaleWand and cropWand method and getScaledDimensions should probably return two sets of dimensions, 1 for scaling, and 2 for cropping. If the second is equal to the first (or maybe nil), no cropping occurs.

[artnez]

I don't think the current values for crop_mode are sufficiently descriptive or powerful.

Agreed. I'll re-think the naming and update this PR. Don't approve the PRs depending on this one for now. I'll need to rebase those once this one is merged.

The scaleWand method shouldn't have any branching based on the crop or image mode.

Good idea. First it would apply scale, then apply crop.

[artnez]

(but feel free to review those anyway so I can make changes when I work on halfshell again)

[rafikk]

I don't want to lose momentum on merging these changes in. Let me know if you want me to help take on any of these changes.

[artnez]

Busy week. Planning on picking it up on again on Fri.

[artnez]

I'll do some more testing tomorrow. In the meantime here's a summary of the refactor with justification behind each major change.

Image is now a simple wrapper around a MagickWand object. The idea is to create a wand as early as possible to minimize copying memory later. Plus we need the wand anyway. This also greatly simplifies the image manipulation functions because they don't need to return a modified state.

ProcessImage doesn't return an image struct anymore. It's now assumed that the processor will mutate the existing image object instead of creating a new one. Creating a new image object is never necessary due to the above changes. And if a future processor ever needs to completely override the image, imagick provides functions to do so. Absolute worst case scenario, a pointer swap would work too.

NewImageFromHTTPResponse is now NewImageFromBuffer. The HTTP body is a readable buffer so we leave all the HTTP stuff in server.go and pass the buffer down to the stuff in image.go. NewImageFromFile doesn't use the imagick method because that method just didn't work. Doesn't matter though, loading the buffer works fine.

You'll see a lot of switching to uint from uint64 (for dimensions mostly). Idea there is to cast to uint as early as possible. The imagick library uses uint for image dimensions so it's better to use uint for the ImageDimensions struct as well.

MaxImageWidth and MaxImageHeight are now a single ImageDimensions struct. Easier to deal with later in the code.

Instead of implementing all of the ContentMode interface you pointed out, I just implemented the scale part. The focalpoint parameter (in a separate PR) handles positioning the image within the crop region.

Took a couple functions out of the imageProcessor struct. In the future we can move them into a separate utility namespace for handling dimensions. I plan on adding tests soon and these functions will be easier to test when they're decoupled from imageProcessor.

[artnez]

The reason why Image wasn't removed entirely is that we'll probably need to attach some metadata to the Image struct eventually.

[artnez]

Ok I think it's ready to go. If it looks good to you I'll update the remaining PRs to cleanly merge on top of this one.

[rafikk]

I really don't like using implicit named return values. Can you leave this line as is?

[artnez]

Thanks I'll will make the fixes today. Bummer about implicit returns, but I understand.

Are you good with the renaming/removal of the *Wand functions? Long term I'm thinking about moving image transforms to a processor package that conforms to the image processor interface. Hence the rename (transforms would operate on the image, not just the wand).

[artnez]

Gonna take a little time remove the implicit returns. I'll rebase everything into a couple commits when it's done.

[rafikk]

I'm having trouble tracking it down, but there's a Q&A session with the Go team where Rob Pike says including implicit returns in Go was "probably" a mistake.

I'm definitely in favor of removing the Wand methods, I think the more we can abstract away IM, the better.

Along those same lines, what do you think about changing Image to be an interface instead of a struct type? especially now that it has a set of methods that define it, e.g. GetBytes, Signature, etc. The imagemagick processor can have its own struct that embeds the wand, but that should be hidden from the caller.

[artnez]

Rebased and squashed into a few commits.

I'm having trouble tracking it down, but there's a Q&A session with the Go team where Rob Pike says including implicit returns in Go was "probably" a mistake.

I remember watching that too. I think it was Griesemer that said it. The official stance is use it if it makes things better and don't if it doesn't. It's never tripped me personally so I like it. I don't feel strongly enough either way so I removed it. Consistency is better anyway.

Along those same lines, what do you think about changing Image to be an interface instead of a struct type? especially now that it has a set of methods that define it, e.g. GetBytes, Signature, etc. The imagemagick processor can have its own struct that embeds the wand, but that should be hidden from the caller.

Agreed (for the future though). We can move the image processing functions (blur, etc) into the new image struct for imagick. The image processor simply defines processing strategy based on request parameters (blur + resize + crop). And the image actually applies them.

[artnez]

ping, should be ready to merge.

[rafikk]

Sorry for sitting on this. Looks great. The only question I have is whether aspect_fill behaves correctly and if it shouldn't instead be split up into two scale modes: one that fills and crops to fit the requested dimensions, and one that resizes to fill the requested dimensions but doesn't crop. E.g. it constrains on the more constrained dimension and returns an image that is at least as large as the requested dimensions. Thoughts?

[artnez]

Doesn't aspect_fit satisfy the latter? If I'm wrong about that, I'd be in favor of picking a new name for aspect_fit and leave the latter suggestion for another time. The 3 modes fit a pretty broad range of use cases for me.

[rafikk]

No, it doesn't. As an example, let's say we have an image that's 500x800 pixels and I request an image that's 400x400.

If it's set to aspect_fit, I should get an image that's 250x400.
If it's set to aspect_fill, I should get an image 400x640.
If it's set to aspect_crop, or crop_fill or something like that, I would get an image that's 400x400, where 120 pixels were cropped off from the top and bottom respectively.

We're missing the second behavior right now. I can definitely see a case for why you would want that. I'm fine with punting it to another release, but I'm afraid of using the aspect_fill name where it could mean multiple things.

[artnez]

Aha. You're right! Adding all 3 should be trivial so I'll do that. I like aspect_crop for the 3rd option. Objections?

[rafikk]

Go for it!

[artnez]

Ready to go with docs.

[rafikk]

Awesome, dude. I'm excited about this. I'm going to take a stab at the Image interface changes.

[artnez]

Baller. I'll refactor the other PRs on top of this refactor.