Joe has isolated the cause as the following CL:

commit 1f109d9c516640dda836493b24fd033da64411ff (HEAD, refs/bisect/bad)
Author: Hans Wennborg <hans@chromium.org>
Date:   Fri Apr 13 13:23:26 2018 +0000

    Roll clang 328716:329921.

    Bug:  828582 
    Change-Id: I85864722c577268d9c95a0d9c37cf7197b04e5d3
    Reviewed-on: https://chromium-review.googlesource.com/1012028
    Reviewed-by: Nico Weber <thakis@chromium.org>
    Commit-Queue: Hans Wennborg <hans@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#550608}

Specifically, it looks like the Clank roll is the cause, not the change in build flags.

Tentatively guessing that GPU is the right component, since it only affects Windows, but I'm not sure.

Is it possible to get more concrete repro steps, i.e. what targets to build and run to reproduce the problem?

Also, we landed a new Clang roll yesterday (https://crrrev.com/553415). Can you check whether that fixed the issue?

Adding screenshot for context.

Deleted: cyan.png 123 KB

	cyan.png 123 KB View Download

Issue still repros in today's Canary (68.0.3406.0). Does that answer you question?

> Issue still repros in today's Canary (68.0.3406.0). Does that answer you question?

Looks like 68.0.3406.0 is #553301, and the new roll landed a handful revisions later (#553415).

Build target is Chrome (you'll need an official build--we couldn't repro this on Chromium). Then you'll need Chrome Remote Desktop set up (install the Chrome app from https://chrome.google.com/webstore/detail/chrome-remote-desktop/gbchcmhmhahfdphkhkmpfmihenigjmpp?hl=en). You can find setup instructions at https://support.google.com/chrome/answer/1649523?co=GENIE.Platform%3DDesktop&hl=en.

Thanks; I'll retest tomorrow.

Still repros for me in 68.0.3409.0 (Official Build) canary (64-bit) (cohort: Clang-64)

I haven't been able to reproduce this yet, mainly because I'm traveling and don't have access to all my machines. I can rdp to my Windows machine, but from there I can't chromote to my laptop because it doesn't seem to be on the same network.

Does chormoting really not have any test executables that exercise the round-tripping of video without jumping through all the manual hoops? Or is there some test host one can chromote to?

cc: more clang folks

We don't have a test host (hosts are bound to your account, so it's a bit difficult to set up). However, the problem repros using the "remote assistance" (top) section of the Chromoting app so you don't need a second machine. Open a second window using the the New Window command in the hamburger menu (top left), then click Share in one window to get a code, then click Access in the other window and paste the code. You'll get a strange hall-of-mirrors effect, but it easy to see that the colours are wrong.

Thanks! I didn't realize I can use remote assistance within the same host. This allowed me to reproduce the bug, and I've verified it's due to the clang roll. I'm really curious what lies at the bottom of this.

Let's try to narrow down which file(s) regressed. Since that can be done with compilers on goma it might be quicker than bisecting the compiler itself.

Synced to just before the roll: 8cb55171dddde570257127a05

"good clang": https://commondatastorage.googleapis.com/chromium-browser-clang/Win/clang-328716-2.tgz
"bad clang":  https://commondatastorage.googleapis.com/chromium-browser-clang/Win/clang-329921-1.tgz

Replacing the compiler used:

set PATH=%PATH%;c:\Program Files\7-Zip\
7z x \src\tmp\clang-329921-1.tar -othird_party\llvm-build\Release+Asserts -aoa


Touch some source files:

c:\src\gnuwin\find gpu -name "*.cc" -exec touch {} ;

Rebuild them with the bad compiler:

ninja -C out\release -j1000 chrome


Nope, that's not enough to regress. Let's try some more directories..

gpu/
extensions/
media/
content/ .. yup, this is the one.

content/renderer/ seems sufficient to repro..

content\renderer\gpu.. nope
content\renderer\media.. nope
content\renderer\pepper.. repros

content\renderer\pepper\pepper_video_decoder_host.cc.. nope
content\renderer\pepper\pepper_video_encoder_host.cc.. nope
content\renderer\pepper\pepper_video_capture_host.cc.. nope
touch content\renderer\pepper\pepper_graphics_2d_host.cc.. nope
touch content\renderer\pepper\*video*.cc repros

