Aura Window Capture: Ensure capture callback is always run.

content/browser/media/capture/aura_window_capture_machine.cc

 // Copyright 2013 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 "content/browser/media/capture/aura_window_capture_machine.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/logging.h"
@@ skipping 181 matching lines @@
 
 void AuraWindowCaptureMachine::DidCopyOutput(
     scoped_refptr<media::VideoFrame> video_frame,
     base::TimeTicks start_time,
     const CaptureFrameCallback& capture_frame_cb,
     std::unique_ptr<cc::CopyOutputResult> result) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   static bool first_call = true;
 
-  bool succeeded = ProcessCopyOutputResponse(
+  const bool succeeded = ProcessCopyOutputResponse(
       video_frame, start_time, capture_frame_cb, std::move(result));
 
   base::TimeDelta capture_time = base::TimeTicks::Now() - start_time;
 
   // The two UMA_ blocks must be put in its own scope since it creates a static
   // variable which expected constant histogram name.
   if (screen_capture_) {
     UMA_HISTOGRAM_TIMES(kUmaScreenCaptureTime, capture_time);
   } else {
     UMA_HISTOGRAM_TIMES(kUmaWindowCaptureTime, capture_time);
   }
 
   if (first_call) {
     first_call = false;
     if (screen_capture_) {
       IncrementDesktopCaptureCounter(succeeded ? FIRST_SCREEN_CAPTURE_SUCCEEDED
                                                : FIRST_SCREEN_CAPTURE_FAILED);
     } else {
       IncrementDesktopCaptureCounter(succeeded
                                          ? FIRST_WINDOW_CAPTURE_SUCCEEDED
                                          : FIRST_WINDOW_CAPTURE_FAILED);
     }
   }
+
+  // If ProcessCopyOutputResponse() failed, it will not run |capture_frame_cb|,
+  // so do that now.
+  if (!succeeded)
+    capture_frame_cb.Run(video_frame, start_time, false);

[GeorgeZ, 2016/04/23 18:47:15]

There is one more loop hole in 
https://code.google.com/p/chromium/codesearch#chromium/src/media/capture/cont...

frame_number = oracle_.RecordCapture(attenuated_utilization); leads to
num_frames_pending_++;

Callback leads to num_frames_pending_--;

if (!(*storage))
    return false;

leads to num_frames_pending_++ and there is no valid call_back.

[miu, 2016/04/26 02:27:58]

Done.  Good catch.

 }
 
 bool AuraWindowCaptureMachine::ProcessCopyOutputResponse(
     scoped_refptr<media::VideoFrame> video_frame,
     base::TimeTicks start_time,
     const CaptureFrameCallback& capture_frame_cb,
     std::unique_ptr<cc::CopyOutputResult> result) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   if (result->IsEmpty() || result->size().IsEmpty() || !desktop_window_)
@@ skipping 67 matching lines @@
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   release_callback->Run(gpu::SyncToken(), false);
 
   // Render the cursor and deliver the captured frame if the
   // AuraWindowCaptureMachine has not been stopped (i.e., the WeakPtr is
   // still valid).
   if (machine) {
     if (machine->cursor_renderer_ && result)
       machine->cursor_renderer_->RenderOnVideoFrame(target);
-    capture_frame_cb.Run(target, start_time, result);
+  } else {
+    result = false;
   }
+
+  capture_frame_cb.Run(target, start_time, result);

[GeorgeZ, 2016/04/23 18:47:15]

You may add a check for result (bool success) in void
ThreadSafeCaptureOracle::DidCaptureFrame() instead of doing it in
oracle_.CompleteCapture. I think do check earlier makes more sensor.

[miu, 2016/04/26 02:27:58]

VideoCaptureOracle::CompleteCapture() must be called regardless of success/fail,
and ThreadSafeCaptureOracle checks the return value from that call to determine
whether to proceed with delivering the video frame to the client.  So, I don't
see how the check could be any sooner than it is.

 }
 
 void AuraWindowCaptureMachine::OnWindowBoundsChanged(
     aura::Window* window,
     const gfx::Rect& old_bounds,
     const gfx::Rect& new_bounds) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(desktop_window_ && window == desktop_window_);
 
   // Post a task to update capture size after first returning to the event loop.
@@ skipping 34 matching lines @@
   // TODO(miu): The CopyOutputRequest should be made earlier, at WillCommit().
   // http://crbug.com/492839
   BrowserThread::PostTask(
       BrowserThread::UI,
       FROM_HERE,
       base::Bind(&AuraWindowCaptureMachine::Capture, weak_factory_.GetWeakPtr(),
                  true));
 }
 
 }  // namespace content