Thanks for filing the issue!

Able to reproduce the issue on reported chrome version 70.0.3538.77 and on the latest canary 72.0.3603.0 using Windows 10.
Note: Issue isn't seen on Mac 10.13.1 and Ubuntu 14.04

Bisect Information:
-------------------
Good Build: 70.0.3501.0
Bad Build:  70.0.3503.0
Note: Version 70.0.3502.0 isn't available for OS Windows and providing the chromium bisect info as we are getting runtime error while running per-revision bisect.

You are probably looking for a change made after 577887 (known good), but no later than 577894 (first known bad).
CHANGELOG URL:
 https://chromium.googlesource.com/chromium/src/+log/b62bd420664564c0b417521a60a54c35fc3daaa2..f22de573d0df738b0dea0bc408649fd10b4bd35d
Suspecting: https://chromium.googlesource.com/chromium/src/+/c931242dba631dfb1e2b63e84550e5859c07788d
Review URL: https://chromium-review.googlesource.com/1148893

@Justin Novosad: Please help in assigning it to the right owner if this isn't related to your change. Adding RBS as this seems to be a recent regression, please remove/change if not required.

junov -> fserb (sorry :) )

Friendly ping to get an update as it is marked as RB stable.
Thanks..!

Reminder M71 Stable is approaching VERY soon. Please review this bug and assess if this is indeed a RBS. If not, please remove the RBS label. If so, please make sure any planned work will be tested in Beta and verified before the Stable date. Thank you.

Requesting to take a look at M71 blockers ASAP due to upcoming Thanksgiving holidays next week.

This seems to be windows only. We are having a hard time reproing here.
If someone from WebGL could take a look at it, I could help with the source problem, I think.

I think the problem maybe has to do with: 
https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/html/canvas/html_canvas_element.cc?q=html_canvas_element&sq=package:chromium&dr=C&l=791

using kRGBA_8888_SkColorType I wonder if we use ColorParams().GetSkColorType() instead, it would automatically solve this.

But again, I can't try it here and I've been trying to come up with a proper test for it, but this is actually hard to test.

Not reproducible on Mac with Chrome 72.0.3611.0 (Official Build) canary (64-bit). Not sure if the kRGBA_8888 color type is the source of error. Only can makes sense if Pixi uses different types of context (2D vs. gl?) on different platforms. FWIW, the kRGBA_8888 here is borrowed from the original code here: https://chromium-review.googlesource.com/c/chromium/src/+/822910/10/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp#b1143

Pixi does choose webgl or 2d depending on support, iirc.

On Thu, Nov 15, 2018, 7:04 AM zakerinasab via monorail <
monorail+v2.3101781462@chromium.org wrote:

Reminder M71 Stable is approaching VERY soon. Please review this bug and assess if this is indeed a RBS. If not, please remove the RBS label. If so, please make sure any planned work will be tested in Beta and verified before the Stable date. Thank you.

Requesting to take a look at M71 blockers ASAP due to upcoming Thanksgiving holidays next week.

As this is regressed in M70, not a blocker for M71. Pls target fix for M72.

Trying out fserb's suggested fix in #8.

Didn't fix it. I will try to continue looking at this tomorrow.

fserb: some questions.

Firstly, in the code you linked, kRGBA_8888_SkColorType is not actually being used - it's overridden:

        SkImageInfo info =
            SkImageInfo::Make(adjusted_size.Width(), adjusted_size.Height(),
                              kRGBA_8888_SkColorType, kUnpremul_SkAlphaType);
        info = info.makeColorSpace(ColorParams().GetSkColorSpace());
        if (ColorParams().GetSkColorType() != kN32_SkColorType)
          info = info.makeColorType(kRGBA_F16_SkColorType);

