Closed Bug 393646 Opened 13 years ago Closed 13 years ago

Support reading image data off the clipboard

Categories

(Core :: Widget: Cocoa, enhancement)

x86
macOS
enhancement
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: cbarrett, Assigned: cbarrett)

Details

Attachments

(1 file, 4 obsolete files)

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.
Flags: blocking1.9?
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.
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?
(From reading bug 223909 it looks like no)
Bug 384116 is investigating to see if outlook and outlook express can render PNGs.

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?
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
blocking1.9-, this is wanted but not a blocker
Flags: blocking1.9? → blocking1.9-
Severity: normal → enhancement
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.
Attached patch fix v1.0 (obsolete) — Splinter Review
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?
Attachment #278658 - Flags: review?(joshmoz)
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 on attachment 278658 [details] [diff] [review]
fix v1.0

Big endian ARGB conversion in one line:

result = ((src << 24) | (src >> 8));
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?
Attached patch fix v2.0 (obsolete) — Splinter Review
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.
Attachment #278658 - Attachment is obsolete: true
Attachment #279028 - Flags: review?(joshmoz)
Attachment #278658 - Flags: review?(joshmoz)
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.
Attachment #279028 - Flags: review?(joshmoz) → review-
"In the Windows nsImageClipboard.cpp, you can remove the following lines" - I mean remove the lines above that comment...
Attached patch fix v2.1 (obsolete) — Splinter Review
Address review comments. I ended up just making us potentially support PNG and GIF as well, since it was easy.
Attachment #279028 - Attachment is obsolete: true
Attachment #279622 - Flags: review?(joshmoz)
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?
Attachment #279622 - Flags: review?(joshmoz) → review-
Attached patch fix v2.2 (obsolete) — Splinter Review
Attachment #279622 - Attachment is obsolete: true
Attachment #279675 - Flags: review?(joshmoz)
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.
Attachment #279675 - Flags: review?(mscott)
Attachment #279675 - Flags: review?(joshmoz)
Attachment #279675 - Flags: review+
d'oh sorry. Will fix on checkin.
Comment on attachment 279675 [details] [diff] [review]
fix v2.2

r=bienvenu for the htmleditor.cpp part...
Attachment #279675 - Flags: review?(mscott) → review+
Attachment #279675 - Flags: superreview?(roc)
Attachment #279675 - Flags: approval1.9?
+      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?
(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 :(
Attachment #279675 - Flags: superreview?(roc)
Attachment #279675 - Flags: approval1.9?
Attached patch fix v2.2.1Splinter Review
address roc's sr comments. I didn't change the libeditor portion, so his review should carry over (I think?)
Attachment #279675 - Attachment is obsolete: true
Attachment #280932 - Flags: review?(joshmoz)
Flags: in-litmus?
Attachment #280932 - Flags: superreview?(roc)
Attachment #280932 - Flags: review?(joshmoz)
Attachment #280932 - Flags: review+
Attachment #280932 - Flags: approval1.9?
Attachment #280932 - Flags: superreview?(roc) → superreview+
Comment on attachment 280932 [details] [diff] [review]
fix v2.2.1

a=mconnor on behalf of drivers
Attachment #280932 - Flags: approval1.9? → approval1.9+
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
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.