Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Image Sizes #10181

Closed
3 of 4 tasks
JohnONolan opened this issue Nov 19, 2018 · 19 comments
Closed
3 of 4 tasks

Image Sizes #10181

JohnONolan opened this issue Nov 19, 2018 · 19 comments
Assignees
Labels
server / core Issues relating to the server or core of Ghost

Comments

@JohnONolan
Copy link
Member

JohnONolan commented Nov 19, 2018

A couple of months ago we implemented basic image resizing and compression support with Sharp, the second half of the work that needed doing was supporting multiple image sizes in Ghost.

For example: You upload a 5,000px wide feature_image to a post.

  • Step 1: Resize to max 2,000px and enable basic compression to load image on post.hbs
  • Step 2: Generate smaller thumbnails to be used elsewhere on the size, eg. index.hbs

There's already been a lot of discussion about this which has gone undocumented, so I'm going to summarise all the approaches and decisions I can remember.

  1. We want to generate resized images on-request. So an image request takes place with a "width=300" setting, and if the image doesn't already exist then we generate it on the fly.
  2. We want to limit the allowed-requests to an explicit set of sizes to prevent DDoS/abuse
  3. To start with those sizes will be defined by theme package.json

In short, this will look like a very basic implementation of cloudinary fetch with a couple of transformation options.


Implementation Details

The theme determines the sizes it wants/needs to use based on its own layout design, and specifies those in its package.json. For example, in Casper we smaller thumbnails on the index page, eg demo.ghost.io

"image_sizes" {
  "small": {
    "width": 320,
    "height": 200,
    "crop": "fill"
  },
  "medium": {
    "width": 500,
    "height": 200,
    "crop": "fill"
  },
  "large": {
    "width": 700,
    "height": 400,
    "crop": "fill"
  }
}

For image sizes to be valid they must specify both width and height. Crop mode is optional and defaults to fill. Have no particular desire to replicate every cropping mode which sharp provides, nor any of the entropy based cropping.

  • Our fill => sharp cover
  • Our fit => sharp contain

Once sizes are specified, they can be passed as paramaters to the img_url helper in Ghost themes, eg:

<img src="{{img_url feature_image size="small"}}" alt="{{title}} />

Once a request is made for an image, as long as a valid size is provided, then Ghost returns the image at the requested size and stores it for re-use in future. There is no spec for what this looks like, but I would guess that we would generate image files based on size names and store them alongside the master images. Eg.

  • file.jpg
    • file-small.jpg
    • file-medium.jpg
    • file-large.jpg

Questions

Unknown: How will this work with a headless front-end eg. Gatsby?

Tasks

  • Add size attribute to img_url helper
  • Add automatic image thumbnail generation
  • Validate image sizes in gscan
  • Update Casper
@JohnONolan
Copy link
Member Author

This issues should close #4453

@allouis
Copy link
Contributor

allouis commented Nov 19, 2018

Initial go at the img_url helper - it uses . to delimit the filename and the size e.g. img.jpg and img.small.jpg #10182

EDIT:

Updated it to actually store so it's like:
original = /some/path/to/images/pic.jpg
small = /some/path/to/images/small/pic.jpg

This allows use to really easily invalidate the images when the theme changes smth like

imageSizes.forEach(size => deleteDirectory(imagesDirectory + '/' + size))

@notanengineercom
Copy link
Contributor

@JohnONolan I worked this weekend on the same feature. #10183
Maybe it could be useful

@naz naz added the server / core Issues relating to the server or core of Ghost label Nov 19, 2018
@JohnONolan
Copy link
Member Author

@notanengineercom Thanks! We'll definitely take a look. Image handling is sort of the third-rail of Ghost at the moment, so there's a lot of nuance to implementing this in a way which is broadly compatible for all users. Appreciate the PR either way though!

@notanengineercom
Copy link
Contributor

notanengineercom commented Nov 19, 2018

Yes I'm aware and I agree that it has to be more compatible/flexible @JohnONolan. I just made the change to process the images on upload, and without changing the image ratio. I also changed some of the image processing in the image manipulator in lib/images/ (enabled cache for unix, changed the processing kernel and added imagemin which helps alot with the compression problem that sharp sometimes gives #10144).