how many of those are there..

04/28/2018  02:08 AM            19,866 pepper_media_stream_video_track_host.cc
04/28/2018  02:08 AM             5,252 pepper_platform_video_capture.cc
04/28/2018  02:08 AM            14,101 pepper_video_capture_host.cc
04/28/2018  02:08 AM            20,215 pepper_video_decoder_host.cc
04/28/2018  02:08 AM             3,828 pepper_video_destination_host.cc
04/28/2018  02:08 AM            19,672 pepper_video_encoder_host.cc
04/28/2018  02:08 AM            12,010 pepper_video_source_host.cc
04/28/2018  02:08 AM            10,651 ppb_video_decoder_impl.cc
04/28/2018  02:08 AM            36,606 video_decoder_shim.cc
04/28/2018  02:08 AM            17,163 video_encoder_shim.cc

Excluding the ones we already tried..

pepper_media_stream_video_track_host.cc
pepper_platform_video_capture.cc
pepper_video_destination_host.cc
pepper_video_source_host.cc
ppb_video_decoder_impl.cc
video_decoder_shim.cc
video_encoder_shim.cc

Confirmed that just compiling video_decoder_shim.cc with the "bad" clang is enough to repro.

Attaching the object file build with the "good" and "bad" compiler version.

Deleted: video_decoder_shim.obj.bad 957 KB

video_decoder_shim.obj.bad 957 KB Download

Deleted: video_decoder_shim.obj.good 957 KB

video_decoder_shim.obj.good 957 KB Download

Preprocessed source and cc1 invocation.

Deleted: a.ii 12.2 MB

a.ii 12.2 MB Download

Deleted: invocation.txt 7.9 KB

invocation.txt 7.9 KB View Download

Shorter invocation:

build.release/bin/clang -cc1 "-cc1" "-triple" "x86_64-pc-windows-msvc19.11.0" "-emit-obj" "-mrelocation-model" "pic" "-pic-level" "2" "-mthread-model" "posix" "-relaxed-aliasing" "-fmath-errno" "-masm-verbose" "-mconstructor-aliases" "-munwind-tables" "-target-cpu" "x86-64" "-mllvm" "-x86-asm-syntax=intel" "-flto-visibility-public-std" "-fno-rtti-data" "-stack-protector" "2" "-gcodeview" "-fms-volatile" "-gcodeview" "-debug-info-kind=limited" "-debugger-tuning=gdb" "-momit-leaf-frame-pointer" "-ffunction-sections" "-fdata-sections" "-nostdsysteminc" "-Os" "-fdeprecated-macro" "-fno-use-cxa-atexit" "-fms-extensions" "-fms-compatibility" "-fms-compatibility-version=19.11" "-std=c++14" "-fdelayed-template-parsing" "-finline-functions" "-fseh-exceptions" "-vectorize-loops" "-vectorize-slp" "-mllvm" "-instcombine-lower-dbg-declare=0" "-o" "/tmp/a.o" "-x" "c++" /tmp/a.ii

Looking at the asm diff from #19, the biggest difference is in ?Convert@YUVConverter@VideoDecoderShim@content@@QEAAXAEBV?$scoped_refptr@VVideoFrame@media@@@@I@Z

It looks like the float arrays are getting copied onto the stack for some reason. I'm currently bisecting to see where that started happening. Maybe it's related to the -fmerge-all-constants change.

If it's not related it would be good to exclude it because it makes the diff hard to read.

Bisection over when we start copying things around indeed points to "r329300 - Disable -fmerge-all-constants as default"

Nico made us pass that explicitly in https://chromium-review.googlesource.com/c/chromium/src/+/999323

Is the flag not working?

Oh it does, we only added the flag for non-Windows, oops. Let's see if setting the flag fixes chromoting or just reduces the diff..

Okay, I think we have an understand of this now.

VideoDecoderShim::YUVConverter::Convert defines some const float arrays.

I'm not familiar with the code, but the arrays end up getting passed to a gl_ object:

  gl_->UniformMatrix3fv(yuv_matrix_loc_, 1, 0, yuv_matrix);
  gl_->Uniform3fv(yuv_adjust_loc_, 1, yuv_adjust);

