444800 - cannot retrieve image data from clipboard in lossless format

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P4)

Description

[Mark Smith [:mcs]]

On Windows, it is not possible to retrieve clipboard data in a lossless format.  The data is always converted to JPEG format, which is bad for applications such as Thunderbird and for Firefox extensions that want to access image data off the clipboard.

This is because the implementation in nsImageFromClipboard::GetEncodedImageStream() always converts DIB data to a JPEG stream, and because the code in nsClipboard::GetDataFromDataObject() was written to only handle JPEG format.

See: http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsImageClipboard.cpp#208

And: http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsClipboard.cpp#565
 
Related: Bug 384116 (Pasting an image from clipboard uses bad quality JPEG-compression)

Comment 1

[rsx11m]

xref: The current code for recoding in JPEG was introduced by bug 223909.

Comment 2

[rsx11m]

From bug 384116 comment #41:
> 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]. ...

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

Comment 3

[rsx11m]

Further references: The corresponding patch for the Cocoa widgets (Mac OSX) is bug 393646, for GTK widgets (Linux) that's bug 21747. Both consider JPEG, PNG, and GIF as paste flavors on the clipboard without recoding. JPEG as the only flavor for the HTML editor was apparently enforced by bug 223909 since image pasting was first implemented on Windows (now extended by bug 384116).

Thus, Windows appears to be the only platform for which the Widgets also have to consult the clipboard.paste_image_quality for JPEG encoding from CF_DIB.

Comment 4

[rsx11m]

Attachment: Partial patch for GetEncodedImageStream (draft, untested)

This draft for nsImageFromClipboard::GetEncodedImageStream() has been modified from attachment 320403 [details] [diff] [review] to accommodate Mike's bug 384116 comment #38.
Rather than getting flavor and quality directly from the preferences, they now have to be passed as parameters. The value ranges and defaults are identical to those specified in attachment 322667 [details] [diff] [review].

The remaining problem is to patch nsClipboard::GetDataFromDataObject() to either get the preference values (e.g., as used by HTML editor, where the flavor is indirectly passed by the order of acceptable flavors listed) and pass them to the encoder, or to utilize whatever the caller (extension case) desires. I've also noticed that there doesn't seem to be a GIF encoder, thus clipboard.paste_image_type=2 is mapped to JPEG instead.

Whoever takes this, please incorporate the draft and make sure that the final patch works with the current implementation of bug 384116 for the editor.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Jim, could you take a look at this patch and see if that looks like the right approach?

Comment 6

[Jim Mathies [:jimm]]

sure...

Comment 7

[Mark Smith [:mcs]]

I have done some thinking about how this should work but have not found time to write any code yet.  I do not think the patch in attachment 330456 [details] [diff] [review] uses the right approach.  If an application such as Thunderbird wants to provide a preference that specifies a "priority" or preferred order for image formats, that logic belongs in the application.  The clipboard code should allow the application to retrieve any and all available formats, including those that may be obtained via conversion (I think that is how things work today with the Mac OS X widget code).

To make things more concrete, when an image is present on the system clipboard, I'd like to be able to call nsIClipboard.hasDataMatchingFlavors() and request image/png or image/jpeg (and both calls should return true).  Then of course nsIClipboard.getData() and related calls may be used to retrieve the data in the requested format.

I am not sure I would start by modifying  nsImageFromClipboard::GetEncodedImageStream(), but if I did I would change that method's signature to look something like:

nsresult nsImageFromClipboard::GetEncodedImageStream (
           unsigned char * aClipboardData,
           nsIInputStream ** aInputStream,
           char * encodeType,     // e.g., image/png or image/jpeg
           char * encoderOptions  // e.g., quality=90
);

Essentially, keep it as general as possible.

Comment 8

[rsx11m]

