-
Notifications
You must be signed in to change notification settings - Fork 270
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
add support for mozJpeg compression #495
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey great job so far!
See my comment that should help you get this running locally to test it out too.
Let me know if anything else comes up :)
Dockerfile.CompressImages
Outdated
@@ -16,6 +16,7 @@ RUN apt-get update && apt-get install -y --no-install-recommends --no-install-su | |||
RUN curl -sL https://deb.nodesource.com/setup_10.x | bash - | |||
RUN apt-get install -y nodejs | |||
RUN npm install -g svgo@^1.3.0 | |||
RUN npm install -g mozjpeg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think instead of using the npm wrapper, we should get the source directly from the github release versions and compile it ourselves right here.
It may help to run the container locally so you can play around with it.
To get the container running locally you can do the following.
$ docker pull dabutvin/imgbot-compressimages
$ docker run -it dabutvin/imgbot-compressimages /bin/bash
This will give you a shell where you can test out install commands to get this working.
Notice how you can run svgo
command within this prompt because it has that installed.
Here are some steps to guide you for the installation
- Add some new dependencies to apt-get install next to
curl
andlibcurl3
above to do thisautoconf automake libtool nasm make wget
- download the source to a tempdir
cd /tmp && wget https://github.com/mozilla/mozjpeg/archive/v3.3.1.tar.gz
- unpack the source code
cd /tmp && tar -xzf v3.3.1.tar.gz
- create the configure file for the source code
cd /tmp/mozjpeg-3.3.1 && autoreconf -fiv
- make the build dir
mkdir /tmp/mozjpeg-3.3.1/build
- config and install
cd /tmp/mozjpeg-3.3.1/build && sh ../configure && make install
This will default install into /opt/mozjpeg/bin/
and you will see them listed there
$ ls /opt/mozjpeg/bin/
cjpeg djpeg jpegtran rdjpgcom tjbench wrjpgcom
lets link them so we can reference them as you named them above
RUN ln -s /opt/mozjpeg/bin/jpegtran /usr/local/bin/mozjpegtran
RUN ln -s /opt/mozjpeg/bin/cjpeg /usr/local/bin/mozcjpeg
from that point you should be able to execute the commands mozjpegtran
and mozcjpeg
in the shell
for example
$ mozjpeg -h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome feedback, and great resource for getting started with docker.
Big thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dabutvin Updated docker file with new commands, will do some local testing with parameters! 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ElSiipo I just tried out your installation setup in the docker file and it works great! Good job.
I went ahead and started some testing and the setup you have for lossless compression with mozjpegtran
is working well, seeing some good results in my tests!!
I did run into a problem with the lossy setup I wanted to flag to you. I am getting an error Empty input file
and not getting any compression results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dabutvin Hmm.. Probably because of overwriting the file.
Will do a test to create a temp file, and then rename it or something. 👍
Question:
From my understanding the code needs to keep the filenames, otherwise it would not change the existing images, but rather add new ones to the GitHub-repo, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's right. The optimized image should overwrite the original image and then the diff is displayed in the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dabutvin Sorry for this taking a bit of time.
Updated the code, it now outputs to temp file when running Lossy
, then removing original and lastly renaming temp to original name.
Haven't figured out why I can't make calls to my Docker container, so not fully tested..
How my build looks:
docker build -t elsiipoimgbot -f "Dockerfile.CompressImages" .
This is my example command for running it:
docker run -p 5000:5000 -it elsiipoimgbot /bin/bash
So, for example 172.17.0.2:5000
isn't reaching anything.
Am I missing something obvious?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no worries!
I can try and test your updates when I get a chance to.
When you execute the docker run
command, do you get a shell into the container?
If you do you should be able to emulate by running mozcjpeg -h
right there in the shell.
To run the C# code you have written, you will have to use a queue to execute.
There is some info for getting started doing this in the docs, https://imgbot.net/docs/#contributing if you want to go that route
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dabutvin I decided to spin up my good old Xubuntu dist, and build MozJpeg, and then my C# code. I created a super simple test, locally, to try out MozJpegCompress.LossyCompress()
. Creating temp name, compressing, removing original and then renaming temp file seems to do the trick.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!!!! Great job.
I'll merge this now and then we can do a release sometime this week. I'll post here when we do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 👍 👍 excellent work
MozJPEG compression is live in production now and running on everyone's images! 🥇 |
Added
LosslessCompress
to run afterLossyCompress
.I'm a little bit unsure about the
FileName
andArguments
part, since I wasn't able to get MozJpeg running locally..As for the quality argument, I think 75 is default, so perhaps a little better quality is preferred? (I put 80 there)
Might be better if we retrieve it from some configuration.