Always restore GL scissor box in GLRenderer::RestoreGLState()

cc/output/gl_renderer.cc

 // Copyright 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/output/gl_renderer.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
@@ skipping 362 matching lines @@
                        ResourceProvider* resource_provider,
                        TextureMailboxDeleter* texture_mailbox_deleter,
                        int highp_threshold_min)
     : DirectRenderer(settings, output_surface, resource_provider),
       offscreen_framebuffer_id_(0),
       shared_geometry_quad_(QuadVertexRect()),
       gl_(output_surface->context_provider()->ContextGL()),
       context_support_(output_surface->context_provider()->ContextSupport()),
       texture_mailbox_deleter_(texture_mailbox_deleter),
       is_scissor_enabled_(false),
-      scissor_rect_needs_reset_(true),
       stencil_shadow_(false),
       blend_shadow_(false),
       highp_threshold_min_(highp_threshold_min),
       highp_threshold_cache_(0),
       use_sync_query_(false),
       gl_composited_texture_quad_border_(
           settings->gl_composited_texture_quad_border),
       bound_geometry_(NO_BINDING),
       color_lut_cache_(gl_) {
   DCHECK(gl_);
@@ skipping 2823 matching lines @@
              GL_FRAMEBUFFER_COMPLETE ||
          IsContextLost());
   return true;
 }
 
 void GLRenderer::SetScissorTestRect(const gfx::Rect& scissor_rect) {
   EnsureScissorTestEnabled();
 
   // Don't unnecessarily ask the context to change the scissor, because it
   // may cause undesired GPU pipeline flushes.
-  if (scissor_rect == scissor_rect_ && !scissor_rect_needs_reset_)
+  if (scissor_rect == scissor_rect_) {
+#if DCHECK_IS_ON()
+    GLint actual_scissor[4];
+    gl_->GetIntegerv(GL_SCISSOR_BOX, actual_scissor);
+    DCHECK(scissor_rect.x() == actual_scissor[0] &&
+           scissor_rect.y() == actual_scissor[1] &&
+           scissor_rect.width() == actual_scissor[2] &&
+           scissor_rect.height() == actual_scissor[3]);
+#endif
     return;
+  }
 
   scissor_rect_ = scissor_rect;
   FlushTextureQuadCache(SHARED_BINDING);
   gl_->Scissor(scissor_rect.x(), scissor_rect.y(), scissor_rect.width(),
                scissor_rect.height());
-
-  scissor_rect_needs_reset_ = false;
 }
 
 void GLRenderer::SetViewport() {
   gl_->Viewport(current_window_space_viewport_.x(),
                 current_window_space_viewport_.y(),
                 current_window_space_viewport_.width(),
                 current_window_space_viewport_.height());
 }
 
 void GLRenderer::InitializeSharedObjects() {
@@ skipping 471 matching lines @@
   solid_color_program_aa_.Cleanup(gl_);
 
   if (offscreen_framebuffer_id_)
     gl_->DeleteFramebuffers(1, &offscreen_framebuffer_id_);
 
   ReleaseRenderPassTextures();
 }
 
 void GLRenderer::ReinitializeGLState() {
   is_scissor_enabled_ = false;
-  scissor_rect_needs_reset_ = true;
+  scissor_rect_ = gfx::Rect(0, 0, 0, 0);

[enne (OOO), 2016/11/21 00:47:44]

I think this is not correct.  If something sets the scissor to the empty rect,
then this will early out and not actually set the scissor.

[trchen, 2016/11/21 22:36:15]

Shouldn't it early out because the scissor rect is already reset to empty?

[enne (OOO), 2016/11/21 23:08:05]

Ok sure, I guess ReinitializeGLState always calls RestoreGLState.

   stencil_shadow_ = false;
   blend_shadow_ = true;
   program_shadow_ = 0;
 
   RestoreGLState();
 }
 
 void GLRenderer::RestoreGLState() {
   // This restores the current GLRenderer state to the GL context.
   bound_geometry_ = NO_BINDING;
@@ skipping 11 matching lines @@
   if (stencil_shadow_)
     gl_->Enable(GL_STENCIL_TEST);
   else
     gl_->Disable(GL_STENCIL_TEST);
 
   if (blend_shadow_)
     gl_->Enable(GL_BLEND);
   else
     gl_->Disable(GL_BLEND);
 
-  if (is_scissor_enabled_) {
+  if (is_scissor_enabled_)
     gl_->Enable(GL_SCISSOR_TEST);
-    gl_->Scissor(scissor_rect_.x(), scissor_rect_.y(), scissor_rect_.width(),
-                 scissor_rect_.height());
-  } else {
+  else
     gl_->Disable(GL_SCISSOR_TEST);
-  }
+
+  gl_->Scissor(scissor_rect_.x(), scissor_rect_.y(), scissor_rect_.width(),

[enne (OOO), 2016/11/21 00:47:44]

I think a patch that just does this line of code this would be great without the
other changes.

[trchen, 2016/11/21 22:36:15]

Sure it will still fix the bug. I don't see why the extra complexity is needed
though...

+               scissor_rect_.height());
 }
 
 bool GLRenderer::IsContextLost() {
   return gl_->GetGraphicsResetStatusKHR() != GL_NO_ERROR;
 }
 
 void GLRenderer::ScheduleCALayers(DrawingFrame* frame) {
   if (overlay_resource_pool_) {
     overlay_resource_pool_->CheckBusyResources();
   }
@@ skipping 269 matching lines @@
   // The alpha has already been applied when copying the RPDQ to an IOSurface.
   GLfloat alpha = 1;
   gl_->ScheduleCALayerSharedStateCHROMIUM(alpha, is_clipped, clip_rect,
                                           sorting_context_id, gl_transform);
   gl_->ScheduleCALayerCHROMIUM(
       texture_id, contents_rect, ca_layer_overlay->background_color,
       ca_layer_overlay->edge_aa_mask, bounds_rect, filter);
 }
 
 }  // namespace cc