Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2270063006:
TimestampScaler no longer depends on NetEqDecoder to figure out scaling. (Closed)
DescriptionTimestampScaler no longer depends on NetEqDecoder to figure out scaling.
This is still a tiny lie, since it checks for kCodecArbitrary to avoid
scaling a codec with an external decoder, because of missing information
in that case. The main point is still true, though. Once the next CL is
in, removing NetEqDecoder usage completely in DecoderDatabase, another
workaround will be in place for external decoders until we can fully
deprecate them.
BUG=webrtc:5805
Committed: https://crrev.com/09f1560f813a36f4176c1fb1068f4d7105c26175
Cr-Commit-Position: refs/heads/master@{#13952}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Make sure the diffs are calculated properly as signed. Also use auto. #
Total comments: 8
Patch Set 3 : Moved a *, turned diffs to const. #
Depends on Patchset: Messages
Total messages: 26 (11 generated)
ossu@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, kwiberg@webrtc.org
One more CL on the way of removing NetEqDecoder usage from within NetEq.
https://codereview.webrtc.org/2270063006/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/timestamp_scaler.cc (right): https://codereview.webrtc.org/2270063006/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/timestamp_scaler.cc:66: internal_ref_ += (external_diff * numerator_) / denominator_; Have you made sure that this still won't overflow now that numerator_ and denominator_ are much larger than before? Otherwise, consider normalizing them. Hmm. This code has only ever been tested with numerator_ in {1, 2} and denominator_ == 1, right? What would happen if the division wasn't exact? Unless you can give a satisfying answer, consider replacing numerator_ and denominator_ with simple integer multiplier_ or something, and fill it in with rtc::CheckedDivExact. https://codereview.webrtc.org/2270063006/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/timestamp_scaler.cc:84: return external_ref_ + (internal_diff * denominator_) / numerator_; Same here. https://codereview.webrtc.org/2270063006/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc (right): https://codereview.webrtc.org/2270063006/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc:28: CreateBuiltinAudioDecoderFactory(); Consider using auto in cases like this. You have a bunch of them...
https://codereview.webrtc.org/2270063006/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/timestamp_scaler.cc (right): https://codereview.webrtc.org/2270063006/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/timestamp_scaler.cc:66: internal_ref_ += (external_diff * numerator_) / denominator_; On 2016/08/26 09:07:53, kwiberg-webrtc wrote: > Have you made sure that this still won't overflow now that numerator_ and > denominator_ are much larger than before? Otherwise, consider normalizing them. > > Hmm. This code has only ever been tested with numerator_ in {1, 2} and > denominator_ == 1, right? What would happen if the division wasn't exact? Unless > you can give a satisfying answer, consider replacing numerator_ and denominator_ > with simple integer multiplier_ or something, and fill it in with > rtc::CheckedDivExact. Well, external_timestamp is 32 bit and clockrates are as well - actually, they're all in the thousands to tens of thousands, so overflow shouldn't be an issue. Non-exact division, though, yeah, that could maybe happen. Actually, I think it's probably quite likely in some cases described in the RFCs (though we don't use them ourselves): i.e. an MPEG clockrate of 90000 and a samplerate of 48000. I _guess_ that's ok. The timestamps would be slightly off but we can't really express them more exact than this. I'll have a little think about the implications. https://codereview.webrtc.org/2270063006/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc (right): https://codereview.webrtc.org/2270063006/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc:28: CreateBuiltinAudioDecoderFactory(); On 2016/08/26 09:07:53, kwiberg-webrtc wrote: > Consider using auto in cases like this. You have a bunch of them... Do big, scary types frighten you? :) Yeah, alright, auto might be good here.
https://codereview.webrtc.org/2270063006/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/timestamp_scaler.cc (right): https://codereview.webrtc.org/2270063006/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/timestamp_scaler.cc:66: internal_ref_ += (external_diff * numerator_) / denominator_; On 2016/08/26 09:22:33, ossu wrote: > On 2016/08/26 09:07:53, kwiberg-webrtc wrote: > > Have you made sure that this still won't overflow now that numerator_ and > > denominator_ are much larger than before? Otherwise, consider normalizing > them. > > > > Hmm. This code has only ever been tested with numerator_ in {1, 2} and > > denominator_ == 1, right? What would happen if the division wasn't exact? > Unless > > you can give a satisfying answer, consider replacing numerator_ and > denominator_ > > with simple integer multiplier_ or something, and fill it in with > > rtc::CheckedDivExact. > > Well, external_timestamp is 32 bit and clockrates are as well - actually, > they're all in the thousands to tens of thousands, so overflow shouldn't be an > issue. > > Non-exact division, though, yeah, that could maybe happen. Actually, I think > it's probably quite likely in some cases described in the RFCs (though we don't > use them ourselves): i.e. an MPEG clockrate of 90000 and a samplerate of 48000. > I _guess_ that's ok. The timestamps would be slightly off but we can't really > express them more exact than this. I'll have a little think about the > implications. No, I guess the scaling would be fine for that as well. So using rtc::CheckedDivExact should be fine for these conversions (i.e. internal_ref_ += rtc::CheckedDivExact(external_diff * numerator_), denominator);)
Patchset #2 (id:20001) has been deleted
https://codereview.webrtc.org/2270063006/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/timestamp_scaler.cc (right): https://codereview.webrtc.org/2270063006/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/timestamp_scaler.cc:66: internal_ref_ += (external_diff * numerator_) / denominator_; On 2016/08/26 09:28:16, ossu wrote: > On 2016/08/26 09:22:33, ossu wrote: > > On 2016/08/26 09:07:53, kwiberg-webrtc wrote: > > > Have you made sure that this still won't overflow now that numerator_ and > > > denominator_ are much larger than before? Otherwise, consider normalizing > > them. > > > > > > Hmm. This code has only ever been tested with numerator_ in {1, 2} and > > > denominator_ == 1, right? What would happen if the division wasn't exact? > > Unless > > > you can give a satisfying answer, consider replacing numerator_ and > > denominator_ > > > with simple integer multiplier_ or something, and fill it in with > > > rtc::CheckedDivExact. > > > > Well, external_timestamp is 32 bit and clockrates are as well - actually, > > they're all in the thousands to tens of thousands, so overflow shouldn't be an > > issue. > > > > Non-exact division, though, yeah, that could maybe happen. Actually, I think > > it's probably quite likely in some cases described in the RFCs (though we > don't > > use them ourselves): i.e. an MPEG clockrate of 90000 and a samplerate of > 48000. > > I _guess_ that's ok. The timestamps would be slightly off but we can't really > > express them more exact than this. I'll have a little think about the > > implications. > > No, I guess the scaling would be fine for that as well. So using > rtc::CheckedDivExact should be fine for these conversions (i.e. internal_ref_ += > rtc::CheckedDivExact(external_diff * numerator_), denominator);) After discussion with hlundin@ the conclusion is that the scaling is done on a best-effort basis already: the timestamps can be slightly off due to rounding, especially in ToExternal below, where exact internal timestamps may contain a few extra samples here and there. In fact, switching to CheckedDivExact in ToExternal causes an RTC_CHECK even before this CL. https://codereview.webrtc.org/2270063006/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc (right): https://codereview.webrtc.org/2270063006/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc:28: CreateBuiltinAudioDecoderFactory(); On 2016/08/26 09:22:33, ossu wrote: > On 2016/08/26 09:07:53, kwiberg-webrtc wrote: > > Consider using auto in cases like this. You have a bunch of them... > > Do big, scary types frighten you? :) > Yeah, alright, auto might be good here. Done. https://codereview.webrtc.org/2270063006/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/timestamp_scaler.cc (right): https://codereview.webrtc.org/2270063006/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/timestamp_scaler.cc:63: int64_t external_diff = int64_t{external_timestamp} - external_ref_; These lines did unsigned 32 bit subtractions and put the result in an int64_t, causing negative values to be 4 billion something. Still seemed to work, but this is more correct.
LGTM with one nit. https://codereview.webrtc.org/2270063006/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/decoder_database.h (right): https://codereview.webrtc.org/2270063006/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/decoder_database.h:62: const AudioDecoder *decoder = GetDecoder(); Join the type with the asterisk.
lgtm https://codereview.webrtc.org/2270063006/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/decoder_database.h (right): https://codereview.webrtc.org/2270063006/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/decoder_database.h:62: const AudioDecoder *decoder = GetDecoder(); On 2016/08/26 11:34:46, hlundin-webrtc wrote: > Join the type with the asterisk. Better yet, get in the habit of running git cl format. https://codereview.webrtc.org/2270063006/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/timestamp_scaler.cc (right): https://codereview.webrtc.org/2270063006/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/timestamp_scaler.cc:63: int64_t external_diff = int64_t{external_timestamp} - external_ref_; On 2016/08/26 11:20:51, ossu wrote: > These lines did unsigned 32 bit subtractions and put the result in an int64_t, > causing negative values to be 4 billion something. Still seemed to work, but > this is more correct. Acknowledged. Consider making it const too.
BOOM! https://codereview.webrtc.org/2270063006/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/decoder_database.h (right): https://codereview.webrtc.org/2270063006/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/decoder_database.h:62: const AudioDecoder *decoder = GetDecoder(); On 2016/08/26 11:40:57, kwiberg-webrtc wrote: > On 2016/08/26 11:34:46, hlundin-webrtc wrote: > > Join the type with the asterisk. > > Better yet, get in the habit of running git cl format. I often do that on single statements I add - mostly to deal with linebreaks (I can do it easily enough for just the part of code I want from within my editor). I don't like handing all my code over to it, though. clang-format produces a conforming version of the code, but maybe not the conforming version I want. :) https://codereview.webrtc.org/2270063006/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/timestamp_scaler.cc (right): https://codereview.webrtc.org/2270063006/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/timestamp_scaler.cc:63: int64_t external_diff = int64_t{external_timestamp} - external_ref_; On 2016/08/26 11:40:57, kwiberg-webrtc wrote: > On 2016/08/26 11:20:51, ossu wrote: > > These lines did unsigned 32 bit subtractions and put the result in an int64_t, > > causing negative values to be 4 billion something. Still seemed to work, but > > this is more correct. > > Acknowledged. > > Consider making it const too. Acknowledged.
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/...
https://codereview.webrtc.org/2270063006/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/decoder_database.h (right): https://codereview.webrtc.org/2270063006/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/decoder_database.h:62: const AudioDecoder *decoder = GetDecoder(); On 2016/08/26 11:47:31, ossu wrote: > On 2016/08/26 11:40:57, kwiberg-webrtc wrote: > > On 2016/08/26 11:34:46, hlundin-webrtc wrote: > > > Join the type with the asterisk. > > > > Better yet, get in the habit of running git cl format. > > I often do that on single statements I add - mostly to deal with linebreaks (I > can do it easily enough for just the part of code I want from within my editor). > I don't like handing all my code over to it, though. clang-format produces a > conforming version of the code, but maybe not the conforming version I want. :) I make sure to commit locally when I'm done, then run git cl format and look at the diff. Most of the time there is either no diff or sensible reformatting, and in the rare cases where it did something objectionable, I selectively revert the objectionable changes. (But note that fighting clang-format is an uphill battle---the next person to touch those lines is likely to just reformat them the way clang-format wants anyway.)
https://codereview.webrtc.org/2270063006/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/decoder_database.h (right): https://codereview.webrtc.org/2270063006/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/decoder_database.h:62: const AudioDecoder *decoder = GetDecoder(); On 2016/08/26 12:30:07, kwiberg-webrtc wrote: > On 2016/08/26 11:47:31, ossu wrote: > > On 2016/08/26 11:40:57, kwiberg-webrtc wrote: > > > On 2016/08/26 11:34:46, hlundin-webrtc wrote: > > > > Join the type with the asterisk. > > > > > > Better yet, get in the habit of running git cl format. > > > > I often do that on single statements I add - mostly to deal with linebreaks (I > > can do it easily enough for just the part of code I want from within my > editor). > > I don't like handing all my code over to it, though. clang-format produces a > > conforming version of the code, but maybe not the conforming version I want. > :) > > I make sure to commit locally when I'm done, then run git cl format and look at > the diff. Most of the time there is either no diff or sensible reformatting, and > in the rare cases where it did something objectionable, I selectively revert the > objectionable changes. (But note that fighting clang-format is an uphill > battle---the next person to touch those lines is likely to just reformat them > the way clang-format wants anyway.) Sisyphus as well as Don Quixote spring to mind...
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/2270063006/#ps60001 (title: "Moved a *, turned diffs to const.")
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
Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/16047)
The CQ bit was checked by ossu@webrtc.org
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.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== TimestampScaler no longer depends on NetEqDecoder to figure out scaling. This is still a tiny lie, since it checks for kCodecArbitrary to avoid scaling a codec with an external decoder, because of missing information in that case. The main point is still true, though. Once the next CL is in, removing NetEqDecoder usage completely in DecoderDatabase, another workaround will be in place for external decoders until we can fully deprecate them. BUG=webrtc:5805 ========== to ========== TimestampScaler no longer depends on NetEqDecoder to figure out scaling. This is still a tiny lie, since it checks for kCodecArbitrary to avoid scaling a codec with an external decoder, because of missing information in that case. The main point is still true, though. Once the next CL is in, removing NetEqDecoder usage completely in DecoderDatabase, another workaround will be in place for external decoders until we can fully deprecate them. BUG=webrtc:5805 Committed: https://crrev.com/09f1560f813a36f4176c1fb1068f4d7105c26175 Cr-Commit-Position: refs/heads/master@{#13952} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/09f1560f813a36f4176c1fb1068f4d7105c26175 Cr-Commit-Position: refs/heads/master@{#13952} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
