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

Galleria: Load first image earlier #307

Merged
merged 1 commit into from Mar 18, 2018

Conversation

2 participants
@edwinsteele
Contributor

edwinsteele commented Mar 3, 2018

Galleria triggers loading of the main gallery iamge via javascript
which has two problems. It only triggers the load when javascript
has been processed and loads it at the same time as the second and
third image which means it's competing with background images for
bandwidth. We know that the first image will always be seen, so
trigger a load in the document rather than waiting for js to do it.

Comparison (this patch is the only difference between the two runs): https://webpagetest.org/video/compare.php?tests=180303_KS_87bf0854a5a346d90d71fad871ae423c,180302_9A_ff950fed21454ed20fd5e9750d3157ca

@codecov

This comment has been minimized.

codecov bot commented Mar 3, 2018

Codecov Report

Merging #307 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #307   +/-   ##
======================================
  Coverage    87.9%   87.9%           
======================================
  Files          20      20           
  Lines        1463    1463           
======================================
  Hits         1286    1286           
  Misses        177     177

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55399b2...d81476f. Read the comment docs.

Galleria: Load first image earlier
Galleria triggers loading of the main gallery iamge via javascript
which has two problems. It only triggers the load when javascript
has been processed and loads it at the same time as the second and
third image which means it's competing with background images for
bandwidth. We know that the first image will always be seen, so
trigger a load in the document rather than waiting for js to do it.

@edwinsteele edwinsteele force-pushed the edwinsteele:feature/faster-first-image-load branch from 52163ce to d81476f Mar 4, 2018

@saimn

This comment has been minimized.

Owner

saimn commented Mar 4, 2018

I agree it would be useful to optimize the page loading for albums with many images, but your solution sounds too hacky and I think we could get better results with galleria loading JSON data:
https://docs.galleria.io/article/18-quick-start
It should not be too difficult to modify the current template to produce the list of images in JSON format, and then I think galleria can also load the images lazily which would be great.

@edwinsteele

This comment has been minimized.

Contributor

edwinsteele commented Mar 5, 2018

Loading an asset in a hidden div, or a suppressed element is an established method for prioritised loading without javascript. It’s inelegant, to be sure, but until preload gets wider support, I don’t think there’s a nicer way.

I’ve done a quick prototype of json loading with lazy loading of thumbnails: edwinsteele@552db1e

From that I can see that json+lazyload does delay the thumbnail loading, so does reduce contention however the image loading only begins after js has been loaded and even then it triggers a download of the first 3 images in parallel.

I think the json work will give a win, and I think this will give us an improvement too. Is it possible to get this in? I’ll followup with the json work.

@saimn

This comment has been minimized.

Owner

saimn commented Mar 5, 2018

Yes, the json approach seems actually much cleaner, it would be great to get this if you are ready to finalize your patch (which seems already quite good).

@edwinsteele

This comment has been minimized.

Contributor

edwinsteele commented Mar 7, 2018

I’ve opened #312 for the switch to json. Are you happy to merge this too? I would prefer to use this solution for loading the first image earlier, but I can rebase with the more elegant but less supported preload tag if you feel strongly about the “hackiness"

@saimn

This comment has been minimized.

Owner

saimn commented Mar 10, 2018

preload seems to have poor support currently, sadly, so I guess we could integrate this for some time. Does it make a big difference even with the json method ?

@edwinsteele

This comment has been minimized.

Contributor

edwinsteele commented Mar 11, 2018

Yes, it still makes quite a difference. On my test I saw the json branch have the first image loaded after 1.3s compared to 0.7s with json + this patch. I would also like to move to preload once there’s more support. I’ll make a note to look at preload support in 6 months for review.

https://webpagetest.org/video/compare.php?tests=180311_D6_5fd54c4fc9e1b198f6c89000963fa9c9,180311_3H_8c3a4271a573410657849a86e517b7b4

@saimn

This comment has been minimized.

Owner

saimn commented Mar 18, 2018

Ok, let's go with this for now, thanks.

@saimn saimn merged commit bd9e547 into saimn:master Mar 18, 2018

3 checks passed

codecov/patch Coverage not affected when comparing 55399b2...d81476f
Details
codecov/project 87.9% remains the same compared to 55399b2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@saimn saimn added this to the 2.0 milestone Mar 19, 2018

@edwinsteele edwinsteele deleted the edwinsteele:feature/faster-first-image-load branch Oct 1, 2018

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