Add video capture on Android.

media/video/capture/android/video_capture_device_android.cc

Index: media/video/capture/android/video_capture_device_android.cc
===================================================================
--- media/video/capture/android/video_capture_device_android.cc	(revision 0)
+++ media/video/capture/android/video_capture_device_android.cc	(revision 0)
@@ -0,0 +1,415 @@
+// Copyright (c) 2013 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/video/capture/android/video_capture_device_android.h"
+
+#include <string>
+
+#include "base/android/jni_android.h"
+#include "base/android/scoped_java_ref.h"
+#include "base/string_number_conversions.h"
+#include "base/stringprintf.h"
+#include "jni/VideoCapture_jni.h"
+
+using base::android::AttachCurrentThread;
+using base::android::CheckException;
+using base::android::GetClass;
+using base::android::MethodID;
+using base::android::JavaRef;
+using base::android::ScopedJavaLocalRef;
+
+namespace {
+
+void ResetBuffer(uint8* buffer, int width, int height) {
+  int y_size = width * height;
+  memset(buffer, 0, y_size);
+  buffer += y_size;
+  memset(buffer, 128, y_size / 2);

[Ami GONE FROM CHROMIUM, 2013/01/28 23:55:47]

What about the stride!=width case?
Also this is obviously very I420/YV12-specific; throw that into the name of the
function?

[wjia(left Chromium), 2013/01/30 18:25:47]

We need support width!=stride case, all the way in all VideoCapture classses. I
added a TODO here. Will follow up a patch soon.

Added I420 as part of function name.

+}
+
+void RotatePlaneByPixels(uint8* src, uint8* dest, int width, int height,

[Ami GONE FROM CHROMIUM, 2013/01/28 23:55:47]

I'm confused by your choice here; why ask Camera to give you a raw byte[] and do
the rotation yourself, instead of having Camera render to a SurfaceTexture,
doing the rotation internally (and hopefully more efficiently) using
setDisplayOrientation, and then registering for frame-available notifications
through SurfaceTexture.setOnFrameAvailableListener() ?

[wjia(left Chromium), 2013/01/30 18:25:47]

There is a TODO in VideoCapture.java to investigate whether reading from texture
could give better performance and frame rate. My take is that there is only one
SurfaceTexture for the camera. When its OnFrameAvailable() is called, it will
prevent camera from generating a new frame to avoid pixel data overwriting. In
case OnFrameAvailable() takes a bit longer time, camera will miss one frame.

However, when using PreviewCallbackWithBuffer and multiple callback buffers
(here is 3), we can avoid this. Then it will improve capturing frame rate.

I agree it would be more efficient to use SurfaceTexture directly since camera
doesn't have to copy data into callback buffer.

[Ami GONE FROM CHROMIUM, 2013/01/30 19:46:23]

This explanation doesn't make sense to me.
The code as-is does far more than 3x the amount of work a memcpy would do, so
even blocking OFA() for a single full copy would result in better framerate than
what you're doing now.

Even *that* copy should be unnecessary, since eventually the frame will end up
being composited back out on a texture.  Ideally, VCD::Observer could be taught
about texture-backed frames and avoid the CSC in 
VideoCaptureController::OnIncomingCapturedFrame() entirely, but that's obviously
a much bigger chunk of work.  Opened
https://code.google.com/p/chromium/issues/detail?id=173158 for that.

[wjia(left Chromium), 2013/02/06 00:45:34]

As we discussed offline, CallbackBuffer would be used as intermittent solution.

+                         int rotation, bool flip_vert, bool flip_horiz) {
+  // Consolidate cases. Only 0 and 90 are left.
+  if (rotation == 180 || rotation == 270) {
+    rotation -= 180;
+    flip_vert = !flip_vert;
+    flip_horiz = !flip_horiz;
+  }
+
+  int num_rows = height;
+  int num_cols = width;
+  int src_stride = width;
+  int dest_row_step = width;
+  int dest_col_step = 1;
+
+  if (rotation == 0) {
+    if (flip_horiz) {
+      // Use pixel copying.
+      dest_col_step = -1;
+      if (flip_vert) {
+        // Rotation 180.
+        dest_row_step = -width;
+        dest += height * width - 1;
+      } else {
+        dest += width - 1;
+      }
+    } else {
+      if (flip_vert) {
+        // Fast copy by rows.
+        dest += width * (height - 1);
+        for (int row = 0; row < height; ++row) {
+          memcpy(dest, src, width);
+          src += width;
+          dest -= width;
+        }
+      } else {
+        memcpy(dest, src, width * height);
+      }
+      return;
+    }
+  } else if (rotation == 90) {
+    int offset = (width - height) / 2;
+    if (width > height) {
+      src += offset;
+      num_rows = num_cols = height;
+    } else {
+      src += width * offset;
+      num_rows = num_cols = width;
+      offset = (height - width) / 2;
+    }
+
+    dest_col_step = (flip_vert ? -width : width);
+    if (flip_horiz) {
+      dest_row_step = 1;
+      if (flip_vert) {
+        dest += (width > height ? width * (height - 1) + offset :
+                                  width * (height - offset - 1));
+      } else {
+        dest += (width > height ? offset : width * offset);
+      }
+    } else {
+      dest_row_step = -1;
+      if (flip_vert) {
+        dest += (width > height ?  width * height - offset - 1 :
+                                   width * (height - offset) - 1);
+      } else {
+        dest += (width > height ? width - offset - 1 :
+                                  width * (offset + 1) - 1);
+      }
+    }
+  }
+
+  // Copy pixels.
+  for (int row = 0; row < num_rows; ++row) {
+    uint8* src_ptr = src;
+    uint8* dest_ptr = dest;
+    for (int col = 0; col < num_cols; ++col) {
+      *dest_ptr = *src_ptr++;
+      dest_ptr += dest_col_step;
+    }
+    src += src_stride;
+    dest += dest_row_step;
+  }
+}
+
+int GetIntField(JNIEnv* env,
+                const JavaRef<jclass>& clazz,
+                const JavaRef<jobject>& instance,
+                const char* field_name) {
+  jfieldID field = GetFieldID(env, clazz, field_name, "I");
+  jint int_value = env->GetIntField(instance.obj(), field);
+  return int_value;
+}
+
+}  // namespace
+
+namespace media {
+
+// static
+void VideoCaptureDevice::GetDeviceNames(Names* device_names) {
+  device_names->clear();
+
+  JNIEnv* env = AttachCurrentThread();
+  CHECK(env);

[Ami GONE FROM CHROMIUM, 2013/01/28 23:55:47]

ACT() already DCHECKs.  Why CHECK here?

[wjia(left Chromium), 2013/01/30 18:25:47]

I think caller wouldn't assume any implementation details of called functions.
In release mode, AttachCurrentThread can still return a NULL env since it has
DCHECK, not CHECk.

+
+  std::string camera_string("android/hardware/Camera");

[Yaron, 2013/01/28 23:58:51]

Can you use the jni generator to generate bindings first. You can see the
media_player_jni_headers target in media/media.gyp

[wjia(left Chromium), 2013/01/30 18:25:47]

Did it for Camera. But jar_file_jni_generator.gypi doesn't support system inner
classes. Add a TODO for Camera$CameraInfo.

+  std::string camera_info_string = camera_string + "$CameraInfo";
+
+  ScopedJavaLocalRef<jclass> camera_class(GetClass(env, camera_string.c_str()));
+
+  jmethodID method_get_number_of_cameras = MethodID::Get<MethodID::TYPE_STATIC>(
+      env, camera_class.obj(), "getNumberOfCameras", "()I");
+
+  int num_cameras = env->CallStaticIntMethod(camera_class.obj(),
+                                             method_get_number_of_cameras);
+  DVLOG(1) << "VideoCaptureDevice::GetDeviceNames: num_cameras=" << num_cameras;
+  if (num_cameras <= 0)
+    return;
+
+  // Signature of getCameraInfo: "(ILandroid/hardware/Camera$CameraInfo;)V"

[Ami GONE FROM CHROMIUM, 2013/01/28 23:55:47]

Is this adding value?

[wjia(left Chromium), 2013/01/30 18:25:47]

Removed.

+  std::string signature = "(IL" + camera_info_string + ";)V";
+  jmethodID method_get_camera_info = MethodID::Get<MethodID::TYPE_STATIC>(
+      env, camera_class.obj(), "getCameraInfo", signature.c_str());
+
+  ScopedJavaLocalRef<jclass> camera_info_class(
+      GetClass(env, camera_info_string.c_str()));
+  jmethodID constructor = MethodID::Get<MethodID::TYPE_INSTANCE>(env,
+      camera_info_class.obj(), "<init>", "()V");
+
+  ScopedJavaLocalRef<jobject> object_camera_info(env,
+      env->NewObject(camera_info_class.obj(), constructor));
+
+  jfieldID field_facing = GetFieldID(env, camera_info_class, "facing", "I");
+  jfieldID field_facing_front = GetStaticFieldID(env,
+      camera_info_class, "CAMERA_FACING_FRONT", "I");
+
+  for (int camera_id = num_cameras - 1; camera_id >= 0; --camera_id) {
+    env->CallStaticVoidMethod(camera_class.obj(),
+                              method_get_camera_info,
+                              camera_id,
+                              object_camera_info.obj());
+    CheckException(env);
+
+    Name name;
+    name.unique_id = StringPrintf("%d", camera_id);
+    std::string facing_string;
+    if (env->GetIntField(object_camera_info.obj(), field_facing) ==
+        env->GetStaticIntField(camera_info_class.obj(), field_facing_front)) {
+      facing_string = "front";
+    } else {
+      facing_string = "back";
+    }
+    name.device_name = StringPrintf("camera %d, facing %s",
+                                    camera_id, facing_string.c_str());
+    device_names->push_back(name);
+    jfieldID field_orientation = GetFieldID(env, camera_info_class,
+                                            "orientation", "I");
+    jint orientation = env->GetIntField(object_camera_info.obj(),
+                                        field_orientation);
+    DVLOG(1) << "VideoCaptureDevice::GetDeviceNames: camera device_name="
+             << name.device_name.c_str()
+             << ", unique_id="
+             << name.unique_id.c_str()

[Ami GONE FROM CHROMIUM, 2013/01/28 23:55:47]

.c_str() unnecessary here and above

[wjia(left Chromium), 2013/01/30 18:25:47]

Done.

+             << ", orientation "
+             << orientation;
+  }
+}
+
+// static
+VideoCaptureDevice* VideoCaptureDevice::Create(const Name& device_name) {
+  VideoCaptureDeviceAndroid* self = new VideoCaptureDeviceAndroid(device_name);
+  if (!self || !self->Init())

[Ami GONE FROM CHROMIUM, 2013/01/28 23:55:47]

!self is unnecessary

[wjia(left Chromium), 2013/01/30 18:25:47]

Done.

+    return NULL;

[Ami GONE FROM CHROMIUM, 2013/01/28 23:55:47]

Init() failure leaks here.  A better idiom is:
scoped_ptr<VideoCaptureDeviceAndroid> ret(new
VideoCaptureDeviceAndroid(device_name));
if (ret->Init())
  return ret.release();
return NULL;

[wjia(left Chromium), 2013/01/30 18:25:47]

Done.

+
+  return self;
+}
+
+// static
+bool VideoCaptureDeviceAndroid::RegisterVideoCaptureDevice(JNIEnv* env) {
+  bool ret = RegisterNativesImpl(env);
+  DCHECK(g_VideoCapture_clazz);

[Yaron, 2013/01/28 23:58:51]

No need to DCHECK. just do: return Register..

[wjia(left Chromium), 2013/01/30 18:25:47]

Done.

+  return ret;
+}
+
+VideoCaptureDeviceAndroid::VideoCaptureDeviceAndroid(const Name& device_name)
+    : state_(kIdle),
+      observer_(NULL),
+      device_name_(device_name),
+      rotation_(0) {
+}
+
+VideoCaptureDeviceAndroid::~VideoCaptureDeviceAndroid() {
+  DeAllocate();
+}
+
+bool VideoCaptureDeviceAndroid::Init() {
+  int id;
+  if (device_name_.device_name.find("camera") == std::string::npos)

[Ami GONE FROM CHROMIUM, 2013/01/28 23:55:47]

why do you care about this?

[wjia(left Chromium), 2013/01/30 18:25:47]

Removed.

+    return false;
+
+  base::StringToInt(device_name_.unique_id, &id);

[Ami GONE FROM CHROMIUM, 2013/01/28 23:55:47]

you should always test the return code of string-parsing routines like this.

[wjia(left Chromium), 2013/01/30 18:25:47]

Done.

+
+  JNIEnv* env = AttachCurrentThread();
+  if (!env)
+    return false;
+
+  j_capture_.Reset(Java_VideoCapture_createVideoCapture(env,
+      base::android::GetApplicationContext(), id,
+      reinterpret_cast<jlong>(this)));
+
+  return true;
+}
+
+const VideoCaptureDevice::Name& VideoCaptureDeviceAndroid::device_name() {
+  return device_name_;
+}
+
+void VideoCaptureDeviceAndroid::Allocate(int width, int height, int frame_rate,
+    EventHandler* observer) {
+  {
+    base::AutoLock lock(lock_);

[Ami GONE FROM CHROMIUM, 2013/01/28 23:55:47]

IWBN for the decl of lock_ in the .h to be accompanied by some comments
explaining what needs to be locked and why.

[wjia(left Chromium), 2013/01/30 18:25:47]

Done.

+    if (state_ != kIdle)
+      return;
+    observer_ = observer;
+    state_ = kAllocated;
+  }
+
+  JNIEnv* env = AttachCurrentThread();
+  CHECK(env);
+
+  jint ret = Java_VideoCapture_allocate(env,
+                                        j_capture_.obj(),
+                                        width,
+                                        height,
+                                        frame_rate);
+  if (ret < 0) {
+    SetErrorState("failed to allocate");
+    return;
+  }
+
+  // Store current width and height.
+  current_settings_.color = VideoCaptureCapability::kYV12;

[Ami GONE FROM CHROMIUM, 2013/01/28 23:55:47]

please initialize struct fields in declaration order to make it easier to verify
all are present.

[wjia(left Chromium), 2013/01/30 18:25:47]

Done.

+  current_settings_.width =
+      Java_VideoCapture_queryWidth(env, j_capture_.obj());
+  current_settings_.height =
+      Java_VideoCapture_queryHeight(env, j_capture_.obj());
+  current_settings_.frame_rate =
+      Java_VideoCapture_queryFrameRate(env, j_capture_.obj());
+  DCHECK(current_settings_.width > 0 && !(current_settings_.width % 2));
+  DCHECK(current_settings_.height > 0 && !(current_settings_.height % 2));

[Ami GONE FROM CHROMIUM, 2013/01/28 23:55:47]

Why insist on even dimensions (but not powers-of-two)?

[Ami GONE FROM CHROMIUM, 2013/01/28 23:55:47]

What about the other fields of VCC?  Shouldn't they be sentinel- or
zero-initialized?

[wjia(left Chromium), 2013/01/30 18:25:47]

Even number would ensure buffer size calculation is correct. The typical
dimensions are 640x480, etc.

[wjia(left Chromium), 2013/01/30 18:25:47]

zero-init'ed current_settings_ in constructor.

+
+  DVLOG(1) << "VideoCaptureDeviceAndroid::Allocate: queried width="
+           << current_settings_.width
+           << ", height="
+           << current_settings_.height
+           << ", frame_rate="
+           << current_settings_.frame_rate;
+  // Report the frame size to the observer.
+  observer_->OnFrameInfo(current_settings_);
+
+  int y_size = current_settings_.width * current_settings_.height;

[Ami GONE FROM CHROMIUM, 2013/01/28 23:55:47]

Again, what about stride?  See, for example, the equations at 
http://developer.android.com/reference/android/hardware/Camera.Parameters.htm...

[wjia(left Chromium), 2013/01/30 18:25:47]

Yes, I am aware of this. Need to do a refactoring for VCC to support stride.

+  rotation_buffer_.reset(new uint8[y_size * 3 / 2]);
+  ResetBuffer(rotation_buffer_.get(), current_settings_.width,
+      current_settings_.height);
+}
+
+void VideoCaptureDeviceAndroid::Start() {
+  DVLOG(1) << "VideoCaptureDeviceAndroid::Start";
+  {
+    base::AutoLock lock(lock_);
+    if (state_ != kAllocated)
+      return;

[Ami GONE FROM CHROMIUM, 2013/01/28 23:55:47]

why isn't this a DCHECK/SetErrorState()?

[wjia(left Chromium), 2013/01/30 18:25:47]

changed to DCHECK.

+  }
+
+  JNIEnv* env = AttachCurrentThread();
+  CHECK(env);
+
+  jint ret = Java_VideoCapture_startCapture(env, j_capture_.obj());
+  if (ret < 0) {
+    SetErrorState("failed to start capture");
+    return;
+  }
+
+  {
+    base::AutoLock lock(lock_);
+    state_ = kCapturing;

[Ami GONE FROM CHROMIUM, 2013/01/28 23:55:47]

having two critical sections means two threads can arrive at Start() and each
call the java method.
But VCD's commentary says the public API is called only from a single thread. 
So actually I think the locks in this method are unnecessary.  I hope the
commentary requested above for lock_ will clarify things.

[wjia(left Chromium), 2013/01/30 18:25:47]

Comments for |lock_| have been added. |state_| can be accessed from a different
thread (JAVA).

+  }
+}
+
+void VideoCaptureDeviceAndroid::Stop() {
+  DVLOG(1) << "VideoCaptureDeviceAndroid::Stop";
+  {
+    base::AutoLock lock(lock_);
+    if (state_ == kIdle || state_ == kAllocated)
+      return;
+    state_ = kAllocated;

[Ami GONE FROM CHROMIUM, 2013/01/28 23:55:47]

This makes Stop() silently clear an error state, which seems like a bug (what if
Allocate failed?)

[wjia(left Chromium), 2013/01/30 18:25:47]

Done.

+  }
+
+  JNIEnv* env = AttachCurrentThread();
+  CHECK(env);
+
+  jint ret = Java_VideoCapture_stopCapture(env, j_capture_.obj());
+  if (ret < 0) {
+    SetErrorState("failed to stop capture");
+    return;
+  }
+}
+
+void VideoCaptureDeviceAndroid::DeAllocate() {
+  DVLOG(1) << "VideoCaptureDeviceAndroid::DeAllocate";
+  {
+    base::AutoLock lock(lock_);
+    if (state_ == kIdle)
+      return;
+
+    if (state_ == kCapturing) {
+      base::AutoUnlock unlock(lock_);

[Ami GONE FROM CHROMIUM, 2013/01/28 23:55:47]

There is a race here if two threads can call DeAllocate() (same as l.316
comment).

[wjia(left Chromium), 2013/01/30 18:25:47]

DeAllocate() is called from only one thread.

+      Stop();
+    }
+
+    if (state_ == kAllocated)

[Ami GONE FROM CHROMIUM, 2013/01/28 23:55:47]

How can this fail to be true, other than the Error-clearing behavior of Stop()
above which I think is a bug?

[Ami GONE FROM CHROMIUM, 2013/01/28 23:55:47]

Generally I think most of your == tests should be changed to != for early
returns.

[wjia(left Chromium), 2013/01/30 18:25:47]

In Stop(), error state is kept. Error could also happen during
Java_VideoCapture_stopCapture() call.

[wjia(left Chromium), 2013/01/30 18:25:47]

Done.

+      state_ = kIdle;
+
+    observer_ = NULL;
+  }
+
+  JNIEnv* env = AttachCurrentThread();
+  CHECK(env);
+
+  Java_VideoCapture_deallocate(env, j_capture_.obj());
+}
+
+void VideoCaptureDeviceAndroid::OnFrameAvailable(JNIEnv* env, jobject obj,
+    jbyteArray data, jint length,
+    jint rotation, jboolean flip_vert, jboolean flip_horiz) {
+  DVLOG(3) << "VideoCaptureDeviceAndroid::OnFrameAvailable: length =" << length;
+
+  base::AutoLock lock(lock_);
+  if (state_ != kCapturing || !observer_)
+    return;
+
+  jbyte* buffer = env->GetByteArrayElements(data, NULL);
+  if (!buffer) {
+    LOG(ERROR) << "VideoCaptureDeviceAndroid::OnFrameAvailable: "
+                  "failed to GetByteArrayElements";
+    return;
+  }
+
+  // TODO(wjia): move rotation into VideoCaptureController to remove
+  // one buffer copying.
+  // Rotate the buffer when needed.
+  int width = current_settings_.width;
+  int height = current_settings_.height;
+  if (rotation_ != rotation) {
+    rotation_ = rotation;
+    ResetBuffer(rotation_buffer_.get(), width, height);
+  }
+
+  uint8* src = reinterpret_cast<uint8*>(buffer);
+  uint8* dest = rotation_buffer_.get();
+
+  RotatePlaneByPixels(src, dest, width, height, rotation, flip_vert,
+                      flip_horiz);
+  int y_size = width * height;
+  src += y_size;
+  dest += y_size;
+  RotatePlaneByPixels(src, dest, width/2, height/2, rotation, flip_vert,
+                      flip_horiz);
+  src += y_size/4;
+  dest += y_size/4;
+  RotatePlaneByPixels(src, dest, width/2, height/2, rotation, flip_vert,
+                      flip_horiz);
+  src = rotation_buffer_.get();

[Ami GONE FROM CHROMIUM, 2013/01/28 23:55:47]

I found this confusing; why not just use rotation_buffer_.get() in the next
line, or else s/src/dest/ in this line and the next?

[wjia(left Chromium), 2013/01/30 18:25:47]

I was kind of looking ahead. Since this rotation code will be moved into
VideoCaptureController later, the call to OnIncomingCapturedFrame will be using
src as parameter. Anyhow, change it to use rotation_buffer_.get() directly for
now.

+  observer_->OnIncomingCapturedFrame(src, length, base::Time::Now());
+
+  env->ReleaseByteArrayElements(data, buffer, JNI_ABORT);
+}
+
+void VideoCaptureDeviceAndroid::SetErrorState(const std::string& reason) {
+  LOG(ERROR) << "VideoCaptureDeviceAndroid::SetErrorState: " << reason;
+  state_ = kError;

[Ami GONE FROM CHROMIUM, 2013/01/28 23:55:47]

This is racy; probably you want to rename SES to SetErrorState_Locked and ensure
lock_ is always held when this is called.

[wjia(left Chromium), 2013/01/30 18:25:47]

Oops, missed this one. Accessing |state_| should acquire lock first. This
function is called without lock acquired.

+  observer_->OnError();
+}
+
+}  // namespace media