I think it's this final kRGBA_F16_SkColorType that's incorrect - that code path does seem to be hit. (And there's no BGRA_F16.) So my current ideas are:
- The old codepath with context_->GetImage(hint) was doing a blit that reordered it to RGBA
- The old codepath set some metadata differently so it would later get correctly converted to BGRA (can't find where though)
- The old codepath wasn't actually using F16 (was RGBA_8888 or something instead)

Can you speculate further on what might be going on?

regression link for reference: https://chromium-review.googlesource.com/c/chromium/src/+/1148893/

The color type is not overwritten in this code. In CanvasColorParams::GetSkColorType() code we always expect kN32_SkColorType returned unless the canvas is wide gamut and the pixel format is set to "float16", in which case kRGBA_F16_SkColorType will be returned. If you put a probe on the line that updates the SkColorType, I believe you won't see that triggered.

Also there is no BGRA_F16 because the issue for RGBA and BGRA happens when using the fast code path on GPUs for 8888 pixel format. We don't have this issue for half float pixels.

Please consider that the bug is not reproducible on Linux or Mac. Therefore, you need to dig a little bit using a Windows build to find the root cause. IMHO this is a corner case not a general issue. I would say the first step is to find out if the context created by Pixi in this test is accelerated or unaccelerated, and the debug the code to see where the pixels get invalid.

I understand that this is only reproducible on Windows; that's why I'm looking at it. I only asked because it was the end of the day for me and I figured that another look before I got back to work on this might accelerate things. Do your respective local teams not have Windows dev set up? Perhaps it would be useful; Windows is, after all, the biggest desktop platform.

Anyway, I'm happy to continue looking at this.

That if() is definitely hit:

  info = info.makeColorSpace(ColorParams().GetSkColorSpace());
  DCHECK_EQ(ColorParams().GetSkColorType(), kN32_SkColorType);
  if (ColorParams().GetSkColorType() != kN32_SkColorType)
    info = info.makeColorType(kRGBA_F16_SkColorType);

[20152:17908:1120/141828.288:FATAL:system_clipboard.cc(176)] Check failed: bitmap.colorType() == kN32_SkColorType (4 vs. 6)

where 4 = kRGBA_8888_SkColorType
and   6 = kBGRA_8888_SkColorType = kN32_SkColorType

So I guess it's not that the kRGBA_F16_SkColorType that's wrong, but the check for whether to set it. I'll continue looking at it.

oh wait, that DCHECK is not the one I put in, and it may have been from a previous run. The one I put in passed.

That DCHECK seems to happen every time now even without any changes, and yet it didn't happen the first time I tested this out (in #14). I have no idea what's causing this yet.

[31632:21716:1120/162337.881:FATAL:system_clipboard.cc(176)] Check failed: bitmap.colorType() == kN32_SkColorType (4 vs. 6)
Backtrace:
	base::debug::StackTrace::StackTrace [0x00007FFA925E3314+36]
	logging::LogMessage::~LogMessage [0x00007FFA9260CBEC+92]
	blink::SystemClipboard::WriteImage [0x00007FFA6A4281C1+313]
	blink::WriteImageNodeToClipboard [0x00007FFA6A79526A+582]
	blink::WebLocalFrameImpl::CopyImageAt [0x00007FFA6A9C4674+292]
	content::RenderFrameImpl::OnCopyImageAt [0x00007FFA7227EC0F+119]
	IPC::MessageT<FrameMsg_CopyImageAt_Meta,std::tuple<int,int>,void>::Dispatch<content::RenderFrameImpl,content::RenderFrameImpl,void,void (__cdecl content::RenderFrameImpl::*)(int,int) __ptr64> [0x00007FFA7227EAD8+132]
	content::RenderFrameImpl::OnMessageReceived [0x00007FFA7227CD75+1769]

Oh, because in #14, applying fserb's patch from #8 prevented that DCHECK from being hit.

Anyway, I have a start of a CL:
https://chromium-review.googlesource.com/c/chromium/src/+/1345076
But I'm pretty certain that it's still wrong in the "ColorParams().GetSkColorType() != kN32_SkColorType" case because SystemClipboard::WriteImage will still want kN32_SkColorType.

I have some ideas about fixing that, but how can I test that code path?

(Actually, can you give me a pointer on how to add automated tests for both code paths? Such tests would have easily caught this bug due to the failing DCHECK.)

Experimented with a patch like this to SystemClipboard::WriteImage:

  if (bitmap.colorType() != kN32_SkColorType) {
    SkBitmap new_bitmap;
    SkImageInfo new_info = bitmap.info().makeColorType(kN32_SkColorType);
    if (!new_bitmap.setInfo(new_info)) return;
    if (!bitmap.readPixels(new_bitmap.pixmap())) return;
    bitmap = std::move(new_bitmap);
  }

converting whatever it receives into N32 - but it doesn't work (bitmap.readPixels fails). However I think this is the right idea.

ah, have to allocate - now it seems to work.

  if (bitmap.colorType() != kN32_SkColorType) {
    SkImageInfo new_info = bitmap.info().makeColorType(kN32_SkColorType);
    SkBitmap new_bitmap;
    new_bitmap.allocPixels(new_info, 0);
    if (!bitmap.readPixels(new_bitmap.pixmap())) return;
    bitmap = std::move(new_bitmap);
  }

Uploaded this as an alternate CL:
https://chromium-review.googlesource.com/c/chromium/src/+/1345809
Not 100% sure N32 is always the "right" clipboard format though.

(... the old DCHECK seems to think N32 is correct though, so seems ok.)

Friendly ping to look into this issue and to provide further update on this issue as it has been marked as a stable blocker.

Thanks!

Friendly ping to look into this issue and to provide further update on this issue as it has been marked as a stable blocker.

Thanks!

I've been working on this, it's almost done.
https://chromium-review.googlesource.com/c/chromium/src/+/1345809

Can someone advise me on whether this should be merged to M71?

(Adding merge-request label for attention even though it hasn't landed yet; apologies.)

This bug requires manual review: We are only 5 days from stable.
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

M71 is going to stable next week, CL listed at #26 is not landed in trunk yet so it is too late to take this merge in. Also this is regressed in M70, pls target fix for M72.

Friendly ping for an update on this issue as it is marked as stable blocker & stable release is coming soon.
Thanks..!

kainino@,
Gentle reminder!
Please land a fix as per C#28.
Thanks..!

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dd9c10161af2dc6f3323061f6d8285d1cba9c378

commit dd9c10161af2dc6f3323061f6d8285d1cba9c378
Author: Kai Ninomiya <kainino@chromium.org>
Date: Mon Dec 10 23:06:35 2018

WriteBitmap: convert pixels to N32 if they're not already

Bug:  902837 
Change-Id: I6d3a78ed9f107775f964d2a3cbc1d652eea4744f
Reviewed-on: https://chromium-review.googlesource.com/c/1345809
Reviewed-by: Stephen White <senorblanco@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Reviewed-by: Mohammad Reza Zakerinasab <zakerinasab@chromium.org>
Commit-Queue: Kai Ninomiya <kainino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615302}
[modify] https://crrev.com/dd9c10161af2dc6f3323061f6d8285d1cba9c378/skia/ext/skia_utils_base.cc
[modify] https://crrev.com/dd9c10161af2dc6f3323061f6d8285d1cba9c378/skia/ext/skia_utils_base.h
[modify] https://crrev.com/dd9c10161af2dc6f3323061f6d8285d1cba9c378/skia/ext/skia_utils_mac.mm
[modify] https://crrev.com/dd9c10161af2dc6f3323061f6d8285d1cba9c378/third_party/blink/renderer/core/clipboard/system_clipboard.cc
[modify] https://crrev.com/dd9c10161af2dc6f3323061f6d8285d1cba9c378/ui/base/clipboard/clipboard_aura.cc
[modify] https://crrev.com/dd9c10161af2dc6f3323061f6d8285d1cba9c378/ui/base/clipboard/clipboard_mac.mm
[modify] https://crrev.com/dd9c10161af2dc6f3323061f6d8285d1cba9c378/ui/base/clipboard/clipboard_test_template.h
[modify] https://crrev.com/dd9c10161af2dc6f3323061f6d8285d1cba9c378/ui/base/clipboard/clipboard_win.cc
[modify] https://crrev.com/dd9c10161af2dc6f3323061f6d8285d1cba9c378/ui/base/mojo/clipboard_client.cc
[modify] https://crrev.com/dd9c10161af2dc6f3323061f6d8285d1cba9c378/ui/base/test/test_clipboard.cc

Requesting merge of #33 to M-72

Your change meets the bar and is auto-approved for M72. Please go ahead and merge the CL to branch 3626 manually. Please contact milestone owner if you have questions.
Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/67aaeec55fdc79a46c083017b22d772d9f3c5a16

commit 67aaeec55fdc79a46c083017b22d772d9f3c5a16
Author: Kai Ninomiya <kainino@chromium.org>
Date: Wed Dec 12 20:12:47 2018

WriteBitmap: convert pixels to N32 if they're not already

Bug:  902837 
Change-Id: I6d3a78ed9f107775f964d2a3cbc1d652eea4744f
Reviewed-on: https://chromium-review.googlesource.com/c/1345809
Reviewed-by: Stephen White <senorblanco@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Reviewed-by: Mohammad Reza Zakerinasab <zakerinasab@chromium.org>
Commit-Queue: Kai Ninomiya <kainino@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#615302}(cherry picked from commit dd9c10161af2dc6f3323061f6d8285d1cba9c378)
Reviewed-on: https://chromium-review.googlesource.com/c/1374509
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#299}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/67aaeec55fdc79a46c083017b22d772d9f3c5a16/skia/ext/skia_utils_base.cc
[modify] https://crrev.com/67aaeec55fdc79a46c083017b22d772d9f3c5a16/skia/ext/skia_utils_base.h
[modify] https://crrev.com/67aaeec55fdc79a46c083017b22d772d9f3c5a16/skia/ext/skia_utils_mac.mm
[modify] https://crrev.com/67aaeec55fdc79a46c083017b22d772d9f3c5a16/third_party/blink/renderer/core/clipboard/system_clipboard.cc
[modify] https://crrev.com/67aaeec55fdc79a46c083017b22d772d9f3c5a16/ui/base/clipboard/clipboard_aura.cc
[modify] https://crrev.com/67aaeec55fdc79a46c083017b22d772d9f3c5a16/ui/base/clipboard/clipboard_mac.mm
[modify] https://crrev.com/67aaeec55fdc79a46c083017b22d772d9f3c5a16/ui/base/clipboard/clipboard_test_template.h
[modify] https://crrev.com/67aaeec55fdc79a46c083017b22d772d9f3c5a16/ui/base/clipboard/clipboard_win.cc
[modify] https://crrev.com/67aaeec55fdc79a46c083017b22d772d9f3c5a16/ui/base/mojo/clipboard_client.cc
[modify] https://crrev.com/67aaeec55fdc79a46c083017b22d772d9f3c5a16/ui/base/test/test_clipboard.cc

The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/67aaeec55fdc79a46c083017b22d772d9f3c5a16

Commit: 67aaeec55fdc79a46c083017b22d772d9f3c5a16
Author: kainino@chromium.org
Commiter: kainino@chromium.org
Date: 2018-12-12 20:12:47 +0000 UTC

WriteBitmap: convert pixels to N32 if they're not already

Bug:  902837 
Change-Id: I6d3a78ed9f107775f964d2a3cbc1d652eea4744f
Reviewed-on: https://chromium-review.googlesource.com/c/1345809
Reviewed-by: Stephen White <senorblanco@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Reviewed-by: Mohammad Reza Zakerinasab <zakerinasab@chromium.org>
Commit-Queue: Kai Ninomiya <kainino@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#615302}(cherry picked from commit dd9c10161af2dc6f3323061f6d8285d1cba9c378)
Reviewed-on: https://chromium-review.googlesource.com/c/1374509
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#299}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}