Zola: resize_image should be a no-op when the image has target dimensions

[muhuk]

Bug Report

Environment

Zola version: 0.9.0

Expected Behavior

When resize_image is called on an image that already has the targeted dimensions:

1. No image should be generated in processed_images
2. Source image's path should be returned to the template.

Current Behavior

A new copy of the source image is created in processed_images unnecessarily.

Step to reproduce

1. Call resize_image on an image that already has the targeted dimensions.

[Keats]

That does depend on which operation is being used but overall it makes sense to not copy the image I think. The function should return the path to the original file.

cc @vojtechkral

[muhuk]

If this gets accepted, I am happy to take a crack at it.

[vojtechkral]

Yeah, this makes sense. I don't think we have any op where performing scaling would make sense if the dimensions already match.

[Keats]

@muhuk still interested in making a patch for that?

[muhuk]

Yes, I still want to take this. I was just busy with other stuff. Expect a patch by Tuesday next Friday latest. :+1:

[muhuk]

I am looking at the code.

Pipeline seems to be:

• In site component:
  
  
  
  • Arc> is instantiated and passed to ResizeImage tera function.
  
  
• In templates component:
  
  
  
  • global_fns::ResizeImage processes the template
  
  
  • Ops are stacked in imageproc::Processor (Arc<Mutex<..>>)
  
  
  • We get the URL, before running any image ops.
  
  
• In site component:
  
  
  
  • Site::build calls imageproc::Processor::do_process
  
  

---

I have a question: when I tried to chain resize_image commands in the template it failed because the 2nd call (outer) was not able to find the file (url generated but file not written yet). Is there a way to stack ops now? (I understand calling resize twice is stupid but I guess this feature was added to have more ops in the future)

I am asking this because I am trying to figure out how to use the original file's URL, Processor relies on a randomly generated URL now.
(Nevermind this, I get the Processor is collecting operations on different files, not a stack of ops on a single file)

[muhuk]

This is going to be tricky.

• My first idea was to add ResizeOp::NoOp, but ImageOp::from_args does not receive image path, so I can't open the file and check the original file's dimensions here and return NoOp. (I was thinking Processor would set the source and target paths same, or something like that)
• ImageOp::perform gets to know the original image's path, but at that point Processor has already generated a hashed filename and it got printed to the rendered template. So we are too late to use the original image's path.

@vojtechkral @Keats am I missing something?

[vojtechkral]

@muhuk I'll have a look ...

[vojtechkral]

Ok, so the way this works is that the resize_image global fn only collects the ops and doesn't do any processing on its own, the processing is done later in a batch (it would be actually quite difficult to not have it this way).

One way to solve this is to have ImageOp::from_args() return a Result<Option<ImageOp>> rather than just Result<ImageOp> and it could detect a no-op and return an Ok(None) in case of no-op.
ImageOp::from_args() could read the image dimensions using image::image_dimensions().
The code in ResizeImage::call() could then simply return the original URL and not insert the op in the imageproc at all. _However_ you also probably need to take care of whether the requested format also matches, not just the dimensions.

@Keats I also noticed the code in get_image_meta() uses image::open() ... this doesn't seem right; image::open() loads the whole image into memory. image::image_dimensions() should also be used here instead I think.

[muhuk]

Hi @vojtechkral

Thanks for looking into this.

One way to solve this is to have ImageOp::from_args() return a Result> rather than just Result and it could detect a no-op and return an Ok(None) in case of no-op. ImageOp::from_args() could read the image dimensions using image::image_dimensions().
The code in ResizeImage::call() could then simply return the original URL and not insert the op in the imageproc at all. However you also probably need to take care of whether the requested format also matches, not just the dimensions.

Thing is; ImageOp::from_args gets the logical path (relative to content_path), not the file path on disk. They way I understand it; real file path is constructed from content_path & ImageOp.source when ImageOp::perform is called. (see line 267, lib.rs)

Also we cannot change the URL after Processor::insert is called AFAICU.

BTW, this design makes sense to me. But some decisions made earlier to keep things simple kinda makes this supposedly small change a big one. Now that I know the codebase a little better I am not sure if the added complexity is worth the benefits of this feature.

[vojtechkral]

@muhuk Ok, I'll do some refactoring...

[muhuk]

Sure.

Just to be clear; this design imposes constraints (like any other), but it still makes sense to me.

[vojtechkral]

@muhuk no worries :)

[vojtechkral]

fyi, I've got some refactoring done, I'm now trying to figure out how to add some tests...

Edit: Ok, figured out how to test site generation, but now I discovered bugs that I need to fix :)

[muhuk]

Cool. Looks like they haven't landed in the repo yet. (I checked master & next)

[vojtechkral]

Yeah, I've got it only locally so far. I'll create a PR once it's done.

[vojtechkral]

Hi. Facing some other problems I completely forgot about this issue. I was meaning to report here earlier...

In any case, I tried to implement the optimization, but ran into a couple of implementation issues, I remember the most difficult one was that the image can be sourced from a location outside of the content or static dirs, ie. inaccesible from the web. This would need to be detected, but that proved difficult. I don't remember all of the details since it was months ago, but overall the number of difficulties I ran into made me conclude that this is not worth implementing. It would only help in a special cases anyway.

Even so, I did some useful refactoring as part of the attempt which might still be worth contributing.

[Keats]

@vojtechkral please send a PR!

[vojtechkral]

@Keats I'm facing some IRL issues now, but I'll try as possible...

[Keats]

No worries take as much time as needed.
I'm closing this issue in the meantime.