Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(116)

Issues Search
    My Issues | Starred     Open | Closed | All

Unified Diff: gpu/command_buffer/service/gles2_cmd_decoder.cc

Issue 2496813002: [Command buffer] workaround the framebuffer completeness bug for Intel Mac (Closed)
Patch Set: code refactoring per zmo@'s suggestion: move read/draw buffer adjustment into CheckBound{Read|Draw}FramebufferValid Created 4 years, 1 month ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« gpu/command_buffer/service/framebuffer_manager.cc ('K') | « gpu/command_buffer/service/framebuffer_manager.cc ('k') | gpu/config/gpu_driver_bug_list_json.cc » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: gpu/command_buffer/service/gles2_cmd_decoder.cc
diff --git a/gpu/command_buffer/service/gles2_cmd_decoder.cc b/gpu/command_buffer/service/gles2_cmd_decoder.cc
index 6f0d2e8db079242a81d3e81a1c5dcb2d04f1e9c2..3d087a5af7f8d071ad66343c7328fb6117029dde 100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder.cc
@@ -1298,9 +1298,14 @@ class GLES2DecoderImpl : public GLES2Decoder, public ErrorStateClient {
GLenum gl_error,
const char* func_name);
- bool CheckBoundDrawFramebufferValid(const char* func_name);
+ bool CheckBoundDrawFramebufferValid(
+ const char* func_name,
+ bool may_need_workaround);
Zhenyao Mo 2016/11/16 03:27:27 may_need_workaround should always be true. i.e.,
// Generates |gl_error| if the bound read fbo is incomplete.
- bool CheckBoundReadFramebufferValid(const char* func_name, GLenum gl_error);
+ bool CheckBoundReadFramebufferValid(
+ const char* func_name,
+ GLenum gl_error,
+ bool may_need_workaround);
Zhenyao Mo 2016/11/16 03:27:27 Same here, see my comments below.
// This is only used by DoBlitFramebufferCHROMIUM which operates read/draw
// framebuffer at the same time.
bool CheckBoundFramebufferValid(const char* func_name);
@@ -1431,6 +1436,18 @@ class GLES2DecoderImpl : public GLES2Decoder, public ErrorStateClient {
void DoBufferSubData(
GLenum target, GLintptr offset, GLsizeiptr size, const GLvoid * data);
+ // If the specified draw buffer have no image, return true.
+ // Otherwise, return false.
+ bool SpecifiedDrawBufferHasNoImage(GLint ii);
+
+ // A workaround to adjust read buffer and draw buffers to GL_NONE
+ // if the specified read buffer or draw buffers have missing images.
+ // Return false if read buffer has no image or all of the draw buffers
+ // have no image after adjustment. Then we can generate
+ // INVALID_OPERATION or return directly in the callers accordingly.
+ bool AdjustColorBufferAttachmentsIfNecessary(
Zhenyao Mo 2016/11/16 03:27:28 It should really be AdjustReadBufferAndDrawBuffers
+ bool read, bool draw);
+
// Wrapper for glCheckFramebufferStatus
GLenum DoCheckFramebufferStatus(GLenum target);
@@ -4274,7 +4291,16 @@ bool GLES2DecoderImpl::CheckFramebufferValid(
return true;
}
-bool GLES2DecoderImpl::CheckBoundDrawFramebufferValid(const char* func_name) {
+bool GLES2DecoderImpl::CheckBoundDrawFramebufferValid(
+ const char* func_name, bool may_need_workaround) {
+ if (may_need_workaround &&
+ workarounds().read_draw_buffers_missing_image &&
+ SupportsDrawBuffers() &&
+ framebuffer_state_.clear_state_dirty) {
+ if (!AdjustColorBufferAttachmentsIfNecessary(false, true)) {
+ return false;
+ }
+ }
GLenum target = features().chromium_framebuffer_multisample ?
GL_DRAW_FRAMEBUFFER : GL_FRAMEBUFFER;
Framebuffer* framebuffer = GetFramebufferInfoForTarget(target);
@@ -4298,7 +4324,17 @@ bool GLES2DecoderImpl::CheckBoundDrawFramebufferValid(const char* func_name) {
}
bool GLES2DecoderImpl::CheckBoundReadFramebufferValid(
- const char* func_name, GLenum gl_error) {
+ const char* func_name, GLenum gl_error, bool may_need_workaround) {
+ if (may_need_workaround &&
+ workarounds().read_draw_buffers_missing_image &&
+ SupportsDrawBuffers() &&
+ framebuffer_state_.clear_state_dirty) {
+ if (!AdjustColorBufferAttachmentsIfNecessary(true, false)) {
+ LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, func_name,
+ "no valid color image");
+ return false;
+ }
+ }
GLenum target = features().chromium_framebuffer_multisample ?
GL_READ_FRAMEBUFFER : GL_FRAMEBUFFER;
Framebuffer* framebuffer = GetFramebufferInfoForTarget(target);
@@ -6167,7 +6203,8 @@ bool GLES2DecoderImpl::GetHelper(
// GL error occur, we fall back to our internal implementation.
*num_written = 1;
if (!CheckBoundReadFramebufferValid("glGetIntegerv",
- GL_INVALID_OPERATION)) {
+ GL_INVALID_OPERATION,
+ false)) {
if (params) {
*params = 0;
}
@@ -7213,7 +7250,20 @@ error::Error GLES2DecoderImpl::HandleDeleteProgram(
error::Error GLES2DecoderImpl::DoClear(GLbitfield mask) {
const char* func_name = "glClear";
DCHECK(!ShouldDeferDraws());
- if (CheckBoundDrawFramebufferValid(func_name)) {
+ if ((mask & GL_COLOR_BUFFER_BIT) != 0 &&
Zhenyao Mo 2016/11/16 03:27:27 This is wrong. See below.
+ workarounds().read_draw_buffers_missing_image &&
+ SupportsDrawBuffers() &&
+ framebuffer_state_.clear_state_dirty) {
+ if (!AdjustColorBufferAttachmentsIfNecessary(false, true)) {
+ if ((mask & GL_DEPTH_BUFFER_BIT) == 0 &&
+ (mask & GL_STENCIL_BUFFER_BIT) == 0) {
+ return error::kNoError;
+ } else {
+ mask &= ~GL_COLOR_BUFFER_BIT;
+ }
+ }
+ }
+ if (CheckBoundDrawFramebufferValid(func_name, false)) {
ApplyDirtyState();
if (workarounds().gl_clear_broken) {
if (!BoundFramebufferHasDepthAttachment())
@@ -7240,8 +7290,14 @@ error::Error GLES2DecoderImpl::DoClear(GLbitfield mask) {
void GLES2DecoderImpl::DoClearBufferiv(GLenum buffer,
GLint drawbuffer,
const volatile GLint* value) {
+ if (workarounds().read_draw_buffers_missing_image &&
+ buffer == GL_COLOR &&
+ framebuffer_state_.clear_state_dirty) {
+ if (SpecifiedDrawBufferHasNoImage(drawbuffer))
Zhenyao Mo 2016/11/16 03:27:28 This is very wrong. It's not just the image that
+ return;
+ }
const char* func_name = "glClearBufferiv";
- if (!CheckBoundDrawFramebufferValid(func_name))
+ if (!CheckBoundDrawFramebufferValid(func_name, false))
return;
ApplyDirtyState();
@@ -7275,8 +7331,14 @@ void GLES2DecoderImpl::DoClearBufferiv(GLenum buffer,
void GLES2DecoderImpl::DoClearBufferuiv(GLenum buffer,
GLint drawbuffer,
const volatile GLuint* value) {
+ if (workarounds().read_draw_buffers_missing_image &&
+ buffer == GL_COLOR &&
+ framebuffer_state_.clear_state_dirty) {
+ if (SpecifiedDrawBufferHasNoImage(drawbuffer))
+ return;
+ }
const char* func_name = "glClearBufferuiv";
- if (!CheckBoundDrawFramebufferValid(func_name))
+ if (!CheckBoundDrawFramebufferValid(func_name, false))
return;
ApplyDirtyState();
@@ -7299,8 +7361,14 @@ void GLES2DecoderImpl::DoClearBufferuiv(GLenum buffer,
void GLES2DecoderImpl::DoClearBufferfv(GLenum buffer,
GLint drawbuffer,
const volatile GLfloat* value) {
+ if (workarounds().read_draw_buffers_missing_image &&
+ buffer == GL_COLOR &&
+ framebuffer_state_.clear_state_dirty) {
+ if (SpecifiedDrawBufferHasNoImage(drawbuffer))
+ return;
+ }
const char* func_name = "glClearBufferfv";
- if (!CheckBoundDrawFramebufferValid(func_name))
+ if (!CheckBoundDrawFramebufferValid(func_name, false))
return;
ApplyDirtyState();
@@ -7334,7 +7402,7 @@ void GLES2DecoderImpl::DoClearBufferfv(GLenum buffer,
void GLES2DecoderImpl::DoClearBufferfi(
GLenum buffer, GLint drawbuffer, GLfloat depth, GLint stencil) {
const char* func_name = "glClearBufferfi";
- if (!CheckBoundDrawFramebufferValid(func_name))
+ if (!CheckBoundDrawFramebufferValid(func_name, false))
return;
ApplyDirtyState();
@@ -7532,6 +7600,55 @@ void GLES2DecoderImpl::RestoreClearState() {
state_.scissor_height);
}
+bool GLES2DecoderImpl::SpecifiedDrawBufferHasNoImage(GLint ii) {
Zhenyao Mo 2016/11/16 03:27:27 No need for this function.
+ if (ii < 0 || ii >= static_cast<GLint>(group_->max_draw_buffers()))
+ return false;
+ const Framebuffer* framebuffer =
+ GetFramebufferInfoForTarget(GL_DRAW_FRAMEBUFFER);
+ if (!framebuffer)
+ return false;
+ GLenum attachment = static_cast<GLenum>(GL_COLOR_ATTACHMENT0 + ii);
+ if (!(framebuffer->GetAttachment(attachment)) &&
+ framebuffer->ColorBuffersHaveImage()) {
+ return true;
+ }
+ return false;
+}
+
+bool GLES2DecoderImpl::AdjustColorBufferAttachmentsIfNecessary(
+ bool read, bool draw) {
+ DCHECK(workarounds().read_draw_buffers_missing_image &&
+ SupportsDrawBuffers() &&
+ framebuffer_state_.clear_state_dirty);
+ if (read) {
+ const Framebuffer* framebuffer =
+ GetFramebufferInfoForTarget(GL_READ_FRAMEBUFFER);
+ if (framebuffer &&
+ framebuffer->ReadBufferHasNoImage() &&
+ framebuffer->ColorBuffersHaveImage()) {
+ glReadBuffer(GL_NONE);
+ return false;
+ }
+ } else {
+ GLsizei num = 0;
+ GLsizei total = group_->max_draw_buffers();
+ GLenum safe_bufs[16];
+ for (int i = 0; i < 16; ++i)
+ safe_bufs[i] = GL_NONE;
+ const Framebuffer* framebuffer =
+ GetFramebufferInfoForTarget(GL_DRAW_FRAMEBUFFER);
+ if (framebuffer &&
+ framebuffer->DrawBuffersHaveMissingImage(&num, safe_bufs) &&
+ framebuffer->ColorBuffersHaveImage()) {
+ glDrawBuffersARB(total, safe_bufs);
+ if (num == 0) {
+ return false;
+ }
+ }
+ }
+ return true;
+}
+
GLenum GLES2DecoderImpl::DoCheckFramebufferStatus(GLenum target) {
Framebuffer* framebuffer =
GetFramebufferInfoForTarget(target);
@@ -7542,6 +7659,21 @@ GLenum GLES2DecoderImpl::DoCheckFramebufferStatus(GLenum target) {
if (completeness != GL_FRAMEBUFFER_COMPLETE) {
return completeness;
}
+
+ bool has_image = true;
+ if (workarounds().read_draw_buffers_missing_image &&
+ SupportsDrawBuffers() &&
+ framebuffer_state_.clear_state_dirty) {
+ if (target == GL_READ_FRAMEBUFFER) {
+ has_image = AdjustColorBufferAttachmentsIfNecessary(true, false);
+ } else {
+ has_image = AdjustColorBufferAttachmentsIfNecessary(false, true);
+ }
+ if (!has_image) {
+ return GL_FRAMEBUFFER_COMPLETE;
+ }
+ }
+
return framebuffer->GetStatus(texture_manager(), target);
}
@@ -7832,6 +7964,19 @@ void GLES2DecoderImpl::DoBlitFramebufferCHROMIUM(
const char* func_name = "glBlitFramebufferCHROMIUM";
DCHECK(!ShouldDeferReads() && !ShouldDeferDraws());
+ if (workarounds().read_draw_buffers_missing_image &&
+ (mask & GL_COLOR_BUFFER_BIT) != 0 &&
+ SupportsDrawBuffers() &&
+ framebuffer_state_.clear_state_dirty) {
+ if (!AdjustColorBufferAttachmentsIfNecessary(false, true)) {
+ return;
+ }
+ if (!AdjustColorBufferAttachmentsIfNecessary(true, false)) {
+ LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, func_name,
+ "no valid color image");
+ return;
+ }
+ }
if (!CheckBoundFramebufferValid(func_name)) {
return;
}
@@ -9806,7 +9951,7 @@ error::Error GLES2DecoderImpl::DoDrawArrays(
LOCAL_SET_GL_ERROR(GL_INVALID_VALUE, function_name, "primcount < 0");
return error::kNoError;
}
- if (!CheckBoundDrawFramebufferValid(function_name)) {
+ if (!CheckBoundDrawFramebufferValid(function_name, true)) {
return error::kNoError;
}
// We have to check this here because the prototype for glDrawArrays
@@ -9970,7 +10115,7 @@ error::Error GLES2DecoderImpl::DoDrawElements(const char* function_name,
return error::kNoError;
}
- if (!CheckBoundDrawFramebufferValid(function_name)) {
+ if (!CheckBoundDrawFramebufferValid(function_name, true)) {
return error::kNoError;
}
@@ -11195,8 +11340,9 @@ error::Error GLES2DecoderImpl::HandleReadPixels(uint32_t immediate_data_size,
return error::kNoError;
}
- if (!CheckBoundReadFramebufferValid(
- func_name, GL_INVALID_FRAMEBUFFER_OPERATION)) {
+ if (!CheckBoundReadFramebufferValid(func_name,
+ GL_INVALID_FRAMEBUFFER_OPERATION,
+ true)) {
return error::kNoError;
}
GLenum src_internal_format = GetBoundReadFramebufferInternalFormat();
@@ -13829,7 +13975,8 @@ void GLES2DecoderImpl::DoCopyTexImage2D(
}
if (!CheckBoundReadFramebufferValid(func_name,
- GL_INVALID_FRAMEBUFFER_OPERATION)) {
+ GL_INVALID_FRAMEBUFFER_OPERATION,
+ true)) {
return;
}
@@ -14076,7 +14223,8 @@ void GLES2DecoderImpl::DoCopyTexSubImage2D(
}
if (!CheckBoundReadFramebufferValid(func_name,
- GL_INVALID_FRAMEBUFFER_OPERATION)) {
+ GL_INVALID_FRAMEBUFFER_OPERATION,
+ true)) {
return;
}
@@ -14192,7 +14340,8 @@ void GLES2DecoderImpl::DoCopyTexSubImage3D(
}
if (!CheckBoundReadFramebufferValid(func_name,
- GL_INVALID_FRAMEBUFFER_OPERATION)) {
+ GL_INVALID_FRAMEBUFFER_OPERATION,
+ true)) {
return;
}
@@ -18203,7 +18352,7 @@ error::Error GLES2DecoderImpl::HandleStencilFillPathCHROMIUM(
// This holds for other rendering functions, too.
return error::kNoError;
}
- if (!CheckBoundDrawFramebufferValid(kFunctionName))
+ if (!CheckBoundDrawFramebufferValid(kFunctionName, false))
return error::kNoError;
ApplyDirtyState();
glStencilFillPathNV(service_id, fill_mode, mask);
@@ -18226,7 +18375,7 @@ error::Error GLES2DecoderImpl::HandleStencilStrokePathCHROMIUM(
}
GLint reference = static_cast<GLint>(c.reference);
GLuint mask = static_cast<GLuint>(c.mask);
- if (!CheckBoundDrawFramebufferValid(kFunctionName))
+ if (!CheckBoundDrawFramebufferValid(kFunctionName, false))
return error::kNoError;
ApplyDirtyState();
glStencilStrokePathNV(service_id, reference, mask);
@@ -18251,7 +18400,7 @@ error::Error GLES2DecoderImpl::HandleCoverFillPathCHROMIUM(
GLuint service_id = 0;
if (!path_manager()->GetPath(static_cast<GLuint>(c.path), &service_id))
return error::kNoError;
- if (!CheckBoundDrawFramebufferValid(kFunctionName))
+ if (!CheckBoundDrawFramebufferValid(kFunctionName, false))
return error::kNoError;
ApplyDirtyState();
glCoverFillPathNV(service_id, cover_mode);
@@ -18277,7 +18426,7 @@ error::Error GLES2DecoderImpl::HandleCoverStrokePathCHROMIUM(
if (!path_manager()->GetPath(static_cast<GLuint>(c.path), &service_id))
return error::kNoError;
- if (!CheckBoundDrawFramebufferValid(kFunctionName))
+ if (!CheckBoundDrawFramebufferValid(kFunctionName, false))
return error::kNoError;
ApplyDirtyState();
glCoverStrokePathNV(service_id, cover_mode);
@@ -18307,7 +18456,7 @@ error::Error GLES2DecoderImpl::HandleStencilThenCoverFillPathCHROMIUM(
if (!path_manager()->GetPath(static_cast<GLuint>(c.path), &service_id))
return error::kNoError;
- if (!CheckBoundDrawFramebufferValid(kFunctionName))
+ if (!CheckBoundDrawFramebufferValid(kFunctionName, false))
return error::kNoError;
ApplyDirtyState();
glStencilThenCoverFillPathNV(service_id, fill_mode, mask, cover_mode);
@@ -18337,7 +18486,7 @@ error::Error GLES2DecoderImpl::HandleStencilThenCoverStrokePathCHROMIUM(
GLint reference = static_cast<GLint>(c.reference);
GLuint mask = static_cast<GLuint>(c.mask);
- if (!CheckBoundDrawFramebufferValid(kFunctionName))
+ if (!CheckBoundDrawFramebufferValid(kFunctionName, false))
return error::kNoError;
ApplyDirtyState();
glStencilThenCoverStrokePathNV(service_id, reference, mask, cover_mode);
@@ -18377,7 +18526,7 @@ error::Error GLES2DecoderImpl::HandleStencilFillPathInstancedCHROMIUM(
if (!v.GetTransforms(c, num_paths, transform_type, &transforms))
return v.error();
- if (!CheckBoundDrawFramebufferValid(kFunctionName))
+ if (!CheckBoundDrawFramebufferValid(kFunctionName, false))
return error::kNoError;
ApplyDirtyState();
glStencilFillPathInstancedNV(num_paths, GL_UNSIGNED_INT, paths.get(), 0,
@@ -18417,7 +18566,7 @@ error::Error GLES2DecoderImpl::HandleStencilStrokePathInstancedCHROMIUM(
GLint reference = static_cast<GLint>(c.reference);
GLuint mask = static_cast<GLuint>(c.mask);
- if (!CheckBoundDrawFramebufferValid(kFunctionName))
+ if (!CheckBoundDrawFramebufferValid(kFunctionName, false))
return error::kNoError;
ApplyDirtyState();
glStencilStrokePathInstancedNV(num_paths, GL_UNSIGNED_INT, paths.get(), 0,
@@ -18456,7 +18605,7 @@ error::Error GLES2DecoderImpl::HandleCoverFillPathInstancedCHROMIUM(
if (!v.GetTransforms(c, num_paths, transform_type, &transforms))
return v.error();
- if (!CheckBoundDrawFramebufferValid(kFunctionName))
+ if (!CheckBoundDrawFramebufferValid(kFunctionName, false))
return error::kNoError;
ApplyDirtyState();
glCoverFillPathInstancedNV(num_paths, GL_UNSIGNED_INT, paths.get(), 0,
@@ -18496,7 +18645,7 @@ error::Error GLES2DecoderImpl::HandleCoverStrokePathInstancedCHROMIUM(
if (!v.GetTransforms(c, num_paths, transform_type, &transforms))
return v.error();
- if (!CheckBoundDrawFramebufferValid(kFunctionName))
+ if (!CheckBoundDrawFramebufferValid(kFunctionName, false))
return error::kNoError;
ApplyDirtyState();
glCoverStrokePathInstancedNV(num_paths, GL_UNSIGNED_INT, paths.get(), 0,
@@ -18539,7 +18688,7 @@ error::Error GLES2DecoderImpl::HandleStencilThenCoverFillPathInstancedCHROMIUM(
if (!v.GetTransforms(c, num_paths, transform_type, &transforms))
return v.error();
- if (!CheckBoundDrawFramebufferValid(kFunctionName))
+ if (!CheckBoundDrawFramebufferValid(kFunctionName, false))
return error::kNoError;
ApplyDirtyState();
glStencilThenCoverFillPathInstancedNV(num_paths, GL_UNSIGNED_INT, paths.get(),
@@ -18583,7 +18732,7 @@ GLES2DecoderImpl::HandleStencilThenCoverStrokePathInstancedCHROMIUM(
GLint reference = static_cast<GLint>(c.reference);
GLuint mask = static_cast<GLuint>(c.mask);
- if (!CheckBoundDrawFramebufferValid(kFunctionName))
+ if (!CheckBoundDrawFramebufferValid(kFunctionName, false))
return error::kNoError;
ApplyDirtyState();
glStencilThenCoverStrokePathInstancedNV(
« gpu/command_buffer/service/framebuffer_manager.cc ('K') | « gpu/command_buffer/service/framebuffer_manager.cc ('k') | gpu/config/gpu_driver_bug_list_json.cc » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698