Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 4663693082099712: Issue 1897 - Compress auto-generated images with pngout (Closed)

Created:
Jan. 28, 2015, 9:40 p.m. by Sebastian Noack
Modified:
Feb. 10, 2015, 8:49 p.m.
Reviewers:
kzar, Wladimir Palant
Visibility:
Public.

Description

Issue 1897 - Compress auto-generated images with pngout

Patch Set 1 #

Total comments: 2

Patch Set 2 : Fixed grammar in comment #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+89 lines, -5 lines) Patch
A imageCompression.py View 1 1 chunk +82 lines, -0 lines 3 comments Download
M imageConversion.py View 3 chunks +7 lines, -5 lines 0 comments Download

Messages

Total messages: 6
Sebastian Noack
Jan. 28, 2015, 9:44 p.m. (2015-01-28 21:44:43 UTC) #1
kzar
Otherwise LGTM http://codereview.adblockplus.org/4663693082099712/diff/5629499534213120/imageCompression.py File imageCompression.py (right): http://codereview.adblockplus.org/4663693082099712/diff/5629499534213120/imageCompression.py#newcode62 imageCompression.py:62: file.name = filename # Set the 'name' ...
Feb. 10, 2015, 9:29 a.m. (2015-02-10 09:29:54 UTC) #2
Sebastian Noack
http://codereview.adblockplus.org/4663693082099712/diff/5629499534213120/imageCompression.py File imageCompression.py (right): http://codereview.adblockplus.org/4663693082099712/diff/5629499534213120/imageCompression.py#newcode62 imageCompression.py:62: file.name = filename # Set the 'name' attribute, that ...
Feb. 10, 2015, 9:57 a.m. (2015-02-10 09:57:38 UTC) #3
Wladimir Palant
http://codereview.adblockplus.org/4663693082099712/diff/5178081291534336/imageCompression.py File imageCompression.py (right): http://codereview.adblockplus.org/4663693082099712/diff/5178081291534336/imageCompression.py#newcode41 imageCompression.py:41: self._thread.start() Why not use subprocess.communicate()? The files aren't large ...
Feb. 10, 2015, 6:47 p.m. (2015-02-10 18:47:59 UTC) #4
Sebastian Noack
http://codereview.adblockplus.org/4663693082099712/diff/5178081291534336/imageCompression.py File imageCompression.py (right): http://codereview.adblockplus.org/4663693082099712/diff/5178081291534336/imageCompression.py#newcode41 imageCompression.py:41: self._thread.start() On 2015/02/10 18:47:59, Wladimir Palant wrote: > Why ...
Feb. 10, 2015, 7:23 p.m. (2015-02-10 19:23:56 UTC) #5
Wladimir Palant
Feb. 10, 2015, 8:49 p.m. (2015-02-10 20:49:18 UTC) #6
http://codereview.adblockplus.org/4663693082099712/diff/5178081291534336/imag...
File imageCompression.py (right):

http://codereview.adblockplus.org/4663693082099712/diff/5178081291534336/imag...
imageCompression.py:41: self._thread.start()
On 2015/02/10 19:23:56, Sebastian Noack wrote:
> So you mean I should save the image into a StringIO, get its value to pass it
to
> communicate()? Sounds kinda like a hack, and not really simpler.

Seems a lot simpler than running a thread IMHO, especially if image_to_file
returns a string rather than a file object (the caller will call read() on it
anyway). Sure, the function needs to be renamed then. But converting the image
to string will always be the first step - whether with compression or without.

Powered by Google App Engine
This is Rietveld