Fix broken draw/upload paths from videos to 2D canvas and WebGL.

media/renderers/skcanvas_video_renderer.cc

 // Copyright (c) 2012 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 "media/renderers/skcanvas_video_renderer.h"
 
 #include <GLES3/gl3.h>
 #include <limits>
 
 #include "base/macros.h"
@@ skipping 142 matching lines @@
                                            handles, yuvSizes,
                                            kTopLeft_GrSurfaceOrigin);
   }
   for (size_t i = 0; i < media::VideoFrame::NumPlanes(video_frame->format());
        ++i) {
     gl->DeleteTextures(1, &source_textures[i].fID);
   }
   return img;
 }
 
+bool VideoTextureNeedsClipping(const VideoFrame* video_frame) {
+  // There are multiple reasons that the size of the video frame's
+  // visible rectangle may differ from the coded size, including the
+  // encoder rounding up to the size of a macroblock, or use of
+  // non-square pixels.
+  //
+  // Some callers of these APIs (HTMLVideoElement and the 2D canvas
+  // context) already clip to the video frame's visible rectangle.
+  // WebGL on the other hand assumes that only the valid pixels are
+  // contained in the destination texture. This helper function
+  // determines whether this slower path is needed.
+  return video_frame->visible_rect().size() != video_frame->coded_size();

[sandersd (OOO until July 31), 2017/04/07 23:40:59]

Might be more clear as Rect(coded_size() == visible_rect().

However, seeing how this is being used, this code is assuming that coded_size ==
texture size. I am not certain that that is always true.

I'll take a look through the various callers and get back.

[Ken Russell (switch to Gerrit), 2017/04/07 23:48:18]

WebGL's the only consumer of the code paths where VideoTextureNeedsClipping
returns true. The intent of my earlier fix, and this revision, is to explicitly
make it so that the texture size isn't equal to the coded size.

[sandersd (OOO until July 31), 2017/04/07 23:51:53]

My question is about the source texture though. When going through the fast
path, the assumption seems to be that the size of the source texture is the same
as this coded_size?

[Ken Russell (switch to Gerrit), 2017/04/07 23:54:51]

Yes, that's my assumption. It appeared to be the case, especially when my
earlier fix failed. Is this incorrect?

[sandersd (OOO until July 31), 2017/04/08 00:01:25]

I'm not sure.

I believe it is true when we create textures from software planes (although
there are rounding issues there, I think they apply to coded_size as well).

I am less sure about the hardware decode paths. I *think* that they all allocate
using the coded_size, but it's not inconceivable that there are situations where
over-allocation is possible.

Then there are the strange cases of decoder-allocated textures. I think that's
just on Android now that the Mac path is a GLImage with a software conversion
path.

Android outputs TEXTURE_EXTERNAL textures, I have no idea how they are allocated
or what happens to them through this path.

[sandersd (OOO until July 31), 2017/04/08 00:05:04]

Upon reflection, I think Android is the only worrying case. The other cases must
match for our regular rendering path to produce correct output.

The Android case does not always have (0,0) -> (1,1) UV mapping for the
coded_size. We need to take its transformation matrix into account, and I don't
recall exactly how that path works.

[sandersd (OOO until July 31), 2017/04/08 00:07:14]

Relevant Android code is here:
https://cs.chromium.org/chromium/src/media/gpu/avda_codec_image.cc?type=cs&sq...

You can tell me whether that goes down any of these paths and whether it is
handled correctly.

+}
+
+gfx::Size AdjustedVideoTextureSize(const VideoFrame* video_frame) {

[sandersd (OOO until July 31), 2017/04/07 23:40:59]

Unused?

[Ken Russell (switch to Gerrit), 2017/04/07 23:48:18]

Fixed in next (current) patch set. I was still in the middle of compiling when I
uploaded this.

+  gfx::Size result = video_frame->natural_size();
+  result.SetToMin(video_frame->coded_size());
+  return result;
+}
+
 // Creates a SkImage from a |video_frame| backed by native resources.
 // The SkImage will take ownership of the underlying resource.
 sk_sp<SkImage> NewSkImageFromVideoFrameNative(VideoFrame* video_frame,
                                               const Context3D& context_3d) {
   DCHECK(PIXEL_FORMAT_ARGB == video_frame->format() ||
          PIXEL_FORMAT_XRGB == video_frame->format() ||
          PIXEL_FORMAT_NV12 == video_frame->format() ||
          PIXEL_FORMAT_UYVY == video_frame->format());
 
   const gpu::MailboxHolder& mailbox_holder = video_frame->mailbox_holder(0);
   DCHECK(mailbox_holder.texture_target == GL_TEXTURE_2D ||
          mailbox_holder.texture_target == GL_TEXTURE_RECTANGLE_ARB ||
          mailbox_holder.texture_target == GL_TEXTURE_EXTERNAL_OES)
       << mailbox_holder.texture_target;
 
   gpu::gles2::GLES2Interface* gl = context_3d.gl;
   unsigned source_texture = 0;
   if (mailbox_holder.texture_target != GL_TEXTURE_2D) {
     // TODO(dcastagna): At the moment Skia doesn't support targets different
     // than GL_TEXTURE_2D.  Avoid this copy once
     // https://code.google.com/p/skia/issues/detail?id=3868 is addressed.
     gl->GenTextures(1, &source_texture);
     DCHECK(source_texture);
     gl->BindTexture(GL_TEXTURE_2D, source_texture);
-    const gfx::Size& natural_size = video_frame->natural_size();
-    gl->TexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, natural_size.width(),
-                   natural_size.height(), 0, GL_RGBA, GL_UNSIGNED_BYTE,
-                   nullptr);
     SkCanvasVideoRenderer::CopyVideoFrameSingleTextureToGLTexture(
-        gl, video_frame, source_texture, true, false);
+        gl, video_frame,
+        SkCanvasVideoRenderer::SingleFrameForVideoElementOrCanvas,
+        source_texture, GL_RGBA, GL_UNSIGNED_BYTE, true, false);
   } else {
     gl->WaitSyncTokenCHROMIUM(mailbox_holder.sync_token.GetConstData());
     source_texture = gl->CreateAndConsumeTextureCHROMIUM(
         mailbox_holder.texture_target, mailbox_holder.mailbox.name);
   }
   GrBackendTextureDesc desc;
   desc.fFlags = kRenderTarget_GrBackendTextureFlag;
   desc.fOrigin = kTopLeft_GrSurfaceOrigin;
   desc.fWidth = video_frame->coded_size().width();
   desc.fHeight = video_frame->coded_size().height();
