Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2326003002:
Moved codec-specific audio packet splitting into decoders. (Closed)
DescriptionMoved codec-specific audio packet splitting into decoders.
There's still some code run specifically for Opus w/ FEC. It will be
addressed in a separate CL.
BUG=webrtc:5805
Committed: https://crrev.com/0d526d558b0f7b677c43f83e64e67ad65a9639c0
Cr-Commit-Position: refs/heads/master@{#14319}
Patch Set 1 #
Total comments: 68
Patch Set 2 : Reworked packet splitting. Renamed SplitBySamples and AudioCodingUtils. #
Total comments: 42
Patch Set 3 : Cleanups. rtc::Buffer passing changes. #
Total comments: 13
Patch Set 4 : A couple of consts and a period. #Patch Set 5 : Rebase #Patch Set 6 : Fixed types in packet splitting (size_t vs. uint32_t) #Depends on Patchset: Dependent Patchsets: Messages
Total messages: 46 (18 generated)
ossu@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, kwiberg@webrtc.org
Step 2. More to come, probably on Monday!
Three nits, then lgtm. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_decoder.cc (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.cc:101: RTC_CHECK(split.byte_offset + split.num_bytes <= payload->size()); RTC_CHECK_LE https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/ilbc/ilbc_unittest.cc (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/ilbc/ilbc_unittest.cc:70: // Test splitting sample-based payloads. iLBC does not split sample-based. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/pcm16b/audio_decoder_pcm16b.cc (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/pcm16b/audio_decoder_pcm16b.cc:50: // TODO(ossu): Investigate if we can ever get 44.1KHz audio here, in which Short answer: no. If codecs should be allowed to return something else than {8, 16, 32, 48} kHz, we've got plenty of bigger fish to fry. But hey, feel free to add that CheckedDivExact. Nit: kHz with small 'k'.
https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_coding.gypi (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_coding.gypi:190: 'target_name': 'audio_coding_utils', Why not name this split_by_samples? https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_decoder.cc (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.cc:109: } If you processed the pieces in the reverse order, could you steal the buffer from the first (but last to be processed) piece? That would both let you eliminate the special case for size() == 1 and apply the corresponding optimization to the size() > 1 case. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_decoder.h (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.h:133: rtc::ArrayView<const uint8_t> payload) const; If you turned the return value into an output argument, you would (with the cooperation of the caller) not have to pay for a heap allocation and deallocation for each call. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc:55: std::vector<PacketSplit> splits; Allocate this one much closer to first point of use? https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc:58: if (payload.size() >= 950) { As I think I've noted before, the only sizes that are actually problematic are those that are zero both mod 38 and mod 50... so you could write this as const bool frame_size_20ms = payload.size() % 38 == 0; const bool frame_size_30ms = payload.size() % 50 == 0; if (frame_size_20ms && frame_size_30ms) { RTC_DCHECK_GE(payload.size(), 950); // LCM(38, 50) = 950. // handle error } else if (frame_size_20ms) { // ... } else if (frame_size_30ms) { // ... } else { // handle error } This way, it's more obvious that you're simple handling all four possible cases. But yes, I expect that towards the end of the CL, I'll discover that you've just moved this piece of code from somewhere else, so never mind... https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc:78: } Why is this special case needed? Doesn't the loop below handle this case just fine? https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc:81: for (size_t len = payload.size(); len > 0; len -= bytes_per_frame) { Better DCHECK that payload.size() % bytes_per_frame == 0 before this loop, since that's crucial. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/ilbc/ilbc_unittest.cc (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/ilbc/ilbc_unittest.cc:74: size_t payload_length_bytes = frame_length_bytes_ * num_frames_; const? https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/ilbc/ilbc_unittest.cc:118: size_t kPayloadLengthBytes = 950; constexpr https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/ilbc/ilbc_unittest.cc:119: rtc::Buffer payload(kPayloadLengthBytes); const https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/ilbc/ilbc_unittest.cc:120: auto splits = decoder.SplitPacket(payload); const https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/ilbc/ilbc_unittest.cc:129: auto splits = decoder.SplitPacket(payload); constexpr, const, const https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/pcm16b/audio_decoder_pcm16b.cc (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/pcm16b/audio_decoder_pcm16b.cc:50: // TODO(ossu): Investigate if we can ever get 44.1KHz audio here, in which On 2016/09/09 12:52:52, hlundin-webrtc wrote: > Short answer: no. If codecs should be allowed to return something else than {8, > 16, 32, 48} kHz, we've got plenty of bigger fish to fry. But hey, feel free to > add that CheckedDivExact. > > Nit: kHz with small 'k'. Also, space between the number and the unit. But I guess it doesn't matter since this comment is going away... https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/split_by_samples.cc (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/split_by_samples.cc:26: size_t min_chunk_size = bytes_per_ms * 20; const https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/split_by_samples.cc:33: uint32_t timestamps_per_chunk = static_cast<uint32_t>( const https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/split_by_samples.cc:37: for (len = payload.size(); You initialize twice. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/split_by_samples.h (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/split_by_samples.h:21: namespace internal { Why put this in the "internal" namespace? It isn't exposed in public headers (is it?), and we're not in the habit of putting non-public things in general in this namespace. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/split_by_samples.h:31: uint32_t timestamps_per_ms); Consider having an output argument instead of a return value to enable reuse of the heap allocation for the vector. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/split_by_samples_unittest.cc (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/split_by_samples_unittest.cc:83: }; Why use the NetEqDecoder enum, instead of feeding the test the numerical values directly? And why test only the values used by our current codecs? https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/split_by_samples_unittest.cc:100: ExpectedSplit expected_splits[] = { constexpr https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/split_by_samples_unittest.cc:112: // int expected_num_packets[] = {1, 1, 1, 2, 2, 2}; Are these useful or not? https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/split_by_samples_unittest.cc:117: size_t payload_size_bytes = expected_split.payload_size_ms * bytes_per_ms_; const https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/split_by_samples_unittest.cc:118: rtc::Buffer payload(payload_size_bytes); const https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/split_by_samples_unittest.cc:119: auto splits = const
My biggest concern with this CL is whether it should use the SplitAudio interface from the previous audio splitting CL at all? There's really no need to, since it just introduces a temporary interface to support the ParsePayload interface that is really there to supplant SplitAudio and others. Keeping it is also the source of some of the concerns in the rest of the CL: adding a build target just for SplitByAudio, returning a set of splits as a std::vector<> rather than output parameter and what parameters are sent in to test SplitBySamples. I'm not sure putting the sample-splitting code into each decoder is the way to go either, though, but maybe SplitBySamples should be rewritten to work according to the ParsePayload interface, so there's no glue necessary. SplitAudio was, however, reviewed previously and I hoped that would help with context in this review. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_coding.gypi (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_coding.gypi:190: 'target_name': 'audio_coding_utils', On 2016/09/12 02:11:01, kwiberg-webrtc wrote: > Why not name this split_by_samples? I ... don't know. I thought it might become a place for similar, internal functionality but this name isn't very good for that either. I'm not even sure it should be put in a separate build target, but I needed to put it somewhere. If it's to be kept in a separate target, it should probably be names split_by_samples for now. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_decoder.cc (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.cc:101: RTC_CHECK(split.byte_offset + split.num_bytes <= payload->size()); On 2016/09/09 12:52:52, hlundin-webrtc wrote: > RTC_CHECK_LE Acknowledged. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.cc:109: } On 2016/09/12 02:11:01, kwiberg-webrtc wrote: > If you processed the pieces in the reverse order, could you steal the buffer > from the first (but last to be processed) piece? That would both let you > eliminate the special case for size() == 1 and apply the corresponding > optimization to the size() > 1 case. It doesn't really matter which of the packets get the stolen payload. I can take a look at an optimization where the last split always steals the payload, though. However, I see this as temporary code. Once all the restructuring CLs have landed, there will be a pass where I updated all of the codecs individually, rather than relying on this base class. Then, they can provide a more fitting implementation without going through the SplitAudio interface. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_decoder.h (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.h:133: rtc::ArrayView<const uint8_t> payload) const; On 2016/09/12 02:11:01, kwiberg-webrtc wrote: > If you turned the return value into an output argument, you would (with the > cooperation of the caller) not have to pay for a heap allocation and > deallocation for each call. I've addressed this in the comment on SplitBySamples. See below! https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc:55: std::vector<PacketSplit> splits; On 2016/09/12 02:11:01, kwiberg-webrtc wrote: > Allocate this one much closer to first point of use? Acknowledged. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc:58: if (payload.size() >= 950) { On 2016/09/12 02:11:01, kwiberg-webrtc wrote: > As I think I've noted before, the only sizes that are actually problematic are > those that are zero both mod 38 and mod 50... so you could write this as > > const bool frame_size_20ms = payload.size() % 38 == 0; > const bool frame_size_30ms = payload.size() % 50 == 0; > if (frame_size_20ms && frame_size_30ms) { > RTC_DCHECK_GE(payload.size(), 950); // LCM(38, 50) = 950. > // handle error > } else if (frame_size_20ms) { > // ... > } else if (frame_size_30ms) { > // ... > } else { > // handle error > } > > This way, it's more obvious that you're simple handling all four possible cases. > > But yes, I expect that towards the end of the CL, I'll discover that you've just > moved this piece of code from somewhere else, so never mind... Yes, that is the case. If acceptable, I'd like to defer that change to a separate CL down the line, so that I try to avoid introducing different behavior with this CL, rather than "just" restructuring. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc:78: } On 2016/09/12 02:11:01, kwiberg-webrtc wrote: > Why is this special case needed? Doesn't the loop below handle this case just > fine? It does. You mentioned that on the previous CL and I just plum forgot to fix that for this one. I'm sorry! :( https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc:81: for (size_t len = payload.size(); len > 0; len -= bytes_per_frame) { On 2016/09/12 02:11:01, kwiberg-webrtc wrote: > Better DCHECK that payload.size() % bytes_per_frame == 0 before this loop, since > that's crucial. Hmm. I see the problem but I'm not sure that's how to best solve it. I'll rewrite. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/ilbc/ilbc_unittest.cc (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/ilbc/ilbc_unittest.cc:70: // Test splitting sample-based payloads. On 2016/09/09 12:52:52, hlundin-webrtc wrote: > iLBC does not split sample-based. Nope! The comment is an old one but it's still wrong. I'll fix! https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/ilbc/ilbc_unittest.cc:74: size_t payload_length_bytes = frame_length_bytes_ * num_frames_; On 2016/09/12 02:11:01, kwiberg-webrtc wrote: > const? Acknowledged. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/ilbc/ilbc_unittest.cc:118: size_t kPayloadLengthBytes = 950; On 2016/09/12 02:11:01, kwiberg-webrtc wrote: > constexpr Acknowledged. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/ilbc/ilbc_unittest.cc:119: rtc::Buffer payload(kPayloadLengthBytes); On 2016/09/12 02:11:01, kwiberg-webrtc wrote: > const Acknowledged. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/ilbc/ilbc_unittest.cc:120: auto splits = decoder.SplitPacket(payload); On 2016/09/12 02:11:01, kwiberg-webrtc wrote: > const Acknowledged. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/ilbc/ilbc_unittest.cc:129: auto splits = decoder.SplitPacket(payload); On 2016/09/12 02:11:01, kwiberg-webrtc wrote: > constexpr, const, const Acknowledged. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/pcm16b/audio_decoder_pcm16b.cc (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/pcm16b/audio_decoder_pcm16b.cc:50: // TODO(ossu): Investigate if we can ever get 44.1KHz audio here, in which On 2016/09/12 02:11:01, kwiberg-webrtc wrote: > On 2016/09/09 12:52:52, hlundin-webrtc wrote: > > Short answer: no. If codecs should be allowed to return something else than > {8, > > 16, 32, 48} kHz, we've got plenty of bigger fish to fry. But hey, feel free to > > add that CheckedDivExact. > > > > Nit: kHz with small 'k'. > > Also, space between the number and the unit. But I guess it doesn't matter since > this comment is going away... The comment is only here for this to get caught in the review, so no, it doesn't really matter. I'll put the CheckedDivExact in and annotate why. And then I'll make sure I spell 44.1 kHz correctly. :) https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/split_by_samples.cc (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/split_by_samples.cc:26: size_t min_chunk_size = bytes_per_ms * 20; On 2016/09/12 02:11:01, kwiberg-webrtc wrote: > const Acknowledged. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/split_by_samples.cc:33: uint32_t timestamps_per_chunk = static_cast<uint32_t>( On 2016/09/12 02:11:01, kwiberg-webrtc wrote: > const Acknowledged. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/split_by_samples.cc:37: for (len = payload.size(); On 2016/09/12 02:11:01, kwiberg-webrtc wrote: > You initialize twice. Would you look at that! https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/split_by_samples.h (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/split_by_samples.h:21: namespace internal { On 2016/09/12 02:11:02, kwiberg-webrtc wrote: > Why put this in the "internal" namespace? It isn't exposed in public headers (is > it?), and we're not in the habit of putting non-public things in general in this > namespace. Hmm, that's true. I wanted to make sure this was non-public, so that it didn't get relied on by external parties. I want it to be used by our existing decoders for now, but to revisit and possibly get rid of it completely once the restructuring is gone. I can live with having it in webrtc directly, though. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/split_by_samples.h:31: uint32_t timestamps_per_ms); On 2016/09/12 02:11:02, kwiberg-webrtc wrote: > Consider having an output argument instead of a return value to enable reuse of > the heap allocation for the vector. I don't like using output arguments in general, since it leads to more convoluted code, both for the caller and the implementation. I don't believe this is a case where an allocation is going to be costly (it'll be a short vector of small objects), so it should be handled quite efficiently. If it turns out that SplitBySamples is actually much more costly due to this, then I'd reconsider. To get around the heap allocation here, you'd not only need to push the allocation of the object to the caller, but it would have to retain that object over time as some sort of working buffer. I'd rather we look at getting a vector-type container with small-size optimization in WebRTC. That would allow us to use more natural syntax without paying the overhead of heap allocations for small sets of data like these. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/split_by_samples_unittest.cc (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/split_by_samples_unittest.cc:83: }; On 2016/09/12 02:11:02, kwiberg-webrtc wrote: > Why use the NetEqDecoder enum, instead of feeding the test the numerical values > directly? And why test only the values used by our current codecs? I took the test as much as possible as it was. My goal was to keep the same test coverage as we had before. I'm not sure that's a good enough reason, though. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/split_by_samples_unittest.cc:100: ExpectedSplit expected_splits[] = { On 2016/09/12 02:11:02, kwiberg-webrtc wrote: > constexpr Sure, but does it really matter? I mean, it's not externally accessible. Do you expect the compiler might do something better if we tell it this is constexpr? I'm not saying you're wrong, I'm just interested in what would be gained from it in cases like these. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/split_by_samples_unittest.cc:112: // int expected_num_packets[] = {1, 1, 1, 2, 2, 2}; On 2016/09/12 02:11:02, kwiberg-webrtc wrote: > Are these useful or not? Useful during review to make sure the values above match the "old way" of declaring that information, but shouldn't be kept when landing. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/split_by_samples_unittest.cc:117: size_t payload_size_bytes = expected_split.payload_size_ms * bytes_per_ms_; On 2016/09/12 02:11:02, kwiberg-webrtc wrote: > const Acknowledged. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/split_by_samples_unittest.cc:118: rtc::Buffer payload(payload_size_bytes); On 2016/09/12 02:11:02, kwiberg-webrtc wrote: > const Acknowledged. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/split_by_samples_unittest.cc:119: auto splits = On 2016/09/12 02:11:02, kwiberg-webrtc wrote: > const Acknowledged.
https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_decoder.cc (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.cc:109: } On 2016/09/12 11:26:37, ossu wrote: > On 2016/09/12 02:11:01, kwiberg-webrtc wrote: > > If you processed the pieces in the reverse order, could you steal the buffer > > from the first (but last to be processed) piece? That would both let you > > eliminate the special case for size() == 1 and apply the corresponding > > optimization to the size() > 1 case. > > It doesn't really matter which of the packets get the stolen payload. Well, if the first one gets it, you won't have to move its contents, just truncate it. > I can take > a look at an optimization where the last split always steals the payload, > though. However, I see this as temporary code. Once all the restructuring CLs > have landed, there will be a pass where I updated all of the codecs > individually, rather than relying on this base class. Then, they can provide a > more fitting implementation without going through the SplitAudio interface. OK, then this is good enough for now. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc:58: if (payload.size() >= 950) { On 2016/09/12 11:26:37, ossu wrote: > On 2016/09/12 02:11:01, kwiberg-webrtc wrote: > > As I think I've noted before, the only sizes that are actually problematic are > > those that are zero both mod 38 and mod 50... so you could write this as > > > > const bool frame_size_20ms = payload.size() % 38 == 0; > > const bool frame_size_30ms = payload.size() % 50 == 0; > > if (frame_size_20ms && frame_size_30ms) { > > RTC_DCHECK_GE(payload.size(), 950); // LCM(38, 50) = 950. > > // handle error > > } else if (frame_size_20ms) { > > // ... > > } else if (frame_size_30ms) { > > // ... > > } else { > > // handle error > > } > > > > This way, it's more obvious that you're simple handling all four possible > cases. > > > > But yes, I expect that towards the end of the CL, I'll discover that you've > just > > moved this piece of code from somewhere else, so never mind... > > Yes, that is the case. If acceptable, I'd like to defer that change to a > separate CL down the line, so that I try to avoid introducing different behavior > with this CL, rather than "just" restructuring. Acknowledged. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc:78: } On 2016/09/12 11:26:37, ossu wrote: > On 2016/09/12 02:11:01, kwiberg-webrtc wrote: > > Why is this special case needed? Doesn't the loop below handle this case just > > fine? > > It does. You mentioned that on the previous CL and I just plum forgot to fix > that for this one. I'm sorry! :( Nice to know that even if my memory isn't what it used to be IIRC, other parts of my brain are still reliable! https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc:81: for (size_t len = payload.size(); len > 0; len -= bytes_per_frame) { On 2016/09/12 11:26:37, ossu wrote: > On 2016/09/12 02:11:01, kwiberg-webrtc wrote: > > Better DCHECK that payload.size() % bytes_per_frame == 0 before this loop, > since > > that's crucial. > > Hmm. I see the problem but I'm not sure that's how to best solve it. I'll > rewrite. I don't think you need to do that. Just a DCHECK immediately before the loop to remind the reader of the invariant that makes the loop safe should be enough. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/pcm16b/audio_decoder_pcm16b.cc (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/pcm16b/audio_decoder_pcm16b.cc:50: // TODO(ossu): Investigate if we can ever get 44.1KHz audio here, in which On 2016/09/12 11:26:37, ossu wrote: > On 2016/09/12 02:11:01, kwiberg-webrtc wrote: > > On 2016/09/09 12:52:52, hlundin-webrtc wrote: > > > Short answer: no. If codecs should be allowed to return something else than > > {8, > > > 16, 32, 48} kHz, we've got plenty of bigger fish to fry. But hey, feel free > to > > > add that CheckedDivExact. > > > > > > Nit: kHz with small 'k'. > > > > Also, space between the number and the unit. But I guess it doesn't matter > since > > this comment is going away... > > The comment is only here for this to get caught in the review, so no, it doesn't > really matter. I'll put the CheckedDivExact in and annotate why. And then I'll > make sure I spell 44.1 kHz correctly. :) Well, if you didn't want pedantic complaints about your spelling, you asked the wrong two people to review. :-) https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/split_by_samples.h (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/split_by_samples.h:21: namespace internal { On 2016/09/12 11:26:37, ossu wrote: > On 2016/09/12 02:11:02, kwiberg-webrtc wrote: > > Why put this in the "internal" namespace? It isn't exposed in public headers > (is > > it?), and we're not in the habit of putting non-public things in general in > this > > namespace. > > Hmm, that's true. I wanted to make sure this was non-public, so that it didn't > get relied on by external parties. I want it to be used by our existing decoders > for now, but to revisit and possibly get rid of it completely once the > restructuring is gone. I can live with having it in webrtc directly, though. Ack. Hmmm. I wonder if it would be more useful than inconvenient to put *everything* that isn't intended to be public in a namespace called "internal" or "impl" or something. If I'm not mistaken, we wouldn't have to actually write internal:: in all that many places. Of course, we should probably make sure we have only one top-level namespace before doing that. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/split_by_samples.h:31: uint32_t timestamps_per_ms); On 2016/09/12 11:26:37, ossu wrote: > On 2016/09/12 02:11:02, kwiberg-webrtc wrote: > > Consider having an output argument instead of a return value to enable reuse > of > > the heap allocation for the vector. > > I don't like using output arguments in general, since it leads to more > convoluted code, both for the caller and the implementation. True. > I don't believe > this is a case where an allocation is going to be costly (it'll be a short > vector of small objects), so it should be handled quite efficiently. I think it's doing an allocation at all that's the issue, not how big it is. The heap metadata has to be fetched, searched, and updated, and we probably have to take locks too. > If it turns > out that SplitBySamples is actually much more costly due to this, then I'd > reconsider. > > To get around the heap allocation here, you'd not only need to push the > allocation of the object to the caller, but it would have to retain that object > over time as some sort of working buffer. Yes. > I'd rather we look at getting a > vector-type container with small-size optimization in WebRTC. That would allow > us to use more natural syntax without paying the overhead of heap allocations > for small sets of data like these. I don't really agree. Small-size optimizations are nice in that they can often speed things up, but since the cutoff size isn't something you can rely on, the optimization may fail to apply in any specific case. The only thing I know of that solves the problem is some sort of pool, whether at the level of objects (as in my recent proof-of-concept CL) or raw memory (such as the Allocator argument to standard library containers). An output argument is for all intents and purposes a simple object pool of size 1, whose lifetime is managed by the caller. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/split_by_samples_unittest.cc (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/split_by_samples_unittest.cc:100: ExpectedSplit expected_splits[] = { On 2016/09/12 11:26:38, ossu wrote: > On 2016/09/12 02:11:02, kwiberg-webrtc wrote: > > constexpr > > Sure, but does it really matter? I mean, it's not externally accessible. Do you > expect the compiler might do something better if we tell it this is constexpr? > I'm not saying you're wrong, I'm just interested in what would be gained from it > in cases like these. Documentation. The important step is making it const; the reader can then tell at a glance that this variable is read-only. Taking the next step to constexpr makes it so that the reader can tell at a glance that this really is a compile-time constant (arguably trivial to do anyway in this particular case, but still...). https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/split_by_samples_unittest.cc:112: // int expected_num_packets[] = {1, 1, 1, 2, 2, 2}; On 2016/09/12 11:26:38, ossu wrote: > On 2016/09/12 02:11:02, kwiberg-webrtc wrote: > > Are these useful or not? > > Useful during review to make sure the values above match the "old way" of > declaring that information, but shouldn't be kept when landing. Acknowledged.
On 2016/09/12 11:26:38, ossu wrote: > My biggest concern with this CL is whether it should use the SplitAudio > interface from the previous audio splitting CL at all? There's really no need > to, since it just introduces a temporary interface to support the ParsePayload > interface that is really there to supplant SplitAudio and others. Keeping it is > also the source of some of the concerns in the rest of the CL: adding a build > target just for SplitByAudio, returning a set of splits as a std::vector<> > rather than output parameter and what parameters are sent in to test > SplitBySamples. I'm not sure putting the sample-splitting code into each decoder > is the way to go either, though, but maybe SplitBySamples should be rewritten to > work according to the ParsePayload interface, so there's no glue necessary. > > SplitAudio was, however, reviewed previously and I hoped that would help with > context in this review. I think it makes sense to do this intermediate step, since (1) you're already quite far along, and (2) there should be few interface compatibility problems since you can simply document that external decoders shouldn't implement the new method. > I'm not sure putting the sample-splitting code into each decoder > is the way to go either, though, What would be the alternative to requiring each decoder to be able to split its own packets?
I've reworked packet splitting quite a bit to get rid of the extra step through SplitPacket. LegacyFrame is now LegacyEncodedAudioFrame and not super secret. SplitBySamples has been turned into a factory method that creates a vector of ParseResults with LegacyEncodedAudioFrames in. All sample based encoders now call that in their ParsePayload, rather than implement SplitPacket. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc:55: std::vector<PacketSplit> splits; On 2016/09/12 11:26:37, ossu wrote: > On 2016/09/12 02:11:01, kwiberg-webrtc wrote: > > Allocate this one much closer to first point of use? > > Acknowledged. I looked at it again and it's used five lines down to return an empty result, so I think it's alright. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc:81: for (size_t len = payload.size(); len > 0; len -= bytes_per_frame) { On 2016/09/13 12:23:37, kwiberg-webrtc wrote: > On 2016/09/12 11:26:37, ossu wrote: > > On 2016/09/12 02:11:01, kwiberg-webrtc wrote: > > > Better DCHECK that payload.size() % bytes_per_frame == 0 before this loop, > > since > > > that's crucial. > > > > Hmm. I see the problem but I'm not sure that's how to best solve it. I'll > > rewrite. > > I don't think you need to do that. Just a DCHECK immediately before the loop to > remind the reader of the invariant that makes the loop safe should be enough. Acknowledged. https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/pcm16b/audio_decoder_pcm16b.cc (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/pcm16b/audio_decoder_pcm16b.cc:50: // TODO(ossu): Investigate if we can ever get 44.1KHz audio here, in which On 2016/09/13 12:23:37, kwiberg-webrtc wrote: > On 2016/09/12 11:26:37, ossu wrote: > > On 2016/09/12 02:11:01, kwiberg-webrtc wrote: > > > On 2016/09/09 12:52:52, hlundin-webrtc wrote: > > > > Short answer: no. If codecs should be allowed to return something else > than > > > {8, > > > > 16, 32, 48} kHz, we've got plenty of bigger fish to fry. But hey, feel > free > > to > > > > add that CheckedDivExact. > > > > > > > > Nit: kHz with small 'k'. > > > > > > Also, space between the number and the unit. But I guess it doesn't matter > > since > > > this comment is going away... > > > > The comment is only here for this to get caught in the review, so no, it > doesn't > > really matter. I'll put the CheckedDivExact in and annotate why. And then I'll > > make sure I spell 44.1 kHz correctly. :) > > Well, if you didn't want pedantic complaints about your spelling, you asked the > wrong two people to review. :-) Acknowledged. https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc:62: LOG(LS_WARNING) << "AudioDecoderIlbc::SplitPacket: Payload too large"; This comment should say ParsePayload and not SplitPacket. https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc:74: LOG(LS_WARNING) << "AudioDecoderIlbc::SplitPacket: Invalid payload"; This comment should say ParsePayload and not SplitPacket. https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/ilbc/ilbc_unittest.cc (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/ilbc/ilbc_unittest.cc:131: rtc::Buffer payload(kPayloadLengthBytes); Since it's now moved from by ParsePayload, it can't be const.
On 2016/09/13 12:29:03, kwiberg-webrtc wrote: > What would be the alternative to requiring each decoder to be able to split its > own packets? I meant having to re-implement the SplitBySamples logic in each decoder. The latest patch set addresses this in what I think is a better way.
https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.h (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.h:28: static std::vector<AudioDecoder::ParseResult> SplitBySamples( I've kept this as SplitBySamples for now, but it could have a more descriptive name, such as: CreateFromSampleBasedPayload or something.
https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc:78: } On 2016/09/13 12:23:37, kwiberg-webrtc wrote: > On 2016/09/12 11:26:37, ossu wrote: > > On 2016/09/12 02:11:01, kwiberg-webrtc wrote: > > > Why is this special case needed? Doesn't the loop below handle this case > just > > > fine? > > > > It does. You mentioned that on the previous CL and I just plum forgot to fix > > that for this one. I'm sorry! :( > > Nice to know that even if my memory isn't what it used to be IIRC, other parts > of my brain are still reliable! If you recall correctly, your memory is not what is used to be. Meta... https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/pcm16b/audio_decoder_pcm16b.cc (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/pcm16b/audio_decoder_pcm16b.cc:50: // TODO(ossu): Investigate if we can ever get 44.1KHz audio here, in which On 2016/09/13 12:23:37, kwiberg-webrtc wrote: > On 2016/09/12 11:26:37, ossu wrote: > > On 2016/09/12 02:11:01, kwiberg-webrtc wrote: > > > On 2016/09/09 12:52:52, hlundin-webrtc wrote: > > > > Short answer: no. If codecs should be allowed to return something else > than > > > {8, > > > > 16, 32, 48} kHz, we've got plenty of bigger fish to fry. But hey, feel > free > > to > > > > add that CheckedDivExact. > > > > > > > > Nit: kHz with small 'k'. > > > > > > Also, space between the number and the unit. But I guess it doesn't matter > > since > > > this comment is going away... > > > > The comment is only here for this to get caught in the review, so no, it > doesn't > > really matter. I'll put the CheckedDivExact in and annotate why. And then I'll > > make sure I spell 44.1 kHz correctly. :) > > Well, if you didn't want pedantic complaints about your spelling, you asked the > wrong two people to review. :-) :) https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc:78: RTC_DCHECK(payload->size() % bytes_per_frame == 0); RTC_DCHECK_EQ https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.h (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.h:19: class SplitIlbcTest; Say what? https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/ilbc/ilbc_unittest.cc (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/ilbc/ilbc_unittest.cc:131: rtc::Buffer payload(kPayloadLengthBytes); On 2016/09/13 14:25:55, ossu wrote: > Since it's now moved from by ParsePayload, it can't be const. Acknowledged. https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc:20: rtc::Buffer* payload, As discussed offline, and simply for the record, I'd prefer to use an rvalue ref here. But, style guide says no. https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc:80: split_size_bytes >>= 1; We could get rid of this very questionable "optimization" and simply /= 2 instead. https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.h (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.h:28: static std::vector<AudioDecoder::ParseResult> SplitBySamples( On 2016/09/13 14:28:29, ossu wrote: > I've kept this as SplitBySamples for now, but it could have a more descriptive > name, such as: CreateFromSampleBasedPayload or something. I think SplitBySamples is good enough.
https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc:78: RTC_DCHECK(payload->size() % bytes_per_frame == 0); On 2016/09/15 08:49:14, hlundin-webrtc wrote: > RTC_DCHECK_EQ I did that initially, but the RTC_DHECK_EQ macro barfed on me, whining about comparing a size_t (payload->size() % bytes_per_frame) to an int (0). I could get around it by typing the zero, but it's a bit longer. I'll do that, though, so the DCHECK message printout is a bit better. https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.h (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.h:19: class SplitIlbcTest; On 2016/09/15 08:49:14, hlundin-webrtc wrote: > Say what? Ah. That's a left-over from me attempting to do tests by adding the test as a friend. I'll remove it. https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc:20: rtc::Buffer* payload, On 2016/09/15 08:49:14, hlundin-webrtc wrote: > As discussed offline, and simply for the record, I'd prefer to use an rvalue ref > here. But, style guide says no. I'd like that too. It's almost (but not completely) without precedent in WebRTC code. A couple of constructors take an rvalue reference and some other parameters, but it's far from the norm. https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc:80: split_size_bytes >>= 1; On 2016/09/15 08:49:14, hlundin-webrtc wrote: > We could get rid of this very questionable "optimization" and simply /= 2 > instead. Acknowledged.
https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc:20: rtc::Buffer* payload, On 2016/09/15 08:58:11, ossu wrote: > On 2016/09/15 08:49:14, hlundin-webrtc wrote: > > As discussed offline, and simply for the record, I'd prefer to use an rvalue > ref > > here. But, style guide says no. > > I'd like that too. It's almost (but not completely) without precedent in WebRTC > code. A couple of constructors take an rvalue reference and some other > parameters, but it's far from the norm. Pass it by value, then. For cheap-to-move move-only types (such as std::unique_ptr and rtc::Buffer) pass-by-value is a good substitute for pass-by-rvalue-reference. I still agree that passing by rvalue reference would be better here, since it may not be well known that rtc::Buffer isn't copyable, but if you'd rather follow the style guide than write easy-to-understand code, pass-by-value will work nicely here.
https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc:20: rtc::Buffer* payload, On 2016/09/15 09:03:43, kwiberg-webrtc wrote: > On 2016/09/15 08:58:11, ossu wrote: > > On 2016/09/15 08:49:14, hlundin-webrtc wrote: > > > As discussed offline, and simply for the record, I'd prefer to use an rvalue > > ref > > > here. But, style guide says no. > > > > I'd like that too. It's almost (but not completely) without precedent in > WebRTC > > code. A couple of constructors take an rvalue reference and some other > > parameters, but it's far from the norm. > > Pass it by value, then. > > For cheap-to-move move-only types (such as std::unique_ptr and rtc::Buffer) > pass-by-value is a good substitute for pass-by-rvalue-reference. I still agree > that passing by rvalue reference would be better here, since it may not be well > known that rtc::Buffer isn't copyable, but if you'd rather follow the style > guide than write easy-to-understand code, pass-by-value will work nicely here. Of course I don't _prefer_ the style guide over easy-to-understand code. I personally think references most places we now have pointers would be clearer of intent. The style guide does, however, work as some sort of arbiter over what is easy to understand, I guess. We can probably get away with putting an rvalue reference in here, since it's a constructor and it's a bit tucked away. I've realized this discussion should also encompass AudioDecoder::ParsePayload. Since that method is intended to steal the data, we should find a clear and uniform way of indicating that in future interfaces. Passing rtc::Buffers by-value might be the only currently acceptable variant.
https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc:20: rtc::Buffer* payload, On 2016/09/15 09:15:13, ossu wrote: > On 2016/09/15 09:03:43, kwiberg-webrtc wrote: > > On 2016/09/15 08:58:11, ossu wrote: > > > On 2016/09/15 08:49:14, hlundin-webrtc wrote: > > > > As discussed offline, and simply for the record, I'd prefer to use an > rvalue > > > ref > > > > here. But, style guide says no. > > > > > > I'd like that too. It's almost (but not completely) without precedent in > > WebRTC > > > code. A couple of constructors take an rvalue reference and some other > > > parameters, but it's far from the norm. > > > > Pass it by value, then. > > > > For cheap-to-move move-only types (such as std::unique_ptr and rtc::Buffer) > > pass-by-value is a good substitute for pass-by-rvalue-reference. I still agree > > that passing by rvalue reference would be better here, since it may not be > well > > known that rtc::Buffer isn't copyable, but if you'd rather follow the style > > guide than write easy-to-understand code, pass-by-value will work nicely here. > > Of course I don't _prefer_ the style guide over easy-to-understand code. I > personally think references most places we now have pointers would be clearer of > intent. The style guide does, however, work as some sort of arbiter over what is > easy to understand, I guess. > > We can probably get away with putting an rvalue reference in here, since it's a > constructor and it's a bit tucked away. I've realized this discussion should > also encompass AudioDecoder::ParsePayload. Since that method is intended to > steal the data, we should find a clear and uniform way of indicating that in > future interfaces. Passing rtc::Buffers by-value might be the only currently > acceptable variant. Sorry if I sounded a bit aggressive. But it is the case here that following the style guide's prohibition of rvalue reference parameters leads to harder-to-understand code, so you do have to make a choice. As for the larger issue: the prohibition of mutable lvalue reference parameters is distinct from the prohibition of rvalue reference parameters in my opinion, since the rationales are different. The former is forbidden because it's hard to tell at the call site that the callee may mutate the argument, whereas the latter has no such problem, but is instead prohibited on the theory that rvalue references are too unfamiliar to people. My position is that the lvalue ref prohibition is good or at least OK, but that the rvalue ref prohibition should be abolished because the increasingly small readability win doesn't outweigh the cost.
https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc:78: RTC_DCHECK(payload->size() % bytes_per_frame == 0); On 2016/09/15 08:58:11, ossu wrote: > On 2016/09/15 08:49:14, hlundin-webrtc wrote: > > RTC_DCHECK_EQ > > I did that initially, but the RTC_DHECK_EQ macro barfed on me, whining about > comparing a size_t (payload->size() % bytes_per_frame) to an int (0). I could > get around it by typing the zero, but it's a bit longer. I'll do that, though, > so the DCHECK message printout is a bit better. I hate it when that happens, but I prefer the nice printouts. https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.h (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.h:19: class SplitIlbcTest; On 2016/09/15 08:58:11, ossu wrote: > On 2016/09/15 08:49:14, hlundin-webrtc wrote: > > Say what? > > Ah. That's a left-over from me attempting to do tests by adding the test as a > friend. I'll remove it. FRIEND_TEST or FRIEND_TEST_ALL_PREFIXES is your... errr... friend. https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc:20: rtc::Buffer* payload, On 2016/09/15 09:29:53, kwiberg-webrtc wrote: > On 2016/09/15 09:15:13, ossu wrote: > > On 2016/09/15 09:03:43, kwiberg-webrtc wrote: > > > On 2016/09/15 08:58:11, ossu wrote: > > > > On 2016/09/15 08:49:14, hlundin-webrtc wrote: > > > > > As discussed offline, and simply for the record, I'd prefer to use an > > rvalue > > > > ref > > > > > here. But, style guide says no. > > > > > > > > I'd like that too. It's almost (but not completely) without precedent in > > > WebRTC > > > > code. A couple of constructors take an rvalue reference and some other > > > > parameters, but it's far from the norm. > > > > > > Pass it by value, then. > > > > > > For cheap-to-move move-only types (such as std::unique_ptr and rtc::Buffer) > > > pass-by-value is a good substitute for pass-by-rvalue-reference. I still > agree > > > that passing by rvalue reference would be better here, since it may not be > > well > > > known that rtc::Buffer isn't copyable, but if you'd rather follow the style > > > guide than write easy-to-understand code, pass-by-value will work nicely > here. > > > > Of course I don't _prefer_ the style guide over easy-to-understand code. I > > personally think references most places we now have pointers would be clearer > of > > intent. The style guide does, however, work as some sort of arbiter over what > is > > easy to understand, I guess. > > > > We can probably get away with putting an rvalue reference in here, since it's > a > > constructor and it's a bit tucked away. I've realized this discussion should > > also encompass AudioDecoder::ParsePayload. Since that method is intended to > > steal the data, we should find a clear and uniform way of indicating that in > > future interfaces. Passing rtc::Buffers by-value might be the only currently > > acceptable variant. > > Sorry if I sounded a bit aggressive. But it is the case here that following the > style guide's prohibition of rvalue reference parameters leads to > harder-to-understand code, so you do have to make a choice. > > As for the larger issue: the prohibition of mutable lvalue reference parameters > is distinct from the prohibition of rvalue reference parameters in my opinion, > since the rationales are different. The former is forbidden because it's hard to > tell at the call site that the callee may mutate the argument, whereas the > latter has no such problem, but is instead prohibited on the theory that rvalue > references are too unfamiliar to people. My position is that the lvalue ref > prohibition is good or at least OK, but that the rvalue ref prohibition should > be abolished because the increasingly small readability win doesn't outweigh the > cost. I tricked you into discussing the merits of the style guide and C++11 features. My work here is done.
https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc:20: rtc::Buffer* payload, On 2016/09/15 09:43:38, hlundin-webrtc wrote: > On 2016/09/15 09:29:53, kwiberg-webrtc wrote: > > Sorry if I sounded a bit aggressive. But it is the case here that following the > > style guide's prohibition of rvalue reference parameters leads to > > harder-to-understand code, so you do have to make a choice. > > > > As for the larger issue: the prohibition of mutable lvalue reference parameters > > is distinct from the prohibition of rvalue reference parameters in my opinion, > > since the rationales are different. The former is forbidden because it's hard to > > tell at the call site that the callee may mutate the argument, whereas the > > latter has no such problem, but is instead prohibited on the theory that rvalue > > references are too unfamiliar to people. My position is that the lvalue ref > > prohibition is good or at least OK, but that the rvalue ref prohibition should > > be abolished because the increasingly small readability win doesn't outweigh > > the cost. So are we saying rtc::Buffer&& in LegacyEncodedAudioFrame _and_ in ParsePayload? I'd be OK with that! Having them take a pointer is a bit dangerous. If a subclass would _just_ copy the pointer, it would work for now, but break at some point in the future. Forcing a copy or a move from the Buffer by passing as value or rvalue-reference avoids that. > I tricked you into discussing the merits of the style guide and C++11 features. > My work here is done. Oh, you! *shakes fist*
https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc:20: rtc::Buffer* payload, On 2016/09/15 11:56:21, ossu wrote: > On 2016/09/15 09:43:38, hlundin-webrtc wrote: > > On 2016/09/15 09:29:53, kwiberg-webrtc wrote: > > > Sorry if I sounded a bit aggressive. But it is the case here that following > the > > > style guide's prohibition of rvalue reference parameters leads to > > > harder-to-understand code, so you do have to make a choice. > > > > > > As for the larger issue: the prohibition of mutable lvalue reference > parameters > > > is distinct from the prohibition of rvalue reference parameters in my > opinion, > > > since the rationales are different. The former is forbidden because it's > hard to > > > tell at the call site that the callee may mutate the argument, whereas the > > > latter has no such problem, but is instead prohibited on the theory that > rvalue > > > references are too unfamiliar to people. My position is that the lvalue ref > > > prohibition is good or at least OK, but that the rvalue ref prohibition > should > > > be abolished because the increasingly small readability win doesn't outweigh > > > the cost. > > So are we saying rtc::Buffer&& in LegacyEncodedAudioFrame _and_ in ParsePayload? > I'd be OK with that! Having them take a pointer is a bit dangerous. If a > subclass would _just_ copy the pointer, it would work for now, but break at some > point in the future. Forcing a copy or a move from the Buffer by passing as > value or rvalue-reference avoids that. > > > I tricked you into discussing the merits of the style guide and C++11 > features. > > My work here is done. > > Oh, you! *shakes fist* Yes, I'd say taking an rtc::Buffer&& argument is best. Since we don't have a styleguide owner team or anything like that, there's no one official to request permission from when breaking the rules seems like the best option, so Henrik and I will simply have to promise to look the other way when you land this. :-)
https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_coding.gypi (right): https://codereview.webrtc.org/2326003002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_coding.gypi:190: 'target_name': 'audio_coding_utils', On 2016/09/12 11:26:37, ossu wrote: > On 2016/09/12 02:11:01, kwiberg-webrtc wrote: > > Why not name this split_by_samples? > > I ... don't know. I thought it might become a place for similar, internal > functionality but this name isn't very good for that either. > I'm not even sure it should be put in a separate build target, but I needed to > put it somewhere. If it's to be kept in a separate target, it should probably be > names split_by_samples for now. Good. I think we generally have build targets that are too large. Putting single .cc files in their own build targets probably makes sense in a lot of cases. https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/BUILD.gn File webrtc/modules/BUILD.gn (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/BUILD.gn#n... webrtc/modules/BUILD.gn:244: "audio_coding/:legacy_encoded_audio_frame", Just curious: Is the slash immediately before the colon necessary? https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_decoder.cc (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.cc:45: return results; Won't a construction along these lines work? std::unique_ptr<EncodedAudioFrame> frame( new LegacyEncodedAudioFrame(this, payload, is_primary)); return {{timestamp, is_primary, std::move(frame)}}; https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_decoder.h (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.h:40: class EncodedAudioFrame { You don't have to change this, but to me this name doesn't imply that it's a decoder. Maybe AudioFrameDecoder? AudioFrameWithDecoder? SelfDecodingAudioFrame? :-) https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.h:56: // speech. Decode must only be called once per frame object. Even more specifically: 0 or 1 times. https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc:78: RTC_DCHECK(payload->size() % bytes_per_frame == 0); On 2016/09/15 09:43:38, hlundin-webrtc wrote: > On 2016/09/15 08:58:11, ossu wrote: > > On 2016/09/15 08:49:14, hlundin-webrtc wrote: > > > RTC_DCHECK_EQ > > > > I did that initially, but the RTC_DHECK_EQ macro barfed on me, whining about > > comparing a size_t (payload->size() % bytes_per_frame) to an int (0). I could > > get around it by typing the zero, but it's a bit longer. I'll do that, though, > > so the DCHECK message printout is a bit better. > > I hate it when that happens, but I prefer the nice printouts. +1. It's annoying, but since a "u" suffix will solve the problem, I've never tried to fix it for real. https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/ilbc/ilbc_unittest.cc (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/ilbc/ilbc_unittest.cc:81: auto results = decoder.ParsePayload(&payload, 0, true); const https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc:80: split_size_bytes >>= 1; On 2016/09/15 08:49:14, hlundin-webrtc wrote: > We could get rid of this very questionable "optimization" and simply /= 2 > instead. +1. split_size_bytes is unsigned, so the compiler will generate the same code anyway. https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame_unittest.cc (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame_unittest.cc:127: value = 0; It's easier to read if you don't reuse variables.
https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/BUILD.gn File webrtc/modules/BUILD.gn (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/BUILD.gn#n... webrtc/modules/BUILD.gn:244: "audio_coding/:legacy_encoded_audio_frame", On 2016/09/15 13:01:24, kwiberg-webrtc wrote: > Just curious: Is the slash immediately before the colon necessary? Just checked: No! :O https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_decoder.cc (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.cc:45: return results; On 2016/09/15 13:01:24, kwiberg-webrtc wrote: > Won't a construction along these lines work? > > std::unique_ptr<EncodedAudioFrame> frame( > new LegacyEncodedAudioFrame(this, payload, is_primary)); > return {{timestamp, is_primary, std::move(frame)}}; Ideally that _should_ work but I can't get it to. I wonder if the initializer list constructor is somehow disabled, seeing as Chromium doesn't allow initializer lists. :/ https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_decoder.h (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.h:40: class EncodedAudioFrame { On 2016/09/15 13:01:25, kwiberg-webrtc wrote: > You don't have to change this, but to me this name doesn't imply that it's a > decoder. Maybe AudioFrameDecoder? AudioFrameWithDecoder? SelfDecodingAudioFrame? > :-) I'd argue that it isn't a decoder, but a frame that can be decoded. It still relies on having an underlying decoder (which potentially has state), which is why we (now) promise in the ParsePayload comment that the AudioDecoder object will outlive every frame from that encoder. https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.h:56: // speech. Decode must only be called once per frame object. On 2016/09/15 13:01:24, kwiberg-webrtc wrote: > Even more specifically: 0 or 1 times. Took care of that in an update to a previous CL. https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc:20: rtc::Buffer* payload, On 2016/09/15 12:06:43, kwiberg-webrtc wrote: > On 2016/09/15 11:56:21, ossu wrote: > > On 2016/09/15 09:43:38, hlundin-webrtc wrote: > > > On 2016/09/15 09:29:53, kwiberg-webrtc wrote: > > > > Sorry if I sounded a bit aggressive. But it is the case here that > following > > the > > > > style guide's prohibition of rvalue reference parameters leads to > > > > harder-to-understand code, so you do have to make a choice. > > > > > > > > As for the larger issue: the prohibition of mutable lvalue reference > > parameters > > > > is distinct from the prohibition of rvalue reference parameters in my > > opinion, > > > > since the rationales are different. The former is forbidden because it's > > hard to > > > > tell at the call site that the callee may mutate the argument, whereas the > > > > latter has no such problem, but is instead prohibited on the theory that > > rvalue > > > > references are too unfamiliar to people. My position is that the lvalue > ref > > > > prohibition is good or at least OK, but that the rvalue ref prohibition > > should > > > > be abolished because the increasingly small readability win doesn't > outweigh > > > > the cost. > > > > So are we saying rtc::Buffer&& in LegacyEncodedAudioFrame _and_ in > ParsePayload? > > I'd be OK with that! Having them take a pointer is a bit dangerous. If a > > subclass would _just_ copy the pointer, it would work for now, but break at > some > > point in the future. Forcing a copy or a move from the Buffer by passing as > > value or rvalue-reference avoids that. > > > > > I tricked you into discussing the merits of the style guide and C++11 > > features. > > > My work here is done. > > > > Oh, you! *shakes fist* > > Yes, I'd say taking an rtc::Buffer&& argument is best. Since we don't have a > styleguide owner team or anything like that, there's no one official to request > permission from when breaking the rules seems like the best option, so Henrik > and I will simply have to promise to look the other way when you land this. :-) FOR GLORY! https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.h (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.h:28: static std::vector<AudioDecoder::ParseResult> SplitBySamples( On 2016/09/15 08:49:14, hlundin-webrtc wrote: > On 2016/09/13 14:28:29, ossu wrote: > > I've kept this as SplitBySamples for now, but it could have a more descriptive > > name, such as: CreateFromSampleBasedPayload or something. > > I think SplitBySamples is good enough. Acknowledged. https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame_unittest.cc (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame_unittest.cc:127: value = 0; On 2016/09/15 13:01:25, kwiberg-webrtc wrote: > It's easier to read if you don't reuse variables. It's easier to write if I do. ;) Just kidding. With this change, neither payload nor value need to leak from the setup phase.
lgtm https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/BUILD.gn File webrtc/modules/BUILD.gn (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/BUILD.gn#n... webrtc/modules/BUILD.gn:244: "audio_coding/:legacy_encoded_audio_frame", On 2016/09/15 14:41:05, ossu wrote: > On 2016/09/15 13:01:24, kwiberg-webrtc wrote: > > Just curious: Is the slash immediately before the colon necessary? > > Just checked: No! :O I had a feeling that might be the case---gn also allows you to omit the name of the build target if it's the same as the name of the directory, so clearly they liked shortening the dependency names. https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_decoder.cc (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.cc:45: return results; On 2016/09/15 14:41:05, ossu wrote: > On 2016/09/15 13:01:24, kwiberg-webrtc wrote: > > Won't a construction along these lines work? > > > > std::unique_ptr<EncodedAudioFrame> frame( > > new LegacyEncodedAudioFrame(this, payload, is_primary)); > > return {{timestamp, is_primary, std::move(frame)}}; > > Ideally that _should_ work but I can't get it to. I wonder if the initializer > list constructor is somehow disabled, seeing as Chromium doesn't allow > initializer lists. :/ No, they're allowed now. Could it be that we're using a language whose syntax is more Byzantine than we're comfortable with? https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_decoder.h (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.h:40: class EncodedAudioFrame { On 2016/09/15 14:41:05, ossu wrote: > On 2016/09/15 13:01:25, kwiberg-webrtc wrote: > > You don't have to change this, but to me this name doesn't imply that it's a > > decoder. Maybe AudioFrameDecoder? AudioFrameWithDecoder? > SelfDecodingAudioFrame? > > :-) > > I'd argue that it isn't a decoder, but a frame that can be decoded. It still > relies on having an underlying decoder (which potentially has state), which is > why we (now) promise in the ParsePayload comment that the AudioDecoder object > will outlive every frame from that encoder. Acknowledged. https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.h:56: // speech. Decode must only be called once per frame object. On 2016/09/15 14:41:05, ossu wrote: > On 2016/09/15 13:01:24, kwiberg-webrtc wrote: > > Even more specifically: 0 or 1 times. > > Took care of that in an update to a previous CL. Acknowledged. https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame_unittest.cc (right): https://codereview.webrtc.org/2326003002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame_unittest.cc:127: value = 0; On 2016/09/15 14:41:06, ossu wrote: > On 2016/09/15 13:01:25, kwiberg-webrtc wrote: > > It's easier to read if you don't reuse variables. > > It's easier to write if I do. ;) > Just kidding. With this change, neither payload nor value need to leak from the > setup phase. Nice! https://codereview.webrtc.org/2326003002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/ilbc/ilbc_unittest.cc (right): https://codereview.webrtc.org/2326003002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/ilbc/ilbc_unittest.cc:74: auto generate_payload = [] (size_t payload_length_bytes) { const auto https://codereview.webrtc.org/2326003002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame_unittest.cc (right): https://codereview.webrtc.org/2326003002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame_unittest.cc:113: auto generate_payload = [] (size_t num_bytes) { const auto? https://codereview.webrtc.org/2326003002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/2326003002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl.cc:671: // Reuse the packet if possible End comment with "." Or, for a less minimalist tone, // Reuse the packet if possible! :-P https://codereview.webrtc.org/2326003002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl.cc:675: } Again, should packet->payload (moved from on line 665) be cleared?
LGTM % kwiberg's comments. https://codereview.webrtc.org/2326003002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_decoder.h (right): https://codereview.webrtc.org/2326003002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.h:54: // |decoded| will be large enough for 120 ms of audio at the decoder's "|decoded| will be large enough" This sounds like the Decode nethod will sort that for you. Should you not say that is *must* be large enough?
https://codereview.webrtc.org/2326003002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_decoder.h (right): https://codereview.webrtc.org/2326003002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.h:54: // |decoded| will be large enough for 120 ms of audio at the decoder's On 2016/09/16 07:35:20, hlundin-webrtc wrote: > "|decoded| will be large enough" > This sounds like the Decode nethod will sort that for you. Should you not say > that is *must* be large enough? Yeah, it's always troublesome to document interfaces, since the docs have to work for both sides. So yes, I agree that "must" is a better choice of words.
https://codereview.webrtc.org/2326003002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_decoder.h (right): https://codereview.webrtc.org/2326003002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.h:54: // |decoded| will be large enough for 120 ms of audio at the decoder's On 2016/09/16 07:56:33, kwiberg-webrtc wrote: > On 2016/09/16 07:35:20, hlundin-webrtc wrote: > > "|decoded| will be large enough" > > This sounds like the Decode nethod will sort that for you. Should you not say > > that is *must* be large enough? > > Yeah, it's always troublesome to document interfaces, since the docs have to > work for both sides. So yes, I agree that "must" is a better choice of words. Hmm. It mustn't really, though. Since we have a Duration call, the reasonable request would be to have the caller make sure decoded is large enough for the frame based on the value returned by Decoded. That would let us keep the specifics of frame sizes in the comment describing ParsePayload. I've updated this comment in an earlier CL. https://codereview.webrtc.org/2326003002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/ilbc/ilbc_unittest.cc (right): https://codereview.webrtc.org/2326003002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/ilbc/ilbc_unittest.cc:74: auto generate_payload = [] (size_t payload_length_bytes) { On 2016/09/16 00:48:08, kwiberg-webrtc wrote: > const auto Acknowledged. https://codereview.webrtc.org/2326003002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame_unittest.cc (right): https://codereview.webrtc.org/2326003002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame_unittest.cc:113: auto generate_payload = [] (size_t num_bytes) { On 2016/09/16 00:48:08, kwiberg-webrtc wrote: > const auto? Acknowledged. https://codereview.webrtc.org/2326003002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/2326003002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl.cc:671: // Reuse the packet if possible On 2016/09/16 00:48:08, kwiberg-webrtc wrote: > End comment with "." Or, for a less minimalist tone, > > // Reuse the packet if possible! :-P Monoton und minimal. https://codereview.webrtc.org/2326003002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl.cc:675: } On 2016/09/16 00:48:08, kwiberg-webrtc wrote: > Again, should packet->payload (moved from on line 665) be cleared? I think it's probably best if it's not: once moved from, payload should not be touched. Since we have checks in place for that in Buffer (at least in debug builds), I think it's preferable if they fire when we mess up.
The CQ bit was checked by ossu@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/17699) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_gyp_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gyp_dbg/builds/684) mac_gyp_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gyp_rel/builds/687)
lgtm https://codereview.webrtc.org/2326003002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_decoder.h (right): https://codereview.webrtc.org/2326003002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.h:54: // |decoded| will be large enough for 120 ms of audio at the decoder's On 2016/09/16 11:46:00, ossu wrote: > On 2016/09/16 07:56:33, kwiberg-webrtc wrote: > > On 2016/09/16 07:35:20, hlundin-webrtc wrote: > > > "|decoded| will be large enough" > > > This sounds like the Decode nethod will sort that for you. Should you not > say > > > that is *must* be large enough? > > > > Yeah, it's always troublesome to document interfaces, since the docs have to > > work for both sides. So yes, I agree that "must" is a better choice of words. > > Hmm. It mustn't really, though. Since we have a Duration call, the reasonable > request would be to have the caller make sure decoded is large enough for the > frame based on the value returned by Decoded. That would let us keep the > specifics of frame sizes in the comment describing ParsePayload. I've updated > this comment in an earlier CL. Acknowledged. https://codereview.webrtc.org/2326003002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/2326003002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl.cc:675: } On 2016/09/16 11:46:00, ossu wrote: > On 2016/09/16 00:48:08, kwiberg-webrtc wrote: > > Again, should packet->payload (moved from on line 665) be cleared? > > I think it's probably best if it's not: once moved from, payload should not be > touched. Since we have checks in place for that in Buffer (at least in debug > builds), I think it's preferable if they fire when we mess up. Acknowledged.
Still LGTM.
Description was changed from ========== Moved codec-specific audio packet splitting into decoders. There's still some code run specifically for Opus w/ FEC. It will be addressed in a separate CL. This change uses the SplitPacket functionality from https://codereview.webrtc.org/2281453002/ although through the ParsePayload interface. Ideally this should be placed into ParsePayload directly. BUG=webrtc:5805 ========== to ========== Moved codec-specific audio packet splitting into decoders. There's still some code run specifically for Opus w/ FEC. It will be addressed in a separate CL. BUG=webrtc:5805 ==========
The CQ bit was checked by ossu@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/17936)
The CQ bit was checked by ossu@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by ossu@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2326003002/#ps100001 (title: "Fixed types in packet splitting (size_t vs. uint32_t)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Moved codec-specific audio packet splitting into decoders. There's still some code run specifically for Opus w/ FEC. It will be addressed in a separate CL. BUG=webrtc:5805 ========== to ========== Moved codec-specific audio packet splitting into decoders. There's still some code run specifically for Opus w/ FEC. It will be addressed in a separate CL. BUG=webrtc:5805 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Moved codec-specific audio packet splitting into decoders. There's still some code run specifically for Opus w/ FEC. It will be addressed in a separate CL. BUG=webrtc:5805 ========== to ========== Moved codec-specific audio packet splitting into decoders. There's still some code run specifically for Opus w/ FEC. It will be addressed in a separate CL. BUG=webrtc:5805 Committed: https://crrev.com/0d526d558b0f7b677c43f83e64e67ad65a9639c0 Cr-Commit-Position: refs/heads/master@{#14319} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/0d526d558b0f7b677c43f83e64e67ad65a9639c0 Cr-Commit-Position: refs/heads/master@{#14319} |
