After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 769976 - [PATCH] Save as JPEG ignore quality setting
[PATCH] Save as JPEG ignore quality setting
Product: GIMP
Classification: Other
Component: Plugins
git master
Other All
: Normal normal
: 2.10
Assigned To: GIMP Bugs
Depends on:
Reported: 2016-08-16 12:12 UTC by Mihail Zenkov
Modified: 2016-11-08 15:23 UTC
See Also:
GNOME target: ---
GNOME version: ---

gimp-jpeg.patch (832 bytes, patch)
2016-08-16 12:12 UTC, Mihail Zenkov
none Details | Review
gimp-jpeg_v2.patch (2.34 KB, patch)
2016-08-19 14:52 UTC, Mihail Zenkov
committed Details | Review
gimp_subsampling_test.c (1.28 KB, text/plain)
2016-11-07 18:02 UTC, Mihail Zenkov

Description Mihail Zenkov 2016-08-16 12:12:16 UTC
Created attachment 333406 [details] [review]

If we open jpeg with quality higher than default we set "jsvals.use_orig_quality = TRUE".

But it not correct: we should set "jsvals.use_orig_quality = TRUE" only when both jsvals.subsmp and jsvals.use_orig_quality have same values as original.

Without attached patch GIMP ignore quality setting if "orig_quality > jsvals.quality" and "orig_subsmp != jsvals.subsmp".
Comment 1 Jehan 2016-08-17 21:50:31 UTC
Wow this use_orig_quality code is terrible.
I guess your code must be good when I see:

>   gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (toggle),
>                                 jsvals.use_orig_quality
>                                 && (orig_quality > 0)
>                                 && (orig_subsmp == jsvals.subsmp)
>                                );

So I assume this was indeed supposed to be TRUE when both values are set some way. This said, I would have imagined that it should have been a OR rather than a AND. Like if any of these values is set some way, we use original quality settings.

Is there anyone who could tell what was the original intent for this code (and what should be the right intent)?
Comment 2 Mihail Zenkov 2016-08-18 09:30:47 UTC
We also have this code:

> static void
> use_orig_qual_changed2 (GtkWidget *toggle,
>                        GtkWidget *combo)
> {
>  /* the test is (orig_quality > 0), not (orig_subsmp > 0) - this is normal */
>  if (jsvals.use_orig_quality && orig_quality > 0)
>    {
>      gimp_int_combo_box_set_active (GIMP_INT_COMBO_BOX (combo), orig_subsmp);
>    }

So if we have "jsvals.use_orig_quality == 1" we also change subsampling to original.
Comment 3 Jehan 2016-08-18 15:56:50 UTC
I guess you must be right. I just hope someone else who actually worked on the jpeg plugin could confirm this is how it is supposed to work. Don't hesitate to add a reminder comment if there is no update within — say — 2 or 3 weeks. If nobody commented by then, I would then check again and take a decision myself.

P.S.: the code in this plugin is a real mess and would definitely deserve cleaning.
Comment 4 Michael Schumacher 2016-08-19 08:41:24 UTC
In bug 675981, the intended behavior is described to use the original values for these settings if the original ones were better.

Please check if the proposed patch changes this intended behavior.
Comment 5 Mihail Zenkov 2016-08-19 14:52:41 UTC
Created attachment 333631 [details] [review]

I look at bug 675981 and add some additional changes to fully fix it.
I also change logic for choosing best subsamplig, as current logic not always work right.
Comment 6 Mihail Zenkov 2016-09-19 09:10:57 UTC
Someone can review my patch?
Comment 7 Ángel Guzmán Maeso (shakaran) 2016-11-07 04:49:46 UTC
BUMP @Jehan or others for 2.10
Comment 8 Michael Schumacher 2016-11-07 13:58:28 UTC
Well, comment 5 makes me a bit wary... can someone describe how this changes the current behavior, and why?
Comment 9 Mihail Zenkov 2016-11-07 18:02:41 UTC
Created attachment 339267 [details]

1.  If we do "load defaults" (bug 675981) we should check is we want use original quality or saved value. Current behavior - always restore saved values for quality and subsampling and ignore original.

2. Choosing best subsamplig for original.

Current behavior: we set saved setting and override it if original jpeg have better quality. But for many cases current algorithm chose wrong value.

For example - if we have:
orig_subsmp = 1 (JPEG_SUBSAMPLING_2x1_1x1_1x1)
jsvals.subsmp = 0 (JPEG_SUBSAMPLING_2x2_1x1_1x1)

with current algorithm we will have 0, but it incorrect as 2x2_1x1_1x1 worse than 2x1_1x1_1x1.

We can fix all wrong case if we set subsampling to original except two case:
1. if we already have best possible quality (jsvals.subsmp == JPEG_SUBSAMPLING_1x1_1x1_1x1)
2. or if original have worst possible quality (orig_subsmp == JPEG_SUBSAMPLING_2x2_1x1_1x1)

Attached test program show old and new value for all possible cases.
Comment 10 Jehan 2016-11-08 15:13:57 UTC
Ok had a closer look and it makes sense.

You are right, the current subsampling selection was completely broken. It was choosing the original subsampling over the defaults one only if the original is the best (that part is OK) or if the original is not the worst AND the defaults is the best. This second test is broken as your example shows. It should even be the opposite: if the defaults is not the best, why would we absolutely stick to it over the original?
Comment 11 Jehan 2016-11-08 15:14:27 UTC
Review of attachment 333631 [details] [review]:

Pushed as:

commit 07eb13c73d8b881bf1e173bb40377400ab702b58
Author: Mihail Zenkov <>
Date:   Tue Nov 8 15:17:10 2016 +0100

    Bug 769976 - JPEG export ignores quality setting and subsampling fix.
    Set use_orig_quality when both the quality and the subsampling are the
    same as in the originally-imported jpeg.
    Also improve subsampling initial selection: use the original subsampling
    unless the default one is the best or the original one is the worst.
    The current code was wrong and would often use the default subsampling
    even when worse than the original one.

 plug-ins/file-jpeg/jpeg-save.c | 16 ++++++++++------
 plug-ins/file-jpeg/jpeg.c      | 14 ++++++++++----
 2 files changed, 20 insertions(+), 10 deletions(-)
Comment 12 Jehan 2016-11-08 15:22:02 UTC
P.S.: Mihail, by the way, for any further patch, could you send them git-formatted? Steps:

1/ Make a local commit on your working tree.
2/ Run:

> git format-patch origin/master

It will generate as many patch files as you have new commits (compared to origin/master). You can just upload these patch files.
This way, we can easily apply your patches, with your authorship all-ready and the commit message you prepared to explain the change.
Even if sometimes we will still edit the message, this is simpler for review and validation.

Thanks for the patch! :-)
Comment 13 Mihail Zenkov 2016-11-08 15:23:54 UTC
Ok ;)