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..a35725e89e663adad54293bad17dba7ae672ab9c 100644
--- a/media/renderers/skcanvas_video_renderer.cc
+++ b/media/renderers/skcanvas_video_renderer.cc
@@ -160,6 +160,34 @@ sk_sp<SkImage> NewSkImageFromVideoFrameYUVTextures(
   return img;
 }
 
+gfx::Size MinVideoFrameSize(const VideoFrame* video_frame) {
+  // On some platforms, and in some situations, 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.
+  //
+  // In other situations, the natural size has been seen to be _larger_
+  // than the coded size -- despite the fact that this means that there are
+  // regions not actually covered by decoded video data! This appears to
+  // happen if the Sample Aspect Ratio in the video's metadata is slightly
+  // incorrect.

[sandersd (OOO until July 31), 2017/04/03 18:56:21]

This wording is misleading; it implies that normal usages are errors. We track
all three values (|coded_size|, |visible_rect|, and |natural_size|) so that we
can account for all of the valid situations:

  - |coded_size| is the actual pixels encoded by the encoder. For H.264 and VPx
this is a multiple of the macroblock size. Encoders generally expand the source
material to this size by smearing the edge pixels.

  - |visible_rect| is the region of the encoded pixels that represent the source
material (cropping away the smeared pixels). The origin may not be (0, 0), in
particular if rotation is part of the encoding pipeline. In practice it is
always aligned to *some* corner, but this isn't a technical requirement.

  - |natural_size| is the intended display size. This is the source of truth for
the pixel aspect ratio of the content. There are no analog sources that use a
1:1 pixel aspect ratio, so it is common that |natural_size| does not match
|visible_rect| size. It is also normal to leave |natural_size| unchanged when
switching stream resolutions.

Natural size is the only size that users (or web developers) interact with in
most situations.

When mapping on to a texture, the size of that texture isn't usually relevant,
just that the sampled region is the interior of the |visible_rect|.

[Ken Russell (switch to Gerrit), 2017/04/03 20:43:32]

Thanks for these clarifications. Sorry for the misleading wording; I'm not an
expert in this area.

The basic problem is that in the previous fix for http://crbug.com/672895 I
changed the code so that the Blink layer allocates the destination texture for
video-to-WebGL-texture copies, and I see now that that can't really be done
because the required sizing information isn't available at that level.

I'll restructure this fix so that
WebMediaPlayer::copyVideoTextureToPlatformTexture reallocates the destination
texture at the correct size.

[sandersd (OOO until July 31), 2017/04/03 21:19:10]

Reading the WebGL spec, I'm not certain what it actually wants. It says 'The
first pixel... corresponds to the upper left corner of the source', which
implies that |visible_rect| cropping is done. It also says 'The width and height
of the texture are set to the width and height of the uploaded frame'; which is
ambiguous with regards to pixel aspect ratio.

Ideally it would be written to allow for optimizations in the common cases, but
at the very least the spec should be more clear on the topic of scaling.

Based on this text, I do agree that Firefox is probably wrong to report a height
different from 1080, even though 1088 is a reasonable way to interpret the
'height of the uploaded frame'. I am curious though how they avoided garbage
along one edge if they are uploading the entire coded frame.

I'm assuming there is no path for us to manipulate the UVs to fake this and
potentially use our existing textures in the fast path? (Is this what Firefox is
doing?)

[Ken Russell (switch to Gerrit), 2017/04/07 07:16:00]

Agreed that the spec needs to be clarified. There wasn't enough expertise in the
WebGL working group to realize the spec was lacking in this area. The language
around the first pixel is intended only to clarify whether, in the uploaded
texture, the Y axis goes "up" or "down".
I've come to realize that the video-to-WebGL-texture upload paths are probably
slightly broken in all browsers and that we'll need more tests around videos
with rotations, videos where it's intended that there be a significantly
different visible rectangle, etc.
No, it's currently necessary to clip the uploaded video frame when uploading it
to WebGL.

This proposed extension:
https://www.khronos.org/registry/webgl/extensions/proposals/OES_EGL_image_ext...

will provide a more optimized, and zero-copy, upload path. I now realize that
the metadata which it returns must contain the texture size (i.e., coded size),
natural size, and visible rectangle (if I understand everything correctly now).

+  //
+  // Callers of some of the APIs in this file are responsible for
+  // allocating the textures into which the video data goes. The only
+  // sizing information these callers have are the width and height of the
+  // video -- which are inevitably the natural width and height, since
+  // that's all that's reliably available in the PipelineMetadata. The
+  // destination textures are therefore allocated at this size.
+  //
+  // To avoid putting garbage into the bottom of the destination frame for
+  // the first case, and to avoid mismatches between the source and
+  // destination textures in CopySubTextureCHROMIUM operations for the
+  // second case, we report the minimum video frame size as the
+  // intersection of the coded size and the natural size.
+  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 +212,9 @@ 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::SingleFrameCopyTexture,
+        source_texture, GL_RGBA, GL_UNSIGNED_BYTE, true, false);
   } else {
     gl->WaitSyncTokenCHROMIUM(mailbox_holder.sync_token.GetConstData());
     source_texture = gl->CreateAndConsumeTextureCHROMIUM(
@@ -753,7 +778,10 @@ void SkCanvasVideoRenderer::ConvertVideoFrameToRGBPixels(
 void SkCanvasVideoRenderer::CopyVideoFrameSingleTextureToGLTexture(
     gpu::gles2::GLES2Interface* gl,
     VideoFrame* video_frame,
+    SingleFrameCopyMode mode,
     unsigned int texture,
+    unsigned int internal_format,
+    unsigned int type,
     bool premultiply_alpha,
     bool flip_y) {
   DCHECK(video_frame);
@@ -776,14 +804,29 @@ 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);
+  switch (mode) {
+    case SingleFrameCopyTexture: {
+      // Assume that the caller is responsible for clipping the source
+      // texture during rendering to the video's natural width and
+      // height. This is done for example during rendering of the actual
+      // video element.
+      gl->CopyTextureCHROMIUM(source_texture, 0, GL_TEXTURE_2D, texture, 0,
+                              internal_format, type, flip_y, premultiply_alpha,
+                              false);
+      break;
+    }
+    case SingleFrameCopySubTexture: {
+      // Use the minimum of the coded size and natural size to copy only
+      // the good portion of the video frame.
+      gfx::Size copy_size = MinVideoFrameSize(video_frame);
+      gl->CopySubTextureCHROMIUM(source_texture, 0, GL_TEXTURE_2D, texture, 0,
+                                 0, 0, 0, 0, copy_size.width(),
+                                 copy_size.height(), flip_y, premultiply_alpha,
+                                 false);
+      break;
+    }
+  }
+
   gl->DeleteTextures(1, &source_texture);
   gl->Flush();
 
@@ -832,14 +875,12 @@ 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();
+    // Use the minimum of the coded size and natural size to copy only the
+    // good portion of the video frame.
+    gfx::Size copy_size = MinVideoFrameSize(video_frame.get());
     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,
+        copy_size.width(), copy_size.height(), flip_y, premultiply_alpha,
         false);
     destination_gl->DeleteTextures(1, &intermediate_texture);
 
@@ -855,8 +896,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(), SingleFrameCopySubTexture, texture,
+        0 /* internal_format */, 0 /* type */, premultiply_alpha, flip_y);
   }
 
   return true;