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

content/renderer/media/video_capture_impl.cc

Index: content/renderer/media/video_capture_impl.cc
diff --git a/content/renderer/media/video_capture_impl.cc b/content/renderer/media/video_capture_impl.cc
index 20d5d921fe483fe41c37bcef49429f3b784c9b8b..1b681d73113451a3a3f82f76a1172eba2ce976f9 100644
--- a/content/renderer/media/video_capture_impl.cc
+++ b/content/renderer/media/video_capture_impl.cc
@@ -258,11 +258,13 @@ void VideoCaptureImpl::OnBufferReady(int32_t buffer_id,
                                      mojom::VideoFrameInfoPtr info) {
   DVLOG(1) << __func__ << " buffer_id: " << buffer_id;
   DCHECK(io_thread_checker_.CalledOnValidThread());
-  DCHECK_EQ(media::PIXEL_FORMAT_I420, info->pixel_format);
+  DCHECK(media::PIXEL_FORMAT_I420 == info->pixel_format ||
+         media::PIXEL_FORMAT_Y16 == info->pixel_format);
   DCHECK_EQ(media::PIXEL_STORAGE_CPU, info->storage_type);

[mcasas, 2016/10/25 22:44:43]

This DCHECK and the one in l.261-262 and the if() clause
below are redundant and we shouldn't handle (D)CHECK 
failures [1], but they were kept here for easier 
logging. You can safely remove them and add instead a
NOTREACHED() << "Wrong state or pixel format or storage";
inside the if() body.

[1]
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

[aleksandar.stojiljkovic, 2016/10/26 14:02:18]

One way is to leave the the DCHECK and remove the handling from if statement
(l.266). That would be a change in behavior adding risk to this patch - similar
issue handled yesterday for cc here [1] in separate patch:
if (!cond) {NOTREACHED(); return,} replaced by DCHECK(cond).
[1]
https://codereview.chromium.org/2428263004/diff/80001/cc/resources/video_reso...

Otherwise, code in video_frame.cc is using LOG(DFATAL) for the same issue as
here inside the if body.

I might have misunderstood the suggestion. Anyway, removed the DCHECK as
suggested and left the handled return (as it was in non debug mode) with
LOG(DFATAL) << "Wrong state or pixel format or storage..." inside the existing
if branch in l.269.

[aleksandar.stojiljkovic, 2016/10/28 18:58:55]

mcasas@chromium.org,
I had to modify this: state_ != VIDEO_CAPTURE_STATE_STARTED is legal and
shouldn't trigger any LOG(DFATAL).
Frame arrived after capture stop. There is a test checking this -
VideoCaptureImplTest.BufferReceivedAfterStop.

 
   if (state_ != VIDEO_CAPTURE_STATE_STARTED ||
-      info->pixel_format != media::PIXEL_FORMAT_I420 ||
+      (info->pixel_format != media::PIXEL_FORMAT_I420 &&
+       info->pixel_format != media::PIXEL_FORMAT_Y16) ||
       info->storage_type != media::PIXEL_STORAGE_CPU) {
     GetVideoCaptureHost()->ReleaseBuffer(device_id_, buffer_id,
                                          gpu::SyncToken(), -1.0);