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).

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.

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.

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

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.

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.

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.

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.

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.

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.

(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.

(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.

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.

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.

Fixed merge conflict with 1.8.1.21pre

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.

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