Add support for drawing video onto a Java ByteBuffer

remoting/client/jni/frame_consumer_impl.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/frame_consumer_impl.h"
+
+#include "base/android/jni_android.h"
+#include "base/logging.h"
+#include "media/base/yuv_convert.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 {
+
+class DirectDesktopFrame : public webrtc::BasicDesktopFrame {

[Wez, 2013/07/16 02:18:12]

What is "direct" about this? Wouldn't JniDesktopFrame be more specific?

[solb, 2013/07/16 18:00:57]

A direct byte buffer (see line 31) is a region of memory shared directly between
Java and C++. I'm adding a comment to explain that this class allocates memory
there.

+ public:
+  DirectDesktopFrame(int width, int height);
+
+  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);
+}
+
+}  // namespace
+
+namespace remoting {
+
+FrameConsumerImpl::FrameConsumerImpl()
+    : frame_producer_(NULL),
+      buffer_(NULL) {
+  media::InitializeCPUSpecificYUVConversions();

[Wez, 2013/07/16 02:18:12]

Isn't this intended as a one-off per-process initialization, i.e call it from
ChromotingJni's ctor, or similar?

[solb, 2013/07/16 18:00:57]

Done.

+}
+
+FrameConsumerImpl::~FrameConsumerImpl() {
+  ReturnBuffer(buffer_);

[Wez, 2013/07/16 02:18:12]

This doesn't look right - surely the caller must have returned the buffer to us,
otherwise we might be tearing it down while it's being used by them?

In fact, I'm not sure you need |buffer_| at all; just delete the supplied buffer
in ReturnBuffer().

[solb, 2013/07/16 18:00:57]

Done.

+}
+
+void FrameConsumerImpl::set_frame_producer(FrameProducer* producer) {
+  frame_producer_ = producer;
+}
+
+void FrameConsumerImpl::ApplyBuffer(const SkISize& view_size,
+                                    const SkIRect& clip_area,
+                                    webrtc::DesktopFrame* buffer,
+                                    const SkRegion& region) {
+  ChromotingJni::GetInstance()->RedrawCanvas();
+  // Give |frame_producer_| back the same buffer so that |buffer| and |buffer_|

[Wez, 2013/07/16 02:18:12]

Blank line before this comment, please.

[solb, 2013/07/16 18:00:57]

Done.

+  // are always the same exact pointer.

[Wez, 2013/07/16 02:18:12]

nit: Suggest "Return |buffer| to |frame_producer_| to write the next frame to."

Doesn't this mean we have serialized render/display/render/display rather than
pipelined?

[solb, 2013/07/16 18:00:57]

Done.

+  frame_producer_->DrawBuffer(buffer);
+}
+
+void FrameConsumerImpl::ReturnBuffer(webrtc::DesktopFrame* buffer) {
+  delete buffer;
+  // |buffer_| is the same pointer as |buffer|, so we just invalidated it.

[Wez, 2013/07/16 02:18:12]

Is that guaranteed by the FrameConsumer interface?  If so then DCHECK it before
deleting.

[Wez, 2013/07/16 02:18:12]

nit: Blank line before this comment, please.

[solb, 2013/07/16 18:00:57]

Done.

[solb, 2013/07/16 18:00:57]

Done.

+  buffer_ = NULL;
+}
+
+void FrameConsumerImpl::SetSourceSize(const SkISize& source_size,
+                                      const SkIPoint& dpi) {

[Wez, 2013/07/16 02:18:12]

Output buffer size isn't dependent on input buffer size, it's determined by the
|view_size| passed to ApplyBuffer - this method should be a no-op and
ApplyBuffer should create new buffer when necessary.

[solb, 2013/07/16 18:00:57]

I'm making ApplyBuffer() check that the buffer is sufficiently sized, but I
still need to create the initial buffer here. Unlike in PepperView, the initial
buffer is based on the source size because I *want* to render at that size;
otherwise, the buggy downscaling routines will get invoked.

+  view_size_ = source_size;
+  clip_area_ = SkIRect::MakeSize(source_size);

[Wez, 2013/07/16 02:18:12]

|clip_area_| is in output coordinates, so surely it should be based on
|view_size_|, not |source_size_|?

[solb, 2013/07/16 18:00:57]

Done.

+  frame_producer_->SetOutputSizeAndClip(view_size_, clip_area_);
+
+  if (!buffer_)
+    buffer_ = new DirectDesktopFrame(source_size.width(),
+                                     source_size.height());

[Wez, 2013/07/16 02:18:12]

This is creating a frame of the |source_size|, i.e. the original desktop
dimensions, whereas the consumer should be rendering at |view_size_|.

[solb, 2013/07/16 18:00:57]

Done.

+
+  ChromotingJni::GetInstance()->UpdateImageBuffer(
+      source_size.width(),
+      source_size.height(),
+      static_cast<DirectDesktopFrame*>(buffer_)->buffer());
+  frame_producer_->DrawBuffer(buffer_);
+}
+
+}  // namespace remoting