Jpeg quality 95 by default with rendering with PIL #1771

Merged
merged 10 commits into from Apr 23, 2013

Conversation

Projects
None yet
4 participants
Contributor

dhyams commented Feb 21, 2013

This is just my opinion, but when rendering a plot to JPEG, the default quality should be a bit higher. Plots are usually (admittedly not always) full of starkly defined edges, that don't look very good once the jpeg format gets finished artifacting around all of them.

While it's true that a user shouldn't write a jpeg of something that doesn't consist of smoothly varying colors, if they do, let's try to make it look as good as is reasonable.

I've attached two images, one with the 75 quality and one with 95. The 75 quality one was 18K in size, and the 95 quality was 30K in size.

after
out

Daniel Hyams #1710
When a font doesn't have a glyph name, we should synthesize.  This fixes the
Arial font problems under Windows 8.
05a9d9e
Owner

mdboom commented Feb 21, 2013

👍 from me. I don't think disk space or bandwidth are at such a premium that this doesn't make sense as a default.

Member

pelson commented Feb 22, 2013

Why not have a .matplotlibrc parameter for this, that way users can set it to whatever they like? I'd be happy enough for 95 to be the default.

Owner

mdboom commented Feb 22, 2013

@pelson: Good suggestion.

Contributor

dhyams commented Feb 23, 2013

On the above commit; I'm not sure what the best practices are when dealing with rcParams (as far as whether to assume the entry is always there, and provide an initialization for it (savefig.jpeg_quality), or to use rcParams.get() as I have in the commit.

Also should the parameter be named savefig.pil_jpeg_quality or something like that, to indicate that it's only used for PIL?

Owner

mdboom commented Feb 25, 2013

The rcParam should be defined in rcsetup.py and described in matplotlibrc.template -- then you don't need to worry about it being defined or not -- it will always have a default value even if the user doesn't specify it.

As for the name -- it would be nice to fix up all of the backends that write jpegs to support this (it looks doable, just a matter of working through all the various APIs), so I'd be in favor of calling it savefig.jpeg_quality and then creating issues for all of the backends with JPEG support to start supporting this parameter.

Contributor

dhyams commented Mar 6, 2013

