Add support for drawing video onto a Java ByteBuffer

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 "remoting/client/frame_producer.h"
+#include "remoting/client/jni/chromoting_jni.h"
+#include "third_party/webrtc/modules/desktop_capture/desktop_frame.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);

[Wez, 2013/07/16 18:45:53]

You need a(n empty) virtual dtor for this class, since the base class is
virtual.

[solb, 2013/07/16 21:35:41]

Done.

+
+  jobject buffer() const {
+    return buffer_;
+  }
+
+ private:
+  jobject buffer_;
+};
+
+DirectDesktopFrame::DirectDesktopFrame(int width, int height)

[Wez, 2013/07/16 18:45:53]

You can move this implementation in-line to the ctor declaration, above, if you
like.

[solb, 2013/07/16 21:35:41]

I'd rather not inline the constructor.

+    : webrtc::BasicDesktopFrame(webrtc::DesktopSize(width, height)) {
+  JNIEnv* env = base::android::AttachCurrentThread();
+  buffer_ = env->NewDirectByteBuffer(data(), stride()*height);
+}
+
+}  // namespace
+
+namespace remoting {
+
+JniFrameConsumer::JniFrameConsumer()
+    : frame_producer_(NULL) {
+}
+
+JniFrameConsumer::~JniFrameConsumer() {}

[Wez, 2013/07/16 18:45:53]

Because of the slightly messy Producer/Consumer interface you really need a
RequestReturnBuffers() blocking call here, to get the buffer back so you can
delete it, guaranteed that no-one else is using it at the time.  (AFAICT the
code in PepperView doesn't do this correctly, though - ReturnBuffers just spits
the buffer back at the producer...)

[solb, 2013/07/16 21:35:41]

Done.

+
+void JniFrameConsumer::set_frame_producer(FrameProducer* producer) {
+  frame_producer_ = producer;
+}
+
+void JniFrameConsumer::ApplyBuffer(const SkISize& view_size,
+                                    const SkIRect& clip_area,

[Wez, 2013/07/16 18:45:53]

nit: Indentation

[solb, 2013/07/16 21:35:41]

Done.

+                                    webrtc::DesktopFrame* buffer,
+                                    const SkRegion& region) {
+  ChromotingJni::GetInstance()->RedrawCanvas();

[Wez, 2013/07/16 18:45:53]

Rather than calling via the global instance, why not pass the instance in when
constructing this class?

[solb, 2013/07/16 21:35:41]

It's a singleton. I think that holding a reference to it would just clutter
things up and make another hurdle for someone trying to follow the code path.

[Wez, 2013/07/22 19:41:28]

I disagree; it's clearer for the reader to see the ChromotingJni instance as a
dependency of this class, and not be troubled by the fact that it's a global,
which isn't relevant to JniFrameConsumer's operation.

[solb, 2013/07/22 22:49:32]

Done.

+
+  if (view_size.width() > view_size_.width() ||
+      view_size.height() > view_size_.height()) {
+    LOG(INFO) << "Existing buffer is too small";
+    view_size_ = view_size;
+    frame_producer_->RequestReturnBuffers(base::Closure());

[Wez, 2013/07/16 18:45:53]

You don't need this - the caller just returned the buffer to you - you can just
delete it and allocate a new one before calling DrawBuffer.

[solb, 2013/07/16 21:35:41]

Done.

+    return;
+  }
+
+  // Return |buffer| to |frame_producer_|.

[Wez, 2013/07/16 18:45:53]

nit: Suggest: Supply |frame_producer_| with a buffer to render the next frame
into.

[solb, 2013/07/16 21:35:41]

Done.

+  frame_producer_->DrawBuffer(buffer);
+}
+
+void JniFrameConsumer::ReturnBuffer(webrtc::DesktopFrame* buffer) {
+  LOG(INFO) << "Returning image buffer";
+
+  // Decide whether we will need a replacement buffer.
+  bool reallocate = view_size_.width() > buffer->size().width() ||
+      view_size_.height() > buffer->size().height();

[Wez, 2013/07/16 18:45:53]

This is fragile in the case where the view size changes and the client is
stopped immediately afterward, since you'll see buffers returned but then
re-dispatch them out.

[solb, 2013/07/16 21:35:41]

I just got rid of the reallocation, since the above change means we no longer
request the buffers to be returned in case of a size change.

+
+  delete buffer;
+  buffer = NULL;

[Wez, 2013/07/16 18:45:53]

nit: no need to NULL |buffer| here.

[solb, 2013/07/16 21:35:41]

Done.

+
+  if (reallocate) {
+    LOG(INFO) << "Allocating replacement image buffer";
+    frame_producer_->DrawBuffer(AllocateBuffer());
+  }
+}
+
+void JniFrameConsumer::SetSourceSize(const SkISize& source_size,
+                                      const SkIPoint& dpi) {

[Wez, 2013/07/16 18:45:53]

nit: Indentation.

[solb, 2013/07/16 21:35:41]

Done.

+  view_size_ = source_size;

[Wez, 2013/07/16 18:45:53]

nit: Add a comment to explain that we currently render the desktop 1:1 in this
client, and let the OS do pinch/zoom scaling in h/w.

[solb, 2013/07/16 21:35:41]

Done.

+  clip_area_ = SkIRect::MakeSize(view_size_);
+  frame_producer_->SetOutputSizeAndClip(view_size_, clip_area_);
+
+  // Allocate a buffer and start drawing frames onto it.
+  frame_producer_->DrawBuffer(AllocateBuffer());

[Wez, 2013/07/16 18:45:53]

This don't look right - you're un-conditionally allocating a new buffer and
passing it to the frame producer to render into every time the source desktop
dimensions change. If you happen to already have passed it a buffer to render
into then that means you now have two buffers in flight, which this code
explicitly doesn't cope with!

What you probably want instead is RequestReturnBuffers() here. You can then
either pass a Closure that will allocate new buffer and call DrawBuffers, or do
that in ReturnBuffer. Either way you'll need a flag so that ApplyBuffer and/or
ReturnBuffer know whether to pass a buffer back via DrawBuffer.

[solb, 2013/07/16 21:35:41]

Done.

+}
+
+webrtc::DesktopFrame* JniFrameConsumer::AllocateBuffer() {
+  DirectDesktopFrame* buffer = new DirectDesktopFrame(view_size_.width(),
+                                                      view_size_.height());
+
+  // Update Java's reference to the buffer and record of its dimensions.
+  ChromotingJni::GetInstance()->UpdateImageBuffer(
+      view_size_.width(),
+      view_size_.height(),
+      buffer->buffer());
+
+  return buffer;
+}
+
+}  // namespace remoting