Regression range: Chromium 8100d4b5a4f5b6f4a6957acd57a1b167022c5..b92be59283c74a119ce3d003f25eb945deb

https://chromium.googlesource.com/chromium/src/+log/8100d4b5a4f5b6f4a6957acd57a1b167022c5..b92be59283c74a119ce3d003f25eb945deb


Local repro gn args:

is_component_build = true
symbol_level = 0
use_goma = true

ninja -C out\release -j800 video_decode_accelerator_unittest.exe

It regressed here: https://chromium.googlesource.com/chromium/src/+/7883e6b920a48029194e382dfb6ed5f7f5b4390c

Reverting: https://chromium-review.googlesource.com/c/chromium/src/+/1349981

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

commit 8ceea004146d58340264b0cef7e1976e94208e6f
Author: Hans Wennborg <hans@chromium.org>
Date: Mon Nov 26 11:32:34 2018

Revert "media/gpu/test: Cleanup and split test helpers build target."

This reverts commit 7883e6b920a48029194e382dfb6ed5f7f5b4390c.

Reason for revert:
video_decode_accelerator_unittest fails to link in Windows component builds

Original change's description:
> media/gpu/test: Cleanup and split test helpers build target.
> 
> This change splits up the test helpers target in separate encode, decode and
> other helpers. Tests now only depend on what they actually need. This change
> doesn't introduce any new code, but some things are moved to avoid weird
> dependancies (such as encode helpers depending on frame mapper which depends on
> decode helpers).
> 
> This change also moves code in the video_accelerator_unittest_helpers.h file
> from the media to the media/test namespace.
> 
> TEST=ran video/jpeg encode/decode tests on nocturne
> 
> CQ-DEPEND=CL:1345670
> 
> BUG=879065
> 
> Change-Id: I17704399c5724cbacc5611578e0dec22191e7a50
> Reviewed-on: https://chromium-review.googlesource.com/c/1345701
> Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> Commit-Queue: David Staessens <dstaessens@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#610757}

TBR=hiroh@chromium.org,acourbot@chromium.org,dstaessens@chromium.org

Change-Id: If341a83edf8b0ad74180895860f9ebfbb396e5f6
No-Presubmit: true
No-Tree-Checks: true
Bug: 879065,  908360 
Reviewed-on: https://chromium-review.googlesource.com/c/1349981
Reviewed-by: Hans Wennborg <hans@chromium.org>
Commit-Queue: Hans Wennborg <hans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610815}
[modify] https://crrev.com/8ceea004146d58340264b0cef7e1976e94208e6f/media/gpu/BUILD.gn
[modify] https://crrev.com/8ceea004146d58340264b0cef7e1976e94208e6f/media/gpu/jpeg_decode_accelerator_unittest.cc
[modify] https://crrev.com/8ceea004146d58340264b0cef7e1976e94208e6f/media/gpu/jpeg_encode_accelerator_unittest.cc
[modify] https://crrev.com/8ceea004146d58340264b0cef7e1976e94208e6f/media/gpu/test/BUILD.gn
[modify] https://crrev.com/8ceea004146d58340264b0cef7e1976e94208e6f/media/gpu/test/rendering_helper.cc
[modify] https://crrev.com/8ceea004146d58340264b0cef7e1976e94208e6f/media/gpu/test/video_accelerator_unittest_helpers.h
[modify] https://crrev.com/8ceea004146d58340264b0cef7e1976e94208e6f/media/gpu/test/video_decode_accelerator_unittest_helpers.cc
[modify] https://crrev.com/8ceea004146d58340264b0cef7e1976e94208e6f/media/gpu/test/video_frame_validator.cc
[modify] https://crrev.com/8ceea004146d58340264b0cef7e1976e94208e6f/media/gpu/test/video_frame_validator.h
[modify] https://crrev.com/8ceea004146d58340264b0cef7e1976e94208e6f/media/gpu/video_decode_accelerator_unittest.cc
[modify] https://crrev.com/8ceea004146d58340264b0cef7e1976e94208e6f/media/gpu/video_encode_accelerator_unittest.cc

Apologies for breaking the build. Unfortunately the windows-related tryjobs didn't catch the issue (win10_chromium_x64_rel_ng, win7_chromium_rel_ng, win_chromium_compile_dbg_ng, win_optional_gpu_tests_rel).

I tried adding more win_* tryjobs, but none of them seem to trigger the failure (expect for win_dbg and win_x64_dbg which time out).

I created a fix (https://crrev.com/c/1351193), and would like to have a way to properly validate it. Do you have any ideas? Thanks!

Maybe the trybots don't do component builds, or they don't build video_decode_accelerator_unittest.

The easiest is of course to try building locally on a Windows box using the repro steps from #1.


If you don't have access to a Windows machine, you can also reproduce with the Linux-Windows cross-build: https://chromium.googlesource.com/chromium/src/+/HEAD/docs/win_cross.md
That's still somewhat experimental, but it works for this case:

At Chromium 3f779cc2475766a543a7a65dcab57d447522542f:

$ cat out/release/args.gn
is_component_build = true
symbol_level = 0
target_os = "win"
$ ninja -C out/release video_decode_accelerator_unittest.exe
[...]
lld-link: error: undefined symbol: NV12ToI420
[...]

Syncing to head (8730008ca25fa53b9d8154b5ec363980cfe67e03), applying your patch and trying again:

$ git checkout origin/master -b test_patch && gclient sync
$ git cl patch https://chromium-review.googlesource.com/c/chromium/src/+/1351193
$ ninja -C out/release video_decode_accelerator_unittest.exe
(builds successfully)

So the patch seems to work :-)

Thanks for verifying Hans!

I don't have a windows machine available currently, and haven't gotten cros-compiling to work yet. I can give it another try.

Maybe we should think about adding windows component builds to the trybots. It happened twice this week already that my change got reverted due to some windows builds failing, without the trybots catching it.

Did you take any other steps to get the windows build working on Linux? When I try to build I get: goma does not yet work in win cross builds, b/64390790. Thanks!

> When I try to build I get: goma does not yet work in win cross builds,

I didn't use goma, as shown by the gn args in #5.

We do do component builds on the CQ, for example this try job from your CL: https://ci.chromium.org/p/chromium/builders/luci.chromium.try/win_chromium_compile_dbg_ng/170198

The problem seems to be that it's not building the video_decode_accelerator_unittests target.

Thanks Hans! I tried different flags but always seem to be hitting different platform-specific asserts. Some of my gn arguments seem to refer chromeos/linux, so I guess something went wrong while generating these.

Did you build from inside the 'chrome-sdk' shell? It requires a --BOARD parameter, not sure what the value should be. What command did you use to generate your gn arguments? Thanks!

> Did you build from inside the 'chrome-sdk' shell? It requires a --BOARD parameter, not sure what the value should be. What command did you use to generate your gn arguments? Thanks!

No, just a regular Linux terminal. I ran "gn args" and put those arguments in the file.

Thanks for the info, I'll give it another try the next time I touch build files.