(In reply to comment #7) I certainly agree with moving the parameters as far outside as possible and keep the lower-level functions as general as possible. The outer limit here would be nsHTMLEditor (thus still in Core code, not the application as suggested), for which the clipboard.paste_image_type preference was introduced in bug 384116. In contrast, not all clipboard mechanisms need to explicitly convert to JPEG and would allow the quality to be determined in the process, thus clipboard.paste_image_quality is present currently for Windows only and should be handled by the respective platform-specific widget.

> To make things more concrete, when an image is present on the system clipboard,
> I'd like to be able to call nsIClipboard.hasDataMatchingFlavors() and request
> image/png or image/jpeg (and both calls should return true).  Then of course
> nsIClipboard.getData() and related calls may be used to retrieve the data in
> the requested format.

In general, this was also the idea of the Editor patch. The list of flavors in nsHTMLEditor::PrepareHTMLTransferable() already reflects the setting by listing the flavors in the order of preference. If available in nsIClipboard.getData(), this could be used to determine the parameters for GetEncodedImageStream () without having to consult the value of clipboard.paste_image_type again. However, clipboard.paste_image_quality should nevertheless be processed by the Windows version of nsIClipboard.getData() itself to obtain the quality value, unless its interface would be extended (implying for all platforms, also those which do not need it) to accommodate such additional options.

Comment 9

[Kathleen :Brade]

Attachment: patch to return image/png data if requested

Mark Smith and I developed this patch to add support for image/png to the Windows clipboard retrieval code.  I am not addressing the JPEG quality issue because that is covered in bug 444898.

Comment 10

[rsx11m]

Ok, thus nsClipboard::GetDataFromDataObject() should actually do what I was thinking of in comment #8 and consider the order of flavors as the priority.

> --- widget/src/windows/nsImageClipboard.cpp	17 Sep 2007 18:02:50 -0000	1.12
> +++ widget/src/windows/nsImageClipboard.cpp	10 Sep 2008 16:39:30 -0000
>  nsresult 
> -nsImageFromClipboard ::GetEncodedImageStream (unsigned char * aClipboardData, nsIInputStream** aInputStream )
> +nsImageFromClipboard ::GetEncodedImageStream (unsigned char * aClipboardData, const char * aMIMEFormat, nsIInputStream** aInputStream )

This contradicts comment #7 in response to my comments to pass down any options to the encoder. There is no way now to pass any option to the encoders by the Windows clipboard version at all, thus restricting it to the defaults and in fact ignoring clipboard.paste_image_quality entirely. Was this intended?

> (In reply to comment #9)  I am not addressing the JPEG quality issue
> because that is covered in bug 444898.

Kathleen, bug 444898 only covers the default settings for the generic JPEG encoder, which is used by the clipboard but also other components. Therefore, the clipboard.* preferences should not be handled at that level either.

Thus, the question is, where should those encoding options be handled (which depend on the platform, as stated before, thus the Widgets appear to be the appropriate place), or are the defaults (with new "quality=92" for JPEG per
bug 444898) sufficient for the encoding and clipboard.paste_image_quality can/should be removed?

Comment 11

[Mark Smith [:mcs]]

(In reply to comment #10)
> Ok, thus nsClipboard::GetDataFromDataObject() should actually do what I was
> thinking of in comment #8 and consider the order of flavors as the priority.

My understanding is that the image format priority issue was addressed by the fix for bug 384116 (by setting several flavors inside the nsITransferable).

> ...
> Thus, the question is, where should those encoding options be handled (which
> depend on the platform, as stated before, thus the Widgets appear to be the
> appropriate place), or are the defaults (with new "quality=92" for JPEG per
> bug 444898) sufficient for the encoding and clipboard.paste_image_quality
> can/should be removed?

The scope of this bug was to make PNG available.  Kathleen and I made the assumption that setting the default JPEG quality to 92 (bug 444898) was sufficient to address the quality issue for people who need to continue to use JPEG format.

I do not know what the requirements are, but I suspect that once it is available people who care about image quality will switch to PNG (e.g., in Thunderbird and in Firefox extensions).  What do other people think?

Comment 12

[rsx11m]

(In reply to comment #11)
> My understanding is that the image format priority issue was addressed by the
> fix for bug 384116 (by setting several flavors inside the nsITransferable).

Yes, that's the intended behavior and worked fine with the Linux clipboard.
The editor puts the flavors in the order of preference, thus taking the first flavor of this list to determine the encoding provides the desired result.

> I do not know what the requirements are, but I suspect that once it is
> available people who care about image quality will switch to PNG (e.g., in
> Thunderbird and in Firefox extensions).  What do other people think?

The discussion in bug 384116 went a bit forth and back with different scopes, so there are no requirements in a strict sense. It was observed that the JPEG encoder used for pasting from the Windows clipboard provides a quality option, therefore the respective preference, and my main motivation here was to carry over that part of the discussion. Personally, I would agree that the choice to paste in lossy but good quality JPEG or in lossless PNG is more important than selecting a specific JPEG quality (and the other platforms are using fixed quality settings for pasting in JPEG as well, so that would be consistent).

Comment 13

[Jim Mathies [:jimm]]

Comment on attachment 337893 [details] [diff] [review]
patch to return image/png data if requested

Looks good to me. I think the comment below ConvertColorBitMap needs a slight update. Have we run this through try server?

Comment 14

[rsx11m]

Comment on attachment 330456 [details] [diff] [review]
Partial patch for GetEncodedImageStream (draft, untested)

This draft is now definitely obsolete.

If the clipboard.paste_image_quality preference is not going to be used after all, it should probably be removed as it was introduced solely for the purpose of making the quality configurable on Windows. Thus, you could either add removal of the last three lines of editor/ui/composer.js to your patch, or we can open a follow-up bug to investigate whether the configurability should be made possible for this and/or other platforms.

Comment 15

[Kathleen :Brade]

(In reply to comment #14)
> ...Thus, you could either add
> removal of the last three lines of editor/ui/composer.js to your patch, or we
> can open a follow-up bug to investigate whether the configurability should be
> made possible for this and/or other platforms.

I'm not sure whether configurability is needed or not.  Could you open a separate bug and cc me?  ([e-mail])  Thanks!

Comment 16

[Kathleen :Brade]

Attachment: patch with revised comments for CVS trunk

carryforward r+ from previous patch

Comment 17

[Kathleen :Brade]

Attachment: patch for mozilla central

This is the mozilla central patch.  It is the same as the other patch except we had to adjust a few new calls to GetNativeDataOffClipboard().

Both patches built successfully on the try servers.

Who should super-review?

Comment 18

[rsx11m]

(In reply to comment #15)
> I'm not sure whether configurability is needed or not.  Could you open a
> separate bug and cc me?

Done, that's bug 456086. I filed it independent of the platform, in case somebody wants to implement that preference for a different platform.

Comment 19

[Mark Smith [:mcs]]

Attachment: test case

Here is a Mochitest automated test.  It uses the browser's cmd_copyImageContents to copy a BMP to the system clipboard.  Then it exercises the clipboard service and ensures that both hasDataMatchingFlavors() and getData() are able to return image/png and image/jpg data.

Unfortunately, the test code fails on Windows (it works fine on Mac).  The call to getData() is not returning any data.  This happens because the call to ::OleGetClipboard() inside nsClipboard::GetNativeClipboardData() fails with error CLIPBRD_E_CANT_OPEN.

Jim M -- any idea what would cause that Win32 call to fail?  We have a Firefox extension that makes exactly the same clipboard service calls from a component, and in that case the call to ::OleGetClipboard() succeeds (and therefore getData() returns image data, and life is good).

Comment 20

[Kathleen :Brade]

Comment on attachment 339491 [details] [diff] [review]
patch for mozilla central

Obsoleting this patch.  There is a problem with image/jpeg vs image/jpg.  New patch coming.

Comment 21

[Kathleen :Brade]

Attachment: revised patch for CVS trunk

Revised patch (fixed a problem where the JPEG encoder was not loaded because it is registered as image/jpeg, not image/jpg).

Comment 22

[Kathleen :Brade]

Attachment: revised patch for mozilla-central

Revised patch (fix image/jpeg vs. image/jpg problem for encoder).

Comment 23

[Kathleen :Brade]

Attachment: revised test

We found a workaround for the OleGetClipboard() problem.  If we add a delay (setTimeout(..., 0)) between copying the test image to the clipboard and pulling it off, everything works as expected.

I have run the test successfully on Mac (mozilla-central) and Windows (CVS trunk and mozilla-central).

Comment 24

[(not currently active) Ted Mielczarek]

Comment on attachment 339975 [details]
test case

Two drive-by nits on this patch:
1) I get an XML parsing error trying to view it here, it doesn't seem to like the one script element that has an empty body (packed.js)
2) There doesn't seem to be any reason to use SimpleTest.waitForExplicitFinish() in this test, as you run all the tests in runTests anyway.

Comment 25

[Kathleen :Brade]

Ted (comment 24) -- the XML parsing error is probably due to you not running a mochikit-build of the browser.  The error is caused by chrome://mochikit/... not being loaded in the browser.

You are right about the second point but the new patch has a timer (to work around the OleGetClipboard problem) so multiple functions exist in the latest test.

Comment 26

[Jim Mathies [:jimm]]

Comment on attachment 340583 [details] [diff] [review]
revised patch for mozilla-central

Rob, I'm guessing you're the best sr,  but if not Kathleen would appreciate a suggestion.

Comment 27

[Jim Mathies [:jimm]]

>Unfortunately, the test code fails on Windows (it works fine on Mac).  The call
>to getData() is not returning any data.  This happens because the call to
>::OleGetClipboard() inside nsClipboard::GetNativeClipboardData() fails with
>error CLIPBRD_E_CANT_OPEN.

Interesting problem. I get a few references out there to problems related to calling OleSetClipboard and OleGetClipboard on top of each other failing. The flush call closes it and renders the data, which is held by an internal ole window. I suppose it might have to do with the message queue needing to get processed for the cliboard operations to complete, but that's just a guess.

Comment 28

[Kathleen :Brade]

Fixed committed: http://hg.mozilla.org/mozilla-central/rev/4c9cfa6569df

Comment 29

[Doug Turner (:dougt)]

this broke the windows mobile builds:

nsClipboard.cpp
c:/builds/mobile/mozilla-central/widget/src/windows/nsClipboard.cpp(817) : error C2665: 'nsClipboard::GetNativeDataOffClipboard' : none of the 2 overloads could
 convert all the argument types
        c:\builds\mobile\mozilla-central\widget\src\windows\nsClipboard.h(76): could be 'nsresult nsClipboard::GetNativeDataOffClipboard(nsIWidget *,UINT,const
char *,void **,PRUint32 *)'
        c:\builds\mobile\mozilla-central\widget\src\windows\nsClipboard.h(77): or 'nsresult nsClipboard::GetNativeDataOffClipboard(IDataObject *,UINT,const char
 *,void **,PRUint32 *)'
        while trying to match the argument list '(IDataObject *, UINT, const unsigned short [23], void **, PRUint32 *)'
c:/builds/mobile/mozilla-central/widget/src/windows/nsClipboard.cpp(829) : error C2665: 'nsClipboard::GetNativeDataOffClipboard' : none of the 2 overloads could
 convert all the argument types
        c:\builds\mobile\mozilla-central\widget\src\windows\nsClipboard.h(76): could be 'nsresult nsClipboard::GetNativeDataOffClipboard(nsIWidget *,UINT,const
char *,void **,PRUint32 *)'
        c:\builds\mobile\mozilla-central\widget\src\windows\nsClipboard.h(77): or 'nsresult nsClipboard::GetNativeDataOffClipboard(IDataObject *,UINT,const char
 *,void **,PRUint32 *)'
        while trying to match the argument list '(IDataObject *, UINT, const unsigned short [23], void **, PRUint32 *)'
