Adding an interface to allow extention of the audio player for CRD and iOS.

Adding an interface to allow extension of the audio player for CRD and iOS.

This CL changes the audio player for CRD into a consumer of audio packets. The choice of consumer is based on a consumer/provider pattern, the intent is to create a new class that will be used in iOS in a following CL that acts as a middle buffer between the CRD Client and the iOS audio player which will extend both AudioConsumer and AudioProvider(not in this CL). Because of this change, I needed the current usage of audio player to extend from a more generic interface (AudioConsumer) which could apply to either this future buffer or audio player.

BUG=611181
TEST=Manual tested webapp client.

R=lambroslambrou@chromium.org

Committed: https://crrev.com/eea69600e07c1d402758d8dd03f5579caddde736
Cr-Commit-Position: refs/heads/master@{#400776}

[nicholss, 2016-06-09 17:26:39]

https://codereview.chromium.org/2052723002/diff/1/remoting/client/audio_playe...
File remoting/client/audio_player.cc (right):

https://codereview.chromium.org/2052723002/diff/1/remoting/client/audio_playe...
remoting/client/audio_player.cc:30: void
AudioPlayer::AddAudioPacket(std::unique_ptr<AudioPacket> packet) {
Add vs Process is to align with how consumers normally consume with "add" so I
used that.

https://codereview.chromium.org/2052723002/diff/1/remoting/client/audio_player.h
File remoting/client/audio_player.h (right):

https://codereview.chromium.org/2052723002/diff/1/remoting/client/audio_playe...
remoting/client/audio_player.h:32: ~AudioPlayer() override;
Maybe this is a mistake? Do I have to override the destructor ?

https://codereview.chromium.org/2052723002/diff/1/remoting/client/jni/chromot...
File remoting/client/jni/chromoting_jni_instance.h (right):

https://codereview.chromium.org/2052723002/diff/1/remoting/client/jni/chromot...
remoting/client/jni/chromoting_jni_instance.h:183:
std::unique_ptr<AudioPlayerAndroid> audio_player_;
So the large change here is that now the chromoting instance objects hold the
master ref for the audio player. It use to be we would just pass the ref down to
the audio decoder and it would hold the ref until it died. Now we pass a pointer
to the decoder.

[Lambros, 2016-06-09 19:01:05]

https://codereview.chromium.org/2052723002/diff/1/remoting/client/audio_player.h
File remoting/client/audio_player.h (right):

https://codereview.chromium.org/2052723002/diff/1/remoting/client/audio_playe...
remoting/client/audio_player.h:32: ~AudioPlayer() override;
On 2016/06/09 17:26:39, nicholss wrote:
> Maybe this is a mistake? Do I have to override the destructor ?

This is fine. The dtor calls STLDeleteElements on a list, so you do need a dtor
to avoid memory-leaks. 'override' is correct, as AudioConsumer declares a
virtual dtor. And that is correct, as AudioConsumer defines other virtual
methods, so the dtor must also be virtual.

https://codereview.chromium.org/2052723002/diff/1/remoting/client/jni/chromot...
File remoting/client/jni/chromoting_jni_instance.h (right):

https://codereview.chromium.org/2052723002/diff/1/remoting/client/jni/chromot...
remoting/client/jni/chromoting_jni_instance.h:183:
std::unique_ptr<AudioPlayerAndroid> audio_player_;
On 2016/06/09 17:26:39, nicholss wrote:
> So the large change here is that now the chromoting instance objects hold the
> master ref for the audio player. It use to be we would just pass the ref down
to
> the audio decoder and it would hold the ref until it died. Now we pass a
pointer
> to the decoder. 

I'm not sure about this. What happens after ChromotingJniInstance gets deleted?
How do we make sure the audio decoder doesn't try to use the audio_player
afterwards?

If audio_player is used on another thread, then it should ideally be owned by an
object that runs code on that thread.

https://codereview.chromium.org/2052723002/diff/20001/remoting/client/audio_c...
File remoting/client/audio_consumer.h (right):

https://codereview.chromium.org/2052723002/diff/20001/remoting/client/audio_c...
remoting/client/audio_consumer.h:1: // Copyright (c) 2016 The Chromium Authors.
All rights reserved.
No (c) in copyright notice.

https://codereview.chromium.org/2052723002/diff/20001/remoting/client/audio_c...
remoting/client/audio_consumer.h:18: virtual void
AddAudioPacket(std::unique_ptr<AudioPacket> packet) = 0;
Blank line between ctor/dtor and other methods.

https://codereview.chromium.org/2052723002/diff/20001/remoting/client/jni/chr...
File remoting/client/jni/chromoting_jni_instance.cc (right):

https://codereview.chromium.org/2052723002/diff/20001/remoting/client/jni/chr...
remoting/client/jni/chromoting_jni_instance.cc:403: if (!audio_player_) {
Can remove the 'if', for consistency with the lines above.

audio_player_ should be null here, though that isn't guaranteed until
crbug.com/617471 is fixed.

https://codereview.chromium.org/2052723002/diff/20001/remoting/client/plugin/...
File remoting/client/plugin/chromoting_instance.h (right):

https://codereview.chromium.org/2052723002/diff/20001/remoting/client/plugin/...
remoting/client/plugin/chromoting_instance.h:252:
std::unique_prt<PepperAudioPlayer> audio_player_;
Should be unique_ptr - did this compile?

[nicholss, 2016-06-09 19:46:30]

https://codereview.chromium.org/2052723002/diff/1/remoting/client/jni/chromot...
File remoting/client/jni/chromoting_jni_instance.h (right):

https://codereview.chromium.org/2052723002/diff/1/remoting/client/jni/chromot...
remoting/client/jni/chromoting_jni_instance.h:183:
std::unique_ptr<AudioPlayerAndroid> audio_player_;
On 2016/06/09 19:01:05, Lambros wrote:
> On 2016/06/09 17:26:39, nicholss wrote:
> > So the large change here is that now the chromoting instance objects hold
the
> > master ref for the audio player. It use to be we would just pass the ref
down
> to
> > the audio decoder and it would hold the ref until it died. Now we pass a
> pointer
> > to the decoder. 
> 
> I'm not sure about this. What happens after ChromotingJniInstance gets
deleted?
> How do we make sure the audio decoder doesn't try to use the audio_player
> afterwards?
> 
> If audio_player is used on another thread, then it should ideally be owned by
an
> object that runs code on that thread.

For that to happen the chromoting client instance would have to die before the
chromoting client. And in all cases I can see, there is a note that client must
be released before all others in the destructor of client_instance. 

So yes it can happen but only if we change how we use client and client
instance.

https://codereview.chromium.org/2052723002/diff/20001/remoting/client/audio_c...
File remoting/client/audio_consumer.h (right):

https://codereview.chromium.org/2052723002/diff/20001/remoting/client/audio_c...
remoting/client/audio_consumer.h:1: // Copyright (c) 2016 The Chromium Authors.
All rights reserved.
On 2016/06/09 19:01:05, Lambros wrote:
> No (c) in copyright notice.

weird. ok, should I fix all the other usages like this too?

https://codereview.chromium.org/2052723002/diff/20001/remoting/client/audio_c...
remoting/client/audio_consumer.h:18: virtual void
AddAudioPacket(std::unique_ptr<AudioPacket> packet) = 0;
On 2016/06/09 19:01:05, Lambros wrote:
> Blank line between ctor/dtor and other methods.

Done.

https://codereview.chromium.org/2052723002/diff/20001/remoting/client/plugin/...
File remoting/client/plugin/chromoting_instance.h (right):

https://codereview.chromium.org/2052723002/diff/20001/remoting/client/plugin/...
remoting/client/plugin/chromoting_instance.h:252:
std::unique_prt<PepperAudioPlayer> audio_player_;
On 2016/06/09 19:01:05, Lambros wrote:
> Should be unique_ptr - did this compile?

I have not tried to compile for android yet because I need to push the code to
my linux box. android does not work on mac. 

I will do it now.

[Sergey Ulanov, 2016-06-09 20:23:44]

sergeyu@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Sergey Ulanov, 2016-06-09 20:23:45]

https://codereview.chromium.org/2052723002/diff/1/remoting/client/jni/chromot...
File remoting/client/jni/chromoting_jni_instance.h (right):

https://codereview.chromium.org/2052723002/diff/1/remoting/client/jni/chromot...
remoting/client/jni/chromoting_jni_instance.h:183:
std::unique_ptr<AudioPlayerAndroid> audio_player_;
On 2016/06/09 19:46:30, nicholss wrote:
> On 2016/06/09 19:01:05, Lambros wrote:
> > On 2016/06/09 17:26:39, nicholss wrote:
> > > So the large change here is that now the chromoting instance objects hold
> the
> > > master ref for the audio player. It use to be we would just pass the ref
> down
> > to
> > > the audio decoder and it would hold the ref until it died. Now we pass a
> > pointer
> > > to the decoder. 
> > 
> > I'm not sure about this. What happens after ChromotingJniInstance gets
> deleted?
> > How do we make sure the audio decoder doesn't try to use the audio_player
> > afterwards?
> > 
> > If audio_player is used on another thread, then it should ideally be owned
by
> an
> > object that runs code on that thread.
> 
> For that to happen the chromoting client instance would have to die before the
> chromoting client. And in all cases I can see, there is a note that client
must
> be released before all others in the destructor of client_instance. 
> 
> So yes it can happen but only if we change how we use client and client
> instance.

This may be acceptable, but it is error prone. It's not clear why you need to
change this.

If there is a good reason for this change then please mention it in the comments
for ChromotingClient that the AudioConsumer must outlive the client. Also here
you want to move audio_player_ before client_ to ensure the player destroyed
after the client (class members are destroyed in reverse order).

https://codereview.chromium.org/2052723002/diff/40001/remoting/client/audio_c...
File remoting/client/audio_consumer.h (right):

https://codereview.chromium.org/2052723002/diff/40001/remoting/client/audio_c...
remoting/client/audio_consumer.h:10: #include "remoting/proto/audio.pb.h"
don't need this include. Forward-declare AudioPacket. See
https://www.chromium.org/developers/coding-style#TOC-Forward-declarations-vs....

https://codereview.chromium.org/2052723002/diff/40001/remoting/client/audio_c...
remoting/client/audio_consumer.h:14: class AudioConsumer {
Add a comment to make it clear what this interface is used for.

https://codereview.chromium.org/2052723002/diff/40001/remoting/client/audio_c...
remoting/client/audio_consumer.h:17: virtual ~AudioConsumer();
I think you need {} here and for the constructor.

https://codereview.chromium.org/2052723002/diff/40001/remoting/client/audio_d...
File remoting/client/audio_decode_scheduler.h (right):

https://codereview.chromium.org/2052723002/diff/40001/remoting/client/audio_d...
remoting/client/audio_decode_scheduler.h:33: AudioConsumer* audio_consumer);
Can we pass consumer ownership here?

https://codereview.chromium.org/2052723002/diff/40001/remoting/client/audio_p...
File remoting/client/audio_player.h (right):

https://codereview.chromium.org/2052723002/diff/40001/remoting/client/audio_p...
remoting/client/audio_player.h:50: void
AddAudioPacket(std::unique_ptr<AudioPacket> packet) override;
Add comment:
 // AudioConsumer implementation.

also please move this to the public section

https://codereview.chromium.org/2052723002/diff/40001/remoting/client/chromot...
File remoting/client/chromoting_client.h (right):

https://codereview.chromium.org/2052723002/diff/40001/remoting/client/chromot...
remoting/client/chromoting_client.h:55: AudioConsumer* audio_consumer);
pass ownership of the consumer here?

https://codereview.chromium.org/2052723002/diff/40001/remoting/client/plugin/...
File remoting/client/plugin/chromoting_instance.cc (right):

https://codereview.chromium.org/2052723002/diff/40001/remoting/client/plugin/...
remoting/client/plugin/chromoting_instance.cc:667: this,
this fits on the previous line. git-cl-format please.

[Lambros, 2016-06-09 20:52:08]

https://codereview.chromium.org/2052723002/diff/20001/remoting/client/plugin/...
File remoting/client/plugin/chromoting_instance.h (right):

https://codereview.chromium.org/2052723002/diff/20001/remoting/client/plugin/...
remoting/client/plugin/chromoting_instance.h:252:
std::unique_prt<PepperAudioPlayer> audio_player_;
On 2016/06/09 19:46:30, nicholss wrote:
> On 2016/06/09 19:01:05, Lambros wrote:
> > Should be unique_ptr - did this compile?
> 
> I have not tried to compile for android yet because I need to push the code to
> my linux box. android does not work on mac. 
> 
> I will do it now.

This isn't Android code. You need to compile the NaCl plugin. Building
remoting_webapp would do it, then you can test to make sure the webapp still
works.

[nicholss, 2016-06-09 20:57:10]

With this current class structure I am getting the following error, I am not
sure what I am missing here...

Undefined symbols for architecture x86_64:
  "remoting::AudioConsumer::~AudioConsumer()", referenced from:
      remoting::AudioPlayer::~AudioPlayer() in audio_player.o
  "vtable for remoting::AudioConsumer", referenced from:
      remoting::AudioConsumer::AudioConsumer() in audio_player.o
  NOTE: a missing vtable usually means the first non-inline virtual member
function has no definition.
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
Traceback (most recent call last):
  File "../../build/toolchain/mac/linker_driver.py", line 186, in <module>
    Main(sys.argv)
  File "../../build/toolchain/mac/linker_driver.py", line 68, in Main
    subprocess.check_call(compiler_driver_args)
  File
"/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py",
line 540, in check_call
    raise CalledProcessError(retcode, cmd)

https://codereview.chromium.org/2052723002/diff/40001/remoting/client/audio_p...
File remoting/client/audio_player.h (right):

https://codereview.chromium.org/2052723002/diff/40001/remoting/client/audio_p...
remoting/client/audio_player.h:50: void
AddAudioPacket(std::unique_ptr<AudioPacket> packet) override;
On 2016/06/09 20:23:45, Sergey Ulanov wrote:
> Add comment:
>  // AudioConsumer implementation.
> 
> also please move this to the public section

Done.

https://codereview.chromium.org/2052723002/diff/40001/remoting/client/chromot...
File remoting/client/chromoting_client.h (right):

https://codereview.chromium.org/2052723002/diff/40001/remoting/client/chromot...
remoting/client/chromoting_client.h:55: AudioConsumer* audio_consumer);
On 2016/06/09 20:23:45, Sergey Ulanov wrote:
> pass ownership of the consumer here?

It does not work for how I plan to use this for iOS is the problem. I need
something to hold on to the implementation of the audio player, not just to the
audio consumer interface. I was having a lot of issues with ownership of a
pointer pointing to a pure virtual class until I realized I need to have the
object held higher in the object stack.

[Sergey Ulanov, 2016-06-09 21:11:44]

https://codereview.chromium.org/2052723002/diff/1/remoting/client/audio_player.h
File remoting/client/audio_player.h (right):

https://codereview.chromium.org/2052723002/diff/1/remoting/client/audio_playe...
remoting/client/audio_player.h:55: typedef std::list<AudioPacket*>
AudioPacketQueue;
BTW, this can be changed to std::list<std::unique_ptr<AudioPacket>>.

https://codereview.chromium.org/2052723002/diff/80001/remoting/client/audio_c...
File remoting/client/audio_consumer.h (right):

https://codereview.chromium.org/2052723002/diff/80001/remoting/client/audio_c...
remoting/client/audio_consumer.h:16: virtual ~AudioConsumer();
Add {} here:
 virtual ~AudioConsumer() {}

Otherwise the destructor remains undefined, but ~AudioPlayer() needs to call it.

[nicholss, 2016-06-10 16:57:57]

https://codereview.chromium.org/2052723002/diff/1/remoting/client/audio_player.h
File remoting/client/audio_player.h (right):

