Delete AndroidVideoCapturer::FrameFactory.

webrtc/api/androidvideocapturer.cc

Index: webrtc/api/androidvideocapturer.cc
diff --git a/webrtc/api/androidvideocapturer.cc b/webrtc/api/androidvideocapturer.cc
index 71a94fedb060b584c02c85469889558128856ac3..aee70866a3f6bb36411a968d2742ccbdfa5123c1 100644
--- a/webrtc/api/androidvideocapturer.cc
+++ b/webrtc/api/androidvideocapturer.cc
@@ -19,103 +19,10 @@
 
 namespace webrtc {
 
-// A hack for avoiding deep frame copies in
-// cricket::VideoCapturer.SignalFrameCaptured() using a custom FrameFactory.
-// A frame is injected using UpdateCapturedFrame(), and converted into a
-// cricket::VideoFrame with CreateAliasedFrame(). UpdateCapturedFrame() should
-// be called before CreateAliasedFrame() for every frame.
-// TODO(magjed): Add an interface cricket::VideoCapturer::OnFrameCaptured()
-// for ref counted I420 frames instead of this hack.
-class AndroidVideoCapturer::FrameFactory : public cricket::VideoFrameFactory {
- public:
-  explicit FrameFactory(
-      const rtc::scoped_refptr<AndroidVideoCapturerDelegate>& delegate)
-      : delegate_(delegate) {
-    // Create a CapturedFrame that only contains header information, not the
-    // actual pixel data.
-    captured_frame_.pixel_height = 1;
-    captured_frame_.pixel_width = 1;
-    captured_frame_.data = nullptr;
-    captured_frame_.data_size = cricket::CapturedFrame::kUnknownDataSize;
-    captured_frame_.fourcc = static_cast<uint32_t>(cricket::FOURCC_ANY);
-  }
-
-  void UpdateCapturedFrame(
-      const rtc::scoped_refptr<webrtc::VideoFrameBuffer>& buffer,
-      int rotation,
-      int64_t time_stamp_in_ns) {
-    RTC_DCHECK(rotation == 0 || rotation == 90 || rotation == 180 ||
-               rotation == 270);
-    buffer_ = buffer;
-    captured_frame_.width = buffer->width();
-    captured_frame_.height = buffer->height();
-    captured_frame_.time_stamp = time_stamp_in_ns;
-    captured_frame_.rotation = static_cast<webrtc::VideoRotation>(rotation);
-  }
-
-  void ClearCapturedFrame() {
-    buffer_ = nullptr;
-    captured_frame_.width = 0;
-    captured_frame_.height = 0;
-    captured_frame_.time_stamp = 0;
-  }
-
-  const cricket::CapturedFrame* GetCapturedFrame() const {
-    return &captured_frame_;
-  }
-
-  cricket::VideoFrame* CreateAliasedFrame(
-      const cricket::CapturedFrame* captured_frame,
-      int dst_width,
-      int dst_height) const override {
-    // Check that captured_frame is actually our frame.
-    RTC_CHECK(captured_frame == &captured_frame_);
-    RTC_CHECK(buffer_->native_handle() == nullptr);
-
-    std::unique_ptr<cricket::VideoFrame> frame(new cricket::WebRtcVideoFrame(
-        ShallowCenterCrop(buffer_, dst_width, dst_height),
-        captured_frame->time_stamp, captured_frame->rotation));
-    // Caller takes ownership.
-    // TODO(magjed): Change CreateAliasedFrame() to return a std::unique_ptr.
-    return apply_rotation_ ? frame->GetCopyWithRotationApplied()->Copy()
-                           : frame.release();
-  }
-
-  cricket::VideoFrame* CreateAliasedFrame(
-      const cricket::CapturedFrame* input_frame,
-      int cropped_input_width,
-      int cropped_input_height,
-      int output_width,
-      int output_height) const override {
-    if (buffer_->native_handle() != nullptr) {
-      rtc::scoped_refptr<webrtc::VideoFrameBuffer> scaled_buffer(
-          static_cast<webrtc_jni::AndroidTextureBuffer*>(buffer_.get())
-              ->CropScaleAndRotate(cropped_input_width, cropped_input_height,
-                                   output_width, output_height,
-                                   apply_rotation_ ? input_frame->rotation
-                                                   : webrtc::kVideoRotation_0));
-      return new cricket::WebRtcVideoFrame(
-          scaled_buffer, input_frame->time_stamp,
-          apply_rotation_ ? webrtc::kVideoRotation_0 : input_frame->rotation);
-    }
-    return VideoFrameFactory::CreateAliasedFrame(input_frame,
-                                                 cropped_input_width,
-                                                 cropped_input_height,
-                                                 output_width,
-                                                 output_height);
-  }
-
- private:
-  rtc::scoped_refptr<webrtc::VideoFrameBuffer> buffer_;
-  cricket::CapturedFrame captured_frame_;
-  rtc::scoped_refptr<AndroidVideoCapturerDelegate> delegate_;
-};
-
 AndroidVideoCapturer::AndroidVideoCapturer(
     const rtc::scoped_refptr<AndroidVideoCapturerDelegate>& delegate)
     : running_(false),
       delegate_(delegate),
-      frame_factory_(NULL),
       current_state_(cricket::CS_STOPPED) {
   thread_checker_.DetachFromThread();
   SetSupportedFormats(delegate_->GetSupportedFormats());
@@ -133,9 +40,6 @@ cricket::CaptureState AndroidVideoCapturer::Start(
   LOG(LS_INFO) << " AndroidVideoCapturer::Start " << capture_format.width << "x"
                << capture_format.height << "@" << fps;
 
-  frame_factory_ = new AndroidVideoCapturer::FrameFactory(delegate_.get());
-  set_frame_factory(frame_factory_);
-
   running_ = true;
   delegate_->Start(capture_format.width, capture_format.height, fps, this);
   SetCaptureFormat(&capture_format);
@@ -178,12 +82,57 @@ void AndroidVideoCapturer::OnCapturerStarted(bool success) {
 
 void AndroidVideoCapturer::OnIncomingFrame(

[magjed_webrtc, 2016/05/17 12:12:39]

Add two methods OnIncomingTexture and OnIncomingNV21 instead, and delegate from
androidvideocapturer_jni. You can use NV12ToI420Rotate directly instead of
rotating with GetCopyWithRotationApplied. Also, try to move the function
CropScaleAndRotate from AndroidTextureBuffer to NativeHandle instead.

[nisse-webrtc, 2016/05/18 11:16:33]

Done. (But named OnNV21Frame and OnTextureFrame).

Actually, I think we can choose whether to do this adaptation in
AndroidVideoCapturer or AndroidVideoCApturerJni, it uses only public
VideoCapturer methods, I think.

[magjed_webrtc, 2016/05/18 11:41:59]

If you can choose, I prefer to keep the conversion here in
AndroidVideoCapturerJni and only use public methods on VideoCapturer.

[nisse-webrtc, 2016/05/18 12:17:52]

I'll give it a try.

     const rtc::scoped_refptr<webrtc::VideoFrameBuffer>& buffer,
-    int rotation,
+    int rotation_int,
     int64_t time_stamp) {
   RTC_CHECK(thread_checker_.CalledOnValidThread());
-  frame_factory_->UpdateCapturedFrame(buffer, rotation, time_stamp);
-  SignalFrameCaptured(this, frame_factory_->GetCapturedFrame());
-  frame_factory_->ClearCapturedFrame();
+  RTC_DCHECK(rotation_int == 0 || rotation_int == 90 || rotation_int == 180 ||
+             rotation_int == 270);
+  webrtc::VideoRotation rotation =
+      static_cast<webrtc::VideoRotation>(rotation_int);
+  int adapted_width;
+  int adapted_height;
+  int crop_width;
+  int crop_height;
+  int crop_x;
+  int crop_y;
+
+  if (!AdaptFrame(buffer->width(), buffer->height(),
+                  &adapted_width, &adapted_height,
+                  &crop_width, &crop_height,
+                  &crop_x, &crop_y)) {
+    return;
+  }
+
+  rtc::scoped_refptr<webrtc::VideoFrameBuffer> adapted;
+  if (buffer->native_handle() != nullptr) {
+    // Texture frame.
+    OnFrame(
+        cricket::WebRtcVideoFrame(
+            static_cast<webrtc_jni::AndroidTextureBuffer*>(buffer.get())
+                ->CropScaleAndRotate(
+                    crop_width, crop_height, crop_width, crop_height,
+                    adapted_width, adapted_height,
+                    apply_rotation() ? rotation : webrtc::kVideoRotation_0),
+            time_stamp, apply_rotation() ? webrtc::kVideoRotation_0 : rotation),
+        buffer->width(), buffer->height());
+  } else {
+    // Memory frame.
+    rtc::scoped_refptr<webrtc::VideoFrameBuffer> adapted;
+    if (adapted_width == buffer->width() &&
+        adapted_height == buffer->height() &&
+        crop_width == buffer->width() &&
+        crop_width == buffer->height()) {
+      adapted = buffer;
+    } else {
+      adapted = I420Buffer::CropAndScale(buffer, crop_x, crop_y,

[magjed_webrtc, 2016/05/17 12:19:12]

Use a frame pool instead of allocating new memory every time.

[nisse-webrtc, 2016/05/18 11:16:33]

Haven't done this yet. Would the pool be an additional argument? I'd like the
CropAndScale method to return an I420Buffer, but that doesn't quite work with
the current frame pool.

[magjed_webrtc, 2016/05/18 11:41:59]

I think it's best if the output I420Buffer is an additional argument instead of
the pool, because I don't think I420Buffer should depend on any specific pool
implementation.

[nisse-webrtc, 2016/05/18 12:17:52]

I really dislike destination arguments accessed via MutableDataX()...

I think we should strive to have code modifying pixel data limited to
I420Buffers which the modifying code owns, with the special case of newly
constructed frame buffers. Eventually, I'd like to remove MutableData from the
VideoFrameBuffer base class.

Have to think a bit about the options here.

BTW, I guess we should try to also reuse the buffer used for the unscaled data?

[nisse-webrtc, 2016/05/18 13:54:58]

I now use buffer_pool_ for the unscaled frame, similar to the old code. I guess
we'd need a separate pool for the scaled frame?

+                                         crop_width, crop_height,
+                                         adapted_width, adapted_height);
+    }
+    // TODO(nisse): Introduce a rotate method working with I420Buffer?
+    cricket::WebRtcVideoFrame frame(adapted, time_stamp, rotation);
+    OnFrame(apply_rotation() ? *frame.GetCopyWithRotationApplied() : frame,
+            buffer->width(), buffer->height());
+  }
 }
 
 void AndroidVideoCapturer::OnOutputFormatRequest(