make[1]: *** [nsClipboard.obj] Error 2
make[1]: Leaving directory `/c/builds/mobile/mozilla-central/objdir-wm6-d/xulrunner/widget/src/windows'
make: *** [all] Error 2


It looks like you are calling GetNativeDataOffClipboard() with the 3rd param as a wide string.  The functions only take narrow strings as the 3rd param.

The values you pass are either CFSTR_INETURLA or CFSTR_INETURLW -- both are defined as L"".  This param is then passed to the function GetFormat() which only deals with narrow strings.

Something is wrong.

Comment 30

[Kathleen :Brade]

You are right; something seems wrong.  Investigating now...

Comment 31

[Doug Turner (:dougt)]

brade, can you back this out until it works?

Comment 32

[Kathleen :Brade]

Attachment: fix for unicode builds

Comment 33

[Doug Turner (:dougt)]

Comment on attachment 342454 [details] [diff] [review]
fix for unicode builds

this patch does make the compiler bustage go away, but i am not sure it is right.  I am looking at the function:

GetNativeDataOffClipboard

It always takes a char* as its mime format regardless of UNICODE being defined or not. It uses this value in strcmp's.  So, i am not sure why or how this could work if you passed it a wide string.

Shouldn't you alwasy be passing a narrow string to GetNativeDataOffClipboard as the mimeformat?

Comment 34

[Mark Smith [:mcs]]

The goal of attachment 342454 [details] [diff] [review] *was* to ensure that GetNativeDataOffClipboard() is always passed a char *.  brade told to me that she and dougt talked about this on irc, and he pointed to this code:

http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsDataObj.h#86

where the CFSTR_INETURLA and CFSTR_INETURLW macros are defined as wide strings if they are not already defined.  I suspect everyone is using a version of the Platform SDK that already defines these, but I am not sure.  Microsoft's definitions use the TEXT() macro so they are only wide in Unicode builds.

Therefore, brade and I believe that the right solution is to change the definitions in nsDataObj.h to match Microsoft's; that is, to use TEXT().  They were recently changed from "always narrow" to "always wide" (bug 422792).

Does the Windows Mobile 6 SDK use a different ShlObj.h that the Win32 SDK?  Does it define CFSTR_INETURLA and CFSTR_INETURLW?  If so, how?

Comment 35

[Doug Turner (:dougt)]

What ever string you use as the "mime format" in the call GetNativeDataOffClipboard, it must be a char*.  This is because GetNativeDataOffClipboard calls GetFormat() which does strcmp's on the incoming string.

what am I missing?

Comment 36

[Mark Smith [:mcs]]

(In reply to comment #35)
> What ever string you use as the "mime format" in the call
> GetNativeDataOffClipboard, it must be a char*.  This is because
> GetNativeDataOffClipboard calls GetFormat() which does strcmp's on the incoming
> string.
> 
> what am I missing?

The patch in attachment 342454 [details] [diff] [review] ensures that a char * is always passed to GetNativeDataOffClipboard() (either via conversion of the mime format in the UNICODE case or by directly using narrow strings in the non-UNICODE case).  The only way this won't be true is if the L"..." CFSTR_INETURLA and CFSTR_INETURLW definitions in nsDataObj.h are used instead of Microsoft's TEXT("...") definitions.

It is very confusing for the Mozilla code to (potentially) define those macros differently than Microsoft does, and I can't see any good reason to leave things that way.  There are only used inside widget/src/windows/nsClipboard.cpp and widget/src/windows/nsDataObj.cpp, so this is a fairly low impact thing to fix.

Comment 37

[Brad Lassey [:blassey] (use needinfo?)]

GetNativeDataOffClipboard passes the format string to GetFormat which does a bunch of comparisons of ascii strings using strcmp.  Then GetFromat converts to unicode to make a system call.

We are doing way too many conversions.  All of this should just being using unicode strings.  As to the previous comment, Mozilla should be using CFSTR_INETURLW and the wide versions of these APIs.

Comment 38

[Kathleen :Brade]

Brad (comment 37) -- CFSTR_INETURLW and CFSTR_INETURLA are clipboard flavors.  The "W" does NOT mean that the string itself is wide (in fact, that is determined by the TEXT macro).  Both CFSTR_INETURLW and CFSTR_INETURLA are wide in UNICODE builds and char* in other builds.  Mozilla needs to support both clipboard flavors so CFSTR_INETURLA should not go away.

As for the string conversion issues, those are not easily addressed since some of the constant strings (such as kPNGImageMime) are defined in nsITransferable.idl.  The nsITransferable APIs use char* for mimetypes.

For example, the code in nsClipboard::SetupNativeDataObject() loops through a set of mimetypes returned by the nsITransferable.  See http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsClipboard.cpp#174.
If we change GetFormat() to always take a wide string, then we will have to convert each mimetype as we go through the loop, even for well-known formats such as text/plain and image/png.

Comment 39

[Stuart Parmenter]

can we back this out until the issues is fixed?

Comment 40

[Kathleen :Brade]

Backed out:
  http://hg.mozilla.org/mozilla-central/rev/be961e0dcf17

Comment 41

[Kathleen :Brade]

Attachment: revised fix: avoid extra string conversions

Instead of replacing the UINT format that was passed to GetNativeDataOffClipboard(), I pass the MIME type as an additional parameter when it is needed (for images).  This approach leaves the existing GetFormat() calls intact, and avoids introducing any additional string conversions.

Comment 42

[Doug Turner (:dougt)]

Comment on attachment 343045 [details] [diff] [review]
revised fix: avoid extra string conversions

I do not understand this change:

@@ -462,25 +466,26 @@ nsresult nsClipboard::GetNativeDataOffCl
                 if ( NS_SUCCEEDED(GetGlobalData(stm.hGlobal, aData, &allocLen)) ) {
                   *aLen = nsCRT::strlen(reinterpret_cast<PRUnichar*>(*aData)) * 2;
                   result = NS_OK;
                 }
               } break;
 
 #ifndef WINCE
             case CF_DIB :
+              if (aMIMEImageFormat)


@@ -220,19 +224,26 @@ nsImageFromClipboard ::GetEncodedImageSt

if we can't get the encoder, lets assert.

give jmathies another shot at this.

Comment 43

[Kathleen :Brade]

DougT (comment 42) -- Strictly speaking, the "if (aMIMEImageFormat)" line is not needed.  I added the check to avoid the memory allocation in GetGlobalData().  However, GetEncodedImageStream() will return an error if it is passed a null mimetype.

If an encoder isn't found in GetEncodedImageStream(), an error is returned.  I can add an assertion if Jim is ok with that.

JimM -- What do you think?

Comment 44

[Jim Mathies [:jimm]]

(In reply to comment #43)
> DougT (comment 42) -- Strictly speaking, the "if (aMIMEImageFormat)" line is
> not needed.  I added the check to avoid the memory allocation in
> GetGlobalData().  However, GetEncodedImageStream() will return an error if it
> is passed a null mimetype.
> 
> If an encoder isn't found in GetEncodedImageStream(), an error is returned.  I
> can add an assertion if Jim is ok with that.
> 
> JimM -- What do you think?

(In reply to comment #43)
> DougT (comment 42) -- Strictly speaking, the "if (aMIMEImageFormat)" line is
> not needed.  I added the check to avoid the memory allocation in
> GetGlobalData().  However, GetEncodedImageStream() will return an error if it
> is passed a null mimetype.
> 
> If an encoder isn't found in GetEncodedImageStream(), an error is returned.  I
> can add an assertion if Jim is ok with that.
> 
> JimM -- What do you think?

Side note, why don't we tighten this up and do what the other cases do - 

if ( NS_SUCCEEDED(GetGlobalData...

the compiler's going to optimize out rv anyway but might as well streamline the code a bit.

I'm fine with the check on aMIMEImageFormat to prevent the allocation.

The old pattern was to check on inputStream after the call to GetEncodedImageStream() so I'm not sure why we need to add an assert on the result of this particular call?

Comment 45

[Kathleen :Brade]

Attachment: revised fix: tighten up NS_SUCCEEDED(GetGlobalData...) check

The only change is the one requested by Jim (tighten up the "if ( NS_SUCCEEDED(GetGlobalData...)" check).  Please review.

Comment 46

[Kathleen :Brade]

Revised fix committed:
  http://hg.mozilla.org/mozilla-central/rev/449af6c4ca81

Comment 47

[rsx11m]

Works well with the recent Thunderbird tinderbox build, see bug 460285.
Thanks to Kathleen and Mark for taking care of this!