393646 - Support reading image data off the clipboard

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, enhancement)

Description

[Colin Barrett [:cbarrett]]

Apps like Thunderbird would like it if we could get image data right off the clipboard for pasting.

There's currently a stubbed out section of nsClipboard.mm that has a comment saying that "we've never supported this on Mac, maybe someday."

We probably should implement this. I talked to Josh and he agrees.

Comment 1

[Scott MacGregor]

Colin, you might find it helpful to check out the last patch in Bug 223909. 

For windows, it shows how we get the clipboard data, convert it to RGB packed format and then pass it to the jpg encoder using something like:

+    rv = ConvertColorBitMap((unsigned char *) (pGlobal + header->bmiHeader.biSize), header, rgbData);
+    // if that succeeded, encode the bitmap as a JPG. Don't return early or we risk leaking rgbData
+    if (NS_SUCCEEDED(rv)) {
+      nsCOMPtr<imgIEncoder> encoder = do_CreateInstance("@mozilla.org/image/encoder;2?type=image/jpeg", &rv);
+      if (NS_SUCCEEDED(rv)){
+        rv = encoder->InitFromData(rgbData, 0, width, height, 3 * width /* RGB * # pixels in a row */, 
+                                   imgIEncoder::INPUT_FORMAT_RGB, NS_LITERAL_STRING("transparency=none"));
+        if (NS_SUCCEEDED(rv))
+          encoder->QueryInterface(NS_GET_IID(nsIInputStream), (void **) aInputStream);

Consumers in the code base know what to do with the jpeg stream.

Comment 2

[Colin Barrett [:cbarrett]]

Scott, does it need to be a JPEG? All of the image data coming off the clipboard is not lossily compressed (TIFF, PICT or PDF). Do consumers know what to do with a PNG stream as well?

Comment 3

[Colin Barrett [:cbarrett]]

(From reading bug 223909 it looks like no)

Comment 4

[Scott MacGregor]

Bug 384116 is investigating to see if outlook and outlook express can render PNGs.

Comment 5

[Colin Barrett [:cbarrett]]

Here's what I get when I try pasting in an image:

Hello, world! Signaling it's OK to paste
looking for clipboard data of type application/x-moz-nativehtml
looking for clipboard data of type text/html
looking for clipboard data of type application/x-moz-file
looking for clipboard data of type text/unicode

These are all debugging messages -- the first one is in HasDataMatchingFlavors. The rest are coming from GetNativeClipboardData. It doesn't seem like callers are looking for kJPEGImageMime. Is there a flag I need to turn on elsewhere as well?

Comment 6

[Scott MacGregor]

Colin, if you are far enough along, you'll want to relax this ifdef in nsHTMLDataTransfer.cpp

+#ifdef XP_WIN32
+      // image pasting from the clipboard is only implemented on Windows right now.
+      (*aTransferable)->AddDataFlavor(kJPEGImageMime);
+#endif

Comment 7

[Josh Aas]

blocking1.9-, this is wanted but not a blocker

Comment 8

[Colin Barrett [:cbarrett]]

One thing I noticed in the windows clipboard code is that it gives a meaningless argument string:

>         rv = encoder->InitFromData(rgbData, 0, width, height, 3 * width /* RGB * # pixels in a row */, 
>                                    imgIEncoder::INPUT_FORMAT_RGB, NS_LITERAL_STRING("transparency=none"));

http://mxr.mozilla.org/seamonkey/source/widget/src/windows/nsImageClipboard.cpp#235

For one thing, the correct option is "usetransparency", not just "transparency". And secondly, the JPEG decoder doesn't support that as an option. Should I make a new bug or just attach a patch here.

My patch is ready to go, will attach next.

Comment 9

[Colin Barrett [:cbarrett]]

Attachment: fix v1.0

This patch supports decoding:

images with the TIFF or PNG pboard type.
images that are RGBA or RGB. These are the two most common types of bitmap data.

We don't try to decode images we don't know how to, and I've added assertions to try and catch bitmap images we can't decode now, but could in the future.

I fixed the windows decoder as well, to not pass a bad format string to the JPEG decoder.

Scott, is there any reason to have the #ifdef in nsHTMLDataTransfer.cpp at all, btw? Is there something wrong with asking and not getting anything?

Comment 10

[Colin Barrett [:cbarrett]]

BTW I haven't had the chance to test this on a big endian machine. I have no reason to believe that the logic is wrong there (simple RGBA->ARGB). I'll try to test on my big endian PowerBook soon.

Comment 11

[Josh Aas]

Comment on attachment 278658 [details] [diff] [review]
fix v1.0

Big endian ARGB conversion in one line:

result = ((src << 24) | (src >> 8));

Comment 12

[Josh Aas]

Comment on attachment 278658 [details] [diff] [review]
fix v1.0

If I take a screenshot and paste part of that from Preview into a message, that works. But if I open the image "mozilla/widget/src/cocoa/cursors/zoomIn.tiff" in Preview and copy that I can't paste that into the message. Why not?

Comment 13

[Colin Barrett [:cbarrett]]

Attachment: fix v2.0

I rewrote this to use ImageIO. Works with everything I threw at it, and doesn't deal with raw bitmap data anymore (since the data on the clipboard is encoded, usually in TIFF format). MUCH less code and much cleaner too.

I also did some digging on NULLs and CoreFoundation. The rule, near as I can tell, is that NULLs propagate freely through CF/CG functions unless otherwise noted (as in ObjC), including functions ending in Release, *except* CFRelease. Very annoying. (Considering just writing a helper macro/inline to take care of that).

I fixed a couple of issues (just NULL and success checks) in the code 'Ili recently put in here as well.

Comment 14

[Josh Aas]

Comment on attachment 279028 [details] [diff] [review]
fix v2.0

-    if (flavorStr.EqualsLiteral(kPNGImageMime) || flavorStr.EqualsLiteral(kJPEGImageMime) ||
-        flavorStr.EqualsLiteral(kGIFImageMime)) {
-      // We have never supported this on Mac OS X, we could someday but nobody does this.
+    if (flavorStr.EqualsLiteral(kJPEGImageMime)) {

You simply removed png and gif here, please leave them in a commented-out "if" block so we don't forget about them. Or implement them, whichever.

+      CGImageDestinationAddImage(destRef, imageRef, NULL);

Please add a comment somewhere around here explaining the null propagation stuff. It looks terrible if you don't know about that (looks like you mistakenly didn't null check destRef).

-----------
#ifdef MOZILLA_1_8_BRANCH
#define imgIEncoder imgIEncoder_MOZILLA_1_8_BRANCH
#endif
-----------

In the Windows nsImageClipboard.cpp, you can remove the following lines at the top of the file. Just a little more cleanup.

-#ifdef XP_WIN32
-      // image pasting from the clipboard is only implemented on Windows right now.
+#if defined(XP_WIN32) || defined(XP_MACOSX)
+      // image pasting from the clipboard is only implemented on Windows & Mac right now.

Please just kill off that ifdef. It is OK to request that and just not get it. libeditor shouldn't have to know such details about widget implementations and the ifdef is one more thing to have to remember to maintain.

Comment 15

[Josh Aas]

"In the Windows nsImageClipboard.cpp, you can remove the following lines" - I mean remove the lines above that comment...

Comment 16

[Colin Barrett [:cbarrett]]

Attachment: fix v2.1

Address review comments. I ended up just making us potentially support PNG and GIF as well, since it was easy.

Comment 17

[Josh Aas]

Comment on attachment 279622 [details] [diff] [review]
fix v2.1

+    else if (flavorStr.EqualsLiteral(kJPEGImageMime) ||

There is an extra line above this line, after the above "if" block ends.

+      // Figure out what type we're converting to
+      CFStringRef outputType = flavorStr.EqualsLiteral(kJPEGImageMime) ? CFSTR("public.jpeg") :
+                               (flavorStr.EqualsLiteral(kGIFImageMime) ? CFSTR("public.png") :
+                               CFSTR("com.compuserve.gif"));

I don't really like this use of ?:. Please use if/else if/else. Harder to read than if.

+      // Use ImageIO to interpret the data on the clipboard and transcode to JPEG.

You're not necessarily converting to JPEG - please fix this comment and the name of the "jpegData" variable.

+      } else if (flavorStr.EqualsLiteral(kJPEGImageMime)) {

This is in HasDataMatchingFlavors. Shouldn't you be doing this for png and gif too?

Comment 18

[Colin Barrett [:cbarrett]]

Attachment: fix v2.2

Comment 19

[Josh Aas]

Comment on attachment 279675 [details] [diff] [review]
fix v2.2

You didn't address my first comment, please fix on checkin.

Requesting review from mscott for the editor change.

Comment 20

[Colin Barrett [:cbarrett]]

d'oh sorry. Will fix on checkin.

Comment 21

[David :Bienvenu]

Comment on attachment 279675 [details] [diff] [review]
fix v2.2

r=bienvenu for the htmleditor.cpp part...

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+      else if (flavorStr.EqualsLiteral(kGIFImageMime))
+        outputType = CFSTR("public.png");
+      else
+        outputType = CFSTR("com.compuserve.gif");

Is this correct? If so, why are we turning GIFs into PNGs and everything else into GIFs?

Comment 23

[Colin Barrett [:cbarrett]]

(In reply to comment #22)
> +      else if (flavorStr.EqualsLiteral(kGIFImageMime))
> +        outputType = CFSTR("public.png");
> +      else
> +        outputType = CFSTR("com.compuserve.gif");
> 
> Is this correct? If so, why are we turning GIFs into PNGs and everything else
> into GIFs?

The first part is a mistake, it should be 
> +      else if (flavorStr.EqualsLiteral(kPNGImageMime))
> +        outputType = CFSTR("public.png");

The second part is because the only mime types we handle in that block are JPEG, PNG, GIF:

+    else if (flavorStr.EqualsLiteral(kJPEGImageMime) ||
+             flavorStr.EqualsLiteral(kPNGImageMime) ||
+             flavorStr.EqualsLiteral(kGIFImageMime)) {


I'll post a new patch with the fix for the GIF/PNG mixup. I didn't catch it because right now, nothing handles anything other than JPEG streams :(

Comment 24

[Colin Barrett [:cbarrett]]

Attachment: fix v2.2.1

address roc's sr comments. I didn't change the libeditor portion, so his review should carry over (I think?)

Comment 25

[Mike Connor [:mconnor]]

Comment on attachment 280932 [details] [diff] [review]
fix v2.2.1

a=mconnor on behalf of drivers

Comment 26

[Colin Barrett [:cbarrett]]

Checking in editor/libeditor/html/nsHTMLDataTransfer.cpp;
/cvsroot/mozilla/editor/libeditor/html/nsHTMLDataTransfer.cpp,v  <--  nsHTMLDataTransfer.cpp
new revision: 1.129; previous revision: 1.128
done
Checking in widget/src/cocoa/nsClipboard.mm;
/cvsroot/mozilla/widget/src/cocoa/nsClipboard.mm,v  <--  nsClipboard.mm
new revision: 1.15; previous revision: 1.14
done
Checking in widget/src/windows/nsImageClipboard.cpp;
/cvsroot/mozilla/widget/src/windows/nsImageClipboard.cpp,v  <--  nsImageClipboard.cpp
new revision: 1.12; previous revision: 1.11
done