Hold video frame in Bitmap instead of keeping a ByteBuffer reference.

Sometimes, the DesktopView was repainted whilst |JniInterface.sBuffer|
no longer referred to valid frame data. This CL cleans things up by
replacing the ByteBuffer with a Bitmap, and having JniFrameConsumer
copy the completely-decoded data directly into the Java Bitmap.

Review URL: https://codereview.chromium.org/24072012

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@228898 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 156 insertions, 118 deletions

diff --git a/remoting/android/java/src/org/chromium/chromoting/DesktopView.java b/remoting/android/java/src/org/chromium/chromoting/DesktopView.java
index 2a4a994..8209d28 100644
--- a/remoting/android/java/src/org/chromium/chromoting/DesktopView.java
+++ b/remoting/android/java/src/org/chromium/chromoting/DesktopView.java
@@ -128,7 +128,7 @@ public class DesktopView extends SurfaceView implements Runnable, SurfaceHolder.
             Log.w("deskview", "Canvas being redrawn on UI thread");
         }
 
-        Bitmap image = JniInterface.retrieveVideoFrame();
+        Bitmap image = JniInterface.getVideoFrame();
         Canvas canvas = getHolder().lockCanvas();
         synchronized (mTransform) {
             canvas.setMatrix(mTransform);
diff --git a/remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java b/remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java
index 9816970..fb9110d 100644
--- a/remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java
+++ b/remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java
@@ -115,6 +115,11 @@ public class JniInterface {
         disconnectNative();
         sSuccessCallback = null;
         sConnected = false;
+
+        // Drop the reference to free the Bitmap for GC.
+        synchronized (sFrameLock) {
+            sFrameBitmap = null;
+        }
     }
 
     /** Performs the native portion of the connection. */
@@ -136,17 +141,11 @@ public class JniInterface {
     /** Callback to signal whenever we need to redraw. */
     private static Runnable sRedrawCallback = null;
 
-    /** Screen width of the video feed. */
-    private static int sWidth = 0;
-
-    /** Screen height of the video feed. */
-    private static int sHeight = 0;
-
-    /** Bitmap holding the latest screen image. */
-    private static Bitmap sBitmap = null;
+    /** Bitmap holding a copy of the latest video frame. */
+    private static Bitmap sFrameBitmap = null;
 
-    /** Buffer holding the video feed. */
-    private static ByteBuffer sBuffer = null;
+    /** Lock to protect the frame bitmap reference. */
+    private static final Object sFrameLock = new Object();
 
     /** Position of cursor hot-spot. */
     private static Point sCursorHotspot = new Point();
@@ -301,10 +300,10 @@ public class JniInterface {
     }
 
     /**
-     * Obtains the image buffer.
-     * This should not be called from the UI thread. (We prefer the native graphics thread.)
+     * Returns a bitmap of the latest video frame. Called on the native graphics thread when
+     * DesktopView is repainted.
      */
-    public static Bitmap retrieveVideoFrame() {
+    public static Bitmap getVideoFrame() {
         if (Looper.myLooper() == Looper.getMainLooper()) {
             Log.w("jniiface", "Canvas being redrawn on UI thread");
         }
@@ -313,19 +312,31 @@ public class JniInterface {
             return null;
         }
 
-        // This is synchronized only to silence a findbugs warning about incorrect initialization of
-        // |sBitmap|.
-        // TODO(lambroslambrou): Annotate this class as @NotThreadSafe to prevent similar warnings
-        // in future.
-        synchronized (JniInterface.class) {
-            if (sBitmap == null || sBitmap.getWidth() != sWidth || sBitmap.getHeight() != sHeight) {
-                sBitmap = Bitmap.createBitmap(sWidth, sHeight, Bitmap.Config.ARGB_8888);
-            }
+        synchronized (sFrameLock) {
+            return sFrameBitmap;
+        }
+    }
+
+    /**
+     * Sets a new video frame. Called from native code on the native graphics thread when a new
+     * frame is allocated.
+     */
+    private static void setVideoFrame(Bitmap bitmap) {
+        if (Looper.myLooper() == Looper.getMainLooper()) {
+            Log.w("jniiface", "Video frame updated on UI thread");
+        }
+
+        synchronized (sFrameLock) {
+            sFrameBitmap = bitmap;
         }
+    }
 
-        sBuffer.rewind();
-        sBitmap.copyPixelsFromBuffer(sBuffer);
-        return sBitmap;
+    /**
+     * Creates a new Bitmap to hold video frame pixels. Called by native code which stores a global
+     * reference to the Bitmap and writes the decoded frame pixels to it.
+     */
+    private static Bitmap newBitmap(int width, int height) {
+        return Bitmap.createBitmap(width, height, Bitmap.Config.ARGB_8888);
     }
 
     /**
diff --git a/remoting/client/jni/DEPS b/remoting/client/jni/DEPS
new file mode 100644
index 0000000..26dcd15
--- /dev/null
+++ b/remoting/client/jni/DEPS
@@ -0,0 +1,3 @@
+include_rules = [
+  "+ui/gfx/android",
+]
diff --git a/remoting/client/jni/chromoting_jni_runtime.cc b/remoting/client/jni/chromoting_jni_runtime.cc
index 108bb71..11ca730 100644
--- a/remoting/client/jni/chromoting_jni_runtime.cc
+++ b/remoting/client/jni/chromoting_jni_runtime.cc
@@ -13,6 +13,7 @@
 #include "media/base/yuv_convert.h"
 #include "net/android/net_jni_registrar.h"
 #include "remoting/base/url_request_context.h"
+#include "third_party/webrtc/modules/desktop_capture/desktop_frame.h"
 
 namespace {
 
@@ -169,24 +170,33 @@ void ChromotingJniRuntime::CommitPairingCredentials(const std::string& host,
   // GCd as soon as the managed method returned, so we mustn't release it here.
 }
 
-void ChromotingJniRuntime::UpdateImageBuffer(int width,
-                                             int height,
-                                             jobject buffer) {
+base::android::ScopedJavaLocalRef<jobject> ChromotingJniRuntime::NewBitmap(
+    webrtc::DesktopSize size) {
+  JNIEnv* env = base::android::AttachCurrentThread();
+
+  jobject bitmap = env->CallStaticObjectMethod(
+      class_,
+      env->GetStaticMethodID(
+          class_,
+          "newBitmap",
+          "(II)Landroid/graphics/Bitmap;"),
+      size.width(),
+      size.height());
+  return base::android::ScopedJavaLocalRef<jobject>(env, bitmap);
+}
+
+void ChromotingJniRuntime::UpdateFrameBitmap(jobject bitmap) {
   DCHECK(display_task_runner_->BelongsToCurrentThread());
 
   JNIEnv* env = base::android::AttachCurrentThread();
-  env->SetStaticIntField(
-      class_,
-      env->GetStaticFieldID(class_, "sWidth", "I"),
-      width);
-  env->SetStaticIntField(
-      class_,
-      env->GetStaticFieldID(class_, "sHeight", "I"),
-      height);
-  env->SetStaticObjectField(
+
+  env->CallStaticVoidMethod(
       class_,
-      env->GetStaticFieldID(class_, "sBuffer", "Ljava/nio/ByteBuffer;"),
-      buffer);
+      env->GetStaticMethodID(
+          class_,
+          "setVideoFrame",
+          "(Landroid/graphics/Bitmap;)V"),
+      bitmap);
 }
 
 void ChromotingJniRuntime::UpdateCursorShape(
diff --git a/remoting/client/jni/chromoting_jni_runtime.h b/remoting/client/jni/chromoting_jni_runtime.h
index bfe8549..93d77bd 100644
--- a/remoting/client/jni/chromoting_jni_runtime.h
+++ b/remoting/client/jni/chromoting_jni_runtime.h
@@ -8,6 +8,7 @@
 #include <jni.h>
 #include <string>
 
+#include "base/android/scoped_java_ref.h"
 #include "base/at_exit.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "remoting/base/auto_thread.h"
@@ -79,8 +80,13 @@ class ChromotingJniRuntime {
                                 const std::string& id,
                                 const std::string& secret);
 
-  // Updates image dimensions and canvas memory space. Call on display thread.
-  void UpdateImageBuffer(int width, int height, jobject buffer);
+  // Creates a new Bitmap object to store a video frame.
+  base::android::ScopedJavaLocalRef<jobject> NewBitmap(
+      webrtc::DesktopSize size);
+
+  // Updates video frame bitmap. |bitmap| must be an instance of
+  // android.graphics.Bitmap. Call on the display thread.
+  void UpdateFrameBitmap(jobject bitmap);
 
   // Updates cursor shape. Call on display thread.
   void UpdateCursorShape(const protocol::CursorShapeInfo& cursor_shape);
diff --git a/remoting/client/jni/jni_frame_consumer.cc b/remoting/client/jni/jni_frame_consumer.cc
index 36a8184..1a560e9 100644
--- a/remoting/client/jni/jni_frame_consumer.cc
+++ b/remoting/client/jni/jni_frame_consumer.cc
@@ -6,57 +6,32 @@
 
 #include "base/android/jni_android.h"
 #include "base/logging.h"
+#include "base/stl_util.h"
 #include "base/synchronization/waitable_event.h"
+#include "remoting/base/util.h"
 #include "remoting/client/frame_producer.h"
 #include "remoting/client/jni/chromoting_jni_runtime.h"
 #include "third_party/webrtc/modules/desktop_capture/desktop_frame.h"
 #include "third_party/webrtc/modules/desktop_capture/desktop_region.h"
-
-namespace {
-
-// Allocates its buffer within a Java direct byte buffer, where it can be
-// accessed by both native and managed code.
-class DirectDesktopFrame : public webrtc::BasicDesktopFrame {
- public:
-  DirectDesktopFrame(int width, int height);
-
-  virtual ~DirectDesktopFrame();
-
-  jobject buffer() const {
-    return buffer_;
-  }
-
- private:
-  jobject buffer_;
-};
-
-DirectDesktopFrame::DirectDesktopFrame(int width, int height)
-    : webrtc::BasicDesktopFrame(webrtc::DesktopSize(width, height)) {
-  JNIEnv* env = base::android::AttachCurrentThread();
-  buffer_ = env->NewDirectByteBuffer(data(), stride()*height);
-}
-
-DirectDesktopFrame::~DirectDesktopFrame() {}
-
-}  // namespace
+#include "ui/gfx/android/java_bitmap.h"
 
 namespace remoting {
 
 JniFrameConsumer::JniFrameConsumer(ChromotingJniRuntime* jni_runtime)
     : jni_runtime_(jni_runtime),
-      in_dtor_(false),
       frame_producer_(NULL) {
 }
 
 JniFrameConsumer::~JniFrameConsumer() {
-  // Stop giving the producer a buffer to work with.
-  in_dtor_ = true;
-
-  // Don't destroy the object until we've deleted the buffer.
+  // The producer should now return any pending buffers. At this point, however,
+  // ReturnBuffer() tasks scheduled by the producer will not be delivered,
+  // so we free all the buffers once the producer's queue is empty.
   base::WaitableEvent done_event(true, false);
   frame_producer_->RequestReturnBuffers(
       base::Bind(&base::WaitableEvent::Signal, base::Unretained(&done_event)));
   done_event.Wait();
+
+  STLDeleteElements(&buffers_);
 }
 
 void JniFrameConsumer::set_frame_producer(FrameProducer* producer) {
@@ -69,29 +44,40 @@ void JniFrameConsumer::ApplyBuffer(const webrtc::DesktopSize& view_size,
                                    const webrtc::DesktopRegion& region) {
   DCHECK(jni_runtime_->display_task_runner()->BelongsToCurrentThread());
 
-  scoped_ptr<webrtc::DesktopFrame> buffer_scoped(buffer);
-  jni_runtime_->RedrawCanvas();
-
-  if (view_size.width() > view_size_.width() ||
-      view_size.height() > view_size_.height()) {
-    LOG(INFO) << "Existing buffer is too small";
-    view_size_ = view_size;
+  if (!view_size_.equals(view_size)) {
+    // Drop the frame, since the data belongs to the previous generation,
+    // before SetSourceSize() called SetOutputSizeAndClip().
+    FreeBuffer(buffer);
+    return;
+  }
 
-    // Manually destroy the old buffer before allocating a new one to prevent
-    // our memory footprint from temporarily ballooning.
-    buffer_scoped.reset();
-    AllocateBuffer();
+  // Copy pixels from |buffer| into the Java Bitmap.
+  // TODO(lambroslambrou): Optimize away this copy by having the VideoDecoder
+  // decode directly into the Bitmap's pixel memory. This currently doesn't
+  // work very well because the VideoDecoder writes the decoded data in BGRA,
+  // and then the R/B channels are swapped in place (on the decoding thread).
+  // If a repaint is triggered from a Java event handler, the unswapped pixels
+  // can sometimes appear on the display.
+  uint8* dest_buffer = static_cast<uint8*>(bitmap_->pixels());
+  webrtc::DesktopRect buffer_rect = webrtc::DesktopRect::MakeSize(view_size);
+
+  for (webrtc::DesktopRegion::Iterator i(region); !i.IsAtEnd(); i.Advance()) {
+    const webrtc::DesktopRect& rect(i.rect());
+    CopyRGB32Rect(buffer->data(), buffer->stride(), buffer_rect, dest_buffer,
+                  bitmap_->stride(), buffer_rect, rect);
   }
 
+  // TODO(lambroslambrou): Optimize this by only repainting the changed pixels.
+  jni_runtime_->RedrawCanvas();
+
   // Supply |frame_producer_| with a buffer to render the next frame into.
-  if (!in_dtor_)
-    frame_producer_->DrawBuffer(buffer_scoped.release());
+  frame_producer_->DrawBuffer(buffer);
 }
 
 void JniFrameConsumer::ReturnBuffer(webrtc::DesktopFrame* buffer) {
   DCHECK(jni_runtime_->display_task_runner()->BelongsToCurrentThread());
   LOG(INFO) << "Returning image buffer";
-  delete buffer;
+  FreeBuffer(buffer);
 }
 
 void JniFrameConsumer::SetSourceSize(const webrtc::DesktopSize& source_size,
@@ -104,9 +90,8 @@ void JniFrameConsumer::SetSourceSize(const webrtc::DesktopSize& source_size,
   clip_area_ = webrtc::DesktopRect::MakeSize(view_size_);
   frame_producer_->SetOutputSizeAndClip(view_size_, clip_area_);
 
-  // Unless being destructed, allocate buffer and start drawing frames onto it.
-  frame_producer_->RequestReturnBuffers(base::Bind(
-      &JniFrameConsumer::AllocateBuffer, base::Unretained(this)));
+  // Allocate buffer and start drawing frames onto it.
+  AllocateBuffer();
 }
 
 FrameConsumer::PixelFormat JniFrameConsumer::GetPixelFormat() {
@@ -114,25 +99,29 @@ FrameConsumer::PixelFormat JniFrameConsumer::GetPixelFormat() {
 }
 
 void JniFrameConsumer::AllocateBuffer() {
-  // Only do anything if we're not being destructed.
-  if (!in_dtor_) {
-    if (!jni_runtime_->display_task_runner()->BelongsToCurrentThread()) {
-      jni_runtime_->display_task_runner()->PostTask(FROM_HERE,
-          base::Bind(&JniFrameConsumer::AllocateBuffer,
-                     base::Unretained(this)));
-      return;
-    }
-
-    DirectDesktopFrame* buffer = new DirectDesktopFrame(view_size_.width(),
-                                                        view_size_.height());
-
-    // Update Java's reference to the buffer and record of its dimensions.
-    jni_runtime_->UpdateImageBuffer(view_size_.width(),
-                                    view_size_.height(),
-                                    buffer->buffer());
-
-    frame_producer_->DrawBuffer(buffer);
-  }
+  DCHECK(jni_runtime_->display_task_runner()->BelongsToCurrentThread());
+
+  webrtc::DesktopSize size(view_size_.width(), view_size_.height());
+
+  // Allocate a new Bitmap, store references here, and pass it to Java.
+  JNIEnv* env = base::android::AttachCurrentThread();
+
+  // |bitmap_| must be deleted before |bitmap_global_ref_| is released.
+  bitmap_.reset();
+  bitmap_global_ref_.Reset(env, jni_runtime_->NewBitmap(size).obj());
+  bitmap_.reset(new gfx::JavaBitmap(bitmap_global_ref_.obj()));
+  jni_runtime_->UpdateFrameBitmap(bitmap_global_ref_.obj());
+
+  webrtc::DesktopFrame* buffer = new webrtc::BasicDesktopFrame(size);
+  buffers_.push_back(buffer);
+  frame_producer_->DrawBuffer(buffer);
+}
+
+void JniFrameConsumer::FreeBuffer(webrtc::DesktopFrame* buffer) {
+  DCHECK(std::find(buffers_.begin(), buffers_.end(), buffer) != buffers_.end());
+
+  buffers_.remove(buffer);
+  delete buffer;
 }
 
 }  // namespace remoting
diff --git a/remoting/client/jni/jni_frame_consumer.h b/remoting/client/jni/jni_frame_consumer.h
index 1509720..47c8632 100644
--- a/remoting/client/jni/jni_frame_consumer.h
+++ b/remoting/client/jni/jni_frame_consumer.h
@@ -5,11 +5,18 @@
 #ifndef REMOTING_CLIENT_JNI_JNI_FRAME_CONSUMER_H_
 #define REMOTING_CLIENT_JNI_JNI_FRAME_CONSUMER_H_
 
-#include "remoting/client/frame_consumer.h"
+#include <list>
 
+#include "base/android/scoped_java_ref.h"
 #include "base/compiler_specific.h"
+#include "base/memory/scoped_ptr.h"
+#include "remoting/client/frame_consumer.h"
 #include "third_party/webrtc/modules/desktop_capture/desktop_geometry.h"
 
+namespace gfx {
+class JavaBitmap;
+}  // namespace gfx
+
 namespace webrtc {
 class DesktopFrame;
 }  // namespace webrtc
@@ -40,22 +47,34 @@ class JniFrameConsumer : public FrameConsumer {
   virtual PixelFormat GetPixelFormat() OVERRIDE;
 
  private:
+  // Allocates a new buffer of |view_size_|, informs Java about it, and tells
+  // the producer to draw onto it.
+  void AllocateBuffer();
+
+  // Frees a frame buffer previously allocated by AllocateBuffer.
+  void FreeBuffer(webrtc::DesktopFrame* buffer);
+
   // Variables are to be used from the display thread.
 
   // Used to obtain task runner references and make calls to Java methods.
   ChromotingJniRuntime* jni_runtime_;
 
-  // Whether we're currently in the constructor, and should deallocate the
-  // buffer instead of passing it back to the producer.
-  bool in_dtor_;
-
   FrameProducer* frame_producer_;
   webrtc::DesktopSize view_size_;
   webrtc::DesktopRect clip_area_;
 
-  // If |provide_buffer_|, allocates a new buffer of |view_size_|, informs
-  // Java about it, and tells the producer to draw onto it. Otherwise, no-op.
-  void AllocateBuffer();
+  // List of allocated image buffers.
+  std::list<webrtc::DesktopFrame*> buffers_;
+
+  // This global reference is required, instead of a local reference, so it
+  // remains valid for the lifetime of |bitmap_| - gfx::JavaBitmap does not
+  // create its own global reference internally. And this global ref must be
+  // destroyed (released) after |bitmap_| is destroyed.
+  base::android::ScopedJavaGlobalRef<jobject> bitmap_global_ref_;
+
+  // Reference to the frame bitmap that is passed to Java when the frame is
+  // allocated. This provides easy access to the underlying pixels.
+  scoped_ptr<gfx::JavaBitmap> bitmap_;
 
   DISALLOW_COPY_AND_ASSIGN(JniFrameConsumer);
 };