On the above commits:

  1. I was unable to test gdk and gtk on my current setup; so that's still pending.
  2. quality parameter not supported in the mac backend. I think it shouldn't be too hard, but I'm unable to test it at the present time, and it takes some mucking around with some objective C code to make it work.
  3. otherwise, works fine on the wx and wxagg backends (which are really the same, in this branch anyway; see #1770 for more info on this)

@pelson pelson and 1 other commented on an outdated diff Mar 30, 2013

lib/matplotlib/backends/backend_gtk.py
@@ -469,9 +469,18 @@ def _print_image(self, filename, format):
pixbuf.get_from_drawable(pixmap, pixmap.get_colormap(),
0, 0, 0, 0, width, height)
+ # set the default quality, if we are writing a JPEG.
+ # http://www.pygtk.org/docs/pygtk/class-gdkpixbuf.html#method-gdkpixbuf--save
+ options = cbook.restrict_dict(kwargs, ['quality'])
+ if format in ['jpg','jpeg']:
+ if 'quality' not in options:
+ options['quality'] = rcParams['savefig.jpeg_quality']
+ options['quality'] = str(options['quality'])
+
@pelson

pelson Mar 30, 2013

Member

PEP8: Please use just one newline between blocks of code.

@dhyams

dhyams Apr 18, 2013

Contributor

do you mean between lines 477 and 478?

@pelson pelson commented on the diff Mar 30, 2013

lib/matplotlib/backends/backend_wx.py
@@ -1162,15 +1162,29 @@ def _print_image(self, filename, filetype, *args, **kwargs):
gc = renderer.new_gc()
self.figure.draw(renderer)
+
+ # image is the object that we call SaveFile on.
+ image = self.bitmap
+
+ # set the JPEG quality appropriately. Unfortunately, it is only possible
+ # to set the quality on a wx.Image object. So if we are saving a JPEG,
+ # convert the wx.Bitmap to a wx.Image, and set the quality.
+ if filetype == wx.BITMAP_TYPE_JPEG:
+ jpeg_quality = kwargs.get('quality',rcParams['savefig.jpeg_quality'])
+ image = self.bitmap.ConvertToImage()
+ image.SetOption(wx.IMAGE_OPTION_QUALITY,str(jpeg_quality))
+
@pelson

pelson Mar 30, 2013

Member

Please remove newline.

@dhyams

dhyams Apr 18, 2013

Contributor

Do you mean line 1168?

Member

pelson commented Mar 30, 2013

@dhyams - looks like you've done most of the hard work. The PR wont merge automatically, so would you mind re-basing. I'd also be interested to hear what pieces of work you think are critical to be done before this PR should be merged.

Thanks,

Member

pelson commented Apr 18, 2013

Bump. I'm keen to get this into v1.3 (~ a months time)

Contributor

dhyams commented Apr 18, 2013

Sorry; I have been a little bogged down lately and let this one slip. I'll make the requested changes as soon as I can, and rebase.

Contributor

dhyams commented Apr 18, 2013

Re: pelson on what else needs to be done before this PR is merged: maybe can someone test on gtk and gdk? My current setup precludes the use of these two backends.

Contributor

dhyams commented Apr 19, 2013

git clone git@github.com:dhyams/matplotlib.git
git remote add original git://github.com/matplotlib/matplotlib.git
git checkout jpeg_quality_95_by_default
git rebase origin/master
conflict happens, so I edit to resolve the conflict
git add matplotlibrc.template
git rebase --continue

At this point, everything looks OK.

git push

To git@github.com:dhyams/matplotlib.git
! [rejected] jpeg_quality_95_by_default -> jpeg_quality_95_by_default (non-fast-forward)
error: failed to push some refs to 'git@github.com:dhyams/matplotlib.git'
To prevent you from losing history, non-fast-forward updates were rejected
Merge the remote changes (e.g. 'git pull') before pushing again. See the
'Note about fast-forwards' section of 'git push --help' for details.

Is it correct to do a

git push -f

Owner

mdboom commented Apr 19, 2013

I usually run the test suite again once the conflicts have been resolved to make sure, but after that, yes, git push -f should be fine.

Member

WeatherGod commented Apr 20, 2013

What probably happened is that you just did a straight out rebase of a
branch that used to be based on your personal copy of master, and is now on
the upstream copy of master. What I like to do is to keep my local
repository up to date by doing "git fetch --all" and updating the relevant
tracking branches (e.g., my copies of master and v1.2.x and so on) by
rebasing them with the upstream counterparts. I push those up to my github
repo.

Then, when it is time to rebase a feature branch, I rebase against my local
tracking branches, which i know to be fresh. Deal with conflicts, and then
push that up to my repo. Not foolproof, but it has kept my repos clean.

Cheers!
Ben Root

On Fri, Apr 19, 2013 at 6:50 PM, Michael Droettboom <
notifications@github.com> wrote:

I usually run the test suite again once the conflicts have been resolved
to make sure, but after that, yes, git push -f should be fine.


Reply to this email directly or view it on GitHubhttps://github.com/matplotlib/matplotlib/pull/1771#issuecomment-16693714
.

Contributor

dhyams commented Apr 22, 2013

I just pushed the rebasing on this one.

Contributor

dhyams commented Apr 23, 2013

Bump, so that the rebasing doesn't get out of date. Is this one OK to merge?

@mdboom mdboom added a commit that referenced this pull request Apr 23, 2013

@mdboom mdboom Merge pull request #1771 from dhyams/jpeg_quality_95_by_default
Jpeg quality 95 by default with rendering with PIL
9bb698a

@mdboom mdboom merged commit 9bb698a into matplotlib:master Apr 23, 2013

1 check passed

default The Travis build passed
Details
Member

pelson commented Apr 24, 2013

Thanks for merging @mdboom - I think the code changes were ready to go & @dhyams was right to encourage a merge before the PR went stale.

@dhyams: Given there is a change to the default JPEG quality, and there is a new rcParam, would you mind creating a new PR with the necessary changes to the doc/api/whats_new.rst (for the fact that the value has changed from 75 to 95) and users/whats_new.rst (for the fact that there is a new rcParam for JPEG quality).

While you're at it, this PR has introduced a couple of double newlines (one in lib/matplotlib/backends/backend_gtk.py and one in lib/matplotlib/backends/backend_wx.py). Whilst these are cosmetic, we are trying to standardise towards PEP8 (specifically http://www.python.org/dev/peps/pep-0008/#blank-lines); would you mind tidying these up? (this is the epitome of nitpicking, for which I apologise 😄)

Contributor

dhyams commented Apr 24, 2013

Ok this has been done here:

#1940

Member

pelson commented Apr 24, 2013

Brilliant. Thanks @dhyams!

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