Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2036023003:
Script to inline shader into C header file (Closed)
DescriptionScript to inline shader into C header file
OpenGL compiles shader code as text at runtime. It is inconvienent to
edit shader code as const char[] inside the C++ source code.
This CL contains script that reads file contents, turns them into const
char[], and combine them into a header file.
BUG=385924
Committed: https://crrev.com/26219410e3069f9cff75c5c487d2fc2d52f8ef4d
Cr-Commit-Position: refs/heads/master@{#398142}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Fixed Comments #
Total comments: 17
Patch Set 3 : Reviewer's Feedback #Patch Set 4 : Renamed script to shader_to_header #
Total comments: 8
Patch Set 5 : Reviewer's Feedback #
Total comments: 1
Messages
Total messages: 21 (5 generated)
The CQ bit was checked by yuweih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2036023003/1
yuweih@chromium.org changed reviewers: + jamiewalch@chromium.org
PTAL:)
https://codereview.chromium.org/2036023003/diff/1/remoting/client/opengl/BUIL... File remoting/client/opengl/BUILD.gn (right): https://codereview.chromium.org/2036023003/diff/1/remoting/client/opengl/BUIL... remoting/client/opengl/BUILD.gn:1: shaders = [ This is how the script will be used. Shaders don't exist in this CL... https://codereview.chromium.org/2036023003/diff/1/remoting/tools/build/remoti... File remoting/tools/build/remoting_c_text_inliner.py (right): https://codereview.chromium.org/2036023003/diff/1/remoting/tools/build/remoti... remoting/tools/build/remoting_c_text_inliner.py:40: return line.encode('string-escape').replace('"', '\\"') Forgot to leave comment for this. encode() escapes single quote but doesn't escape double quote, so it needs to be manually escaped... https://codereview.chromium.org/2036023003/diff/1/remoting/tools/build/remoti... remoting/tools/build/remoting_c_text_inliner.py:69: # a, b -> "a, "b -> "a\n" oops... Should be "a, "b -> "a\n", "b -> "a\n", "b"
Just fixed the comments...
https://codereview.chromium.org/2036023003/diff/20001/remoting/client/opengl/... File remoting/client/opengl/BUILD.gn (right): https://codereview.chromium.org/2036023003/diff/20001/remoting/client/opengl/... remoting/client/opengl/BUILD.gn:7: script = "../../tools/build/remoting_c_text_inliner.py" I think the script name would be better as something like shader_to_header.py. It doesn't need to begin with remoting_ because it's implicit from its location in the source, but it should say something about what the input is. https://codereview.chromium.org/2036023003/diff/20001/remoting/client/opengl/... remoting/client/opengl/BUILD.gn:10: include_guard = "REMOTING_CLIENT_OPENGL_SHADERS_H_" Using the same include guard for every invocation of this rule means it can effectively only be used once. I think it would be better to autogenerate it (randomly) in the script itself. https://codereview.chromium.org/2036023003/diff/20001/remoting/tools/build/re... File remoting/tools/build/remoting_c_text_inliner.py (right): https://codereview.chromium.org/2036023003/diff/20001/remoting/tools/build/re... remoting/tools/build/remoting_c_text_inliner.py:8: Example: Make it clear here that a.vert and b.frag are input files. https://codereview.chromium.org/2036023003/diff/20001/remoting/tools/build/re... remoting/tools/build/remoting_c_text_inliner.py:25: + "2\n" You can't append string like this in C. Just omitting the '+' should do what you want, since these are string literals. https://codereview.chromium.org/2036023003/diff/20001/remoting/tools/build/re... remoting/tools/build/remoting_c_text_inliner.py:52: output_path = sys.argv[2] Don't need blank lines between these. https://codereview.chromium.org/2036023003/diff/20001/remoting/tools/build/re... remoting/tools/build/remoting_c_text_inliner.py:63: input_file = open(input_path, 'r') You can use the "with" keyword to automatically close this when a Python scope ends. https://codereview.chromium.org/2036023003/diff/20001/remoting/tools/build/re... remoting/tools/build/remoting_c_text_inliner.py:67: .replace('.', '') Since you're losing path information when converting to a variable name, I think you should validate that the names you're producing are all unique so that the build failure is more obvious. https://codereview.chromium.org/2036023003/diff/20001/remoting/tools/build/re... remoting/tools/build/remoting_c_text_inliner.py:71: # a, b -> "a, "b -> "a\n" + "b -> "a\n" + "b" I don't understand this comment.
https://codereview.chromium.org/2036023003/diff/20001/remoting/client/opengl/... File remoting/client/opengl/BUILD.gn (right): https://codereview.chromium.org/2036023003/diff/20001/remoting/client/opengl/... remoting/client/opengl/BUILD.gn:10: include_guard = "REMOTING_CLIENT_OPENGL_SHADERS_H_" On 2016/06/04 00:24:37, Jamie wrote: > Using the same include guard for every invocation of this rule means it can > effectively only be used once. I think it would be better to autogenerate it > (randomly) in the script itself. Hmm... But I think this action should only be called once and there will only be one header file for the shaders? I agree that a randomly generated guard may be better...
PTAL https://codereview.chromium.org/2036023003/diff/20001/remoting/client/opengl/... File remoting/client/opengl/BUILD.gn (right): https://codereview.chromium.org/2036023003/diff/20001/remoting/client/opengl/... remoting/client/opengl/BUILD.gn:7: script = "../../tools/build/remoting_c_text_inliner.py" On 2016/06/04 00:24:37, Jamie wrote: > I think the script name would be better as something like shader_to_header.py. > It doesn't need to begin with remoting_ because it's implicit from its location > in the source, but it should say something about what the input is. Done. https://codereview.chromium.org/2036023003/diff/20001/remoting/client/opengl/... remoting/client/opengl/BUILD.gn:10: include_guard = "REMOTING_CLIENT_OPENGL_SHADERS_H_" On 2016/06/04 00:24:37, Jamie wrote: > Using the same include guard for every invocation of this rule means it can > effectively only be used once. I think it would be better to autogenerate it > (randomly) in the script itself. Done. https://codereview.chromium.org/2036023003/diff/20001/remoting/tools/build/re... File remoting/tools/build/remoting_c_text_inliner.py (right): https://codereview.chromium.org/2036023003/diff/20001/remoting/tools/build/re... remoting/tools/build/remoting_c_text_inliner.py:8: Example: On 2016/06/04 00:24:37, Jamie wrote: > Make it clear here that a.vert and b.frag are input files. Done. https://codereview.chromium.org/2036023003/diff/20001/remoting/tools/build/re... remoting/tools/build/remoting_c_text_inliner.py:25: + "2\n" On 2016/06/04 00:24:37, Jamie wrote: > You can't append string like this in C. Just omitting the '+' should do what you > want, since these are string literals. Done. https://codereview.chromium.org/2036023003/diff/20001/remoting/tools/build/re... remoting/tools/build/remoting_c_text_inliner.py:52: output_path = sys.argv[2] On 2016/06/04 00:24:37, Jamie wrote: > Don't need blank lines between these. Removed blank line between output_path and include_guard. I think it is okay to have blank line before open? https://codereview.chromium.org/2036023003/diff/20001/remoting/tools/build/re... remoting/tools/build/remoting_c_text_inliner.py:63: input_file = open(input_path, 'r') On 2016/06/04 00:24:37, Jamie wrote: > You can use the "with" keyword to automatically close this when a Python scope > ends. Done. https://codereview.chromium.org/2036023003/diff/20001/remoting/tools/build/re... remoting/tools/build/remoting_c_text_inliner.py:67: .replace('.', '') On 2016/06/04 00:24:37, Jamie wrote: > Since you're losing path information when converting to a variable name, I think > you should validate that the names you're producing are all unique so that the > build failure is more obvious. Done. Added checks. https://codereview.chromium.org/2036023003/diff/20001/remoting/tools/build/re... remoting/tools/build/remoting_c_text_inliner.py:71: # a, b -> "a, "b -> "a\n" + "b -> "a\n" + "b" On 2016/06/04 00:24:37, Jamie wrote: > I don't understand this comment. Just trying to explain how the strings are concatenated... Changed to more detailed comments.
https://codereview.chromium.org/2036023003/diff/60001/remoting/tools/build/sh... File remoting/tools/build/shader_to_header.py (right): https://codereview.chromium.org/2036023003/diff/60001/remoting/tools/build/sh... remoting/tools/build/shader_to_header.py:10: Input File in_a.vert: s/File/file/ https://codereview.chromium.org/2036023003/diff/60001/remoting/tools/build/sh... remoting/tools/build/shader_to_header.py:15: Output File in_b.frag: s/File/file/ https://codereview.chromium.org/2036023003/diff/60001/remoting/tools/build/sh... remoting/tools/build/shader_to_header.py:44: STRING_CHARACTERS = string.ascii_uppercase + \ Use parentheses instead of backslashes for line continuation: https://google.github.io/styleguide/pyguide.html?showone=Line_length#Line_length https://codereview.chromium.org/2036023003/diff/60001/remoting/tools/build/sh... remoting/tools/build/shader_to_header.py:94: # line. As discussed, there's no need for this file to be human-readable, so this can be simplified.
PTAL https://codereview.chromium.org/2036023003/diff/60001/remoting/tools/build/sh... File remoting/tools/build/shader_to_header.py (right): https://codereview.chromium.org/2036023003/diff/60001/remoting/tools/build/sh... remoting/tools/build/shader_to_header.py:10: Input File in_a.vert: On 2016/06/06 20:03:48, Jamie wrote: > s/File/file/ Done. https://codereview.chromium.org/2036023003/diff/60001/remoting/tools/build/sh... remoting/tools/build/shader_to_header.py:15: Output File in_b.frag: On 2016/06/06 20:03:47, Jamie wrote: > s/File/file/ Done. https://codereview.chromium.org/2036023003/diff/60001/remoting/tools/build/sh... remoting/tools/build/shader_to_header.py:44: STRING_CHARACTERS = string.ascii_uppercase + \ On 2016/06/06 20:03:47, Jamie wrote: > Use parentheses instead of backslashes for line continuation: > > https://google.github.io/styleguide/pyguide.html?showone=Line_length#Line_length Done. https://codereview.chromium.org/2036023003/diff/60001/remoting/tools/build/sh... remoting/tools/build/shader_to_header.py:94: # line. On 2016/06/06 20:03:47, Jamie wrote: > As discussed, there's no need for this file to be human-readable, so this can be > simplified. Done.
lgtm
The CQ bit was checked by yuweih@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2036023003/80001
lambroslambrou@chromium.org changed reviewers: + lambroslambrou@chromium.org
https://codereview.chromium.org/2036023003/diff/80001/remoting/tools/build/sh... File remoting/tools/build/shader_to_header.py (right): https://codereview.chromium.org/2036023003/diff/80001/remoting/tools/build/sh... remoting/tools/build/shader_to_header.py:45: return ''.join(random.choice(STRING_CHARACTERS) drive-by: How much do we trust Python's 'random' to seed with a different value each time? From help(random): "None or no argument seeds from current time or from an operating system specific randomness source if available." So if a builder has no available random source, it's conceivable that two different files might get the same header guard, introducing flakiness into the build. Maybe just derive the guard from "const_name" below, since that already has to be unique?
On 2016/06/06 21:17:46, Lambros wrote: > https://codereview.chromium.org/2036023003/diff/80001/remoting/tools/build/sh... > File remoting/tools/build/shader_to_header.py (right): > > https://codereview.chromium.org/2036023003/diff/80001/remoting/tools/build/sh... > remoting/tools/build/shader_to_header.py:45: return > ''.join(random.choice(STRING_CHARACTERS) > drive-by: > > How much do we trust Python's 'random' to seed with a different value each time? > From help(random): > "None or no argument seeds from current time or from an operating system > specific randomness source if available." > > So if a builder has no available random source, it's conceivable that two > different files might get the same header guard, introducing flakiness into the > build. > > Maybe just derive the guard from "const_name" below, since that already has to > be unique? That's a good point, although I don't think it is likely to be a problem since the name of the output file also contributes to the guard and we normally won't include the shader in a header file of a class... I think we can later consider hashing the const names and attaching the value to the guard.
On 2016/06/06 21:43:24, Yuwei wrote: > On 2016/06/06 21:17:46, Lambros wrote: > > > https://codereview.chromium.org/2036023003/diff/80001/remoting/tools/build/sh... > > File remoting/tools/build/shader_to_header.py (right): > > > > > https://codereview.chromium.org/2036023003/diff/80001/remoting/tools/build/sh... > > remoting/tools/build/shader_to_header.py:45: return > > ''.join(random.choice(STRING_CHARACTERS) > > drive-by: > > > > How much do we trust Python's 'random' to seed with a different value each > time? > > From help(random): > > "None or no argument seeds from current time or from an operating system > > specific randomness source if available." > > > > So if a builder has no available random source, it's conceivable that two > > different files might get the same header guard, introducing flakiness into > the > > build. > > > > Maybe just derive the guard from "const_name" below, since that already has to > > be unique? > > That's a good point, although I don't think it is likely to be a problem since > the name of the output file also contributes to the guard and we normally won't > include the shader in a header file of a class... Ah OK, hadn't noticed that!
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Script to inline shader into C header file OpenGL compiles shader code as text at runtime. It is inconvienent to edit shader code as const char[] inside the C++ source code. This CL contains script that reads file contents, turns them into const char[], and combine them into a header file. BUG=385924 ========== to ========== Script to inline shader into C header file OpenGL compiles shader code as text at runtime. It is inconvienent to edit shader code as const char[] inside the C++ source code. This CL contains script that reads file contents, turns them into const char[], and combine them into a header file. BUG=385924 Committed: https://crrev.com/26219410e3069f9cff75c5c487d2fc2d52f8ef4d Cr-Commit-Position: refs/heads/master@{#398142} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/26219410e3069f9cff75c5c487d2fc2d52f8ef4d Cr-Commit-Position: refs/heads/master@{#398142} |
