16 bpp video stream capture, render and createImageBitmap(video) using (CPU) shared memory buffers

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 c3387ae15db6db9f9918d5192865ca36e5827313..501b9c9d2068dbf8089ac609c1b037cc58f6f381 100644
--- a/media/renderers/skcanvas_video_renderer.cc
+++ b/media/renderers/skcanvas_video_renderer.cc
@@ -347,6 +347,7 @@ void SkCanvasVideoRenderer::Paint(const scoped_refptr<VideoFrame>& video_frame,
   // frame has an unexpected format.
   if (!video_frame.get() || video_frame->natural_size().IsEmpty() ||
       !(media::IsYuvPlanar(video_frame->format()) ||
+        video_frame->format() == media::PIXEL_FORMAT_Y16 ||
         video_frame->HasTextures())) {
     SkPaint blackWithAlphaPaint;
     blackWithAlphaPaint.setAlpha(paint.getAlpha());
@@ -520,6 +521,30 @@ scoped_refptr<VideoFrame> DownShiftHighbitVideoFrame(
   }
   return ret;
 }
+
+void ConvertY16ToARGB(const VideoFrame* video_frame,

[xhwang, 2016/10/27 16:59:18]

Please add a comment to summarize what we are doing here, e.g. put the upper 8
bits of the 16 bit data and convert it to ARGB etc.

[aleksandar.stojiljkovic, 2016/10/27 19:12:23]

Done.

+                      void* argb_pixels,
+                      size_t argb_row_bytes) {
+  const uint8_t* source =
+      reinterpret_cast<const uint8_t*>(video_frame->visible_data(0));

[xhwang, 2016/10/27 16:59:18]

why do you need the cast?

[xhwang, 2016/10/27 16:59:18]

It seems these functions only convert the visible rect, is this intended? 

https://cs.chromium.org/chromium/src/media/renderers/skcanvas_video_renderer....

If so, maybe we should rename these functions to make it more clear.

[aleksandar.stojiljkovic, 2016/10/27 19:12:23]

Done.

[aleksandar.stojiljkovic, 2016/10/27 19:12:23]

Almost all of the functionality in the file is based on visible_rect; gpu
storage is copied as it is but Paint/UpdateLastImage/code under
VideoImageGenerator are all about visible_rect. It is specified in documentation
as you pointed out and renaming it only for methods called from
ConvertVideoFrameToRGBPixels might be problematic.  

+  uint8_t* out = reinterpret_cast<uint8_t*>(argb_pixels);
+  const size_t stride = video_frame->stride(0);
+  for (int i = 0; i < video_frame->visible_rect().height(); ++i) {
+    const uint8_t* row = source;
+    uint32_t* rgba = reinterpret_cast<uint32_t*>(out);
+    for (const uint8_t* row_end = row + video_frame->row_bytes(0);
+         row < row_end; ++row) {

[xhwang, 2016/10/27 16:59:18]

This for-loop is confusing. Does it make sense to reorganize as:

const uint8_t* row_end = source + video_frame->row_bytes(0);
for (const uint8_t* row = source; row < row_end; ++row) {
  
  ...
}

Also, would it be more cleaner to s/source/row_head?

[aleksandar.stojiljkovic, 2016/10/27 19:12:23]

Done. That was strange looking loop - after all the fixes here, looks better
now.

+      // We loose the precision here and take only upper 8 bits of 16 bit data.
+      // It is important not to render Y16 as RG_88. To get the full precision
+      // use float textures with WebGL1 and e.g. R16UI or R32F textures with
+      // WebGL2.
+      uint32_t green = *++row;

[xhwang, 2016/10/27 16:59:18]

why this is "green"? Should it be something like "grey_value"?

[aleksandar.stojiljkovic, 2016/10/27 19:12:23]

Done.

+      *rgba++ = SkColorSetARGB(0xFF, green, green, green);

[hubbe, 2016/10/27 17:25:03]

I wonder if it would be helpful to do an inverse-gamma conversion here?
ARGB textures are *usually* in sRGB space, and I assume that the Y16 texture is
usually in linear space.

[aleksandar.stojiljkovic, 2016/10/27 19:12:23]

Important to keep the values; some developers might be fine with 8 bit value
precision for depth/infrared and use GetBitmapData, someone will need full
precision through WebGL, but the normalized values needs to match.

+    }
+    out += argb_row_bytes;
+    source += stride;
+  }
+}
 }

[xhwang, 2016/10/27 16:59:18]

Can this compile?

[aleksandar.stojiljkovic, 2016/10/27 19:12:22]

Yes - it is closing // anonymous namespace. Added the comment.

 
 // static
@@ -531,10 +556,6 @@ void SkCanvasVideoRenderer::ConvertVideoFrameToRGBPixels(
     NOTREACHED() << "Cannot extract pixels from non-CPU frame formats.";
     return;
   }
-  if (!media::IsYuvPlanar(video_frame->format())) {
-    NOTREACHED() << "Non YUV formats are not supported";
-    return;
-  }
 
   switch (video_frame->format()) {
     case PIXEL_FORMAT_YV12:
@@ -627,6 +648,10 @@ void SkCanvasVideoRenderer::ConvertVideoFrameToRGBPixels(
       break;
     }
 
+    case PIXEL_FORMAT_Y16:
+      ConvertY16ToARGB(video_frame, rgb_pixels, row_bytes);
+      break;
+
     case PIXEL_FORMAT_NV12:
     case PIXEL_FORMAT_NV21:
     case PIXEL_FORMAT_UYVY:
@@ -637,14 +662,9 @@ void SkCanvasVideoRenderer::ConvertVideoFrameToRGBPixels(
     case PIXEL_FORMAT_RGB32:
     case PIXEL_FORMAT_MJPEG:
     case PIXEL_FORMAT_MT21:
-    // TODO(dshwang): Use either I400ToARGB or J400ToARGB depending if we want
-    // BT.601 constrained range of 16 to 240, or JPEG full range BT.601
-    // coefficients. Implement it when Y8/16 foramt is supported.
-    // crbug.com/624436
     case PIXEL_FORMAT_Y8:
-    case PIXEL_FORMAT_Y16:
     case PIXEL_FORMAT_UNKNOWN:
-      NOTREACHED();
+      NOTREACHED() << "Only YUV formats and Y16 are supported.";
   }
 }