Move NV21 colorspace treatment from VideoCapture java to C++

content/browser/renderer_host/media/video_capture_controller.cc

 // Copyright (c) 2012 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 "content/browser/renderer_host/media/video_capture_controller.h"
 
 #include <set>
 
 #include "base/bind.h"
 #include "base/debug/trace_event.h"
@@ skipping 242 matching lines @@
   if (!buffer_pool_.get())
     return NULL;
   return buffer_pool_->ReserveI420VideoFrame(gfx::Size(frame_info_.width,
                                                        frame_info_.height),
                                              0);
 }
 
 // Implements VideoCaptureDevice::EventHandler.
 // OnIncomingCapturedFrame is called the thread running the capture device.
 // I.e.- DirectShow thread on windows and v4l2_thread on Linux.
 #if !defined(OS_IOS) && !defined(OS_ANDROID)

[Ami GONE FROM CHROMIUM, 2013/09/12 17:03:16]

Why not use libyuv on clank?
I suspect you propagated the meme that "no libyuv on android" introduced in
https://codereview.chromium.org/11366084.  But that was for the webview build,
not clank (ironically, before clank had webrtc support).  Further, the OS_IOS
check is even more strange because bling doesn't even ship webrtc.
I suspect there are performance (and probably correctness) wins to be had by
using libyuv everywhere this function might be called.

How would you feel about replacing this line with an
#ifndef AVOID_LIBYUV_FOR_ANDROID_WEBVIEW
...current libyuv-using-impl
#else
...alternative, non-libyuv-using-impl
#endif
and add to the gyp target building this file a condition like
['OS=="android" and android_webview_build==1', {
  'defines': ['AVOID_LIBYUV_FOR_ANDROID_WEBVIEW'],
}]
?

[scherkus (not reviewing), 2013/09/12 17:26:49]

+1

it's also unfortunate to introduce i420_intermediate_buffer_ even though its
goes unused when libyuv is being used

if we go the #define route we should guard use of that variable

[mcasas, 2013/09/13 08:42:51]

It all sounds good and I had a secret post-it to check the use of libyuv in
Android. I asked other chromiumers and found indeed not concluding reason why it
shouldn't be used, being more performant than the current implementation. I
suggest doing the change in another CL and crbug, this one already has a couple
of things going on (jni bits etc).

FTR: Libyuv works out of the box :) but rotations are not correctly letterboxed.

[mcasas, 2013/09/16 08:43:02]

FWIW, http://crbug.com/292400 tracks the introduction of libyuv in android.

 void VideoCaptureController::OnIncomingCapturedFrame(
     const uint8* data,
     int length,
     base::Time timestamp,
     int rotation,
     bool flip_vert,
     bool flip_horiz) {
   TRACE_EVENT0("video", "VideoCaptureController::OnIncomingCapturedFrame");
 
   scoped_refptr<media::VideoFrame> dst;
@@ skipping 119 matching lines @@
 #else
 void VideoCaptureController::OnIncomingCapturedFrame(
     const uint8* data,
     int length,
     base::Time timestamp,
     int rotation,
     bool flip_vert,
     bool flip_horiz) {
   DCHECK(frame_info_.color == media::PIXEL_FORMAT_I420 ||
          frame_info_.color == media::PIXEL_FORMAT_YV12 ||
+         frame_info_.color == media::PIXEL_FORMAT_NV21 ||
          (rotation == 0 && !flip_vert && !flip_horiz));
 
   TRACE_EVENT0("video", "VideoCaptureController::OnIncomingCapturedFrame");
 
   scoped_refptr<media::VideoFrame> dst;
   {
     base::AutoLock lock(buffer_pool_lock_);
     if (!buffer_pool_.get())
       return;
     dst = buffer_pool_->ReserveI420VideoFrame(gfx::Size(frame_info_.width,
@@ skipping 17 matching lines @@
       RotatePackedYV12Frame(
           data, yplane, uplane, vplane, frame_info_.width, frame_info_.height,
           rotation, flip_vert, flip_horiz);
       break;
     case media::PIXEL_FORMAT_YV12:
       DCHECK(!chopped_width_ && !chopped_height_);
       RotatePackedYV12Frame(
           data, yplane, vplane, uplane, frame_info_.width, frame_info_.height,
           rotation, flip_vert, flip_horiz);
       break;
-    case media::PIXEL_FORMAT_NV21:
+    case media::PIXEL_FORMAT_NV21: {
       DCHECK(!chopped_width_ && !chopped_height_);
-      media::ConvertNV21ToYUV(data, yplane, uplane, vplane, frame_info_.width,
+      int num_pixels = frame_info_.width * frame_info_.height;
+      media::ConvertNV21ToYUV(data,
+                              &i420_intermediate_buffer_[0],
+                              &i420_intermediate_buffer_[num_pixels],
+                              &i420_intermediate_buffer_[num_pixels * 5 / 4],
+                              frame_info_.width,
                               frame_info_.height);
+      RotatePackedYV12Frame(
+          &i420_intermediate_buffer_[0], yplane, uplane, vplane,
+          frame_info_.width, frame_info_.height,
+          rotation, flip_vert, flip_horiz);
       break;
+    }
     case media::PIXEL_FORMAT_YUY2:
       DCHECK(!chopped_width_ && !chopped_height_);
       if (frame_info_.width * frame_info_.height * 2 != length) {
         // If |length| of |data| does not match the expected width and height
         // we can't convert the frame to I420. YUY2 is 2 bytes per pixel.
         break;
       }
 
       media::ConvertYUY2ToYUV(data, yplane, uplane, vplane, frame_info_.width,
                               frame_info_.height);
@@ skipping 158 matching lines @@
     chopped_width_ = 1;
   } else {
     chopped_width_ = 0;
   }
   if (info.height & 1) {
     --frame_info_.height;
     chopped_height_ = 1;
   } else {
     chopped_height_ = 0;
   }
+  if ((frame_info_.color == media::PIXEL_FORMAT_NV21) &&
+      (i420_intermediate_buffer_.get() == NULL)) {
+    i420_intermediate_buffer_.reset(

[scherkus (not reviewing), 2013/09/12 17:26:49]

does this mean we're allocating memory for i420_intermediate_buffer_ but never
using it when libyuv is being used?

[mcasas, 2013/09/13 08:42:51]

I should guard it with the #ifdef - sigh.

[mcasas, 2013/09/16 08:43:02]

Done.

+        new uint8[frame_info_.width * frame_info_.height * 12 / 8]);
+  }
   BrowserThread::PostTask(BrowserThread::IO,
       FROM_HERE,
       base::Bind(&VideoCaptureController::DoFrameInfoOnIOThread, this));
 }
 
 void VideoCaptureController::OnFrameInfoChanged(
     const media::VideoCaptureCapability& info) {
   BrowserThread::PostTask(BrowserThread::IO,
       FROM_HERE,
       base::Bind(&VideoCaptureController::DoFrameInfoChangedOnIOThread,
@@ skipping 210 matching lines @@
     controller_clients_.push_back((*client_it));
     pending_clients_.erase(client_it++);
   }
   // Request the manager to start the actual capture.
   video_capture_manager_->Start(current_params_, this);
   state_ = VIDEO_CAPTURE_STATE_STARTED;
   device_in_use_ = true;
 }
 
 }  // namespace content