Closed Bug 444898 Opened 12 years ago Closed 11 years ago

JPEG writer uses poor quality default options for compression

Categories

(Core :: ImageLib, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: glennrp+bmo, Assigned: glennrp+bmo)

References

Details

(Keywords: fixed1.9.1)

Attachments

(6 files, 1 obsolete file)

This is a spinoff of bug #384116.  When the JPEG encoder writes an image, it uses a default "quality 50" and subsampling of the color channels, resulting in visibly poor quality due to compression artifacts and bleeding of colors.  Better default options should be used, namely quality 92 which is similar to digital camera quality, and 1x1 sampling (i.e., no subsampling of the color channels).

The severity of this problem on Linux platforms has been reduced by
bug #384116, but Windows platforms still use JPEG by default (see bug #444800).
Attachment #329211 - Flags: review?(tor)
Version: unspecified → Trunk
Attachment #329211 - Flags: review?(tor) → review+
Attachment #329211 - Flags: superreview?(pavlov)
Attachment #329211 - Flags: superreview?(pavlov) → superreview+
Glenn, as suggested in the forum discussions, I would recommend requesting 1.8.1.x branch approval for this patch. In this way, it may be available for
the next Thunderbird 2.0.0.17 and SeaMonkey 1.1.12 releases already.

I think this patch qualifies for the low-risk/high-gain category.
Attachment #329211 - Flags: approval1.8.1.17?
This needs to land on trunk (mozilla-central) before it'll get approval. We'll also probably want to take it on 1.9 after it's landed and has baked.
Whiteboard: [needs trunk baking]
Attachment #329211 - Flags: approval1.8.1.17?
Keywords: checkin-needed
This will change the default quality of canvas.toDataURL("image/jpeg"), right? Do we have a sense of how high the speed and size costs are?
Here's a quick test with ImageMagick, converting
a photo and a mostly-text screenshot from PPM to
JPG (quality 50 or 92) or PNG (compression level 7):

  time    size  file
 0.166s  158157 photo50.jpg
 0.288s  693670 photo92.jpg
 2.378s 2948081 photo.png
 0.355s  108361 screen50.jpg
 0.392s  241095 screen92.jpg
 0.673s  253834 screen.png
http://hg.mozilla.org/index.cgi/mozilla-central/rev/8d52648bfab8
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1a2
backed out:

http://hg.mozilla.org/index.cgi/mozilla-central/rev/cb6eede58eea

The reference images for the jpeg encoding tests in <http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/test/unit/> need to be updated.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla1.9.1a2 → ---
Is anyone working on replacing the test files?  I'm not equipped to do it myself.
There's code at the top of modules/libpr0n/test/unit/test_imgtools.js to dump out data into a file, just locally modify the test to call that at strategic points to save the generated test images (and then use those as the new reference images). 

[Also, reftest images can be updated by loading the data: URL logged when the reftest fails, and doing a Save As to put it into a file.]
I've updated the tests per comment #8, thanks to Justin for providing the instructions. I'm setting this up for review, this should be the same for
trunk and 1.9.0 branch on all platforms.
Attachment #343558 - Flags: review?(tor)
modules/libpr0n/test/unit/image1png16x16.jpg
modules/libpr0n/test/unit/image1png64x64.jpg
Comment on attachment 343558 [details] [diff] [review]
Updated unit test for trunk and 1.9.0 branch

Ping for review of the unit test updates, please.
Attachment #343558 - Flags: review?(tor) → review?(pavlov)
Second ping for review of the unit tests. Thanks to Reed for switching the review assignment, I'm certainly flexible if Stuart or Tim is reviewing this (whoever gets a chance first).

