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..4131dac3b93f14353a96aaf9323a0f160ebd3e79 100644
--- a/media/renderers/skcanvas_video_renderer.cc
+++ b/media/renderers/skcanvas_video_renderer.cc
@@ -160,6 +160,25 @@ sk_sp<SkImage> NewSkImageFromVideoFrameYUVTextures(
   return img;
 }
 
+bool VideoTextureSizeNeedsAdjustment(const VideoFrame* video_frame) {
+  // There are multiple reasons that the video frame's natural size
+  // 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->natural_size() != video_frame->coded_size();

[sandersd (OOO until July 31), 2017/04/07 19:07:13]

I'm not sure which check you are intending here. The possible checks are:
  - visible_rect().size() != natural_size() => scaling is required if you want
the result to have 1:1 pixel aspect ratio. (<50% of videos)
  - visible_rect().size() != coded_size() => cropping is required (>50% of
videos)

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

Thanks for the info.

The reason I didn't look at the visible rectangle is that I don't understand its
semantics and we don't have enough test cases.

There isn't a primitive in Chromium's GPU subsystem to copy and stretch a
sub-rectangle of one texture into another: the available primitives are
CopyTextureCHROMIUM and CopySubTextureCHROMIUM. This restricts the possible
fixes for this bug.

The user is currently always going to think that the texture they get from
TexImage2D taking a video element is sized the natural width and height. I think
it's acceptable if we get the aspect ratio wrong.

Copying the smaller of the coded size and natural size into the user's texture
has worked around the bugs seen to date. Should I take the visible rectangle
into account, too? Can that be deferred until I have a test case for that?

[sandersd (OOO until July 31), 2017/04/07 20:58:06]

If we don't have the option of scaling, then the only correct option is to clip
to the |visible_rect|, thus returning a texture with a size different from
|natural_size|.

Other choices produce minor visual errors in common cases, significant visual
errors in less common cases, and generally unreliable results for any
computation that isn't visual.

The cases where this texture size distinction can matter to web developers are
small, and it's not clear to me that this implementation is wrong (it does
contradict your recently added test though).

[Ken Russell (switch to Gerrit), 2017/04/07 21:15:27]

Can the coded size or natural size be smaller (in width or height) than the
visible rectangle?
There were two tests added recently, the one for crbug.com/672895
(conformance2/textures/misc/npot-video-sizing.html) and the one for this bug
(conformance/textures/misc/texture-corner-case-videos.html). I'm pretty sure
texture-corner-case-videos is a correct test, and npot-video-sizing may be
overly strict and somewhat incorrect. Is this what you meant?

If you can think of a way to make npot-video-sizing less fragile, I'll be happy
to encode a new video that exercises things differently. Please send any
suggestions.

[sandersd (OOO until July 31), 2017/04/07 21:41:57]

Strict rules:
  - Visible rect must be a subset of coded_size.

Common practices:
  - Natural size is scaled to have one dimension match the visible rect size.
  - Natural size is fixed while the coded size may be smaller (and vary over
time).

It is uncommon for natural size to be smaller than the visible rect size in both
dimensions, but there is no rule.
That sounds correct, although I've not looked at everything that
texture-corner-case-videos.html verifies.

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

OK. Thanks for your feedback. I've revised the fix in this file.
Fingers crossed that the new logic still allows that test to pass.

+}
+
+gfx::Size AdjustedVideoTextureSize(const VideoFrame* video_frame) {
+  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 +203,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 +770,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 +796,30 @@ 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 ||
+      !VideoTextureSizeNeedsAdjustment(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::Size dest_size = AdjustedVideoTextureSize(video_frame);
+#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);
+#endif
+    gl->TexImage2D(GL_TEXTURE_2D, 0, internal_format, dest_size.width(),
+                   dest_size.height(), 0, internal_format, type, nullptr);
+    gl->CopySubTextureCHROMIUM(source_texture, 0, GL_TEXTURE_2D, texture, 0, 0,
+                               0, 0, 0, dest_size.width(), dest_size.height(),
+                               flip_y, premultiply_alpha, false);
+  }
+
   gl->DeleteTextures(1, &source_texture);
   gl->Flush();
 
@@ -796,6 +832,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 +870,31 @@ 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 (VideoTextureSizeNeedsAdjustment(video_frame.get())) {
+      // Reallocate destination texture and copy only valid region.
+      gfx::Size dest_size = AdjustedVideoTextureSize(video_frame.get());
+#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);
+#endif
+      destination_gl->TexImage2D(GL_TEXTURE_2D, 0, internal_format,
+                                 dest_size.width(), dest_size.height(), 0,
+                                 internal_format, type, nullptr);
+      destination_gl->CopySubTextureCHROMIUM(
+          intermediate_texture, 0, GL_TEXTURE_2D, texture, 0, 0, 0, 0, 0,
+          dest_size.width(), dest_size.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 +909,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;