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

remoting/client/jni/jni_frame_consumer.cc

Index: remoting/client/jni/jni_frame_consumer.cc
diff --git a/remoting/client/jni/jni_frame_consumer.cc b/remoting/client/jni/jni_frame_consumer.cc
index 36a81841771a0f9d9f4c537b3e653425ecff99db..ae90337b2cdd0a1dbc842c2144ea1d7a0e83f1f5 100644
--- a/remoting/client/jni/jni_frame_consumer.cc
+++ b/remoting/client/jni/jni_frame_consumer.cc
@@ -7,38 +7,12 @@
 #include "base/android/jni_android.h"
 #include "base/logging.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 {
 
@@ -70,7 +44,6 @@ void JniFrameConsumer::ApplyBuffer(const webrtc::DesktopSize& view_size,
   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()) {

[Sergey Ulanov, 2013/10/08 22:15:02]

I think if you get different view_size you can just ignore the frame here - it
belongs to the previous generation, before SetSourceSize() called
SetOutputSizeAndClip().

[Lambros, 2013/10/10 01:35:58]

Done.

@@ -83,6 +56,25 @@ void JniFrameConsumer::ApplyBuffer(const webrtc::DesktopSize& view_size,
     AllocateBuffer();

[Sergey Ulanov, 2013/10/08 22:15:02]

This also calls DrawBuffer() internally. That doesn't look right to me.

[Lambros, 2013/10/10 01:35:58]

I think that's OK. Whenever you allocate a new buffer, you always want the
FrameProducer to draw into it.

   }
 
+  // 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()) {
+    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());
@@ -123,13 +115,16 @@ void JniFrameConsumer::AllocateBuffer() {
       return;
     }
 
-    DirectDesktopFrame* buffer = new DirectDesktopFrame(view_size_.width(),
-                                                        view_size_.height());
+    webrtc::DesktopSize size(view_size_.width(), view_size_.height());
+    webrtc::DesktopFrame* buffer = new webrtc::BasicDesktopFrame(size);
 
-    // Update Java's reference to the buffer and record of its dimensions.
-    jni_runtime_->UpdateImageBuffer(view_size_.width(),
-                                    view_size_.height(),
-                                    buffer->buffer());
+    // Allocate a new Bitmap, store references here, and pass it to Java.
+    JNIEnv* env = base::android::AttachCurrentThread();
+    jobject bitmap_global_ref = env->NewGlobalRef(

[Sergey Ulanov, 2013/10/08 22:15:02]

Do you need to call NewGlobalRef() here? ScopedJavaGlobalRef should call
NewGlobalRef() internally, so you will leak the reference by calling
NewGlobalRef() without corresponding DeleteGlobalRef().

[Yaron, 2013/10/09 06:42:30]

This is why our base APIs are designed to return Scoped refs so it's clear and
leak free.

[Lambros, 2013/10/10 01:35:58]

Ouch! I thought I checked this, but I messed up.

So, IIUC, new ScopedJavaLocalRef<>(ref) takes ownership of |ref|. But new
ScopedJavaGlobalRef<>(ref) does *not* take ownership of |ref| and instead
creates a new global reference internally. I think the header comments should be
improved to explain this.

[Yaron, 2013/10/10 11:13:37]

I think when used in concert with ScopedJavaLocalRef the simple/straightforward
usage behaves as you'd want. The class comment for ScopedGlobalRef could
possibly be improved. Please feel free to send something my way.

+        jni_runtime_->NewBitmap(size).obj());
+    bitmap_global_ref_.Reset(env, bitmap_global_ref);
+    bitmap_.reset(new gfx::JavaBitmap(bitmap_global_ref));
+    jni_runtime_->UpdateFrameBitmap(bitmap_global_ref);
 
     frame_producer_->DrawBuffer(buffer);
   }