which probably ends up using the arrays after we've returned from the Convert() function, or something similar.

Before the Clang roll, this worked because -fmerge-all-constants was on by default and would make the code refer to a global array of float values instead of one on the stack as required by the standard (to give each function-local object a unique address).

We will turn on -fmerge-all-constants also on Windows, and also fix the code to declare those arrays static.

> which probably ends up using the arrays after we've returned from the Convert() function, or something similar.

It's not using the arrays after returning, just after the scope in which they were defined has ended, i.e. after the objects' lifetime, which means Clang may have re-used (part of) their stack slots by then.

Fixing the code here:
https://chromium-review.googlesource.com/c/chromium/src/+/1035323

And enabling -fmerge-all-constants for Windows here:
https://chromium-review.googlesource.com/c/chromium/src/+/1035265


MSVC does not merge constants by default, but it also doesn't track object lifetimes based on scopes within functions, which is why this problem didn't show.

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

commit 3e568aae1d572c4b373f40e0826de7ea0a39d3c0
Author: Hans Wennborg <hans@chromium.org>
Date: Mon Apr 30 21:24:46 2018

Build with -fmerge-all-constants also on Windows

After Clang r329300 we need to request this explicitly. Make sure to do
so also on Windows.

Bug:  835506 , 829795
Change-Id: I4b47e897ac202eb796d8c5b31808dfae2aeb5b22
Reviewed-on: https://chromium-review.googlesource.com/1035265
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Hans Wennborg <hans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554881}
[modify] https://crrev.com/3e568aae1d572c4b373f40e0826de7ea0a39d3c0/build/config/compiler/BUILD.gn

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

commit 8ce1c8536fe92cb0e48b88dfa4ce530144579181
Author: Hans Wennborg <hans@chromium.org>
Date: Tue May 01 00:23:13 2018

VideoDecoderShim: mark transformation constants static

The constants are used outside the scope they're defined in, which means
unless they're marked 'static', their lifetime has ended and the compiler
is free to reuse their stack slot. Marking them static means they're always
alive, and also removes the need to copy them to the stack.

Re-using the stack slot for some part of these was causing the red channel
to disappear during chromoting when compiling with Clang without
-fmerge-all-constants.

Bug:  835506 
Change-Id: I6bddbf531979fecf78c80c0f96afda8ea0d05e33
Reviewed-on: https://chromium-review.googlesource.com/1035323
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Raymes Khoury <raymes@chromium.org>
Commit-Queue: Raymes Khoury <raymes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554935}
[modify] https://crrev.com/8ce1c8536fe92cb0e48b88dfa4ce530144579181/content/renderer/pepper/video_decoder_shim.cc

Red is back in 68.0.3416.0 (Official Build) canary (64-bit) (cohort: Clang-64)! W00t!

Thanks hans@ for the great work tracking this down.

Remoting folks: if this code had any coverage, asan should've told us about the use after scope, with a nice stack and all. And ideally the test would've tested some color roundtripping, and that would've caught the problem before the compiler roll. Do you want to use this bug for adding test coverage for this code, or do you want to use a new bug for that?

 Issue 838610  has been merged into this issue.

Tested this issue on Windows 10 on the reported version 68.0.3400.0 and unable to reproduce the issue by following the steps given in the original comment.

After entering the Access code to connect to Remote Desktop, unable to reach the Host.  Issue 832272  raised for the same.
Hence we are not able to verify the fix on the latest Canary 68.0.3417.0.
Attached is the screen shot for reference.

hans@ Request you to check and help us in verifying the fix on the latest M-68 68.0.3417.0 build.

Thanks...

Deleted: 835506-error.png 89.4 KB

	835506-error.png 89.4 KB View Download

Greg verified it with 68.0.3416.0 yesterday, and I've just verified with today's canary (68.0.3417.0 (Official Build) canary (64-bit) (cohort: Clang-64)) so I'm confident this is fixed.

Jamie: please file a separate bug to track adding tests for this.

I filed bug 839016 to track adding tests, but I don't know who the right owner for that work is. The remoting team doesn't maintain this code, we just use it (for now--we're switching to a WebRTC-based solution soon).