Modify build scripts for ios .framework to compile metal shaders.

Modify build scripts for ios .framework  to compile metal shaders.

  - Add compile script that compiles given metal shader file and produces .metallib binary
  - Add gn template for metal library compilation
  - Modify the .framework target in build file to bundle metallib

BUG=webrtc:7079

[daniela-webrtc, 2017-02-02 12:01:41]

Description was changed from

==========
Modify build scripts for ios .framework  to compile metal shaders..
	- Add compile script that compiles given metal shader file
and produces .metallib binary
	- Add gn template for metal library compilation
        - Modify the .framework target in build file to bundle metallib

BUG=7079
==========

to

==========
Modify build scripts for ios .framework  to compile metal shaders..
	- Add compile script that compiles given metal shader file
and produces .metallib binary
	- Add gn template for metal library compilation
        - Modify the .framework target in build file to bundle metallib

BUG=webrtc:7079
==========

[daniela-webrtc, 2017-02-02 12:05:31]

denicija@webrtc.org changed reviewers:
+ kjellander@webrtc.org, kthelgason@webrtc.org, magjed@webrtc.org

[daniela-webrtc, 2017-02-02 12:05:32]

Changes need to enable compilation of metal .shader files in webrtc.
Please take a look.
Note this CL should not be merged before
https://codereview.webrtc.org/2651743007/ is landed.

[kjellander_webrtc, 2017-02-02 12:20:43]

https://codereview.webrtc.org/2673573003/diff/1/webrtc/sdk/objc/compile_metal...
File webrtc/sdk/objc/compile_metal_lib.py (right):

https://codereview.webrtc.org/2673573003/diff/1/webrtc/sdk/objc/compile_metal...
webrtc/sdk/objc/compile_metal_lib.py:1: # Copyright 2016 The WebRTC project
authors. All Rights Reserved.
2017

https://codereview.webrtc.org/2673573003/diff/1/webrtc/sdk/objc/compile_metal...
webrtc/sdk/objc/compile_metal_lib.py:9: import argparse
Please add a short module docstring explaining what this script does.
It's worth mentioning it's designed to be run as an GN action.

