DirectX based screen capturer logic

webrtc/modules/desktop_capture/desktop_frame.h

Index: webrtc/modules/desktop_capture/desktop_frame.h
diff --git a/webrtc/modules/desktop_capture/desktop_frame.h b/webrtc/modules/desktop_capture/desktop_frame.h
index 3cd839ca1b817e45f5e4cc7954827c0d1393a0f7..e4c6e8e555b3e618029d20af1b0b24db5e39198b 100644
--- a/webrtc/modules/desktop_capture/desktop_frame.h
+++ b/webrtc/modules/desktop_capture/desktop_frame.h
@@ -26,6 +26,11 @@ class DesktopFrame {
   // DesktopFrame objects always hold RGBA data.
   static const int kBytesPerPixel = 4;
 
+  DesktopFrame(DesktopSize size,

[Sergey Ulanov, 2016/04/14 23:10:42]

I don't think we want this constructor public. It doesn't take ownership of the
buffer, so it's easy to use it incorrectly

[Hzj_jie, 2016/04/15 19:42:17]

Sure, I will write a new class in screen_capturer_win_directx.cc to achieve the
similar functionality.

+               int stride,
+               uint8_t* data,
+               SharedMemory* shared_memory);
+
   virtual ~DesktopFrame();
 
   // Size of the frame.
@@ -62,7 +67,8 @@ class DesktopFrame {
 
   // Copies pixels from a buffer or another frame. |dest_rect| rect must lay
   // within bounds of this frame.
-  void CopyPixelsFrom(uint8_t* src_buffer, int src_stride,
+  void CopyPixelsFrom(uint8_t* src_buffer,
+                      int src_stride,
                       const DesktopRect& dest_rect);
   void CopyPixelsFrom(const DesktopFrame& src_frame,
                       const DesktopVector& src_pos,
@@ -72,26 +78,21 @@ class DesktopFrame {
   uint8_t* GetFrameDataAtPos(const DesktopVector& pos) const;
 
  protected:
-  DesktopFrame(DesktopSize size,
-               int stride,
-               uint8_t* data,
-               SharedMemory* shared_memory);
-
-  const DesktopSize size_;
-  const int stride_;
-
   // Ownership of the buffers is defined by the classes that inherit from this
   // class. They must guarantee that the buffer is not deleted before the frame
   // is deleted.
   uint8_t* const data_;
   SharedMemory* const shared_memory_;
 
+ private:

[Sergey Ulanov, 2016/04/14 23:10:42]

If you are making these members private then I think |data_| and
|shared_memory_| should also be private.

[Hzj_jie, 2016/04/15 19:42:17]

Unfortunately SharedMemoryDesktopFrame uses it in destructor.

+  const DesktopSize size_;
+  const int stride_;
+
   DesktopRegion updated_region_;
   DesktopVector dpi_;
   int64_t capture_time_ms_;
   std::unique_ptr<DesktopRegion> shape_;
 
- private:
   RTC_DISALLOW_COPY_AND_ASSIGN(DesktopFrame);
 };
 
@@ -115,14 +116,16 @@ class SharedMemoryDesktopFrame : public DesktopFrame {
       DesktopSize size,
       SharedMemoryFactory* shared_memory_factory);
 
+  static std::unique_ptr<DesktopFrame> Create(
+      DesktopSize size,
+      int stride,

[Sergey Ulanov, 2016/04/14 23:10:42]

Why do you need this constructor?

[Hzj_jie, 2016/04/15 19:42:17]

In Windows, the pitch (stride in DesktopFrame) may not be consistent with
size.width() * 4. i.e. GPU renders a slightly larger area than the screen, I
have reproduced the issue on Thinkpad X130e.

And the reason I have removed unique_ptr based constructor is because the order
of evaluating parameters are not guaranteed to be fixed. i.e.
shared_memory.release() may be called before shared_memory->data(), and cause a
crash.

[Sergey Ulanov, 2016/04/19 23:51:07]

I understand that, but this constructor allocates new buffer which doesn't have
to have the same stride as the source buffer. CopyPixelsFrom() should be able
handle the case when stride is different.
It's still better to use unique_ptr<> whenever ownership is passed. Add a
uint8_t* data parameter and make that constructor private. Also while you are
here please add a Create() method that takes std::unique_ptr<SharedMemory>
instead of shared_memory_factory - that will allow to address your TODO below.

[Hzj_jie, 2016/04/26 23:00:07]

This constructor is used by remoting/protocol/fake_desktop_capturer.cc, I will
change fake_desktop_capturer.cc before privatize this constructor.

+      SharedMemoryFactory* shared_memory_factory);
+
   // Takes ownership of |shared_memory|.
-  // TODO(sergeyu): Remove this constructor and keep the second one.
+  // TODO(zijiehe): Hide constructors, Create function is preferred.
   SharedMemoryDesktopFrame(DesktopSize size,
                            int stride,
                            SharedMemory* shared_memory);
-  SharedMemoryDesktopFrame(DesktopSize size,
-                           int stride,
-                           std::unique_ptr<SharedMemory> shared_memory);
   ~SharedMemoryDesktopFrame() override;
 
  private: