Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More fixes for specific sample apk. #1166

Merged
merged 4 commits into from
Oct 6, 2017
Merged

More fixes for specific sample apk. #1166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f944771
gapis/shadertools: Add Preamble & Relaxed options.
ben-clayton Oct 6, 2017
7d66c66
gles/compat: Use relaxed compilation for cross-compilation.
ben-clayton Oct 6, 2017
99ab27f
gles/compat: Handle missing glTexStorage2DMultisample().
ben-clayton Oct 6, 2017
83f2be2
gles/compat: Ignore glInvalidateSubFramebuffer.
ben-clayton Oct 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/shadertool/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ func run(ctx context.Context) error {
}

func convert(source, shaderType string) (result string, err error) {
opts := shadertools.Option{}
opts := shadertools.Options{}
switch shaderType {
case ".vert":
opts.ShaderType = shadertools.TypeVertex
Expand Down
3 changes: 3 additions & 0 deletions gapis/api/gles/api/gl.api
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,6 @@ cmd void glTexStorage1D(GLenum target, GLsizei levels, GLenum internalformat, GL

cmd void glBindFragDataLocation(ProgramId program, GLuint color, string name) {
}

cmd void glTexImage2DMultisample(GLenum target, GLsizei samples, GLenum internalformat, GLsizei width, GLsizei height, GLboolean fixedsamplelocations) {
}
24 changes: 19 additions & 5 deletions gapis/api/gles/compat.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,7 +454,10 @@ func compat(ctx context.Context, device *device.Instance) (transform.Transformer
if err != nil {
log.W(ctx, "%v compat: %v", cmd, err)
}
opts := shadertools.Option{ShaderType: st}
opts := shadertools.Options{
ShaderType: st,
Relaxed: true, // find_issues will still report bad GLSL.
}

// Trim any prefix whitespace / newlines.
// This isn't legal if it comes before the #version, but this
Expand Down Expand Up @@ -598,12 +601,19 @@ func compat(ctx context.Context, device *device.Instance) (transform.Transformer
return
}
case *GlTexStorage2DMultisample:
{
if version.IsES || version.AtLeastGL(4, 3) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would drop the 4.3 part of the condition. But that is probably just me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to steer the compat code towards doing the right checks. My longer term plan of action is to pull this logic out of this mega function and as methods of the command. We can then use these methods check to see if the particular command is supported by the given device - and use this to pick the most suitable device and/or show compatibility warnings in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with regard to checks - but in different way - ideally add some support for desktop GL into the api files as well, and then mutate the output through that to verify it should work. For a set of devices this would tell us which would fail (and how), and which would pass, all without trying to run it on the devices. As a poor man's version of this you could just record the minimal requirements as you execute the compat. So even without any replay device you would get transformed stream and the needed min spec.

With regards transforms - I prefer unconditional ones. That is, simplify complicated GL commands to simpler canonical ones. This is great example of that. There is no reason not to always simplify to more supported glTexImage2DMultisample version. Similarly all other *Storage methods can be simplified to more supported GL commands (although that is a bit more code, and we did not hit that compat need yet).

Anyway, it just my 1 cent. Feel free to follow your path. But if you get to situation where you can not reproduce a bug because the user's GL version and set of extensions follows a different path then yours, don't say I didn't warn you :-P (e.g. if the AtLeast(4,3) part of this block gets accidentally broken in the future as part of refactoring, you will never find out on your machine)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged.

// glTexStorage2DMultisample is supported by replay device.
cmd := *cmd
textureCompat.convertFormat(ctx, cmd.Target, &cmd.Internalformat, nil, nil, out, id, &cmd)
out.MutateAndWrite(ctx, id, &cmd)
return
} else {
// glTexStorage2DMultisample is not supported by replay device.
// Use glTexImage2DMultisample instead.
cmd := cb.GlTexImage2DMultisample(cmd.Target, cmd.Samples, cmd.Internalformat, cmd.Width, cmd.Height, cmd.Fixedsamplelocations)
textureCompat.convertFormat(ctx, cmd.Target, &cmd.Internalformat, nil, nil, out, id, cmd)
out.MutateAndWrite(ctx, id, cmd)
}
return
case *GlTexStorage3D:
{
cmd := *cmd
Expand Down Expand Up @@ -905,13 +915,17 @@ func compat(ctx context.Context, device *device.Instance) (transform.Transformer
// It has no effect on rendering so just drop it.
return

case *GlInvalidateFramebuffer,
*GlDiscardFramebufferEXT: // GL_EXT_discard_framebuffer
case *GlDiscardFramebufferEXT: // GL_EXT_discard_framebuffer
// It may not be implemented by the replay driver.
// It is only a hint so we can just drop it.
// TODO: It has performance impact so we should not ignore it when profiling.
return

case *GlInvalidateFramebuffer, *GlInvalidateSubFramebuffer:
if !version.AtLeastES(3, 0) || !version.AtLeastGL(4, 3) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I would prefer unconditional. You might disagree.

return // Not supported. Only a hint. Drop it.
}

case *GlMapBufferOES:
if !version.IsES { // Remove extension suffix on desktop.
cmd := cb.GlMapBuffer(cmd.Target, cmd.Access, memory.Pointer(cmd.Result))
Expand Down
2 changes: 1 addition & 1 deletion gapis/api/gles/find_issues.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ func (t *findIssues) Transform(ctx context.Context, id api.CmdID, cmd api.Cmd, o
t.onIssue(cmd, id, service.Severity_ErrorLevel, err)
return
}
opts := shadertools.Option{
opts := shadertools.Options{
ShaderType: st,
CheckAfterChanges: true,
Disassemble: true,
Expand Down
12 changes: 7 additions & 5 deletions gapis/shadertools/cc/libmanager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,11 +130,11 @@ void set_error_msg(code_with_debug_info_t* x, std::string msg) {
strcpy(x->message, msg.c_str());
}

std::vector<unsigned int> parseGlslang(const char* code, std::string* err_msg,
shader_type type, bool es_profile) {
std::vector<unsigned int> parseGlslang(const char* code, const char* preamble,
std::string* err_msg, shader_type type, bool es_profile, bool relaxed_errs) {
std::vector<unsigned int> spirv;

EShMessages messages = EShMsgDefault;
EShMessages messages = relaxed_errs ? EShMsgRelaxedErrors : EShMsgDefault;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, glslang has native support for relaxed compilation?
That makes this less of a hack. Good!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah - I spent a while trying to hack away, then found this. Happy days.

EShLanguage lang = EShLangVertex;
switch (type) {
case VERTEX: { lang = EShLangVertex; break; }
Expand All @@ -147,6 +147,7 @@ std::vector<unsigned int> parseGlslang(const char* code, std::string* err_msg,

glslang::InitializeProcess();
glslang::TShader shader(lang);
shader.setPreamble(preamble);
shader.setStrings(&code, 1);
// use 100 for ES environment, 330 for desktop
int default_version = es_profile ? 100 : 330;
Expand Down Expand Up @@ -186,7 +187,8 @@ code_with_debug_info_t* convertGlsl(const char* input, size_t length, const opti
code_with_debug_info_t* result = new code_with_debug_info_t{};
std::string err_msg;

std::vector<unsigned int> spirv = parseGlslang(input, &err_msg, options->shader_type, true);
std::vector<unsigned int> spirv = parseGlslang(
input, options->preamble, &err_msg, options->shader_type, true, options->relaxed);

if (!err_msg.empty()) {
set_error_msg(result, "Failed to parse original source code:\n" + err_msg);
Expand Down Expand Up @@ -239,7 +241,7 @@ code_with_debug_info_t* convertGlsl(const char* input, size_t length, const opti

// check if changed source code compiles again
if (options->check_after_changes) {
parseGlslang(result->source_code, &err_msg, options->shader_type, false);
parseGlslang(result->source_code, nullptr, &err_msg, options->shader_type, false, false);
}

if (!err_msg.empty()) {
Expand Down
2 changes: 2 additions & 0 deletions gapis/shadertools/cc/libmanager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,15 @@ typedef enum shader_type_t {
**/
typedef struct options_t {
shader_type shader_type;
const char* preamble; /* optional */
bool prefix_names;
const char* names_prefix; /* optional */
bool add_outputs_for_inputs;
const char* output_prefix; /* optional */
bool make_debuggable;
bool check_after_changes;
bool disassemble;
bool relaxed;
} options_t;

typedef struct spirv_binary_t {
Expand Down
57 changes: 35 additions & 22 deletions gapis/shadertools/shadertools.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,10 +103,12 @@ func (t ShaderType) String() string {
}
}

// Option controls how ConvertGlsl converts its passed-in GLSL source code.
type Option struct {
// Options controls how ConvertGlsl converts its passed-in GLSL source code.
type Options struct {
// The type of shader.
ShaderType ShaderType
// Shader source preamble.
Preamble string
// Whether to add prefix to all non-builtin symbols.
PrefixNames bool
// The name prefix to be added to all non-builtin symbols.
Expand All @@ -121,33 +123,44 @@ type Option struct {
CheckAfterChanges bool
// Whether to disassemble the generated GLSL code.
Disassemble bool
// If true, let some minor invalid statements compile.
Relaxed bool
}

// ConvertGlsl modifies the given GLSL according to the options specified via
// option and returns the modification status and result. Possible
// modifications includes creating output variables for input variables,
// prefixing all non-builtin symbols with a given prefix, etc.
func ConvertGlsl(source string, option *Option) (CodeWithDebugInfo, error) {
// o and returns the modification status and result. Possible modifications
// includes creating output variables for input variables, prefixing all
// non-builtin symbols with a given prefix, etc.
func ConvertGlsl(source string, o *Options) (CodeWithDebugInfo, error) {
toFree := []unsafe.Pointer{}
defer func() {
for _, ptr := range toFree {
C.free(ptr)
}
}()

mutex.Lock()
defer mutex.Unlock()

np := C.CString(option.NamesPrefix)
op := C.CString(option.OutputPrefix)
cstr := func(s string) *C.char {
out := C.CString(s)
toFree = append(toFree, unsafe.Pointer(out))
return out
}

opts := C.struct_options_t{
shader_type: C.shader_type(option.ShaderType),
prefix_names: C.bool(option.PrefixNames),
names_prefix: np,
add_outputs_for_inputs: C.bool(option.AddOutputsForInputs),
output_prefix: op,
make_debuggable: C.bool(option.MakeDebuggable),
check_after_changes: C.bool(option.CheckAfterChanges),
disassemble: C.bool(option.Disassemble),
shader_type: C.shader_type(o.ShaderType),
preamble: cstr(o.Preamble),
prefix_names: C.bool(o.PrefixNames),
names_prefix: cstr(o.NamesPrefix),
add_outputs_for_inputs: C.bool(o.AddOutputsForInputs),
output_prefix: cstr(o.OutputPrefix),
make_debuggable: C.bool(o.MakeDebuggable),
check_after_changes: C.bool(o.CheckAfterChanges),
disassemble: C.bool(o.Disassemble),
relaxed: C.bool(o.Relaxed),
}
csource := C.CString(source)
result := C.convertGlsl(csource, C.size_t(len(source)), &opts)
C.free(unsafe.Pointer(np))
C.free(unsafe.Pointer(op))
C.free(unsafe.Pointer(csource))
result := C.convertGlsl(cstr(source), C.size_t(len(source)), &opts)
defer C.deleteGlslCodeWithDebug(result)

ret := CodeWithDebugInfo{
Expand Down Expand Up @@ -175,7 +188,7 @@ func ConvertGlsl(source string, option *Option) (CodeWithDebugInfo, error) {

if !result.ok {
msg := []string{
fmt.Sprintf("Failed to convert %v shader.", option.ShaderType),
fmt.Sprintf("Failed to convert %v shader.", o.ShaderType),
}
if m := C.GoString(result.message); len(m) > 0 {
msg = append(msg, m)
Expand Down