On the latest run on my Windows machine,
with 720p VP9 I get 27-30 fps and
with 720p H264 I get 6-10 fps (googFrameRateSent).

Performance would likely also be improved by using multiple threads for encoding, which it currently can't do (at least on Mac) due to bug 583348.

Note: The performance is much worse for Debug builds! On my Linux machine 720p loopback gives me 7-8 fps, encoding being the bottleneck (decoding in < 8 ms). This is not the case for other codecs.

On Linux locally this looks dominated by:

  16.77%  EncoderThread    video_loopback         [.] _Z18WelsSampleSad8x8_cPhiS_i
   6.80%  CaptureThread    video_loopback         [.] decode_mcu
   6.49%  EncoderThread    video_loopback         [.] _ZN7WelsEnc19WelsSampleSatd4x4_cEPhiS0_i

Forcing four threads:

20.71%  EncoderThread    video_loopback         [.] _Z18WelsSampleSad8x8_cPhiS_i                               
 7.61%  EncoderThread    video_loopback         [.] _ZN7WelsEnc19WelsSampleSatd4x4_cEPhiS0_i                   
 4.21%  EncoderThread    video_loopback         [.] _ZN7WelsEnc21WelsQuantFour4x4Max_cEPsPKsS2_S0_             
 4.07%  EncoderThread    video_loopback         [.] _ZN6WelsVP18VAACalcSadSsdBgd_cEPKhS1_iiiPiS2_S2_S2_S2_S2_Ph

Done using 720p 30fps input on a release build (webrtc, not in chromium).

Not very impressed with multi-thread performance gains, so I don't think they'll be sufficient:

One thread:

(send_statistics_proxy.cc:154): WebRTC.Video.EncodeTimeInMs 32

Four threads:

(send_statistics_proxy.cc:154): WebRTC.Video.EncodeTimeInMs 24

Maybe assembly is top prio to improve these numbers.

For comparison, VP8 takes 9 ms:

(send_statistics_proxy.cc:154): WebRTC.Video.EncodeTimeInMs 9, so ToT H264 is 3-4x slower on my machine.

The multi-threading performance might be better if you also set the encoder to use the same number of slices as threads.

The obvious target here is to use the assembler versions that are present in the OpenH264 source. In fact it's so obvious, it should probably be a separate bug.

@hta we've been looking at getting the openh264 assembly code compiled and leveraged, but have been having some trouble with getting it done via the chrome build (as opposed to building openh264 separately and linking it)--would be great to share some notes there if anyone on the chrome side has been working on that.

we did get it working on mac via compiling openh264 separately and were able to get a sense of how much of an improvement it makes...so far it's been a big win.  attached some screenshots of webrtc-internal stats.

CameraBefore/After is camera video, relatively static scene (just me sitting in front of the computer without moving much).

ScreenshareBefore/After is 1080p screenshare (entire screen), capped at 15fps.  No motion there really.

ScreenshareBefore/AfterHighMotion is 1080p screenshare (entire screen), capped at 15fps.  Playing the 'planet earth bird scene' on youtube, fullscreen.  Very noticeable difference there.

Deleted: CameraBefore.png 796 KB

	CameraBefore.png 796 KB View Download

Deleted: CameraAfter.png 638 KB

	CameraAfter.png 638 KB View Download

Deleted: ScreenshareBefore.png 646 KB

	ScreenshareBefore.png 646 KB View Download

Deleted: ScreenshareAfter.png 585 KB

	ScreenshareAfter.png 585 KB View Download

Deleted: ScreenshareBeforeHighMotion.png 722 KB

	ScreenshareBeforeHighMotion.png 722 KB View Download

Deleted: ScreenshareAfterHighMotion.png 747 KB

	ScreenshareAfterHighMotion.png 747 KB View Download

we did get the mac build working, though we're getting complaints about out-of-order symbols from the verify-order script (we commented out the error and nothing seems to be falling over, but not entirely sure what that script is trying to enforce).  we did have to upgrade the openh264 version to 1.3-1247-gb2d6902.  we got it working on windows as well, but in addition to the openh264 version upgrade, we also had to upgrade yasm to 1.3.

Attached is the patch for compiling openh264 with asm through gyp. Let me know if we need to create a cl. 

As @brian noted we had to disable the verify_order script for mac.

