Aura Window Capture: Ensure capture callback is always run.

Aura Window Capture: Ensure capture callback is always run.

The capture callback must always be run, or eventually the capture
decision logic will think there are too many captures in-flight.  This
closes some "holes" in content::AuraWindowCaptureMachine where methods
exit early without running the callback.  This is a potential fix for
crbug 600031.

BUG=600031
Committed: https://crrev.com/47efd876604698bfebe78ef495d789148694f649
Cr-Commit-Position: refs/heads/master@{#389867}

[miu, 2016-04-23 01:19:59]

miu@chromium.org changed reviewers:
+ gyzhou@chromium.org

[miu, 2016-04-23 01:19:59]

gyzhou: PTAL (speculative fix for crbug 600031).

[GeorgeZ, 2016-04-23 18:47:16]

Yuri,

Test with this patch failed in chrome device blaze.
 
A comment is added with screenshot in
https://bugs.chromium.org/p/chromium/issues/detail?id=600031

https://codereview.chromium.org/1913283004/diff/1/content/browser/media/captu...
File content/browser/media/capture/aura_window_capture_machine.cc (right):

https://codereview.chromium.org/1913283004/diff/1/content/browser/media/captu...
content/browser/media/capture/aura_window_capture_machine.cc:230:
capture_frame_cb.Run(video_frame, start_time, false);
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.

https://codereview.chromium.org/1913283004/diff/1/content/browser/media/captu...
content/browser/media/capture/aura_window_capture_machine.cc:322:
capture_frame_cb.Run(target, start_time, result);
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:59]

PTAL.

https://codereview.chromium.org/1913283004/diff/1/content/browser/media/captu...
File content/browser/media/capture/aura_window_capture_machine.cc (right):

https://codereview.chromium.org/1913283004/diff/1/content/browser/media/captu...
content/browser/media/capture/aura_window_capture_machine.cc:230:
capture_frame_cb.Run(video_frame, start_time, false);
On 2016/04/23 18:47:15, GeorgeZ wrote:
> 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.

Done.  Good catch.

https://codereview.chromium.org/1913283004/diff/1/content/browser/media/captu...
content/browser/media/capture/aura_window_capture_machine.cc:322:
capture_frame_cb.Run(target, start_time, result);
On 2016/04/23 18:47:15, GeorgeZ wrote:
> 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.

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.

[GeorgeZ, 2016-04-26 16:30:12]

Miu,

I tested this CL on Device blaze. It solved the issue of frame rate dropping to
zero when screen has no update. As I said, it has nothing with another issue
that is associated with AuraWindowCaptureMachine::OnCompositingEnded().

LGTM

[miu, 2016-04-26 16:39:07]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2016-04-26 16:39:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1913283004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1913283004/20001

[commit-bot: I haz the power, 2016-04-26 18:10:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-26 18:10:42]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[miu, 2016-04-26 18:23:32]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2016-04-26 18:23:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1913283004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1913283004/20001

[commit-bot: I haz the power, 2016-04-26 20:12:58]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-26 20:14:43]

Description was changed from

==========
Aura Window Capture: Ensure capture callback is always run.

The capture callback must always be run, or eventually the capture
decision logic will think there are too many captures in-flight.  This
closes some "holes" in content::AuraWindowCaptureMachine where methods
exit early without running the callback.  This is a potential fix for
crbug 600031.

BUG=600031
==========

to

==========
Aura Window Capture: Ensure capture callback is always run.

The capture callback must always be run, or eventually the capture
decision logic will think there are too many captures in-flight.  This
closes some "holes" in content::AuraWindowCaptureMachine where methods
exit early without running the callback.  This is a potential fix for
crbug 600031.

BUG=600031

Committed: https://crrev.com/47efd876604698bfebe78ef495d789148694f649
Cr-Commit-Position: refs/heads/master@{#389867}
==========

[commit-bot: I haz the power, 2016-04-26 20:14:46]

Patchset 2 (id:??) landed as
https://crrev.com/47efd876604698bfebe78ef495d789148694f649
Cr-Commit-Position: refs/heads/master@{#389867}