Last Comment Bug 460969 - Copy/paste of images from content area lose transparency information
: Copy/paste of images from content area lose transparency information
Status: REOPENED
:
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: x86 Windows XP
: -- normal with 5 votes (vote)
: mozilla17
Assigned To: Nobody; OK to take it and work on it
:
: Jim Mathies [:jimm]
Mentors:
: 249032 611977 743176 (view as bug list)
Depends on: 787741 816530 440911 444800 722555 787769
Blocks:
  Show dependency treegraph
 
Reported: 2008-10-21 08:33 PDT by Jim Mathies [:jimm]
Modified: 2014-01-23 05:01 PST (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
example of transparency loss (31.28 KB, image/jpeg)
2008-10-21 08:33 PDT, Jim Mathies [:jimm]
no flags Details
bitmap header v5 support patch .v1 (16.49 KB, patch)
2008-10-24 17:45 PDT, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
Patch to be reviewed (image to clipboard) (28.10 KB, patch)
2012-05-15 15:35 PDT, Eddy Bruel [:ejpbruel]
no flags Details | Diff | Splinter Review
Patch to be reviewed (image from clipboard) (28.10 KB, patch)
2012-05-15 15:42 PDT, Eddy Bruel [:ejpbruel]
no flags Details | Diff | Splinter Review
Patch to be reviewed (image to clipboard) (8.02 KB, patch)
2012-05-20 18:19 PDT, Eddy Bruel [:ejpbruel]
no flags Details | Diff | Splinter Review
Patch to be reviewed (image from clipboard) (7.23 KB, patch)
2012-05-20 18:22 PDT, Eddy Bruel [:ejpbruel]
no flags Details | Diff | Splinter Review
Patch to be reviewed (image to clipboard) (8.15 KB, patch)
2012-06-05 12:01 PDT, Eddy Bruel [:ejpbruel]
netzen: review+
netzen: checkin+
Details | Diff | Splinter Review
Patch to be reviewed (image from clipboard) (8.18 KB, patch)
2012-06-05 12:16 PDT, Eddy Bruel [:ejpbruel]
netzen: review+
Details | Diff | Splinter Review
Patch to be committed (image from clipboard) (8.50 KB, patch)
2012-06-07 14:27 PDT, Eddy Bruel [:ejpbruel]
netzen: review+
emorley: checkin-
Details | Diff | Splinter Review

Description Jim Mathies [:jimm] 2008-10-21 08:33:19 PDT
Created attachment 344094 [details]
example of transparency loss

Outdated bitmap handling code for both rendering and receiving bitmap data from the clipboard causes transparency information to be lost. The code uses older bitmap header / format types which do not support alpha channel or jpg/png compression. On win platforms where these newer formats are available, we should support them so image data isn't lost.

nsImageToClipboard::CreateFromImage -
http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsImageClipboard.cpp#146

nsImageFromClipboard::ConvertColorBitMap -
http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsImageClipboard.cpp#292
Comment 1 Jim Mathies [:jimm] 2008-10-21 09:02:01 PDT
I should add, the drag/drop portion of this is specific to the patch for bug 440911 if it lands. Regular drag and drop of files that use the newer stream interfaces do NOT loose image quality. (in fact I'm removing that from the title as it's probably misleading.)
Comment 2 Jim Mathies [:jimm] 2008-10-24 17:45:34 PDT
Created attachment 344706 [details] [diff] [review]
bitmap header v5 support patch .v1

This is an experimental patch that adds support for sending alpha channel data in a newer bitmap header format. Unfortunately in testing I found that pretty much every app I worked with ignored the alpha data even though it was present. I might come back to this later and experiment around some more, but at this point I don't see any point in making these changes. We could add support on the receiving end so that apps like thunderbird recognize the info. But in general it would have limited usefulness.
Comment 3 [:Aleksej] 2010-04-06 10:07:09 PDT
Related to bug 249032?
Comment 4 Brian R. Bondy [:bbondy] 2011-08-19 10:13:18 PDT
As of Bug 670466 we now have a BMP encoder here: mozilla-central\modules\libpr0n\encoders\bmp with support for generating transparent BMPs w/ 32bpp DIBs.  It also does work with other applications.  I think that should be used instead now that we have it.
Comment 5 Brian R. Bondy [:bbondy] 2011-10-01 12:44:30 PDT
The fix in bug 555176 will make it so this problem is no longer reproducible.   It adds HTML Format (CF_HTML) to the list of data formats and Thunderbird will use that first instead of having to use CF_DIB which doesn't support transaprency info directly.  

An alternate way to fix this would be to properly support a new flavour in addition to the CF_DIB which is CF_DIBV5 and mark it that way on the clipboard.
Comment 6 Kathleen Brade 2011-10-01 19:37:03 PDT
Maybe I'm missing something in comment 5, but can't I reproduce this bug by pasting into the Gimp or other graphic editor?
Comment 7 Brian R. Bondy [:bbondy] 2011-10-01 19:45:39 PDT
Try right clicking on the image and selecting copy image.  Then paste away.
Comment 8 Brian R. Bondy [:bbondy] 2011-10-01 20:03:11 PDT
You can use this sample image:
chrome://branding/content/about-logo.png
Comment 9 Jeff Muizelaar [:jrmuizel] 2012-01-13 11:54:55 PST
*** Bug 249032 has been marked as a duplicate of this bug. ***
Comment 10 Eddy Bruel [:ejpbruel] 2012-04-16 04:57:36 PDT
*** Bug 743176 has been marked as a duplicate of this bug. ***
Comment 11 Eddy Bruel [:ejpbruel] 2012-05-15 15:35:49 PDT
Created attachment 624216 [details] [diff] [review]
Patch to be reviewed (image to clipboard)

This patch reimplements the nsImageToClipboard code in terms of the BMP encoder (note that this depends on a patch for bug 722555 that is currently pending review). This allows us to put images on the clipboard using the CF_DIBV5 format, for which the alpha channel is defined.
Comment 12 Eddy Bruel [:ejpbruel] 2012-05-15 15:42:38 PDT
Created attachment 624218 [details] [diff] [review]
Patch to be reviewed (image from clipboard)

This patch updates the nsImageFromClipboard code to take the alpha channel into account when dealing with 32bpp bitmaps (which is what nsImageToClipboard generates). This solves the immediate transparency problem described in the bug.

Eventually we probably want to reimplement the nsImageFromClipboard code to use the BMP decoder. However, unlike the encoder, the decoder doesn't have a public API, and can only be accessed via the image tools API. This is problematic, because the image tools API only takes an input stream as input, and we need to generate a file header in a separate buffer and feed it to the decoder in addition to the image data. This requires the use of a multiplexed image stream.

Such a stream exists, but after some initial attempts, I considered it too much work for this particular bug. We should probably file a separate bug that addresses this issue.
Comment 13 Brian R. Bondy [:bbondy] 2012-05-15 17:49:39 PDT
I think you attached the wrong patches. Both seem to be the code for the image refactoring for the v5 header.
Comment 14 Eddy Bruel [:ejpbruel] 2012-05-20 18:19:40 PDT
Created attachment 625531 [details] [diff] [review]
Patch to be reviewed (image to clipboard)
Comment 15 Eddy Bruel [:ejpbruel] 2012-05-20 18:22:09 PDT
Created attachment 625532 [details] [diff] [review]
Patch to be reviewed (image from clipboard)

Oops. I don't know how I messed up with those patches, but these should be the correct ones. Please take another look at them :-)
Comment 16 Brian R. Bondy [:bbondy] 2012-05-24 07:53:49 PDT
Sorry for the delay in reviewing. I want to make sure the approach in bug 722555 is accepted before reviewing this.
Comment 17 Eddy Bruel [:ejpbruel] 2012-05-29 10:26:42 PDT
(In reply to Brian R. Bondy [:bbondy] from comment #16)
> Sorry for the delay in reviewing. I want to make sure the approach in bug
> 722555 is accepted before reviewing this.

Could you please go ahead and review them anyway? jgilbert is apparently very busy, so the BMP encoder patch is taking a long time to get r+'d. If there are any issues with these patches it would be much more efficient if I can fix those now. rather than wait until the BMP encoder patch gets approved.

We can still wait with landing this stuff until the approach in bug 722555 is accepted, of course (note that jgilbert's first review was a r-, but didn't seem to have issue with the overall approach taken).
Comment 18 Jeff Gilbert [:jgilbert] 2012-05-29 10:32:59 PDT
I'll have that review done this afternoon, fwiw.
Comment 19 Brian R. Bondy [:bbondy] 2012-05-29 13:20:39 PDT
Yup I'll look into this soon, I was only concerned with the v5 header being accepted in general.
Comment 20 Brian R. Bondy [:bbondy] 2012-05-30 13:18:39 PDT
Comment on attachment 625532 [details] [diff] [review]
Patch to be reviewed (image from clipboard)

Review of attachment 625532 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsImageClipboard.cpp
@@ +237,2 @@
>  
> +  if (rgbaData) {

You can remove this check since new is infallible by default.
https://developer.mozilla.org/en/Infallible_memory_allocation

@@ +426,5 @@
>              PRUint32 val = *((PRUint32*) (aInputBuffer + index) );
>              aOutBuffer[writeIndex++] = (val & colorMasks.red) >> colorMasks.redRightShift << colorMasks.redLeftShift;
>              aOutBuffer[writeIndex++] =  (val & colorMasks.green) >> colorMasks.greenRightShift << colorMasks.greenLeftShift;
>              aOutBuffer[writeIndex++] = (val & colorMasks.blue) >> colorMasks.blueRightShift << colorMasks.blueLeftShift;
> +            aOutBuffer[writeIndex++] = 0xFF;

It would be better to use the biefields mask here no?

@@ +438,2 @@
>              aOutBuffer[writeIndex++] = aInputBuffer[index];
> +            aOutBuffer[writeIndex++] = aInputBuffer[index+3];

This code assumes that the input format buffer always contain 4 bytes but that is not always the case.  The old code only used the first 3 bytes but then skipped over the 4th byte if it was present.
Comment 21 Brian R. Bondy [:bbondy] 2012-05-30 13:19:39 PDT
Comment on attachment 625531 [details] [diff] [review]
Patch to be reviewed (image to clipboard)

Review of attachment 625531 [details] [diff] [review]:
-----------------------------------------------------------------

nsImageToClipboard::CreateFromImage doesn't seem to be working correctly.

I right clicked on an image that was transparent and copied image data, then tried to paste it in mspaint. It had a black background on the transparent parts.

::: widget/windows/nsImageClipboard.cpp
@@ +185,3 @@
>  
> +    PRUint32 size;
> +    encoder->GetImageBufferUsed(&size);

Please check here that size more than BFH_LENGTH you can use NS_ENSURE_TRUE here.

You can also simplify the surrounding code by simply checking NS_ENSURE_SUCCESS(rv, rv).  The advantage of that is it will also log a message to the error console.
Comment 22 Eddy Bruel [:ejpbruel] 2012-06-05 12:01:23 PDT
Created attachment 630256 [details] [diff] [review]
Patch to be reviewed (image to clipboard)
Comment 23 Eddy Bruel [:ejpbruel] 2012-06-05 12:16:01 PDT
Created attachment 630265 [details] [diff] [review]
Patch to be reviewed (image from clipboard)
Comment 24 Brian R. Bondy [:bbondy] 2012-06-06 08:21:36 PDT
Comment on attachment 630256 [details] [diff] [review]
Patch to be reviewed (image to clipboard)

Review of attachment 630256 [details] [diff] [review]:
-----------------------------------------------------------------

Please r=me with the nits addressed.

::: widget/windows/nsImageClipboard.cpp
@@ +158,2 @@
>  
> +    nsCOMPtr<imgIEncoder> encoder = do_CreateInstance("@mozilla.org/image/encoder;2?type=image/bmp", &rv);

nit: Please put the do_CreateInstance on the next line indented an extra 2 spaces from the start of nsCOMPtr.

@@ +190,5 @@
>  
> +    char *dst = (char*) ::GlobalLock(glob);
> +    char *src;
> +    rv = encoder->GetImageBuffer(&src);
> +    NS_ENSURE_SUCCESS(rv, rv);

Move these 3 lines:
char *src;
rv = encoder->GetImageBuffer(&src);
NS_ENSURE_SUCCESS(rv, rv);

above the "GlobalAlloc" and just below "NE_ENSURE_TRUE(size,..." to avoid having to do extra cleanup on failure conditions.
Comment 25 Brian R. Bondy [:bbondy] 2012-06-06 08:21:52 PDT
Comment on attachment 630265 [details] [diff] [review]
Patch to be reviewed (image from clipboard)

Review of attachment 630265 [details] [diff] [review]:
-----------------------------------------------------------------

I made a small test program by the way and could extract both data for CF_DIB and CF_DIBV5.
Please r=me with the nits addressed. 
Please push both patches to try and ensure all try tests pass before pushing it out.

Thanks so much for the patches, good work!

::: widget/windows/nsImageClipboard.cpp
@@ +229,5 @@
>    PRInt32 height = header->bmiHeader.biHeight;
>    // neg. heights mean the Y axis is inverted and we don't handle that case
>    NS_ENSURE_TRUE(height > 0, NS_ERROR_FAILURE); 
>  
> +  unsigned char * rgbaData = new unsigned char[width * height * 4 /* RGB */];

nit: /* RGBA */

I know last time I said that the "if (rgbaData)" wasn't needed (and it's not as is), but as I think about it more it is probably best to allocate this with "new (fallible)" instead and then re-add the check and return NS_ERROR_OUT_OF_MEMORY as before.  The reason is because there may be a big image and not enough memory and in that case it would be better not to abort.

@@ +424,2 @@
>              numPixelsLeftInRow--;
>              pos += 4; // we read in 4 bytes of data in order to process this pixel

Please add a comment above this line that BI_BITFIELDS only supports 16bpp and 32bpp so we always add 4 bytes.
Comment 26 Brian R. Bondy [:bbondy] 2012-06-06 08:23:35 PDT
Also you have to wait for the image lib changes to land first or at the same time.
Comment 27 Eddy Bruel [:ejpbruel] 2012-06-07 14:27:33 PDT
Created attachment 631139 [details] [diff] [review]
Patch to be committed (image from clipboard)
Comment 28 Brian R. Bondy [:bbondy] 2012-06-07 18:22:54 PDT
https://tbpl.mozilla.org/?tree=Try&rev=9d57dc3bc0ea
Comment 29 Brian R. Bondy [:bbondy] 2012-06-08 20:34:01 PDT
Waiting to land for the problem to be fixed in bug 722555 Comment 30.
Comment 30 Brian R. Bondy [:bbondy] 2012-06-13 06:30:12 PDT
Comment on attachment 630256 [details] [diff] [review]
Patch to be reviewed (image to clipboard)

Dependent patch needs to land yet, see bug 722555 Comment 30.
Comment 31 Brian R. Bondy [:bbondy] 2012-06-13 06:30:30 PDT
Comment on attachment 631139 [details] [diff] [review]
Patch to be committed (image from clipboard)

Dependent patch needs to land yet, see bug 722555 Comment 30.
Comment 32 Eddy Bruel [:ejpbruel] 2012-07-17 07:30:27 PDT
Comment on attachment 631139 [details] [diff] [review]
Patch to be committed (image from clipboard)

Dependent patch just landed :-)
Comment 35 Ed Morley [:emorley] 2012-07-31 08:31:02 PDT
Comment on attachment 631139 [details] [diff] [review]
Patch to be committed (image from clipboard)

(In reply to Eddy Bruel [:ejpbruel] from comment #34)
> http://hg.mozilla.org/integration/mozilla-inbound/rev/31f42dd24f25

This changeset backed out for mochitest-other failures in test_bug444800.xul:
https://tbpl.mozilla.org/php/getParsedLog.php?id=14004465&tree=Mozilla-Inbound
{
34246 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/widget/tests/test_bug444800.xul | an unexpected uncaught JS exception reported through window.onerror - NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsITransferable.getAnyTransferData] at chrome://mochitests/content/chrome/widget/tests/test_bug444800.xul:64 
}

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d7c387f47ec
Comment 36 Eddy Bruel [:ejpbruel] 2012-07-31 08:35:05 PDT
(In reply to Ed Morley [:edmorley] from comment #35)
> Comment on attachment 631139 [details] [diff] [review]
> Patch to be committed (image from clipboard)
> 
> (In reply to Eddy Bruel [:ejpbruel] from comment #34)
> > http://hg.mozilla.org/integration/mozilla-inbound/rev/31f42dd24f25
> 
> This changeset backed out for mochitest-other failures in test_bug444800.xul:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=14004465&tree=Mozilla-
> Inbound
> {
> 34246 ERROR TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/chrome/widget/tests/test_bug444800.xul | an
> unexpected uncaught JS exception reported through window.onerror -
> NS_ERROR_FAILURE: Component returned failure code: 0x80004005
> (NS_ERROR_FAILURE) [nsITransferable.getAnyTransferData] at
> chrome://mochitests/content/chrome/widget/tests/test_bug444800.xul:64 
> }
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/2d7c387f47ec

I'm sorry, that was careless. I ran the patch on try before I pushed it but had assumed it was a known intermittent. Obviously its not. I'll look into it.
Comment 37 Ed Morley [:emorley] 2012-07-31 08:39:47 PDT
No problem. In cases where the intermittent is known, there should be bugs suggested by TBPL. If there aren't, you've either been extremely unlucky to get a new (as yet unfiled) intermittent, or it is real :-)

A debug build has just finished - here's the log, in case it helps you debug more easily:
https://tbpl.mozilla.org/php/getParsedLog.php?id=14005151&tree=Mozilla-Inbound
Comment 39 rsx11m 2012-12-04 07:18:26 PST
Comment on attachment 631139 [details] [diff] [review]
Patch to be committed (image from clipboard)

I'm confused - in light of bug 816530, it appears that the image-from-clipboard part was backed out but there was no action since. Has this ever been fixed?
Comment 40 Jeff Gilbert [:jgilbert] 2012-12-04 18:56:16 PST
(In reply to rsx11m from comment #39)
> Comment on attachment 631139 [details] [diff] [review]
> Patch to be committed (image from clipboard)
> 
> I'm confused - in light of bug 816530, it appears that the
> image-from-clipboard part was backed out but there was no action since. Has
> this ever been fixed?

It does look like this is the case.

Indeed, this was backed out here:
http://hg.mozilla.org/mozilla-central/rev/2d7c387f47ec
Comment 41 Tiziana Sellitto [:tiziana] 2013-07-12 09:21:06 PDT
*** Bug 611977 has been marked as a duplicate of this bug. ***
Comment 42 Hamish Robertson 2014-01-23 05:01:51 PST
Is progress likely to be made on this issue?

It seems to have stalled since #816530.

Note You need to log in before you can comment on or make changes to this bug.