@allouis allouis self-assigned this Dec 4, 2018
allouis added a commit that referenced this issue Dec 13, 2018
refs #10181 

Adds support to request a size in the img_url helper using syntax like:
    <img src="{{img_url profile_image size="small"}}"/>

Requires the image_sizes config to be defined in the themes package.json
allouis added a commit that referenced this issue Dec 13, 2018
refs #10181 

* Added initial handleImageSizes middleware

* Implemented saveRaw method on local file storage

* Wired up handleImageSizes middleware

* Implemented delete for LocalFileStorage

* Removed delete method from theme Storage class

* Deleted sizes directory when theme is activated

* Ensured that smaller images are not enlarged

* Renamed sizes -> size

* Exited middleware as early as possible

* Called getStorage as late as possible

* Updated image sizes middleware to handle dimension paths

* Revert "Deleted sizes directory when theme is activated"

This reverts commit 9204dfcc73a6a79d597dbf23651817bcbfc59991.

* Revert "Removed delete method from theme Storage class"

This reverts commit b45fdb405a05faeaf4bd87e977c4ac64ff96b057.

* Revert "Implemented delete for LocalFileStorage"

This reverts commit a587cd6bae45b68a293b2d5cfd9b7705a29e7bfa.

* Fixed typo

Co-Authored-By: allouis <[email protected]>

* Redirected to original image if no image_sizes config

* Refactored redirection because rule of three

* Updated comments

* Added rubbish tests

* Added @todo comment for handleImageSizes tests

* Added safeResizeImage method to image manipulator

* Used image manipulator lib in image_size middleware
@zce
Copy link
Contributor

zce commented Dec 23, 2018

Maybe Ghost Content API also needs size parameters.

@kirrg001
Copy link
Contributor

kirrg001 commented Jan 2, 2019

@allouis Can we close this issue?

@allouis
Copy link
Contributor

allouis commented Jan 2, 2019

@kirrg001 Looks like " Validate image sizes in gscan" hasn't been completed - should we leave it open until that's done?

@kirrg001
Copy link
Contributor

kirrg001 commented Jan 2, 2019

I'd suggest to track this task in GScan as a separate issue, leave a comment here and close this one.

If we track this in GScan separately, maybe we get some help/attraction from the community.

@allouis
Copy link
Contributor

allouis commented Jan 2, 2019

Closing - the gscan issue can be tracked here TryGhost/gscan#173

@allouis allouis closed this as completed Jan 2, 2019
@luciandavidescu
Copy link

On-the-fly generation fails if nginx is configured to directly serve images. Is there any other way to trigger the resizing?

@allouis
Copy link
Contributor

allouis commented Jan 14, 2019

@luciandavidescu Unfortunately not, you could try adding a rule to nginx to forward /content/images/size uris to the ghost process.

@pysysops
Copy link
Contributor

@luciandavidescu the nginx try_files directive will be your friend here. Perhaps this post will help you: https://hashnode.com/post/how-to-make-nginx-read-static-files-directory-when-proxy-passing-cj1yw4esm007mzz53dj40vle2 with some experimentation / modification of course.

@luciandavidescu
Copy link

Indeed, thanks, possibly nailed it with this:

    location /content {
        alias /path/to/content;
	try_files $uri $uri/ @ghost;
        access_log off;
        expires max;
    }

    location @ghost {
        proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
        proxy_set_header X-Real-IP $remote_addr;
        proxy_set_header X-Forwarded-Proto $scheme;
        proxy_set_header Host $http_host;
        proxy_pass http://127.0.0.1:2368;
    }

Unfortunately, the resizing thing seems to no longer be working at all, for me at least, even after I turn these blocks off and restart ghost an nginx. For newly uploaded images, the helper {{img_url feature_image size="s"}} displays the expected url but there's an "error 500" on the resource. It started after updating to 2.11.0 so possibly code deprecation or a bug?

@kirrg001
Copy link
Contributor

@luciandavidescu Could you pls share the server (error) log? Do you have steps to reproduce for us?

@luciandavidescu
Copy link

luciandavidescu commented Jan 23, 2019

