Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1337243003:
Extracted hardcoded constants from cast_rtp_stream. (Closed)
|
Created:
5 years, 3 months ago by msu.koo Modified:
5 years, 3 months ago CC:
chromium-reviews, imcheng+watch_chromium.org, jasonroberts+watch_google.com, avayvod+watch_chromium.org, feature-media-reviews_chromium.org, xjz+watch_chromium.org, isheriff+watch_chromium.org, miu+watch_chromium.org, mcasas Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExtracted hardcoded constants from cast_rtp_stream.
BUG=522661
Committed: https://crrev.com/838220024e6912c20545c95bd7f704b51d5af516
Cr-Commit-Position: refs/heads/master@{#350120}
Patch Set 1 #
Total comments: 9
Patch Set 2 : #Patch Set 3 : #
Total comments: 19
Patch Set 4 : #
Messages
Total messages: 33 (13 generated)
msu.koo@samsung.com changed reviewers: + mcasas@chromium.org
Please kindly review this patch :)
ajose@chromium.org changed reviewers: + ajose@chromium.org
lgtm https://codereview.chromium.org/1337243003/diff/1/media/cast/cast_defines.h File media/cast/cast_defines.h (right): https://codereview.chromium.org/1337243003/diff/1/media/cast/cast_defines.h#n... media/cast/cast_defines.h:31: // This value is carefully choosen such that it fits in the 8-bits range for s/choosen/chosen/
miu@chromium.org changed reviewers: + miu@chromium.org - mcasas@chromium.org
Replacing mcasas@ with myself for OWNER approval.
isheriff@google.com changed reviewers: + isheriff@google.com
https://codereview.chromium.org/1337243003/diff/1/media/cast/cast_defines.h File media/cast/cast_defines.h (right): https://codereview.chromium.org/1337243003/diff/1/media/cast/cast_defines.h#n... media/cast/cast_defines.h:35: const int kMinUnackedFrequency = 8000; Comment here
not lgtm https://codereview.chromium.org/1337243003/diff/1/chrome/renderer/media/cast_... File chrome/renderer/media/cast_rtp_stream.cc (right): https://codereview.chromium.org/1337243003/diff/1/chrome/renderer/media/cast_... chrome/renderer/media/cast_rtp_stream.cc:184: if (config->frequency < media::cast::kMinUnackedFrequency) 8000 is the largest of the minimum sampling rate required by all codecs (e.g., Opus). It has nothing to do with unacked frames. https://codereview.chromium.org/1337243003/diff/1/chrome/renderer/media/cast_... chrome/renderer/media/cast_rtp_stream.cc:225: if (config->max_frame_rate > media::cast::kMaxUnackedFrames) 120 was chosen arbitrarily in the past. Instead, I think media::limits::kMaxFramesPerSecond is more appropriate. You'll need to #include "media/base/limits.h" https://codereview.chromium.org/1337243003/diff/1/media/cast/cast_defines.h File media/cast/cast_defines.h (right): https://codereview.chromium.org/1337243003/diff/1/media/cast/cast_defines.h#n... media/cast/cast_defines.h:35: const int kMinUnackedFrequency = 8000; This is not the meaning of this value. See comment above.
Thank you for all of your valuable comments :) I addressed your comments and left some of my thoughts. PTAL. https://codereview.chromium.org/1337243003/diff/1/chrome/renderer/media/cast_... File chrome/renderer/media/cast_rtp_stream.cc (right): https://codereview.chromium.org/1337243003/diff/1/chrome/renderer/media/cast_... chrome/renderer/media/cast_rtp_stream.cc:184: if (config->frequency < media::cast::kMinUnackedFrequency) On 2015/09/14 17:55:07, miu wrote: > 8000 is the largest of the minimum sampling rate required by all codecs (e.g., > Opus). It has nothing to do with unacked frames. Changed to use media::limits::kMinSampleRate instead. And if the minimum audio sample rate is 8000 for all codecs, I think we can also change the value of kMinSamleRate to 8000.(currently 3000) https://codereview.chromium.org/1337243003/diff/1/chrome/renderer/media/cast_... chrome/renderer/media/cast_rtp_stream.cc:225: if (config->max_frame_rate > media::cast::kMaxUnackedFrames) On 2015/09/14 17:55:07, miu wrote: > 120 was chosen arbitrarily in the past. Instead, I think > media::limits::kMaxFramesPerSecond is more appropriate. You'll need to #include > "media/base/limits.h" Done. And IMHO, kMaxFramesPerSecond 1000 is too large for the MaxFPS. It was originally introduced for video capture implementations, so I think 120 is also OK for this value. https://codereview.chromium.org/1337243003/diff/1/media/cast/cast_defines.h File media/cast/cast_defines.h (right): https://codereview.chromium.org/1337243003/diff/1/media/cast/cast_defines.h#n... media/cast/cast_defines.h:35: const int kMinUnackedFrequency = 8000; On 2015/09/14 17:55:08, miu wrote: > This is not the meaning of this value. See comment above. Done. Thank you for your comments, this change is reverted.
https://codereview.chromium.org/1337243003/diff/1/chrome/renderer/media/cast_... File chrome/renderer/media/cast_rtp_stream.cc (right): https://codereview.chromium.org/1337243003/diff/1/chrome/renderer/media/cast_... chrome/renderer/media/cast_rtp_stream.cc:184: if (config->frequency < media::cast::kMinUnackedFrequency) On 2015/09/15 00:05:06, esnada wrote: > On 2015/09/14 17:55:07, miu wrote: > > 8000 is the largest of the minimum sampling rate required by all codecs (e.g., > > Opus). It has nothing to do with unacked frames. > > Changed to use media::limits::kMinSampleRate instead. > And if the minimum audio sample rate is 8000 for all codecs, > I think we can also change the value of kMinSamleRate to 8000.(currently 3000) There's a difference: 3000 is an operational lower-bound for the entire audio stack. However, 8000 is the lower-bound for the audio codecs that process the audio here. So, this constant really does need to be 8000. Can you add a kMinSampleRateForEncoding constant (set to 8000) here: https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/cast_de...
On 2015/09/15 04:16:57, miu wrote: > https://codereview.chromium.org/1337243003/diff/1/chrome/renderer/media/cast_... > File chrome/renderer/media/cast_rtp_stream.cc (right): > > https://codereview.chromium.org/1337243003/diff/1/chrome/renderer/media/cast_... > chrome/renderer/media/cast_rtp_stream.cc:184: if (config->frequency < > media::cast::kMinUnackedFrequency) > On 2015/09/15 00:05:06, esnada wrote: > > On 2015/09/14 17:55:07, miu wrote: > > > 8000 is the largest of the minimum sampling rate required by all codecs > (e.g., > > > Opus). It has nothing to do with unacked frames. > > > > Changed to use media::limits::kMinSampleRate instead. > > And if the minimum audio sample rate is 8000 for all codecs, > > I think we can also change the value of kMinSamleRate to 8000.(currently 3000) > > There's a difference: 3000 is an operational lower-bound for the entire audio > stack. However, 8000 is the lower-bound for the audio codecs that process the > audio here. So, this constant really does need to be 8000. > > Can you add a kMinSampleRateForEncoding constant (set to 8000) here: > https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/cast_de... Your comments was addressed. PTAL :)
mcasas@chromium.org changed reviewers: + mcasas@chromium.org
https://codereview.chromium.org/1337243003/diff/40001/chrome/renderer/media/c... File chrome/renderer/media/cast_rtp_stream.cc (right): https://codereview.chromium.org/1337243003/diff/40001/chrome/renderer/media/c... chrome/renderer/media/cast_rtp_stream.cc:84: // once b/13696137 is fixed. Note: We shouldn't have buganizer bugs in Chromium codebase. And, this b/ is fixed :) https://codereview.chromium.org/1337243003/diff/40001/chrome/renderer/media/c... chrome/renderer/media/cast_rtp_stream.cc:91: payload.min_bitrate = 50; What about these |50| and |2000| ? (l.73-74, 90-91) https://codereview.chromium.org/1337243003/diff/40001/chrome/renderer/media/c... chrome/renderer/media/cast_rtp_stream.cc:171: config->min_playout_delay = see my comment on l.207 https://codereview.chromium.org/1337243003/diff/40001/chrome/renderer/media/c... chrome/renderer/media/cast_rtp_stream.cc:185: if (config->frequency < media::cast::kMinSampleRateForEncoding) If |kMinSampleRateForEncoding| is only used in this file you should move it to here around l.43, so is not exported -- less symbols for the linker etc. https://codereview.chromium.org/1337243003/diff/40001/chrome/renderer/media/c... chrome/renderer/media/cast_rtp_stream.cc:207: base::TimeDelta::FromMilliseconds( l.206-210 should be formatted: config->min_playout_delay = base::TimeDelta::FromMilliseconds( params.payload.min_latency_ms ? params.payload.min_latency_ms : params.payload.max_latency_ms); https://codereview.chromium.org/1337243003/diff/40001/chrome/renderer/media/c... chrome/renderer/media/cast_rtp_stream.cc:237: if (!config->use_external_encoder) { no {} https://codereview.chromium.org/1337243003/diff/40001/media/cast/cast_defines.h File media/cast/cast_defines.h (right): https://codereview.chromium.org/1337243003/diff/40001/media/cast/cast_defines... media/cast/cast_defines.h:23: const int64 kDontShowTimeoutMs = 33; I think this is never used, so please remove. (But it might be, in unindexed locations) https://codereview.chromium.org/1337243003/diff/40001/media/cast/cast_defines... media/cast/cast_defines.h:24: const float kDefaultCongestionControlBackOff = 0.875f; This is only used once in [1], suggest moving it there :) [1] https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/cast_co... https://codereview.chromium.org/1337243003/diff/40001/media/cast/cast_defines... media/cast/cast_defines.h:26: const int kMinSampleRateForEncoding = 8000; s/int/uint32_t/ I personally like writing 8000u for unsigned constants, but that's not pervasive in the code base. Just makes it easier to read IMHO.
Some corrections: https://codereview.chromium.org/1337243003/diff/40001/chrome/renderer/media/c... File chrome/renderer/media/cast_rtp_stream.cc (right): https://codereview.chromium.org/1337243003/diff/40001/chrome/renderer/media/c... chrome/renderer/media/cast_rtp_stream.cc:84: // once b/13696137 is fixed. On 2015/09/15 16:59:39, mcasas wrote: > Note: We shouldn't have buganizer bugs in Chromium > codebase. And, this b/ is fixed :) > Note: esnada@ is an external-to-Google contributor. ;-) Looking at this comment and the bug tracker, I'd say it'd be fine to just delete it. https://codereview.chromium.org/1337243003/diff/40001/chrome/renderer/media/c... chrome/renderer/media/cast_rtp_stream.cc:185: if (config->frequency < media::cast::kMinSampleRateForEncoding) On 2015/09/15 16:59:39, mcasas wrote: > If |kMinSampleRateForEncoding| is only used in this file > you should move it to here around l.43, so is not exported > -- less symbols for the linker etc. I disagree with this one. IMO, the constant belongs in the cast_defines.h header file since it pertains to all clients of the media/cast library. Side note: We plan to re-do all the int constants as enums as part of a massive clean-up bug we're about to start on, which would resolve the "extra linking" problem. https://codereview.chromium.org/1337243003/diff/40001/media/cast/cast_defines.h File media/cast/cast_defines.h (right): https://codereview.chromium.org/1337243003/diff/40001/media/cast/cast_defines... media/cast/cast_defines.h:26: const int kMinSampleRateForEncoding = 8000; On 2015/09/15 16:59:39, mcasas wrote: > s/int/uint32_t/ > > I personally like writing 8000u for unsigned constants, but > that's not pervasive in the code base. Just makes it easier > to read IMHO. The "int" is more correct, per style guide (https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types). We're trying to get rid of uses of unsigned ints in media/cast code wherever we can (style guide discussion explains why they are bad). In media/cast code, the only allowed cases (in new code) are: 1. Bit manipulation, wire-format (de)serialization. This includes blob/opaque data (e.g., use of uint8 to indicate byte-sized binary payloads). 2. In cases where size_t should be used. 3. Where wrap-around "counter" semantics are a part of the design (though this has caused us headaches, and we're looking to replace these unsigneds with class types).
Please take a look :) https://codereview.chromium.org/1337243003/diff/40001/chrome/renderer/media/c... File chrome/renderer/media/cast_rtp_stream.cc (right): https://codereview.chromium.org/1337243003/diff/40001/chrome/renderer/media/c... chrome/renderer/media/cast_rtp_stream.cc:84: // once b/13696137 is fixed. On 2015/09/15 22:05:22, miu wrote: > On 2015/09/15 16:59:39, mcasas wrote: > > Note: We shouldn't have buganizer bugs in Chromium > > codebase. And, this b/ is fixed :) > > > > Note: esnada@ is an external-to-Google contributor. ;-) > > Looking at this comment and the bug tracker, I'd say it'd be fine to just delete > it. This comment lines were removed. https://codereview.chromium.org/1337243003/diff/40001/chrome/renderer/media/c... chrome/renderer/media/cast_rtp_stream.cc:171: config->min_playout_delay = On 2015/09/15 16:59:39, mcasas wrote: > see my comment on l.207 Done. https://codereview.chromium.org/1337243003/diff/40001/chrome/renderer/media/c... chrome/renderer/media/cast_rtp_stream.cc:207: base::TimeDelta::FromMilliseconds( On 2015/09/15 16:59:39, mcasas wrote: > l.206-210 should be formatted: > > config->min_playout_delay = base::TimeDelta::FromMilliseconds( > params.payload.min_latency_ms ? > params.payload.min_latency_ms : > params.payload.max_latency_ms); Done. https://codereview.chromium.org/1337243003/diff/40001/chrome/renderer/media/c... chrome/renderer/media/cast_rtp_stream.cc:237: if (!config->use_external_encoder) { On 2015/09/15 16:59:39, mcasas wrote: > no {} Done. https://codereview.chromium.org/1337243003/diff/40001/media/cast/cast_defines.h File media/cast/cast_defines.h (right): https://codereview.chromium.org/1337243003/diff/40001/media/cast/cast_defines... media/cast/cast_defines.h:23: const int64 kDontShowTimeoutMs = 33; On 2015/09/15 16:59:39, mcasas wrote: > I think this is never used, so please remove. > (But it might be, in unindexed locations) Done. https://codereview.chromium.org/1337243003/diff/40001/media/cast/cast_defines... media/cast/cast_defines.h:24: const float kDefaultCongestionControlBackOff = 0.875f; On 2015/09/15 16:59:39, mcasas wrote: > This is only used once in [1], suggest moving it there :) > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/cast_co... Done. https://codereview.chromium.org/1337243003/diff/40001/media/cast/cast_defines... media/cast/cast_defines.h:26: const int kMinSampleRateForEncoding = 8000; On 2015/09/15 22:05:22, miu wrote: > On 2015/09/15 16:59:39, mcasas wrote: > > s/int/uint32_t/ > > > > I personally like writing 8000u for unsigned constants, but > > that's not pervasive in the code base. Just makes it easier > > to read IMHO. > > The "int" is more correct, per style guide > (https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types). > > We're trying to get rid of uses of unsigned ints in media/cast code wherever we > can (style guide discussion explains why they are bad). In media/cast code, the > only allowed cases (in new code) are: > > 1. Bit manipulation, wire-format (de)serialization. This includes blob/opaque > data (e.g., use of uint8 to indicate byte-sized binary payloads). > > 2. In cases where size_t should be used. > > 3. Where wrap-around "counter" semantics are a part of the design (though this > has caused us headaches, and we're looking to replace these unsigneds with class > types). Acknowledged.
lgtm Thanks for the clean-up! :)
The CQ bit was checked by msu.koo@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from ajose@chromium.org Link to the patchset: https://codereview.chromium.org/1337243003/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1337243003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1337243003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by msu.koo@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1337243003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1337243003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by msu.koo@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1337243003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1337243003/60001
The CQ bit was unchecked by msu.koo@samsung.com
The CQ bit was checked by msu.koo@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1337243003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1337243003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/838220024e6912c20545c95bd7f704b51d5af516 Cr-Commit-Position: refs/heads/master@{#350120} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