On windows the yasm version bundled with chromium@51.0.2704.106-0-g21afa7f would fail trying to compile for 32bit with invalid opcodes. Openh264 uses nasm in their Makefile compilation and the upgraded version of yasm-1.3.0 which features a "Full NASM-compatible parser" [http://yasm.tortall.net/releases/Release1.3.0.html] works. So for windows we force this in gyp by using the `use_system_yasm` variable. Unfortunately the default implementation of use_system_yasm in gyp `<!(which yasm)` doesn't work because ninja is unhappy with non-relative paths. So we hardcoded the path and copy the yasm binary to out/Release. 

Lastly webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc was changed to remove SM_AUTO_SLICE config as this was removed in openh264 by this commit https://github.com/cisco/openh264/commit/33c378f7b791310e4cb64b53e2bb8f3f3bded105.

Deleted: 001_integrated_openh264_asm_build.patch 9.7 KB

001_integrated_openh264_asm_build.patch 9.7 KB Download

Deleted: webrtc_openh264_asm_upgrade.patch 800 bytes

webrtc_openh264_asm_upgrade.patch 800 bytes Download

Good job! :D

@sprang, @pbos: Will you take a look at this? Can we get this in for the next cut M-54? It should be prioritized. (Know what steps to take to upgrade yasm?)

I'll take a look today.

+@dalecurtis, do you know what's required to update yasm to 1.3.0?

Didn't have time to get very far with this. pbos@, can you take over or reassign? I'm OOO until the 22nd.

I don't think anyone here has time before you get back. I think dalecurtis@ is back from vacation today and might help answer #19 though.

Should be trivial, just sync the new copy into the tree.

@dalecurtis I'm looking at chromium/src/third_party/yasm, and it doesn't look trivial to me as a "stranger" to update - multiple source patches and generated files that have to be checked in, and we're syncing from a Chromium repo called "patched-yasm", not from the yasm master.

You touched this last - do you want to look at it again, or should I call on someone else? (The upgrade to 1.2.0 was done by hbono@ back in 2012)

sprang@ will this make it to M54?

Probably not.
Ping dalecurtis@, I had the same reaction as hta@ when I looked at this. Have not had time to work on it, I will probably be pretty swamped for awhile..

Those patches are all from yasm upstream so I think syncing should be just mean bringing in the latest repo. Also for the verify order stuff, look at the settings in third_party/ffmpeg/build.gn to ensure you hide symbols properly.

If you're using the x86inc.asm file see the modifications that we have in third_party/x86inc/ that allow hiding of symbols on OSX.

It didn't make it into M55 so moving to M56.

@anatoli when is the cutoff for M55?  Anything we could do on our end to try and get this into 55?  I'm doing some work now to port the build changes over to gn.

@brian thanks for continuing to follow up on this!
Cisco claims that it's possible to compile openh264 on yasm 1.2, I've been unable to verify.

thanks @hta, that's interesting.  i assume then that upgrading yasm/rebasing the google patches is the big blocker here?  i'd have to go back and try again with 1.2 to see the specific errors we were hitting, maybe i can get in touch with someone on the cisco side.  how long before the expected 55 cut?

Yes, upgrading yasm in the chromium build system is a big blocker - since we're aiming for hermetic builds, using a system-installed yasm is not an option.

The dates for 55 are:

Beta
55
Key Dates:
Feature Freeze:	Sep 23 2016 PT
Branch:	Oct 06 2016 PT
Beta Range:	Oct 20 2016 PT -
Oct 27 2016 PT
Stable Release:	Dec 06 2016 PT

So it's currently in beta; you/we need to get a patch landed in tip-of-tree (56) and ask for a downmerge if we want to have it in 55.

@hta, @brian, I am an engineer from openh264, could you let me know what kind of issues on compiling openh264 on yasm 1.2?
many thanks.

@guanwei thanks...sent you an email (pulled from the openh264 github) so we can try and get it sorted without clouding up this thread too much.  will report back here with what we find

@hta I talked with guangwei a bit and got things building with yasm 1.2 (windows 64 bit is giving me a bit of trouble, still working on that).  The twist is that it requires updating to a much more recent version of openh264 (0fd88df93c5dcaf858c57eb7892bd27763f0f0ac[0] which was committed in late September).  So if we keep yasm the same and just need to update openh264, will that work?

[0]: https://github.com/cisco/openh264/commit/0fd88df93c5dcaf858c57eb7892bd27763f0f0ac

Keeping OpenH264 up to date is generally a good idea in itself, so I'm on board with that unless you expect the branch to be unstable between releases.

@pbos great!  believe i've got the windows 64 bit problem sorted.  i'll spend some time cleaning the patch up and then i'll post it here.

as far as openh264 branch stability, maybe @guangwei has some info there

The bug for rolling H.264 version is here:
https://bugs.chromium.org/p/chromium/issues/detail?id=614970

I'm marking that as a blocker for this one; I think we can roll openh264 without fixing the assembler problem, so let's do that first.

@hta i've got changes that address the openh264 api changes (mentioned in https://bugs.chromium.org/p/webrtc/issues/detail?id=6583).  there are only a couple places that need to be changed, but can post that patch.

I just posted https://codereview.chromium.org/2440143002 and https://codereview.chromium.org/2440113002
bbaldino@, can take a look and see if they match what you did?

thanks @sprang, just commented on the other bug (and looks like I forgot to mention the source file additions, oops).  only difference there is the slice mode, I haven't tried the one you're using there (we did SM_SINGLE_SLICE) but I can test it out.

@sprang quick test looks good!  didn't see the bitrate problem we were seeing with SM_FIXEDSLCNUM_SLICE

attached here a first take at the patch to build the assembly sources, i suspect it'll need some cleanup to better handle the different platforms.  this was based on master as of some time over the weekend, as well as pulling in the openh264 version roll patches from comment #41.

i only really handle the x86 and x86_64 cases as those are the only platforms i've tested any of this on, so maybe it makes sense just to start with those for now?

Deleted: openh264_build_gn_diff 11.8 KB

openh264_build_gn_diff 11.8 KB View Download

Deleted: openh264_sources.gni 17.2 KB

openh264_sources.gni 17.2 KB Download

FYI, the roll of OpenH264 to v1.6.0 has now landed.

some issues with the assembly patch in #44 on mac, will post a new one today.

Bumping to M57. Please update if that's wrong.

The version roll seems to be sticking, but the assembly patch isn't landed, so yes, this can't make M56.

Ok, took slightly longer than planned but do have an updated patch.  I've tested this on windows (32 and 64 bit) and mac (64 bit).  linux isn't handled in this patch so it won't use the assembly yet.  Note that on mac verify_order still complains, but I think you know how to get that sorted.

Deleted: openh264_asm_patch 30.3 KB

openh264_asm_patch 30.3 KB View Download

hta@, could you possibly have a look at this? I fear I won't have time for this for several more weeks.

Brian, have you signed the WebRTC CLA?
If I manage to land this, I'm effectively landing your code on the repo, so the lawyers will insist that you've done the CLA thing.

Correction: Since this BUILD file lives in the chromium repo, that would be the Chromium CLA.

@hta just signed the agreement (under this email).

I now have a patch (https://codereview.chromium.org/2585733002/), but it fails due to verify_order on mac (I'm hoping sprang@ or someone knows how to fix that), and it crashes in the MediaRecorder code.

Adding mcasas@ to the CC list for advice.

I have no idea, sorry.

# Verifies that no global text symbols are present in a Mach-O file
# (MACH_O_FILE) at an address higher than the address of a specific symbol
# (LAST_SYMBOL).  If LAST_SYMBOL is not found in MACH_O_FILE or if symbols
# are present with addresses higher than LAST_SYMBOL's address, an error
# message is printed to stderr, and the script will exit nonzero.

Not even sure I know what a Mach-O file is.

#54: the bots won't say what goes wrong, so I guess the only
way to debug this is to patch in the CL and lldb it.
A Mach-o file is an object file format, much like an elf 
in unix or a dex in Android.

@mcasas would love it if you can take a stab at it - I have no macs to try it on.
The command that fails is pretty clear, but it looks like it's stack-dumping rather than giving a decent output, so there may be something unexpected here.

#58: I patched https://crrev.com/2585733002 against ToT in my
MBP2015, compiled and run 

./out/gn/content_browsertests --gtest_filter=WebRtcMediaRecorderTest.StartAndDataAvailable/*

which will touch all h264 variants (2,3 and 6) and saw no
crash :?

what else do you want to try?

+emircan@ since he knows many arcanes of the VideoEncodeAccelerator.

My gn args:

is_component_build = true
is_debug = true
use_goma = true
enable_nacl = false

remove_webcore_debug_symbols = true

use_openh264 = true
rtc_use_h264 = true

@mcasas did you build the specific target

[12762/14218] ACTION //chrome:verify_chrome_framework_order(//build/toolchain/mac:clang_x64)


?
This was the build step that crashed on the mac build bot (if I read the bot log right).

Great that you saw no issue with the tests!

@mcasas there is also a failure on MediaStreamRecorder on linux - see bot logs for (e.g.) cast_shell_linux.

Re. #60, I compiled 
  time ninja -j 100 -C out/gn all
-- which stuffed my HD full and forced me to remove a lot of 
stuff :-) but it all compiled fine. 

//chrome:verify_chrome_framework_order  seems to me a 
prerequisite of a bunch of other pretty normal things:

gn desc out/gn chrome:verify_chrome_framework_order
Target //chrome:verify_chrome_framework_order
Type: group
Toolchain: //build/toolchain/mac:clang_x64

visibility
  *

testonly
  false

public
  [All headers listed in the sources are public.]

all_dependent_configs (in order applying, try also --tree)
  //third_party/khronos:khronos_headers
  //ui/gl:gl_config
  //media:media_dependent_config
  //device/bluetooth:bluetooth_config
  //third_party/webrtc/base:rtc_base_approved_all_dependent_config
  //third_party/webrtc/base:rtc_base_all_dependent_config
  //third_party/libwebp:libwebp_config
  //ui/views:flags
  //third_party/crashpad/crashpad/util:util_link_config

Direct dependencies (try also "--all", "--tree", or even "--all --tree")
  //chrome:chrome_framework

libs
  Cocoa.framework
  Foundation.framework
  IOKit.framework
  Security.framework
  SystemConfiguration.framework
  bsm


So: can't repro this build failure, but bots have older Mac versions
and sometimes miss libraries.

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

commit e1926525b79fdbd181867190c7ef012e7f594e98
Author: hta <hta@chromium.org>
Date: Mon Jan 16 11:54:45 2017

Refactor GNI files in preparation for assembly

This CL moves around the definitions of the OpenH264
source files, in preparation for adding H.264 assembly.

It is a spin-off from CL/2585733002

BUG= 600399 

Review-Url: https://codereview.chromium.org/2637703003
Cr-Commit-Position: refs/heads/master@{#443869}

[modify] https://crrev.com/e1926525b79fdbd181867190c7ef012e7f594e98/third_party/openh264/BUILD.gn
[add] https://crrev.com/e1926525b79fdbd181867190c7ef012e7f594e98/third_party/openh264/openh264_sources.gni

Status update: On iOS, we SHOULD be always doing hardware encode, so the only Apple target of interest is macs where we don't use hardware encoders.

There's a persistent problem with symbol visibility on Mac - as mcasas said, it's not clear how to reproduce that locally. So the first order of business is Windows and Linux (the latter so that we can verify speedups on a developer's workstation).
 

Re #64, on macs we use H264 HW encoder 97% of the time, see the UMAs below. It makes sense to focus on Windows and Linux first. Also, note that HW H264 encoder is planned to be rolled in Windows for M56.
https://uma.googleplex.com/p/chrome/timeline_v2/?sid=0c73c6c0b9d1ff17ad68eecec2f7acac
https://uma.googleplex.com/p/chrome/timeline_v2/?sid=629ae6b3cb56b8c1248e2503edf4ecfa

is it possible to always enable the assembly-optimized code (on supported platforms) and just let it be overridden by hardware (if it's available) in the same way that software encoders are overridden by hardware when it's available?  

maybe this is no longer the case, but in my experience i've seen compatibility/quality issues with hardware accelerated encoding and i've done --disable-webrtc-hw-decoding and --disable-webrtc-hw-encoding to use software (where it would obviously be a big benefit to using the assembly-optimized code).

re #66 - as presently constructed, the assembly-optimized code will always ship for the platforms it is enabled on; we don't have a runtime switch between C++ and assembly. So the switch between hardware and software will run just as before.

If there's hardware encoders that are so bogus that software is preferable, we should blacklist those; please file bugs naming those hardware encoders - I think we already blacklist some.

@hta ok, gotcha.  I thought you were discussing not including the assembly code at all for targets where we expected to be able to use hw.  thanks.

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

commit 6d1501f500099ff3e25f92593e9efcb27d352252
Author: hta <hta@chromium.org>
Date: Fri Jan 20 10:31:53 2017

Add assembly for x86 to OpenH264 encoder

On Linux on a build workstation, these changes seem to give around 2.5x faster encoding (frame encode time in test goes from ~25 to ~9 ms)

We do expect alerts from our performance bots when this lands.

Credit: These build changes were written by brian@highfive.com aka bbaldino@gmail.com.

BUG= 600399 

Review-Url: https://codereview.chromium.org/2585733002
Cr-Commit-Position: refs/heads/master@{#445033}

[modify] https://crrev.com/6d1501f500099ff3e25f92593e9efcb27d352252/third_party/openh264/BUILD.gn

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

commit 3f4ee1b009741853987773717ebc2f22c5f0c60b
Author: haraken <haraken@chromium.org>
Date: Mon Jan 23 01:29:10 2017

Revert of Add assembly for x86 to OpenH264 encoder (patchset #5 id:80001 of https://codereview.chromium.org/2585733002/ )

Reason for revert:
This broke Chrome OS builders:

https://uberchromegw.corp.google.com/i/chromeos/builders/veyron_minnie-chrome-pfq/builds/669/steps/BuildPackages/logs/stdio

Original issue's description:
> Add assembly for x86 to OpenH264 encoder
>
> On Linux on a build workstation, these changes seem to give around 2.5x faster encoding (frame encode time in test goes from ~25 to ~9 ms)
>
> We do expect alerts from our performance bots when this lands.
>
> Credit: These build changes were written by brian@highfive.com aka bbaldino@gmail.com.
>
> BUG= 600399 
>
> Review-Url: https://codereview.chromium.org/2585733002
> Cr-Commit-Position: refs/heads/master@{#445033}
> Committed: https://chromium.googlesource.com/chromium/src/+/6d1501f500099ff3e25f92593e9efcb27d352252

TBR=kthelgason@chromium.org,ehmaldonado@chromium.org,kthelgason@webrtc.org,hta@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 600399 

Review-Url: https://codereview.chromium.org/2651543002
Cr-Commit-Position: refs/heads/master@{#445311}

[modify] https://crrev.com/3f4ee1b009741853987773717ebc2f22c5f0c60b/third_party/openh264/BUILD.gn

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

commit 2c39ea391c4df544e2f6162ae595083ce2d04e99
Author: hta <hta@chromium.org>
Date: Mon Jan 23 11:49:17 2017

Reland of Add assembly for x86 to OpenH264 encoder (patchset #1 id:1 of https://codereview.chromium.org/2651543002/ )

Reason for revert:
Reland

Original issue's description:
> Revert of Add assembly for x86 to OpenH264 encoder (patchset #5 id:80001 of https://codereview.chromium.org/2585733002/ )
>
> Reason for revert:
> This broke Chrome OS builders:
>
> https://uberchromegw.corp.google.com/i/chromeos/builders/veyron_minnie-chrome-pfq/builds/669/steps/BuildPackages/logs/stdio
>
>
> Original issue's description:
> > Add assembly for x86 to OpenH264 encoder
> >
> > On Linux on a build workstation, these changes seem to give around 2.5x faster encoding (frame encode time in test goes from ~25 to ~9 ms)
> >
> > We do expect alerts from our performance bots when this lands.
> >
> > Credit: These build changes were written by brian@highfive.com aka bbaldino@gmail.com.
> >
> > BUG= 600399 
> >
> > Review-Url: https://codereview.chromium.org/2585733002
> > Cr-Commit-Position: refs/heads/master@{#445033}
> > Committed: https://chromium.googlesource.com/chromium/src/+/6d1501f500099ff3e25f92593e9efcb27d352252
>
> TBR=kthelgason@chromium.org,ehmaldonado@chromium.org,kthelgason@webrtc.org,hta@chromium.org
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG= 600399 
>
> Review-Url: https://codereview.chromium.org/2651543002
> Cr-Commit-Position: refs/heads/master@{#445311}
> Committed: https://chromium.googlesource.com/chromium/src/+/3f4ee1b009741853987773717ebc2f22c5f0c60b

BUG= 600399 

Review-Url: https://codereview.chromium.org/2645293002
Cr-Commit-Position: refs/heads/master@{#445358}

[modify] https://crrev.com/2c39ea391c4df544e2f6162ae595083ce2d04e99/third_party/openh264/BUILD.gn

Bumping this to M58. Please correct if that's wrong.

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

commit 95cc5f514856c42df9287ca5f68e3b632e6b0312
Author: hbos <hbos@chromium.org>
Date: Tue Jan 24 08:31:49 2017

Revert of Add assembly for x86 to OpenH264 encoder (patchset #2 id:40001 of https://codereview.chromium.org/2645293002/ )

Reason for revert:
Causes linux_msan to flake about uninitialized value, this warning needs to be ignored before re-landing. Example: https://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/16972

The address of the uninitialized value is used when calling a function through a function pointer. The function writes to this value so it's safe, but linux_msan doesn't know that.

Original issue's description:
> Reland of Add assembly for x86 to OpenH264 encoder (patchset #1 id:1 of https://codereview.chromium.org/2651543002/ )
>
> Reason for revert:
> Reland
>
> Original issue's description:
> > Revert of Add assembly for x86 to OpenH264 encoder (patchset #5 id:80001 of https://codereview.chromium.org/2585733002/ )
> >
> > Reason for revert:
> > This broke Chrome OS builders:
> >
> > https://uberchromegw.corp.google.com/i/chromeos/builders/veyron_minnie-chrome-pfq/builds/669/steps/BuildPackages/logs/stdio
> >
> >
> > Original issue's description:
> > > Add assembly for x86 to OpenH264 encoder
> > >
> > > On Linux on a build workstation, these changes seem to give around 2.5x faster encoding (frame encode time in test goes from ~25 to ~9 ms)
> > >
> > > We do expect alerts from our performance bots when this lands.
> > >
> > > Credit: These build changes were written by brian@highfive.com aka bbaldino@gmail.com.
> > >
> > > BUG= 600399 
> > >
> > > Review-Url: https://codereview.chromium.org/2585733002
> > > Cr-Commit-Position: refs/heads/master@{#445033}
> > > Committed: https://chromium.googlesource.com/chromium/src/+/6d1501f500099ff3e25f92593e9efcb27d352252
> >
> > TBR=kthelgason@chromium.org,ehmaldonado@chromium.org,kthelgason@webrtc.org,hta@chromium.org
> > # Not skipping CQ checks because original CL landed more than 1 days ago.
> > BUG= 600399 
> >
> > Review-Url: https://codereview.chromium.org/2651543002
> > Cr-Commit-Position: refs/heads/master@{#445311}
> > Committed: https://chromium.googlesource.com/chromium/src/+/3f4ee1b009741853987773717ebc2f22c5f0c60b
>
>
> BUG= 600399 
>
> Review-Url: https://codereview.chromium.org/2645293002
> Cr-Commit-Position: refs/heads/master@{#445358}
> Committed: https://chromium.googlesource.com/chromium/src/+/2c39ea391c4df544e2f6162ae595083ce2d04e99

TBR=kthelgason@chromium.org,ehmaldonado@chromium.org,kjellander@chromium.org,haraken@chromium.org,hta@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 600399 

Review-Url: https://codereview.chromium.org/2651643004
Cr-Commit-Position: refs/heads/master@{#445680}

[modify] https://crrev.com/95cc5f514856c42df9287ca5f68e3b632e6b0312/third_party/openh264/BUILD.gn

Re last commit, +eugenis@ how can we best annotate code that calls assembly for use with MSan?

actually +eugenis@, see above for a patch we had to revert b/c of assembly

It's probably easiest to add !is_msan condition in GN.

The other option is to find places in C/C++ where you know that some memory is initialized, and MSan does not, and fix it with __msan_unpoison(). That requires understanding what's going on at the C/asm boundary, and does not really buy you much, because the asm code is not checked either way.

Sounds good, hta@ can you reland with assembly not built under is_msan?

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

commit 2529431f34dfdfef0d698b5ab35a5c9cb9ff566a
Author: hta <hta@chromium.org>
Date: Fri Jan 27 12:29:57 2017

Reland of Add assembly for x86 to OpenH264 encoder (patchset #1 id:1 of https://codereview.chromium.org/2651643004/ )

Reason for revert:
Disabling assembly for MSAN build.

Original issue's description:
> Revert of Add assembly for x86 to OpenH264 encoder (patchset #2 id:40001 of https://codereview.chromium.org/2645293002/ )
>
> Reason for revert:
> Causes linux_msan to flake about uninitialized value, this warning needs to be ignored before re-landing. Example: https://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/16972
>
> The address of the uninitialized value is used when calling a function through a function pointer. The function writes to this value so it's safe, but linux_msan doesn't know that.
>
> Original issue's description:
> > Reland of Add assembly for x86 to OpenH264 encoder (patchset #1 id:1 of https://codereview.chromium.org/2651543002/ )
> >
> > Reason for revert:
> > Reland
> >
> > Original issue's description:
> > > Revert of Add assembly for x86 to OpenH264 encoder (patchset #5 id:80001 of https://codereview.chromium.org/2585733002/ )
> > >
> > > Reason for revert:
> > > This broke Chrome OS builders:
> > >
> > > https://uberchromegw.corp.google.com/i/chromeos/builders/veyron_minnie-chrome-pfq/builds/669/steps/BuildPackages/logs/stdio
> > >
> > >
> > > Original issue's description:
> > > > Add assembly for x86 to OpenH264 encoder
> > > >
> > > > On Linux on a build workstation, these changes seem to give around 2.5x faster encoding (frame encode time in test goes from ~25 to ~9 ms)
> > > >
> > > > We do expect alerts from our performance bots when this lands.
> > > >
> > > > Credit: These build changes were written by brian@highfive.com aka bbaldino@gmail.com.
> > > >
> > > > BUG= 600399 
> > > >
> > > > Review-Url: https://codereview.chromium.org/2585733002
> > > > Cr-Commit-Position: refs/heads/master@{#445033}
> > > > Committed: https://chromium.googlesource.com/chromium/src/+/6d1501f500099ff3e25f92593e9efcb27d352252
> > >
> > > TBR=kthelgason@chromium.org,ehmaldonado@chromium.org,kthelgason@webrtc.org,hta@chromium.org
> > > # Not skipping CQ checks because original CL landed more than 1 days ago.
> > > BUG= 600399 
> > >
> > > Review-Url: https://codereview.chromium.org/2651543002
> > > Cr-Commit-Position: refs/heads/master@{#445311}
> > > Committed: https://chromium.googlesource.com/chromium/src/+/3f4ee1b009741853987773717ebc2f22c5f0c60b
> >
> >
> > BUG= 600399 
> >
> > Review-Url: https://codereview.chromium.org/2645293002
> > Cr-Commit-Position: refs/heads/master@{#445358}
> > Committed: https://chromium.googlesource.com/chromium/src/+/2c39ea391c4df544e2f6162ae595083ce2d04e99
>
> TBR=kthelgason@chromium.org,ehmaldonado@chromium.org,kjellander@chromium.org,haraken@chromium.org,hta@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG= 600399 
>
> Review-Url: https://codereview.chromium.org/2651643004
> Cr-Commit-Position: refs/heads/master@{#445680}
> Committed: https://chromium.googlesource.com/chromium/src/+/95cc5f514856c42df9287ca5f68e3b632e6b0312

TBR=kthelgason@chromium.org,ehmaldonado@chromium.org,kjellander@chromium.org,haraken@chromium.org,hbos@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 600399 

Review-Url: https://codereview.chromium.org/2651183003
Cr-Commit-Position: refs/heads/master@{#446649}

[modify] https://crrev.com/2529431f34dfdfef0d698b5ab35a5c9cb9ff566a/third_party/openh264/BUILD.gn

Can this be closed now?

crbug.com/583348 (https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc?q=h264_encoder_impl.cc&sq=package:chromium&dr) is related to performance but that is tracked by its own bug.

This can probably be closed, hta?

Closing since the most important platform (Windows) is working. Separate bugs exist for ARM and Mac - removing those from the blocker list. (Most Macs have HW acceleration).