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

content/renderer/media/video_capture_impl.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.
 //
 // Notes about usage of this object by VideoCaptureImplManager.
 //
 // VideoCaptureImplManager access this object by using a Unretained()
 // binding and tasks on the IO thread. It is then important that
 // VideoCaptureImpl never post task to itself. All operations must be
 // synchronous.
@@ skipping 240 matching lines @@
           .insert(std::make_pair(buffer_id,
                                  new ClientBuffer(std::move(shm), memory_size)))
           .second;
   DCHECK(inserted);
 }
 
 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);
     return;
   }
 
   base::TimeTicks reference_time;
   media::VideoFrameMetadata frame_metadata;
   frame_metadata.MergeInternalValuesFrom(info->metadata);
   const bool success = frame_metadata.GetTimeTicks(
@@ skipping 154 matching lines @@
   double consumer_resource_utilization = -1.0;
   if (!metadata->GetDouble(media::VideoFrameMetadata::RESOURCE_UTILIZATION,
                            &consumer_resource_utilization)) {
     consumer_resource_utilization = -1.0;
   }
 
   callback_to_io_thread.Run(*release_sync_token, consumer_resource_utilization);
 }
 
 }  // namespace content