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

media/renderers/skcanvas_video_renderer.cc

Index: media/renderers/skcanvas_video_renderer.cc
diff --git a/media/renderers/skcanvas_video_renderer.cc b/media/renderers/skcanvas_video_renderer.cc
index 1e2e9b21ab1a7e25977f25aeb0a8452a56a533ca..6f9cb7c815b75af4782708c97fbfb6b31430e8f2 100644
--- a/media/renderers/skcanvas_video_renderer.cc
+++ b/media/renderers/skcanvas_video_renderer.cc
@@ -160,6 +160,26 @@ sk_sp<SkImage> NewSkImageFromVideoFrameYUVTextures(
   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,
@@ -184,12 +204,10 @@ sk_sp<SkImage> NewSkImageFromVideoFrameNative(VideoFrame* video_frame,
     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(
@@ -753,7 +771,10 @@ void SkCanvasVideoRenderer::ConvertVideoFrameToRGBPixels(
 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);
@@ -776,14 +797,35 @@ void SkCanvasVideoRenderer::CopyVideoFrameSingleTextureToGLTexture(
   // "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();
 
@@ -796,6 +838,8 @@ bool SkCanvasVideoRenderer::CopyVideoFrameTexturesToGLTexture(
     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());
@@ -832,15 +876,35 @@ bool SkCanvasVideoRenderer::CopyVideoFrameTexturesToGLTexture(
         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
@@ -855,8 +919,9 @@ bool SkCanvasVideoRenderer::CopyVideoFrameTexturesToGLTexture(
     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;