ScreenCapture for Android phase1, part I

media/capture/content/android/screen_capture_machine_android.cc

+// Copyright 2016 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 "media/capture/content/android/screen_capture_machine_android.h"
+
+#include "base/android/context_utils.h"
+#include "base/android/jni_android.h"
+#include "base/android/scoped_java_ref.h"
+#include "jni/ScreenCapture_jni.h"
+#include "media/base/video_capture_types.h"
+#include "media/base/yuv_convert.h"
+#include "media/capture/content/video_capture_oracle.h"
+#include "third_party/libyuv/include/libyuv.h"
+
+using base::android::AttachCurrentThread;
+
+namespace media {
+
+// static
+bool ScreenCaptureMachineAndroid::RegisterScreenCaptureMachine(JNIEnv* env) {
+  return RegisterNativesImpl(env);
+}
+
+ScreenCaptureMachineAndroid::ScreenCaptureMachineAndroid() {}
+
+ScreenCaptureMachineAndroid::~ScreenCaptureMachineAndroid() {}
+
+// static
+ScopedJavaLocalRef<jobject>
+ScreenCaptureMachineAndroid::createScreenCaptureMachineAndroid(
+    jlong nativeScreenCaptureMachineAndroid) {
+  return (Java_ScreenCapture_createScreenCaptureMachine(
+      AttachCurrentThread(), base::android::GetApplicationContext(),
+      nativeScreenCaptureMachineAndroid));
+}
+
+void ScreenCaptureMachineAndroid::OnRGBAFrameAvailable(JNIEnv* env,
+                                                       jobject obj,
+                                                       jobject buf,
+                                                       jint row_stride,
+                                                       jint left,
+                                                       jint top,
+                                                       jint width,
+                                                       jint height,
+                                                       jlong timestamp) {
+  scoped_refptr<VideoFrame> i420_frame = VideoFrame::CreateFrame(
+      PIXEL_FORMAT_I420, gfx::Size(width, height), gfx::Rect(width, height),
+      gfx::Size(width, height), base::TimeDelta());
+
+  int offset = top * row_stride + left * 4;

[mcasas, 2016/06/12 10:11:51]

const

[braveyao, 2016/06/13 22:30:32]

Done.

+  // ABGR little endian (rgba in memory) to I420.
+  libyuv::ABGRToI420(
+      reinterpret_cast<uint8_t*>(env->GetDirectBufferAddress(buf)) + offset,
+      row_stride, i420_frame->visible_data(VideoFrame::kYPlane),
+      i420_frame->stride(VideoFrame::kYPlane),
+      i420_frame->visible_data(VideoFrame::kUPlane),
+      i420_frame->stride(VideoFrame::kUPlane),
+      i420_frame->visible_data(VideoFrame::kVPlane),
+      i420_frame->stride(VideoFrame::kVPlane),
+      i420_frame->visible_rect().width(), i420_frame->visible_rect().height());
+
+  OnIncomingFrameAvailable(i420_frame->visible_data(VideoFrame::kYPlane),
+                           i420_frame->stride(VideoFrame::kYPlane),
+                           i420_frame->visible_data(VideoFrame::kUPlane),
+                           i420_frame->stride(VideoFrame::kUPlane),
+                           i420_frame->visible_data(VideoFrame::kVPlane),
+                           i420_frame->stride(VideoFrame::kVPlane),
+                           i420_frame->visible_rect().width(),
+                           i420_frame->visible_rect().height(),
+                           static_cast<int64_t>(timestamp));
+}
+
+void ScreenCaptureMachineAndroid::OnI420FrameAvailable(JNIEnv* env,
+                                                       jobject obj,
+                                                       jobject y_buffer,
+                                                       jint y_stride,
+                                                       jobject u_buffer,
+                                                       jobject v_buffer,
+                                                       jint uv_row_stride,
+                                                       jint uv_pixel_stride,
+                                                       jint left,
+                                                       jint top,
+                                                       jint width,
+                                                       jint height,
+                                                       jlong timestamp) {
+  uint8_t* y_src =

[mcasas, 2016/06/12 10:11:51]

uint8_t* const

[braveyao, 2016/06/13 22:30:32]

Done.

[mcasas, 2016/06/15 15:19:13]

I don't see the "Done"s being done. Did you
forget to upload PS9?

+      reinterpret_cast<uint8_t*>(env->GetDirectBufferAddress(y_buffer));
+  CHECK(y_src);

[mcasas, 2016/06/12 10:11:50]

Why CHECK here and take it face-value in l.54?

[braveyao, 2016/06/13 22:30:32]

Done.

+  uint8_t* u_src =
+      reinterpret_cast<uint8_t*>(env->GetDirectBufferAddress(u_buffer));

[mcasas, 2016/06/12 10:11:51]

uint8_t* const

[braveyao, 2016/06/13 22:30:32]

can't be const. It will be changed below.

+  CHECK(u_src);
+  uint8_t* v_src =
+      reinterpret_cast<uint8_t*>(env->GetDirectBufferAddress(v_buffer));

[mcasas, 2016/06/12 10:11:51]

uint8_t* const

[braveyao, 2016/06/13 22:30:32]

ditto

+  CHECK(v_src);
+
+  // De-interleave the U and V planes into temporary buffers, if needed.
+  int uv_stride = uv_row_stride;
+  std::unique_ptr<uint8_t[]> u_tmp, v_tmp;
+  if (uv_pixel_stride != 1) {
+    // U and V planes are actually interleaved, unpack them here.
+    int uv_plane_len = (int)env->GetDirectBufferCapacity(u_buffer);
+    u_tmp.reset(new uint8_t[(uv_plane_len + 1) / uv_pixel_stride]);
+    v_tmp.reset(new uint8_t[(uv_plane_len + 1) / uv_pixel_stride]);
+    for (int index = 0; index * uv_pixel_stride <= uv_plane_len; index++) {
+      u_tmp[index] = u_src[index * uv_pixel_stride];
+      v_tmp[index] = v_src[index * uv_pixel_stride];
+    }
+    u_src = u_tmp.get();
+    v_src = v_tmp.get();
+    uv_stride /= uv_pixel_stride;
+  }
+
+  int y_offset = top * y_stride + left;
+  int uv_offset = (top / 2) * uv_stride + left / 2;

[mcasas, 2016/06/15 15:19:13]

l.114 and this line should be const.

[braveyao, 2016/06/15 21:56:36]

Done.

+  OnIncomingFrameAvailable(y_src + y_offset, y_stride, u_src + uv_offset,
+                           uv_stride, v_src + uv_offset, uv_stride, width,
+                           height, static_cast<int64_t>(timestamp));
+}
+
+void ScreenCaptureMachineAndroid::OnActivityResult(JNIEnv* env,
+                                                   jobject obj,
+                                                   jboolean result) {
+  if (!result) {
+    oracle_proxy_->ReportError(FROM_HERE, "The user denied screen capture");
+    return;
+  }
+
+  Java_ScreenCapture_startCapture(env, obj);
+}
+
+void ScreenCaptureMachineAndroid::Start(
+    const scoped_refptr<ThreadSafeCaptureOracle>& oracle_proxy,
+    const VideoCaptureParams& params,
+    const base::Callback<void(bool)> callback) {
+  DCHECK(oracle_proxy.get());
+  oracle_proxy_ = oracle_proxy;
+
+  j_capture_.Reset(
+      createScreenCaptureMachineAndroid(reinterpret_cast<intptr_t>(this)));
+
+  if (j_capture_.obj() == nullptr) {
+    LOG(ERROR) << "Failed to createScreenCaptureAndroid,"
+                  " Maybe android version is too low";

[mcasas, 2016/06/12 10:11:50]

DLOG()

and, I think this error message is not really informative.
Say I'm a developer working on building Chromium, and
I see this message... what does it mean "Maybe"?

[braveyao, 2016/06/13 22:30:32]

Done.

+    callback.Run(false);
+    return;
+  }
+
+  DCHECK(params.requested_format.frame_size.GetArea());
+  DCHECK(!(params.requested_format.frame_size.width() % 2));
+  DCHECK(!(params.requested_format.frame_size.height() % 2));
+
+  const jboolean ret = Java_ScreenCapture_startPrompt(
+      AttachCurrentThread(), j_capture_.obj(),
+      params.requested_format.frame_size.width(),
+      params.requested_format.frame_size.height());
+
+  callback.Run(ret);
+}
+
+void ScreenCaptureMachineAndroid::Stop(const base::Closure& callback) {
+  Java_ScreenCapture_stopCapture(AttachCurrentThread(), j_capture_.obj());
+
+  callback.Run();
+}
+
+// ScreenCapture on Android works in passive way and there will be no captured
+// frame when there is no update to the screen. When oracle asks capturing for
+// refresh, the cached last frame will be delivered.

[mcasas, 2016/06/12 10:11:50]

Strange reading. Suggestions:

 ... in _a_ passive way and there _are_ no
 captured frame_s_ ...When _the_ oracle asks 
 _for a capture refresh_, the _cached captured 
frame_ is redelivered.

[braveyao, 2016/06/13 22:30:32]

Done.

+void ScreenCaptureMachineAndroid::MaybeCaptureForRefresh() {
+  if (lastFrame_.get() == nullptr)
+    return;
+
+  OnIncomingFrameAvailable(lastFrame_->visible_data(VideoFrame::kYPlane),
+                           lastFrame_->stride(VideoFrame::kYPlane),
+                           lastFrame_->visible_data(VideoFrame::kUPlane),
+                           lastFrame_->stride(VideoFrame::kUPlane),
+                           lastFrame_->visible_data(VideoFrame::kVPlane),
+                           lastFrame_->stride(VideoFrame::kVPlane),
+                           lastFrame_->visible_rect().width(),
+                           lastFrame_->visible_rect().height(),
+                           base::TimeTicks::Now().ToInternalValue());
+}
+
+void ScreenCaptureMachineAndroid::OnIncomingFrameAvailable(const uint8_t* y_src,
+                                                           int y_stride,
+                                                           const uint8_t* u_src,
+                                                           int u_stride,
+                                                           const uint8_t* v_src,
+                                                           int v_stride,
+                                                           int width,
+                                                           int height,
+                                                           int64_t timestamp) {
+  const VideoCaptureOracle::Event event = VideoCaptureOracle::kCompositorUpdate;
+  const uint64_t absolute_micro =
+      timestamp / base::Time::kNanosecondsPerMicrosecond;
+  const base::TimeTicks start_time =
+      base::TimeTicks() + base::TimeDelta::FromMicroseconds(absolute_micro);
+  scoped_refptr<VideoFrame> frame;
+  ThreadSafeCaptureOracle::CaptureFrameCallback capture_frame_cb;
+
+  if (!oracle_proxy_->ObserveEventAndDecideCapture(
+          event, gfx::Rect(), start_time, &frame, &capture_frame_cb)) {
+    return;
+  }
+
+  DCHECK(frame->format() == PIXEL_FORMAT_I420 ||
+         frame->format() == PIXEL_FORMAT_YV12);
+
+  libyuv::I420Scale(y_src, y_stride, u_src, u_stride, v_src, v_stride, width,

[mcasas, 2016/06/12 10:11:51]

I don't like that we have a step of libyuv::ABGRToI420()
-- or a hand made deinterlacing. Those step seem 
wasteful if you're going to Scale here to a smaller 
resolution, i.e., why not convert-and-scale at the 
same time?

Moreover, you're allocating heap memory (l.47 or
l.103-104) for _every_ incoming frame, which is
tantamount to memory bashing and, in Android
devices, probably not good at all. 

How are you addressing these concerns? Are
you planning on having content_browsertests?
I see miu@ has lgtm'd but at the very least, you
need a Todo (with a crbug, obviously), to have
another, more in depth, look at all this code.

[braveyao, 2016/06/13 22:30:32]

Not that wasteful. 
As to OnRGBAFrameAvailable, I just want to reuse the codes in
OnIncomingFrameAvailable(). So far I haven't observed any scaling happening.
I420Scale() will do a plane-copy only for most of time. If you don't mind having
some duplicated codes, I can make OnRGABFrameAvailable() to have the codes of
OnIncomingFrameAvailable(). You call!
Codes are here:
void ScreenCaptureMachineAndroid::OnRGBAFrameAvailable(JNIEnv* env,
                                                       jobject obj,
                                                       jobject buf,
                                                       jint row_stride,
                                                       jint left,
                                                       jint top,
                                                       jint width,
                                                       jint height,
                                                       jlong timestamp) {
  uint8_t* const src =
      reinterpret_cast<uint8_t*>(env->GetDirectBufferAddress(buf));
  CHECK(src);

  const VideoCaptureOracle::Event event = VideoCaptureOracle::kCompositorUpdate;
  const uint64_t absolute_micro =
      timestamp / base::Time::kNanosecondsPerMicrosecond;
  const base::TimeTicks start_time =
      base::TimeTicks() + base::TimeDelta::FromMicroseconds(absolute_micro);
  scoped_refptr<VideoFrame> frame;
  ThreadSafeCaptureOracle::CaptureFrameCallback capture_frame_cb;

  if (!oracle_proxy_->ObserveEventAndDecideCapture(
          event, gfx::Rect(), start_time, &frame, &capture_frame_cb)) {
    return;
  }

  DCHECK(frame->format() == PIXEL_FORMAT_I420 ||
         frame->format() == PIXEL_FORMAT_YV12);

  const int offset = top * row_stride + left * 4;
  // ABGR little endian (rgba in memory) to I420.
  libyuv::ABGRToI420(
      src + offset, row_stride, frame->visible_data(VideoFrame::kYPlane),
      frame->stride(VideoFrame::kYPlane),
      frame->visible_data(VideoFrame::kUPlane),
      frame->stride(VideoFrame::kUPlane),
      frame->visible_data(VideoFrame::kVPlane),
      frame->stride(VideoFrame::kVPlane),
      frame->visible_rect().width(), frame->visible_rect().height());

  capture_frame_cb.Run(frame, start_time, true);

  lastFrame_ = frame;
}

As to the hand-made-deinterlacing, it is better because the only thing we know
for sure is that we can start at the pointers and read every other bytes from
there, despite the real output format from Android (I420/NV21/NV12) which is not
documented or guaranteed on all platforms. Miu@ and I had a discuss about this
and both agree that hand-made-deinterlacing is more appropriate here.

[mcasas, 2016/06/15 15:19:13]

What do you mean "for most of time" ? Regardless...

IIUC, this is bad because it seems like you're 
adding a code path (the OnRGBA... --> OnIncomingFrameAvailable() )
that doesn't need to be there, since you have never 
"observed any scaling happening", which essentially
deems this code unnecessary "most of the time" 
(all the time?).
Then please separate both paths: Create an 
OnRGBAFrameAvailable() that calls 
libyuv::ABGRToI420() with the appropriate final
resolution and then sends it to |capture_frame_cb|.
(If that's what you're doing in the wall of code
below, then fine).
Then please:
1 - file a bug against libyuv issue tracker describing your 
 situation and why you can't use the library function. 
  https://bugs.chromium.org/p/libyuv

then, add a comment in OnI420FrameAvailable() :
2 - explain why you cannot just deinterlace using a "normal" 
 libyuv function
3 - add links to Android Developer documentation backing
 your statements
4 - and, obviously, add a reference to the libyuv bug number 
in the comment you're writing. When you add that URL, make 
sure it is a well formed URL.

[braveyao, 2016/06/15 21:56:37]

Done.

+                    height, frame->visible_data(VideoFrame::kYPlane),
+                    frame->stride(VideoFrame::kYPlane),
+                    frame->visible_data(VideoFrame::kUPlane),
+                    frame->stride(VideoFrame::kUPlane),
+                    frame->visible_data(VideoFrame::kVPlane),
+                    frame->stride(VideoFrame::kVPlane),
+                    frame->visible_rect().width(),
+                    frame->visible_rect().height(), libyuv::kFilterBilinear);
+
+  // Deliver the populated VideoFrame.

[mcasas, 2016/06/12 10:11:50]

nit: superfluous comment?

[braveyao, 2016/06/13 22:30:32]

Done.

+  capture_frame_cb.Run(frame, start_time, true);
+
+  lastFrame_ = frame;
+}
+
+}  // namespace media