https://codereview.chromium.org/2052723002/diff/1/remoting/client/audio_playe...
remoting/client/audio_player.h:55: typedef std::list<AudioPacket*>
AudioPacketQueue;
On 2016/06/09 21:11:44, Sergey Ulanov wrote:
> BTW, this can be changed to std::list<std::unique_ptr<AudioPacket>>.

Done.

But, in usage of the queue, do I need to change how I give audio packets to the
list? it is used like this:

queued_packets_.push_back(packet.release());

change to 

queued_packets_.push_back(std::move(packet));

correct?

And then would I release the packet before calling delete on front?

--> delete queued_packets_.front();

changed to, 
queued_packets_.front().release();
delete queued_packets_.front();

Please confirm.

[Sergey Ulanov, 2016-06-14 17:51:09]

https://codereview.chromium.org/2052723002/diff/1/remoting/client/audio_player.h
File remoting/client/audio_player.h (right):

https://codereview.chromium.org/2052723002/diff/1/remoting/client/audio_playe...
remoting/client/audio_player.h:55: typedef std::list<AudioPacket*>
AudioPacketQueue;
On 2016/06/10 16:57:57, nicholss wrote:
> On 2016/06/09 21:11:44, Sergey Ulanov wrote:
> > BTW, this can be changed to std::list<std::unique_ptr<AudioPacket>>.
> 
> Done.
> 
> But, in usage of the queue, do I need to change how I give audio packets to
the
> list? it is used like this:
> 
> queued_packets_.push_back(packet.release());
> 
> change to 
> 
> queued_packets_.push_back(std::move(packet));
> 
> correct?

Yes.

> 
> And then would I release the packet before calling delete on front?
> 
> --> delete queued_packets_.front();
> 
> changed to, 
> queued_packets_.front().release();
> delete queued_packets_.front();

You should never need to delete the packets explicitly when using unique_ptr<>.
To remove first packet from the queue just call queued_packets_.pop_front();

And then STLDeleteElements(&queued_packets_); will need to be replaced with
queued_packets_.clear(). Also ResetQueue() can be removed from the destructor.


> 
> Please confirm.

[nicholss, 2016-06-15 21:07:26]

PTAL. Thanks!!

https://codereview.chromium.org/2052723002/diff/80001/remoting/client/audio_c...
File remoting/client/audio_consumer.h (right):

https://codereview.chromium.org/2052723002/diff/80001/remoting/client/audio_c...
remoting/client/audio_consumer.h:16: virtual ~AudioConsumer();
On 2016/06/09 21:11:44, Sergey Ulanov wrote:
> Add {} here:
>  virtual ~AudioConsumer() {}
> 
> Otherwise the destructor remains undefined, but ~AudioPlayer() needs to call
it.

Done.

[Lambros, 2016-06-15 22:13:50]

This basically looks fine - my only concern is the ownership change. I'd be
happier if the AudioDecodeScheduler::Core class weren't ref-counted, then it
would be much easier to prove that the AudioPlayer outlives the Core object.

https://codereview.chromium.org/2052723002/diff/160001/remoting/client/audio_...
File remoting/client/audio_consumer.h (right):

https://codereview.chromium.org/2052723002/diff/160001/remoting/client/audio_...
remoting/client/audio_consumer.h:21: AudioConsumer() {};
As this is an interface with pure virtual methods, I don't think you need to
declare a ctor? Try removing it - I think everything will still compile and
link.

https://codereview.chromium.org/2052723002/diff/160001/remoting/client/audio_...
File remoting/client/audio_decode_scheduler.cc (right):

https://codereview.chromium.org/2052723002/diff/160001/remoting/client/audio_...
remoting/client/audio_decode_scheduler.cc:48: AudioConsumer* audio_consumer_;
I still have concerns about removing ownership of the audio_player here.
You have to ensure that the AudioConsumer out-lives this object. But that's
difficult to guarantee because this Core object is ref-counted. You have to
reason about who might hold onto a ref on this Core, before you know when it's
safe to delete AudioConsumer.
Ideally, we should remove ref-counting and use WeakPtr-invalidation instead to
manage object lifetime, but that's beyond the scope of this CL.

[nicholss, 2016-06-15 22:42:48]

https://codereview.chromium.org/2052723002/diff/160001/remoting/client/audio_...
File remoting/client/audio_consumer.h (right):

https://codereview.chromium.org/2052723002/diff/160001/remoting/client/audio_...
remoting/client/audio_consumer.h:21: AudioConsumer() {};
On 2016/06/15 22:13:49, Lambros wrote:
> As this is an interface with pure virtual methods, I don't think you need to
> declare a ctor? Try removing it - I think everything will still compile and
> link.

I removed it and got this:

../../remoting/client/audio_player.cc:18:14: error: constructor for
'remoting::AudioPlayer' must explicitly initialize the base class
'remoting::AudioConsumer' which does not have a default constructor
AudioPlayer::AudioPlayer()
             ^
../../remoting/client/audio_consumer.h:14:7: note: 'remoting::AudioConsumer'
declared here
class AudioConsumer {
      ^

https://codereview.chromium.org/2052723002/diff/160001/remoting/client/audio_...
File remoting/client/audio_decode_scheduler.cc (right):

https://codereview.chromium.org/2052723002/diff/160001/remoting/client/audio_...
remoting/client/audio_decode_scheduler.cc:48: AudioConsumer* audio_consumer_;
On 2016/06/15 22:13:49, Lambros wrote:
> I still have concerns about removing ownership of the audio_player here.
> You have to ensure that the AudioConsumer out-lives this object. But that's
> difficult to guarantee because this Core object is ref-counted. You have to
> reason about who might hold onto a ref on this Core, before you know when it's
> safe to delete AudioConsumer.
> Ideally, we should remove ref-counting and use WeakPtr-invalidation instead to
> manage object lifetime, but that's beyond the scope of this CL.

Core is never exposed outside of the class. I am not sure how it would ever have
more than one ref. I did not make it this way, it seems like it could just be a
unique pointer rather than ref counted and do the same thing. This pattern is
used other places and I assume someone copied one of the others to make this
class. 

Should I try to make it a unique pointer for this cl?

[Lambros, 2016-06-15 23:45:36]

On 2016/06/15 22:42:48, nicholss wrote:
>
https://codereview.chromium.org/2052723002/diff/160001/remoting/client/audio_...
> File remoting/client/audio_consumer.h (right):
> 
>
https://codereview.chromium.org/2052723002/diff/160001/remoting/client/audio_...
> remoting/client/audio_consumer.h:21: AudioConsumer() {};
> On 2016/06/15 22:13:49, Lambros wrote:
> > As this is an interface with pure virtual methods, I don't think you need to
> > declare a ctor? Try removing it - I think everything will still compile and
> > link.
> 
> I removed it and got this:
> 
> ../../remoting/client/audio_player.cc:18:14: error: constructor for
> 'remoting::AudioPlayer' must explicitly initialize the base class
> 'remoting::AudioConsumer' which does not have a default constructor
> AudioPlayer::AudioPlayer()
>              ^
> ../../remoting/client/audio_consumer.h:14:7: note: 'remoting::AudioConsumer'
> declared here
> class AudioConsumer {
>       ^
> 
>
https://codereview.chromium.org/2052723002/diff/160001/remoting/client/audio_...
> File remoting/client/audio_decode_scheduler.cc (right):
> 
>
https://codereview.chromium.org/2052723002/diff/160001/remoting/client/audio_...
> remoting/client/audio_decode_scheduler.cc:48: AudioConsumer* audio_consumer_;
> On 2016/06/15 22:13:49, Lambros wrote:
> > I still have concerns about removing ownership of the audio_player here.
> > You have to ensure that the AudioConsumer out-lives this object. But that's
> > difficult to guarantee because this Core object is ref-counted. You have to
> > reason about who might hold onto a ref on this Core, before you know when
it's
> > safe to delete AudioConsumer.
> > Ideally, we should remove ref-counting and use WeakPtr-invalidation instead
to
> > manage object lifetime, but that's beyond the scope of this CL.
> 
> Core is never exposed outside of the class. I am not sure how it would ever
have
> more than one ref. I did not make it this way, it seems like it could just be
a
> unique pointer rather than ref counted and do the same thing. This pattern is
> used other places and I assume someone copied one of the others to make this
> class. 
> 
> Should I try to make it a unique pointer for this cl?

The problem is, with each PostTask call, the posted task adds 1 to Core's
ref-count:
https://cs.chromium.org/chromium/src/remoting/client/audio_decode_scheduler.c...

Tasks are posted to the audio-decoding thread and the main thread. That's
probably
why we made the Core ref-counted, because it is ref'ed by tasks running on
multiple
threads.

I think you can make this safe, by null-ing the Core's ref to audio_consumer_
when you destroy the AudioScheduler. There are two tasks on Core that are
posted:
* Core::ProcessDecodedPacket on the main thread
* Core::DecodePacket on the audio-decode thread
Of these 2, only ProcessDecodedPacket() accesses |audio_consumer_| and that
happens
on the main thread.
So I think if you set Core::audio_consumer_ to null during
~AudioDecodeScheduler(),
we are safe (as long as the AudioConsumer out-lives AudioDecodeScheduler which I
think you've already documented?).

Looking at the code, I see the dtor calls Core::Detach(). So all you have to do
is add "audio_consumer_ = null;" to Core::Detach(). And maybe add a comment
somewhere to say that audio_consumer_ is accessed on the main thread?

[nicholss, 2016-06-16 21:18:02]

Hello Lambros, I have added the null'ing out in detach as requested and I am
attempting to write a unit test to verify this works. Could you please take a
look at the test and let me know if it would even test this case?

I am hoping that by resetting the pointer right away that the program will not
have a chance to process the packet on the other thread, I know this is a bit of
a race condition (but so is the issue I am attempting to test for I suppose) 

If there is a better (or real way) to test this please let me know. (or come
over and chat) thanks very much!

[Lambros, 2016-06-17 02:21:59]

On 2016/06/16 21:18:02, nicholss wrote:
> Hello Lambros, I have added the null'ing out in detach as requested and I am
> attempting to write a unit test to verify this works. Could you please take a
> look at the test and let me know if it would even test this case?
> 
> I am hoping that by resetting the pointer right away that the program will not
> have a chance to process the packet on the other thread, I know this is a bit
of
> a race condition (but so is the issue I am attempting to test for I suppose) 
> 
> If there is a better (or real way) to test this please let me know. (or come
> over and chat) thanks very much!

Note that AudioDecodeScheduler::audio_consumer_ is only accessed and used on
the main thread, so it should be possible to test this in a non-racy way.
It is true that the audio packet is processed on another thread, but that
thread then ping-pongs back to the main thread before calling
AudioConsumer::AddAudioPacket(). So you are right that you can immediately
reset the pointer. You will not see any effects on the main thread, until
you start pumping the message-loop with something like
run_loop_.Run().

I think you might need to somehow "join" the decode thread (wait for it to
run and quit). I'm not sure how you do this.
Once the decode thread has done its thing, it will PostTask()
back to the main thread, and so you are guaranteed that the task is sitting
on the main thread after you've waited for the decode thread to finish.
Then you call run_loop_.RunUntilIdle() to run any pending tasks on the main
thread. Then you can EXPECT_EQ(..) on how many times
AddAudioPacket() got called on your mock AudioConsumer.

One test will keep the AudioDecodeScheduler alive the whole time you're
waiting for tasks to stop running. Then verify the added-count is 1.

Another test will create the AudioDecodeScheduler, process the audio packet,
then immediately delete the scheduler. *Then* wait for the loop to go idle.
This time, you can verify that the added-count is 0.

Also, remove the line you added in Core::Detach(): audio_consumer_ = nullptr;
and verify this second test then fails.

I'm just not sure of the details of how you wait for tasks to stop
ping-ponging between two threads. You can call RunLoop::RunUntilIdle() but
I think that only works for the main thread. In this case, there is only
one ping-pong, so you can wait for the decode thread to finish first, before
running any pending tasks on the main thread.

[Lambros, 2016-06-17 18:32:29]

Just a few nits from patch #11. I'll take a look at the latest patch today.

https://codereview.chromium.org/2052723002/diff/160001/remoting/client/audio_...
File remoting/client/audio_consumer.h (right):

https://codereview.chromium.org/2052723002/diff/160001/remoting/client/audio_...
remoting/client/audio_consumer.h:21: AudioConsumer() {};
On 2016/06/15 22:42:47, nicholss wrote:
> On 2016/06/15 22:13:49, Lambros wrote:
> > As this is an interface with pure virtual methods, I don't think you need to
> > declare a ctor? Try removing it - I think everything will still compile and
> > link.
> 
> I removed it and got this:
> 
> ../../remoting/client/audio_player.cc:18:14: error: constructor for
> 'remoting::AudioPlayer' must explicitly initialize the base class
> 'remoting::AudioConsumer' which does not have a default constructor
> AudioPlayer::AudioPlayer()
>              ^
> ../../remoting/client/audio_consumer.h:14:7: note: 'remoting::AudioConsumer'
> declared here
> class AudioConsumer {
>       ^

Guess you need to keep it then? :(

https://codereview.chromium.org/2052723002/diff/200001/remoting/client/audio_...
File remoting/client/audio_decode_scheduler.cc (right):

https://codereview.chromium.org/2052723002/diff/200001/remoting/client/audio_...
remoting/client/audio_decode_scheduler.cc:80: audio_consumer_ = NULL;
nullptr

https://codereview.chromium.org/2052723002/diff/200001/remoting/client/audio_...
File remoting/client/audio_decode_scheduler_unittest.cc (right):

https://codereview.chromium.org/2052723002/diff/200001/remoting/client/audio_...
remoting/client/audio_decode_scheduler_unittest.cc:1: // Copyright (c) 2016 The
Chromium Authors. All rights reserved.
No (c).

https://codereview.chromium.org/2052723002/diff/200001/remoting/client/audio_...
remoting/client/audio_decode_scheduler_unittest.cc:25: class
AudioDecodeSchedulerTest : public ::testing::Test {
I'm a bit puzzled by these tests, since you don't have any ASSERT_x or EXPECT_x
calls anywhere?

https://codereview.chromium.org/2052723002/diff/200001/remoting/client/audio_...
remoting/client/audio_decode_scheduler_unittest.cc:56: // TODO:: could share the
following in a common class for use other places.
Remove TODO or use correct formatting: TODO(username): xxx

https://codereview.chromium.org/2052723002/diff/200001/remoting/client/audio_...
remoting/client/audio_decode_scheduler_unittest.cc:84: void Done() {}
Remove (see below).

https://codereview.chromium.org/2052723002/diff/200001/remoting/client/audio_...
remoting/client/audio_decode_scheduler_unittest.cc:91:
audio_decode_task_runner_, audio_consumer.get()));
Looks like wrong indentation. Please run git cl format.

https://codereview.chromium.org/2052723002/diff/200001/remoting/client/audio_...
remoting/client/audio_decode_scheduler_unittest.cc:96: base::Bind(&Done));
base::Bind(&base::DoNothing)

[nicholss, 2016-06-17 22:35:00]

https://codereview.chromium.org/2052723002/diff/200001/remoting/client/audio_...
File remoting/client/audio_decode_scheduler_unittest.cc (right):

https://codereview.chromium.org/2052723002/diff/200001/remoting/client/audio_...
remoting/client/audio_decode_scheduler_unittest.cc:1: // Copyright (c) 2016 The
Chromium Authors. All rights reserved.
On 2016/06/17 18:32:29, Lambros wrote:
> No (c).

Done.

https://codereview.chromium.org/2052723002/diff/200001/remoting/client/audio_...
remoting/client/audio_decode_scheduler_unittest.cc:56: // TODO:: could share the
following in a common class for use other places.
On 2016/06/17 18:32:29, Lambros wrote:
> Remove TODO or use correct formatting: TODO(username): xxx

Done.

https://codereview.chromium.org/2052723002/diff/200001/remoting/client/audio_...
remoting/client/audio_decode_scheduler_unittest.cc:84: void Done() {}
On 2016/06/17 18:32:29, Lambros wrote:
> Remove (see below).

Done.

https://codereview.chromium.org/2052723002/diff/200001/remoting/client/audio_...
remoting/client/audio_decode_scheduler_unittest.cc:91:
audio_decode_task_runner_, audio_consumer.get()));
On 2016/06/17 18:32:29, Lambros wrote:
> Looks like wrong indentation. Please run git cl format.

Done.

https://codereview.chromium.org/2052723002/diff/200001/remoting/client/audio_...
remoting/client/audio_decode_scheduler_unittest.cc:96: base::Bind(&Done));
On 2016/06/17 18:32:29, Lambros wrote:
> base::Bind(&base::DoNothing)

Done.

[Lambros, 2016-06-17 23:51:10]

lgtm when comments are addressed.

https://codereview.chromium.org/2052723002/diff/220001/remoting/client/audio_...
File remoting/client/audio_consumer.h (right):

https://codereview.chromium.org/2052723002/diff/220001/remoting/client/audio_...
remoting/client/audio_consumer.h:16: virtual ~AudioConsumer(){};
I think we normally have space between ) and {. Check with
git cl format

https://codereview.chromium.org/2052723002/diff/220001/remoting/client/audio_...
File remoting/client/audio_decode_scheduler.cc (right):

https://codereview.chromium.org/2052723002/diff/220001/remoting/client/audio_...
remoting/client/audio_decode_scheduler.cc:81: // TODO(nicholss) No work to do
anymore. Remove this?
You can remove this. Now that you take in a WeakPtr,
it becomes the caller's responsibility to invalidate
the ptr. So please remove this method in this CL.

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/BUILD.gn
File remoting/client/BUILD.gn (right):

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/BUILD....
remoting/client/BUILD.gn:44: source_set("test_support") {
Why does this need to be a separate target? Couldn't you add these files to the
"unit_tests" sources list?

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/audio_...
File remoting/client/audio_consumer.h (right):

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/audio_...
remoting/client/audio_consumer.h:16: virtual ~AudioConsumer(){};
Space between ) and {

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/audio_...
File remoting/client/audio_decode_scheduler.cc (right):

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/audio_...
remoting/client/audio_decode_scheduler.cc:31: void Detach();
Remove.

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/audio_...
remoting/client/audio_decode_scheduler.cc:81: // TODO(nicholss) No work to do
anymore. Remove this?
Yep, remove! :)

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/audio_...
remoting/client/audio_decode_scheduler.cc:116: core_->Detach();
Remove.

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/audio_...
File remoting/client/audio_decode_scheduler_unittest.cc (right):

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/audio_...
remoting/client/audio_decode_scheduler_unittest.cc:11: #include
"remoting/client/audio_decode_scheduler.h"
Move this line above the other headers, with blank line separating. See
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/audio_...
remoting/client/audio_decode_scheduler_unittest.cc:25: class
AudioDecodeSchedulerTest : public ::testing::Test {
I don't see any EXPECTations anywhere, so very little is actually being tested
(you are only checking that the code doesn't actually crash).

You can follow up with another CL if it's a significant chunk of work to add
expectations (for example, if you need to write some mock classes).

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/audio_...
remoting/client/audio_decode_scheduler_unittest.cc:56: // TODO(nicholss) could
share the following in a common class for use
Colon after TODO:
TODO(nicholss): Blah ...
Begin sentence with capital letter, end with period.
s/other/in other/ (or ".. for use elsewhere.")

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/fake_a...
File remoting/client/fake_audio_consumer.cc (right):

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/fake_a...
remoting/client/fake_audio_consumer.cc:10:
FakeAudioConsumer::FakeAudioConsumer() : weak_factory_(this) {}
You should maybe name this MockAudioConsumer, as it behaves like a mock. The
only implementation you need is to record how many times the method gets called.

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/fake_a...
remoting/client/fake_audio_consumer.cc:14: void
FakeAudioConsumer::AddAudioPacket(std::unique_ptr<AudioPacket> packet) {}
You could increment a counter here, which would let you set expectations in
tests.

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/fake_a...
File remoting/client/fake_audio_consumer.h (right):

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/fake_a...
remoting/client/fake_audio_consumer.h:8: #include <stddef.h>
remove?

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/fake_a...
remoting/client/fake_audio_consumer.h:9: #include <stdint.h>
remove?

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/fake_a...
remoting/client/fake_audio_consumer.h:11: #include <list>
remove?

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/fake_a...
remoting/client/fake_audio_consumer.h:17: #include "remoting/proto/audio.pb.h"
remove?
AudioPacket is already part of the audio_consumer.h interface which we inherit
from, so you shouldn't even need to forward-declare AudioPacket here.

[Sergey Ulanov, 2016-06-18 00:43:38]

Some style nits. Also please update CL description to add more details about
what you are doing here. See
https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list...

https://codereview.chromium.org/2052723002/diff/260001/remoting/client/audio_...
File remoting/client/audio_consumer.h (right):

https://codereview.chromium.org/2052723002/diff/260001/remoting/client/audio_...
remoting/client/audio_consumer.h:14: class AudioConsumer {
Please add a comment here to explain how this interface is used.

https://codereview.chromium.org/2052723002/diff/260001/remoting/client/audio_...
remoting/client/audio_consumer.h:16: virtual ~AudioConsumer(){};
don't need ;
Then run git cl format - it will add space between ) and {

https://codereview.chromium.org/2052723002/diff/260001/remoting/client/audio_...
remoting/client/audio_consumer.h:16: virtual ~AudioConsumer(){};
make the destructor protected - it's not part of the interface as you don't pass
AudioConsumer ownership anywhere.

https://codereview.chromium.org/2052723002/diff/260001/remoting/client/audio_...
File remoting/client/audio_decode_scheduler.cc (right):

https://codereview.chromium.org/2052723002/diff/260001/remoting/client/audio_...
remoting/client/audio_decode_scheduler.cc:81: // TODO(nicholss) No work to do
anymore. Remove this?
Yes please.

https://codereview.chromium.org/2052723002/diff/260001/remoting/client/audio_...
File remoting/client/audio_decode_scheduler.h (right):

https://codereview.chromium.org/2052723002/diff/260001/remoting/client/audio_...
remoting/client/audio_decode_scheduler.h:27: class AudioConsumer;
Order these alphabetically

https://codereview.chromium.org/2052723002/diff/260001/remoting/client/audio_...
File remoting/client/audio_player.cc (right):

https://codereview.chromium.org/2052723002/diff/260001/remoting/client/audio_...
remoting/client/audio_player.cc:26: }
Please run "git cl format"

https://codereview.chromium.org/2052723002/diff/260001/remoting/client/audio_...
File remoting/client/audio_player.h (right):

https://codereview.chromium.org/2052723002/diff/260001/remoting/client/audio_...
remoting/client/audio_player.h:30: void
ProcessAudioPacket(std::unique_ptr<AudioPacket> packet);
Why do we need to both ProcessAudioPacket() and AddAudioPacket()? I think you
can just remove ProcessAudioPacket().

https://codereview.chromium.org/2052723002/diff/260001/remoting/client/fake_a...
File remoting/client/fake_audio_consumer.cc (right):

https://codereview.chromium.org/2052723002/diff/260001/remoting/client/fake_a...
remoting/client/fake_audio_consumer.cc:6: #include "remoting/proto/audio.pb.h"
empty line here please

[nicholss, 2016-06-20 17:39:38]

Description was changed from

==========
Adding an interface to allow extention of the audio player for CRD and iOS.

BUG=611181

R=lambroslambrou@chromium.org
==========

to

==========
Adding an interface to allow extension of the audio player for CRD and iOS.

This CL changes the audio player for CRD into a consumer of audio packets. The
choice of consumer is based on a consumer/provider pattern, the intent is to
create a new class that will be used in iOS in a following CL that acts as a
middle buffer between the CRD Client and the iOS audio player which will extend
both AudioConsumer and AudioProvider(not in this CL). Because of this change, I
needed the current usage of audio player to extend from a more generic interface
(AudioConsumer) which could apply to either this future buffer or audio player. 

BUG=611181

R=lambroslambrou@chromium.org
==========

[nicholss, 2016-06-20 17:47:52]

Addressed Lambros' and Sergey's comments. Thank you for the reviews. I would
like to commit this and then make a second followup CL to beef up the unit test
I added that does not do much at the moment other than prove it can run without
crashing right away...

I did test this for the native client and no issues for multi-reconnect and new
connections.

https://codereview.chromium.org/2052723002/diff/220001/remoting/client/audio_...
File remoting/client/audio_decode_scheduler.cc (right):

https://codereview.chromium.org/2052723002/diff/220001/remoting/client/audio_...
remoting/client/audio_decode_scheduler.cc:81: // TODO(nicholss) No work to do
anymore. Remove this?
On 2016/06/17 23:51:09, Lambros wrote:
> You can remove this. Now that you take in a WeakPtr,
> it becomes the caller's responsibility to invalidate
> the ptr. So please remove this method in this CL.

Done.

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/BUILD.gn
File remoting/client/BUILD.gn (right):

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/BUILD....
remoting/client/BUILD.gn:44: source_set("test_support") {
On 2016/06/17 23:51:09, Lambros wrote:
> Why does this need to be a separate target? Couldn't you add these files to
the
> "unit_tests" sources list?

Just following the patterns seen elsewhere. The unit_tests target seems to only
have unit test files so I thought it would be wrong to add a non testing file to
the list. Elsewhere in the codebase this is how it is done from what I saw.

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/audio_...
File remoting/client/audio_decode_scheduler.cc (right):

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/audio_...
remoting/client/audio_decode_scheduler.cc:31: void Detach();
On 2016/06/17 23:51:09, Lambros wrote:
> Remove.

Done.

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/audio_...
remoting/client/audio_decode_scheduler.cc:81: // TODO(nicholss) No work to do
anymore. Remove this?
On 2016/06/17 23:51:09, Lambros wrote:
> Yep, remove! :)

Done.

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/audio_...
remoting/client/audio_decode_scheduler.cc:116: core_->Detach();
On 2016/06/17 23:51:09, Lambros wrote:
> Remove.

Done.

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/audio_...
File remoting/client/audio_decode_scheduler_unittest.cc (right):

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/audio_...
remoting/client/audio_decode_scheduler_unittest.cc:56: // TODO(nicholss) could
share the following in a common class for use
On 2016/06/17 23:51:10, Lambros wrote:
> Colon after TODO:
> TODO(nicholss): Blah ...
> Begin sentence with capital letter, end with period.
> s/other/in other/ (or ".. for use elsewhere.")

Thanks, sorry... I am use to the style of TODO:: Blah and I am over correcting
by removing both the :: :) fixed.

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/fake_a...
File remoting/client/fake_audio_consumer.cc (right):

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/fake_a...
remoting/client/fake_audio_consumer.cc:14: void
FakeAudioConsumer::AddAudioPacket(std::unique_ptr<AudioPacket> packet) {}
On 2016/06/17 23:51:10, Lambros wrote:
> You could increment a counter here, which would let you set expectations in
> tests.

Ok sounds good, I will give this a try and see if I can make the test ... test
something :)

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/fake_a...
File remoting/client/fake_audio_consumer.h (right):

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/fake_a...
remoting/client/fake_audio_consumer.h:8: #include <stddef.h>
On 2016/06/17 23:51:10, Lambros wrote:
> remove?

Done.

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/fake_a...
remoting/client/fake_audio_consumer.h:9: #include <stdint.h>
On 2016/06/17 23:51:10, Lambros wrote:
> remove?

Done.

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/fake_a...
remoting/client/fake_audio_consumer.h:11: #include <list>
On 2016/06/17 23:51:10, Lambros wrote:
> remove?

Done.

https://codereview.chromium.org/2052723002/diff/240001/remoting/client/fake_a...
remoting/client/fake_audio_consumer.h:17: #include "remoting/proto/audio.pb.h"
On 2016/06/17 23:51:10, Lambros wrote:
> remove?
> AudioPacket is already part of the audio_consumer.h interface which we inherit
> from, so you shouldn't even need to forward-declare AudioPacket here.

Done.

https://codereview.chromium.org/2052723002/diff/260001/remoting/client/audio_...
File remoting/client/audio_consumer.h (right):

https://codereview.chromium.org/2052723002/diff/260001/remoting/client/audio_...
remoting/client/audio_consumer.h:16: virtual ~AudioConsumer(){};
On 2016/06/18 00:43:38, Sergey Ulanov wrote:
> don't need ;
> Then run git cl format - it will add space between ) and {

I think the mac formatter is not as good as the linux formatter. I had already
run the formatter as is, and without the ; and it remained as (){} so I will
just have to remember this style rule.

https://codereview.chromium.org/2052723002/diff/260001/remoting/client/audio_...
File remoting/client/audio_decode_scheduler.cc (right):

https://codereview.chromium.org/2052723002/diff/260001/remoting/client/audio_...
remoting/client/audio_decode_scheduler.cc:81: // TODO(nicholss) No work to do
anymore. Remove this?
On 2016/06/18 00:43:38, Sergey Ulanov wrote:
> Yes please.

Done.

https://codereview.chromium.org/2052723002/diff/260001/remoting/client/audio_...
File remoting/client/audio_decode_scheduler.h (right):

https://codereview.chromium.org/2052723002/diff/260001/remoting/client/audio_...
remoting/client/audio_decode_scheduler.h:27: class AudioConsumer;
On 2016/06/18 00:43:38, Sergey Ulanov wrote:
> Order these alphabetically

Done.

https://codereview.chromium.org/2052723002/diff/260001/remoting/client/audio_...
File remoting/client/audio_player.cc (right):

https://codereview.chromium.org/2052723002/diff/260001/remoting/client/audio_...
remoting/client/audio_player.cc:26: }
On 2016/06/18 00:43:38, Sergey Ulanov wrote:
> Please run "git cl format"

I have many times. it did not change this...

https://codereview.chromium.org/2052723002/diff/260001/remoting/client/audio_...
File remoting/client/audio_player.h (right):

https://codereview.chromium.org/2052723002/diff/260001/remoting/client/audio_...
remoting/client/audio_player.h:30: void
ProcessAudioPacket(std::unique_ptr<AudioPacket> packet);
On 2016/06/18 00:43:38, Sergey Ulanov wrote:
> Why do we need to both ProcessAudioPacket() and AddAudioPacket()? I think you
> can just remove ProcessAudioPacket().

Process is the deprecated way for the audio player, I choose the Add* type
method to be inline with the Consumer/Producer pattern. Let me just remove it. I
was attempting to move to the new one without breaking interfaces but I can just
refactor the existing usages... Thanks for reminding me.

https://codereview.chromium.org/2052723002/diff/260001/remoting/client/fake_a...
File remoting/client/fake_audio_consumer.cc (right):

https://codereview.chromium.org/2052723002/diff/260001/remoting/client/fake_a...
remoting/client/fake_audio_consumer.cc:6: #include "remoting/proto/audio.pb.h"
On 2016/06/18 00:43:38, Sergey Ulanov wrote:
> empty line here please

Done.

[nicholss, 2016-06-20 17:49:02]

Description was changed from

==========
Adding an interface to allow extension of the audio player for CRD and iOS.

This CL changes the audio player for CRD into a consumer of audio packets. The
choice of consumer is based on a consumer/provider pattern, the intent is to
create a new class that will be used in iOS in a following CL that acts as a
middle buffer between the CRD Client and the iOS audio player which will extend
both AudioConsumer and AudioProvider(not in this CL). Because of this change, I
needed the current usage of audio player to extend from a more generic interface
(AudioConsumer) which could apply to either this future buffer or audio player. 

BUG=611181

R=lambroslambrou@chromium.org
==========

to

==========
Adding an interface to allow extension of the audio player for CRD and iOS.

This CL changes the audio player for CRD into a consumer of audio packets. The
choice of consumer is based on a consumer/provider pattern, the intent is to
create a new class that will be used in iOS in a following CL that acts as a
middle buffer between the CRD Client and the iOS audio player which will extend
both AudioConsumer and AudioProvider(not in this CL). Because of this change, I
needed the current usage of audio player to extend from a more generic interface
(AudioConsumer) which could apply to either this future buffer or audio player. 

BUG=611181

TEST=Manual tested webapp client.

R=lambroslambrou@chromium.org
==========

[nicholss, 2016-06-20 20:55:19]

The CQ bit was checked by nicholss@chromium.org

[commit-bot: I haz the power, 2016-06-20 20:55:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2052723002/260001

[commit-bot: I haz the power, 2016-06-20 21:03:57]

Description was changed from

==========
Adding an interface to allow extension of the audio player for CRD and iOS.

This CL changes the audio player for CRD into a consumer of audio packets. The
choice of consumer is based on a consumer/provider pattern, the intent is to
create a new class that will be used in iOS in a following CL that acts as a
middle buffer between the CRD Client and the iOS audio player which will extend
both AudioConsumer and AudioProvider(not in this CL). Because of this change, I
needed the current usage of audio player to extend from a more generic interface
(AudioConsumer) which could apply to either this future buffer or audio player. 

BUG=611181

TEST=Manual tested webapp client.

R=lambroslambrou@chromium.org
==========

to

==========
Adding an interface to allow extension of the audio player for CRD and iOS.

This CL changes the audio player for CRD into a consumer of audio packets. The
choice of consumer is based on a consumer/provider pattern, the intent is to
create a new class that will be used in iOS in a following CL that acts as a
middle buffer between the CRD Client and the iOS audio player which will extend
both AudioConsumer and AudioProvider(not in this CL). Because of this change, I
needed the current usage of audio player to extend from a more generic interface
(AudioConsumer) which could apply to either this future buffer or audio player. 

BUG=611181

TEST=Manual tested webapp client.

R=lambroslambrou@chromium.org
==========

[commit-bot: I haz the power, 2016-06-20 21:03:58]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-06-20 21:11:08]

Description was changed from

==========
Adding an interface to allow extension of the audio player for CRD and iOS.

This CL changes the audio player for CRD into a consumer of audio packets. The
choice of consumer is based on a consumer/provider pattern, the intent is to
create a new class that will be used in iOS in a following CL that acts as a
middle buffer between the CRD Client and the iOS audio player which will extend
both AudioConsumer and AudioProvider(not in this CL). Because of this change, I
needed the current usage of audio player to extend from a more generic interface
(AudioConsumer) which could apply to either this future buffer or audio player. 

BUG=611181

TEST=Manual tested webapp client.

R=lambroslambrou@chromium.org
==========

to

==========
Adding an interface to allow extension of the audio player for CRD and iOS.

This CL changes the audio player for CRD into a consumer of audio packets. The
choice of consumer is based on a consumer/provider pattern, the intent is to
create a new class that will be used in iOS in a following CL that acts as a
middle buffer between the CRD Client and the iOS audio player which will extend
both AudioConsumer and AudioProvider(not in this CL). Because of this change, I
needed the current usage of audio player to extend from a more generic interface
(AudioConsumer) which could apply to either this future buffer or audio player.

BUG=611181

TEST=Manual tested webapp client.

R=lambroslambrou@chromium.org

Committed: https://crrev.com/eea69600e07c1d402758d8dd03f5579caddde736
Cr-Commit-Position: refs/heads/master@{#400776}
==========

[commit-bot: I haz the power, 2016-06-20 21:11:09]

Patchset 14 (id:??) landed as
https://crrev.com/eea69600e07c1d402758d8dd03f5579caddde736
Cr-Commit-Position: refs/heads/master@{#400776}

[Sergey Ulanov, 2016-06-20 23:15:15]

Looks like the change that was landed didn't have any nits from the last
patch-set addressed. Please fix them in a separate CL.