Below. Something that "sharp wasn't installed" apparently. Not sure if there's anything specific with my deployment to reproduce, just uploaded the image and expected it to show on the frontend, it did work with 2.10...

Console:
Failed to load resource: the server responded with a status of 500 ()

Ghost error log (domain ctrl-h'd to example.com):
{"name":"Log","hostname":"ip-172-26-4-161","pid":550,"level":50,"req":{"meta":{"requestId":"743ddd30-1eeb-11e9-9926-f3e2610e554f","userId":null},"url":"/content/images/size/w640h360/2019/01/venveo-1.jpg","method":"GET","originalUrl":"/content/images/size/w640h360/2019/01/venveo-1.jpg","params":{},"headers":{"x-forwarded-for":"86.121.180.69","x-forwarded-proto":"https","x-real-ip":"86.121.180.69","host":"example.com","connection":"close","pragma":"no-cache","cache-control":"no-cache","accept":"text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8","upgrade-insecure-requests":"1","user-agent":"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/71.0.3578.98 Safari/537.36","accept-encoding":"gzip, deflate, br","accept-language":"en-US,en;q=0.9,ro-RO;q=0.8,ro;q=0.7"},"body":{},"query":{}},"res":{"_headers":{"x-powered-by":"Express","cache-control":"no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0","content-type":"text/html; charset=utf-8","etag":"W/\"26e4-OgSejCC5lFdVzUwkmw5Bo0abiL8\"","vary":"Accept-Encoding","content-encoding":"gzip"},"statusCode":500,"responseTime":"302ms"},"err":{"id":"743f63d0-1eeb-11e9-9926-f3e2610e554f","domain":"https://example.com/","code":"MODULE_NOT_FOUND","name":"InternalServerError","statusCode":500,"level":"critical","message":"Sharp wasn't installed","context":"empty","help":"empty","stack":"InternalServerError: Sharp wasn't installed\n at new InternalServerError (/home/example.com/versions/2.12.0/node_modules/ghost-ignition/lib/errors/index.js:71:23)\n at Object.args [as resizeImage] (/home/example.com/versions/2.12.0/core/server/lib/image/manipulator.js:49:31)\n at storageInstance.exists.then.then.then (/home/example.com/versions/2.12.0/core/server/web/shared/middlewares/image/handle-image-sizes.js:78:42)\n at tryCatcher (/home/example.com/versions/2.12.0/node_modules/bluebird/js/release/util.js:16:23)\n at Promise._settlePromiseFromHandler (/home/example.com/versions/2.12.0/node_modules/bluebird/js/release/promise.js:512:31)\n at Promise._settlePromise (/home/example.com/versions/2.12.0/node_modules/bluebird/js/release/promise.js:569:18)\n at Promise._settlePromise0 (/home/example.com/versions/2.12.0/node_modules/bluebird/js/release/promise.js:614:10)\n at Promise._settlePromises (/home/example.com/versions/2.12.0/node_modules/bluebird/js/release/promise.js:694:18)\n at _drainQueueStep (/home/example.com/versions/2.12.0/node_modules/bluebird/js/release/async.js:138:12)\n at _drainQueue (/home/example.com/versions/2.12.0/node_modules/bluebird/js/release/async.js:131:9)\n at Async._drainQueues (/home/example.com/versions/2.12.0/node_modules/bluebird/js/release/async.js:147:5)\n at Immediate.Async.drainQueues (/home/example.com/versions/2.12.0/node_modules/bluebird/js/release/async.js:17:14)\n at runCallback (timers.js:810:20)\n at tryOnImmediate (timers.js:768:5)\n at processImmediate [as _immediateCallback] (timers.js:745:5)\n\nError: Cannot find module 'sharp'\n at Function.Module._resolveFilename (module.js:548:15)\n at Function.Module._load (module.js:475:25)\n at Module.require (module.js:597:17)\n at require (internal/module.js:11:18)\n at Object.args [as resizeImage] (/home/example.com/versions/2.12.0/core/server/lib/image/manipulator.js:47:9)\n at storageInstance.exists.then.then.then (/home/example.com/versions/2.12.0/core/server/web/shared/middlewares/image/handle-image-sizes.js:78:42)\n at tryCatcher (/home/example.com/versions/2.12.0/node_modules/bluebird/js/release/util.js:16:23)\n at Promise._settlePromiseFromHandler (/home/example.com/versions/2.12.0/node_modules/bluebird/js/release/promise.js:512:31)\n at Promise._settlePromise (/home/example.com/versions/2.12.0/node_modules/bluebird/js/release/promise.js:569:18)\n at Promise._settlePromise0 (/home/example.com/versions/2.12.0/node_modules/bluebird/js/release/promise.js:614:10)\n at Promise._settlePromises (/home/example.com/versions/2.12.0/node_modules/bluebird/js/release/promise.js:694:18)\n at _drainQueueStep (/home/example.com/versions/2.12.0/node_modules/bluebird/js/release/async.js:138:12)\n at _drainQueue (/home/example.com/versions/2.12.0/node_modules/bluebird/js/release/async.js:131:9)\n at Async._drainQueues (/home/example.com/versions/2.12.0/node_modules/bluebird/js/release/async.js:147:5)\n at Immediate.Async.drainQueues (/home/example.com/versions/2.12.0/node_modules/bluebird/js/release/async.js:17:14)\n at runCallback (timers.js:810:20)","errorDetails":"empty"},"msg":"Sharp wasn't installed","time":"2019-01-23T08:47:03.090Z","v":0}

@kirrg001
Copy link
Contributor

"sharp wasn't installed"

Sharp is an optional dependency.

@allouis Could you please double check this report? If sharp was not installed, the image should just redirect to the original image? 🤔

@allouis
Copy link
Contributor

allouis commented Jan 23, 2019

Yeah it should, though it seems errors in ghost-ignition ignore the props passed to them, in favor of any error passed to them. So the error code was not correct.

@luciandavidescu
Copy link

luciandavidescu commented Jan 23, 2019

If I may, apparently there's an underlying issue with sharp as well - optional or not, it came by default with my 2.10 install (or maybe I just don't recall manually installing it, but it shouldn't matter either way), worked fine but was then dropped without warning on upgrade.

Later edit: Indeed, it's no longer among the node_modules and I couldn't install it with npm install sharp (throwing a whole bunch of errors) so what I did was to just copy the folder over from /2.10.1/node_modules/sharp to /2.12.0/node_modules/sharp and now it works fine

allouis added a commit to TryGhost/Ignition that referenced this issue Jan 28, 2019
refs TryGhost/Ghost#10415
refs TryGhost/Ghost#10181
refs TryGhost/Ghost#10421

When supplying a code to an ignition error, if you pass an error with a code, the error's code takes precedence rather than the explicit code you pass.
allouis added a commit that referenced this issue Jan 28, 2019
closes #10421
refs #10181

This bumbs the ghost-ignition dep, so that the code passed to errors
takes priority over any code the error is inheriting from.
daniellockyer pushed a commit that referenced this issue Jul 26, 2022
refs #10181 

* Added initial handleImageSizes middleware

* Implemented saveRaw method on local file storage

* Wired up handleImageSizes middleware

* Implemented delete for LocalFileStorage

* Removed delete method from theme Storage class

* Deleted sizes directory when theme is activated

* Ensured that smaller images are not enlarged

* Renamed sizes -> size

* Exited middleware as early as possible

* Called getStorage as late as possible

* Updated image sizes middleware to handle dimension paths

* Revert "Deleted sizes directory when theme is activated"

This reverts commit 9204dfcc73a6a79d597dbf23651817bcbfc59991.

* Revert "Removed delete method from theme Storage class"

This reverts commit b45fdb405a05faeaf4bd87e977c4ac64ff96b057.

* Revert "Implemented delete for LocalFileStorage"

This reverts commit a587cd6bae45b68a293b2d5cfd9b7705a29e7bfa.

* Fixed typo

Co-Authored-By: allouis <[email protected]>

* Redirected to original image if no image_sizes config

* Refactored redirection because rule of three

* Updated comments

* Added rubbish tests

* Added @todo comment for handleImageSizes tests

* Added safeResizeImage method to image manipulator

* Used image manipulator lib in image_size middleware
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
server / core Issues relating to the server or core of Ghost
Projects
None yet
Development

No branches or pull requests

8 participants