Readability review.

remoting/host/video_frame_recorder.h

 // 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.
 
 #ifndef REMOTING_HOST_VIDEO_FRAME_RECORDER_H_
 #define REMOTING_HOST_VIDEO_FRAME_RECORDER_H_
 
 #include <stdint.h>

[Peter Kasting, 2014/08/22 06:38:30]

Nit: Prefer C++ headers to C ones (<cstdint>)

[Wez, 2014/08/25 23:23:12]

Done.

[Wez, 2014/08/26 19:22:25]

Hmmm, stdint.h -> cstdint seems to break Mac Chromium Compile Dbg. :(

[Peter Kasting, 2014/08/26 20:58:14]

Huh!  Apparently cstdint is formally C++11, and I guess Mac Clang is being
strict about that.  Better change back to stdint.h then... sorry!

[Wez, 2014/08/28 00:13:20]

Done.

 #include <list>
 
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/single_thread_task_runner.h"
 
 namespace webrtc {
 class DesktopFrame;
 }
 
 namespace remoting {
 
 class VideoEncoder;
 
 // Allows sequences of DesktopFrames passed to a VideoEncoder to be recorded.
 //
 // VideoFrameRecorder is designed to support applications which use a dedicated
 // thread for video encoding, but need to manage that process from a "main"
 // or "control" thread.
 //
 // On the control thread:
 // 1. Create the VideoFrameRecorder on the controlling thread.

[Peter Kasting, 2014/08/22 06:38:30]

Nit: "on the controlling thread" unnecessary (you're in the "on the control
thread" section)

[Wez, 2014/08/25 23:23:11]

Done.

 // 2. Specify the amount of memory that may be used for recording.
 // 3. Call WrapVideoEncoder(), passing the actual VideoEncoder that will be
 //    used to encode frames.
 // 4. Hand the returned wrapper VideoEncoder of to the video encoding thread,

[Peter Kasting, 2014/08/22 06:38:30]

Nit: of -> off ("Hand off the..." might be even better)

[Wez, 2014/08/25 23:23:11]

Done.

 //    to call in place of the actual VideoEncoder.
 // 5. Start/stop frame recording as necessary.
 // 6. Use NextFrame() to read each recorded frame in sequence.
 //
 // The wrapper VideoEncoder is designed to be handed off to the video encoding
 // thread, and used and torn down there.
 //
 // The VideoFrameRecorder and VideoEncoder may be torn down in any order; frame
 // recording will stop as soon as either is destroyed.
 
 class VideoFrameRecorder {
  public:
   VideoFrameRecorder();
   virtual ~VideoFrameRecorder();
 
   // Wraps the supplied VideoEncoder, returning a replacement VideoEncoder that
   // will route frames to the recorder, as well as passing them for encoding.
   // The caller must delete the previous recording VideoEncoder, or call

[Peter Kasting, 2014/08/22 06:38:30]

Nit: Perhaps this second sentence would be clearer as (assuming I understand it
correctly):

This function must not be called when a functioning wrapper already exists;
either delete the wrapper VideoEncoder entirely, or detach the wrapper from the
underlying encoder by calling DetachVideoEncoderWrapper(), before calling this
method to create a new wrapper.

[Wez, 2014/08/25 23:23:12]

Acknowledged; I've gone with another rewording that I think is clearer.

   // DetachVideoEncoderWrapper() before calling WrapVideoEncoder() to create
   // a new wrapper.
   scoped_ptr<VideoEncoder> WrapVideoEncoder(scoped_ptr<VideoEncoder> encoder);
 
   // Detaches the existing VideoEncoder wrapper, stopping it from recording.

[Peter Kasting, 2014/08/22 06:38:30]

What about the lifetime of the wrapper?  Will it exist but do nothing until
WrapVideoEncoder() is called again?  Say how/when it is cleaned up.

[Wez, 2014/08/25 23:23:12]

Done.

   void DetachVideoEncoderWrapper();
 
   // Enables/disables frame recording. Frame recording is initially disabled.
   void SetEnableRecording(bool enable_recording);
 
   // Sets the maximum number of bytes of pixel data that may be recorded.
   // When this maximum is reached older frames will be discard to make space

[Peter Kasting, 2014/08/22 06:38:30]

Nit: discarded

[Wez, 2014/08/25 23:23:11]

Done.

   // for new ones.
   void SetMaxContentBytes(int64_t max_content_bytes);
 
   // Pops the next recorded frame in the sequence, and returns it.
   scoped_ptr<webrtc::DesktopFrame> NextFrame();
 
  private:
   class RecordingVideoEncoder;
   friend class RecordingVideoEncoder;
 
@@ skipping 20 matching lines @@
 
   scoped_refptr<base::SingleThreadTaskRunner> caller_task_runner_;
   base::WeakPtrFactory<VideoFrameRecorder> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(VideoFrameRecorder);
 };
 
 }  // namespace remoting
 
 #endif  // REMOTING_HOST_VIDEO_FRAME_RECORDER_H_
+