New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 766721 link

Starred by 5 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-11-13
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 3
Type: Task

Blocking:
issue 787983



Sign in to add a comment

Consider switching from yasm to nasm; it's better maintained and 80% slower

Project Member Reported by dalecur...@chromium.org, Sep 19 2017

Issue description

A recent build of ffmpeg takes ~minutes with yasm and ~seconds with nasm. We should look into switching Chromium over to save on build times. Additionally, the last stable release of yasm was ~3 years ago, while nasm is under active development. 

http://www.nasm.us/
http://yasm.tortall.net/

One caveat is that assembly syntax varies slightly; so some minor modifications to the assembly code will be necessary in some cases.

TODO(dalecurtis): Add exact build time numbers and syntax example.
 
Cc: jzern@chromium.org
Hmm, so the standalone ffmpeg build is a bad example because we don't use the same configuration settings as a full ffmpeg build. If we ever started building the ffvp9 or ffhevc assembly (no plans to do so), it would be a big win. However, for our more limited ffmpeg assembly code, the differences are pretty minor and yasm is much faster when compiling many small files:

ffmpeg_yasm target w/ nasm:
real  0m3.710s
user  0m31.524s
sys   0m0.776s

ffmpeg_yasm target w/ yasm:
real  0m2.144s
user  0m14.740s
sys   0m0.708s

I can't test the libvpx targets since they require some work to use nasm. I think later version of libvpx do support nasm, but we don't have such copies inside our tree. +jzern who might be able to comment on nasm vs yasm in libvpx.

Notably trying to build upstream libvpx with nasm fails, ./configure --as=nasm:
vpx_dsp/x86/add_noise_sse2.asm:29: error: invalid combination of opcode and operands
vpx_dsp/x86/add_noise_sse2.asm:35: error: invalid combination of opcode and operands
make[1]: *** [vpx_dsp/x86/add_noise_sse2.asm.o] Error 1
make[1]: *** Waiting for unfinished jobs....
    [AS] vpx_dsp/x86/deblock_sse2.asm.o
vpx_dsp/x86/intrapred_sse2.asm:64: error: invalid combination of opcode and operands
third_party/x86inc/x86inc.asm:1383: ... from macro `movd' defined here
third_party/x86inc/x86inc.asm:1283: ... from macro `RUN_AVX_INSTR' defined here
make[1]: *** [vpx_dsp/x86/intrapred_sse2.asm.o] Error 1
make: *** [.DEFAULT] Error 2

So this is probably WontFix until there are stronger benefits.

Comment 2 by jzern@chromium.org, Sep 20 2017

libvpx should build with nasm, so it's worth working through this. I've filed a bug for the breakage [1] and another one to add a nasm build to our test set.

[1] https://bugs.chromium.org/p/webm/issues/detail?id=1462
jzern: Any commentary on the expected speed / reasons libvpx added support?

Comment 4 by jzern@chromium.org, Sep 20 2017

I have no data on that. It was claimed to be worse when it landed in 2010. Looks like it was done in part for compatibility.

commit 7be093ea4d50c8d38438f88cb9fa817c1c9de8dd
Author: Jan Kratochvil <jan.kratochvil@redhat.com>
Date:   Tue Oct 5 19:15:08 2010 +0200

    nasm: add configure support

    yasm has to be preferred as currently nasm produces marginally less
    efficient code (longer opcodes). Filed for nasm as:
    https://sourceforge.net/tracker/?func=detail&atid=106208&aid=3037462&group_id=6208

    OTOH package should be built always the same, no matter which additional
    packages are / are not present on the system. As the package should be
    built with nasm (as yasm may not be available) we should not use yasm
    even if it is possibly available.

    nasm >= approx. 2.09 is required for the nasm compilation as the former
    versions had a section alignment bug.

    Provide nasm compatibility. No binary change by this patch with yasm on
    {x86_64,i686}-fedora13-linux-gnu. Few longer opcodes with nasm on
    {x86_64,i686}-fedora13-linux-gnu have been checked as safe.

Comment 5 by jzern@chromium.org, Oct 4 2017

Spelled the chrome bug wrong, this should be working now:

commit bc4bc9b6223df26d34f5576da32fa412f01d865e
Author: Scott LaVarnway <slavarnway@google.com>
Date:   Sat Sep 30 05:51:24 2017 -0700

    vpx: fix nasm build errors
    
    BUG= webm:1462 ,766721
    
    Change-Id: Icfa536a8e38623636b96c396e3c94889bfde7a98
Blocking: 495670
Blocking: 787983
Blocking: -495670

Comment 9 by jzern@chromium.org, May 21 2018

Cc: johannkoenig@chromium.org
There were a few more minor issues with the libvpx nasm build but they were resolved in December and we added a per-commit build to ensure it does not regress. The chromium version of libvpx includes all of these changes.

Assembly is strongly discouraged in libaom but we have tests that build what does exist with nasm there as well. I just double-checked the branch we have checked in to chromium currently and it builds with nasm.
Oops, the libaom branch in chromium does *not* compile for Windows. That will be addressed when we roll up to HEAD, hopefully around the end of May.
Cc: davidben@chromium.org
Mentioning this so we don't duplicate work: we're interested in nasm on the BoringSSL side as well. OpenSSL assembly, which we import, often uses new x86 instructions, and yasm hasn't been updated with them for a while. We're fine right now, but we will need it switched when it's time to enable the next feature.

I've gotten a branch with nasm working and building BoringSSL.

That said, in terms of build time, I'm not observing much of a difference. Actually yasm might be slightly faster. For the boringssl_asm target, yasm is 1.589s and nasm is 1.626s.

The yasm tool itself builds slower though, and is much *much* more of a hassle to update, on account of its build being more complex. yasm is 8.004s, nasm is 7.053s. My nasm build files total only 161 lines, compared to yasm's 558. And half of that count is the file lists which I have a script to automatically generate.

(dalecurtis@ is OOO right now, and I'm in no rush, so I'll probably wait until later to sort out all the reviews, unless someone wants it done soon.)
Project Member

Comment 13 by bugdroid1@chromium.org, Jun 26 2018

The following revision refers to this bug:
  https://boringssl.googlesource.com/boringssl/+/eaf0a17db85a71efc834657631423c52055560a0

commit eaf0a17db85a71efc834657631423c52055560a0
Author: David Benjamin <davidben@google.com>
Date: Tue Jun 26 15:47:29 2018

Add a copy of NASM to util/bot/ in BoringSSL.

This is to transition BoringSSL's Windows build from Yasm to NASM. This
change itself is a no-op for now, but a later change to the BoringSSL
recipes will add a pair of standalone builders here. Then I'll get the
change I have lying around for Chromium moving.

Bug: chromium:766721
Change-Id: I4dca1c299f93bc5c01695983fe0478490c472deb
Reviewed-on: https://boringssl-review.googlesource.com/29324
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

[modify] https://crrev.com/eaf0a17db85a71efc834657631423c52055560a0/util/bot/DEPS
[add] https://crrev.com/eaf0a17db85a71efc834657631423c52055560a0/util/bot/nasm-win32.exe.sha1
[modify] https://crrev.com/eaf0a17db85a71efc834657631423c52055560a0/util/bot/UPDATING

fyi: libaom has been updated. The revision in chromium builds cleanly with nasm.
Great! Thanks!

My plan is to drop in nasm alongside yasm to start with, switch BoringSSL over, and then some combination of individual module OWNERS and nasm/yasm OWNERS can drive the rest of the transition incrementally, so it's okay if everything's not working all at once.
nasm is substantially slower for a couple of files in libvpx. I don't think this is necessarily a blocker but one particular file is ~3 seconds for yasm and ~11 seconds for nasm:

https://bugs.chromium.org/p/webm/issues/detail?id=1557#c2
yasm allows setting YASM_TEST_SUITE to force a deterministic value for the coff timestamp field. We should make sure nasm can do that too before switching.
I believe I've patched the places where nasm calls into time and such. (yasm needed patches too. YASM_TEST_SUITE alone still left a build time in the binary, if I recall.) How would I confirm this? Just check that the nasm binaries and the assembled object files don't change on a rerun?

(This has stalled a bit but I hope to pick it back up at some point. I recently moved across the country and am still settling into everything.)
We have a few bots that check for this:

https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Windows%20deterministic (build 9792 doesn't yet set YASM_TEST_SUITE and the stdout prints the .o files generated by yasm at the bottom, build 9792 does have that change and hopefully won't print that diff anymore once it has cycled)
Project Member

Comment 21 by bugdroid1@chromium.org, Oct 31

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/deps/nasm/+/3c16ae0099239e01503493e9be6a2db0f5fbc582

commit 3c16ae0099239e01503493e9be6a2db0f5fbc582
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Wed Oct 31 20:02:29 2018

Update .gitignore not to ignore generated files Chrome needs.

By default any file generated by "make perlreq" is hidden by the
.gitignore. This may require ongoing maintanence, so a PRESUBMIT
has been added to prevent future regressions.

This is the first of the commits preparing NASM for use in Chrome.

BUG=766721
TEST=none

Change-Id: Ib6cae0b2e6ad0420185a01497e2184c094cc105e
[add] https://crrev.com/3c16ae0099239e01503493e9be6a2db0f5fbc582/PRESUBMIT.py
[modify] https://crrev.com/3c16ae0099239e01503493e9be6a2db0f5fbc582/.gitignore

Project Member

Comment 22 by bugdroid1@chromium.org, Oct 31

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/deps/nasm/+/107604940bdc9787bc12fab8cdcc22ca6d53fdbb

commit 107604940bdc9787bc12fab8cdcc22ca6d53fdbb
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Wed Oct 31 20:04:48 2018

Apply deterministic build patch.

This removes __DATE__ from a few places to ensure the build is always
the same. Patch originally authored by davidben at chromium.org

This is one of the initial commits preparing NASM for use in Chrome.

BUG=766721

Change-Id: I4217f9ffed1455b8f244b024dc10dbb8c5c0664d

[modify] https://crrev.com/107604940bdc9787bc12fab8cdcc22ca6d53fdbb/nasmlib/ver.c
[modify] https://crrev.com/107604940bdc9787bc12fab8cdcc22ca6d53fdbb/asm/nasm.c
[modify] https://crrev.com/107604940bdc9787bc12fab8cdcc22ca6d53fdbb/include/ver.h
[modify] https://crrev.com/107604940bdc9787bc12fab8cdcc22ca6d53fdbb/disasm/ndisasm.c

Project Member

Comment 23 by bugdroid1@chromium.org, Oct 31

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/deps/nasm/+/9596cc0f7bf15a421347556407f9b7a5197e911f

commit 9596cc0f7bf15a421347556407f9b7a5197e911f
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Wed Oct 31 21:25:55 2018

Add all the chromium stuff necessary to build nasm.

This adds the BUILD file and configuration davidben at chromium.org wrote
originally on https://chromium-review.googlesource.com/c/chromium/src/+/1119211

This is the last CL enabling nasm usage in Chromium (fingers crossed), but may
need some tweaks to build on Windows mac. A subsequent Chromium CL adding the
DEP will test this theory.

BUG=766721

Change-Id: I481fc8d7f1747dad74cd0a02a251a65cfd78a134
[add] https://crrev.com/9596cc0f7bf15a421347556407f9b7a5197e911f/OWNERS
[add] https://crrev.com/9596cc0f7bf15a421347556407f9b7a5197e911f/generate_nasm_sources.py
[add] https://crrev.com/9596cc0f7bf15a421347556407f9b7a5197e911f/BUILD.gn
[add] https://crrev.com/9596cc0f7bf15a421347556407f9b7a5197e911f/README.chromium
[add] https://crrev.com/9596cc0f7bf15a421347556407f9b7a5197e911f/nasm_assemble.gni
[add] https://crrev.com/9596cc0f7bf15a421347556407f9b7a5197e911f/nasm_sources.gni

This is the only Windows-specific bug I've had to work around for nasm:
https://bugzilla.nasm.us/show_bug.cgi?id=3392451

libvpx/libaom have been fixed but if other projects don't normally build with nasm you might run into it. Manifests as not being able to find the symbols in the affected .asm files.
Thanks for the pointer! I'll keep an eye on it as I get the platforms up.
This is the fix if you do run across it:
https://chromium-review.googlesource.com/c/webm/libvpx/+/804514

It just adds
SECTION .text
to the top of every .asm file.
Project Member

Comment 27 by bugdroid1@chromium.org, Nov 1

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/deps/nasm/+/3dacc5e63698920fa8b6371a7e4eab7281ad6afc

commit 3dacc5e63698920fa8b6371a7e4eab7281ad6afc
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Thu Nov 01 17:48:41 2018

Change config.h into a platform specific includer. Mac needs config.

Unfortunately Mac needs its own build config, possibly Windows does
too, so we need to create special configs per platform. Since the
nasm source also explicitly include "config/config.h" we need to
change the base config.h into one that includes the right platform
config.

This only fixes Mac for the moment, I'll send a fix for Windows
once I've tried clang-cl on my Windows machine. At present the
config-win.h file is just a copy of the Linux one.

BUG=766721

Change-Id: Ib92a2aa5ad8a0ecb651072a7e7d91f3d8ad6478d
[add] https://crrev.com/3dacc5e63698920fa8b6371a7e4eab7281ad6afc/config/config-mac.h
[add] https://crrev.com/3dacc5e63698920fa8b6371a7e4eab7281ad6afc/config/config-linux.h
[add] https://crrev.com/3dacc5e63698920fa8b6371a7e4eab7281ad6afc/config/config-win.h
[modify] https://crrev.com/3dacc5e63698920fa8b6371a7e4eab7281ad6afc/config/config.h
[modify] https://crrev.com/3dacc5e63698920fa8b6371a7e4eab7281ad6afc/README.chromium
[modify] https://crrev.com/3dacc5e63698920fa8b6371a7e4eab7281ad6afc/BUILD.gn

Project Member

Comment 28 by bugdroid1@chromium.org, Nov 1

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/deps/nasm/+/f7c35ecfb8545f88578fc1eedcbc46e667705908

commit f7c35ecfb8545f88578fc1eedcbc46e667705908
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Thu Nov 01 17:46:27 2018

Add codereview.settings file to suppress warning spam.

I don't think this is used for anything anymore, but this clones
the file from the ffmpeg repository to avoid spam when operating
with some depot tools.

BUG=766721
R=davidben

Change-Id: I424d550edf59c47b1c764e9cf8e68b8ac83140f7
[add] https://crrev.com/f7c35ecfb8545f88578fc1eedcbc46e667705908/codereview.settings

Project Member

Comment 29 by bugdroid1@chromium.org, Nov 2

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/deps/nasm/+/274b50137b6209b106f8989a24ab92f287654147

commit 274b50137b6209b106f8989a24ab92f287654147
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Thu Nov 01 23:59:29 2018

Just use the built-in msvc.h instead of a hand generated Windows one.

Per local benchmarks on test/avx512f.asm, a config.h generated using
Cygwin plus clang-cl yielded a compilation time of ~450ms, while just
using the built in msvc.h yielded ~405ms. So this is a no brainer:
one less platform to generate and it's faster.

BUG=766721

Change-Id: Ic6e930bce1a99adef037ac3fd56fef205696febc
[modify] https://crrev.com/274b50137b6209b106f8989a24ab92f287654147/config/config.h
[delete] https://crrev.com/9e049bbfa101d1ce4022a99fe3835460469c9484/config/config-win.h

Project Member

Comment 30 by bugdroid1@chromium.org, Nov 2

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/deps/nasm/+/0e9e5befa05e964279f2648a5d03f62133c3a457

commit 0e9e5befa05e964279f2648a5d03f62133c3a457
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Fri Nov 02 00:34:21 2018

Provide a bit more clarity in the README file for updating.

BUG=766721

Change-Id: I0d62c8e718714ad227f579d5c41ed34a77c47110
[modify] https://crrev.com/0e9e5befa05e964279f2648a5d03f62133c3a457/README.chromium

Project Member

Comment 31 by bugdroid1@chromium.org, Nov 8

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

commit 7d284aff8ccf98d2d1550fdfe28c3058864b0873
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Thu Nov 08 01:27:52 2018

Add nasm support to Chromium, use it for boringssl.

I'll switch ffmpeg over in a future CL since it seems to need some
work before it's ready to go.

Per davidben at chromium.org who originally authored this update
on https://chromium-review.googlesource.com/c/chromium/src/+/1119211:
Yasm has been unmaintained for a few years, while NASM is actively
maintained. Yasm is already missing some of the newer Intel
instructions, and will continue to miss newer ones. Additionally,
BoringSSL gets its assembly from OpenSSL which natively tests with NASM,
not Yasm, so using NASM puts us closer to the tested path.

BUG=766721
TEST=CQ is happy
R=brucedawson, davidben

Change-Id: Ifa49686e2985967c19b7fc91f94ef6350038fb7c
Reviewed-on: https://chromium-review.googlesource.com/c/1313832
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Bruce Dawson <brucedawson@chromium.org>
Reviewed-by: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606271}
[modify] https://crrev.com/7d284aff8ccf98d2d1550fdfe28c3058864b0873/DEPS
[modify] https://crrev.com/7d284aff8ccf98d2d1550fdfe28c3058864b0873/build/compiled_action.gni
[modify] https://crrev.com/7d284aff8ccf98d2d1550fdfe28c3058864b0873/third_party/.gitignore
[modify] https://crrev.com/7d284aff8ccf98d2d1550fdfe28c3058864b0873/third_party/boringssl/BUILD.gn

Project Member

Comment 32 by bugdroid1@chromium.org, Nov 8

The following revision refers to this bug:
  https://webrtc.googlesource.com/src.git/+/0393c64af13702f297b31aace210b5e8dda13d73

commit 0393c64af13702f297b31aace210b5e8dda13d73
Author: Yves Gerey <yvesg@webrtc.org>
Date: Thu Nov 08 18:02:49 2018

[Win/boringSSL] Add nasm as part of required dependencies.

In order to fix the roll https://webrtc-review.googlesource.com/c/src/+/110080,
this CL updates WebRTC DEPS to be on a par with Chromium's CL:
"Add nasm support to Chromium, use it for boringssl."
https://chromium.googlesource.com/chromium/src/+/7d284aff8ccf98d2d1550fdfe28c3058864b0873

Bug: chromium:766721
Change-Id: I048116f6ec49876a1b878097efff631db8cafe68
Reviewed-on: https://webrtc-review.googlesource.com/c/110340
Reviewed-by: Oleh Prypin <oprypin@webrtc.org>
Commit-Queue: Yves Gerey <yvesg@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#25573}
[modify] https://crrev.com/0393c64af13702f297b31aace210b5e8dda13d73/DEPS

Project Member

Comment 33 by bugdroid1@chromium.org, Nov 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/third_party/ffmpeg/+/db936a442432ae185a15f1fce378c08342dff082

commit db936a442432ae185a15f1fce378c08342dff082
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Thu Nov 08 18:07:52 2018

Switch ffmpeg to using nasm for compilation.

Seems slightly slower in local testing, ~3.3s yasm vs 5.9s nasm,
but since yasm isn't maintained and is started to be required for
new projects we're slowly phasing it out of Chromium.

Seems the only difference is NASM doet s not have CPUNOP, but does
have AVX512 so these are now enabled.

BUG=766721

Change-Id: I7243faa862f958d80f55f9c4089a6d34b7783a7a
Reviewed-on: https://chromium-review.googlesource.com/c/1316287
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>

