RasterDecoder: only reset necessary gl state for DoCopySubTexture
Because RasterDecoder::DoCopySubTexture only depends on several pixel
unpack related state.
Bug: 902904
Change-Id: I08fe3996c85818c745605ca6e80c5af8fbf713a7
Reviewed-on: https://chromium-review.googlesource.com/c/1354135
Commit-Queue: Peng Huang <penghuang@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613511}diff --git a/gpu/command_buffer/service/feature_info.cc b/gpu/command_buffer/service/feature_info.cc
index 7932e09..84425e9 100644
--- a/gpu/command_buffer/service/feature_info.cc
+++ b/gpu/command_buffer/service/feature_info.cc
@@ -1493,7 +1493,10 @@
feature_flags_.ext_robustness =
gfx::HasExtension(extensions, "GL_EXT_robustness");
feature_flags_.ext_pixel_buffer_object =
+ gfx::HasExtension(extensions, "GL_ARB_pixel_buffer_object") ||
gfx::HasExtension(extensions, "GL_NV_pixel_buffer_object");
+ feature_flags_.ext_unpack_subimage =
+ gfx::HasExtension(extensions, "GL_EXT_unpack_subimage");
feature_flags_.oes_rgb8_rgba8 =
gfx::HasExtension(extensions, "GL_OES_rgb8_rgba8");
feature_flags_.angle_robust_resource_initialization =
diff --git a/gpu/command_buffer/service/feature_info.h b/gpu/command_buffer/service/feature_info.h
index 925fd2e..cb42ce97 100644
--- a/gpu/command_buffer/service/feature_info.h
+++ b/gpu/command_buffer/service/feature_info.h
@@ -127,6 +127,7 @@
bool khr_robustness = false;
bool ext_robustness = false;
bool ext_pixel_buffer_object = false;
+ bool ext_unpack_subimage = false;
bool oes_rgb8_rgba8 = false;
bool angle_robust_resource_initialization = false;
bool nv_fence = false;
diff --git a/gpu/command_buffer/service/raster_decoder.cc b/gpu/command_buffer/service/raster_decoder.cc
index 42aa52f..d3d5f1af3 100644
--- a/gpu/command_buffer/service/raster_decoder.cc
+++ b/gpu/command_buffer/service/raster_decoder.cc
@@ -162,36 +162,44 @@
class ScopedTextureBinder {
public:
ScopedTextureBinder(gles2::ContextState* state,
+ GLenum target,
+ GLuint texture,
+ GrContext* gr_context,
+ bool state_is_dirty)
+ : state_(state), target_(target), state_is_dirty_(state_is_dirty) {
+ auto* api = state->api();
+ api->glActiveTextureFn(GL_TEXTURE0);
+ api->glBindTextureFn(target_, texture);
+ if (gr_context)
+ gr_context->resetContext(kTextureBinding_GrGLBackendState);
+ }
+
+ ScopedTextureBinder(gles2::ContextState* state,
gles2::TextureManager* texture_manager,
gles2::TextureRef* texture_ref,
GLenum target,
GrContext* gr_context,
bool state_is_dirty)
- : state_(state),
- target_(target),
- gr_context_(gr_context),
- state_is_dirty_(state_is_dirty) {
- auto* api = state->api();
- api->glActiveTextureFn(GL_TEXTURE0);
- gles2::Texture* texture = texture_ref->texture();
- if (texture->target() == 0)
+ : ScopedTextureBinder(state,
+ target,
+ texture_ref->texture()->service_id(),
+ gr_context,
+ state_is_dirty) {
+ auto* texture = texture_ref->texture();
+ if (!texture->target())
texture_manager->SetTarget(texture_ref, target);
DCHECK_EQ(texture->target(), target_)
<< "Texture bound to more than 1 target.";
- api->glBindTextureFn(target_, texture_ref->service_id());
}
~ScopedTextureBinder() {
if (!state_is_dirty_)
state_->api()->glBindTextureFn(target_, 0);
- if (gr_context_)
- gr_context_->resetContext(kTextureBinding_GrGLBackendState);
}
private:
gles2::ContextState* state_;
GLenum target_;
- GrContext* gr_context_;
const bool state_is_dirty_;
DISALLOW_COPY_AND_ASSIGN(ScopedTextureBinder);
@@ -202,14 +210,29 @@
// scope.
class ScopedPixelUnpackState {
public:
- explicit ScopedPixelUnpackState(gles2::ContextState* state) : state_(state) {
- DCHECK(state_);
- state_->PushTextureUnpackState();
+ explicit ScopedPixelUnpackState(gles2::ContextState* state,
+ GrContext* gr_context,
+ const gles2::FeatureInfo* feature_info) {
+ DCHECK(state);
+ auto* api = state->api();
+ api->glPixelStoreiFn(GL_UNPACK_ALIGNMENT, 4);
+ if (feature_info->gl_version_info().is_es3 ||
+ feature_info->gl_version_info().is_desktop_core_profile ||
+ feature_info->feature_flags().ext_pixel_buffer_object)
+ api->glBindBufferFn(GL_PIXEL_UNPACK_BUFFER, 0);
+
+ if (feature_info->gl_version_info().is_es3 ||
+ feature_info->gl_version_info().is_desktop_core_profile ||
+ feature_info->feature_flags().ext_unpack_subimage)
+ api->glPixelStoreiFn(GL_UNPACK_ROW_LENGTH, 0);
+ if (gr_context) {
+ gr_context->resetContext(kMisc_GrGLBackendState |
+ kPixelStore_GrGLBackendState);
+ }
}
- ~ScopedPixelUnpackState() { state_->RestoreUnpackState(); }
+ ~ScopedPixelUnpackState() = default;
private:
- gles2::ContextState* state_;
DISALLOW_COPY_AND_ASSIGN(ScopedPixelUnpackState);
};
@@ -231,84 +254,13 @@
}
}
-// Commands that do not require that GL state matches ContextState. Some
-// are completely indifferent to GL state. Others require that GL state
-// matches GrContext state tracking.
-bool PermitsInconsistentContextState(CommandId command) {
- // Note that state restoration is expensive. If you're adding any new
- // command which is frequently used between multiple RasterCHROMIUMs for
- // tiled rasterization, make sure to add it to the whitelist below for
- // commands which don't need consistent GL state.
- switch (command) {
- case kBeginQueryEXT:
- case kBeginRasterCHROMIUMImmediate:
- case kCreateAndConsumeTextureINTERNALImmediate:
- case kCreateTransferCacheEntryINTERNAL:
- case kDeleteQueriesEXTImmediate:
- case kDeleteTexturesImmediate:
- case kDeleteTransferCacheEntryINTERNAL:
- case kEndQueryEXT:
- case kEndRasterCHROMIUM:
- case kFinish:
- case kFlush:
- case kGenQueriesEXTImmediate:
- case kGetError:
- case kInsertFenceSyncCHROMIUM:
- case kRasterCHROMIUM:
- case kSetActiveURLCHROMIUM:
- case kUnlockTransferCacheEntryINTERNAL:
- case kWaitSyncTokenCHROMIUM:
- case kTraceBeginCHROMIUM:
- case kTraceEndCHROMIUM:
- case kDeletePaintCacheTextBlobsINTERNALImmediate:
- case kDeletePaintCachePathsINTERNALImmediate:
- case kClearPaintCacheINTERNAL:
- return true;
- case kLoseContextCHROMIUM:
- case kCopySubTexture:
- return false;
- case kNumCommands:
- case kOneBeforeStartPoint:
- NOTREACHED();
- return false;
- }
-
- NOTREACHED();
- return false;
-}
-
} // namespace
// RasterDecoderImpl uses two separate state trackers (gpu::gles2::ContextState
// and GrContext) that cache the current GL driver state. Each class sees a
// fraction of the GL calls issued and can easily become inconsistent with GL
// state. We guard against that by resetting. But resetting is expensive, so we
-// avoid it as much as possible. The argument for correctness is as follows:
-//
-// - GLES2Decoder: The GL state matches the ContextState before and after a
-// command is executed here. The interesting case is making a GLES2Decoder
-// current. If using a virtual context, we will restore state appropriately
-// when the GLES2Decoder is made current because of the call to
-// RasterDecoderImpl::GetContextState.
-//
-// - RasterDecoder: There are two cases to consider
-//
-// Case 1: Making a RasterDecoder current. If we are using virtual contexts,
-// we will restore to |state_| and GrContext::resetContext because of
-// RasterDecoderImpl::{GetContextState,RestoreState}. If not, we will
-// restore to the previous GL state (either |state_| or GrContext consistent
-// with previous GL state).
-//
-// Case 2a: Executing a PermitsInconsistentContextState command: Either the
-// command doesn't inspect/modify GL state (InsertSyncPoint,
-// CreateAndConsumeTexture) or it requires and maintains that GrContext
-// state tracking matches GL context state (e.g. *RasterCHROMIUM --- see
-// raster_decoder_context_state_->PessimisticallyResetGrContext).
-//
-// Case 2b: Executing a command that is not whitelisted: We force GL state to
-// match |state_| as necessary (see |need_context_state_reset|) in
-// DoCommandsImpl with RestoreState(nullptr). This will call
-// GrContext::resetContext.
+// avoid it as much as possible.
class RasterDecoderImpl final : public RasterDecoder,
public gles2::ErrorStateClient,
public ServiceFontManager::Client {
@@ -1348,11 +1300,6 @@
continue;
}
}
- if (!PermitsInconsistentContextState(command)) {
- if (raster_decoder_context_state_->need_context_state_reset) {
- RestoreState(nullptr);
- }
- }
unsigned int info_arg_count = static_cast<unsigned int>(info.arg_count);
if ((info.arg_flags == cmd::kFixed && arg_count == info_arg_count) ||
(info.arg_flags == cmd::kAtLeastN && arg_count >= info_arg_count)) {
@@ -1482,9 +1429,10 @@
uint32_t size;
uint32_t padded_row_size;
+ constexpr GLint unpack_alignment = 4;
if (!gles2::GLES2Util::ComputeImageDataSizes(width, height, 1, format, type,
- state_.unpack_alignment, &size,
- nullptr, &padded_row_size)) {
+ unpack_alignment, &size, nullptr,
+ &padded_row_size)) {
return false;
}
@@ -1501,23 +1449,29 @@
DCHECK_GT(padded_row_size, 0U);
tile_height = kMaxZeroSize / padded_row_size;
if (!gles2::GLES2Util::ComputeImageDataSizes(width, tile_height, 1, format,
- type, state_.unpack_alignment,
- &size, nullptr, nullptr)) {
+ type, unpack_alignment, &size,
+ nullptr, nullptr)) {
return false;
}
} else {
tile_height = height;
}
- api()->glBindTextureFn(texture->target(), texture->service_id());
{
+ const bool state_is_dirty =
+ raster_decoder_context_state_->need_context_state_reset;
+ ScopedTextureBinder binder(&state_, texture->target(),
+ texture->service_id(), gr_context(),
+ state_is_dirty);
+ base::Optional<ScopedPixelUnpackState> pixel_unpack_state;
+ if (raster_decoder_context_state_->need_context_state_reset) {
+ pixel_unpack_state.emplace(&state_, gr_context(), group_->feature_info());
+ }
// Add extra scope to destroy zero and the object it owns right
// after its usage.
// Assumes the size has already been checked.
std::unique_ptr<char[]> zero(new char[size]);
memset(zero.get(), 0, size);
-
- ScopedPixelUnpackState reset_restore(&state_);
GLint y = 0;
while (y < height) {
GLint h = y + tile_height > height ? height - y : tile_height;
@@ -1529,12 +1483,6 @@
}
}
DCHECK(glGetError() == GL_NO_ERROR);
-
- if (gr_context()) {
- gr_context()->resetContext(kPixelStore_GrGLBackendState |
- kTextureBinding_GrGLBackendState);
- }
-
return true;
}
@@ -1544,48 +1492,8 @@
unsigned format,
int width,
int height) {
- DCHECK(target != GL_TEXTURE_3D && target != GL_TEXTURE_2D_ARRAY);
- // This code path can only be called if the texture was originally
- // allocated via TexStorage2D. Note that TexStorage2D is exposed
- // internally for ES 2.0 contexts, but compressed texture support is
- // not part of that exposure.
- DCHECK(feature_info_->IsWebGL2OrES3Context());
-
- GLsizei bytes_required = 0;
- std::string error_str;
- if (!GetCompressedTexSizeInBytes("ClearCompressedTextureLevel", width, height,
- 1, format, &bytes_required,
- state_.GetErrorState())) {
- return false;
- }
-
- TRACE_EVENT1("gpu", "RasterDecoderImpl::ClearCompressedTextureLevel",
- "bytes_required", bytes_required);
-
- api()->glBindBufferFn(GL_PIXEL_UNPACK_BUFFER, 0);
- {
- // Add extra scope to destroy zero and the object it owns right
- // after its usage.
- std::unique_ptr<char[]> zero(new char[bytes_required]);
- memset(zero.get(), 0, bytes_required);
- api()->glBindTextureFn(texture->target(), texture->service_id());
- api()->glCompressedTexSubImage2DFn(target, level, 0, 0, width, height,
- format, bytes_required, zero.get());
- }
- gles2::TextureRef* bound_texture =
- texture_manager()->GetTextureInfoForTarget(&state_, texture->target());
- api()->glBindTextureFn(texture->target(),
- bound_texture ? bound_texture->service_id() : 0);
- gles2::Buffer* bound_buffer =
- buffer_manager()->GetBufferInfoForTarget(&state_, GL_PIXEL_UNPACK_BUFFER);
- if (bound_buffer) {
- api()->glBindBufferFn(GL_PIXEL_UNPACK_BUFFER, bound_buffer->service_id());
- }
-
- if (gr_context()) {
- gr_context()->resetContext(kTextureBinding_GrGLBackendState);
- }
- return true;
+ NOTREACHED();
+ return false;
}
int RasterDecoderImpl::DecoderIdForTest() {
@@ -1996,6 +1904,7 @@
ScopedTextureBinder binder(
&state_, texture_manager(), dest_texture_ref, dest_target, gr_context(),
raster_decoder_context_state_->need_context_state_reset);
+ base::Optional<ScopedPixelUnpackState> pixel_unpack_state;
int source_width = 0;
int source_height = 0;
@@ -2026,6 +1935,14 @@
"source texture bad dimensions");
return;
}
+
+ if (image->GetType() == gl::GLImage::Type::MEMORY &&
+ raster_decoder_context_state_->need_context_state_reset) {
+ // If the image is in shared memory, we may need upload the pixel data
+ // with SubTexImage2D, so we need reset pixel unpack state if gl context
+ // state has been touched by skia.
+ pixel_unpack_state.emplace(&state_, gr_context(), group_->feature_info());
+ }
} else {
if (!source_texture->GetLevelSize(source_target, 0 /* level */,
&source_width, &source_height, nullptr)) {
diff --git a/gpu/command_buffer/service/raster_decoder_unittest.cc b/gpu/command_buffer/service/raster_decoder_unittest.cc
index 8bd41e7..89d2836 100644
--- a/gpu/command_buffer/service/raster_decoder_unittest.cc
+++ b/gpu/command_buffer/service/raster_decoder_unittest.cc
@@ -138,6 +138,7 @@
}
TEST_P(RasterDecoderTest, CopyTexSubImage2DTwiceClearsUnclearedTexture) {
+ raster_decoder_context_state_->need_context_state_reset = true;
// Create uninitialized source texture.
GLuint source_texture_id = kNewClientId;
CreateFakeTexture(source_texture_id, kNewServiceId,
@@ -311,13 +312,6 @@
EXPECT_FALSE(error::IsError(ExecuteCmd(decoder2.get(), end_raster_cmd)));
EXPECT_TRUE(context_state_->need_context_state_reset);
- // Now process a command which requires consistent state.
- LoseContextCHROMIUM lose_context_cmd;
- lose_context_cmd.Init(GL_GUILTY_CONTEXT_RESET_ARB,
- GL_INNOCENT_CONTEXT_RESET_ARB);
- EXPECT_FALSE(error::IsError(ExecuteCmd(decoder2.get(), lose_context_cmd)));
- EXPECT_FALSE(context_state_->need_context_state_reset);
-
decoder1->Destroy(true);
context_state_->context->MakeCurrent(context_state_->surface.get());
decoder2->Destroy(true);
diff --git a/gpu/command_buffer/service/raster_decoder_unittest_base.cc b/gpu/command_buffer/service/raster_decoder_unittest_base.cc
index 5e8409f..0c9edddb 100644
--- a/gpu/command_buffer/service/raster_decoder_unittest_base.cc
+++ b/gpu/command_buffer/service/raster_decoder_unittest_base.cc
@@ -253,14 +253,13 @@
SetupInitCapabilitiesExpectations(group_->feature_info()->IsES3Capable());
SetupInitStateExpectations(group_->feature_info()->IsES3Capable());
- scoped_refptr<raster::RasterDecoderContextState> context_state =
- new raster::RasterDecoderContextState(
- new gl::GLShareGroup(), surface_, context_,
- feature_info->workarounds().use_virtualized_gl_contexts,
- base::DoNothing());
+ raster_decoder_context_state_ = new raster::RasterDecoderContextState(
+ new gl::GLShareGroup(), surface_, context_,
+ feature_info->workarounds().use_virtualized_gl_contexts,
+ base::DoNothing());
decoder_.reset(RasterDecoder::Create(this, command_buffer_service_.get(),
&outputter_, group_.get(),
- std::move(context_state)));
+ raster_decoder_context_state_));
decoder_->SetIgnoreCachedStateForTest(ignore_cached_state_for_test_);
decoder_->GetLogger()->set_log_synthesized_gl_errors(false);
@@ -400,7 +399,8 @@
.Times(Between(1, 2))
.RetiresOnSaturation();
EXPECT_CALL(*gl_, BindTexture(target, Ne(0U))).Times(1).RetiresOnSaturation();
- EXPECT_CALL(*gl_, BindTexture(target, 0)).Times(1).RetiresOnSaturation();
+ if (!raster_decoder_context_state_->need_context_state_reset)
+ EXPECT_CALL(*gl_, BindTexture(target, 0)).Times(1).RetiresOnSaturation();
}
void RasterDecoderTestBase::SetupClearTextureExpectations(
@@ -420,7 +420,7 @@
.Times(1)
.RetiresOnSaturation();
EXPECT_CALL(*gl_, PixelStorei(GL_UNPACK_ALIGNMENT, _))
- .Times(2)
+ .Times(1)
.RetiresOnSaturation();
if (bound_pixel_unpack_buffer) {
EXPECT_CALL(*gl_, BindBuffer(GL_PIXEL_UNPACK_BUFFER, _))
diff --git a/gpu/command_buffer/service/raster_decoder_unittest_base.h b/gpu/command_buffer/service/raster_decoder_unittest_base.h
index f34c5db5..1eae7d8 100644
--- a/gpu/command_buffer/service/raster_decoder_unittest_base.h
+++ b/gpu/command_buffer/service/raster_decoder_unittest_base.h
@@ -262,6 +262,8 @@
uint32_t immediate_buffer_[64];
const bool ignore_cached_state_for_test_;
+ scoped_refptr<raster::RasterDecoderContextState>
+ raster_decoder_context_state_;
private:
GpuPreferences gpu_preferences_;
