theotherphil / imagecli

A command line image processing tool
MIT License
262 stars 5 forks source link

Replacing benchmark implementation with Criterion and other small changes #60

Closed hbina closed 3 years ago

hbina commented 3 years ago

You cannot currently call cargo bench right now,

PS C:\Users\Hanif\git\imagecli> cargo bench
   Compiling imagecli v0.2.1 (C:\Users\Hanif\git\imagecli)
error[E0554]: `#![feature]` may not be used on the stable release channel
 --> src/lib.rs:8:19
  |
8 | #![cfg_attr(test, feature(test))]
  |                   ^^^^^^^^^^^^^

error: aborting due to previous error

For more information about this error, try `rustc --explain E0554`.
error: could not compile `imagecli`

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: build failed

And bench is slated to be deprecated. The fix is to use criterion instead. Also minor change to not use "" to represent empty pipeline but use empty vec instead... IDK its just my brain screaming for this change, I can undo it you don't want it.

Aside: Why does the program loads all the images in the folder first? Can't it just load whatever is necessary? Is it done this way because the pipeline can be arbitrarily complicated and figuring those out is just too much.

I ask because my use-case is to downscale entire images in a folder from 2560x1600 to 1600x900 and its quite slow.

Note: This branch is on top on the previous PR, I will rebase later (assuming that one is merged, else I will undo those as well).

theotherphil commented 3 years ago

cargo bench has only ever worked in nightly. Replacing it with criterion seems reasonable for an easier user experience.

As an aside, I'd be interested in a source for the claim that cargo bench is slated for deprecation - my understanding is that there has been a lot of discussion around stabilisation that is just not going anywhere due to lack of available time to work on this.

This library loads images eagerly solely because this was quickest to implement. Lazy image support could be added by just making everything in the image stack an Either(Image, ImageLoader). However, this has the downside that you need to deal with potentially having pointers in your image stack to files that are moved or deleted before you get to them. A better option might just be to handle this via a top level option to apply a pipeline to every image in a directory and load the images as you prepare to run the pipeline for them. I'd be happy to see this change made, but don't have the time available for this library to do it myself.

theotherphil commented 3 years ago

I've commented on one minor issue, but otherwise this LGTM. Thanks!

theotherphil commented 3 years ago

I've commented on one minor issue, but otherwise this LGTM. Thanks!

theotherphil commented 3 years ago

Thanks!