getUserMedia: handle the device starting status report.

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 52 matching lines @@
 
   VideoCaptureDeliverFrameCB deliver_frame_cb;
 };
 
 VideoCaptureImpl::VideoCaptureImpl(media::VideoCaptureSessionId session_id)
     : device_id_(session_id),
       session_id_(session_id),
       video_capture_host_for_testing_(nullptr),
       observer_binding_(this),
       state_(VIDEO_CAPTURE_STATE_STOPPED),
+      should_request_refresh_frame_on_started_(false),
       weak_factory_(this) {
   io_thread_checker_.DetachFromThread();
 
   if (ChildThread::Get()) {  // This will be null in unit tests.
     mojom::VideoCaptureHostPtr temp_video_capture_host;
     ChildThread::Get()->GetRemoteInterfaces()->GetInterface(
         mojo::MakeRequest(&temp_video_capture_host));
     video_capture_host_info_ = temp_video_capture_host.PassInterface();
   }
 }
@@ skipping 17 matching lines @@
     const media::VideoCaptureParams& params,
     const VideoCaptureStateUpdateCB& state_update_cb,
     const VideoCaptureDeliverFrameCB& deliver_frame_cb) {
   DVLOG(1) << __func__ << " |device_id_| = " << device_id_;
   DCHECK(io_thread_checker_.CalledOnValidThread());
   ClientInfo client_info;
   client_info.params = params;
   client_info.state_update_cb = state_update_cb;
   client_info.deliver_frame_cb = deliver_frame_cb;
 
   if (state_ == VIDEO_CAPTURE_STATE_ERROR) {

[miu, 2017/03/06 22:37:33]

Can we re-organize this complex logic into a flattened switch-statement, to
better clarify that this logic is correctly handling things in each possible
|state_|?

Something like:

  switch (state_) {
  case VIDEO_CAPTURE_STATE_STARTING:
  case VIDEO_CAPTURE_STATE_STARTED:
    ...L118 to L122 goes here...
    return;
  case VIDEO_CAPTURE_STATE_STOPPING:
  case VIDEO_CAPTURE_STATE_STOPPED:
    ...L124 to L126 goes here...
    return;
  case VIDEO_CAPTURE_STATE_PAUSED:
  case VIDEO_CAPTURE_STATE_RESUMED:
  case VIDEO_CAPTURE_STATE_PAUSED:
    ...L128 to L138 goes here...
    return;
  case VIDEO_CAPTURE_STATE_ENDED:
    // Do nothing because...
    return;
  case VIDEO_CAPTURE_STATE_ERROR:
    state_update_cb.Run(VIDEO_CAPTURE_STATE_ERROR);
    return;
  }

[braveyao, 2017/03/08 22:02:52]

Done.
By my understanding, the check, 
"if (clients_pending_on_restart_.count(client_id) ||
     clients_.count(client_id))"
is useless here. Please help to double check!

[miu, 2017/03/10 23:56:56]

Yes, that's my understanding too. :)

[braveyao, 2017/03/13 19:04:44]

Acknowledged.

     state_update_cb.Run(VIDEO_CAPTURE_STATE_ERROR);
   } else if (clients_pending_on_restart_.count(client_id) ||
              clients_.count(client_id)) {
     DLOG(FATAL) << __func__ << " This client has already started.";
   } else {
-    // Note: |state_| might not be started at this point. But we tell
-    // client that we have started.
-    state_update_cb.Run(VIDEO_CAPTURE_STATE_STARTED);

[miu, 2017/03/06 22:37:33]

Per my earlier comment, how about keeping this, but calling it with the new
VIDEO_CAPTURE_STATE_STARTING state?

[braveyao, 2017/03/08 22:02:52]

This does no good and will cause troubles to the unittests. So I still don't
want to keep it.

     if (state_ == VIDEO_CAPTURE_STATE_STARTED) {
       clients_[client_id] = client_info;
       // TODO(sheu): Allowing resolution change will require that all
       // outstanding clients of a capture session support resolution change.
       DCHECK_EQ(params_.resolution_change_policy,
                 params.resolution_change_policy);
     } else if (state_ == VIDEO_CAPTURE_STATE_STOPPING) {
       clients_pending_on_restart_[client_id] = client_info;
       DVLOG(1) << __func__ << " Got new resolution while stopping: "
                << params.requested_format.frame_size.ToString();
     } else {
       clients_[client_id] = client_info;
       if (state_ == VIDEO_CAPTURE_STATE_STARTED)

[miu, 2017/03/06 22:37:33]

Note: Here's one reason I suggest the switch-statement reorganization: This
if-statement is meaningless because |state_| cannot be
VIDEO_CAPTURE_STATE_STARTED at this point.

[braveyao, 2017/03/08 22:02:52]

Done.
Also found the STARTING state is necessary for StopDevice(). Thanks!

         return;
       params_ = params;
       params_.requested_format.frame_rate =
           std::min(params_.requested_format.frame_rate,
                    static_cast<float>(media::limits::kMaxFramesPerSecond));
 
       DVLOG(1) << "StartCapture: starting with first resolution "
                << params_.requested_format.frame_size.ToString();
       StartCaptureInternal();
     }
@@ skipping 39 matching lines @@
       base::Bind(&VideoCaptureImpl::OnDeviceFormatsInUse,
                  weak_factory_.GetWeakPtr(), callback));
 }
 
 void VideoCaptureImpl::OnStateChanged(mojom::VideoCaptureState state) {
   DVLOG(1) << __func__ << " state: " << state;
   DCHECK(io_thread_checker_.CalledOnValidThread());
 
   switch (state) {
     case mojom::VideoCaptureState::STARTED:
-      // Capture has started in the browser process. Since we have already
-      // told all clients that we have started there's nothing to do.
+      state_ = VIDEO_CAPTURE_STATE_STARTED;
+      for (const auto& client : clients_)
+        client.second.state_update_cb.Run(VIDEO_CAPTURE_STATE_STARTED);
+      if (should_request_refresh_frame_on_started_) {

[miu, 2017/03/06 22:37:33]

Suggestion: Instead of this logic, what if VideoCaptureImpl just unconditionally
requests a refresh frame once reaching the STARTED state? IMHO, it makes sense
to provide the client with a first frame as soon as possible. Also, it wouldn't
be horribly redundant because capture devices can ignore the request if they
know they are going to provide a new frame soon anyway.

[braveyao, 2017/03/08 22:02:52]

Done.
Good to know!
But it will cause many GMock Warnings. And most cases don't care the calling to
RequestRefreshFrames. So I didn't add EXPECT_CALL there. Is it really good?

[miu, 2017/03/10 23:56:56]

Yes, this should be fine. It's up to you, but it would probably be reasonable to
add the EXPECT_CALL() to reduce log spam; and because it's an acceptable
behavior change.

[braveyao, 2017/03/13 19:04:44]

Acknowledged.

+        RequestRefreshFrame();
+        should_request_refresh_frame_on_started_ = false;
+      }
       break;
     case mojom::VideoCaptureState::STOPPED:
       state_ = VIDEO_CAPTURE_STATE_STOPPED;
       client_buffers_.clear();
       weak_factory_.InvalidateWeakPtrs();
       if (!clients_.empty() || !clients_pending_on_restart_.empty())
         RestartCapture();
       break;
     case mojom::VideoCaptureState::PAUSED:
       for (const auto& client : clients_)
@@ skipping 18 matching lines @@
       break;
   }
 }
 
 void VideoCaptureImpl::OnBufferCreated(int32_t buffer_id,
                                        mojo::ScopedSharedBufferHandle handle) {
   DVLOG(1) << __func__ << " buffer_id: " << buffer_id;
   DCHECK(io_thread_checker_.CalledOnValidThread());
   DCHECK(handle.is_valid());
 
-  if (state_ != VIDEO_CAPTURE_STATE_STARTED)
-    return;
-
   base::SharedMemoryHandle memory_handle;
   size_t memory_size = 0;
   bool read_only_flag = false;
 
   const MojoResult result = mojo::UnwrapSharedMemoryHandle(
       std::move(handle), &memory_handle, &memory_size, &read_only_flag);
   DCHECK_EQ(MOJO_RESULT_OK, result);
   DCHECK_GT(memory_size, 0u);
 
   std::unique_ptr<base::SharedMemory> shm(
       new base::SharedMemory(memory_handle, true /* read_only */));
   if (!shm->Map(memory_size)) {
     DLOG(ERROR) << "OnBufferCreated: Map failed.";
     return;
   }
   const bool inserted =
       client_buffers_
           .insert(std::make_pair(buffer_id,
                                  new ClientBuffer(std::move(shm), memory_size)))
           .second;
   DCHECK(inserted);
 }
 
 void VideoCaptureImpl::OnBufferReady(int32_t buffer_id,
                                      media::mojom::VideoFrameInfoPtr info) {
   DVLOG(1) << __func__ << " buffer_id: " << buffer_id;
   DCHECK(io_thread_checker_.CalledOnValidThread());
 
   bool consume_buffer = state_ == VIDEO_CAPTURE_STATE_STARTED;
+  if (!consume_buffer && first_frame_ref_time_.is_null())
+    should_request_refresh_frame_on_started_ = true;
   if ((info->pixel_format != media::PIXEL_FORMAT_I420 &&
        info->pixel_format != media::PIXEL_FORMAT_Y16) ||
       info->storage_type != media::PIXEL_STORAGE_CPU) {
     consume_buffer = false;
     LOG(DFATAL) << "Wrong pixel format or storage, got pixel format:"
                 << VideoPixelFormatToString(info->pixel_format)
                 << ", storage:" << info->storage_type;
   }
   if (!consume_buffer) {
     GetVideoCaptureHost()->ReleaseBuffer(device_id_, buffer_id, -1.0);
@@ skipping 102 matching lines @@
   params_.requested_format.frame_size.SetSize(width, height);
   DVLOG(1) << __func__ << " " << params_.requested_format.frame_size.ToString();
   StartCaptureInternal();
 }
 
 void VideoCaptureImpl::StartCaptureInternal() {
   DCHECK(io_thread_checker_.CalledOnValidThread());
 
   GetVideoCaptureHost()->Start(device_id_, session_id_, params_,
                                observer_binding_.CreateInterfacePtrAndBind());
-  state_ = VIDEO_CAPTURE_STATE_STARTED;

[miu, 2017/03/06 22:37:33]

Retain this, but it should be changed to:

  state_ = VIDEO_CAPTURE_STATE_STARTING;

[braveyao, 2017/03/08 22:02:52]

Done.

 }
 
 void VideoCaptureImpl::OnDeviceSupportedFormats(
     const VideoCaptureDeviceFormatsCB& callback,
     const media::VideoCaptureFormats& supported_formats) {
   DCHECK(io_thread_checker_.CalledOnValidThread());
   callback.Run(supported_formats);
 }
 
 void VideoCaptureImpl::OnDeviceFormatsInUse(
@@ skipping 33 matching lines @@
   // destructor.  |metadata| is still valid for read-access at this point.
   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(consumer_resource_utilization);
 }
 
 }  // namespace content