Added files for audio readers.

Added files for audio readers.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=142497

[kxing, 2012-06-15 15:37:01]

Please let me know if you think we should merge ProtobufAudioReader into
AudioReader.

[Sergey Ulanov, 2012-06-15 17:48:15]

On 2012/06/15 15:37:01, kxing wrote:
> Please let me know if you think we should merge ProtobufAudioReader into
> AudioReader.

Yes please

[kxing, 2012-06-15 18:46:08]

(new patch)

[Sergey Ulanov, 2012-06-15 19:35:23]

some style nits

http://codereview.chromium.org/10562007/diff/6001/remoting/protocol/audio_rea...
File remoting/protocol/audio_reader.h (right):

http://codereview.chromium.org/10562007/diff/6001/remoting/protocol/audio_rea...
remoting/protocol/audio_reader.h:26: static AudioReader* Create(const
SessionConfig& config);
this should be defined after InitializedCallback.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

http://codereview.chromium.org/10562007/diff/6001/remoting/protocol/audio_rea...
remoting/protocol/audio_reader.h:32: AudioReader(AudioPacket::Encoding
encoding);
Constructors with a single argument must be marked as explicit:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Explicit_Const...

http://codereview.chromium.org/10562007/diff/6001/remoting/protocol/audio_rea...
remoting/protocol/audio_reader.h:36: // Initializies the reader. Doesn't take
ownership of either |connection|
nit: remove the second sentence: we now use scoped_ptr<> to pass ownership, so
the comment is redundant.

http://codereview.chromium.org/10562007/diff/6001/remoting/protocol/audio_rea...
remoting/protocol/audio_reader.h:39: AudioStub* audio_stub,
incorrect indentation

[kxing, 2012-06-15 19:46:41]

http://codereview.chromium.org/10562007/diff/6001/remoting/protocol/audio_rea...
File remoting/protocol/audio_reader.h (right):

http://codereview.chromium.org/10562007/diff/6001/remoting/protocol/audio_rea...
remoting/protocol/audio_reader.h:26: static AudioReader* Create(const
SessionConfig& config);
On 2012/06/15 19:35:23, sergeyu wrote:
> this should be defined after InitializedCallback.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

Done.

http://codereview.chromium.org/10562007/diff/6001/remoting/protocol/audio_rea...
remoting/protocol/audio_reader.h:32: AudioReader(AudioPacket::Encoding
encoding);
On 2012/06/15 19:35:23, sergeyu wrote:
> Constructors with a single argument must be marked as explicit:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Explicit_Const...

Done.

http://codereview.chromium.org/10562007/diff/6001/remoting/protocol/audio_rea...
remoting/protocol/audio_reader.h:36: // Initializies the reader. Doesn't take
ownership of either |connection|
On 2012/06/15 19:35:23, sergeyu wrote:
> nit: remove the second sentence: we now use scoped_ptr<> to pass ownership, so
> the comment is redundant.

Done.

http://codereview.chromium.org/10562007/diff/6001/remoting/protocol/audio_rea...
remoting/protocol/audio_reader.h:39: AudioStub* audio_stub,
On 2012/06/15 19:35:23, sergeyu wrote:
> incorrect indentation

Done.

[Sergey Ulanov, 2012-06-15 19:54:41]

http://codereview.chromium.org/10562007/diff/6002/remoting/protocol/audio_rea...
File remoting/protocol/audio_reader.h (right):

http://codereview.chromium.org/10562007/diff/6002/remoting/protocol/audio_rea...
remoting/protocol/audio_reader.h:30: explicit AudioReader(AudioPacket::Encoding
encoding);
Since this is called from Create, this constructor should be private.

http://codereview.chromium.org/10562007/diff/6002/remoting/protocol/audio_rea...
remoting/protocol/audio_reader.h:40: static AudioReader* Create(const
SessionConfig& config);
Move this between the destructor and Init().

http://codereview.chromium.org/10562007/diff/6002/remoting/protocol/audio_rea...
remoting/protocol/audio_reader.h:40: static AudioReader* Create(const
SessionConfig& config);
Since this passes ownership, use scoped_ptr<AudioReader> for the result. I
overlooked this in AudioWriter - it's good idea to fix it too, in a separate CL.

[kxing, 2012-06-15 20:03:06]

http://codereview.chromium.org/10562007/diff/6002/remoting/protocol/audio_rea...
File remoting/protocol/audio_reader.h (right):