(In reply to comment #2)
> This needs to land on trunk (mozilla-central) before it'll get approval. We'll
> also probably want to take it on 1.9 after it's landed and has baked.

The meaning of "trunk" has become a bit ambiguous since the recent 1.9.1-branch split. The main patch was checked in initially for mozilla1.9.1a2 and was working properly there, except for the unit tests. Now, the freeze for 1.9.1b3 is upcoming next week. I assume that it would suffice to flag the patches for approval1.9.1 once the review is done, this should definitely land on 1.9.1 and any other branches after the necessary baking there.
Comment on attachment 343558 [details] [diff] [review]
Updated unit test for trunk and 1.9.0 branch

The updated test looks fine to me (I wrote the original version).

I suspect Stuart isn't too worried about the details of this test change, and he already sr+'d the code portion of this patch.
Attachment #343558 - Flags: review?(pavlov) → review+
Comment on attachment 329211 [details] [diff] [review]
Increase default JPEG encoding quality to 92 and do not subsample

Thanks, requesting approval1.9.1 per reasoning in comment #13 then.
Attachment #329211 - Flags: approval1.9.1?
Attachment #343558 - Flags: approval1.9.1?
Attachment #329211 - Flags: approval1.9.1?
Comment on attachment 329211 [details] [diff] [review]
Increase default JPEG encoding quality to 92 and do not subsample

Sorry, wrong order again. This needs to land on current trunk before approval1.9.1 can be requested... canceling.
Attachment #343558 - Flags: approval1.9.1?
Keywords: checkin-needed
(In reply to comment #6)
> backed out:
> http://hg.mozilla.org/index.cgi/mozilla-central/rev/cb6eede58eea

I've noticed that this changeset is not an exact mirror of 8d52648bfab8, thus

1. please backout changeset 17032:cb6eede58eea to reinstate original patch;
2. then checkin of attachment 343558 [details] [diff] [review] with 343559 and 343551.
Whiteboard: [needs trunk baking] → [needs trunk baking][c-n: comment #17]
(In reply to comment #17)
> 1. please backout changeset 17032:cb6eede58eea to reinstate original patch;
> 2. then checkin of attachment 343558 [details] [diff] [review] with 343559 and 343551.

Could you please attach an hg patch that does all that?
Single patch for push on mozilla-central (trunk) as requested.
This is now relative to the 17032:cb6eede58eea changeset.

Forwarding r+/sr+ from attachments 329211 and 343558.
Attachment #343558 - Attachment is obsolete: true
Attachment #357172 - Flags: superreview+
Attachment #357172 - Flags: review+
Whiteboard: [needs trunk baking][c-n: comment #17] → [needs trunk baking][c-n: comment #19]
http://hg.mozilla.org/mozilla-central/rev/48f39782a6f6
Status: REOPENED → RESOLVED
Closed: 12 years ago11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs trunk baking][c-n: comment #19] → [needs trunk baking]
Target Milestone: --- → mozilla1.9.2a1
Attachment #357172 - Flags: approval1.9.1?
Thanks Dão, test_imgtools.js shows a TEST-PASS result on Linux, OS X 10.5.2,
and WINNT 5.2 mozilla-central unit test tinderboxes now.

I also have a consolidated CVS patch for 1.9.0 ready, assuming per comment #2 that's still supposed to go into the other branches after landing on 1.9.1.
I'm not actually sure we'd take this on 1.8.1 or 1.9.0 at all. I'm not sure it's worth it, but if the change is really minor we might consider it. Requesting approval1.9.0.7 on an appropriate patch will get it considered though.
Ok, here it is. I wasn't able to include the binary image data though,
this doesn't seem to work with CVS. Thus, the patch would also need:

> Index: modules/libpr0n/test/unit/image1png16x16.jpg
> ===================================================================
> RCS file: /cvsroot/mozilla/modules/libpr0n/test/unit/image1png16x16.jpg,v
> retrieving revision 1.3

This is attachment 343559 [details].

> Index: modules/libpr0n/test/unit/image1png64x64.jpg
> ===================================================================
> RCS file: /cvsroot/mozilla/modules/libpr0n/test/unit/image1png64x64.jpg,v
> retrieving revision 1.1

This is attachment 343561 [details].

The resulting code is the same as for trunk and the 1.9.1 branch.
Attachment #357377 - Flags: approval1.9.0.7?
Comment on attachment 329211 [details] [diff] [review]
Increase default JPEG encoding quality to 92 and do not subsample


There are no unit test for the 1.8.1 branch, thus the original patch would apply.
Attachment #329211 - Flags: approval1.8.1.next?
Whiteboard: [needs trunk baking] → [needs trunk baking - 01/23]
Fixed merge conflict with 1.8.1.21pre
Attachment #329211 - Attachment is obsolete: true
Attachment #329211 - Flags: approval1.8.1.next?
Attachment #329211 - Attachment is obsolete: false
Attachment #357673 - Flags: approval1.8.1.next?
Comment on attachment 357377 [details] [diff] [review]
Consolidated patch for 1.9.0 branch (without images)

We're going to opt not to take this on the 1.9.0 (or 1.8.1) branches. It's not a critical issue that needs to be fixed and doesn't really meet our requirements. It should get fixed in 1.9.1 though.
Attachment #357377 - Flags: approval1.9.0.7? → approval1.9.0.7-
Attachment #357673 - Flags: approval1.8.1.next? → approval1.8.1.next-
Whiteboard: [needs trunk baking - 01/23]
Too bad, but thanks and understood.
Awaiting approval1.9.1+ on attachment 357172 [details] [diff] [review] then.
Comment on attachment 357172 [details] [diff] [review]
Consolidated patch for mozilla-central and mozilla-1.9.1

a191=beltzner
Attachment #357172 - Flags: approval1.9.1? → approval1.9.1+
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.