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

remoting/client/jni/jni_frame_consumer.cc

 // Copyright 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 "remoting/client/jni/jni_frame_consumer.h"
 
 #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 {
 
 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;

[Sergey Ulanov, 2013/10/11 20:32:17]

Don't need this.

[Lambros, 2013/10/15 23:50:33]

Done.

 
   // Don't destroy the object until we've deleted the buffer.
   base::WaitableEvent done_event(true, false);
   frame_producer_->RequestReturnBuffers(
       base::Bind(&base::WaitableEvent::Signal, base::Unretained(&done_event)));

[Sergey Ulanov, 2013/10/11 20:32:17]

This doesn't really cleanup the buffers. ReturnBuffers() will not be called by
the proxy so we are leaking the frames.
PepperView::~PepperView() deleting the frames in the queue, and so
RequestReturnBuffers() is required to make sure they are not reused on the
decode thread. In this case RequestReturnBuffers() doesn't really do anything.
The leak can be fixed later, please TODO() and open a bug. I think you'll need
to do something similar to PepperView - keep list of buffers and delete them
here explicitly.

[Lambros, 2013/10/15 23:50:33]

Done.

   done_event.Wait();
 }
 
 void JniFrameConsumer::set_frame_producer(FrameProducer* producer) {
   frame_producer_ = producer;
 }
 
 void JniFrameConsumer::ApplyBuffer(const webrtc::DesktopSize& view_size,
                                    const webrtc::DesktopRect& clip_area,
                                    webrtc::DesktopFrame* buffer,
                                    const webrtc::DesktopRegion& region) {
   DCHECK(jni_runtime_->display_task_runner()->BelongsToCurrentThread());
 
   scoped_ptr<webrtc::DesktopFrame> buffer_scoped(buffer);
+
+  if (!view_size_.equals(view_size)) {
+    // Drop the frame, since the data belongs to the previous generation,
+    // before SetSourceSize() called SetOutputSizeAndClip().
+    return;
+  }
+
+  // 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());

[Sergey Ulanov, 2013/10/11 20:32:17]

nit: make this a reference.

[Lambros, 2013/10/15 23:50:33]

Done.

+    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();
 
-  if (view_size.width() > view_size_.width() ||
-      view_size.height() > view_size_.height()) {
-    LOG(INFO) << "Existing buffer is too small";
-    view_size_ = view_size;
-
-    // Manually destroy the old buffer before allocating a new one to prevent
-    // our memory footprint from temporarily ballooning.
-    buffer_scoped.reset();
-    AllocateBuffer();
-  }
-
   // Supply |frame_producer_| with a buffer to render the next frame into.
   if (!in_dtor_)

[Sergey Ulanov, 2013/10/11 20:32:17]

Don't need this. This method will not be called from the destructor.

[Lambros, 2013/10/15 23:50:33]

Done.

     frame_producer_->DrawBuffer(buffer_scoped.release());
 }
 
 void JniFrameConsumer::ReturnBuffer(webrtc::DesktopFrame* buffer) {
   DCHECK(jni_runtime_->display_task_runner()->BelongsToCurrentThread());
   LOG(INFO) << "Returning image buffer";
   delete buffer;
 }
 
 void JniFrameConsumer::SetSourceSize(const webrtc::DesktopSize& source_size,
                                      const webrtc::DesktopVector& dpi) {
   DCHECK(jni_runtime_->display_task_runner()->BelongsToCurrentThread());
 
   // We currently render the desktop 1:1 and perform pan/zoom scaling
   // and cropping on the managed canvas.
   view_size_ = 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)));
+  AllocateBuffer();
 }
 
 FrameConsumer::PixelFormat JniFrameConsumer::GetPixelFormat() {
   return FORMAT_RGBA;
 }
 
 void JniFrameConsumer::AllocateBuffer() {
+  DCHECK(jni_runtime_->display_task_runner()->BelongsToCurrentThread());
+
   // 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;
-    }
+  if (in_dtor_)

[Sergey Ulanov, 2013/10/11 20:32:17]

Don't need this.

[Lambros, 2013/10/15 23:50:33]

Done.

+    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);

[Sergey Ulanov, 2013/10/11 20:32:17]

nit: move this line just before DrawBuffer.

[Lambros, 2013/10/15 23:50:33]

Done.

 
-    // 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();
 
-    frame_producer_->DrawBuffer(buffer);
-  }
+  // |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());
+
+  frame_producer_->DrawBuffer(buffer);
 }
 
 }  // namespace remoting