viz: Always initialize filter params for child resources.

Instead of assuming that the child initializes the filter on the
received resource to a known state, always initialize it before using.
If the client sends the resource with the specified |filter| on
TransferableResource, then it is returned with the same state. If the
client sends it in any other state, then it should assume that it
is returned in an unknown state.

R=sunnyps@chromium.org

Change-Id: Idd2cc884d27c053edd2ac48078faf324914cc8b0
Bug: 1018898
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1888951
Commit-Queue: Khushal <khushalsagar@chromium.org>
Auto-Submit: Khushal <khushalsagar@chromium.org>
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Cr-Commit-Position: refs/heads/master@{#713317}

cc/test/render_pass_test_utils.cc

@@ -414,8 +414,8 @@ void AddOneOfEveryQuadTypeInDisplayResourceProvider(
 
   viz::SharedQuadState* shared_state2 =
       to_pass->CreateAndAppendSharedQuadState();
-  shared_state->SetAll(gfx::Transform(), rect, rect, gfx::RRectF(), rect, false,
-                       false, 1, SkBlendMode::kSrcOver, 0);
+  shared_state2->SetAll(gfx::Transform(), rect, rect, gfx::RRectF(), rect,
+                        false, false, 1, SkBlendMode::kSrcOver, 0);
 
   viz::TileDrawQuad* tile_quad =
       to_pass->CreateAndAppendDrawQuad<viz::TileDrawQuad>();

components/viz/service/display/display_resource_provider.cc

@@ -684,9 +684,10 @@ void DisplayResourceProvider::DeleteAndReturnUnusedResourcesToChild(
       image_contexts_to_return.emplace_back(std::move(resource.image_context));
 
     if (resource.is_gpu_resource_type() &&
+        resource.gl_id &&
         resource.filter != resource.transferable.filter) {
       DCHECK(resource.transferable.mailbox_holder.texture_target);
-      DCHECK(resource.gl_id);
+      DCHECK(!resource.ShouldWaitSyncToken());
       DCHECK(gl);
       gl->BindTexture(resource.transferable.mailbox_holder.texture_target,
                       resource.gl_id);
@@ -1045,9 +1046,7 @@ DisplayResourceProvider::Child::~Child() = default;
 DisplayResourceProvider::ChildResource::ChildResource(
     int child_id,
     const TransferableResource& transferable)
-    : child_id(child_id),
-      transferable(transferable),
-      filter(transferable.filter) {
+    : child_id(child_id), transferable(transferable), filter(GL_NONE) {
   if (is_gpu_resource_type())
     UpdateSyncToken(transferable.mailbox_holder.sync_token);
   else

components/viz/service/display/gl_renderer_unittest.cc

@@ -1127,6 +1127,11 @@ class TextureStateTrackingGLES2Interface : public TestGLES2Interface {
   GLenum active_texture_;
 };
 
+#define EXPECT_FILTER_CALL(filter)                                          \
+  EXPECT_CALL(*gl,                                                          \
+              TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, filter)); \
+  EXPECT_CALL(*gl, TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, filter));
+
 TEST_F(GLRendererTest, ActiveTextureState) {
   auto child_gl_owned = std::make_unique<TextureStateTrackingGLES2Interface>();
   auto child_context_provider =
@@ -1194,17 +1199,34 @@ TEST_F(GLRendererTest, ActiveTextureState) {
     EXPECT_CALL(*gl, WaitSyncTokenCHROMIUM(_)).Times(7);
 
     // yuv_quad is drawn with the default linear filter.
+    for (int i = 0; i < 4; ++i) {
+      EXPECT_FILTER_CALL(GL_LINEAR);
+    }
     EXPECT_CALL(*gl, DrawElements(_, _, _, _));
 
     // tile_quad is drawn with GL_NEAREST because it is not transformed or
     // scaled.
-    EXPECT_CALL(
-        *gl, TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST));
-    EXPECT_CALL(
-        *gl, TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST));
-    // The remaining quads also use GL_LINEAR because nearest neighbor
-    // filtering is currently only used with tile quads.
-    EXPECT_CALL(*gl, DrawElements(_, _, _, _)).Times(8);
+    EXPECT_FILTER_CALL(GL_NEAREST);
+    EXPECT_CALL(*gl, DrawElements(_, _, _, _));
+
+    // transformed tile_quad
+    EXPECT_FILTER_CALL(GL_LINEAR);
+    EXPECT_CALL(*gl, DrawElements(_, _, _, _));
+
+    // scaled tile_quad
+    EXPECT_FILTER_CALL(GL_LINEAR);
+    EXPECT_CALL(*gl, DrawElements(_, _, _, _));
+
+    // texture_quad without nearest neighbor
+    EXPECT_FILTER_CALL(GL_LINEAR);
+    EXPECT_CALL(*gl, DrawElements(_, _, _, _));
+
+    // texture_quad without nearest neighbor
+    EXPECT_FILTER_CALL(GL_LINEAR);
+    EXPECT_CALL(*gl, DrawElements(_, _, _, _));
+
+    // stream video, solid color and debug draw quads
+    EXPECT_CALL(*gl, DrawElements(_, _, _, _)).Times(3);
   }
 
   gfx::Size viewport_size(100, 100);

third_party/blink/renderer/platform/graphics/canvas_resource.cc

@@ -99,6 +99,9 @@ static void ReleaseFrameResources(
     bool lost_resource) {
   resource->WaitSyncToken(sync_token);
 
+  if (resource_provider)
+    resource_provider->NotifyTexParamsModified(resource.get());
+
   // TODO(khushalsagar): If multiple readers had access to this resource, losing
   // it once should make sure subsequent releases don't try to recycle this
   // resource.
@@ -465,10 +468,6 @@ void CanvasResourceSharedImage::WillDraw() {
   if (!is_accelerated_)
     return;
 
-  // If skia is accessing the resource, it can modify the texture's filter
-  // params. Make sure to set them to the desired value before sending the
-  // resource to the display compositor.
-  owning_thread_data().needs_gl_filter_reset = true;
   owning_thread_data().mailbox_needs_new_sync_token = true;
 }
 
@@ -499,11 +498,6 @@ void CanvasResourceSharedImage::OnBitmapImageDestroyed(
     }
   }
 
-  // The StaticBitmapImage is used for readbacks which may modify the texture
-  // params. Note that this is racy, since the modification and resetting of the
-  // param is not atomic so the display may draw with incorrect params, but its
-  // a good enough fix for now.
-  resource->owning_thread_data().needs_gl_filter_reset = true;
   auto weak_provider = resource->WeakProvider();
   ReleaseFrameResources(std::move(weak_provider), std::move(resource),
                         sync_token, is_lost);
@@ -513,10 +507,6 @@ void CanvasResourceSharedImage::Transfer() {
   if (is_cross_thread() || !ContextProviderWrapper())
     return;
 
-  // Initialize GLFilter first so that the generated sync token includes this
-  // update.
-  SetGLFilterIfNeeded();
-
   // TODO(khushalsagar): This is for consistency with MailboxTextureHolder
   // transfer path. Its unclear why the verification can not be deferred until
   // the resource needs to be transferred cross-process.
@@ -578,13 +568,6 @@ scoped_refptr<StaticBitmapImage> CanvasResourceSharedImage::Bitmap() {
       context_provider_wrapper_, owning_thread_id_, is_origin_top_left_,
       std::move(release_callback));
 
-  // The StaticBitmapImage is used for readbacks which may modify the texture
-  // params. We reset this when the image is destroyed but it is important to
-  // also do it here in case we try to send the resource to the display
-  // compositor while the |image| is still alive.
-  if (!is_cross_thread())
-    owning_thread_data().needs_gl_filter_reset = true;
-
   DCHECK(image);
   return image;
 }
@@ -610,30 +593,11 @@ void CanvasResourceSharedImage::CopyRenderingResultsToGpuMemoryBuffer(
 const gpu::Mailbox& CanvasResourceSharedImage::GetOrCreateGpuMailbox(
     MailboxSyncMode sync_mode) {
   if (!is_cross_thread()) {
-    SetGLFilterIfNeeded();
     owning_thread_data().mailbox_sync_mode = sync_mode;
   }
   return mailbox();
 }
 
-void CanvasResourceSharedImage::SetGLFilterIfNeeded() {
-  DCHECK(!is_cross_thread());
-
-  if (!owning_thread_data().needs_gl_filter_reset || !ContextGL() ||
-      !WeakProvider())
-    return;
-
-  ContextGL()->BindTexture(texture_target_, GetTextureIdForReadAccess());
-  ContextGL()->TexParameteri(texture_target_, GL_TEXTURE_MIN_FILTER,
-                             GLFilter());
-  ContextGL()->TexParameteri(texture_target_, GL_TEXTURE_MAG_FILTER,
-                             GLFilter());
-  ContextGL()->BindTexture(texture_target_, 0u);
-  owning_thread_data().mailbox_needs_new_sync_token = true;
-  owning_thread_data().needs_gl_filter_reset = false;
-  Provider()->NotifyTexParamsModified(this);
-}
-
 bool CanvasResourceSharedImage::HasGpuMailbox() const {
   return !mailbox().IsZero();
 }
@@ -673,9 +637,6 @@ const gpu::SyncToken CanvasResourceSharedImage::GetSyncToken() {
 void CanvasResourceSharedImage::NotifyResourceLost() {
   owning_thread_data().is_lost = true;
 
-  // Since the texture params are in an unknown state, reset the cached tex
-  // params state for the resource.
-  owning_thread_data().needs_gl_filter_reset = true;
   if (WeakProvider())
     Provider()->NotifyTexParamsModified(this);
 }

third_party/blink/renderer/platform/graphics/canvas_resource.h

@@ -320,7 +320,6 @@ class PLATFORM_EXPORT CanvasResourceSharedImage final : public CanvasResource {
     bool mailbox_needs_new_sync_token = true;
     gpu::Mailbox shared_image_mailbox;
     gpu::SyncToken sync_token;
-    bool needs_gl_filter_reset = true;
     size_t bitmap_image_read_refs = 0u;
     MailboxSyncMode mailbox_sync_mode = kVerifiedSyncToken;
     bool is_lost = false;
@@ -359,7 +358,6 @@ class PLATFORM_EXPORT CanvasResourceSharedImage final : public CanvasResource {
                             bool is_origin_top_left,
                             bool allow_concurrent_read_write_access,
                             bool is_accelerated);
-  void SetGLFilterIfNeeded();
 
   OwningThreadData& owning_thread_data() {
     DCHECK_EQ(base::PlatformThread::CurrentId(), owning_thread_id_);