Avoid using MakeExclusive, which is going to be deleted in webrtc.

remoting/protocol/webrtc_video_capturer_adapter.cc

 // Copyright 2015 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 "remoting/protocol/webrtc_video_capturer_adapter.h"
 
 #include <utility>
 
 #include "third_party/libyuv/include/libyuv/convert.h"
 #include "third_party/webrtc/media/engine/webrtcvideoframe.h"
@@ skipping 155 matching lines @@
 
   scoped_ptr<webrtc::DesktopFrame> owned_frame(frame);
 
   // TODO(sergeyu): Currently the adapter keeps generating frames even when
   // nothing is changing on the screen. This is necessary because the video
   // sender drops frames. Obviously this is a suboptimal. The sending code in
   // WebRTC needs to have some mechanism to notify when the bandwidth is
   // exceeded, so the capturer can adapt frame rate.
 
   size_t width = frame->size().width();
   size_t height = frame->size().height();

[Sergey Ulanov, 2016/03/22 17:27:09]

change these two to int then you wouldn't need the casts below.

[nisse-chromium (ooo August 14), 2016/03/24 13:11:26]

Done.

-  if (!yuv_frame_ || yuv_frame_->GetWidth() != width ||
-      yuv_frame_->GetHeight() != height) {
+  if (!yuv_frame_ || static_cast<size_t>(yuv_frame_->width()) != width ||
+      static_cast<size_t>(yuv_frame_->height()) != height) {
     if (!size_callback_.is_null())
       size_callback_.Run(frame->size());
 
-    scoped_ptr<cricket::WebRtcVideoFrame> webrtc_frame(
-        new cricket::WebRtcVideoFrame());
-    webrtc_frame->InitToEmptyBuffer(width, height, 0);
-    yuv_frame_ = std::move(webrtc_frame);
-
+    yuv_frame_ = new rtc::RefCountedObject<webrtc::I420Buffer>(width, height);
     // Set updated_region so the whole frame is converted to YUV below.
     frame->mutable_updated_region()->SetRect(
         webrtc::DesktopRect::MakeWH(width, height));
   }
 
   // TODO(sergeyu): This will copy the buffer if it's being used. Optimize it by

[perkj_chrome, 2016/03/23 14:43:54]

About this todo: Is the whole yuv_frame updated below or just a part of it for
each captured frame? 
If the complete frame is always updated, there is a webrtc::I420BufferPool we
can use instead.

[Sergey Ulanov, 2016/03/23 23:46:43]

The for loop below performs YUV conversion only for parts that are changing.

   // keeping a queue of frames.
-  CHECK(yuv_frame_->MakeExclusive());
-
-  yuv_frame_->SetTimeStamp(base::TimeTicks::Now().ToInternalValue() *
-                           base::Time::kNanosecondsPerMicrosecond);
+  if (!yuv_frame_->HasOneRef()) {
+    // Frame is still used, typically by the encoder. We have to make

[Sergey Ulanov, 2016/03/22 17:27:09]

Please move this comment above the TODO or move the TODO after this comment.

[nisse-chromium (ooo August 14), 2016/03/24 13:11:26]

Done.

+    // a copy before modifying it.
+    yuv_frame_ = yuv_frame_->Copy();
+  }
 
   for (webrtc::DesktopRegion::Iterator i(frame->updated_region()); !i.IsAtEnd();
        i.Advance()) {
     int left = i.rect().left();
     int top = i.rect().top();
     int width = i.rect().width();
     int height = i.rect().height();
 
     if (left % 2 == 1) {
       --left;
       ++width;
     }
     if (top % 2 == 1) {
       --top;
       ++height;
     }
     libyuv::ARGBToI420(
         frame->data() + frame->stride() * top +
             left * webrtc::DesktopFrame::kBytesPerPixel,
-        frame->stride(),
-        yuv_frame_->GetYPlane() + yuv_frame_->GetYPitch() * top + left,
-        yuv_frame_->GetYPitch(),
-        yuv_frame_->GetUPlane() + yuv_frame_->GetUPitch() * top / 2 + left / 2,
-        yuv_frame_->GetUPitch(),
-        yuv_frame_->GetVPlane() + yuv_frame_->GetVPitch() * top / 2 + left / 2,
-        yuv_frame_->GetVPitch(), width, height);
+        frame->stride(), yuv_frame_->MutableData(webrtc::kYPlane) +
+                             yuv_frame_->stride(webrtc::kYPlane) * top + left,
+        yuv_frame_->stride(webrtc::kYPlane),
+        yuv_frame_->MutableData(webrtc::kUPlane) +
+            yuv_frame_->stride(webrtc::kUPlane) * top / 2 + left / 2,
+        yuv_frame_->stride(webrtc::kUPlane),
+        yuv_frame_->MutableData(webrtc::kVPlane) +
+            yuv_frame_->stride(webrtc::kVPlane) * top / 2 + left / 2,
+        yuv_frame_->stride(webrtc::kVPlane), width, height);

[Sergey Ulanov, 2016/03/22 17:27:09]

nit: this ARGBToI420() call would be cleaner if you add variables for the
strides above it, i.e.
  int y_stride = yuv_frame_->stride(webrtc::kYPlane);
  int u_stride = yuv_frame_->stride(webrtc::kUPlane);
  int v_stride = yuv_frame_->stride(webrtc::kVPlane);

FWIW the stride() method should really be called Stride() with capital S. It's
virtual, not inlineable and has a switch statement in it, i.e. calling it is
much more expensive that accessing a local variable.
https://google.github.io/styleguide/cppguide.html#Function_Names

[nisse-chromium (ooo August 14), 2016/03/24 13:11:26]

Done.

   }
 
-  SignalVideoFrame(this, yuv_frame_.get());
+  scoped_ptr<cricket::VideoFrame> video_frame(new cricket::WebRtcVideoFrame(
+      yuv_frame_, base::TimeTicks::Now().ToInternalValue() *

[Sergey Ulanov, 2016/03/22 17:27:09]

This is not a proper way to convert TimeTicks to microseconds. You assume that
internal value is in nanoseconds, but that may not be the case. I suggest
changing this to 
  (base::TimeTicks::Now() - base::TimeTicks()) /
      base::TimeDelta::FromMicroseconds(1)

This expression doesn't depend on how the ticks represented internally.

[nisse-chromium (ooo August 14), 2016/03/24 13:11:26]

Done.

+                      base::Time::kNanosecondsPerMicrosecond,
+      webrtc::kVideoRotation_0));
+
+  SignalVideoFrame(this, video_frame.get());
 }
 
 void WebrtcVideoCapturerAdapter::CaptureNextFrame() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (capture_pending_)
     return;
   capture_pending_ = true;
   desktop_capturer_->Capture(webrtc::DesktopRegion());
 }
 
 }  // namespace protocol
 }  // namespace remoting