YUV software decode path stride fixes.

media/base/video_frame.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/base/video_frame.h"
 
 #include "base/logging.h"
 #include "base/string_piece.h"
 #include "media/base/limits.h"
 #include "media/base/video_util.h"
@@ skipping 126 matching lines @@
       av_malloc(bytes_per_row * aligned_height + kFramePadBytes));
 #else
   data_[VideoFrame::kRGBPlane] = new uint8_t[bytes_per_row * aligned_height];
 #endif
   DCHECK(!(reinterpret_cast<intptr_t>(data_[VideoFrame::kRGBPlane]) & 7));
   COMPILE_ASSERT(0 == VideoFrame::kRGBPlane, RGB_data_must_be_index_0);
 }
 
 void VideoFrame::AllocateYUV() {
   DCHECK(format_ == VideoFrame::YV12 || format_ == VideoFrame::YV16);
-  // Align Y rows at least at 16 byte boundaries.  The stride for both
-  // YV12 and YV16 is 1/2 of the stride of Y.  For YV12, every row of bytes for
-  // U and V applies to two rows of Y (one byte of UV for 4 bytes of Y), so in
-  // the case of YV12 the strides are identical for the same width surface, but
-  // the number of bytes allocated for YV12 is 1/2 the amount for U & V as
-  // YV16. We also round the height of the surface allocated to be an even
-  // number to avoid any potential of faulting by code that attempts to access
-  // the Y values of the final row, but assumes that the last row of U & V
-  // applies to a full two rows of Y.
+  // Align Y rows at least at 32 byte boundaries, so the stride for both YV12
+  // and YV16 at 1/2 of the stride of Y is aligned to 16. For YV12, every row of
+  // bytes for U and V applies to two rows of Y (one byte of UV for 4 bytes of
+  // Y), so in the case of YV12 the strides are identical for the same width
+  // surface, but the number of bytes allocated for YV12 is 1/2 the amount for
+  // U & V as YV16. We also round the height of the surface allocated to be an
+  // even number to avoid any potential of faulting by code that attempts to
+  // access the Y values of the final row, but assumes that the last row of U &
+  // V applies to a full two rows of Y.
   size_t y_stride = RoundUp(row_bytes(VideoFrame::kYPlane),
-                            kFrameSizeAlignment);
-  size_t uv_stride = RoundUp(row_bytes(VideoFrame::kUPlane),
-                             kFrameSizeAlignment);
+                            kFrameSizeAlignment * 2);
+  size_t uv_stride = y_stride / 2;

[Ami GONE FROM CHROMIUM, 2012/11/15 18:15:36]

I don't understand the impetus for this change.
Why is the old code wrong / why is it important that y_stride==uv_stride*2?

[sheu, 2012/11/15 19:46:38]

When we're doing GL rendering of the uploaded texture, we have only one scale
factor for the Y and UV planes.  If the strides/height aspect ratios aren't the
same, we'll get chroma and luma misaligned with each other.

I'll add this to the comment.

[Ami GONE FROM CHROMIUM, 2012/11/15 20:39:09]

It seems strange that an impl detail of cc::VideoLayerImpl should place this
requirement on VideoFrame.

Shouldn't this instead imply that the shader should take multiple scale factors,
one per plane?

[sheu, 2012/11/15 22:20:53]

I guess that would have been a better way to put this -- we're implicitly
assuming that scale factors are identical for Y and UV.  It's not necessarily
just aa cc::VideoLayerImpl implementation detaiil.

[Ami GONE FROM CHROMIUM, 2012/11/15 23:11:15]

That seems wrong.  stride can differ per-plane, and width cannot.  Therefore
their ratio can differ per plane.
I don't follow.  Who else cares?


Can you assume the presence of GL_EXT_unpack_subimage and use
GL_UNPACK_ROW_LENGTH to tell GL exactly what's going on (stride!=width) instead
of scaling to have the visible rect cover the stride and then get scaled back
down to the <video> size?

[sheu, 2012/11/16 03:00:10]

Right, and I'm making sure that the planes have the same ratio here.

On 2012/11/15 23:11:15, Ami Fischman wrote:
I'm not using the visible rect to cover both the stride and the <video> size
scaling.  Basically:

* <video> size scaling is handled by the quadRect, which affects the vertex
coordinates when we draw the textured quad to the screen.
* visible/coded/stride scaling is handled by the texScale factor, which affects
texture coordinates when we draw the textured quad.

I've uploaded a next WIP version to get your thoughts on it. 
GL_EXT_unpack_subimage has some other problems that I'll notate on that version.

   // The *2 here is because some formats (e.g. h264) allow interlaced coding,
   // and then the size needs to be a multiple of two macroblocks (vertically).
   // See libavcodec/utils.c:avcodec_align_dimensions2().
   size_t y_height = RoundUp(coded_size_.height(), kFrameSizeAlignment * 2);
   size_t uv_height = format_ == VideoFrame::YV12 ? y_height / 2 : y_height;
   size_t y_bytes = y_height * y_stride;
   size_t uv_bytes = uv_height * uv_stride;
 
 #if !defined(OS_ANDROID)
   // TODO(dalecurtis): use DataAligned or so, so this #ifdef hackery
@@ skipping 148 matching lines @@
       break;
     for(int row = 0; row < rows(plane); row++) {
       base::MD5Update(context, base::StringPiece(
           reinterpret_cast<char*>(data(plane) + stride(plane) * row),
           row_bytes(plane)));
     }
   }
 }
 
 }  // namespace media