[modify] https://crrev.com/db936a442432ae185a15f1fce378c08342dff082/chromium/config/Chromium/win/x64/config.asm
[modify] https://crrev.com/db936a442432ae185a15f1fce378c08342dff082/chromium/config/Chromium/win-msvc/x64/config.asm
[modify] https://crrev.com/db936a442432ae185a15f1fce378c08342dff082/chromium/config/Chromium/android/x64/config.asm
[modify] https://crrev.com/db936a442432ae185a15f1fce378c08342dff082/chromium/config/Chrome/win/x64/config.asm
[modify] https://crrev.com/db936a442432ae185a15f1fce378c08342dff082/chromium/scripts/generate_gn.py
[modify] https://crrev.com/db936a442432ae185a15f1fce378c08342dff082/chromium/config/Chromium/android/ia32/config.asm
[modify] https://crrev.com/db936a442432ae185a15f1fce378c08342dff082/chromium/config/Chrome/mac/x64/config.asm
[modify] https://crrev.com/db936a442432ae185a15f1fce378c08342dff082/chromium/config/Chrome/linux/ia32/config.asm
[modify] https://crrev.com/db936a442432ae185a15f1fce378c08342dff082/ffmpeg_generated.gni
[modify] https://crrev.com/db936a442432ae185a15f1fce378c08342dff082/chromium/config/Chrome/android/x64/config.asm
[modify] https://crrev.com/db936a442432ae185a15f1fce378c08342dff082/chromium/config/Chromium/win/ia32/config.asm
[modify] https://crrev.com/db936a442432ae185a15f1fce378c08342dff082/chromium/config/Chrome/win/ia32/config.asm
[modify] https://crrev.com/db936a442432ae185a15f1fce378c08342dff082/chromium/config/ChromeOS/linux/x64/config.asm
[modify] https://crrev.com/db936a442432ae185a15f1fce378c08342dff082/chromium/config/Chromium/mac/x64/config.asm
[modify] https://crrev.com/db936a442432ae185a15f1fce378c08342dff082/chromium/config/Chrome/linux/x64/config.asm
[modify] https://crrev.com/db936a442432ae185a15f1fce378c08342dff082/BUILD.gn
[modify] https://crrev.com/db936a442432ae185a15f1fce378c08342dff082/chromium/config/ChromeOS/linux/ia32/config.asm
[modify] https://crrev.com/db936a442432ae185a15f1fce378c08342dff082/chromium/scripts/build_ffmpeg.py
[modify] https://crrev.com/db936a442432ae185a15f1fce378c08342dff082/chromium/config/Chrome/android/ia32/config.asm
[modify] https://crrev.com/db936a442432ae185a15f1fce378c08342dff082/chromium/config/Chromium/win-msvc/ia32/config.asm
[modify] https://crrev.com/db936a442432ae185a15f1fce378c08342dff082/chromium/config/Chrome/win-msvc/ia32/config.asm
[modify] https://crrev.com/db936a442432ae185a15f1fce378c08342dff082/chromium/config/Chromium/linux/ia32/config.asm
[modify] https://crrev.com/db936a442432ae185a15f1fce378c08342dff082/chromium/config/Chromium/linux/x64/config.asm
[modify] https://crrev.com/db936a442432ae185a15f1fce378c08342dff082/chromium/config/Chrome/win-msvc/x64/config.asm

Summary: Consider switching from yasm to nasm; it's better maintained and 80% slower (was: Consider switching from yasm to nasm; it's better maintained and much faster.)
Retitling to make title match reality.

Is anyone on the hook to make sure that we're nasm-only at some point?
Owner: dalecur...@chromium.org
Status: Assigned (was: Available)
Sure, I'll take this. There aren't that many yasm clients.

In terms of speed it depends on the file, but at least in ffmpeg's case we don't compile any of the cases for which nasm is faster at this point. 
The nasm community is active, https://forum.nasm.us/index.php -- I'll also send a bug report on speed over there and see if folks care.
Project Member

Comment 38 by bugdroid1@chromium.org, Nov 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/third_party/ffmpeg/+/81f7d1dd5fa4ff50d9a8cd86e7b76796c056459d

commit 81f7d1dd5fa4ff50d9a8cd86e7b76796c056459d
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Thu Nov 08 21:58:05 2018

Drop function: prefix in x86util.asm for macOS ffmpeg builds.

nasm doesn't want the function prefix before the visibliity
on macOS (which remains private_extern). I'm not sure why it's
fine with it on other platforms, but it is ¯\_(ツ)_/¯

BUG=766721
R=chcunningham

Change-Id: I3f20ca7268eb482198c51647caf8c9cc9d52bb1e
Reviewed-on: https://chromium-review.googlesource.com/c/1327383
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>

[modify] https://crrev.com/81f7d1dd5fa4ff50d9a8cd86e7b76796c056459d/libavutil/x86/x86inc.asm
[modify] https://crrev.com/81f7d1dd5fa4ff50d9a8cd86e7b76796c056459d/chromium/patches/README

NextAction: 2018-11-13
Project Member

Comment 40 by bugdroid1@chromium.org, Nov 9

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

commit 449d7b9c7981745a6972b9a7f40cf520132973a5
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Fri Nov 09 00:46:14 2018

Roll src/third_party/ffmpeg/ 458e9fd3f..81f7d1dd5 (7 commits)

https://chromium.googlesource.com/chromium/third_party/ffmpeg.git/+log/458e9fd3f8e8..81f7d1dd5fa4

