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

tpl: Adds imageConfig function which calls image.DecodeConfig and returns the height, width and color mode of the image. #2677

Merged
merged 1 commit into from Nov 16, 2016

Conversation

d4l3k
Copy link
Contributor

@d4l3k d4l3k commented Nov 8, 2016

This allows for more advanced image shortcodes and templates such as those required by AMP.

layouts/shortcodes/amp-img.html

{{ $src := .Get "src" }}
{{ $img := readFile (printf "/static/%s" $src) }}
{{ $config := imageConfig $img }}

<amp-img src="{{$src}}"
           height="{{$config.Height}}"
           width="{{$config.Width}}"
           layout="responsive">
</amp-img>
{{< amp-img src="/foo.png" >}}

@bep
Copy link
Member

bep commented Nov 9, 2016

It is a good idea, but we need to make some changes. Here is how I see it:

{{ $src := .Get "src" }}
{{ $config := imageConfig (printf "/static/%s" $src) }}

<amp-img src="{{$src}}"
           height="{{$config.Height}}"
           width="{{$config.Width}}"
           layout="responsive">
</amp-img>
  • The user should not need to bother with file reading, that is ineffective
  • As this will potentially be used with many images in the single template, it needs to be cached (with the path relative to the project root as key).
  • See the readFile for the path logic.

There are some challenges with the cache on reloads etc. that we will have to revisit later --we will probably reuse this in some way if we decide to do this on some scale.

@d4l3k
Copy link
Contributor Author

d4l3k commented Nov 11, 2016

I'm not sure if caching is really necessary. At least for my use cases, images will typically only be embedded once. I guess if the image was being embedded in a shared template/header it'll be used a lot.

As for the readFile thing, yeah, it probably would be better to just take a direct path. Just took another look at it, and it's probably way better to use (*afero.BasePathFs).Open() and then pass the returned file into image.DecodeConfig. From my understanding, only a tiny part of the image would then be read from disk since the headers are at the beginning of the file.

As an example, for PNG all the width/height/color type information is contained in the first 33 bytes of the image. https://en.wikipedia.org/wiki/Portable_Network_Graphics#Technical_details

If we're only reading a small section of the file, I'm not sure if there's really going to be any large benefit to caching those results. I can make the change and benchmark it if you'd like.

What are your thoughts on renaming this to be imageMeta or imageDetails instead? imageConfig is kind of ambiguous.

@bep
Copy link
Member

bep commented Nov 11, 2016

I was just pointing out what is needed to get this merged into Hugo's master. You are more than welcome to keep this in your personal fork, but then please close this PR.

This would be a good addition to Hugo, but not in its current state.

@bep
Copy link
Member

bep commented Nov 11, 2016

Just to add this this: Even if reading the first 33 bytes of a file is enough, if I use that one picture on 5000 pages, it would be pretty wasteful to open that same file 5000 times to read of that same 33 bytes.

@d4l3k
Copy link
Contributor Author

d4l3k commented Nov 12, 2016

I fully understand that you have the final say. It's no problem to add caching. It seemed to add a layer of complexity that will rarely be needed, but does have valid reasons. I just wanted to discuss it a bit before making the change.

…urns the height, width and color mode of the image.

This allows for more advanced image shortcodes and templates such as those required by AMP.

layouts/shortcodes/amp-img.html
```
{{ $src := .Get "src" }}
{{ $config := imageConfig (printf "/static/%s" $src) }}

<amp-img src="{{$src}}"
           height="{{$config.Height}}"
           width="{{$config.Width}}"
           layout="responsive">
</amp-img>
```
@d4l3k
Copy link
Contributor Author

d4l3k commented Nov 12, 2016

I've made the changes requested. There's now an image.Config cache and imageConfig directly takes the path.

@bep bep merged commit a49f838 into gohugoio:master Nov 16, 2016
@bep
Copy link
Member

bep commented Nov 16, 2016

Merged, and thanks, this is very useful -- now we can create some proper AMP templates!

Solid code, but please note I changed your commit message ... we are strangely pedantic about that. It is in the contribution guidelines.

@d4l3k
Copy link
Contributor Author

d4l3k commented Nov 16, 2016

Awesome! Any interest in having something like the amp-img shortcode built in?

I briefly glanced over that section earlier. Guess I didn't look closely enough.

@bep
Copy link
Member

bep commented Nov 16, 2016

amp-img shortcode would be good. If you could create a tracking issue for it (don't need any text other than the headline) so the implementation could close it, it makes it a little easier to write release notes etc.

Also see this discussion https://discuss.gohugo.io/t/custom-output-content-types/4584/1

@d4l3k d4l3k deleted the imageconfig branch November 16, 2016 20:34
@d4l3k d4l3k mentioned this pull request Nov 16, 2016
@Jos512
Copy link

Jos512 commented Mar 6, 2017

I know pull requests are typical for technical discussion, but just want to say a quick thanks to @d4l3k and @bep for your work on the imageConfig function. This handy function saves me a lot of time when I add new content to my Hugo site. Thank you!

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants