User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.4) Gecko/20070531 BonEcho/2.0.0.4 (tete009 G7 SSE2) Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070506 SeaMonkey/1.5a When pasting an image from the system clipboard to a Composer window, Composer correctly creates a new image file on disk for this. However, it saves by default in JPEG and with very low quality compression (esp. for line graphic images like charts), causing bad visual artifacts and making small text in the image impossible to see. This is reproducible and happens every time, regardless of what kind of an image is in the clipboard to be copied. Workaround is to save each image manually to a file OUTSIDE Composer and then manually link each image from within composer (from the files created). However this is very tedious and time-consuming when editing existing web pages and copying multiple images from other applications. Reproducible: Always Steps to Reproduce: 1.Copy an image of your choice to clipboard, pref. of original quality (no previous compression), one with line graphics and some text in the image. A time series chart with a title and X & Y axis labels is a a good example 2. Paste the image from clipboard to a composer Edit window (Ctrl+V). Actual Results: Compare the original image (clipboard) vs the one now in the Composer window. See how badly the jpeg compression destroys the image contents in the Compoöser copy. Expected Results: 1. User selectable Image format (JPEG/JFIFF, PNG, GIF, etc) format and compression ratio (for JPEG and PNG at least) in the preferences. 2. Additionally by default, the compression ratio for jpeg (if jpeg format is default) should be lower, resulting in less image artifacts in the copy/pasted images.

This is a very annoying "feature". Makes image pasting useless. This same bug is present in Thunderbird too. I tried to find the code that is the culprit for this and the closest I came to was the imglib method EncodeClipboardImage(): http://www.google.com/codesearch?hl=en&q=+moz-screenshot+show:tiQ1OVDfAeQ:ivjRWCKDbq8:G7CqV--pS0s&sa=N&cd=1&ct=rc&cs_p=http://nvu.viapanda.com/1.0/source/nvu-1.0-sources.tar.bz2&cs_f=mozilla/modules/libpr0n/decoders/jpeg/nsJPEGEncoder.cpp#a0 But when searching the TB source tree I couldn't find the code for that method any more. Presumably it's linked against some external library which I can't seem to find.

Casper attached a patch in bug 386576 that fixed the code in the encoder. IMO, this needs to be fixed in the caller, though: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/widget/src/windows/nsImageClipboard.cpp&rev=1.9&mark=351,352#343

Here's the link to the caller code, I think: http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsImageClipboard.cpp#351 see the line that says "transparency=none", the libimg encoder also accepts a "quality=NN" setting, so the fix might be applied here instead ("quality=95" for example).

I think this could be an interesting change to take (casper's comment 4). How do we decide what tradeoff to make between quality and image size. This feature is most often used to paste images into e-mails for sending so we wouldn't necessarily want a very big image. At the same time, we want better quality than what we have today.

It's a good point. And I'm not even sure jpg is the right choice here for pasting images. Usually when you paste an image it's most probably a screen capture. Screen captures compress terribly with jpg. For once thing jpg was never made to compress images with lots of sharp edges and lines like screen captures have. So even if you compress at quality 99 it will always look fuzzy. Second point is that even at quality 90, which is considered "acceptable" for photographic-type images, but still totally inadequate for screen capture-type images..the file size is going to be in order around 3-5 times larger than the same screen capture image compressed using PNG. PNG again is a lossless compression method, but on screen capture type images the compression is far better than JPG, and the quality is as good as you can possibly get. For this reason I'm definitely of the opinion the system should be switched to PNG. A more advanced solution would be to allow you to choose PNG/JPG somehere in an options dialog. But for screen captures PNG wins hands down. It compresses better, and provides superior (lossless) image quality.

We looked at using the PNG encoder instead of the JPG encoder. I tried it out and it looked so much better! But I don't believe Outlook / Outlook Express can render PNGs. Maybe someone can prove me wrong.

I just tried with Outlook Express with just a plain text message and attached one JPG and one PNG. Both showed up fine straight in the mail view window on OE.

The hard-coded default JPEG quality seems to be 50, which is very low, in modules/libpr0n/encoders/jpeg/nsJPEGEncoder.cpp There is provided a method of passing a "quality=NN" option when writing a JPEG. Maybe that hard-coded default could be made a user preference, selectable via a variable in about:config. For PNG, the default "quality" seems to be left to the libpng default. Likewise, decisions about PNG filtering are left to libpng. There is no option to write an indexed-color PNG. An easy FIX for this bug would be to change the default JPEG quality "50" to "75" or "80". There would still be artifacts but not as obvious.

I've already tried to increase the quality. You can do it by searching for the unicode string "transparency=none" in the Thunderbird binary and replacing it with "quality=95" for example. The result are not encouraging. Even at quality 95, which increases the image size considerably, the image is still fuzzy, the colors smear into each other, and it just looks plain bad. I still think a switch to PNG is the way to go. Either there is something fishy with the JPG encoder, or you just can't encode screen captures properly with JPG no matter what you do. I haven't tried to encode one with Photoshop yet, which has one of the best JPG encoders I know..but with the thunderbird encoder you won't get anything decent out of it even at high quality settings.

In ImageMagick, when we use high quality we also use 1,1,1 sampling_factors (i.e., no donwnsampling). The libpr0n JPEG encoder doesn't provide a method for changing the sampling factors from the default 1,2,2, which leads to color-bleeding. I agree that PNG is probably a better way, in general, for capturing screen shots.

This patch increases the default JPEG encoding quality from 50 to 75. If the user requests a quality higher than 90, then all sampling factors are set to 1. The patch could be improved by making the "75" a user preference, and maybe by making PNG the default encoder.

Yes, this is a very annoying "feature" which makes image pasting useless. For every new version of Thunderbird the first thing I do is batching the binary... It should be changed to be user-defineable, at least in about:config. I would suggest to set the default jpeg quality to 80 or 90 or better use png.

Yes, pls make quick fix to this annoying image quality pasting issue since pasting image does always happen to writing emails!

I think the bug severity should be upped from minor. This is a major problem when using Thunderbird for e-mails in any work situation where you are talking with clients that are graphics-oriented (graphic design, for example) and need to send images back and forth. The quality is so bad it is an embarrasment to send pasted picture with Thunderbird. In addition to this the patch to increase the default quality and sampling frequency are straightforward to integrate. I don't understand why this patch has not been integrated yet. What gives!? Also I propose the default quality should be 95. We're not sending e-mails over 9600 bps modems any more. The different in jpg size between quality 75 and 95 is probably around 10%, but the quality improvement will be substantial. The part that improves the quality the most however is the patch for the sampling frequency if (quality >= 90) { int i; for (i=0; i < MAX_COMPONENTS; i++) { cinfo.comp_info[i].h_samp_factor=1; cinfo.comp_info[i].v_samp_factor=1; } } This is the reason the quality should be set > 90 in the final patch.

(In reply to comment #15) > Yes, pls make quick fix to this annoying image quality pasting issue since > pasting image does always happen to writing emails! > Comments like these don't really contribute much. If you support this bug, use the vote feature (top right of this page)

Well I didn't mean to make it pointless, but I was new to Bugzilla. Thanks for all of your tolerance to this reply too.

Just increasing the JPEG quality will likely improve the situation in most cases already, so this would be a good move regardless of any additional steps. Back to comment #13, the question remains whether to make this configurable and/or to make PNG available as an alternate encoding scheme. The latter is preferred for including graphics (rather than photographs) into the message, which would still be subject to lossy-compression artifacts even at a high quality setting. As for the concerns raised in comment #7, I think PNG should be supported by most HTML-capable e-mail clients by now, and one could always switch to JPEG then if not. Adding configuration preferences should happen in nsImageClipboard.cpp as the caller of the encoder. It is currently passing an empty optional quality argument when calling nsJPEGEncoder::InitFromData(), thus the default quality is used. There could be an integer preference "image.jpeg_encode_quality" defaulting to 92 now and used to form an argument; a second boolean preference "image.paste_as_png" switches between "image/jpeg" and "image/png" encoders. This should cover all cases from low-speed modem to high-quality graphics requiring lossless quality. However, it may not be a good idea to have preferences in routines at such rather basic levels... Also, it appears that this may be a problem on Windows only but not for other platforms. Note that windows/nsImageClipboard.cpp is specific for Windows, and this part was introduced by bug 223909 for the Windows version only. For other platforms, there are gtk2/nsClipboard.cpp suggesting multiple types available for pasting on Linux, and cocoa/nsClipboard.mm suggesting PNG being the clipboard default for Mac OSX. I can't locate comparable JPEG-conversions from the clipboard for those platforms in the code.

Glenn, this is a patch draft for making the Windows version configurable per comment #20. I didn't test this, but even if it doesn't work out of the box, it hopefully serves as a good start. You will have to add those preferences to either mailnews.js or editor.js, depending on where it fits better: pref("image.paste_as_png", true); pref("image.paste_jpeg_quality", 92); I renamed the quality pref as it affects only pasting from the clipboard, whereas the patch in attachment 320378 [details] [diff] [review] is applied as default to any JPEG encoding, as you correctly pointed out. Also, the image type could be an enumeration if other types are to be added. Feel free to modify and extent this if necessary for other platforms as well.

Rsx, There are two issues here. One is the selection of a good default JPEG quality and sampling treatment when writing JPEGs. My patch takes care of that, could be reviewed and checked in now, and is pretty much independent of the other issue. The second issue is what your patch addresses, namely making the image format and quality user selectable. As I mentioned over on mozillazine, I don't have any experience with setting up user preferences, so would need some help implementing your suggestion (you are way ahead of me already!). About the "image." prefix: I see in Fx3b5 about:config that we already have "gfx.", "image.", "images.", and "clipboard.*". Any of those look OK to me except for "images.".

> (In reply to comment #22) One is the selection of a good default JPEG > quality and sampling treatment when writing JPEGs. My patch takes care of > that, could be reviewed and checked in now, and is pretty much independent of > the other issue. I fully agree with this. Go ahead and put the changes to the JPEG encoder up for review, and then we can continue figuring out if and how a configuration would be feasible. On top of that, there are different reviewers (and possibly review requirements) for the ImageLib and the Windows-widget modules, thus it makes sense to have this covered by two separate patches. > I don't have any experience with setting up user preferences, so > would need some help implementing your suggestion Unfortunately, I'm currently not having a Windows build for testing and debugging that patch, and don't quite have the time to set this up... Did you plug in the patch to see what's happening? > About the "image." prefix: I see in Fx3b5 about:config that we > already have "gfx.", "image.", "images.", and "clipboard.*". Sure, your call. I've chosen "image.*" as it refers to the ImageLib, whereas "clipboard.paste_image_*" or similar may be more intuitive for the user.

While Glenn's patch for increasing the default quality for encoding JPEG images is up for review, I have looked further into the option to paste images in a different format. Testing on Linux, clipboard images are in general pasted in JPEG as well, even if other formats are available through the clipboard. The reason for this is that the HTML editor only accepts JPEG as image "flavor" but not the others (set in nsHTMLDataTransfer.cpp). Apparently, the PNG-to-JPEG translation is performed in GTK for Linux (and in a much better quality than the Windows version now). To allow a selection of formats, I've therefore made the respective portions in nsHTMLDataTransfer.cpp depend on "clipboard.paste_image_type" as well, and pasting in PNG works fine then. Most likely, the same changes will have to be applied for the Windows draft patch I've posted before, since the PNG encoding may not be accepted by the HTML editor then. The preferences are currently defined in mailnews.js, maybe composer.js would be a better pick, but this should be easy to switch. Also, as "clipboard.paste_image_quality" would not be applicable to all platforms, it may be present depending on the OS_* preprocessor variables. This editor patch is for all platforms. Based on Glenn's comments, I have changed the boolean preference to an integer enumeration, with 0=JPEG / 1=PNG / 2=GIF as values. However, GIF is not provided on the Linux/KDE clipboard, thus when switching to GIF nothing is pasted. That's probably the correct behavior, since the user has to switch it to that mode manually and should realize that as a reason. The other possibility would be to offer a fallback "flavor" on the clipboard, which is taken when the primary flavor is not available (then we probably get bug reports that images are pasted in PNG despite GIF being selected...). This also opens the question if PNG would be the right default setting (as currently in the patch), or if JPEG should remain the default then. Obviously, PNG would offer a lossless quality, thus my personal preference. However, if there is any platform which doesn't support this flavor through its clipboard mechanism, nothing would be pasted. Consequently, this needs to be verified if PNG is going to be the default.

This is a better patch to address the selectability issue. Rather than insisting on a specific MIME type to be used for the image, the preference specifies the order in which image flavors are pasted. Thus, if any of the JPEG, PNG, or GIF formats are provided on the clipboard, they will be included in the document, simplifying the changes to CanPaste() substantially and also addressing the issue of a specific flavor not provided on the clipboard. In addition, this would avoid having to flip the preference manually if a clipboard mechanism offers varying types depending on the application copied from. For clipboard mechanisms allowing to paste in multiple flavors, the pref in its default setting would choose PNG first, then fall back to JPEG if it is not available, then fall back to GIF if neither PNG nor JPEG are available. The pref allows to specify alternate JPEG/PNG/GIF or GIF/JPEG/PNG orders. Thus, in the default "1" setting, the Linux clipboard pastes in PNG, or JPEG if set to the other "0" (per selection) or "2" (here as fall-back to GIF) settings. Glenn, that's all I can contribute at this point. This patch should work for all clipboard mechanisms which offer multiple image flavors for pasting. Other platforms need additional patches in order to encode and provide the image in the first-preferred flavor to the HTML editor. I can set this patch up for review in parallel, as it solves part of the problem, or you can include it in a consolidated patch, whatever you prefer.

I think they are separate issues (default quality, selectability) and should be reviewed separately. Is the "partial patch" now obsolete?

Ok, I'll look up who is reviewing the editor component and will set it up for review then. The partial patch should still apply if you switch the preference names and make the boolean preference an integer one, then switch() according to "clipboard.paste_image_type" being one of 0=JPEG / 1=PNG / 2=GIF as the primary flavor. Thus, I'm not marking attachment 320403 [details] [diff] [review] obsolete until somebody comes up with a complete version for this addition, possibly based on this draft as template.

Comment on attachment 322377 [details] [diff] [review] Patch for HTML editor (v2) with order of image-type preference >+ case 0: >+ case 1: >+ case 2: Please add a // comment at the end of those lines to say which format is selected. >Index: editor/ui/composer.js Don't forget mail/app/profile/all-thunderbird.js for TB ? Providing the fact these changes are made, you have r=daniel@glazman.org Thanks a lot for doing this !

Daniel, thanks for the quick review! (In reply to comment #28) > >Index: editor/ui/composer.js > Don't forget mail/app/profile/all-thunderbird.js for TB ? I don't think that this is actually necessary: Thunderbird has composer.js installed in defaults/pref as well, thus it should be covered. This still needs superreview because it is Core code, correct?

Updated patch with comments added to switch() cases as requested. Double-checked that editor/ui/composer.js is used in the TB trunk build. Asking David for sr? to make sure that this is ok for mail/mailnews. r+ forwarded from attachment 322377 [details] [diff] [review].

Comment on attachment 322667 [details] [diff] [review] Patch for HTML editor (v3), comments added this is definitely worth a try.

Thanks, check-in of attachment 322667 [details] [diff] [review] for trunk then, please... (bug needs to stay open for 320378 and 320403 patches).

Revised comments

(In reply to comment #32) > Thanks, check-in of attachment 322667 [details] [diff] [review] for trunk Where by "trunk" you mean "push to mozilla-central, then I'll wait until 1.9.1 opens and try to get approval there so we can see it in Tbird before it switches to hg, or in case it doesn't," right? editor/libeditor/html/ seems to be built by Firefox.

Phil, I'm a bit confused myself where this patch is supposed to end up. The primary target from my perspective would be mail/news and composer, thus TB 3.0 and SM 2.0, which would probably imply 1.9.1 then if this is the decision for those two applications. I'm not sure how FF utilizes this, but I'll be happy to request 1.9.0-approval if this seems the better way to go.

Comment on attachment 322667 [details] [diff] [review] Patch for HTML editor (v3), comments added Based on the logic in the (otherwise unrelated) bug 431819 comment #134, requesting approval for 1.9.0.1 then to get this patch included for the current TB 3.0 and SM 2.0 nightlies. This was tested from the current CVS sources, thus should cover the 1.9.0 branch. The checkin-needed request is for mozilla-central to also have it in trunk/hg. I hope this covers it, following comment #35 here. If my thinking is still flawed, I'd appreciate some guidance how to do it right.

I encountered a variation of this problem while working on a Firefox extension that needs to access image data on the clipboard. See: Bug 444800 (cannot retrieve image data from clipboard in lossless format). Regarding the proposed attachment 320403 [details] [diff] [review], a better fix would be to give applications what they ask for rather than have the widget code on Windows consult preferences to determine how to behave. To put it another way, it may be appropriate for Thunderbird to consult preferences to decide which image format to retrieve from the clipboard, but the API should be as general as possible so that extensions can also get what they need.

(In reply to comment #38, attachment 320403 [details] [diff] [review]): In general, I would agree with this. I was drafting that patch under the assumption that only one version could be put on the clipboard on Windows, without having detailed knowledge of how it works on this platform. If this could be translated to a scheme where the Editor can pick the image flavor from the clipboard similar to Linux, this would certainly be preferable. Is the idea of your bug 444800 to address this in a broader scope?

(In reply to comment #39) > Is the idea of your bug 444800 to address this in a broader scope? Yes, I think so. I am not an expert on the Windows clipboard, but most applications that allow copying of images seem to place a bitmap (CF_DIB) on the clipboard (including Firefox). Therefore, for now we can focus on getting the data out in a lossless format (e.g., image/png and/or image/bmp).

The current code in widget/src/windows/nsClipboard.cpp is looking for a CF_DIB object, then using nsImageFromClipboard::GetEncodedImageStream() to convert it to JPEG, true. There are several occurrences of hard-wired kJPEGImageMime tests that probably would need to be cleaned up in addition to switching the flavor according to the preferences as drafted in attachment 320403 [details] [diff] [review]. > Therefore, for now we can focus on getting the data out in a lossless format I respectfully disagree with this statement, just flipping one hard-wired encoding to another doesn't solve the underlying core problem. The IMO best solution would be to provide either all possible flavors as alternatives for nsClipboard::GetDataFromDataObject() for the editor (or extension) to pick from, or to look up the order of flavors provided by the editor and convert directly from the CF_DIB data to the image flavor with the highest priority. > (e.g., image/png and/or image/bmp). Keep in mind that BMP does not provide any compression. So, it wouldn't be acceptable for mailing (and not recognized by the Editor patch here), but certainly may be an alternative for extensions that may find it useful.

Comment on attachment 320403 [details] [diff] [review] Partial patch for type and quality preferences on Windows (draft, moved to bug 444800) I'm marking my draft obsolete now as it doesn't address the full issue, this seems to be more complex for the Windows Widgets than anticipated. Glenn, I think that the Windows-specific part should be moved to bug 444800 then, thus keeping the bug here platform independent. Mark, I may have misunderstood comment #40; if this should have read "for now we can focus on getting the data out *also* in a lossless format" we are certainly in agreement.

Pushed in 15861:e88bda85bbcd. Glenn, can you move your patch to the other bug, as requested? One patch per bug, please.

Comment on attachment 322885 [details] [diff] [review] Increase jpeg quality to 92 and set sampling factors (v1) (moved to bug #444898) Moved patch to new bug #444898

Thanks Reed, and my apologies to Glenn, it would have been my job rather to move the format-switching issue to a new bug. Also, I was referring to the Windows Widgets patch in comment #42, improving the JPEG-encoder quality would have been platform-independent as well. Anyway, awaiting 1.9.0-branch approval then for attachment 322667 [details] [diff] [review] to finalize the editor part.

(In reply to comment #42) > Mark, I may have misunderstood comment #40; if this should have read "for now > we can focus on getting the data out *also* in a lossless format" we are > certainly in agreement. Yes, I meant also. Sorry for any confusion. PNG is what I would really like in the extension I am working on (lossless but compressed).

No problem, I see your point that the extension would need to tell the widgets which flavor(s) to provide rather than basing it on a global preference. The main source of confusion is probably that we tried to solve the three issues contributing to the problem in a single bug, which has now been resolved by moving the other two into their own bugs. Since the Editor component of the overall problem was solved here, I'm adjusting the flags respectively to get the QA right.

Comment on attachment 322667 [details] [diff] [review] Patch for HTML editor (v3), comments added Cancelling approval1.9.0.2 request. Now that TB 3.0 and SM 2.0 are built from comm-central, the most recent nightlies include this patch. Thus, the basis for requesting branch approval in comment #37 is no longer given.