@@ skipping 543 matching lines @@
     case PIXEL_FORMAT_Y8:
     case PIXEL_FORMAT_UNKNOWN:
       NOTREACHED() << "Only YUV formats and Y16 are supported.";
   }
 }
 
 // static
 void SkCanvasVideoRenderer::CopyVideoFrameSingleTextureToGLTexture(
     gpu::gles2::GLES2Interface* gl,
     VideoFrame* video_frame,
+    SingleFrameCopyMode copy_mode,
     unsigned int texture,
+    unsigned int internal_format,
+    unsigned int type,
     bool premultiply_alpha,
     bool flip_y) {
   DCHECK(video_frame);
   DCHECK(video_frame->HasTextures());
 
   const gpu::MailboxHolder& mailbox_holder = video_frame->mailbox_holder(0);
   DCHECK(mailbox_holder.texture_target == GL_TEXTURE_2D ||
          mailbox_holder.texture_target == GL_TEXTURE_RECTANGLE_ARB ||
          mailbox_holder.texture_target == GL_TEXTURE_EXTERNAL_OES)
       << mailbox_holder.texture_target;
 
   gl->WaitSyncTokenCHROMIUM(mailbox_holder.sync_token.GetConstData());
   uint32_t source_texture = gl->CreateAndConsumeTextureCHROMIUM(
       mailbox_holder.texture_target, mailbox_holder.mailbox.name);
 
   // The video is stored in a unmultiplied format, so premultiply
   // if necessary.
   // Application itself needs to take care of setting the right |flip_y|
   // value down to get the expected result.
   // "flip_y == true" means to reverse the video orientation while
   // "flip_y == false" means to keep the intrinsic orientation.
 
-  // The video's texture might be larger than the natural size because
-  // the encoder might have had to round up to the size of a macroblock.
-  // Make sure to only copy the natural size to avoid putting garbage
-  // into the bottom of the destination texture.
-  const gfx::Size& natural_size = video_frame->natural_size();
-  gl->CopySubTextureCHROMIUM(source_texture, 0, GL_TEXTURE_2D, texture, 0, 0, 0,
-                             0, 0, natural_size.width(), natural_size.height(),
-                             flip_y, premultiply_alpha, false);
+  if (copy_mode == SingleFrameForVideoElementOrCanvas ||
+      !VideoTextureNeedsClipping(video_frame)) {
+    // No need to clip the source video texture.
+    gl->CopyTextureCHROMIUM(source_texture, 0, GL_TEXTURE_2D, texture, 0,
+                            internal_format, type, flip_y, premultiply_alpha,
+                            false);
+  } else {
+    // Must reallocate the destination texture and copy only a sub-portion.
+    gfx::Rect dest_rect = video_frame->visible_rect();
+#if DCHECK_IS_ON()
+    // The caller should have bound _texture_ to the GL_TEXTURE_2D
+    // binding point already.
+    GLuint current_texture = 0;
+    gl->GetIntegerv(GL_TEXTURE_BINDING_2D,
+                    reinterpret_cast<GLint*>(&current_texture));
+    DCHECK_EQ(current_texture, texture);
+    // There should always be enough data in the source texture to
+    // cover this copy.
+    DCHECK_LE(dest_rect.width(), video_frame->coded_size().width());
+    DCHECK_LE(dest_rect.height(), video_frame->coded_size().height());

[sandersd (OOO until July 31), 2017/04/07 23:40:59]

Perhaps better written as Rect(coded_size()).Contains(dest_rect).

+#endif
+    gl->TexImage2D(GL_TEXTURE_2D, 0, internal_format, dest_rect.width(),
+                   dest_rect.height(), 0, internal_format, type, nullptr);
+    gl->CopySubTextureCHROMIUM(source_texture, 0, GL_TEXTURE_2D, texture, 0, 0,
+                               0, dest_rect.x(), dest_rect.y(),
+                               dest_rect.width(), dest_rect.height(), flip_y,
+                               premultiply_alpha, false);
+  }
+
   gl->DeleteTextures(1, &source_texture);
   gl->Flush();
 
   SyncTokenClientImpl client(gl);
   video_frame->UpdateReleaseSyncToken(&client);
 }
 
 bool SkCanvasVideoRenderer::CopyVideoFrameTexturesToGLTexture(
     const Context3D& context_3d,
     gpu::gles2::GLES2Interface* destination_gl,
     const scoped_refptr<VideoFrame>& video_frame,
     unsigned int texture,
+    unsigned int internal_format,
+    unsigned int type,
     bool premultiply_alpha,
     bool flip_y) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(video_frame);
   DCHECK(video_frame->HasTextures());
   if (media::VideoFrame::NumPlanes(video_frame->format()) > 1) {
     if (!context_3d.gr_context)
       return false;
     if (!UpdateLastImage(video_frame, context_3d))
       return false;
@@ skipping 16 matching lines @@
     canvas_gl->ShallowFlushCHROMIUM();
     canvas_gl->GenSyncTokenCHROMIUM(fence_sync,
                                     mailbox_holder.sync_token.GetData());
 
     destination_gl->WaitSyncTokenCHROMIUM(
         mailbox_holder.sync_token.GetConstData());
     uint32_t intermediate_texture =
         destination_gl->CreateAndConsumeTextureCHROMIUM(
             mailbox_holder.texture_target, mailbox_holder.mailbox.name);
 
-    // The video's texture might be larger than the natural size because
-    // the encoder might have had to round up to the size of a macroblock.
-    // Make sure to only copy the natural size to avoid putting garbage
-    // into the bottom of the destination texture.
-    const gfx::Size& natural_size = video_frame->natural_size();
-    destination_gl->CopySubTextureCHROMIUM(
-        intermediate_texture, 0, GL_TEXTURE_2D, texture, 0, 0, 0, 0, 0,
-        natural_size.width(), natural_size.height(), flip_y, premultiply_alpha,
-        false);
+    // See whether the source video texture must be clipped.
+    if (VideoTextureNeedsClipping(video_frame.get())) {
+      // Reallocate destination texture and copy only valid region.
+      gfx::Size dest_rect = video_frame->visible_rect();
+#if DCHECK_IS_ON()
+      // The caller should have bound _texture_ to the GL_TEXTURE_2D
+      // binding point already.
+      GLuint current_texture = 0;
+      destination_gl->GetIntegerv(GL_TEXTURE_BINDING_2D,
+                                  reinterpret_cast<GLint*>(&current_texture));
+      DCHECK_EQ(current_texture, texture);
+      // There should always be enough data in the source texture to
+      // cover this copy.
+      DCHECK_LE(dest_rect.width(), video_frame->coded_size().width());
+      DCHECK_LE(dest_rect.height(), video_frame->coded_size().height());
+#endif
+      destination_gl->TexImage2D(GL_TEXTURE_2D, 0, internal_format,
+                                 dest_rect.width(), dest_rect.height(), 0,
+                                 internal_format, type, nullptr);
+      destination_gl->CopySubTextureCHROMIUM(
+          intermediate_texture, 0, GL_TEXTURE_2D, texture, 0, 0, 0,
+          dest_rect.x(), dest_rect.y(), dest_rect.width(), dest_rect.height(),
+          flip_y, premultiply_alpha, false);
+    } else {
+      destination_gl->CopyTextureCHROMIUM(
+          intermediate_texture, 0, GL_TEXTURE_2D, texture, 0, internal_format,
+          type, flip_y, premultiply_alpha, false);
+    }
+
     destination_gl->DeleteTextures(1, &intermediate_texture);
 
     // Wait for destination context to consume mailbox before deleting it in
     // canvas context.
     const GLuint64 dest_fence_sync = destination_gl->InsertFenceSyncCHROMIUM();
     destination_gl->ShallowFlushCHROMIUM();
     gpu::SyncToken dest_sync_token;
     destination_gl->GenSyncTokenCHROMIUM(dest_fence_sync,
                                          dest_sync_token.GetData());
     canvas_gl->WaitSyncTokenCHROMIUM(dest_sync_token.GetConstData());
 
     SyncTokenClientImpl client(canvas_gl);
     video_frame->UpdateReleaseSyncToken(&client);
   } else {
-    CopyVideoFrameSingleTextureToGLTexture(destination_gl, video_frame.get(),
-                                           texture, premultiply_alpha, flip_y);
+    CopyVideoFrameSingleTextureToGLTexture(
+        destination_gl, video_frame.get(), SingleFrameForWebGL, texture,
+        internal_format, type, premultiply_alpha, flip_y);
   }
 
   return true;
 }
 
 bool SkCanvasVideoRenderer::TexImage2D(unsigned target,
                                        gpu::gles2::GLES2Interface* gl,
                                        VideoFrame* frame,
                                        int level,
                                        int internalformat,
@@ skipping 87 matching lines @@
       last_image_->bounds().contains(visible_rect)) {
     last_image_ = last_image_->makeSubset(visible_rect);
   }
 }
 
 SkISize SkCanvasVideoRenderer::LastImageDimensionsForTesting() {
   return last_image_dimensions_for_testing_;
 }
 
 }  // namespace media