DevTools: FrameRecorder: add cancelRecordingFrames command

content/browser/devtools/protocol/frame_recorder.cc

 // Copyright 2014 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/devtools/protocol/frame_recorder.h"
 
 #include "base/base64.h"
 #include "base/bind.h"
 #include "base/macros.h"
 #include "base/task_runner_util.h"
 #include "base/threading/worker_pool.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
 #include "content/browser/renderer_host/render_widget_host_view_base.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/gfx/codec/png_codec.h"
 #include "ui/gfx/geometry/size.h"
 
 namespace content {
 namespace devtools {
 namespace page {
 
 namespace {
 
 static int kMaxRecordFrameCount = 180;
 
-std::string EncodeFrame(const SkBitmap& bitmap) {
+std::pair<std::string, double> EncodeFrame(

[dgozman, 2015/01/29 17:53:06]

Looks like you are copying encoded data a number of times. Can we avoid that?

[eustas, 2015/01/30 09:26:30]

Done. Wrapped to scoped_ptr

+    const SkBitmap& bitmap, double timestamp) {
   std::vector<unsigned char> data;
   SkAutoLockPixels lock_image(bitmap);
   bool encoded = gfx::PNGCodec::Encode(
       reinterpret_cast<unsigned char*>(bitmap.getAddr32(0, 0)),
       gfx::PNGCodec::FORMAT_SkBitmap,
       gfx::Size(bitmap.width(), bitmap.height()),
       bitmap.width() * bitmap.bytesPerPixel(),
       false, std::vector<gfx::PNGCodec::Comment>(), &data);
 
   if (!encoded)
-    return std::string();
+    return std::make_pair(std::string(), timestamp);
 
   std::string base_64_data;
   base::Base64Encode(
       base::StringPiece(reinterpret_cast<char*>(&data[0]), data.size()),
       &base_64_data);
 
-  return base_64_data;
+  return std::make_pair(base_64_data, timestamp);
 }
 } // namespace
 
 typedef DevToolsProtocolClient::Response Response;
 
 FrameRecorder::FrameRecorder()
     : host_(nullptr),
       state_(Ready),
       inflight_requests_count_(0),
       max_frame_count_(0),
       captured_frames_count_(0),
       last_captured_frame_timestamp_(base::Time()),
       weak_factory_(this) {
 }
 
 FrameRecorder::~FrameRecorder() {
 }
 
 void FrameRecorder::SetRenderViewHost(RenderViewHostImpl* host) {
   host_ = host;
 }
 
 Response FrameRecorder::StartRecordingFrames(int max_frame_count) {
   if (max_frame_count <= 0 || max_frame_count > kMaxRecordFrameCount)
     return Response::InvalidParams("maxFrameCount");
-  if (state_ != Ready)
+  if (state_ != Ready && state_ != Encoding)
     return Response::InternalError("Already recording");
+  if (state_ == Encoding) {

[dgozman, 2015/01/29 17:53:06]

No. This should be an error. Let the caller ensure correct call sequence.

[eustas, 2015/01/30 09:26:30]

Workflow:
(Ready) + Start -> (Recording)
(Recording) + Stop -> (Encoding)
(Encoding) -> (Ready)

There is transition that is not controlled directly.

Scenario: user starts recording frames shortly after they stopped. Obviously,
previous recording is out of interest.

[eustas, 2015/01/30 09:26:30]

Done.

+    std::vector<scoped_refptr<devtools::page::RecordedFrame>> no_frames;
+    callback_.Run(StopRecordingFramesResponse::Create()->set_frames(no_frames));
+  }
+  frame_encoded_callback_.Reset(
+      base::Bind(&FrameRecorder::FrameEncoded, weak_factory_.GetWeakPtr()));
   state_ = Recording;
   max_frame_count_ = max_frame_count;
   captured_frames_count_ = 0;
   last_captured_frame_timestamp_ = base::Time();
   std::vector<scoped_refptr<devtools::page::RecordedFrame>> frames;
   frames.reserve(max_frame_count);
   frames_.swap(frames);
 
   return Response::OK();
 }
 
 Response FrameRecorder::StopRecordingFrames(
-    StopRecordingFramesCallback callback) {
+    StopRecordingFramesCallback callback, bool cancel) {
   if (state_ != Recording)

[dgozman, 2015/01/29 17:53:06]

You should be able to cancel during encoding. Perhaps, it would be better with a
separate |cancelRecordingFrames| call.

[eustas, 2015/01/30 09:26:30]

Done.

     return Response::InternalError("Not recording");
+  if (cancel) {
+    frame_encoded_callback_.Cancel();
+    std::vector<scoped_refptr<devtools::page::RecordedFrame>> no_frames;
+    frames_.swap(no_frames);
+    callback.Run(StopRecordingFramesResponse::Create()->set_frames(no_frames));

[dgozman, 2015/01/29 17:53:06]

|no_frames| has been swapped and contains collected frames, which you should not
send.

[eustas, 2015/01/30 09:26:30]

Ooops.

+    state_ = Ready;
+    return Response::OK();
+  }
   state_ = Encoding;
   callback_ = callback;
   MaybeSendResponse();
   return Response::OK();
 }
 
 void FrameRecorder::OnSwapCompositorFrame() {
   if (!host_ || state_ != Recording)
     return;
   if (captured_frames_count_ >= max_frame_count_)
@@ skipping 16 matching lines @@
   inflight_requests_count_--;
   base::Time timestamp = last_captured_frame_timestamp_;
   last_captured_frame_timestamp_ = base::Time::Now();
   if (timestamp.is_null() || response != READBACK_SUCCESS)
     return;
 
   captured_frames_count_++;
   base::PostTaskAndReplyWithResult(
       base::WorkerPool::GetTaskRunner(true).get(),
       FROM_HERE,
-      base::Bind(&EncodeFrame, bitmap),
-      base::Bind(&FrameRecorder::FrameEncoded, weak_factory_.GetWeakPtr(),
-                 timestamp.ToDoubleT()));
+      base::Bind(&EncodeFrame, bitmap, timestamp.ToDoubleT()),
+      frame_encoded_callback_.callback());
 }
 
 void FrameRecorder::FrameEncoded(
-    double timestamp, const std::string& encoded_frame) {
+    const std::pair<std::string, double>& encoded_frame) {
   frames_.push_back(RecordedFrame::Create()
-      ->set_data(encoded_frame)
-      ->set_timestamp(timestamp));
+      ->set_data(encoded_frame.first)
+      ->set_timestamp(encoded_frame.second));
   MaybeSendResponse();
 }
 
 void FrameRecorder::MaybeSendResponse() {
   if (state_ != Encoding)
     return;
   if (inflight_requests_count_ || frames_.size() != captured_frames_count_)
     return;
   callback_.Run(StopRecordingFramesResponse::Create()->set_frames(frames_));
   std::vector<scoped_refptr<devtools::page::RecordedFrame>> frames;
   frames_.swap(frames);
   state_ = Ready;
 }
 
 }  // namespace page
 }  // namespace devtools
 }  // namespace content