Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2326953003:
Added a ParsePayload method to AudioDecoder. (Closed)
DescriptionAdded a ParsePayload method to AudioDecoder.
It allows the decoder to split the input up into usable chunks before
they are put into NetEq's PacketBuffer. Eventually, all packet splitting
will move into ParsePayload.
There's currently a base implementation of ParsePayload. It will
generate a single Frame that calls the underlying AudioDecoder for
getting Duration() and to Decode.
BUG=webrtc:5805
BUG=chromium:428099
Committed: https://crrev.com/61a208b1b8e88716747971fe4ba1da8ddf521bb1
Cr-Commit-Position: refs/heads/master@{#14300}
Patch Set 1 #
Total comments: 48
Patch Set 2 : Addressed comments #
Total comments: 12
Patch Set 3 : Clarifications. Cleanups. #
Total comments: 12
Patch Set 4 : Clarified comments. #Patch Set 5 : IWYU and other linting issues. #Patch Set 6 : Added PacketBuffer::DiscardPacketsWithPayloadType, so that NetEq can flush packets in RemovePayload… #Patch Set 7 : Added some casts from size_t to int. #
Dependent Patchsets: Messages
Total messages: 40 (19 generated)
ossu@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, kwiberg@webrtc.org
This is the first in a series of CLs to move codec specific stuff from NetEq internals into AudioDecoder. The final CL will also change NetEq to not use the NetEqDecoder enum internally.
Those are awesome changes. See inline for some comments. https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_decoder.cc (right): https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.cc:29: using std::swap; Why the using statement? https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.cc:30: swap(this->payload_, *payload); #include what you use. https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.cc:65: AudioDecoder* decoder_; const https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.cc:67: bool is_primary_payload_; const https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_decoder.h (right): https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.h:39: class Frame { Frame is too general, and already claimed for too many different concepts in audio processing and coding. Consider EncodedFrame. https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.h:52: // Decodes this frame of audio and writes the result in |decoded|. What can be expected of the state of the Frame after having called Decode? Is it dead then? What if I call Decode twice on the same object? https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.h:56: virtual rtc::Optional<DecodeResult> Decode( How will error codes from the decoder be handled? Today we are storing them for later reporting. Will this change? https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.h:68: uint32_t timestamp; rtp_timestamp? Or timestamp_in_samples? G.722... https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/neteq_impl.cc:1947: assert(packet->frame || !packet->payload.empty()); Switch to DCHECK while you're at it. https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/neteq_impl.cc:1967: // TODO(ossu): Is this the correct way to track samples decoded from a Good question. https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc (left): https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:799: // http://crbug.com/428099. You may want to link the CL to this bug.
https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_decoder.cc (right): https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.cc:29: using std::swap; On 2016/09/09 12:11:49, hlundin-webrtc wrote: > Why the using statement? It's the standard idiom for swapping stuff. See e.g. http://en.cppreference.com/w/cpp/concept/Swappable https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.cc:30: swap(this->payload_, *payload); On 2016/09/09 12:11:49, hlundin-webrtc wrote: > #include what you use. +1 https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.cc:31: } Why not move *payload to payload_ instead of swapping? https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.cc:67: bool is_primary_payload_; On 2016/09/09 12:11:49, hlundin-webrtc wrote: > const All three could be const, right? https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_decoder.h (right): https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.h:71: }; Why do you need ParseResult and Frame to be two different classes? ... Ah. So that Frame can be a pure interface. https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.h:78: bool is_primary); "and retain them for as long as necessary" is redundant; it follows from being allowed to steal the contents of |payload|. Maybe it's good to be specific, and say that the callee is allowed to swap or move the buffer. https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/neteq_impl.cc:665: Packet* packet = packet_list.front(); unique_ptr? https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/neteq_impl.cc:695: Packet* parsed_packet = new Packet; unique_ptr? https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/neteq_impl.cc:704: delete packet; unique_ptr! https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/neteq_impl.cc:1975: RTC_DCHECK(false); RTC_NOTREACHED();
https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_decoder.cc (right): https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.cc:29: using std::swap; On 2016/09/10 07:34:59, kwiberg-webrtc wrote: > On 2016/09/09 12:11:49, hlundin-webrtc wrote: > > Why the using statement? > > It's the standard idiom for swapping stuff. See e.g. > http://en.cppreference.com/w/cpp/concept/Swappable Acknowledged.
https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_decoder.cc (right): https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.cc:30: swap(this->payload_, *payload); On 2016/09/10 07:34:59, kwiberg-webrtc wrote: > On 2016/09/09 12:11:49, hlundin-webrtc wrote: > > #include what you use. > > +1 Acknowledged. https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.cc:31: } On 2016/09/10 07:34:59, kwiberg-webrtc wrote: > Why not move *payload to payload_ instead of swapping? Hmm. I was thinking of doing this to allow buffer storage to be reused but at this point, it really doesn't matter, since LegacyFrame::payload_ will always be created empty. I'll simplify with a move. https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.cc:67: bool is_primary_payload_; On 2016/09/10 07:34:59, kwiberg-webrtc wrote: > On 2016/09/09 12:11:49, hlundin-webrtc wrote: > > const > > All three could be const, right? Acknowledged. https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_decoder.h (right): https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.h:39: class Frame { On 2016/09/09 12:11:50, hlundin-webrtc wrote: > Frame is too general, and already claimed for too many different concepts in > audio processing and coding. Consider EncodedFrame. I agree - EncodedFrame is better. EncodedAudioFrame? I'm not sure. It's a bit long, especially when prefixed with AudioDecoder::. Hmm... https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.h:52: // Decodes this frame of audio and writes the result in |decoded|. On 2016/09/09 12:11:49, hlundin-webrtc wrote: > What can be expected of the state of the Frame after having called Decode? Is it > dead then? What if I call Decode twice on the same object? I'm not sure. Practically, it currently acts as if doing AudioDecoder::Decode() twice with the same payload, but that's just the implementation and not really a contract. Besides this issue, I'm wondering about the contract for the size of decoded versus the length of a frame. How will the implementer of an AudioDecoder know how long a Frame is expected to be? How large can it be? Some guidance like: - An AudioDecoder::Frame is expected to be about 20 ms long, but must not be larger than X ms. - If the input payload is longer than X ms, it must be split up into multiple frames. We'll either need to decide on rules such as these, or that Decode() can be called several times, each returning a new set of samples until the Frame is actually completely decoded, at which point it will return zero decoded samples and the Frame will be discarded. https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.h:56: virtual rtc::Optional<DecodeResult> Decode( On 2016/09/09 12:11:50, hlundin-webrtc wrote: > How will error codes from the decoder be handled? Today we are storing them for > later reporting. Will this change? No idea! You tell me! :) Are these the error codes gotten from AudioDecoder::ErrorCode? If so, I think that information should be put either as a getter on the Frame, or as a return value in DecodeResult - which in turn means it no longer makes sense to make it an Optional. It could still contain some internal Optional stuff, but by then it's probably easier to just have a |status| field and let the rest of the fields be valid only if |status| is kOK. https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.h:68: uint32_t timestamp; On 2016/09/09 12:11:50, hlundin-webrtc wrote: > rtp_timestamp? Or timestamp_in_samples? G.722... I think it should be in samples, however I'm really not sure that that is the case right now. If we want to decouple the decoders from the specifics of the network transport (which I strongly think we do), then we shouldn't be using RTP timestamps to communicate between AudioDecoder and NetEq, just as we're not sending whole (parsed) RTP packets into the AudioDecoder. https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.h:71: }; On 2016/09/10 07:34:59, kwiberg-webrtc wrote: > Why do you need ParseResult and Frame to be two different classes? > > ... Ah. So that Frame can be a pure interface. Also, the stuff in ParseResult is used to create new Packet objects by cloning the original packet and updating timestamp, primary and (the newly added) frame. This could be changed by having a (simpler) Packet type that is shared by NetEq and AudioDecoder, so that the AudioDecoder can create fully-fledged packets to be put into NetEq's PacketBuffer directly, rather than having NetEq fill in the missing details. https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.h:78: bool is_primary); On 2016/09/10 07:34:59, kwiberg-webrtc wrote: > "and retain them for as long as necessary" is redundant; it follows from being > allowed to steal the contents of |payload|. > > Maybe it's good to be specific, and say that the callee is allowed to swap or > move the buffer. Yeah, I'll change it to something more specific. https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/neteq_impl.cc:665: Packet* packet = packet_list.front(); On 2016/09/10 07:34:59, kwiberg-webrtc wrote: > unique_ptr? Acknowledged. https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/neteq_impl.cc:695: Packet* parsed_packet = new Packet; On 2016/09/10 07:34:59, kwiberg-webrtc wrote: > unique_ptr? Acknowledged. https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/neteq_impl.cc:1947: assert(packet->frame || !packet->payload.empty()); On 2016/09/09 12:11:50, hlundin-webrtc wrote: > Switch to DCHECK while you're at it. Acknowledged. https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/neteq_impl.cc:1967: // TODO(ossu): Is this the correct way to track samples decoded from a On 2016/09/09 12:11:50, hlundin-webrtc wrote: > Good question. What does the SecondaryDecodedSamples stat mean? I guess the meaning is secondary as in non-primary RED payload. At this point, it would also include Opus FEC packets. A future CL of mine separates RED packet primarity from decoder-specific packet priority. At that point, we can decide exactly what of those two we want to track, and how. https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/neteq_impl.cc:1975: RTC_DCHECK(false); On 2016/09/10 07:34:59, kwiberg-webrtc wrote: > RTC_NOTREACHED(); Ah, yes, of course! https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc (left): https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:799: // http://crbug.com/428099. On 2016/09/09 12:11:50, hlundin-webrtc wrote: > You may want to link the CL to this bug. Alright, makes sense!
Description was changed from ========== Added a ParsePayload method to AudioDecoder. It allows the decoder to split the input up into usable chunks before they are put into NetEq's PacketBuffer. Eventually, all packet splitting will move into ParsePayload. There's currently a base implementation of ParsePayload. It will generate a single Frame that calls the underlying AudioDecoder for getting Duration() and to Decode. BUG=webrtc:5805 ========== to ========== Added a ParsePayload method to AudioDecoder. It allows the decoder to split the input up into usable chunks before they are put into NetEq's PacketBuffer. Eventually, all packet splitting will move into ParsePayload. There's currently a base implementation of ParsePayload. It will generate a single Frame that calls the underlying AudioDecoder for getting Duration() and to Decode. BUG=webrtc:5805 BUG=chromium:428099 ==========
https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_decoder.h (right): https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.h:56: virtual rtc::Optional<DecodeResult> Decode( On 2016/09/12 10:31:36, ossu wrote: > On 2016/09/09 12:11:50, hlundin-webrtc wrote: > > How will error codes from the decoder be handled? Today we are storing them > for > > later reporting. Will this change? > > No idea! You tell me! :) Are these the error codes gotten from > AudioDecoder::ErrorCode? If so, I think that information should be put either as > a getter on the Frame, or as a return value in DecodeResult - which in turn > means it no longer makes sense to make it an Optional. It could still contain > some internal Optional stuff, but by then it's probably easier to just have a > |status| field and let the rest of the fields be valid only if |status| is kOK. From what I can see, the decoder error code is free-form and logged at one point in neteq_impl.cc. It is also possible to get the latest decoder error through NetEq::LastDecoderError(). This is only used in four places, all of which are in NetEq unit tests. I see no reason why this information could not be put into DecodeResult directly, even if we want to retain the current functionality (i.e. LastDecoderError()). The question is: do we? If it's only used by a couple of unit tests.
Patchset #2 (id:20001) has been deleted
https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_decoder.h (right): https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.h:39: class Frame { On 2016/09/12 10:31:37, ossu wrote: > On 2016/09/09 12:11:50, hlundin-webrtc wrote: > > Frame is too general, and already claimed for too many different concepts in > > audio processing and coding. Consider EncodedFrame. > > I agree - EncodedFrame is better. EncodedAudioFrame? I'm not sure. It's a bit > long, especially when prefixed with AudioDecoder::. > > Hmm... I've gone with EncodedAudioFrame. It makes more sense for naming with the changes I'm making to LegacyFrame in the next CL. https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.h:52: // Decodes this frame of audio and writes the result in |decoded|. On 2016/09/12 10:31:36, ossu wrote: > On 2016/09/09 12:11:49, hlundin-webrtc wrote: > > What can be expected of the state of the Frame after having called Decode? Is > it > > dead then? What if I call Decode twice on the same object? > > I'm not sure. Practically, it currently acts as if doing AudioDecoder::Decode() > twice with the same payload, but that's just the implementation and not really a > contract. Besides this issue, I'm wondering about the contract for the size of > decoded versus the length of a frame. How will the implementer of an > AudioDecoder know how long a Frame is expected to be? How large can it be? Some > guidance like: > - An AudioDecoder::Frame is expected to be about 20 ms long, but must not be > larger than X ms. > - If the input payload is longer than X ms, it must be split up into multiple > frames. > > We'll either need to decide on rules such as these, or that Decode() can be > called several times, each returning a new set of samples until the Frame is > actually completely decoded, at which point it will return zero decoded samples > and the Frame will be discarded. I've clarified that Decode should only be called once per frame. It's not yet enforced in any way, but I guess the implementations could keep track and DCHECK if we really wanted to. https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.h:68: uint32_t timestamp; On 2016/09/12 10:31:36, ossu wrote: > On 2016/09/09 12:11:50, hlundin-webrtc wrote: > > rtp_timestamp? Or timestamp_in_samples? G.722... > > I think it should be in samples, however I'm really not sure that that is the > case right now. If we want to decouple the decoders from the specifics of the > network transport (which I strongly think we do), then we shouldn't be using RTP > timestamps to communicate between AudioDecoder and NetEq, just as we're not > sending whole (parsed) RTP packets into the AudioDecoder. I've looked through the calling code and the timestamp is in samples already. Yay! https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.h:78: bool is_primary); On 2016/09/12 10:31:37, ossu wrote: > On 2016/09/10 07:34:59, kwiberg-webrtc wrote: > > "and retain them for as long as necessary" is redundant; it follows from being > > allowed to steal the contents of |payload|. > > > > Maybe it's good to be specific, and say that the callee is allowed to swap or > > move the buffer. > > Yeah, I'll change it to something more specific. Also tried to address the contract of EncodedAudioFrame here, i.e. how big a frame is supposed to be. Should we mention an ideal frame size as well? 10 ms? 20 ms?
LGTM, but you may want to sort out the question of returning error codes, in which case I'm happy to review again. https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_decoder.h (right): https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.h:52: // Decodes this frame of audio and writes the result in |decoded|. On 2016/09/13 13:37:46, ossu wrote: > On 2016/09/12 10:31:36, ossu wrote: > > On 2016/09/09 12:11:49, hlundin-webrtc wrote: > > > What can be expected of the state of the Frame after having called Decode? > Is > > it > > > dead then? What if I call Decode twice on the same object? > > > > I'm not sure. Practically, it currently acts as if doing > AudioDecoder::Decode() > > twice with the same payload, but that's just the implementation and not really > a > > contract. Besides this issue, I'm wondering about the contract for the size of > > decoded versus the length of a frame. How will the implementer of an > > AudioDecoder know how long a Frame is expected to be? How large can it be? > Some > > guidance like: > > - An AudioDecoder::Frame is expected to be about 20 ms long, but must not be > > larger than X ms. > > - If the input payload is longer than X ms, it must be split up into multiple > > frames. > > > > We'll either need to decide on rules such as these, or that Decode() can be > > called several times, each returning a new set of samples until the Frame is > > actually completely decoded, at which point it will return zero decoded > samples > > and the Frame will be discarded. > > I've clarified that Decode should only be called once per frame. It's not yet > enforced in any way, but I guess the implementations could keep track and DCHECK > if we really wanted to. Acknowledged. https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.h:56: virtual rtc::Optional<DecodeResult> Decode( On 2016/09/12 12:37:35, ossu wrote: > On 2016/09/12 10:31:36, ossu wrote: > > On 2016/09/09 12:11:50, hlundin-webrtc wrote: > > > How will error codes from the decoder be handled? Today we are storing them > > for > > > later reporting. Will this change? > > > > No idea! You tell me! :) Are these the error codes gotten from > > AudioDecoder::ErrorCode? If so, I think that information should be put either > as > > a getter on the Frame, or as a return value in DecodeResult - which in turn > > means it no longer makes sense to make it an Optional. It could still contain > > some internal Optional stuff, but by then it's probably easier to just have a > > |status| field and let the rest of the fields be valid only if |status| is > kOK. > > From what I can see, the decoder error code is free-form and logged at one point > in neteq_impl.cc. It is also possible to get the latest decoder error through > NetEq::LastDecoderError(). This is only used in four places, all of which are in > NetEq unit tests. I see no reason why this information could not be put into > DecodeResult directly, even if we want to retain the current functionality (i.e. > LastDecoderError()). The question is: do we? If it's only used by a couple of > unit tests. Right. Since no production code seems to care about the decoder error codes, we might just scrap that. However, this interface should still define how a decoder error is flagged (not specified). Does an empty return value imply an error? https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.h:68: uint32_t timestamp; On 2016/09/13 13:37:46, ossu wrote: > On 2016/09/12 10:31:36, ossu wrote: > > On 2016/09/09 12:11:50, hlundin-webrtc wrote: > > > rtp_timestamp? Or timestamp_in_samples? G.722... > > > > I think it should be in samples, however I'm really not sure that that is the > > case right now. If we want to decouple the decoders from the specifics of the > > network transport (which I strongly think we do), then we shouldn't be using > RTP > > timestamps to communicate between AudioDecoder and NetEq, just as we're not > > sending whole (parsed) RTP packets into the AudioDecoder. > > I've looked through the calling code and the timestamp is in samples already. > Yay! Acknowledged. https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder.h:78: bool is_primary); On 2016/09/13 13:37:46, ossu wrote: > On 2016/09/12 10:31:37, ossu wrote: > > On 2016/09/10 07:34:59, kwiberg-webrtc wrote: > > > "and retain them for as long as necessary" is redundant; it follows from > being > > > allowed to steal the contents of |payload|. > > > > > > Maybe it's good to be specific, and say that the callee is allowed to swap > or > > > move the buffer. > > > > Yeah, I'll change it to something more specific. > > Also tried to address the contract of EncodedAudioFrame here, i.e. how big a > frame is supposed to be. Should we mention an ideal frame size as well? 10 ms? > 20 ms? kMaxFrameSize in neteq_impl.h defines that 120 ms per frame is the maximum allowed. And lower than 10 ms is not recommended either. Other than that, I would argue that the ideal frame size is between 10 and 30 ms. https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/neteq_impl.cc:704: delete packet; On 2016/09/10 07:34:59, kwiberg-webrtc wrote: > unique_ptr! FTW! https://codereview.webrtc.org/2326953003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/neteq_impl.cc:1967: // TODO(ossu): Is this the correct way to track samples decoded from a On 2016/09/12 10:31:37, ossu wrote: > On 2016/09/09 12:11:50, hlundin-webrtc wrote: > > Good question. > > What does the SecondaryDecodedSamples stat mean? I guess the meaning is > secondary as in non-primary RED payload. At this point, it would also include > Opus FEC packets. A future CL of mine separates RED packet primarity from > decoder-specific packet priority. At that point, we can decide exactly what of > those two we want to track, and how. It was added to track how often Opus FEC saved the day. Including RED or not is an academic question, since VoiceEngine no longer supports RED.
https://codereview.webrtc.org/2326953003/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_decoder.cc (right): https://codereview.webrtc.org/2326953003/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.cc:23: class LegacyFrame : public AudioDecoder::EncodedAudioFrame { final? Good both as documentation and because it sometimes allows the compiler to optimize better. https://codereview.webrtc.org/2326953003/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_decoder.h (right): https://codereview.webrtc.org/2326953003/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.h:55: // speech. Decode must only be called once per frame object. You don't say under what circumstances the return value will be empty. Also, it's probably best to be specific: Decode may be called 0 or 1 times. Is |decoded| guaranteed to be of the correct size? Or at least as large as necessary? https://codereview.webrtc.org/2326953003/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.h:81: bool is_primary); Do you intend for this to be a sink for the payload Buffers, or should |payload| own a usable allocation after the call? The latter should be documented, and the former is probably best expressed by changing the argument type to rtc::Buffer&&. :-) Also, please document that the caller is responsible for keeping the AudioDecoder alive longer than the returned ParseResult objects. https://codereview.webrtc.org/2326953003/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/2326953003/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl.cc:692: // If you can't parse, return an empty list instead! Not sure I understand this comment. https://codereview.webrtc.org/2326953003/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl.cc:702: } Can you eliminate the special case? E.g. by checking whether packet is non-null in the beginning of the loop, and reusing it if so? That would let the optimization trigger when there are multiple results.
https://codereview.webrtc.org/2326953003/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_decoder.cc (right): https://codereview.webrtc.org/2326953003/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.cc:23: class LegacyFrame : public AudioDecoder::EncodedAudioFrame { On 2016/09/15 12:00:59, kwiberg-webrtc wrote: > final? Good both as documentation and because it sometimes allows the compiler > to optimize better. Acknowledged. https://codereview.webrtc.org/2326953003/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_decoder.h (right): https://codereview.webrtc.org/2326953003/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.h:55: // speech. Decode must only be called once per frame object. On 2016/09/15 12:00:59, kwiberg-webrtc wrote: > You don't say under what circumstances the return value will be empty. > > Also, it's probably best to be specific: Decode may be called 0 or 1 times. > > Is |decoded| guaranteed to be of the correct size? Or at least as large as > necessary? I'd really prefer it if this method were handed a Buffer<int16> instead, but I won't do that change in this CL. For now, I'll just mention it's supposed to fit 120 ms of audio in the decoder's sample rate. https://codereview.webrtc.org/2326953003/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.h:81: bool is_primary); On 2016/09/15 12:00:59, kwiberg-webrtc wrote: > Do you intend for this to be a sink for the payload Buffers, or should |payload| > own a usable allocation after the call? The latter should be documented, and the > former is probably best expressed by changing the argument type to > rtc::Buffer&&. :-) > > Also, please document that the caller is responsible for keeping the > AudioDecoder alive longer than the returned ParseResult objects. It's a sink. I'll clarify the AudioDecoder's lifetime, https://codereview.webrtc.org/2326953003/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/2326953003/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl.cc:692: // If you can't parse, return an empty list instead! On 2016/09/15 12:00:59, kwiberg-webrtc wrote: > Not sure I understand this comment. Me neither. https://codereview.webrtc.org/2326953003/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl.cc:702: } On 2016/09/15 12:00:59, kwiberg-webrtc wrote: > Can you eliminate the special case? E.g. by checking whether packet is non-null > in the beginning of the loop, and reusing it if so? That would let the > optimization trigger when there are multiple results. Acknowledged.
Almost done... https://codereview.webrtc.org/2326953003/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_decoder.h (right): https://codereview.webrtc.org/2326953003/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.h:55: // speech. Decode must only be called once per frame object. On 2016/09/15 13:07:54, ossu wrote: > On 2016/09/15 12:00:59, kwiberg-webrtc wrote: > > You don't say under what circumstances the return value will be empty. > > > > Also, it's probably best to be specific: Decode may be called 0 or 1 times. > > > > Is |decoded| guaranteed to be of the correct size? Or at least as large as > > necessary? > > I'd really prefer it if this method were handed a Buffer<int16> instead, but I > won't do that change in this CL. For now, I'll just mention it's supposed to fit > 120 ms of audio in the decoder's sample rate. IOW, it's supposed to be at least as large as necessary? (Relatedly, I presume the EncodedAudioFrame knows how much output to produce?) I agree about using a Buffer, and to not make that change in this CL. https://codereview.webrtc.org/2326953003/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.h:81: bool is_primary); On 2016/09/15 13:07:54, ossu wrote: > On 2016/09/15 12:00:59, kwiberg-webrtc wrote: > > Do you intend for this to be a sink for the payload Buffers, or should > |payload| > > own a usable allocation after the call? The latter should be documented, and > the > > former is probably best expressed by changing the argument type to > > rtc::Buffer&&. :-) > > > > Also, please document that the caller is responsible for keeping the > > AudioDecoder alive longer than the returned ParseResult objects. > > It's a sink. I'll clarify the AudioDecoder's lifetime, Acknowledged. https://codereview.webrtc.org/2326953003/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_decoder.h (right): https://codereview.webrtc.org/2326953003/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.h:54: // sample rate. On success, returns an rtc::Optional containing the total Might it be better to simply state that |decoded| will be large enough? https://codereview.webrtc.org/2326953003/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.h:57: // rtc::Optional. Decode must be called at most once per frame object. My sometimes reliable sense of English suggests s/must be/may be/. https://codereview.webrtc.org/2326953003/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.h:80: // than 10 ms. The caller must ensure that the AudioDecoder object outlives This use of "must" and "should" gives the sense that the 10 ms limit may not be absolute. Perhaps better to say something like Frames must be between 10 and 120 ms long. Are they also required to be an integer multiple of 10 ms? https://codereview.webrtc.org/2326953003/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/2326953003/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl.cc:689: } If you reuse |packet| here, packet->payload will still be in a moved-from state. Is that OK, or should you clear it?
https://codereview.webrtc.org/2326953003/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_decoder.h (right): https://codereview.webrtc.org/2326953003/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.h:80: // than 10 ms. The caller must ensure that the AudioDecoder object outlives On 2016/09/16 00:14:07, kwiberg-webrtc wrote: > This use of "must" and "should" gives the sense that the 10 ms limit may not be > absolute. Perhaps better to say something like > > Frames must be between 10 and 120 ms long. > > Are they also required to be an integer multiple of 10 ms? They need not be an integer multiple of 10.
https://codereview.webrtc.org/2326953003/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_decoder.h (right): https://codereview.webrtc.org/2326953003/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.h:80: // than 10 ms. The caller must ensure that the AudioDecoder object outlives On 2016/09/16 07:40:45, hlundin-webrtc wrote: > On 2016/09/16 00:14:07, kwiberg-webrtc wrote: > > This use of "must" and "should" gives the sense that the 10 ms limit may not > be > > absolute. Perhaps better to say something like > > > > Frames must be between 10 and 120 ms long. > > > > Are they also required to be an integer multiple of 10 ms? > > They need not be an integer multiple of 10. Are there no restrictions whatsoever, except the upper and lower bounds? In that case, maybe something like Frames may be of any length between 10 and 120 ms.
https://codereview.webrtc.org/2326953003/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_decoder.h (right): https://codereview.webrtc.org/2326953003/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.h:80: // than 10 ms. The caller must ensure that the AudioDecoder object outlives On 2016/09/16 07:51:57, kwiberg-webrtc wrote: > On 2016/09/16 07:40:45, hlundin-webrtc wrote: > > On 2016/09/16 00:14:07, kwiberg-webrtc wrote: > > > This use of "must" and "should" gives the sense that the 10 ms limit may not > > be > > > absolute. Perhaps better to say something like > > > > > > Frames must be between 10 and 120 ms long. > > > > > > Are they also required to be an integer multiple of 10 ms? > > > > They need not be an integer multiple of 10. > > Are there no restrictions whatsoever, except the upper and lower bounds? In that > case, maybe something like > > Frames may be of any length between 10 and 120 ms. In theory, no other restrictions. In practice I cannot be sure. I think our tests are only running 10, 20, 25, 30, so NetEq works with those at least.
https://codereview.webrtc.org/2326953003/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_decoder.h (right): https://codereview.webrtc.org/2326953003/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.h:54: // sample rate. On success, returns an rtc::Optional containing the total On 2016/09/16 00:14:07, kwiberg-webrtc wrote: > Might it be better to simply state that |decoded| will be large enough? I've incorporated the phrasing changes suggested in the next CL into this one. https://codereview.webrtc.org/2326953003/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.h:57: // rtc::Optional. Decode must be called at most once per frame object. On 2016/09/16 00:14:07, kwiberg-webrtc wrote: > My sometimes reliable sense of English suggests s/must be/may be/. I believe you're right. https://codereview.webrtc.org/2326953003/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_decoder.h:80: // than 10 ms. The caller must ensure that the AudioDecoder object outlives On 2016/09/16 08:25:02, hlundin-webrtc wrote: > On 2016/09/16 07:51:57, kwiberg-webrtc wrote: > > On 2016/09/16 07:40:45, hlundin-webrtc wrote: > > > On 2016/09/16 00:14:07, kwiberg-webrtc wrote: > > > > This use of "must" and "should" gives the sense that the 10 ms limit may > not > > > be > > > > absolute. Perhaps better to say something like > > > > > > > > Frames must be between 10 and 120 ms long. > > > > > > > > Are they also required to be an integer multiple of 10 ms? > > > > > > They need not be an integer multiple of 10. > > > > Are there no restrictions whatsoever, except the upper and lower bounds? In > that > > case, maybe something like > > > > Frames may be of any length between 10 and 120 ms. > > In theory, no other restrictions. In practice I cannot be sure. I think our > tests are only running 10, 20, 25, 30, so NetEq works with those at least. Acknowledged. https://codereview.webrtc.org/2326953003/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/2326953003/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl.cc:689: } On 2016/09/16 00:14:07, kwiberg-webrtc wrote: > If you reuse |packet| here, packet->payload will still be in a moved-from state. > Is that OK, or should you clear it? I think it's best: it should not be touched after it's been moved from so if it is, our checks should catch it. Actually highlighted an error that snuck into a rebase I did recently.
lgtm https://codereview.webrtc.org/2326953003/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/2326953003/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl.cc:689: } On 2016/09/16 11:24:16, ossu wrote: > On 2016/09/16 00:14:07, kwiberg-webrtc wrote: > > If you reuse |packet| here, packet->payload will still be in a moved-from > state. > > Is that OK, or should you clear it? > > I think it's best: it should not be touched after it's been moved from so if it > is, our checks should catch it. Actually highlighted an error that snuck into a > rebase I did recently. Very good.
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_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/14407) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
As a wise man once said: "The caller must ensure that the AudioDecoder object outlives any frame objects returned by this call." A few tests in modules_tests replaced one payload type with another while re-using the same payload id. It seems like this "worked" before more by accident than anything: the new decoder got the old payloads still but dealt with that gracefully. I've chosen to solve this by clearing out all packets with the payload type in NetEq::RemovePayloadType. That could be a slightly expensive but should never happen. The alternative would be to reference count the decoder and have each Frame hold on to the decoder instance. That would be cheaper but happen all the time.
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/17853)
On 2016/09/16 13:51:37, ossu wrote: > As a wise man once said: "The caller must ensure that the AudioDecoder object > outlives any frame objects returned by this call." > > A few tests in modules_tests replaced one payload type with another while > re-using the same payload id. It seems like this "worked" before more by > accident than anything: the new decoder got the old payloads still but dealt > with that gracefully. > > I've chosen to solve this by clearing out all packets with the payload type in > NetEq::RemovePayloadType. That could be a slightly expensive but should never > happen. The alternative would be to reference count the decoder and have each > Frame hold on to the decoder instance. That would be cheaper but happen all the > time. Sounds like the right approach. Still lgtm as of patch set #6.
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/2326953003/#ps140001 (title: "Added some casts from size_t to int.")
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 ========== Added a ParsePayload method to AudioDecoder. It allows the decoder to split the input up into usable chunks before they are put into NetEq's PacketBuffer. Eventually, all packet splitting will move into ParsePayload. There's currently a base implementation of ParsePayload. It will generate a single Frame that calls the underlying AudioDecoder for getting Duration() and to Decode. BUG=webrtc:5805 BUG=chromium:428099 ========== to ========== Added a ParsePayload method to AudioDecoder. It allows the decoder to split the input up into usable chunks before they are put into NetEq's PacketBuffer. Eventually, all packet splitting will move into ParsePayload. There's currently a base implementation of ParsePayload. It will generate a single Frame that calls the underlying AudioDecoder for getting Duration() and to Decode. BUG=webrtc:5805 BUG=chromium:428099 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Added a ParsePayload method to AudioDecoder. It allows the decoder to split the input up into usable chunks before they are put into NetEq's PacketBuffer. Eventually, all packet splitting will move into ParsePayload. There's currently a base implementation of ParsePayload. It will generate a single Frame that calls the underlying AudioDecoder for getting Duration() and to Decode. BUG=webrtc:5805 BUG=chromium:428099 ========== to ========== Added a ParsePayload method to AudioDecoder. It allows the decoder to split the input up into usable chunks before they are put into NetEq's PacketBuffer. Eventually, all packet splitting will move into ParsePayload. There's currently a base implementation of ParsePayload. It will generate a single Frame that calls the underlying AudioDecoder for getting Duration() and to Decode. BUG=webrtc:5805 BUG=chromium:428099 Committed: https://crrev.com/61a208b1b8e88716747971fe4ba1da8ddf521bb1 Cr-Commit-Position: refs/heads/master@{#14300} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/61a208b1b8e88716747971fe4ba1da8ddf521bb1 Cr-Commit-Position: refs/heads/master@{#14300} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