$ git log 458e9fd3f..81f7d1dd5 --date=short --no-merges --format='%ad %ae %s'
2018-11-08 dalecurtis Drop function: prefix in x86util.asm for macOS ffmpeg builds.
2018-11-08 dalecurtis Cleanup a few remaining yasm references in ffmpeg.
2018-11-02 dalecurtis Switch ffmpeg to using nasm for compilation.
2018-10-17 jeremya set install_name on mac when is_component_ffmpeg && !is_component_build
2018-10-16 vtsyrklevich CFI: Disable cfi-icall for ffmpeg component build
2018-10-02 liberato Group gn configs and gn config changes summary into one commit.
2018-10-02 liberato Cleanup of robosushi.py and robo_branch.py .

Created with:
  roll-dep src/third_party/ffmpeg

R=chcunningham

Bug: 766721
Change-Id: Ic513abcd66ca725b29dd9a745b5b93dd5b99ae26
Reviewed-on: https://chromium-review.googlesource.com/c/1327153
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606679}
[modify] https://crrev.com/449d7b9c7981745a6972b9a7f40cf520132973a5/DEPS

Project Member

Comment 41 by bugdroid1@chromium.org, Nov 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/deps/nasm/+/e701d16949cf86a65f9f5d3ecba253e3bafd13c9

commit e701d16949cf86a65f9f5d3ecba253e3bafd13c9
Author: Yves Gerey <yvesg@google.com>
Date: Fri Nov 09 17:44:47 2018

Fix windows build warnings for MSVC.

Warnings were only muted for clang.

Bug: chromium:766721
Change-Id: Ie7871b791ea3bc0013c774d9a8118cc0eab302aa
[modify] https://crrev.com/e701d16949cf86a65f9f5d3ecba253e3bafd13c9/BUILD.gn

Project Member

Comment 42 by bugdroid1@chromium.org, Nov 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/deps/nasm/+/a0a6951e259bd347c133969740348bb5ebb468c4

commit a0a6951e259bd347c133969740348bb5ebb468c4
Author: Yves Gerey <yvesg@google.com>
Date: Fri Nov 09 18:35:11 2018

Fix windows build warnings for MSVC (continued).

Bug: chromium:766721
Change-Id: I27ab96eeb9e25a87ac37553a33577d79680ec32a
[modify] https://crrev.com/a0a6951e259bd347c133969740348bb5ebb468c4/BUILD.gn

The NextAction date has arrived: 2018-11-13
Project Member

Comment 44 by bugdroid1@chromium.org, Nov 13

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

commit 047254142c539d0a157ddc11c42ce1a9180fcb97
Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Date: Tue Nov 13 17:55:05 2018

Roll src/third_party/webrtc e769ed90c359..44ca9a392ac6 (75 commits)

https://webrtc.googlesource.com/src.git/+log/e769ed90c359..44ca9a392ac6


