Motown: Video renderer library code

mojo/services/media/common/cpp/video_converter.cc

Index: mojo/services/media/common/cpp/video_converter.cc
diff --git a/mojo/services/media/common/cpp/video_converter.cc b/mojo/services/media/common/cpp/video_converter.cc
new file mode 100644
index 0000000000000000000000000000000000000000..cd8c35a0967ea973dc59fe2a45407f2ad48b6364
--- /dev/null
+++ b/mojo/services/media/common/cpp/video_converter.cc
@@ -0,0 +1,174 @@
+// Copyright 2016 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 "mojo/services/media/common/cpp/video_converter.h"
+
+namespace mojo {
+namespace media {
+
+VideoConverter::VideoConverter() {
+  BuildColorspaceTable();
+}
+
+VideoConverter::~VideoConverter() {}
+
+namespace {
+
+uint8_t ToByte(float f) {
+  if (f < 0.0f) {

[kulakowski, 2016/06/17 21:15:40]

I'm paranoid having once seen someone spend days debugging such a function. I
believe that you correctly handle the undefined behavior via this clamping.
Double checking that it also appropriately handles -0. / denormals / NaNs.

[dalesat, 2016/06/18 00:07:08]

This isn't terribly critical, being temporary code. Also, the consequences of a
bug here is that, on rare occasions, the wrong color pixel will be shown.

+    return 0u;
+  }
+
+  if (f > 255.0f) {
+    return 255u;
+  }
+
+  return static_cast<uint8_t>(f);
+}
+
+size_t ColorspaceTableOffset(uint8_t y, uint8_t u, uint8_t v) {

[kulakowski, 2016/06/17 21:15:40]

Why do this math yourself and not write [y][u][v]?

[dalesat, 2016/06/18 00:07:08]

I'm just not familiar with using arrays of arrays of arrays. Isn't that an array
of pointers to arrays of pointers to arrays? Is there a unique_ptr that'll clean
that up properly? Given that this code is temporary, this felt less likely to
misbehave.

+  return (y << 8u | u) << 8u | v;
+}
+
+}  // namespace
+
+void VideoConverter::BuildColorspaceTable() {
+  colorspace_table_.reset(new uint32_t[256 * 256 * 256]);
+
+  uint32_t* p = colorspace_table_.get();
+
+  for (float y = 0.0f; y < 256.0f; y += 1.0f) {

[kulakowski, 2016/06/17 21:15:40]

I hope you typed these constants correctly :)

I believe that the for loop bounds are correct at floats this size, but man,
it's a little spooky seeing loop bounds being float comparisons.

I'm also curious if the preferred specification of this color table is in terms
of real math, floating point math, or fixed point math.

[dalesat, 2016/06/18 00:07:08]

Doing the iteration with ints rather than floats makes sense.

I copied the comments from an article written by an amateur. This isn't intended
to be definitive. I don't think we'll be building a 64MB table for this in
production.

+    for (float u = 0.0f; u < 256.0f; u += 1.0f) {
+      for (float v = 0.0f; v < 256.0f; v += 1.0f) {
+        // R = 1.164(Y - 16) + 1.596(V - 128)
+        uint8_t r = ToByte(1.164f * (y - 16.0f) + 1.596f * (v - 128.0f));
+
+        // G = 1.164(Y - 16) - 0.813(V - 128) - 0.391(U - 128)
+        uint8_t g = ToByte(1.164f * (y - 16.0f) - 0.813f * (v - 128.0f) -
+                           0.391f * (u - 128.0f));
+
+        // B = 1.164(Y - 16) + 2.018(U - 128)
+        uint8_t b = ToByte(1.164f * (y - 16.0f) + 2.018f * (u - 128.0f));
+
+        *p = r | g << 8u | b << 16u | 255u << 24u;

[kulakowski, 2016/06/17 21:15:40]

I and someone looking over my shoulder both are unsure off the top of our heads
if the precedence of this is correct, so parens please.

[dalesat, 2016/06/18 00:07:08]

Done.

+        ++p;
+      }
+    }
+  }
+}
+
+void VideoConverter::SetMediaType(const MediaTypePtr& media_type) {
+  MOJO_DCHECK(media_type);
+  MOJO_DCHECK(media_type->medium == MediaTypeMedium::VIDEO);
+  MOJO_DCHECK(media_type->encoding == MediaType::kVideoEncodingUncompressed);
+  MOJO_DCHECK(media_type->details);
+
+  const VideoMediaTypeDetailsPtr& details = media_type->details->get_video();
+  MOJO_DCHECK(details);
+  MOJO_DCHECK(details->pixel_format == PixelFormat::YV12)
+      << "only YV12 video conversion is currently implemented";
+
+  layout_ =
+      VideoPacketLayout(details->pixel_format, details->width, details->height,
+                        details->coded_width, details->coded_height);
+
+  media_type_set_ = true;
+}
+
+void VideoConverter::ConvertFrame(uint8_t* rgba_buffer,
+                                  uint32_t view_width,
+                                  uint32_t view_height,
+                                  void* payload,
+                                  uint64_t payload_size) {
+  MOJO_DCHECK(rgba_buffer != nullptr);
+  MOJO_DCHECK(view_width != 0);
+  MOJO_DCHECK(view_height != 0);
+  MOJO_DCHECK(payload != nullptr);
+  MOJO_DCHECK(payload_size != 0);
+  MOJO_DCHECK(media_type_set_)
+      << "need to call SetMediaType before ConvertFrame";
+
+  uint32_t height = std::min(layout_.height(), view_height);
+  uint32_t width = std::min(layout_.width(), view_width);
+
+  // YV12 frames have three separate planes. The Y plane has 8-bit Y values for
+  // each pixel. The U and V planes have 8-bit U and V values for 2x2 grids of
+  // pixels, so those planes are each 1/4 the size of the Y plane. Both the
+  // inner and outer loops below are unrolled to deal with the 2x2 logic.
+
+  size_t dest_line_stride = view_width;
+  size_t y_line_stride =
+      layout_.line_stride_for_plane(VideoPacketLayout::kYPlaneIndex);
+  size_t u_line_stride =
+      layout_.line_stride_for_plane(VideoPacketLayout::kUPlaneIndex);
+  size_t v_line_stride =
+      layout_.line_stride_for_plane(VideoPacketLayout::kVPlaneIndex);
+
+  uint32_t* dest_line = reinterpret_cast<uint32_t*>(
+      rgba_buffer + dest_line_stride * (view_height - 1) * sizeof(uint32_t));
+  uint8_t* y_line =
+      reinterpret_cast<uint8_t*>(payload) +
+      layout_.plane_offset_for_plane(VideoPacketLayout::kYPlaneIndex);
+  uint8_t* u_line =
+      reinterpret_cast<uint8_t*>(payload) +
+      layout_.plane_offset_for_plane(VideoPacketLayout::kUPlaneIndex);
+  uint8_t* v_line =
+      reinterpret_cast<uint8_t*>(payload) +
+      layout_.plane_offset_for_plane(VideoPacketLayout::kVPlaneIndex);
+
+  for (uint32_t line = 0; line < height; ++line) {
+    ConvertLine(dest_line, y_line, u_line, v_line, width);
+
+    dest_line -= dest_line_stride;
+    y_line += y_line_stride;
+    // Notice we aren't updating u_line and v_line here.
+
+    // If we hadn't unrolled the loop, it would have ended here.
+    if (++line == height) {
+      break;
+    }
+
+    ConvertLine(dest_line, y_line, u_line, v_line, width);
+
+    dest_line -= dest_line_stride;
+    y_line += y_line_stride;
+    // Here, we ARE updating u_line and v_line, because we've moved vertically
+    // out of the 2x2 grid.
+    u_line += u_line_stride;
+    v_line += v_line_stride;
+  }
+}
+
+void VideoConverter::ConvertLine(uint32_t* dest_pixel,
+                                 uint8_t* y_pixel,
+                                 uint8_t* u_pixel,
+                                 uint8_t* v_pixel,
+                                 uint32_t width) {
+  for (uint32_t pixel = 0; pixel < width; ++pixel) {
+    *dest_pixel =
+        colorspace_table_
+            .get()[ColorspaceTableOffset(*y_pixel, *u_pixel, *v_pixel)];
+    ++dest_pixel;
+    ++y_pixel;
+    // Notice we aren't incrementing u_pixel and v_pixel here.
+
+    // If we hadn't unrolled the loop, it would have ended here.
+    if (++pixel == width) {
+      break;
+    }
+
+    *dest_pixel =
+        colorspace_table_
+            .get()[ColorspaceTableOffset(*y_pixel, *u_pixel, *v_pixel)];
+    ++dest_pixel;
+    ++y_pixel;
+    // Here, we ARE incrementing u_pixel and v_pixel, because we've moved
+    // horizontally out of the 2x2 grid.
+    ++u_pixel;
+    ++v_pixel;
+  }
+}
+
+}  // namespace media
+}  // namespace mojo