http://codereview.chromium.org/10562007/diff/6002/remoting/protocol/audio_rea...
remoting/protocol/audio_reader.h:30: explicit AudioReader(AudioPacket::Encoding
encoding);
On 2012/06/15 19:54:41, sergeyu wrote:
> Since this is called from Create, this constructor should be private.

Done.

http://codereview.chromium.org/10562007/diff/6002/remoting/protocol/audio_rea...
remoting/protocol/audio_reader.h:40: static AudioReader* Create(const
SessionConfig& config);
On 2012/06/15 19:54:41, sergeyu wrote:
> Since this passes ownership, use scoped_ptr<AudioReader> for the result. I
> overlooked this in AudioWriter - it's good idea to fix it too, in a separate
CL.

Done.

http://codereview.chromium.org/10562007/diff/6002/remoting/protocol/audio_rea...
remoting/protocol/audio_reader.h:40: static AudioReader* Create(const
SessionConfig& config);
On 2012/06/15 19:54:41, sergeyu wrote:
> Move this between the destructor and Init().

Done.

[kxing, 2012-06-15 20:06:52]

For the scoped_ptr issue, should we also refactor the VideoReader and
VideoWriter Create() methods as well?

[Sergey Ulanov, 2012-06-15 20:10:33]

LGTM.

On 2012/06/15 20:06:52, kxing wrote:
> For the scoped_ptr issue, should we also refactor the VideoReader and
> VideoWriter Create() methods as well?

Yes, ideally VideoWriter and VideoReader should be fixed too. Please fix them
all together with AudioWriter.

[commit-bot: I haz the power, 2012-06-15 20:17:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kxing@chromium.org/10562007/12001

[kxing, 2012-06-15 20:38:01]

The cleanup CL can be found at http://codereview.chromium.org/10556020/

[Wez, 2012-06-15 20:40:54]

http://codereview.chromium.org/10562007/diff/12001/remoting/protocol/audio_re...
File remoting/protocol/audio_reader.cc (right):

http://codereview.chromium.org/10562007/diff/12001/remoting/protocol/audio_re...
remoting/protocol/audio_reader.cc:30: return scoped_ptr<AudioReader>(new
AudioReader(AudioPacket::ENCODING_RAW));
This limits you to using a single audio encoding per audio channel, and you only
have one audio channel per session, so you won't be able to e.g. handle
notification sounds differently from streaming audio. Is that a problem?

http://codereview.chromium.org/10562007/diff/12001/remoting/protocol/audio_re...
remoting/protocol/audio_reader.cc:55: DCHECK(!channel_.get());
DCHECK() this in Init() as well, to trap mistaken duplicate calls to it.

http://codereview.chromium.org/10562007/diff/12001/remoting/protocol/audio_re...
File remoting/protocol/audio_reader.h (right):

http://codereview.chromium.org/10562007/diff/12001/remoting/protocol/audio_re...
remoting/protocol/audio_reader.h:32: static scoped_ptr<AudioReader> Create(const
SessionConfig& config);
Document this Create() function.

http://codereview.chromium.org/10562007/diff/12001/remoting/protocol/audio_re...
remoting/protocol/audio_reader.h:34: // Initializies the reader.
typo: Initializes

http://codereview.chromium.org/10562007/diff/12001/remoting/protocol/audio_re...
remoting/protocol/audio_reader.h:34: // Initializies the reader.
What does it mean to "initialize the reader"? What does initialization actually
do? Who owns |session| and |audio_stub|, and what guarantees do they need to
provide about the object's lifetimes?

http://codereview.chromium.org/10562007/diff/12001/remoting/protocol/audio_re...
remoting/protocol/audio_reader.h:38: bool is_connected();
nit: const

http://codereview.chromium.org/10562007/diff/12001/remoting/protocol/audio_re...
remoting/protocol/audio_reader.h:44: void OnNewData(scoped_ptr<AudioPacket>
packet,
nit: OnNewData -> OnAudioData or OnAudioPacket

http://codereview.chromium.org/10562007/diff/12001/remoting/protocol/audio_re...
remoting/protocol/audio_reader.h:47: Session* session_;
Why not keep |session_|, |audio_stub_| and |initialized_callback_| together,
since they are the parameters the caller passed in to Init?

[commit-bot: I haz the power, 2012-06-15 22:21:42]

Change committed as 142497