https://codereview.webrtc.org/2673573003/diff/1/webrtc/sdk/objc/compile_metal...
webrtc/sdk/objc/compile_metal_lib.py:12: 
+1 blank line for top-level statements (it's a silly style rule, I know).
https://google.github.io/styleguide/pyguide.html?showone=Blank_Lines#Blank_Lines

https://codereview.webrtc.org/2673573003/diff/1/webrtc/sdk/objc/compile_metal...
webrtc/sdk/objc/compile_metal_lib.py:13: if __name__ == '__main__':
Please use a main() function:
https://google.github.io/styleguide/pyguide.html?showone=Main#Main

https://codereview.webrtc.org/2673573003/diff/1/webrtc/sdk/objc/compile_metal...
webrtc/sdk/objc/compile_metal_lib.py:22: # TODO(denicija): replace the hardcoded
iphoneos with the proper sdk var when we add metal for mac
Wrap at column 80.

https://codereview.webrtc.org/2673573003/diff/1/webrtc/sdk/objc/metal.gni
File webrtc/sdk/objc/metal.gni (right):

https://codereview.webrtc.org/2673573003/diff/1/webrtc/sdk/objc/metal.gni#new...
webrtc/sdk/objc/metal.gni:1: # Copyright 2016 The WebRTC project authors. All
Rights Reserved.
2017

[kthelgason, 2017-02-02 12:30:21]

Great work, this seems really clean to me.

https://codereview.webrtc.org/2673573003/diff/1/webrtc/sdk/objc/compile_metal...
File webrtc/sdk/objc/compile_metal_lib.py (right):

https://codereview.webrtc.org/2673573003/diff/1/webrtc/sdk/objc/compile_metal...
webrtc/sdk/objc/compile_metal_lib.py:43: toolout =
subprocess.Popen(metal_lib_args)
We need to wait for this subproccess to finish as well before looking at
`returncode`.

https://codereview.webrtc.org/2673573003/diff/1/webrtc/sdk/objc/metal.gni
File webrtc/sdk/objc/metal.gni (right):

https://codereview.webrtc.org/2673573003/diff/1/webrtc/sdk/objc/metal.gni#new...
webrtc/sdk/objc/metal.gni:20: script = "./objc/compile_metal_lib.py"
I prefer absolute paths whenever possible.

Also, Isn't this path wrong? It seems to me it expands to
webrtc/sdk/objc/objc/compile_metal_lib.py

[magjed_webrtc, 2017-02-02 12:48:16]

This lgtm as well. Only small comment is that you should update the CL
description: use one or zero dots at the end of the title, and you need a blank
line between the title and the rest of the message, otherwise everything will be
included in the title in git. Also, you are mixing tabs and spaces, just go with
spaces (and maybe less indentation?)

Great that you were able to find a clean way to add the metal stuff to GN!

[daniela-webrtc, 2017-02-02 15:10:38]

Description was changed from

==========
Modify build scripts for ios .framework  to compile metal shaders..
	- Add compile script that compiles given metal shader file
and produces .metallib binary
	- Add gn template for metal library compilation
        - Modify the .framework target in build file to bundle metallib

BUG=webrtc:7079
==========

to

==========
Modify build scripts for ios .framework  to compile metal shaders.
  - Add compile script that compiles given metal shader file and produces
.metallib binary
  - Add gn template for metal library compilation
  - Modify the .framework target in build file to bundle metallib

BUG=webrtc:7079
==========

[daniela-webrtc, 2017-02-02 15:11:44]

https://codereview.webrtc.org/2673573003/diff/1/webrtc/sdk/objc/compile_metal...
File webrtc/sdk/objc/compile_metal_lib.py (right):

https://codereview.webrtc.org/2673573003/diff/1/webrtc/sdk/objc/compile_metal...
webrtc/sdk/objc/compile_metal_lib.py:1: # Copyright 2016 The WebRTC project
authors. All Rights Reserved.
On 2017/02/02 12:20:43, kjellander_webrtc wrote:
> 2017

Done.

https://codereview.webrtc.org/2673573003/diff/1/webrtc/sdk/objc/compile_metal...
webrtc/sdk/objc/compile_metal_lib.py:9: import argparse
On 2017/02/02 12:20:43, kjellander_webrtc wrote:
> Please add a short module docstring explaining what this script does.
> It's worth mentioning it's designed to be run as an GN action.

Done.

https://codereview.webrtc.org/2673573003/diff/1/webrtc/sdk/objc/compile_metal...
webrtc/sdk/objc/compile_metal_lib.py:12: 
On 2017/02/02 12:20:43, kjellander_webrtc wrote:
> +1 blank line for top-level statements (it's a silly style rule, I know).
>
https://google.github.io/styleguide/pyguide.html?showone=Blank_Lines#Blank_Lines

Done.

https://codereview.webrtc.org/2673573003/diff/1/webrtc/sdk/objc/compile_metal...
webrtc/sdk/objc/compile_metal_lib.py:13: if __name__ == '__main__':
On 2017/02/02 12:20:43, kjellander_webrtc wrote:
> Please use a main() function:
> https://google.github.io/styleguide/pyguide.html?showone=Main#Main

Done.

https://codereview.webrtc.org/2673573003/diff/1/webrtc/sdk/objc/compile_metal...
webrtc/sdk/objc/compile_metal_lib.py:22: # TODO(denicija): replace the hardcoded
iphoneos with the proper sdk var when we add metal for mac
On 2017/02/02 12:20:43, kjellander_webrtc wrote:
> Wrap at column 80.

Done.

https://codereview.webrtc.org/2673573003/diff/1/webrtc/sdk/objc/compile_metal...
webrtc/sdk/objc/compile_metal_lib.py:43: toolout =
subprocess.Popen(metal_lib_args)
On 2017/02/02 12:30:21, kthelgason wrote:
> We need to wait for this subproccess to finish as well before looking at
> `returncode`. 

Done.

https://codereview.webrtc.org/2673573003/diff/1/webrtc/sdk/objc/metal.gni
File webrtc/sdk/objc/metal.gni (right):

https://codereview.webrtc.org/2673573003/diff/1/webrtc/sdk/objc/metal.gni#new...
webrtc/sdk/objc/metal.gni:1: # Copyright 2016 The WebRTC project authors. All
Rights Reserved.
On 2017/02/02 12:20:43, kjellander_webrtc wrote:
> 2017

Done.

https://codereview.webrtc.org/2673573003/diff/1/webrtc/sdk/objc/metal.gni#new...
webrtc/sdk/objc/metal.gni:20: script = "./objc/compile_metal_lib.py"
On 2017/02/02 12:30:21, kthelgason wrote:
> I prefer absolute paths whenever possible.
> 
> Also, Isn't this path wrong? It seems to me it expands to
> webrtc/sdk/objc/objc/compile_metal_lib.py

Done.

https://codereview.webrtc.org/2673573003/diff/1/webrtc/sdk/objc/metal.gni#new...
webrtc/sdk/objc/metal.gni:20: script = "./objc/compile_metal_lib.py"
On 2017/02/02 12:30:21, kthelgason wrote:
> I prefer absolute paths whenever possible.
> 
> Also, Isn't this path wrong? It seems to me it expands to
> webrtc/sdk/objc/objc/compile_metal_lib.py

I think it unwraps from the folder of the BUILD file it's invoked from. In this
case it was ./BUILD.gn so this was working.
But yeah it's gonna break completely when included from another BUILD file or if
this is moved so your suggestion is on point.

[kthelgason, 2017-02-02 15:22:36]

lgtm given you address my comments 🤠

https://codereview.webrtc.org/2673573003/diff/40001/webrtc/sdk/BUILD.gn
File webrtc/sdk/BUILD.gn (right):

https://codereview.webrtc.org/2673573003/diff/40001/webrtc/sdk/BUILD.gn#newco...
webrtc/sdk/BUILD.gn:52: "objc/Framework/Classes/metal/RTCMTLVideoView.m",
Is it ok to include these on macOS as well? In fact, maybe it makes sense to
only include the metal renderer in the framework target, since we need the
shaders to be in a bundle?

https://codereview.webrtc.org/2673573003/diff/40001/webrtc/sdk/objc/compile_m...
File webrtc/sdk/objc/compile_metal_lib.py (right):

https://codereview.webrtc.org/2673573003/diff/40001/webrtc/sdk/objc/compile_m...
webrtc/sdk/objc/compile_metal_lib.py:57: sys.exit(toolout.returncode)
return this instead of sys.exit.

[magjed_webrtc, 2017-02-02 15:32:04]

Description was changed from

==========
Modify build scripts for ios .framework  to compile metal shaders.
  - Add compile script that compiles given metal shader file and produces
.metallib binary
  - Add gn template for metal library compilation
  - Modify the .framework target in build file to bundle metallib

BUG=webrtc:7079
==========

to

==========
Modify build scripts for ios .framework  to compile metal shaders.

  - Add compile script that compiles given metal shader file and produces
.metallib binary
  - Add gn template for metal library compilation
  - Modify the .framework target in build file to bundle metallib

BUG=webrtc:7079
==========

[daniela-webrtc, 2017-02-02 15:36:54]

https://codereview.webrtc.org/2673573003/diff/40001/webrtc/sdk/BUILD.gn
File webrtc/sdk/BUILD.gn (right):

https://codereview.webrtc.org/2673573003/diff/40001/webrtc/sdk/BUILD.gn#newco...
webrtc/sdk/BUILD.gn:52: "objc/Framework/Classes/metal/RTCMTLVideoView.m",
On 2017/02/02 15:22:36, kthelgason wrote:
> Is it ok to include these on macOS as well? In fact, maybe it makes sense to
> only include the metal renderer in the framework target, since we need the
> shaders to be in a bundle?

Great catch! They are not inteded to be included in this CL at all.

[kjellander_webrtc, 2017-02-03 09:03:17]

lgtm with the last comment fixed.

https://codereview.webrtc.org/2673573003/diff/40001/webrtc/sdk/objc/compile_m...
File webrtc/sdk/objc/compile_metal_lib.py (right):

https://codereview.webrtc.org/2673573003/diff/40001/webrtc/sdk/objc/compile_m...
webrtc/sdk/objc/compile_metal_lib.py:57: sys.exit(toolout.returncode)
On 2017/02/02 15:22:36, kthelgason wrote:
> return this instead of sys.exit.

+1

[daniela-webrtc, 2017-02-07 10:43:59]

https://codereview.webrtc.org/2673573003/diff/40001/webrtc/sdk/BUILD.gn
File webrtc/sdk/BUILD.gn (right):

https://codereview.webrtc.org/2673573003/diff/40001/webrtc/sdk/BUILD.gn#newco...
webrtc/sdk/BUILD.gn:52: "objc/Framework/Classes/metal/RTCMTLVideoView.m",
On 2017/02/02 15:22:36, kthelgason wrote:
> Is it ok to include these on macOS as well? In fact, maybe it makes sense to
> only include the metal renderer in the framework target, since we need the
> shaders to be in a bundle?

Done.

https://codereview.webrtc.org/2673573003/diff/40001/webrtc/sdk/objc/compile_m...
File webrtc/sdk/objc/compile_metal_lib.py (right):

https://codereview.webrtc.org/2673573003/diff/40001/webrtc/sdk/objc/compile_m...
webrtc/sdk/objc/compile_metal_lib.py:57: sys.exit(toolout.returncode)
On 2017/02/02 15:22:36, kthelgason wrote:
> return this instead of sys.exit.

Done.

[kjellander_webrtc, 2017-02-15 07:26:42]

shall we submit this?

[daniela-webrtc, 2017-02-15 08:36:05]

On 2017/02/15 07:26:42, kjellander_webrtc wrote:
> shall we submit this?

Not yet. The CL should be merged after https://codereview.webrtc.org/2651743007/
Also, we've started looking into run-time compilation of the shaders so this CL
*might* be closed. Is this blocking some further work?

[daniela-webrtc, 2017-02-16 14:38:29]

On 2017/02/15 08:36:05, denicija-webrtc wrote:
> On 2017/02/15 07:26:42, kjellander_webrtc wrote:
> > shall we submit this?
> 
> Not yet. The CL should be merged after
https://codereview.webrtc.org/2651743007/
> Also, we've started looking into run-time compilation of the shaders so this
CL
> *might* be closed. Is this blocking some further work?

I've closed this issue as is no longer relevant as we will use (for now) run
time compilation of shaders and that doesn't require any changes in the build
scripts.