1396587 - Colors of pasted image differ from original image (incorrect TIFF -> PNG/JPG conversion?)

Status: NEW

(Core :: Widget: Cocoa, defect, P3)

Description

[Rob Wu [:robwu]]

Attachment: red.html

When I paste a red pixel with RGB value #FF0000, and paste the result in Firefox, then the pasted image does not have color value #FF0000.
I reduced the problem to a fragment in nsClipboard::TransferableFromPasteboard [1], where the raw image on the clipboard (TIFF format) is converted to the target format (PNG, JPEG or GIF).


STR:
1. Open red.html in Firefox. It has a magnified 1x1 red pixel. Right-click on the image, and choose "Copy image".
   (or, open an image editor such as GIMP, fill the canvas with red and copy it).
2. Focus the editable area and paste.
3. The test case will read the color value of the pixel and show it.

Expected:
Pasted <img>           rgba(255, 0, 0, 255)  <-- HTML flavor on the clipboard
clipboardData.files[0] rgba(252, 0, 0, 255)  <-- Image data

Actual (copied from Firefox):
Pasted <img>           rgba(255, 0, 0, 255)
clipboardData.files[0] rgba(252, 13, 27, 255)

Actual (copied from GIMP):
Pasted <img>           rgba(252, 13, 27, 255)
clipboardData.files[0] rgba(252, 13, 27, 255)
(since GIMP does not put HTML on the clipboard, the "Pasted <img>" HTML is generated from the image data)



red.html only tests PNG (because Firefox pastes as PNG), to directly test other image MIME types, use the following code snippet from the global JS console (with Chrome debugging enabled):

 mime = 'image/png'; // or image/gif or image/jpeg
 var t = Components.classes['@mozilla.org/widget/transferable;1'].createInstance(Components.interfaces.nsITransferable);
 t.addDataFlavor(mime);
 Services.clipboard.getData(t, 1);
 var container = {};
 t.getTransferData(mime, container, {});
 var imgData = Uint8Array.from(container.value.data, a=>a.charCodeAt(0));

 var i = new Image();
 i.src = URL.createObjectURL(new Blob([imgData], {type: mime}));
 i.onerror = () => { console.log('failed to load'); };
 i.onload = () => {
  var c = document.createElementNS('http://www.w3.org/1999/xhtml', 'canvas');
  var ctx = c.getContext('2d');
  ctx.drawImage(i, 0, 0);
  console.log(ctx.getImageData(0, 0, 1, 1).data); // prints [r, g, b, a]
 };

https://searchfox.org/mozilla-central/rev/999385a5e8c2d360cc37286882508075fc2078bd/widget/cocoa/nsClipboard.mm#314-333

Comment 1

[Rob Wu [:robwu]]

Correction, the actual color is:

clipboardData.files[0] rgba(255, 42, 26, 255)

(this result is still wrong, because "42, 26" should be "0, 0")
(I got the originally reported color result (252, 13, 27) in the OP after replacing kCGColorSpaceGenericRGB with kCGColorSpaceSRGB in https://searchfox.org/mozilla-central/rev/999385a5e8c2d360cc37286882508075fc2078bd/widget/cocoa/nsCocoaUtils.mm#400)

(this was tested on Firefox stable (54) and Firefox Nightly (57, built just now)).

Comment 2

[Stephen A Pohl [:spohl] (OOO until 3/20)]

Could you use mozregression[1] to determine if this is a regression? We made some changes to drag & drop on OSX that may have caused this. Thanks!

[1] http://mozilla.github.io/mozregression/

Comment 3

[Rob Wu [:robwu]]

This is not a recent regression.
I tried Firefox 50 with the red.html test case, and it is already broken.

I also tried Firefox 10.0a1 (2011-11-08) by running the following snippet from the global JS console (Cmd+Shift+J) after copying the red image from red.html, and I see a dialog with colors "255,42,26,255" on macOS 10.11.6, while it should have been "255,0,0,255".

 mime = 'image/png';
 var t = Components.classes['@mozilla.org/widget/transferable;1'].createInstance(Components.interfaces.nsITransferable);
 t.addDataFlavor(mime);
 Components.classes['@mozilla.org/widget/clipboard;1'].getService(Components.interfaces.nsIClipboard).getData(t, 1);
 var container = {};
 t.getTransferData(mime, container, {});

 var i = new Image();
 i.src = 'data:image/png;base64,' + btoa(container.value.data);
 setTimeout(function() {
  var c = document.createElementNS('http://www.w3.org/1999/xhtml', 'canvas');
  var ctx = c.getContext('2d');
  ctx.drawImage(i, 0, 0);
  alert([].slice.call(ctx.getImageData(0, 0, 1, 1).data));
 });

Comment 4

[Tom S. (please needinfo tschuster)]

I have a patch that fixes part of this problem. On macOS in the same-process case we use a cached transferable instead of reading the native clipboard. This actually causes problems if that transferable contains an image, because when we put something into the clipboard it's an imgIContainer, but when reading it *out* of the clipboard we expect an nsIInputStream. (At least various callers expect this, see the lengthy comment in ext-clipboard.js). This file also describes the hack that we are currently using: We use a zero data length for the transferable, to force reading from the native clipboard ... This hack will stop working with my planned change of removing the length from nsITransferables completely (bug 1493292).

So finally to fix this problem, I propose we encode the imgIContainer to an nsIInputStream in the nsClipboard::GetNativeClipboardData method: https://hg.mozilla.org/try/rev/a3a1ca9c2641a9b39c10d69b2f22325efe87e7cd
This also seems to fix the test failure in toolkit/components/extensions/test/mochitest/test_ext_clipboard_image.html. 

Obviously there is still going to be an issue when we do go through the native clipboard, apparently that TIFF conversion is lossy. I don't have a solution for that.

Comment 5

[Rob Wu [:robwu]]

In ext-clipboard.js I documented the state of the internals at the time that the feature was implemented.

If you intend to land the proposed change, could you also edit the tail of the comment in ext-clipboard.js [1] so that it is up-to-date?

(In reply to Tom Schuster [:evilpie] from comment #4)
> This also seems to fix the test failure in
> toolkit/components/extensions/test/mochitest/test_ext_clipboard_image.html. 

Which failure? Are you referring to the special hack at [2]? If so, please remove the hack too :)


[1] https://searchfox.org/mozilla-central/rev/28fe656de6a022d1fc01bbd3811023fca99cf223/toolkit/components/extensions/parent/ext-clipboard.js#48-66
[2] https://searchfox.org/mozilla-central/rev/28fe656de6a022d1fc01bbd3811023fca99cf223/toolkit/components/extensions/test/mochitest/test_ext_clipboard_image.html#48-57

Comment 6

[Tom S. (please needinfo tschuster)]

(In reply to Rob Wu [:robwu] from comment #5)
> In ext-clipboard.js I documented the state of the internals at the time that
> the feature was implemented.
> 
> If you intend to land the proposed change, could you also edit the tail of
> the comment in ext-clipboard.js [1] so that it is up-to-date?
> 
Yes, I actually already did this locally after pushing to try.
> (In reply to Tom Schuster [:evilpie] from comment #4)
> > This also seems to fix the test failure in
> > toolkit/components/extensions/test/mochitest/test_ext_clipboard_image.html. 
> 
> Which failure? Are you referring to the special hack at [2]? If so, please
> remove the hack too :)
> 
Yeah, I was referring to the adjusted test result for macOS. Looking at try server result, this is now correct: https://treeherder.mozilla.org/#/jobs?repo=try&revision=856606e555f88db2ce2791161f1fd774b9270402&selectedJob=205066950

Comment 7

[Tom S. (please needinfo tschuster)]

Maybe I need a slightly different approach: https://treeherder.mozilla.org/#/jobs?repo=try&revision=335a7cd0b9464d0d0fee223bd89ef8da864146f9&selectedJob=205274291. 

If we put an image/png into the clipbooard and try to read out and image/jpeg, we will still fallback to native clipboard, so I might need to handle that case as well.

Comment 8

[Tom S. (please needinfo tschuster)]

Attachment: Bug 1396587 - Convert cached imgIContainer in clipboard to nsIInputStream on MacOS. r?spohl,robwu

Comment 9

[Tom S. (please needinfo tschuster)]

I could actually go for a much simpler fix: Always fallback to the native clipboard when trying to retrieve an image flavor. This will of course not fix the wrong color problem, even in the same-process as this patch did. 

I am also wondering if maybe the clipboard code should paste/encode images as PNG instead of TIFF when writing to the native clipboard? I have no knowledge of mac code, so maybe other applications would no work with that, no idea.

Comment 10

[Stephen A Pohl [:spohl] (OOO until 3/20)]

Answered in phabricator.

Comment 11

[Tom S. (please needinfo tschuster)]

I still think this approach has value, because it improves the situation when copying images inside Firefox. However bug 1500206 should be enough for my purposes.

Comment 12

[skeddles]

This bug still exists and it's a huge problem for me, as my users expect colors to be kept exact. It's also part of a larger problem of firefox displaying PNGs incorrectly when a color profile is present (though I can at least use PNGCRUSH to remove them). But I'm afraid because of this bug I have to disable uploading images via pasting in firefox, which is very disappointing since the functionality has been around for years and users have come to expect it.

I don't believe it has anything to do with filetype conversion as it happens when copying from an image editor such as paint or photoshop, and I believe windows stores that as bitmap (which also means it's probably not related to color profiles like the displaying PNGs bug).

Comment 13

[skeddles]

Yup. Still struggling with this. Trying to make a game where users can upload an image to create a level, based on the color of the pixels. Unsurprisingly, it's broken in firefox because firefox says a #ff0000 pixel is #fe0000 (etc).

Really sad that I still have to tell people they can't use my site unless they use Chrome (or pngcrush all their images first).

(should be noted it's not just copy-pasted images, uploading an image with a filereader causes this too).

Comment 14

[Josh Aas]

Attachment: Bug 1396587 - Fix clipboard colorspace issues on macOS, also fix screenshot testing. r=mstange,extension-reviewers,robwu

Comment 15

[Josh Aas]

I believe the patch I just uploaded fixes this issue. The attached test case fails without the patch, succeeds with it.

The patch does three things:

1. nsCocoaUtils::CreateCGImageFromSurface was using kCGColorSpaceGenericRGB, a legacy generic RGB colorspace. Firefox's pixel buffers are sRGB. Any color-managed consumer of clipboard data from Firefox would interpret the pixel values through the wrong profile, producing shifted colors.
2. nsCocoaUtils::CreateNSImageFromCGImage was making roughly the same mistake, but using NSDeviceRGBColorSpace. The code there could be simplified a lot by just using [[NSBitmapImageRep alloc] initWithCGImage:aInputImage].
3. Prefer NSPasteboardTypePNG over NSPasteboardTypeTIFF from the clipboard. PNG is more likely to just be sRGB up and if the output is PNG we can avoid any TIFF->PNG conversion.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/105fa1793bae
https://hg.mozilla.org/integration/autoland/rev/24f86fe0ccce
Fix colorspace issues on macOS, particularly for copy and paste. r=mstange,extension-reviewers,robwu

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/fa8f4e5b6f41
https://hg.mozilla.org/integration/autoland/rev/a434d1674662
Revert "Bug 1396587 - Fix colorspace issues on macOS, particularly for copy and paste. r=mstange,extension-reviewers,robwu" for causing failures at browser_screenshots_test_visible.js.

Comment 18

[Atila Butkovits]

Backed out browser_screenshots_test_visible.js.

Backout link: https://hg.mozilla.org/integration/autoland/rev/a434d1674662

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=24f86fe0cccec6b3507e5584751eed0910421fc3&selectedTaskRun=LzOQHepSQza3CRMdcHZURg.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=553631293&repo=autoland&task=LzOQHepSQza3CRMdcHZURg.0&lineNumber=3236