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

Projects
None yet
3 participants
@d4l3k
Contributor

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

This comment has been minimized.

Show comment
Hide comment
@bep

bep Nov 9, 2016

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@d4l3k

d4l3k Nov 11, 2016

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@bep

bep Nov 11, 2016

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@bep

bep Nov 11, 2016

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@d4l3k

d4l3k Nov 12, 2016

Contributor

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.

Contributor

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.

tpl: Adds imageConfig function which calls image.DecodeConfig and ret…
…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

This comment has been minimized.

Show comment
Hide comment
@d4l3k

d4l3k Nov 12, 2016

Contributor

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

Contributor

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 approved these changes Nov 16, 2016

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

4 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/wercker Wercker build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Nov 16, 2016

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@d4l3k

d4l3k Nov 16, 2016

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@bep

bep Nov 16, 2016

Member

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

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 d4l3k:imageconfig branch Nov 16, 2016

@d4l3k d4l3k referenced this pull request Nov 16, 2016

Closed

Add shortcode for amp-img #2700

@Jos512

This comment has been minimized.

Show comment
Hide comment
@Jos512

Jos512 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!

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment