Add support for drawing video onto a Java ByteBuffer

Add support for drawing video onto a Java ByteBuffer

We now place frames into a Java-accessible memory region as we decode them.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=212063

[solb, 2013-07-15 22:59:39]

This is designed to apply on top of https://codereview.chromium.org/19253003/,
so ignore all those trybot errors...

[garykac, 2013-07-16 01:28:19]

lgtm

https://codereview.chromium.org/19297003/diff/1/remoting/client/jni/frame_con...
File remoting/client/jni/frame_consumer_impl.h (right):

https://codereview.chromium.org/19297003/diff/1/remoting/client/jni/frame_con...
remoting/client/jni/frame_consumer_impl.h:43: // State is to be used from the
display thread.
This sounds odd. Can you clarify what you mean here?

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

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/chromo...
File remoting/client/jni/chromoting_jni_instance.cc (right):

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/chromo...
remoting/client/jni/chromoting_jni_instance.cc:132:
frame_consumer_->Attach(view_->AsWeakPtr());
Rather than have JniFrameConsumer SupportsWeakPtr, have this class contain a
WeakPtrFactory that references it, and that is deleted or invalidated before
|frame_consumer_|, and use that to generate the WeakPtr.

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/chromo...
File remoting/client/jni/chromoting_jni_instance.h (right):

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/chromo...
remoting/client/jni/chromoting_jni_instance.h:54: void RedrawGraphics();
nit: RedrawCanvas, for consistency w/ ChromotingJni, or RedrawDesktop?

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
File remoting/client/jni/frame_consumer_impl.cc (right):

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.cc:16: class DirectDesktopFrame : public
webrtc::BasicDesktopFrame {
What is "direct" about this? Wouldn't JniDesktopFrame be more specific?

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.cc:41:
media::InitializeCPUSpecificYUVConversions();
Isn't this intended as a one-off per-process initialization, i.e call it from
ChromotingJni's ctor, or similar?

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.cc:45: ReturnBuffer(buffer_);
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().

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.cc:57: // Give |frame_producer_| back
the same buffer so that |buffer| and |buffer_|
Blank line before this comment, please.

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.cc:58: // are always the same exact
pointer.
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?

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.cc:64: // |buffer_| is the same pointer
as |buffer|, so we just invalidated it.
Is that guaranteed by the FrameConsumer interface?  If so then DCHECK it before
deleting.

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.cc:64: // |buffer_| is the same pointer
as |buffer|, so we just invalidated it.
nit: Blank line before this comment, please.

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.cc:69: const SkIPoint& dpi) {
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.

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.cc:71: clip_area_ =
SkIRect::MakeSize(source_size);
|clip_area_| is in output coordinates, so surely it should be based on
|view_size_|, not |source_size_|?

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.cc:76: source_size.height());
This is creating a frame of the |source_size|, i.e. the original desktop
dimensions, whereas the consumer should be rendering at |view_size_|.

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
File remoting/client/jni/frame_consumer_impl.h (right):

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.h:22: class FrameConsumerImpl
Shouldn't this be something like e.g. JniFrameConsumer?

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.h:24: public
base::SupportsWeakPtr<FrameConsumerImpl> {
Use WeakPtrFactory rather than SupportsWeakPtr, please; WeakPtr is an aspect of
the way in which the caller manages this object's lifetime, not an intrinsic
property of the FrameConsumerImpl itself.

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.h:27: 
No need for a blank line here.

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.h:28: ~FrameConsumerImpl();
virtual

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.h:33: // FrameConsumer implementation:
Here and everywhere else, please end sentences with "." instead of ":"

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

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/chromo...
File remoting/client/jni/chromoting_jni_instance.cc (right):

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/chromo...
remoting/client/jni/chromoting_jni_instance.cc:132:
frame_consumer_->Attach(view_->AsWeakPtr());
On 2013/07/16 02:18:12, Wez wrote:
> Rather than have JniFrameConsumer SupportsWeakPtr, have this class contain a
> WeakPtrFactory that references it, and that is deleted or invalidated before
> |frame_consumer_|, and use that to generate the WeakPtr.

Done.

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/chromo...
File remoting/client/jni/chromoting_jni_instance.h (right):

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/chromo...
remoting/client/jni/chromoting_jni_instance.h:54: void RedrawGraphics();
On 2013/07/16 02:18:12, Wez wrote:
> nit: RedrawCanvas, for consistency w/ ChromotingJni, or RedrawDesktop?

Done.

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
File remoting/client/jni/frame_consumer_impl.cc (right):

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.cc:16: class DirectDesktopFrame : public
webrtc::BasicDesktopFrame {
On 2013/07/16 02:18:12, Wez wrote:
> What is "direct" about this? Wouldn't JniDesktopFrame be more specific?

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.

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.cc:41:
media::InitializeCPUSpecificYUVConversions();
On 2013/07/16 02:18:12, Wez wrote:
> Isn't this intended as a one-off per-process initialization, i.e call it from
> ChromotingJni's ctor, or similar?

Done.

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.cc:45: ReturnBuffer(buffer_);
On 2013/07/16 02:18:12, Wez wrote:
> 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().

Done.

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.cc:57: // Give |frame_producer_| back
the same buffer so that |buffer| and |buffer_|
On 2013/07/16 02:18:12, Wez wrote:
> Blank line before this comment, please.

Done.

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.cc:58: // are always the same exact
pointer.
On 2013/07/16 02:18:12, Wez wrote:
> 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?

Done.

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.cc:64: // |buffer_| is the same pointer
as |buffer|, so we just invalidated it.
On 2013/07/16 02:18:12, Wez wrote:
> Is that guaranteed by the FrameConsumer interface?  If so then DCHECK it
before
> deleting.

Done.

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.cc:64: // |buffer_| is the same pointer
as |buffer|, so we just invalidated it.
On 2013/07/16 02:18:12, Wez wrote:
> nit: Blank line before this comment, please.

Done.

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.cc:69: const SkIPoint& dpi) {
On 2013/07/16 02:18:12, Wez wrote:
> 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.

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.

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.cc:71: clip_area_ =
SkIRect::MakeSize(source_size);
On 2013/07/16 02:18:12, Wez wrote:
> |clip_area_| is in output coordinates, so surely it should be based on
> |view_size_|, not |source_size_|?

Done.

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.cc:76: source_size.height());
On 2013/07/16 02:18:12, Wez wrote:
> This is creating a frame of the |source_size|, i.e. the original desktop
> dimensions, whereas the consumer should be rendering at |view_size_|.

Done.

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
File remoting/client/jni/frame_consumer_impl.h (right):

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.h:22: class FrameConsumerImpl
On 2013/07/16 02:18:12, Wez wrote:
> Shouldn't this be something like e.g. JniFrameConsumer?

Done.

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.h:24: public
base::SupportsWeakPtr<FrameConsumerImpl> {
On 2013/07/16 02:18:12, Wez wrote:
> Use WeakPtrFactory rather than SupportsWeakPtr, please; WeakPtr is an aspect
of
> the way in which the caller manages this object's lifetime, not an intrinsic
> property of the FrameConsumerImpl itself.

Done.

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.h:27: 
On 2013/07/16 02:18:12, Wez wrote:
> No need for a blank line here.

Done.

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.h:28: ~FrameConsumerImpl();
On 2013/07/16 02:18:12, Wez wrote:
> virtual

Thanks!

https://codereview.chromium.org/19297003/diff/6001/remoting/client/jni/frame_...
remoting/client/jni/frame_consumer_impl.h:33: // FrameConsumer implementation:
On 2013/07/16 02:18:12, Wez wrote:
> Here and everywhere else, please end sentences with "." instead of ":"

Done.

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

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/chrom...
File remoting/client/jni/chromoting_jni.h (right):

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni.h:60: // |session| is null.
nit: |session| -> |session_|

It doesn't make sense to document this as "don't call when session_ is null",
though, since the only way for the caller to determine that is by calling this
method... I think you mean "only call while a host connection is active"?

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/chrom...
File remoting/client/jni/chromoting_jni_instance.cc (right):

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni_instance.cc:15: const char* const CHAT_SERVER
= "talk.google.com";
IIRC 'const' at file scope is implicitly static-linked so you don't need the
anonymous namespace here.

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni_instance.cc:24: const char* auth_token,
nit: Indentation

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni_instance.cc:45:
view_weak_factory_->InvalidateWeakPtrs();
nit: Since you only grab a WeakPtr to the view at construction time, you could
just reset() view_weak_factory_ here, or even leave it to invalidate the WeakPtr
when the scoped_ptr<> holding it tears it down, though that would mean you'd
need the |view| and |view_weak_factory| member order to be correct (I think it
currently is). Up to you.

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
File remoting/client/jni/jni_frame_consumer.cc (right):

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:19: DirectDesktopFrame(int width, int
height);
You need a(n empty) virtual dtor for this class, since the base class is
virtual.

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:29:
DirectDesktopFrame::DirectDesktopFrame(int width, int height)
You can move this implementation in-line to the ctor declaration, above, if you
like.

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:43:
JniFrameConsumer::~JniFrameConsumer() {}
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...)

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:50: const SkIRect& clip_area,
nit: Indentation

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:53:
ChromotingJni::GetInstance()->RedrawCanvas();
Rather than calling via the global instance, why not pass the instance in when
constructing this class?

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:59:
frame_producer_->RequestReturnBuffers(base::Closure());
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.

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:63: // Return |buffer| to
|frame_producer_|.
nit: Suggest: Supply |frame_producer_| with a buffer to render the next frame
into.

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:72: view_size_.height() >
buffer->size().height();
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.

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:75: buffer = NULL;
nit: no need to NULL |buffer| here.

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:84: const SkIPoint& dpi) {
nit: Indentation.

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:85: view_size_ = source_size;
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.

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:90:
frame_producer_->DrawBuffer(AllocateBuffer());
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:40]

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/chrom...
File remoting/client/jni/chromoting_jni.h (right):

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni.h:60: // |session| is null.
On 2013/07/16 18:45:53, Wez wrote:
> nit: |session| -> |session_|
> 
> It doesn't make sense to document this as "don't call when session_ is null",
> though, since the only way for the caller to determine that is by calling this
> method... I think you mean "only call while a host connection is active"?

Done.

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/chrom...
File remoting/client/jni/chromoting_jni_instance.cc (right):

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni_instance.cc:15: const char* const CHAT_SERVER
= "talk.google.com";
On 2013/07/16 18:45:53, Wez wrote:
> IIRC 'const' at file scope is implicitly static-linked so you don't need the
> anonymous namespace here.

Done.

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni_instance.cc:24: const char* auth_token,
On 2013/07/16 18:45:53, Wez wrote:
> nit: Indentation

Done.

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni_instance.cc:45:
view_weak_factory_->InvalidateWeakPtrs();
On 2013/07/16 18:45:53, Wez wrote:
> nit: Since you only grab a WeakPtr to the view at construction time, you could
> just reset() view_weak_factory_ here, or even leave it to invalidate the
WeakPtr
> when the scoped_ptr<> holding it tears it down, though that would mean you'd
> need the |view| and |view_weak_factory| member order to be correct (I think it
> currently is). Up to you.

Done.

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
File remoting/client/jni/jni_frame_consumer.cc (right):

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:19: DirectDesktopFrame(int width, int
height);
On 2013/07/16 18:45:53, Wez wrote:
> You need a(n empty) virtual dtor for this class, since the base class is
> virtual.

Done.

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:29:
DirectDesktopFrame::DirectDesktopFrame(int width, int height)
On 2013/07/16 18:45:53, Wez wrote:
> You can move this implementation in-line to the ctor declaration, above, if
you
> like.

