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 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,
                                               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 ||
+      !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();
 
   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 (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
     // 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