From 7b0b4ba57d1755c38baa1f569760683b12c49940 Mon Sep 17 00:00:00 2001 From: dyen Date: Wed, 4 Feb 2015 13:38:06 -0800 Subject: [PATCH] Clean up some of the shader compilation code. This cL contains some micro optimizations as well as cleaning up some variables that are left over from old refactors. One variable in particular that was deleted is the "compiler_options_" member within the Shader class. This member was used to hold extra compiler options such as extensions for cache key purposes. However, at some point this mechanism has been replaced by querying the angle compiler directly for a string which represents the resources. BUG=453543 TEST=trybots Review URL: https://codereview.chromium.org/900543004 Cr-Commit-Position: refs/heads/master@{#314642} --- .../service/gles2_cmd_decoder.cc | 7 +++-- gpu/command_buffer/service/shader_manager.cc | 17 ++++++----- .../service/shader_translator.cc | 1 - .../service/shader_translator.h | 1 - .../tests/gl_program_unittest.cc | 29 +++++++++++++++++++ 5 files changed, 43 insertions(+), 12 deletions(-) diff --git a/gpu/command_buffer/service/gles2_cmd_decoder.cc b/gpu/command_buffer/service/gles2_cmd_decoder.cc index f37c61a4808d98..9ad26770273af8 100644 --- a/gpu/command_buffer/service/gles2_cmd_decoder.cc +++ b/gpu/command_buffer/service/gles2_cmd_decoder.cc @@ -7051,14 +7051,17 @@ void GLES2DecoderImpl::DoShaderSource( GLuint client_id, GLsizei count, const char** data, const GLint* length) { std::string str; for (GLsizei ii = 0; ii < count; ++ii) { - str.append(data[ii]); + if (length && length[ii] > 0) + str.append(data[ii], length[ii]); + else + str.append(data[ii]); } Shader* shader = GetShaderInfoNotProgram(client_id, "glShaderSource"); if (!shader) { return; } // Note: We don't actually call glShaderSource here. We wait until - // the call to glCompileShader. + // we actually compile the shader. shader->set_source(str); } diff --git a/gpu/command_buffer/service/shader_manager.cc b/gpu/command_buffer/service/shader_manager.cc index 2707b90b92de41..819b80faf03aea 100644 --- a/gpu/command_buffer/service/shader_manager.cc +++ b/gpu/command_buffer/service/shader_manager.cc @@ -61,13 +61,14 @@ void Shader::DoCompile(ShaderTranslatorInterface* translator, glGetShaderiv(service_id_, GL_TRANSLATED_SHADER_SOURCE_LENGTH_ANGLE, &max_len); - scoped_ptr buffer(new char[max_len]); + translated_source_.resize(max_len); GLint len = 0; glGetTranslatedShaderSourceANGLE( - service_id_, max_len, &len, buffer.get()); + service_id_, translated_source_.size(), + &len, &translated_source_.at(0)); DCHECK(max_len == 0 || len < max_len); - DCHECK(len == 0 || buffer[len] == '\0'); - translated_source_ = std::string(buffer.get(), len); + DCHECK(len == 0 || translated_source_[len] == '\0'); + translated_source_.resize(len); } GLint status = GL_FALSE; @@ -78,13 +79,13 @@ void Shader::DoCompile(ShaderTranslatorInterface* translator, // All translated shaders must compile. GLint max_len = 0; glGetShaderiv(service_id_, GL_INFO_LOG_LENGTH, &max_len); - scoped_ptr buffer(new char[max_len]); + log_info_.resize(max_len); GLint len = 0; - glGetShaderInfoLog(service_id_, max_len, &len, buffer.get()); + glGetShaderInfoLog(service_id_, log_info_.size(), &len, &log_info_.at(0)); DCHECK(max_len == 0 || len < max_len); - DCHECK(len == 0 || buffer[len] == '\0'); + DCHECK(len == 0 || log_info_[len] == '\0'); valid_ = false; - log_info_ = std::string(buffer.get(), len); + log_info_.resize(len); LOG_IF(ERROR, translator) << "Shader translator allowed/produced an invalid shader " << "unless the driver is buggy:" diff --git a/gpu/command_buffer/service/shader_translator.cc b/gpu/command_buffer/service/shader_translator.cc index 878490c3cb9d7e..98d738d74fd1bb 100644 --- a/gpu/command_buffer/service/shader_translator.cc +++ b/gpu/command_buffer/service/shader_translator.cc @@ -123,7 +123,6 @@ bool ShaderTranslator::Init( compiler_ = ShConstructCompiler( shader_type, shader_spec, shader_output, resources); } - compiler_options_ = *resources; implementation_is_glsl_es_ = (glsl_implementation_type == kGlslES); driver_bug_workarounds_ = driver_bug_workarounds; return compiler_ != NULL; diff --git a/gpu/command_buffer/service/shader_translator.h b/gpu/command_buffer/service/shader_translator.h index 6075896b66643e..ef25ebb5592ece 100644 --- a/gpu/command_buffer/service/shader_translator.h +++ b/gpu/command_buffer/service/shader_translator.h @@ -110,7 +110,6 @@ class GPU_EXPORT ShaderTranslator int GetCompileOptions() const; ShHandle compiler_; - ShBuiltInResources compiler_options_; bool implementation_is_glsl_es_; ShCompileOptions driver_bug_workarounds_; ObserverList destruction_observers_; diff --git a/gpu/command_buffer/tests/gl_program_unittest.cc b/gpu/command_buffer/tests/gl_program_unittest.cc index aec95b2f5e4ee9..186b28b8004eb9 100644 --- a/gpu/command_buffer/tests/gl_program_unittest.cc +++ b/gpu/command_buffer/tests/gl_program_unittest.cc @@ -121,6 +121,35 @@ TEST_F(GLProgramTest, NewShaderInCurrentProgram) { GLTestHelper::CheckGLError("no errors", __LINE__); } +TEST_F(GLProgramTest, ShaderLengthSpecified) { + const std::string valid_shader_str = SHADER( + attribute vec4 a_position; + void main() + { + gl_Position = a_position; + } + ); + + const std::string invalid_shader = valid_shader_str + "invalid suffix"; + + // Compiling invalid program should fail. + const char* invalid_shader_strings[] = { invalid_shader.c_str() }; + GLuint vs = glCreateShader(GL_VERTEX_SHADER); + glShaderSource(vs, 1, invalid_shader_strings, NULL); + glCompileShader(vs); + + GLint compile_state = 0; + glGetShaderiv(vs, GL_COMPILE_STATUS, &compile_state); + EXPECT_EQ(GL_FALSE, compile_state); + + // Compiling program cutting off invalid parts should succeed. + const GLint lengths[] = { valid_shader_str.length() }; + glShaderSource(vs, 1, invalid_shader_strings, lengths); + glCompileShader(vs); + glGetShaderiv(vs, GL_COMPILE_STATUS, &compile_state); + EXPECT_EQ(GL_TRUE, compile_state); +} + TEST_F(GLProgramTest, UniformsInCurrentProgram) { static const char* v_shader_str = SHADER( attribute vec4 a_position;