I'd rather not inline the constructor.

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:43:
JniFrameConsumer::~JniFrameConsumer() {}
On 2013/07/16 18:45:53, Wez wrote:
> 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...)

Done.

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:50: const SkIRect& clip_area,
On 2013/07/16 18:45:53, Wez wrote:
> nit: Indentation

Done.

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:53:
ChromotingJni::GetInstance()->RedrawCanvas();
On 2013/07/16 18:45:53, Wez wrote:
> Rather than calling via the global instance, why not pass the instance in when
> constructing this class?

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.

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:59:
frame_producer_->RequestReturnBuffers(base::Closure());
On 2013/07/16 18:45:53, Wez wrote:
> 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.

Done.

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:63: // Return |buffer| to
|frame_producer_|.
On 2013/07/16 18:45:53, Wez wrote:
> nit: Suggest: Supply |frame_producer_| with a buffer to render the next frame
> into.

Done.

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:72: view_size_.height() >
buffer->size().height();
On 2013/07/16 18:45:53, Wez wrote:
> 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.

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.

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:75: buffer = NULL;
On 2013/07/16 18:45:53, Wez wrote:
> nit: no need to NULL |buffer| here.

Done.

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:84: const SkIPoint& dpi) {
On 2013/07/16 18:45:53, Wez wrote:
> nit: Indentation.

Done.

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:85: view_size_ = source_size;
On 2013/07/16 18:45:53, Wez wrote:
> 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.

Done.

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:90:
frame_producer_->DrawBuffer(AllocateBuffer());
On 2013/07/16 18:45:53, Wez wrote:
> 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.

Done.

[commit-bot: I haz the power, 2013-07-17 07:17:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/solb@chromium.org/19297003/39001

[commit-bot: I haz the power, 2013-07-17 16:36:55]

Change committed as 212063

[Wez, 2013-07-22 19:41:27]

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
File remoting/client/jni/jni_frame_consumer.cc (right):

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:53:
ChromotingJni::GetInstance()->RedrawCanvas();
On 2013/07/16 21:35:41, solb wrote:
> On 2013/07/16 18:45:53, Wez wrote:
> > Rather than calling via the global instance, why not pass the instance in
when
> > constructing this class?
> 
> 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.

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.

https://codereview.chromium.org/19297003/diff/39001/remoting/client/jni/jni_f...
File remoting/client/jni/jni_frame_consumer.cc (right):

https://codereview.chromium.org/19297003/diff/39001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:45: : provide_buffer_(true),
nit: This should be e.g. |shutting_down_| or |in_dtor_| to more precisely
describe what it means, since the code assumes it's only false during shutdown.

https://codereview.chromium.org/19297003/diff/39001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:82: if (provide_buffer_)
If |provide_buffer_| is false _and_ the view hasn't got bigger then you'll
neither delete nor pass back |buffer|, will you?

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

See https://codereview.chromium.org/19967007.

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
File remoting/client/jni/jni_frame_consumer.cc (right):

https://codereview.chromium.org/19297003/diff/11001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:53:
ChromotingJni::GetInstance()->RedrawCanvas();
On 2013/07/22 19:41:28, Wez wrote:
> On 2013/07/16 21:35:41, solb wrote:
> > On 2013/07/16 18:45:53, Wez wrote:
> > > Rather than calling via the global instance, why not pass the instance in
> when
> > > constructing this class?
> > 
> > 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.
> 
> 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.

Done.

https://codereview.chromium.org/19297003/diff/39001/remoting/client/jni/jni_f...
File remoting/client/jni/jni_frame_consumer.cc (right):

https://codereview.chromium.org/19297003/diff/39001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:45: : provide_buffer_(true),
On 2013/07/22 19:41:28, Wez wrote:
> nit: This should be e.g. |shutting_down_| or |in_dtor_| to more precisely
> describe what it means, since the code assumes it's only false during
shutdown.

Done.

https://codereview.chromium.org/19297003/diff/39001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:82: if (provide_buffer_)
On 2013/07/22 19:41:28, Wez wrote:
> If |provide_buffer_| is false _and_ the view hasn't got bigger then you'll
> neither delete nor pass back |buffer|, will you?

Oops! Thanks for catching that.