Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10831246:
Speex encoding/decoding. (Closed)
|
Created:
8 years, 4 months ago by kxing Modified:
8 years, 3 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSpeex encoding/decoding.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152637
Patch Set 1 #
Total comments: 20
Patch Set 2 : Addressed Comments #Patch Set 3 : spx_int16_t -> int16 #
Total comments: 36
Patch Set 4 : Addressed comments #
Total comments: 8
Patch Set 5 : Addressed comments #
Total comments: 38
Patch Set 6 : Addressed comments #
Total comments: 6
Patch Set 7 : Addressed comments #
Total comments: 1
Messages
Total messages: 21 (0 generated)
Could someone please PTAL? Regarding the encoded frame size, I'm not completely sure if it'll fit in 256 bytes, but empirically it looks like it fits. I'll ping satish to double check.
http://codereview.chromium.org/10831246/diff/1/remoting/codec/audio_decoder_s... File remoting/codec/audio_decoder_speex.cc (right): http://codereview.chromium.org/10831246/diff/1/remoting/codec/audio_decoder_s... remoting/codec/audio_decoder_speex.cc:12: #include "base/stl_util.h" is this used anywhere? http://codereview.chromium.org/10831246/diff/1/remoting/codec/audio_decoder_s... remoting/codec/audio_decoder_speex.cc:64: speex_decode_int(speex_state_, &speex_bits_, buffer_.get()); This call may fail (e.g. if other side sends garbage). You should not ignore the error here. http://codereview.chromium.org/10831246/diff/1/remoting/codec/audio_decoder_s... remoting/codec/audio_decoder_speex.cc:76: decoded_packet->set_data(decoded_data); you can avoid copying the data by allocating decoded_packet first http://codereview.chromium.org/10831246/diff/1/remoting/codec/audio_decoder_s... File remoting/codec/audio_decoder_speex.h (right): http://codereview.chromium.org/10831246/diff/1/remoting/codec/audio_decoder_s... remoting/codec/audio_decoder_speex.h:26: SpeexBits speex_bits_; maybe use scoped_ptr here - that way you can avoid speex.h include http://codereview.chromium.org/10831246/diff/1/remoting/codec/audio_encoder_s... File remoting/codec/audio_encoder_speex.cc (right): http://codereview.chromium.org/10831246/diff/1/remoting/codec/audio_encoder_s... remoting/codec/audio_encoder_speex.cc:66: LOG(ERROR) << "Speex encoder is not compressing data."; WARNING? http://codereview.chromium.org/10831246/diff/1/remoting/codec/audio_encoder_s... remoting/codec/audio_encoder_speex.cc:87: encoded_packet->set_data(encoded_data); To avoid copying it here it's better to allocate encoded_packet first and store encoded data directly in it. http://codereview.chromium.org/10831246/diff/1/remoting/codec/audio_encoder_s... File remoting/codec/audio_encoder_speex.h (right): http://codereview.chromium.org/10831246/diff/1/remoting/codec/audio_encoder_s... remoting/codec/audio_encoder_speex.h:26: SpeexBits speex_bits_; scoped_ptr to avoid the header http://codereview.chromium.org/10831246/diff/1/remoting/proto/audio.proto File remoting/proto/audio.proto (right): http://codereview.chromium.org/10831246/diff/1/remoting/proto/audio.proto#new... remoting/proto/audio.proto:22: ENCODING_SPEEX = 1; Please keep OGG and rename it to ENCODING_VORBIS http://codereview.chromium.org/10831246/diff/1/remoting/protocol/content_desc... File remoting/protocol/content_description.cc (right): http://codereview.chromium.org/10831246/diff/1/remoting/protocol/content_desc... remoting/protocol/content_description.cc:46: const char kSpeexCodec[] = "speex"; No need to remove vorbis - we will still plug it in eventually http://codereview.chromium.org/10831246/diff/1/remoting/remoting.gyp File remoting/remoting.gyp (right): http://codereview.chromium.org/10831246/diff/1/remoting/remoting.gyp#newcode1218 remoting/remoting.gyp:1218: '../third_party/speex/include', You can avoid it - see my comments in audio_decoder_speex.h . In any case it would be better to add dependency directly in this target instead of specifying include_dir
Could someone PTAL again? http://codereview.chromium.org/10831246/diff/1/remoting/codec/audio_decoder_s... File remoting/codec/audio_decoder_speex.cc (right): http://codereview.chromium.org/10831246/diff/1/remoting/codec/audio_decoder_s... remoting/codec/audio_decoder_speex.cc:12: #include "base/stl_util.h" It's used to call string_as_array(). http://codereview.chromium.org/10831246/diff/1/remoting/codec/audio_decoder_s... remoting/codec/audio_decoder_speex.cc:64: speex_decode_int(speex_state_, &speex_bits_, buffer_.get()); On 2012/08/10 20:41:37, sergeyu wrote: > This call may fail (e.g. if other side sends garbage). You should not ignore the > error here. Done. http://codereview.chromium.org/10831246/diff/1/remoting/codec/audio_decoder_s... remoting/codec/audio_decoder_speex.cc:76: decoded_packet->set_data(decoded_data); On 2012/08/10 20:41:37, sergeyu wrote: > you can avoid copying the data by allocating decoded_packet first Done. http://codereview.chromium.org/10831246/diff/1/remoting/codec/audio_decoder_s... File remoting/codec/audio_decoder_speex.h (right): http://codereview.chromium.org/10831246/diff/1/remoting/codec/audio_decoder_s... remoting/codec/audio_decoder_speex.h:26: SpeexBits speex_bits_; On 2012/08/10 20:41:37, sergeyu wrote: > maybe use scoped_ptr here - that way you can avoid speex.h include Done. http://codereview.chromium.org/10831246/diff/1/remoting/codec/audio_encoder_s... File remoting/codec/audio_encoder_speex.cc (right): http://codereview.chromium.org/10831246/diff/1/remoting/codec/audio_encoder_s... remoting/codec/audio_encoder_speex.cc:66: LOG(ERROR) << "Speex encoder is not compressing data."; On 2012/08/10 20:41:37, sergeyu wrote: > WARNING? Done. http://codereview.chromium.org/10831246/diff/1/remoting/codec/audio_encoder_s... remoting/codec/audio_encoder_speex.cc:87: encoded_packet->set_data(encoded_data); On 2012/08/10 20:41:37, sergeyu wrote: > To avoid copying it here it's better to allocate encoded_packet first and store > encoded data directly in it. Done. http://codereview.chromium.org/10831246/diff/1/remoting/codec/audio_encoder_s... File remoting/codec/audio_encoder_speex.h (right): http://codereview.chromium.org/10831246/diff/1/remoting/codec/audio_encoder_s... remoting/codec/audio_encoder_speex.h:26: SpeexBits speex_bits_; On 2012/08/10 20:41:37, sergeyu wrote: > scoped_ptr to avoid the header Done. http://codereview.chromium.org/10831246/diff/1/remoting/proto/audio.proto File remoting/proto/audio.proto (right): http://codereview.chromium.org/10831246/diff/1/remoting/proto/audio.proto#new... remoting/proto/audio.proto:22: ENCODING_SPEEX = 1; On 2012/08/10 20:41:37, sergeyu wrote: > Please keep OGG and rename it to ENCODING_VORBIS Done. http://codereview.chromium.org/10831246/diff/1/remoting/protocol/content_desc... File remoting/protocol/content_description.cc (right): http://codereview.chromium.org/10831246/diff/1/remoting/protocol/content_desc... remoting/protocol/content_description.cc:46: const char kSpeexCodec[] = "speex"; On 2012/08/10 20:41:37, sergeyu wrote: > No need to remove vorbis - we will still plug it in eventually Done. http://codereview.chromium.org/10831246/diff/1/remoting/remoting.gyp File remoting/remoting.gyp (right): http://codereview.chromium.org/10831246/diff/1/remoting/remoting.gyp#newcode1218 remoting/remoting.gyp:1218: '../third_party/speex/include', On 2012/08/10 20:41:37, sergeyu wrote: > You can avoid it - see my comments in audio_decoder_speex.h . In any case it > would be better to add dependency directly in this target instead of specifying > include_dir Done.
I think we should modify AudioPacket protobuf to make framing of Speex packets explicit. Also there is bunch of places where we can avoid copying data between buffers. http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_decode... File remoting/codec/audio_decoder_speex.cc (right): http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.cc:45: // or the API is not const-correct, we have to convert the packet looking at the speex code it's the latter. speex_bits_read_from() just copies data to internal buffer. So you can just const_cast<> the packet to avoid copying it twice. http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.cc:52: decoded_packet->set_data(std::string()); don't need this. mutable_data() will do it for you the first time you call that method. http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.cc:61: ptr++; nit: in remoting code it's more common to use prefix increment: ++ptr. http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.cc:62: bytes_left--; nit: same here http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.cc:74: LOG(ERROR) << "Error in decoding Speex data."; Do you need to return or break here? http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.cc:81: decoded_packet->mutable_data()->append( instead of appending data here can you specify mutable_data()->data() when calling speex_decode_int() (provided you resize it appropriately first). http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_decode... File remoting/codec/audio_decoder_speex.h (right): http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.h:23: // AudioDecoder implementation nit: add period at the end of the comment http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.h:32: scoped_array<int16> buffer_; Do we need this buffer? each input packet always contains whole speex frames, so we don't need to buffer any data between Decode() calls. http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_encode... File remoting/codec/audio_encoder_speex.cc (right): http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:51: int number_of_samples = packet->data().size() / sizeof(spx_int16_t); nit: divide by packet->bytes_per_sample(). It's the same value, but more readable. http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:55: queued_samples_.push_back(*sample); The data we store between Encode() calls is always smaller than a single frame. So maybe store the leftovers from the previous call in speex_bits_ and reset the bits struct after speex_encode_int()? http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:61: encoded_packet->set_data(std::string()); don't need this line. http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:70: raw_data_buffer_[i] = queued_samples_.front(); ouch. we just copied data to queued_samples, now we are copying it again to raw_data_buffer_. That's slow because you will need malloc/free for each sample. http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:75: speex_bits_reset(speex_bits_.get()); why do we need to reset it here? http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:84: encoded_data_buffer_.get(), pass a buffer inside encoded_packet->mutable_data() instead? http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:91: uint8 bytes_written_as_byte = static_cast<uint8>(bytes_written); can we be sure that each encoded frame is smaller than 255? http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:92: encoded_packet->mutable_data()->append( Would it be better to make data field in AudioPacket repeated? Alternately you can keep data field as as, but add repeated field speex_frame and use it for speex-encoded frames? http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:98: return encoded_packet.Pass(); we may return an empty packet that will be wasting the bandwidth. return NULL instead? http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_encode... File remoting/codec/audio_encoder_speex.h (right): http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.h:33: std::list<int16> queued_samples_; ouch. can we use std::vector here? Or better scoped_vector<AudioPacket>?
Could someone PTAL? http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_decode... File remoting/codec/audio_decoder_speex.cc (right): http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.cc:45: // or the API is not const-correct, we have to convert the packet On 2012/08/14 00:54:32, sergeyu wrote: > looking at the speex code it's the latter. speex_bits_read_from() just copies > data to internal buffer. So you can just const_cast<> the packet to avoid > copying it twice. Done. http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.cc:52: decoded_packet->set_data(std::string()); On 2012/08/14 00:54:32, sergeyu wrote: > don't need this. mutable_data() will do it for you the first time you call that > method. Done. http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.cc:61: ptr++; On 2012/08/14 00:54:32, sergeyu wrote: > nit: in remoting code it's more common to use prefix increment: ++ptr. Done. http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.cc:62: bytes_left--; On 2012/08/14 00:54:32, sergeyu wrote: > nit: same here Done. http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.cc:74: LOG(ERROR) << "Error in decoding Speex data."; On 2012/08/14 00:54:32, sergeyu wrote: > Do you need to return or break here? Done. http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.cc:81: decoded_packet->mutable_data()->append( On 2012/08/14 00:54:32, sergeyu wrote: > instead of appending data here can you specify mutable_data()->data() when > calling speex_decode_int() (provided you resize it appropriately first). Done. http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_decode... File remoting/codec/audio_decoder_speex.h (right): http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.h:23: // AudioDecoder implementation On 2012/08/14 00:54:32, sergeyu wrote: > nit: add period at the end of the comment Done. http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.h:32: scoped_array<int16> buffer_; On 2012/08/14 00:54:32, sergeyu wrote: > Do we need this buffer? each input packet always contains whole speex frames, so > we don't need to buffer any data between Decode() calls. Done. http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_encode... File remoting/codec/audio_encoder_speex.cc (right): http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:51: int number_of_samples = packet->data().size() / sizeof(spx_int16_t); On 2012/08/14 00:54:32, sergeyu wrote: > nit: divide by packet->bytes_per_sample(). It's the same value, but more > readable. Done. http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:55: queued_samples_.push_back(*sample); On 2012/08/14 00:54:32, sergeyu wrote: > The data we store between Encode() calls is always smaller than a single frame. > So maybe store the leftovers from the previous call in speex_bits_ and reset the > bits struct after speex_encode_int()? Done. http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:61: encoded_packet->set_data(std::string()); On 2012/08/14 00:54:32, sergeyu wrote: > don't need this line. Done. http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:70: raw_data_buffer_[i] = queued_samples_.front(); On 2012/08/14 00:54:32, sergeyu wrote: > ouch. we just copied data to queued_samples, now we are copying it again to > raw_data_buffer_. That's slow because you will need malloc/free for each sample. Done. http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:75: speex_bits_reset(speex_bits_.get()); On 2012/08/14 00:54:32, sergeyu wrote: > why do we need to reset it here? Done. http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:84: encoded_data_buffer_.get(), On 2012/08/14 00:54:32, sergeyu wrote: > pass a buffer inside encoded_packet->mutable_data() instead? Done. http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:91: uint8 bytes_written_as_byte = static_cast<uint8>(bytes_written); On 2012/08/14 00:54:32, sergeyu wrote: > can we be sure that each encoded frame is smaller than 255? Done. http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:92: encoded_packet->mutable_data()->append( On 2012/08/14 00:54:32, sergeyu wrote: > Would it be better to make data field in AudioPacket repeated? Alternately you > can keep data field as as, but add repeated field speex_frame and use it for > speex-encoded frames? Done. http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:98: return encoded_packet.Pass(); The empty packet filtering happens at the capturer level, so it should not reach the encoder. http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_encode... File remoting/codec/audio_encoder_speex.h (right): http://codereview.chromium.org/10831246/diff/2002/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.h:33: std::list<int16> queued_samples_; On 2012/08/14 00:54:32, sergeyu wrote: > ouch. can we use std::vector here? Or better scoped_vector<AudioPacket>? Done.
Ping.
I've noticed that you are not taking into account that we have more than one audio channel. Looks like a bug to me. http://codereview.chromium.org/10831246/diff/11006/remoting/codec/audio_decod... File remoting/codec/audio_decoder_speex.cc (right): http://codereview.chromium.org/10831246/diff/11006/remoting/codec/audio_decod... remoting/codec/audio_decoder_speex.cc:22: speex_state_ = speex_decoder_init(&speex_wb_mode); Do we tell speex anywhere that this is a stereo stream? http://codereview.chromium.org/10831246/diff/11006/remoting/codec/audio_decod... remoting/codec/audio_decoder_speex.cc:26: speex_decoder_ctl(speex_state_, SPEEX_SET_ENH, &enhancer); DCHECK here and below to verify that it returns 0. http://codereview.chromium.org/10831246/diff/11006/remoting/codec/audio_encod... File remoting/codec/audio_encoder_speex.cc (right): http://codereview.chromium.org/10831246/diff/11006/remoting/codec/audio_encod... remoting/codec/audio_encoder_speex.cc:51: int samples_left = packet->data(0).size() / packet->bytes_per_sample(); This is not correct because we normally have two channels. It looks to me that you are encoding samples from two channels is if they are one channel. I think you need to use speex_decode_stereo_int() and speex_encode_stereo_int() in order to encode/decode stereo stream http://codereview.chromium.org/10831246/diff/11006/remoting/codec/audio_encod... remoting/codec/audio_encoder_speex.cc:103: memcpy(raw_data_buffer_.get() + leftover_samples_, Need to verify that there is always space in the buffer. Add CHECK_LE(lefover_samples + samples_left, speex_frame_size_);
(Depends on http://codereview.chromium.org/10823420/ - so the DCHECK(channels == 2) code doesn't break.) Could someone PTAL? http://codereview.chromium.org/10831246/diff/11006/remoting/codec/audio_decod... File remoting/codec/audio_decoder_speex.cc (right): http://codereview.chromium.org/10831246/diff/11006/remoting/codec/audio_decod... remoting/codec/audio_decoder_speex.cc:22: speex_state_ = speex_decoder_init(&speex_wb_mode); On 2012/08/17 21:52:17, sergeyu wrote: > Do we tell speex anywhere that this is a stereo stream? Done. http://codereview.chromium.org/10831246/diff/11006/remoting/codec/audio_decod... remoting/codec/audio_decoder_speex.cc:26: speex_decoder_ctl(speex_state_, SPEEX_SET_ENH, &enhancer); On 2012/08/17 21:52:17, sergeyu wrote: > DCHECK here and below to verify that it returns 0. Done. http://codereview.chromium.org/10831246/diff/11006/remoting/codec/audio_encod... File remoting/codec/audio_encoder_speex.cc (right): http://codereview.chromium.org/10831246/diff/11006/remoting/codec/audio_encod... remoting/codec/audio_encoder_speex.cc:51: int samples_left = packet->data(0).size() / packet->bytes_per_sample(); On 2012/08/17 21:52:18, sergeyu wrote: > This is not correct because we normally have two channels. It looks to me that > you are encoding samples from two channels is if they are one channel. > I think you need to use speex_decode_stereo_int() and > speex_encode_stereo_int() in order to encode/decode stereo stream Done. http://codereview.chromium.org/10831246/diff/11006/remoting/codec/audio_encod... remoting/codec/audio_encoder_speex.cc:103: memcpy(raw_data_buffer_.get() + leftover_samples_, On 2012/08/17 21:52:18, sergeyu wrote: > Need to verify that there is always space in the buffer. Add > CHECK_LE(lefover_samples + samples_left, speex_frame_size_); Done.
http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_decode... File remoting/codec/audio_decoder_speex.cc (right): http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.cc:19: } nit: // extern "C" http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.cc:53: LOG(ERROR) << "Failed to get the frame size"; Should we also remember that we failed to initialize and drop everything in Decode()? E.g. you can do it by setting speex_frame_size_ to some invalid value. Otherwise we will be trying to decode using uninitialized value of speex_frame_size_ Or alternatively you can replace this with LOG(FATAL) so we just crash if this error ever happens (we don't expect it ever happen under normal conditions anyway). http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.cc:79: LOG(WARNING) << "Received a corrupted packet."; It's not necessarily corrupt. It's just not supported (e.g. Mono). http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_encode... File remoting/codec/audio_encoder_speex.cc (right): http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:36: speex_encoder_ctl(speex_state_, SPEEX_GET_FRAME_SIZE, &speex_frame_size_); Check that this operation succeeds? also see my comments in the decoder. http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:55: int samples_left = packet->data(0).size() / packet->bytes_per_sample(); It's better to divide this by number of channels() - you never want to have samples for only some of the channels. I.e. this should be samples per channel, not total number of samples. It will be also more readable that way - you won't need to multiply by channels() in many places in the code below. http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:67: speex_frame_size_ * packet->channels()) { nit: indent one more space to align with ( http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:124: return encoded_packet.Pass(); Return null if there is no data in the packet (e.g. if the raw packet we've got is smaller than the speex frame size).
http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_decode... File remoting/codec/audio_decoder_speex.cc (right): http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.cc:40: // Turn perceptual enhancer. nit: Turn on perceptual enhancer? Perhaps indicate in the comment what this is actually for. http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.cc:45: return; You're early exiting here, but you don't have any member initializers above, so there is a risk that if one of these errors occurs you end up with a partially-uninitialized object. Also, how does the caller detect that the decoder is broken in that case? http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.cc:48: // Get the frame size. nit: Similarly, indicate _why_ we need to get the frame size. http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.cc:57: // Set the stereo callback. nit: Similarly, indicate what the significance of the stereo callback is; is this just a stereo equivalent of a mono audio callback, for instance? http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_encode... File remoting/codec/audio_encoder_speex.cc (right): http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:18: const int kSpeexQuality = 8; nit: Name this constant to indicate what quality we're requesting, e.g. kSpeexHighQuality http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:68: if (leftover_samples_ > 0) { Can you restructure this if...else... so that the SPEEX calls are outside it entirely, rather than duplicated in the two blocks, and all it does is set up a pointer & frame size to supply to those calls, either by setting up the data buffer, or by passing the current packet's data directly? http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:79: speex_bits_.get()); nit: Missing blank line after this? http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:90: speex_bits_.get()); nit: blank line http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:94: speex_bits_.get()); nit: and here http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_encode... File remoting/codec/audio_encoder_speex.h (right): http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.h:33: scoped_array<int16> raw_data_buffer_; This buffer is just used to store "left-over" samples, so rename it |leftover_buffer|, or rename it sample_buffer_ and rename leftover_samples to sample_count_? http://codereview.chromium.org/10831246/diff/8005/remoting/proto/audio.proto File remoting/proto/audio.proto (right): http://codereview.chromium.org/10831246/diff/8005/remoting/proto/audio.proto#... remoting/proto/audio.proto:25: ENCODING_VORBIS = 1; nit: Remove VORBIS since we're unlikely to get around to implementing it?
http://codereview.chromium.org/10831246/diff/8005/remoting/proto/audio.proto File remoting/proto/audio.proto (right): http://codereview.chromium.org/10831246/diff/8005/remoting/proto/audio.proto#... remoting/proto/audio.proto:25: ENCODING_VORBIS = 1; On 2012/08/21 01:14:13, Wez wrote: > nit: Remove VORBIS since we're unlikely to get around to implementing it? Or maybe just replace it with OPUS?
Could someone PTAL? http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_decode... File remoting/codec/audio_decoder_speex.cc (right): http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.cc:19: } On 2012/08/20 22:50:39, sergeyu wrote: > nit: // extern "C" Done. http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.cc:40: // Turn perceptual enhancer. On 2012/08/21 01:14:13, Wez wrote: > nit: Turn on perceptual enhancer? Perhaps indicate in the comment what this is > actually for. Done. http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.cc:45: return; On 2012/08/21 01:14:13, Wez wrote: > You're early exiting here, but you don't have any member initializers above, so > there is a risk that if one of these errors occurs you end up with a > partially-uninitialized object. Also, how does the caller detect that the > decoder is broken in that case? Done. http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.cc:48: // Get the frame size. On 2012/08/21 01:14:13, Wez wrote: > nit: Similarly, indicate _why_ we need to get the frame size. Done. http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.cc:53: LOG(ERROR) << "Failed to get the frame size"; On 2012/08/20 22:50:39, sergeyu wrote: > Should we also remember that we failed to initialize and drop everything in > Decode()? E.g. you can do it by setting speex_frame_size_ to some invalid value. > Otherwise we will be trying to decode using uninitialized value of > speex_frame_size_ > > Or alternatively you can replace this with LOG(FATAL) so we just crash if this > error ever happens (we don't expect it ever happen under normal conditions > anyway). Done. http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.cc:57: // Set the stereo callback. On 2012/08/21 01:14:13, Wez wrote: > nit: Similarly, indicate what the significance of the stereo callback is; is > this just a stereo equivalent of a mono audio callback, for instance? Done. http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_decode... remoting/codec/audio_decoder_speex.cc:79: LOG(WARNING) << "Received a corrupted packet."; On 2012/08/20 22:50:39, sergeyu wrote: > It's not necessarily corrupt. It's just not supported (e.g. Mono). Done. http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_encode... File remoting/codec/audio_encoder_speex.cc (right): http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:18: const int kSpeexQuality = 8; On 2012/08/21 01:14:13, Wez wrote: > nit: Name this constant to indicate what quality we're requesting, e.g. > kSpeexHighQuality Done. http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:36: speex_encoder_ctl(speex_state_, SPEEX_GET_FRAME_SIZE, &speex_frame_size_); On 2012/08/20 22:50:39, sergeyu wrote: > Check that this operation succeeds? also see my comments in the decoder. Done. http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:55: int samples_left = packet->data(0).size() / packet->bytes_per_sample(); On 2012/08/20 22:50:39, sergeyu wrote: > It's better to divide this by number of channels() - you never want to have > samples for only some of the channels. I.e. this should be samples per channel, > not total number of samples. > It will be also more readable that way - you won't need to multiply by > channels() in many places in the code below. Done. http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:67: speex_frame_size_ * packet->channels()) { On 2012/08/20 22:50:39, sergeyu wrote: > nit: indent one more space to align with ( Done. http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:68: if (leftover_samples_ > 0) { On 2012/08/21 01:14:13, Wez wrote: > Can you restructure this if...else... so that the SPEEX calls are outside it > entirely, rather than duplicated in the two blocks, and all it does is set up a > pointer & frame size to supply to those calls, either by setting up the data > buffer, or by passing the current packet's data directly? Done. http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:79: speex_bits_.get()); On 2012/08/21 01:14:13, Wez wrote: > nit: Missing blank line after this? Done. http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:90: speex_bits_.get()); On 2012/08/21 01:14:13, Wez wrote: > nit: blank line Done. http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:94: speex_bits_.get()); On 2012/08/21 01:14:13, Wez wrote: > nit: and here Done. http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.cc:124: return encoded_packet.Pass(); On 2012/08/20 22:50:39, sergeyu wrote: > Return null if there is no data in the packet (e.g. if the raw packet we've got > is smaller than the speex frame size). Done. http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_encode... File remoting/codec/audio_encoder_speex.h (right): http://codereview.chromium.org/10831246/diff/8005/remoting/codec/audio_encode... remoting/codec/audio_encoder_speex.h:33: scoped_array<int16> raw_data_buffer_; On 2012/08/21 01:14:13, Wez wrote: > This buffer is just used to store "left-over" samples, so rename it > |leftover_buffer|, or rename it sample_buffer_ and rename leftover_samples to > sample_count_? Done. http://codereview.chromium.org/10831246/diff/8005/remoting/proto/audio.proto File remoting/proto/audio.proto (right): http://codereview.chromium.org/10831246/diff/8005/remoting/proto/audio.proto#... remoting/proto/audio.proto:25: ENCODING_VORBIS = 1; On 2012/08/21 01:14:13, Wez wrote: > nit: Remove VORBIS since we're unlikely to get around to implementing it? Sounds good. This CL's getting huge, so I'll do in a separate CL. http://codereview.chromium.org/10831246/diff/8005/remoting/proto/audio.proto#... remoting/proto/audio.proto:25: ENCODING_VORBIS = 1; On 2012/08/21 01:19:44, sergeyu wrote: > On 2012/08/21 01:14:13, Wez wrote: > > nit: Remove VORBIS since we're unlikely to get around to implementing it? > Or maybe just replace it with OPUS? Sounds good. This CL's getting huge, so I'll do in a separate CL.
lgtm when my nits are addressed http://codereview.chromium.org/10831246/diff/11012/remoting/codec/audio_decod... File remoting/codec/audio_decoder_speex.cc (right): http://codereview.chromium.org/10831246/diff/11012/remoting/codec/audio_decod... remoting/codec/audio_decoder_speex.cc:41: if (result < 0) { nit: If you move result to a separate variable, then CHECK_EQ(result, 0); is more appropriate as it takes only one line. http://codereview.chromium.org/10831246/diff/11012/remoting/codec/audio_decod... remoting/codec/audio_decoder_speex.cc:43: return; nit: don't need this return because you have LOG(FATAL) above. http://codereview.chromium.org/10831246/diff/11012/remoting/codec/audio_encod... File remoting/codec/audio_encoder_speex.cc (right): http://codereview.chromium.org/10831246/diff/11012/remoting/codec/audio_encod... remoting/codec/audio_encoder_speex.cc:62: / packet->channels(); nit: / goes to the previous line.
I'll commit this soon, unless someone objects. http://codereview.chromium.org/10831246/diff/11012/remoting/codec/audio_decod... File remoting/codec/audio_decoder_speex.cc (right): http://codereview.chromium.org/10831246/diff/11012/remoting/codec/audio_decod... remoting/codec/audio_decoder_speex.cc:41: if (result < 0) { On 2012/08/21 17:32:23, sergeyu wrote: > nit: If you move result to a separate variable, then CHECK_EQ(result, 0); is > more appropriate as it takes only one line. Done. http://codereview.chromium.org/10831246/diff/11012/remoting/codec/audio_decod... remoting/codec/audio_decoder_speex.cc:43: return; On 2012/08/21 17:32:23, sergeyu wrote: > nit: don't need this return because you have LOG(FATAL) above. Done. http://codereview.chromium.org/10831246/diff/11012/remoting/codec/audio_encod... File remoting/codec/audio_encoder_speex.cc (right): http://codereview.chromium.org/10831246/diff/11012/remoting/codec/audio_encod... remoting/codec/audio_encoder_speex.cc:62: / packet->channels(); On 2012/08/21 17:32:23, sergeyu wrote: > nit: / goes to the previous line. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kxing@chromium.org/10831246/3008
Change committed as 152637
https://chromiumcodereview.appspot.com/10831246/diff/3008/remoting/codec/audi... File remoting/codec/audio_decoder_speex.cc (right): https://chromiumcodereview.appspot.com/10831246/diff/3008/remoting/codec/audi... remoting/codec/audio_decoder_speex.cc:15: #include "third_party/speex/include/speex/speex_callbacks.h" This breaks build with system speex, see https://bugs.gentoo.org/show_bug.cgi?id=432748 Please take a look at how third_party/speex/speex.h is written, and either create similar shim headers for speex_callbacks.h and speex_stereo.h, or make speex.h shim header include those other headers. Let me know if you need more hints how to fix this.
On 2012/08/29 08:51:33, Paweł Hajdan Jr. wrote: > https://chromiumcodereview.appspot.com/10831246/diff/3008/remoting/codec/audi... > File remoting/codec/audio_decoder_speex.cc (right): > > https://chromiumcodereview.appspot.com/10831246/diff/3008/remoting/codec/audi... > remoting/codec/audio_decoder_speex.cc:15: #include > "third_party/speex/include/speex/speex_callbacks.h" > This breaks build with system speex, see > https://bugs.gentoo.org/show_bug.cgi?id=432748 > > Please take a look at how third_party/speex/speex.h is written, and either > create similar shim headers for speex_callbacks.h and speex_stereo.h, or make > speex.h shim header include those other headers. > > Let me know if you need more hints how to fix this. Can you create a bug on crbug.com with the build failure output, and we'll look into it? Thanks.
On 2012/08/29 08:51:33, Paweł Hajdan Jr. wrote: > https://chromiumcodereview.appspot.com/10831246/diff/3008/remoting/codec/audi... > File remoting/codec/audio_decoder_speex.cc (right): > > https://chromiumcodereview.appspot.com/10831246/diff/3008/remoting/codec/audi... > remoting/codec/audio_decoder_speex.cc:15: #include > "third_party/speex/include/speex/speex_callbacks.h" > This breaks build with system speex, see > https://bugs.gentoo.org/show_bug.cgi?id=432748 > > Please take a look at how third_party/speex/speex.h is written, and either > create similar shim headers for speex_callbacks.h and speex_stereo.h, or make > speex.h shim header include those other headers. > > Let me know if you need more hints how to fix this. This fix seems simple, but I'm not sure the best way to update the third_party code. Do the chromium-specific files (like the .gyp and the shims) get checked in directly in third_party/speex or are they automatically pulled from somewhere else?
On 2012/08/29 17:17:53, garykac wrote: > On 2012/08/29 08:51:33, Paweł Hajdan Jr. wrote: > > > https://chromiumcodereview.appspot.com/10831246/diff/3008/remoting/codec/audi... > > File remoting/codec/audio_decoder_speex.cc (right): > > > > > https://chromiumcodereview.appspot.com/10831246/diff/3008/remoting/codec/audi... > > remoting/codec/audio_decoder_speex.cc:15: #include > > "third_party/speex/include/speex/speex_callbacks.h" > > This breaks build with system speex, see > > https://bugs.gentoo.org/show_bug.cgi?id=432748 > > > > Please take a look at how third_party/speex/speex.h is written, and either > > create similar shim headers for speex_callbacks.h and speex_stereo.h, or make > > speex.h shim header include those other headers. > > > > Let me know if you need more hints how to fix this. > > This fix seems simple, but I'm not sure the best way to update the third_party > code. Do the chromium-specific files (like the .gyp and the shims) get checked > in directly in third_party/speex or are they automatically pulled from somewhere > else? You first need to check in new headers in svn://svn.chromium.org/chrome/trunk/deps/third_party/speex and then roll new change by updating DEPS file.
Filed https://code.google.com/p/chromium/issues/detail?id=145507 third_party/speex is pulled via DEPS from deps/third_party. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
