Add blogPhoto shortcode #30
No reviewers
Labels
No Label
bug
easy pick
enhancement
fixneeded
help wanted
No Milestone
No project
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: patek/patek.cz#30
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "photo-shortcode"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This shortcode is intended to be used in blogposts to include photos. It downscales them and uses srcset to serve appropriate sizes to the browsers.
IMHO the original image should also be somehow included (maybe just as a link on click) as the FHD version still might be quite low quality, especially since the jpeg quality parameter is set to 50, which is quite low.
2d40f6a2c1- This is what I'm thinking of.To answer your questions @Greenscreener, we need to make a discussion first (all other patek.cz contributors, @sijisu, @tomkys144 and others (@patek-devs) feel free to join). I originally planned it with my upcoming post, but let's do it now.
This shortcode is mostly useful, when you add original (uncompressed) images and let Hugo handle it. They can be in size anywhere from hundreds of kilo up to 10MB. Are we willing to clutter our repo with such large files? I personally support it, but it is a decision that cannot be taken back without ugly history rewrites. What are your opinions?
Now, why am I talking about repo size and not addressing your @Greenscreener proposal? My answer depends on the conclusion above. If we plan to compress the images before committing them, your solution is just fine. If not, I would still be a dissatisfied user, if clicking an image would result in downloading an 8MB image (especially on mobile connection). We should then create a high quality, but still compressed, alternative and serve originals when explicitly requested only.
I mean sure, but q50 is quite heavy compression to serve as the highest quality. In my experience, 90 is way enough to decrease the file size quite significantly (in my experience up to 10x) without too much detail loss.
Also I noticed the images aren't explicitly converted to JPEGs, but the parameter "q50" is used which can be only used for JPEGs. I think it might error out if we decide to attach a PNG image.
I agree with this statement, but I am unsure how to deal with it. Are you suggesting to alter the quality of the miniatures?
Good point, I will try it
I think this is something we'd have to try out. Either we could do some unpredictable magic by trying to directly limit the file size by doing a binary search of quality to filesize, or settle on a "high quality" version, which would be presented to the user after a click, with a constant higher quality of like 90-ish. But I don't exactly know the file sizes that would be created at this amount of compression.
The docs state:
I've also tried it with a PNG image and it works just fine.
Nice
added 1 commit
cc5e75e4- Fix blogPhoto shortcodeCompare with previous version
added 1 commit
315ec5b7- Generalize blogPhoto shortcodeCompare with previous version
What? Does it then convert automatically to JPEG or is the parameter just ignored? I think this should be explicit.
Yeah, that sounds reasonable, but in my experience real world photos are 90-ish compressed anyway, so doing it again wouldn't result in much savings. In the end I propose you original solution (
2d40f6a2c1) and keeping the file sizes and user feedback on eye. Do you agree?And @sijisu and @Greenscreener, what are your opinions on the repo size? Can I commit full res photos in the future?
It is just ignored as it is not relevant for a PNG image
And we are sure that is what we want? Shouldn't we convert all images where this shortcode is used to JPEG?
It depends on what input formats we consider. I was thinking about JPEG and PNG only and in such a case I consider it a desired behavior as correctly used PNGs (those used for graphics) will almost certainly be at most of the size of JPEGs, but with much higher quality.
I still don't agree with you. Build this, and try to compare the images. q50 is WAAAY overkill IMHO. I think we shouldn't go below 80 and I wouldn't go below 90 and images which already have this much or more compression should stay that way. Just because the images are inside a webpage doesn't mean they have to look shit. Right now the uncompressed (by us) images have tens of KB, which is IMHO way less than would be requiring compression.
But this shortcode is blogPhoto, for photos. I agree that PNG is usually used for something else, but if a PNG is passed to this shortcode it should IMHO be considered a photo and converted to JPEG. This doesn't have to be the one shortcode to use for all and every image that will ever be shown on the website.
Also, the sizes of those images act very weird. Again, try to build this and compare how the image sizes change with different viewports. There is a lot of weirdness.
Also, there should be a way to view a large version of the image, I still think having a link to a large version of the image should be included, even if the large image shown to the user would be processed to not have 8MB.
This might be a proof there is no single value for all. Till now, I was testing this shortcode on my upcoming post and q50 was mostly indistinguishable from the original, especially given the size on the page. I just build the atthack page you suggested and while some photos look really ugly, the one of fablab experience is kinda OK while changing its size from about 700kB to about 90kB, which is a HUUUGE difference to just say "Nah, not worth my time". Broadband connection is still a too precious resource these days.
At the same time, complaining about compressing compressed images produces shit results is weird. If you compressed them manually (or obtained the already compressed), just do not use this shortcode for serving the images.
And I believe the general idea of this shortcode still holds. If you want to illustrate your words with images, they should look shit of course, but otherwise, they are just to help you to express your ideas. When you mention fablab experience, I just want to see "aha, it is truly a truck and it has a red color". And if I am were interested in what it looks like, I will click it since it is currently unusably small anyway.
And to this comment, it is what I propose here, isn't it?
This is definitely not a general purpose shortcode, especially since the
sizesattribute is based on the current state of blog<img>s. But I would consider forcibly converting one's PNG to JPEG as unexpected and unwanted behavior. Should we rename this shortcode?Or we can include the quality parameter as an argument to this shortcode with some default value.
Can you elaborate please? I am not aware of any weirdness.
Oh, right, sure, but the image that we show on click shouldn't have the quality under 90. To be honest I think we should settle on one quality value. The compression will be more visible on the smaller images, but, obviously, the smaller images are displayed smaller, which balances this effect out. Unless the images are small in the first place, the relatively low quality setting produces some very visible artifacts. Maybe this could be solved by checking if the largest produced image corresponds to the largest size (1920w) and if not, the compression should be decreased on this largest image.
I think for this shortcode, converting to JPEG might be wanted, since you still can include the PNG image without using this shortcode. The main focus of this shortcode is to decrease size, while sacrificing quality and at that point I think it is acceptable to convert to JPEG.
We still kinda miss each other. I changed my mind in the previous comment and propose showing the original image on click, since most images are already 90-ish compressed.
And to the quality, have you tried it on other images, apart from atthack? Especially some photos straight from your phone (usually 4K) or DSLR? On my 15.6" FullHD screen, the artifacts are not visible on most of the photos.
Should the image really be this tiny at this viewport size?
Shouldn't the image be spanning the entire width of the screen here?
Yes, if the original image is larger than 1080p it's fine, because you never get to the point, where 1920px is smaller than half of your viewport width, so there is always "enough pixels". If you use a small image, you will have to show the largest image you have in your srcset, even though the actual width of that image is smaller than the width at which it is presented (you don't have "enough pixels"). At that point the artifacts start becoming very obvious.
Also, I have no idea how it will look on a 4K screen, since these things are starting to become increasingly common. If you send me the article you've been using as your test, I can try to view it on a 4K computer monitor (or a 4K TV - it has much larger PPI) to see how it will look.
But the main way of decreasing PNG size is scaling it down, which this shortcode does as well.
I am thinking about the real world usage and that is, in case of PNG, including screenshots. Converting to JPEG is almost always not what you want, while serving low-res variants to low-res screen is.
I believe sooner or later there will be some big files (videos, downloads) that we will want to have hosted on the site and in this repository anyway. Then there will be no other way than to include them than to have them in this repository because of the way the deploying system works (if I understand correctly). A gitignored data folder is not an option too.
I say let's don't worry about the repository size at the moment. I also think we want the full-sized images included as well (readability of text etc.). As @vojta001 said, most of the photos are already 90p compressed, so the size won't be so terrifying. I suggest 50p compression for the thumbnails.
Right, sure, that's true, but it would be nice to have the option, could you add a bool parameter for JPEG conversion?
Sure, but I think that even the images included in the article should have some quality and IMHO aren't "thumbnails". I still haven't seen the article in which the q50 images look fine and I would very much like to and I believe you that if the images were originally ≥ 1080p they will look fine, but my point from here still stands. For images that are noticeably smaller than 1080p (and which would still be pretty legitimate to include) the compression turns otherwise small but still usable images into a blocky mess.
Also I bloody hope we don't include videos directly in the repository and the hosting, not only would it be expensive AF to pay for the stored and transferred data on firebase (not even mentioning cloning this repository), but we also can just upload it to YouTube. Large files can be shared via a file sharing service (dropbox, gdrive, mega, you name it).
added 1 commit
89111077- Include the compressed original image in src/srcset, if it happens to beCompare with previous version
I made another improvement for small images (
891110771e).Line 36 might be especially important for you:
It includes the quality you mention in
If we agree on a different value, it can be changed there.
Given the range of supported formats with webp on the roadmap don't we want a more general approach to force the output file format?
Also found some interesting weird behavior.
"123x123 q50""123x123 png""123x123 q50 jpg""123x123 q50 png"or any other non-jpeg formatAwesome! I did a test with q90 and even the largest fablab-experience image shrunk by almost half, but it wasn't used in all the viewports I managed to simulate. For the last image there (atthack-win) it helped quite a bit and now I can't distinguish any noticable differences from the original. The image this helped the most is ~80kB, which is a third of the minified main.css stylesheet, so I'd argue, going below q90 is not worth it.
Alright hmmm interesting.
Also, I'd rename the variable to something like
lessCompressedas naming the least compressed image out of the imagescompressedfeels a little weird.Might be, but given template variables are block scoped, it doesn't make much sense to me. It is named like this to contrast with line 32, where we actually resize something, not only compress.
Again, atthack is just not what I am for, all of the images there are already compressed and kinda small enough not to require much additional handling. Most of them might not even be worth resizing given their filesize. For real photos (not FB thumbnails), q50 is IMHO more then enough. Since you have been unable to provide yourself with any uncompressed (apart from what the camera itself does) photos, you can you the pictures attached. One is from a phone in bad lighting conditions, the other one from a DSLR. I hope GitLab won't compressed them by itself.
29c291a0801f8bd8d0685500b255fcb84585ee4dfd16417d3c4833915a3122bd)6e594244fa02dd950efb911e54073bcc86233bd0ffd5156d22dda1e715a5790c)And to end this q50 vs q90-ish flame, wouldn't a configurable quality, or even a switch to turn additional compression off, make it?
After a long research on this topic, I am not aware of any solution, that would:
max-height: 30rem;A fix that fulfills all but number 1. is to always target images to
100vw(by specifying it insizesattribute)I think you didn't understand me correctly. I agree with q50 for all images with q90 for the special non-resized images from
891110771e.mentioned in merge request !10
added 2 commits
e591d16e- Use <figure> and <figcaption> in blogPhoto shortcodee922f2f2- Merge in using <figure> elementCompare with previous version
Ping?
added 1 commit
5be48c59- Raise the quality of low res imagesCompare with previous version
Done in
5be48c59c6As no feedback/suggestion was provided, I will implement it using the method described above
approved this merge request
added 1 commit
058de799- Remove the sizes attributeCompare with previous version
058de79991Might be a very good idea to revisit in the future
Do we still want this feature? If so, I suggest moving it to a separate issue not to block this MR
Sure
created #12 to continue this discussion
resolved all threads
mentioned in issue #12
approved this merge request
mentioned in commit
d10218c320merged
mentioned in issue #13