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

Optimized the images so they look the same, but are less bytes in size #112

Closed
wants to merge 1 commit into from

Conversation

eperret
Copy link

@eperret eperret commented Jul 1, 2014

No description provided.

@eperret
Copy link
Author

eperret commented Jul 16, 2014

Typically how long does it take for a full request to be accepted or rejected? I see some of them open for a long time.

@Reinmar
Copy link
Member

Reinmar commented Jul 24, 2014

Hi,
Sorry for the late reply, but we do not always have time to respond quickly, especially now, during the holiday period. We take reviewing patches very seriously, so we never do it in 5 minutes. That's why some patches have to wait longer.

As for your pull request, the thing that worries me is that optimising images should be a step of the building process. We should not care about keeping files in the development repository optimised, because they come from many sources and it's hard to verify whether they are optimised or not.

Therefore, I think that it's not a correct pull request. It does not fix the issue (lack of optimisation step in the building process) but only hide its symptoms.

As for the CKBuilder, we planned to add images optimisation, but I can't find a ticket on http://dev.ckeditor.com, so I created ckeditor/ckbuilder#2.

@Reinmar Reinmar closed this Jul 24, 2014
@eperret
Copy link
Author

eperret commented Jul 24, 2014

Okay, it is your code, but I would completely disagree with your assertion that there is any problem with the build process. Images are rarely updated (most were updated a year ago) and the optimization takes a long time to perform, so it would be better to do it at checkin time than every time. What I have seen done is a unit style test that checks all of the images and fails the test if any of them are not optimal. The optimization I ran against this code base's images is set up to remove every last un-needed bytes and took 3 minutes to run.

How long should I expect ckeditor to continue using (and for my users to continue downloading the bloated) images before you put in your strategic fix?

@Reinmar
Copy link
Member

Reinmar commented Jul 25, 2014

First of all, it's not my code ;). The opinion was mine though and it was even challenged by other members of the CKEditor team. So I decided to do one test in order to understand if at least for some time this patch will improve the situation. I built a CKEditor package using the ./dev/builder/build.sh script on current master (67433b0) and your commit rebased on the current master. The results were, I have to say, surprising. The icons strip file (the only file that might really make any difference for users) was:

  • 17.7 KB - on master
  • 18.0 KB - after applying your patch

I have no idea why the first size is smaller, I was guessing that it will be equal to the second number. But I proved what I wanted to prove - builder reverts all optimisations because it takes all icons files and creates a new file. Clever optimisations are lost, because that's not a compression but some tricks within the PNG format. That's why optimising the dev version does not make sense.

Other image files are slightly smaller, but they are not loaded (except may the anchor image) with CKEditor, so that change is negligible and even mitigated by the bigger icons strip.

How long should I expect ckeditor to continue using (and for my users to continue downloading the bloated) images before you put in your strategic fix?

@wwalc: What do you think? We were planning to rewrite CKBuilder in Node.JS so to avoid using Java. But since we dropped this idea for now because of lack of time, can we speed up the output image files optimisation feature? And can we do that for the online CKBuilder, taking into account the fact that optimisation takes some time, but on the other hand that there will be far less images in the built package than in the whole dev repo?

@eperret: BTW. What tool have you used?

@wwalc
Copy link
Member

wwalc commented Jul 25, 2014

The optimization should be done by builder during the release process because we have no influence on icons used by 3rd party plugins included in custom builds, thus we cannot assume that optimized images are provided.

In fact whether the image is optimized at the beginning or not doesn't make any difference as internally images are read into the memory, translated into bitmap and then a new sprite image is created, using its own algorithm.

Using different optimization technique will not slow down the buld process significantly. In Java it's already too long, so it doesn't matter while the online builder will be done in a way so that it was as fast as before, when we find time to fix it...

@wimleers
Copy link
Contributor

You could add a soft dependency on http://www.advsys.net/ken/util/pngout.htm, http://pmt.sourceforge.net/pngcrush/, http://advancemame.sourceforge.net/doc-advpng.html and http://optipng.sourceforge.net/. Whenever any of those are available in the environment, you apply them to all images.

That way, you only have to:

  1. add a minor update to the build.sh script (check if those PNG optimization tools are present, and if so, run them on all images)
  2. update CKEditor's "build server" to have those tools
  3. update documentation to indicate it's preferable to have those tools available

(https://imageoptim.com/ uses all these PNG tools, and applies them in sequence, because they each have different optimizations — running all of them guarantees the smallest possible size.)

@Reinmar
Copy link
Member

Reinmar commented Aug 21, 2014

update CKEditor's "build server" to have those tools

Performance is a key here. We need to think about smart caching in case of custom builds, because otherwise we'll kill our servers. Or better - use a library which saves PNGs that are already optimised.

@Reinmar Reinmar mentioned this pull request Dec 14, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants