Closed Bug 1520656 Opened 10 months ago Closed 9 months ago

Images uploaded from the Windows clipboard have discolored pixels in the bottom-left corner

Categories

(Core :: ImageLib, defect, P3)

65 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla67
Tracking Status
firefox-esr60 --- unaffected
firefox65 --- wontfix
firefox66 --- fixed
firefox67 --- verified

People

(Reporter: vaindil, Assigned: aosmond)

References

Details

(Keywords: regression)

Attachments

(7 files)

Attached file clipboard.html

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:65.0) Gecko/20100101 Firefox/65.0

Steps to reproduce:

  • Open this page: https://vaindil.com/clipboard (.html file also attached)
  • Use Snipping Tool or Win+Shift+S to take a screenshot on Windows
  • Use Ctrl+V on the page above to paste it to the website

Actual results:

The image is pasted with discolored pixels in the bottom-left corner.

Expected results:

The bottom-left pixels should not be discolored.

Sorry, probably should've just used the attachment. The page on my site may be taken down at some point.

Summary: Images uploaded from the Windows clipboard have a discolored pixel in the bottom-left corner → Images uploaded from the Windows clipboard have discolored pixels in the bottom-left corner

Hi,

I'm not able to reproduce this issue on Firefox 64.0.2 / Firefox 65.0b12 / Firefox 66.0a1.

Could you test if the issue occurs for you in safe-mode, here is a link that can help you with that:
https://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-mode

If the issue still occurs, please try using a new profile and test it again, you can find the steps to do that here:
https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles?redirectlocale=en-US&redirectslug=Managing-profiles#w_starting-the-profile-manager

If it does still reproduce please attach a screenshot of the bug you've encountered.

Thank you !

Flags: needinfo?(vaindil)

I think I was able to narrow it down, but I don't have enough people with the requisite hardware/software to verify for certain.

It only happens on v65, not on v64 (haven't tested v66). It only seems to happen to people with AMD hardware. I don't know anyone with an AMD CPU but a different GPU (or vice versa) so I can't test which of the two causes it. It happens on a brand new profile, and it happens regardless of whether hardware acceleration is enabled.

I attached two screenshots. I took a small screenshot of the original .html page and pasted it onto the same page, so the colors are the same. You can see in the first screenshot that there's a small area of discoloration in the bottom-left corner of the pasted image. The second screenshot is zoomed in to 300%, you can see the individual pixels that are discolored.

Flags: needinfo?(vaindil)
Attached image bugdemo.png

I can reproduce this on Win10 (Intel x32, integrated gfx) and Win7 (Intel x64, AMD gfx) in Nightly 66a1 and DevEd 65b3, but not Release 64. I've included an image showing the result with a test image.

Affected:

  • Screenshot
  • Snipping Tool
  • 'Copy Image' from Chrome

Not affected (displays correctly):

  • Loading an Image from a file
  • 'Copy Image' from Firefox (66,65 and 64), IE11 or Edge

Reproducible on Fx 67.0a1 / Fx 66.0b5 / Fx 65.0 but not on Firefox 64.0

Here is the Regression window I found for this issue:: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ae143b5f65fb4a80538e765541de7f999c909233&tochange=6c28ad7f98a7629f5a1b2f123aaad66f1f6233f0

Last good revision: ae143b5f65fb4a80538e765541de7f999c909233
First bad revision: 6c28ad7f98a7629f5a1b2f123aaad66f1f6233f0

I'm going to set a component to this issue in order to involve the development team in reviewing it.
If this ain't the correct component for it, please feel free to move it to a more appropriate one.

Thanks!

Status: UNCONFIRMED → NEW
Component: Untriaged → Editor
Ever confirmed: true
Product: Firefox → Core

This seems to be regression by 1501482 according to comment #7. aosmond, could you handle this?

Component: Editor → ImageLib
Flags: needinfo?(aosmond)
Assignee: nobody → aosmond
Priority: -- → P3

BMPs decode from bottom to top, left to right, thus the first pixels read are at the bottom left. Those are precisely the pixels which stand out as completely wrong. This suggests we are start reading the first pixel row a few bytes too early, and then everything is shifted as a result. I must have made a mistake adding support for clipboard BMPs to nsBMPDecoder.

Attachment #9043989 - Attachment description: Shows discoloration of the entire left side of the image → Discoloration of the entire left side of the image (1)

Since I posted this I've seen several other discoloration issues. I attached two more images, they were pasted from the clipboard into Discord and I downloaded the actual files. The entire left side of the images seem to have been copied from somewhere else in the image, not sure what the logic is though. (They're from RimWorld in case anyone is curious.)

Makes sense, looks like it is wrapping around. If the image is

A A A A B B B B
C C C C D D D D

and we start processing it 1 pixel too early, it ends up decoding as:

D A A A A B B B
X C C C C D D D

Just trying to find out where the algorithm went wrong now.

It appears that even if the header size includes the bitfield masks (as we assume in the full fledged decoder), the original decoder added some padding to the start of the RGB data:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c28ad7f98a7#l9.336

It seems like it was always written this way, so I can't find an explanation for this behaviour. So let's just try a build which does the same thing and see if it fixes the issue:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=faa2d7c3f28ac4650813e7f4ebd0ef983ecb3621

(In reply to Andrew Osmond [:aosmond] from comment #14)

It appears that even if the header size includes the bitfield masks (as we assume in the full fledged decoder), the original decoder added some padding to the start of the RGB data:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c28ad7f98a7#l9.336

It seems like it was always written this way, so I can't find an explanation for this behaviour. So let's just try a build which does the same thing and see if it fixes the issue:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=faa2d7c3f28ac4650813e7f4ebd0ef983ecb3621

Local testing shows it fixes the problem. Sigh.

Flags: needinfo?(aosmond)

In the original Windows clipboard BMP decoder implementation in
nsImageFromClipboard::ConvertColorBitMap, if the bitmap used bitfields
compression, it always adjusted the offset to the RGB data by 12 bytes.
It did this even for newer BMP header formats which explicitly include
space for the bitfields in their header sizes. This patch updates our
BMP decoder to do the same for clipboard BMPs, since we have observed
pasted BMPs using bitfield compression appearing incorrectly. To the
user this appears as if we read a color mask; completely red, blue,
green pixels at the start of the last row, causing all of the other rows
to start with the last three pixels of the previous row.

Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4a2586e511a
BMPs from the clipboard may include extra padding. r=tnikkel
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Blocks: 1501482
Keywords: regression

Would you be able to confirm it is working for you on the latest nightly? I'd like to request uplift to beta, but need independent confirmation :).

Flags: needinfo?(vaindil)

Sorry for the delay, I started a new job today and didn't get a chance to test until now. I can confirm that the bug is fixed in the latest nightly. Thank you so much!

Status: RESOLVED → VERIFIED
Flags: needinfo?(vaindil)

Comment on attachment 9044210 [details]
Bug 1520656 - BMPs from the clipboard may include extra padding.

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1501482

User impact if declined

Certain types of BMPs will be decoded incorrectly when pasted into Firefox via the clipboard. The leftmost three pixels will actually be the last three pixels from the next row.

Is this code covered by automated tests?

Yes

Has the fix been verified in Nightly?

Yes

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

The patch is modeled after the original clipboard behaviour. While this goes against the general understanding of the BMP format, it has obviously been the correct thing to do in Firefox for over a decade. The decoder changes only impact BMPs from the clipboard, and those are already sort of broken.

String changes made/needed

None

Attachment #9044210 - Flags: approval-mozilla-beta?

(In reply to vaindil from comment #20)

Sorry for the delay, I started a new job today and didn't get a chance to test until now. I can confirm that the bug is fixed in the latest nightly. Thank you so much!

No worries :). Thank you for verifying!

Comment on attachment 9044210 [details]
Bug 1520656 - BMPs from the clipboard may include extra padding.

Fix for regression from 65, verified in Nightly.
OK for uplift for beta 10.

Attachment #9044210 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.