VideoCapturerAndroid, handle cvo correctly

talk/app/webrtc/java/jni/native_handle_impl.cc

 /*
  * libjingle
  * Copyright 2015 Google Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
  *
  *  1. Redistributions of source code must retain the above copyright notice,
  *     this list of conditions and the following disclaimer.
  *  2. Redistributions in binary form must reproduce the above copyright notice,
@@ skipping 12 matching lines @@
  * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
  * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
  * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #include "talk/app/webrtc/java/jni/native_handle_impl.h"
 
 #include "webrtc/base/checks.h"
 #include "webrtc/base/keep_ref_until_done.h"
 #include "webrtc/base/scoped_ref_ptr.h"
+#include "webrtc/base/logging.h"
 
 using webrtc::NativeHandleBuffer;
 
+namespace {
+
+const float ROTATE_90[16] =
+    {0.0, 1.0, 0.0, 0.0,
+    -1.0, 0.0, 0.0, 0.0,
+     0.0, 0.0, 1.0, 0.0,
+     1.0, 0.0 , 0.0, 1.0};
+const float ROTATE_180[16] =
+    {-1.0, 0.0, 0.0, 0.0,
+     0.0, -1.0, 0.0, 0.0,
+     0.0, 0.0, 1.0, 0.0,
+     1.0, 1.0, 0.0, 1.0};
+const float ROTATE_270[16] =
+    {0.0, -1.0, 0.0, 0.0,
+     1.0, 0.0, 0.0, 0.0,
+     0.0, 0.0, 1.0, 0.0,
+     0.0, 1.0, 0.0, 1.0};
+
+void InPlaceMultiplyMatrix(float a[16], const float b[16]) {

[nisse-chromium (ooo August 14), 2015/12/08 08:46:34]

Please explain operation. Is it A <-- A * B or A <-- B * A (in math. matrix
notation)? Is it using row-major or column-major order?

[perkj_chrome, 2015/12/09 21:00:40]

not relevant now?

+    float ab[16];
+    int ab_ofs = 0;
+    int b_ofs = 0;
+    for (int i = 0; i < 4; i++) {
+      ab[ab_ofs + 0] =
+          (b[b_ofs + 0] * a[0]) +
+          (b[b_ofs + 1] * a[4]) +
+          (b[b_ofs + 2] * a[8]) +
+          (b[b_ofs + 3] * a[12]) ;
+
+      ab[ab_ofs + 1] =
+          (b[b_ofs + 0] * a[1]) +
+          (b[b_ofs + 1] * a[5]) +
+          (b[b_ofs + 2] * a[9]) +
+          (b[b_ofs + 3] * a[13]) ;
+
+      ab[ab_ofs + 2] =
+          (b[b_ofs + 0] * a[2]) +
+          (b[b_ofs + 1] * a[6]) +
+          (b[b_ofs + 2] * a[10]) +
+          (b[b_ofs + 3] * a[14]) ;
+
+      ab[ab_ofs + 3] =
+          (b[b_ofs + 0] * a[3]) +
+          (b[b_ofs + 1] * a[7]) +
+          (b[b_ofs + 2] * a[11]) +
+          (b[b_ofs + 3] * a[15]) ;
+      ab_ofs += 4;

[nisse-chromium (ooo August 14), 2015/12/08 08:46:34]

Move these updates up to the for(;;...). Maybe rename *_ofs to *_row (if
matrices are row-major).

[perkj_chrome, 2015/12/09 21:00:40]

Acknowledged.

+      b_ofs += 4;
+    }
+
+    memcpy(static_cast<void*>(a), static_cast<const void*>(ab),
+        16*sizeof(float));

[nisse-chromium (ooo August 14), 2015/12/08 08:46:34]

Replace the size expression with sizeof(ab). (Unfortunately, sizeof(a) won't
work, since a really is a pointer; the argument array syntax is only sugar).

Regarding the whole function, I see that we only touch one half of the matrix,
so maybe something simpler and more explicit would actually be shorter. Like, an
outer switch, and then in each case, loop over the columns of a, read out the
two affected values, write back rotated values in place.

[perkj_chrome, 2015/12/09 21:00:40]

Done.

+}
+
+}  // anonymouse namespace
+
 namespace webrtc_jni {
 
 NativeHandleImpl::NativeHandleImpl(JNIEnv* jni,
                                    jint j_oes_texture_id,
                                    jfloatArray j_transform_matrix)
     : oes_texture_id(j_oes_texture_id) {
   RTC_CHECK_EQ(16, jni->GetArrayLength(j_transform_matrix));
   jfloat* transform_matrix_ptr =
       jni->GetFloatArrayElements(j_transform_matrix, nullptr);
   for (int i = 0; i < 16; ++i) {
@@ skipping 15 matching lines @@
   no_longer_used_cb_();
 }
 
 rtc::scoped_refptr<webrtc::VideoFrameBuffer>
 AndroidTextureBuffer::NativeToI420Buffer() {
   RTC_NOTREACHED()
       << "AndroidTextureBuffer::NativeToI420Buffer not implemented.";
   return nullptr;
 }
 
-rtc::scoped_refptr<AndroidTextureBuffer> AndroidTextureBuffer::CropAndScale(
-    int cropped_input_width,
-    int cropped_input_height,
-    int dst_widht,
-    int dst_height) {
+rtc::scoped_refptr<AndroidTextureBuffer>
+AndroidTextureBuffer::CropScaleAndRotate(int cropped_input_width,

[nisse-chromium (ooo August 14), 2015/12/08 08:46:34]

Maybe we could come up with a better name. TransformFrame or something?

Maybe drop the crop-related arguments, until that is actually implemented. It's
not clear to me how they're intended to be used, and for cropping, won't you
also need some offsets for positioning of the rectangle you cut out?

[perkj_chrome, 2015/12/09 21:00:40]

If you don't object too much I will call this ScaleAndRotate in that case.

[nisse-chromium (ooo August 14), 2015/12/10 09:04:57]

Good enough.

+                                         int cropped_input_height,
+                                         int dst_widht,
+                                         int dst_height,
+                                         webrtc::VideoRotation rotation) {
   // TODO(perkj) Implement cropping.
   RTC_CHECK_EQ(cropped_input_width, width_);
   RTC_CHECK_EQ(cropped_input_height, height_);
+  if (width() == dst_widht && height() == dst_height &&
+      rotation == webrtc::kVideoRotation_0) {
+    return this;
+  }
 
-  // Here we use Bind magic to add a reference count to |this| until the newly
-  // created AndroidTextureBuffer is destructed. ScaledFrameNotInUse will be
-  // called that happens and when it finishes, the reference count to |this|

[nisse-chromium (ooo August 14), 2015/12/08 08:46:34]

Keep this comment, if it is still relevant?

[perkj_chrome, 2015/12/09 21:00:40]

Done.

-  // will be decreased by one.
-  return new rtc::RefCountedObject<AndroidTextureBuffer>(
-      dst_widht, dst_height, native_handle_,
-      rtc::KeepRefUntilDone(this));
+  int rotated_width = (rotation % 180 == 0) ? dst_widht : dst_height;
+  int rotated_height = (rotation % 180 == 0) ? dst_height : dst_widht;
+
+  rtc::scoped_refptr<AndroidTextureBuffer> buffer(
+      new rtc::RefCountedObject<AndroidTextureBuffer>(
+          rotated_width, rotated_height, native_handle_,
+          rtc::KeepRefUntilDone(this)));
+
+  switch (rotation) {
+  case webrtc::kVideoRotation_0:
+    break;
+  case webrtc::kVideoRotation_90:
+    InPlaceMultiplyMatrix(buffer->native_handle_.sampling_matrix, ROTATE_90);
+    break;
+  case webrtc::kVideoRotation_180:
+    InPlaceMultiplyMatrix(buffer->native_handle_.sampling_matrix,
+        ROTATE_180);
+    break;
+  case webrtc::kVideoRotation_270:
+    InPlaceMultiplyMatrix(buffer->native_handle_.sampling_matrix,
+        ROTATE_270);
+    break;
+  }
+
+  return buffer;
 }
 
 }  // namespace webrtc_jni