Copy/paste of images from content area lose transparency information

REOPENED
Unassigned

Status

()

Core
Widget: Win32
REOPENED
9 years ago
3 years ago

People

(Reporter: jimm, Unassigned)

Tracking

(Depends on: 2 bugs)

Trunk
mozilla17
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Reporter)

Description

9 years ago
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
(Reporter)

Comment 1

9 years ago
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.)
(Reporter)

Updated

9 years ago
Summary: Copy/paste & drag/drop of images from content area loses transparency information → Copy/paste of images from content area loses transparency information
(Reporter)

Updated

9 years ago
Summary: Copy/paste of images from content area loses transparency information → Copy/paste of images from content area lose transparency information
(Reporter)

Comment 2

9 years ago
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.
(Reporter)

Updated

9 years ago
Assignee: jmathies → nobody

Comment 3

7 years ago
Related to bug 249032?
Whiteboard: DUPEME? bug 249032
Assignee: nobody → netzen
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.
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

6 years ago
Maybe I'm missing something in comment 5, but can't I reproduce this bug by pasting into the Gimp or other graphic editor?
Try right clicking on the image and selecting copy image.  Then paste away.
You can use this sample image:
chrome://branding/content/about-logo.png
Duplicate of this bug: 249032

Updated

5 years ago
Whiteboard: DUPEME? bug 249032

Updated

5 years ago
Assignee: netzen → ejpbruel

Updated

5 years ago
Duplicate of this bug: 743176

Updated

5 years ago
Depends on: 722555
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.
Attachment #344706 - Attachment is obsolete: true
Attachment #624216 - Flags: review?(netzen)

Updated

5 years ago
Attachment #624216 - Attachment is patch: true
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.
Attachment #624218 - Flags: review?(netzen)
Attachment #624218 - Flags: review?(netzen)
Attachment #624216 - Flags: review?(netzen)
I think you attached the wrong patches. Both seem to be the code for the image refactoring for the v5 header.
Created attachment 625531 [details] [diff] [review]
Patch to be reviewed (image to clipboard)
Attachment #624216 - Attachment is obsolete: true
Attachment #625531 - Flags: review?(netzen)
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 :-)
Attachment #624218 - Attachment is obsolete: true
Attachment #625532 - Flags: review?(netzen)
Sorry for the delay in reviewing. I want to make sure the approach in bug 722555 is accepted before reviewing this.
(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).
I'll have that review done this afternoon, fwiw.
Yup I'll look into this soon, I was only concerned with the v5 header being accepted in general.
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.
Attachment #625532 - Flags: review?(netzen)
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.
Attachment #625531 - Flags: review?(netzen)
Created attachment 630256 [details] [diff] [review]
Patch to be reviewed (image to clipboard)
Attachment #625531 - Attachment is obsolete: true
Attachment #630256 - Flags: review?(netzen)
Created attachment 630265 [details] [diff] [review]
Patch to be reviewed (image from clipboard)
Attachment #625532 - Attachment is obsolete: true
Attachment #630265 - Flags: review?(netzen)
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.
Attachment #630256 - Flags: review?(netzen) → review+
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.
Attachment #630265 - Flags: review?(netzen) → review+
Also you have to wait for the image lib changes to land first or at the same time.

Updated

5 years ago
Attachment #630256 - Flags: checkin?(netzen)

Updated

5 years ago
Attachment #630265 - Flags: checkin?(netzen)
Created attachment 631139 [details] [diff] [review]
Patch to be committed (image from clipboard)
Attachment #630265 - Attachment is obsolete: true
Attachment #630265 - Flags: checkin?(netzen)
Attachment #631139 - Flags: checkin?(netzen)
https://tbpl.mozilla.org/?tree=Try&rev=9d57dc3bc0ea
Waiting to land for the problem to be fixed in bug 722555 Comment 30.
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.
Attachment #630256 - Flags: checkin?(netzen) → checkin-
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.
Attachment #631139 - Flags: checkin?(netzen) → checkin-
Comment on attachment 631139 [details] [diff] [review]
Patch to be committed (image from clipboard)

Dependent patch just landed :-)
Attachment #631139 - Flags: review?(netzen)
Attachment #631139 - Flags: review?(netzen)
Attachment #631139 - Flags: review+
Attachment #631139 - Flags: checkin-
Attachment #631139 - Flags: checkin+
Attachment #630256 - Flags: checkin- → checkin+
http://hg.mozilla.org/integration/mozilla-inbound/rev/0a570be19457
http://hg.mozilla.org/integration/mozilla-inbound/rev/31f42dd24f25
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
Attachment #631139 - Flags: checkin+ → checkin-
(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.
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
https://hg.mozilla.org/mozilla-central/rev/0a570be19457
https://hg.mozilla.org/mozilla-central/rev/4fa21b477540
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Depends on: 787741

Updated

5 years ago
Depends on: 787769

Updated

5 years ago
Depends on: 816530

Comment 39

5 years ago
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?
(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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Duplicate of this bug: 611977

Updated

4 years ago
Assignee: ejpbruel → nobody

Comment 42

3 years ago
Is progress likely to be made on this issue?

It seems to have stalled since #816530.
You need to log in before you can comment on or make changes to this bug.