GNOME Bugzilla – Bug 769976
[PATCH] Save as JPEG ignore quality setting
Last modified: 2016-11-08 15:23:54 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".
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),
> && (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)?
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.
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.
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.
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.
Someone can review my patch?
BUMP @Jehan or others for 2.10
Well, comment 5 makes me a bit wary... can someone describe how this changes the current behavior, and why?
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.
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?
Review of attachment 333631 [details] [review]:
Author: Mihail Zenkov <firstname.lastname@example.org>
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(-)
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.
> 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! :-)