git log e769ed90c359..44ca9a392ac6 --date=short --no-merges --format='%ad %ae %s'
2018-11-13 mbonadei@webrtc.org Allow usage of stringstream under examples/.
2018-11-13 kwiberg@webrtc.org Remove some unused RentACodec static methods
2018-11-13 peah@webrtc.org AEC3: Corrected erroneous if-statement that always returned true
2018-11-13 nisse@webrtc.org Add missing include of unistd.h
2018-11-13 nisse@webrtc.org Delete deprecated class WrappedI420Buffer
2018-11-13 mbonadei@webrtc.org Configs to run slow_tests.
2018-11-13 nisse@webrtc.org Delete obsolete interface class RtpData
2018-11-13 srte@webrtc.org Adds setup of RTP Extensions in Scenario tests.
2018-11-13 asapersson@webrtc.org Add tests for cpu overuse scaling.
2018-11-12 ouj@fb.com Adding rtcp report interval into RTCConfiguration.
2018-11-12 ouj@fb.com Explicitly retain self in objc blocks to avoid compiler warning.
2018-11-12 srte@webrtc.org Allows change of fake encoder max rate in scenarios tests.
2018-11-12 srte@webrtc.org Add support for screenshare content type in scenario tests.
2018-11-12 srte@webrtc.org Simplifies audio priority rate config in scenario tests.
2018-11-12 eladalon@webrtc.org Remove obsolete comment (WebRtcSessionDescriptionFactory ctor)
2018-11-12 srte@webrtc.org Using early acknowledged rate for safe reset in GoogCC.
2018-11-12 ilnik@webrtc.org In RTP to NTP estimator use linear regression instead of ad hoc filter
2018-11-12 eladalon@webrtc.org Event log - Use ToUnsigned() and ToSigned() on timestamp_ms
2018-11-12 eladalon@webrtc.org Event logs - encode N channels as N-1
2018-11-12 kwiberg@webrtc.org AudioCodingModule: Remove support for creating encoders
2018-11-12 nisse@webrtc.org Tweak ChannelReceive interface, to make it closer to ChannelReceiveProxy
2018-11-12 nisse@webrtc.org Eliminate use of EventWrapper from android audio device tests
2018-11-12 eladalon@webrtc.org Add RtcEvent::timestamp_ms()
2018-11-12 kron@webrtc.org Add offer_extmap_allow_mixed to RTCConfiguration
2018-11-12 danilchap@webrtc.org Revert "Run robolectric tests for Android on several Android API versions"
2018-11-12 aleloi@webrtc.org Fuzzer crash in AGC2.
2018-11-12 jonasolsson@webrtc.org Remove most of api/ortc/.
2018-11-12 kron@webrtc.org Fix overflow for high bitrates in BitrateProber
2018-11-12 yvesg@google.com Revert "Roll "Enable SSE, SSE2, and run-time detected SSE4.1 for libopus.""
2018-11-10 eladalon@webrtc.org Hide RtcEvent members behind accessors
2018-11-10 eladalon@webrtc.org Event logs - separate audio_level and voice_activity
2018-11-09 yvesg@webrtc.org Roll "Enable SSE, SSE2, and run-time detected SSE4.1 for libopus."
2018-11-09 eladalon@webrtc.org Rename fields in rtc_event_log2.proto
2018-11-09 mellem@webrtc.org Fix up an outdated comment in peerconnection_integrationtest.cc.
2018-11-09 Peter) Slatala Signal Network route change in fake ice.
2018-11-09 eladalon@webrtc.org Use delta-encoding in new WebRTC event logs
2018-11-09 phoglund@webrtc.org Clean up root OWNERS.
2018-11-09 artit@webrtc.org Run robolectric tests for Android on several Android API versions
2018-11-09 kron@webrtc.org Pass HdrMetadata between VideoFrame and EncodedImage for VP9
2018-11-09 terelius@webrtc.org Add support for audio in latency visualization.
2018-11-09 jonasolsson@webrtc.org Fix flaky JsepTransportControllerTests.
2018-11-09 kron@webrtc.org Add RTP header extension for HDR metadata
2018-11-09 ilnik@webrtc.org In RTP to NTP estimator do not allow huge jumps in NTP timestamps
2018-11-09 yvesg@webrtc.org Reintroduce missing dependencies in libwebrtc.a library.
2018-11-09 mellem@webrtc.org Implement data channels over media transport.
2018-11-08 ouj@fb.com Reland "Use the factory instead of using the builtin code path in `VideoCodecInitializer`"
2018-11-08 yvesg@webrtc.org [Win/boringSSL] Add nasm as part of required dependencies.
2018-11-08 Peter) Slatala Callback changes to media transport interface:
2018-11-08 Peter) Slatala Add owners for media_transport_interface
2018-11-08 sprang@webrtc.org Add ability to specify if rate controller of video encoder is trusted.
2018-11-08 sprang@webrtc.org In Android encoders, cache EncoderInfo in InitEncode.
2018-11-08 nisse@webrtc.org Delete rtc::Filesystem. Move needed functions to filerotatingstream.cc.
2018-11-08 nisse@webrtc.org Eliminate use of EventWrapper from mac audio device
2018-11-08 sprang@webrtc.org Add magjed/nisse/sprang/brandtr as api/video_codecs owners
2018-11-08 danilchap@webrtc.org Introduce RtpPacket::GetExtension accessor that return result
2018-11-08 yujo@chromium.org Split a separate codecs target off of :video_jni
2018-11-08 nisse@webrtc.org Eliminate use of EventWrapper from ios audio device tests
2018-11-08 alessiob@webrtc.org Tolerate optional chunks in WAV files
2018-11-08 saza@webrtc.org Add flag for fast jitter buffer playout in neteq simulation
2018-11-08 alessiob@webrtc.org MsanUninitialized: restric type check to msan case.
2018-11-08 nisse@webrtc.org Delete classes EventFactory and EventFactoryImpl.
2018-11-08 oprypin@webrtc.org Make the bitrate_allocator param optional to prepare for its removal
2018-11-08 nisse@webrtc.org Reenable test RampUpTest.AudioTransportSequenceNumber
2018-11-08 kwiberg@webrtc.org Add a style rule about not using const optional<T>& arguments
2018-11-08 saza@webrtc.org Add missing conditional defines to neteq test and tools targets
2018-11-08 nisse@webrtc.org Deprecate EventFactory and delete all usage.
2018-11-07 sprang@webrtc.org Update H264 encoder to use GetEncoderInfo
2018-11-07 sprang@webrtc.org Update LibVpxVp8Encoder to use GetEncoderInfo
2018-11-07 sprang@webrtc.org Update VP9 encoder to use GetEncoderInfo
2018-11-07 orphis@webrtc.org Remove multiple RTX codec entries in GetRtpReceiver/SenderCapabilities
2018-11-07 sprang@webrtc.org Update SimulcastEncoderAdapter merging of EncoderInfo
2018-11-07 ilnik@webrtc.org Clear FrameBuffer if there were no frames received for 10 minutes
2018-11-07 alessiob@webrtc.org Reland "Isolating APM API build target: making :api an actual target."
2018-11-07 brandtr@webrtc.org Add field trial for target bitrate RTCP XR message.
2018-11-07 nisse@webrtc.org Delete NullEventFactory


