Piping for audio encoding.

Piping for audio encoding.


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

[kxing, 2012-07-30 22:25:04]

Could someone PTAL?

[Sergey Ulanov, 2012-07-31 20:00:17]

I think the encode/decode interfaces can be simpler. Specifically they should be
explicitly synchronous and we don't need the AudioCaptureData class (we need
CaptureData the video case, because video codec needs more information about
screen captures, e.g. dirty rects, etc. that's not necessary for the audio
pipeline).

http://codereview.chromium.org/10836017/diff/3001/remoting/base/audio_capture...
File remoting/base/audio_capture_data.h (right):

http://codereview.chromium.org/10836017/diff/3001/remoting/base/audio_capture...
remoting/base/audio_capture_data.h:15: class AudioCaptureData {
add comments to explain what this is for.

http://codereview.chromium.org/10836017/diff/3001/remoting/base/audio_capture...
remoting/base/audio_capture_data.h:15: class AudioCaptureData {
Do we really need this class at all? Can we just use AudioPacket for both
compressed and uncompressed data - we already have fields to specify encoding
there

http://codereview.chromium.org/10836017/diff/3001/remoting/base/audio_capture...
remoting/base/audio_capture_data.h:21: void set_data(std::string data) {
data_.assign(data); }
use const reference for the argument. Might be better to provide "std::string*
mutable_data()" that returns pointer to data_

http://codereview.chromium.org/10836017/diff/3001/remoting/base/audio_encoder.h
File remoting/base/audio_encoder.h (right):

http://codereview.chromium.org/10836017/diff/3001/remoting/base/audio_encoder...
remoting/base/audio_encoder.h:6: #define REMOTING_BASE_AUDIO_ENCODER_H_
can you please create new directory remoting/codec and put the new codec-related
code there instead of remoting/base. We will move video codecs there later.

http://codereview.chromium.org/10836017/diff/3001/remoting/base/audio_encoder...
remoting/base/audio_encoder.h:24: const PacketEncodedCallback&
packet_captured_callback) = 0;
Don't need this to be asyncronous. This function should just return
scoped_ptr<AudioPacket>, don't need the callback.
Ou video encode it also synchronous, even though the interface make it look like
it is asynchronous.

[kxing, 2012-07-31 22:11:44]

Could someone PTAL again?

http://codereview.chromium.org/10836017/diff/3001/remoting/base/audio_capture...
File remoting/base/audio_capture_data.h (right):

http://codereview.chromium.org/10836017/diff/3001/remoting/base/audio_capture...
remoting/base/audio_capture_data.h:15: class AudioCaptureData {
On 2012/07/31 20:00:18, sergeyu wrote:
> add comments to explain what this is for.

Done.

http://codereview.chromium.org/10836017/diff/3001/remoting/base/audio_capture...
remoting/base/audio_capture_data.h:15: class AudioCaptureData {
On 2012/07/31 20:00:18, sergeyu wrote:
> Do we really need this class at all? Can we just use AudioPacket for both
> compressed and uncompressed data - we already have fields to specify encoding
> there

Done.

http://codereview.chromium.org/10836017/diff/3001/remoting/base/audio_capture...
remoting/base/audio_capture_data.h:21: void set_data(std::string data) {
data_.assign(data); }
On 2012/07/31 20:00:18, sergeyu wrote:
> use const reference for the argument. Might be better to provide "std::string*
> mutable_data()" that returns pointer to data_

Done.

http://codereview.chromium.org/10836017/diff/3001/remoting/base/audio_encoder.h
File remoting/base/audio_encoder.h (right):

http://codereview.chromium.org/10836017/diff/3001/remoting/base/audio_encoder...
remoting/base/audio_encoder.h:6: #define REMOTING_BASE_AUDIO_ENCODER_H_
On 2012/07/31 20:00:18, sergeyu wrote:
> can you please create new directory remoting/codec and put the new
codec-related
> code there instead of remoting/base. We will move video codecs there later.

Done.

http://codereview.chromium.org/10836017/diff/3001/remoting/base/audio_encoder...
remoting/base/audio_encoder.h:24: const PacketEncodedCallback&
packet_captured_callback) = 0;
On 2012/07/31 20:00:18, sergeyu wrote:
> Don't need this to be asyncronous. This function should just return
> scoped_ptr<AudioPacket>, don't need the callback.
> Ou video encode it also synchronous, even though the interface make it look
like
> it is asynchronous.

Done.

[Sergey Ulanov, 2012-07-31 22:22:16]

http://codereview.chromium.org/10836017/diff/8001/remoting/codec/audio_encode...
File remoting/codec/audio_encoder_verbatim.cc (right):

http://codereview.chromium.org/10836017/diff/8001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_verbatim.cc:17: return audio_packet.Pass();
add DCHECK to verify that encoding in audio_packet is set to ENCODING_RAW.

http://codereview.chromium.org/10836017/diff/8001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_verbatim.cc:20: scoped_ptr<AudioEncoder>
AudioEncoderVerbatim::CreateVerbatimAudioEncoder() {
Why do we need this. Can you just make constructor public?

http://codereview.chromium.org/10836017/diff/8001/remoting/host/audio_schedul...
File remoting/host/audio_scheduler.h (right):

http://codereview.chromium.org/10836017/diff/8001/remoting/host/audio_schedul...
remoting/host/audio_scheduler.h:23: class AudioCaptureData;
remove this

http://codereview.chromium.org/10836017/diff/8001/remoting/host/chromoting_ho...
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/10836017/diff/8001/remoting/host/chromoting_ho...
remoting/host/chromoting_host.cc:219: scoped_ptr<AudioEncoder> audio_encoder =
Move this below, inside the if statement where you creatae AudioScheduler.

http://codereview.chromium.org/10836017/diff/8001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

http://codereview.chromium.org/10836017/diff/8001/remoting/remoting.gyp#newco...
remoting/remoting.gyp:1170: # TODO(kxing): Seperate the following into a
separate target.
These and all video codec files.

[kxing, 2012-07-31 23:06:06]

Could someone PTAL again?

http://codereview.chromium.org/10836017/diff/8001/remoting/codec/audio_encode...
File remoting/codec/audio_encoder_verbatim.cc (right):

http://codereview.chromium.org/10836017/diff/8001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_verbatim.cc:17: return audio_packet.Pass();
On 2012/07/31 22:22:17, sergeyu wrote:
> add DCHECK to verify that encoding in audio_packet is set to ENCODING_RAW.

Done.

http://codereview.chromium.org/10836017/diff/8001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_verbatim.cc:20: scoped_ptr<AudioEncoder>
AudioEncoderVerbatim::CreateVerbatimAudioEncoder() {
On 2012/07/31 22:22:17, sergeyu wrote:
> Why do we need this. Can you just make constructor public?

Done.

http://codereview.chromium.org/10836017/diff/8001/remoting/host/audio_schedul...
File remoting/host/audio_scheduler.h (right):

http://codereview.chromium.org/10836017/diff/8001/remoting/host/audio_schedul...
remoting/host/audio_scheduler.h:23: class AudioCaptureData;
On 2012/07/31 22:22:17, sergeyu wrote:
> remove this

Done.

http://codereview.chromium.org/10836017/diff/8001/remoting/host/chromoting_ho...
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/10836017/diff/8001/remoting/host/chromoting_ho...
remoting/host/chromoting_host.cc:219: scoped_ptr<AudioEncoder> audio_encoder =
On 2012/07/31 22:22:17, sergeyu wrote:
> Move this below, inside the if statement where you creatae AudioScheduler.

Done.

http://codereview.chromium.org/10836017/diff/8001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

http://codereview.chromium.org/10836017/diff/8001/remoting/remoting.gyp#newco...
remoting/remoting.gyp:1170: # TODO(kxing): Seperate the following into a
separate target.
On 2012/07/31 22:22:17, sergeyu wrote:
> These and all video codec files.

Done.

[Sergey Ulanov, 2012-07-31 23:41:33]

lgtm

[commit-bot: I haz the power, 2012-08-01 14:29:34]

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

[commit-bot: I haz the power, 2012-08-01 14:29:38]

Failed to apply patch for remoting/host/audio_capturer_win.cc:
While running patch -p1 --forward --force;
patching file remoting/host/audio_capturer_win.cc
Hunk #1 FAILED at 267.
1 out of 1 hunk FAILED -- saving rejects to file
remoting/host/audio_capturer_win.cc.rej

[commit-bot: I haz the power, 2012-08-01 15:38:25]

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

[commit-bot: I haz the power, 2012-08-01 15:52:31]

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

[commit-bot: I haz the power, 2012-08-01 17:41:44]

Change committed as 149441

[Wez, 2012-08-07 18:07:40]

https://chromiumcodereview.appspot.com/10836017/diff/5002/remoting/host/chrom...
File remoting/host/chromoting_host.cc (right):

https://chromiumcodereview.appspot.com/10836017/diff/5002/remoting/host/chrom...
remoting/host/chromoting_host.cc:434: NOTIMPLEMENTED();
nit: NOTIMPLEMENTED() is for methods that aren't implemented, whereas this is
effectively an invalid-parameter error, so you should probably DCHECK or DFATAL
log something.