Use webrtc::DesktopCapturer for screen capturer implementation.

media/video/capture/screen/screen_capture_frame_queue.cc

Index: media/video/capture/screen/screen_capture_frame_queue.cc
diff --git a/media/video/capture/screen/screen_capture_frame_queue.cc b/media/video/capture/screen/screen_capture_frame_queue.cc
index 610a0628cfb753bf618d1779d003609c1ca2a79c..3027c29e035da66f294c8d77b4e6accd4feceef8 100644
--- a/media/video/capture/screen/screen_capture_frame_queue.cc
+++ b/media/video/capture/screen/screen_capture_frame_queue.cc
@@ -7,28 +7,81 @@
 #include <algorithm>
 
 #include "base/basictypes.h"
-#include "media/video/capture/screen/screen_capture_frame.h"
+#include "base/logging.h"
+#include "base/threading/non_thread_safe.h"
+#include "third_party/webrtc/modules/desktop_capture/desktop_frame.h"
 
 namespace media {
 
+class ScreenCaptureFrameQueue::EmittedFrame

[alexeypa (please no reviews), 2013/04/26 21:33:58]

Describe briefly what this class is for.

[Sergey Ulanov, 2013/05/07 22:25:50]

Done.

+    : public webrtc::DesktopFrame,

[alexeypa (please no reviews), 2013/04/26 21:33:58]

It does not look EmittedFrame is ever used as webrtc::DesktopFrame. Only the
destructor and DetachFromQueue() is used.

[Sergey Ulanov, 2013/05/07 22:25:50]

Actually EmitCurrentFrameAndMoveToNext() returns instances of this class.

+      public base::NonThreadSafe {
+ public:
+  EmittedFrame(webrtc::DesktopFrame* frame, ScreenCaptureFrameQueue* queue)
+      : DesktopFrame(frame->size(), frame->stride(), frame->data(),
+                     frame->shared_memory()),
+        frame_(frame),
+        queue_(queue) {
+    set_dpi(frame->dpi());
+    set_capture_time_ms(frame->capture_time_ms());
+    updated_region_ = frame->updated_region();
+  }
+
+  virtual ~EmittedFrame() {
+    if (queue_)
+      queue_->ReturnEmittedFrame(frame_.release());
+  }
+
+  void DetachFromQueue() {
+    queue_ = NULL;
+  }
+
+ private:
+  scoped_ptr<webrtc::DesktopFrame> frame_;
+  ScreenCaptureFrameQueue* queue_;
+};
+
 ScreenCaptureFrameQueue::ScreenCaptureFrameQueue()
     : current_(0),
       previous_(NULL) {
+  memset(frames_, 0, sizeof(frames_));
+  memset(emitted_frames_, 0, sizeof(emitted_frames_));
   SetAllFramesNeedUpdate();
 }
 
 ScreenCaptureFrameQueue::~ScreenCaptureFrameQueue() {
+  for (int i = 0; i < kQueueLength; ++i) {
+    if (emitted_frames_[i]) {
+      emitted_frames_[i]->DetachFromQueue();
+    } else {
+      delete frames_[i];
+    }
+  }
 }
 
-void ScreenCaptureFrameQueue::DoneWithCurrentFrame() {
+webrtc::DesktopFrame* ScreenCaptureFrameQueue::EmitCurrentFrameAndMoveToNext() {
+  DCHECK(!emitted_frames_[current_]);
+
+  EmittedFrame* result = new EmittedFrame(frames_[current_], this);

[Wez, 2013/04/26 18:48:14]

Add some comments to explain what this logic is trying to achieve, please!

[alexeypa (please no reviews), 2013/04/26 21:33:58]

This code is very confusing. I stared at it for a hour and still can say I fully
understand it.

So it looks like |EmittedFrame| is about ownership of the frame. It involves a
lot of moving parts: ~EmittedFrame(), DetachFromQueue(), and two frame pointers
in the queue. I think it will be much clearer to have a raw frame pointer in the
queue and a boolean flag saying whether it is owned on not.
~ScreenCaptureFrameQueue() already has to release frames manually. With an
explicit flag the logic will be simpler.

Also, please document how frame ownership is passed around.

[Sergey Ulanov, 2013/05/07 22:25:50]

What wouldn't work without downcasting frames to EmittedFrame when calling
DetachFromQueue(), so we need to have two arrays.
Done.

[Sergey Ulanov, 2013/05/07 22:25:50]

I've split this method into two separate methods.

+  emitted_frames_[current_] = result;
   previous_ = current_frame();
   current_ = (current_ + 1) % kQueueLength;
+  if (emitted_frames_[current_]) {

[Wez, 2013/04/26 18:48:14]

Wouldn't it be more correct to leave the frame in-place, and instead CHECK
whether there is already an emitted frame outstanding for this queued-frame,
before creating a new EmittedFrame for it?

[Sergey Ulanov, 2013/05/07 22:25:50]

The DesktopCapturer interface doesn't specify when captured frames must be
deleted, so ideally I think that it should support the case when consumer of
that interface can keep captured frames as long as it wants. That obviously hard
to implement efficiently without duplicating frames, but normally we should
never need to do that with the current code that uses that interface. Anyway, I
split this function into two and added DCHECK() that verifies that EmittedFrame
is deleted before we try to reuse the same frame.

+    emitted_frames_[current_]->DetachFromQueue();

[alexeypa (please no reviews), 2013/04/26 21:33:58]

If I read this right this will always delete the frame instead of reusing it.
There is no point in using this class if we always create new frames.

[Sergey Ulanov, 2013/05/07 22:25:50]

See my comment above, we normally should never reach this code (though it wasn't
implemented correctly here). Anyway I replaced it with a DCHECK in
MoveToNextFrame().

+    emitted_frames_[current_] = NULL;
+    frames_[current_] = NULL;
+  }
+  return result;
 }
 
 void ScreenCaptureFrameQueue::ReplaceCurrentFrame(
-    scoped_ptr<ScreenCaptureFrame> frame) {
-  frames_[current_] = frame.Pass();
+    scoped_ptr<webrtc::DesktopFrame> frame) {
+  frames_[current_] = frame.release();
   needs_update_[current_] = false;
+  if (emitted_frames_[current_]) {
+    emitted_frames_[current_]->DetachFromQueue();

[Wez, 2013/04/26 18:48:14]

For this to happen the caller would have to have caused three frames to be
emitted by the queue, and only then return the third one, which isn't something
calling code should ever do (with the current capturer impls...)

[Sergey Ulanov, 2013/05/07 22:25:50]

Yes, but that's not documented for DesktopCapturer interface. Anyway replaced it
with a DCHECK for now.

+    emitted_frames_[current_] = NULL;
+  }
 }
 
 void ScreenCaptureFrameQueue::SetAllFramesNeedUpdate() {
@@ -36,4 +89,17 @@ void ScreenCaptureFrameQueue::SetAllFramesNeedUpdate() {
   previous_ = NULL;
 }
 
+void ScreenCaptureFrameQueue::ReturnEmittedFrame(
+    webrtc::DesktopFrame* frame) {
+  if (frame == frames_[0]) {
+    emitted_frames_[0] = NULL;

[alexeypa (please no reviews), 2013/04/26 21:33:58]

This does not work if kQueueLength is not 2.

[Sergey Ulanov, 2013/05/07 22:25:50]

Done.

+  } else if (frame == frames_[1]) {
+    emitted_frames_[1] = NULL;
+  } else {
+    if (previous_ == frame)
+      previous_ = NULL;

[Wez, 2013/04/26 18:48:14]

nit: If previous_ were an index into frames_ rather than a pointer to an actual
frame, you wouldn't need this if...

[Sergey Ulanov, 2013/05/07 22:25:50]

Done.

+    delete frame;
+  }
+}
+
 }  // namespace media