Created with:
  gclient setdep -r src/third_party/webrtc@44ca9a392ac6

The AutoRoll server is located here: https://autoroll.skia.org/r/webrtc-chromium-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.

CQ_INCLUDE_TRYBOTS=luci.chromium.try:linux_chromium_archive_rel_ng;luci.chromium.try:mac_chromium_archive_rel_ng

BUG=chromium:None,chromium:none,chromium:None,chromium:901661,chromium:None,chromium:None,chromium:None,chromium:766721,chromium:None,chromium:None,chromium:None,chromium:none,chromium:None
TBR=webrtc-chromium-sheriffs-robots@google.com

Change-Id: I80b2d4e7908e09e4b4b99e592eca5879ce252ca2
Reviewed-on: https://chromium-review.googlesource.com/c/1333849
Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#607647}
[modify] https://crrev.com/047254142c539d0a157ddc11c42ce1a9180fcb97/DEPS

I think we lost this build determinism thing in the transition: https://chromium-review.googlesource.com/c/chromium/src/+/1180718/

I don't know if nasm has a flag like this; if not it should grow one.
Oof, yeah. Sorry, I'd searched for uses of __DATE__ to patch out, but it didn't occur to me that, being a build tool, we'd also care about uses of time() and friends. :-(
Not having absolute paths in the output is important too.
There doesn't appear to be an option to patch out the time, so we'll want to add a flag for that, yeah. 

The absolute paths might already not be in there. I don't see a getcwd call, at least. (nasm is intentionally very dumb about paths. Until the most recent release, it didn't even know what path separators were: https://www.nasm.us/xdoc/2.14/html/nasmdoc2.html#section-2.1.17 )

From poking in the source, I believe we want to patch:

NASM version in debug info:
https://repo.or.cz/nasm.git/blob/HEAD:/output/codeview.c#l733

Timestamps in output:
https://repo.or.cz/nasm.git/blob/HEAD:/output/outcoff.c#l924
https://repo.or.cz/nasm.git/blob/HEAD:/asm/nasm.c#l423

There are also some time calls in rdoff, but that seems to build some separate tool we don't use.
NASM_VER should be okay, it'll only change when we update nasm. I'll fix the other two.
Project Member

Comment 51 by bugdroid1@chromium.org, Nov 26

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/deps/nasm/+/417709dfd155acb4c7097d0232d4478817a77d2f

commit 417709dfd155acb4c7097d0232d4478817a77d2f
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Mon Nov 19 23:46:52 2018

Add README.patches, clone find_patches.py from ffmpeg repository.

Script allows for automatically generating the README.patches file
so we can track local modifications more easily.

BUG=766721

Change-Id: I4170a5b629ded08d6bafe1d0b131527588773059
[add] https://crrev.com/417709dfd155acb4c7097d0232d4478817a77d2f/README.patches
[add] https://crrev.com/417709dfd155acb4c7097d0232d4478817a77d2f/find_patches.py
[modify] https://crrev.com/417709dfd155acb4c7097d0232d4478817a77d2f/README.chromium

Project Member

Comment 52 by bugdroid1@chromium.org, Nov 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/deps/nasm/+/4ee6a69ce33be1e96fd3c44a6e3ae3d8177453da

commit 4ee6a69ce33be1e96fd3c44a6e3ae3d8177453da
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Mon Nov 26 23:02:03 2018

Remove uses of time(NULL) for build determism.

Removes cases suggested on the bug.

BUG=766721
TEST=none

Change-Id: I88c11d052aef8a9c4e48b3b976ad432f3b008dd6
[modify] https://crrev.com/4ee6a69ce33be1e96fd3c44a6e3ae3d8177453da/README.patches
[modify] https://crrev.com/4ee6a69ce33be1e96fd3c44a6e3ae3d8177453da/output/outcoff.c
[modify] https://crrev.com/4ee6a69ce33be1e96fd3c44a6e3ae3d8177453da/asm/nasm.c

Project Member

Comment 53 by bugdroid1@chromium.org, Nov 28

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

commit 336e4debe7e9439f07b60be05fcf4af377b780f6
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Wed Nov 28 01:29:59 2018

Roll src/third_party/nasm/ a0a6951e2..4ee6a69ce (2 commits)

https://chromium.googlesource.com/chromium/deps/nasm.git/+log/a0a6951e259b..4ee6a69ce33b

$ git log a0a6951e2..4ee6a69ce --date=short --no-merges --format='%ad %ae %s'
2018-11-19 dalecurtis Remove uses of time(NULL) for build determism.
2018-11-19 dalecurtis Add README.patches, clone find_patches.py from ffmpeg repository.

Created with:
  roll-dep src/third_party/nasm

R=davidben

Bug: 766721
Change-Id: Ie98e0aae2b4c955913da9bc88e19820232aa0a88
TBR: davidben
Reviewed-on: https://chromium-review.googlesource.com/c/1352586
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: David Benjamin <davidben@chromium.org>
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611492}
[modify] https://crrev.com/336e4debe7e9439f07b60be05fcf4af377b780f6/DEPS

Sign in to add a comment