Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2559953002:
Log audio network adapter decisions in event log. (Closed)
|
Created:
4 years ago by michaelt Modified:
3 years, 11 months ago CC:
webrtc-reviews_webrtc.org, kwiberg-webrtc, Andrew MacDonald, henrika_webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionLog audio network adapter decisions in event log.
BUG=webrtc:6845
Review-Url: https://codereview.webrtc.org/2559953002
Cr-Commit-Position: refs/heads/master@{#16053}
Committed: https://chromium.googlesource.com/external/webrtc/+/3663681b5d05682f38e75d9eaf6049299f62fb02
Patch Set 1 : Log Ana #
Total comments: 10
Patch Set 2 : Response to comments. #
Total comments: 9
Patch Set 3 : Response to comments #Patch Set 4 : Response to comments #
Total comments: 28
Patch Set 5 : Response to comments. #
Total comments: 8
Patch Set 6 : Responde to comments. #
Total comments: 16
Patch Set 7 : Response to comments #
Total comments: 6
Patch Set 8 : Minyue's changes #
Total comments: 5
Patch Set 9 : Added a parser and a unittest #Patch Set 10 : Rebased #
Total comments: 6
Patch Set 11 : Responde to comments. #Patch Set 12 : Change abs to fabs #Patch Set 13 : Fix windows build #Patch Set 14 : Revert ana dump changes #Depends on Patchset: Messages
Total messages: 90 (50 generated)
The CQ bit was checked by michaelt@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/16951)
The CQ bit was checked by michaelt@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/20145) mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/16956) presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/11210)
The CQ bit was checked by michaelt@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: linux_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...) presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/11212)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== Log audio network adapter decisions in event log. BUG=webrtc:6845 ========== to ========== Log audio network adapter decisions in event log. BUG=webrtc:6845 ==========
michaelt@webrtc.org changed reviewers: + minyue@webrtc.org, solenberg@webrtc.org
Here the Cl which does the actual logging.
Before going into fine details, I add some general questions https://codereview.webrtc.org/2559953002/diff/60001/webrtc/logging/rtc_event_... File webrtc/logging/rtc_event_log/rtc_event_log.proto (right): https://codereview.webrtc.org/2559953002/diff/60001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/rtc_event_log.proto:71: optional AudioNetworkAdaptorDecition audio_network_adaptor_decition = 12; We call the output of AudioNetworkAdaptor, RuntimeConfig. Can we somehow eliminate "Decision"? How about AudioEncoderRuntimeConfig? https://codereview.webrtc.org/2559953002/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc (right): https://codereview.webrtc.org/2559953002/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc:39: void EventLogWriter::MayLogEncoderRuntimeConfig( Do we want to log only the changes or the whole runtimeConfig? https://codereview.webrtc.org/2559953002/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.h (right): https://codereview.webrtc.org/2559953002/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.h:33: const int min_bitreate_change_bps_; bitrate and do we want to make it a ratio? 5 -> 10 is considered a large change while 35 -> 40 may not? https://codereview.webrtc.org/2559953002/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.h:34: const float min_packet_loss_change_fraction_; also a ratio?
https://codereview.webrtc.org/2559953002/diff/60001/webrtc/logging/rtc_event_... File webrtc/logging/rtc_event_log/rtc_event_log.proto (right): https://codereview.webrtc.org/2559953002/diff/60001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/rtc_event_log.proto:71: optional AudioNetworkAdaptorDecition audio_network_adaptor_decition = 12; Sure why not "AudioEncoderRuntimeConfig" https://codereview.webrtc.org/2559953002/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc (right): https://codereview.webrtc.org/2559953002/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc:39: void EventLogWriter::MayLogEncoderRuntimeConfig( We could save some space if we would just log changes. But it would make the logging more complex. https://codereview.webrtc.org/2559953002/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.h (right): https://codereview.webrtc.org/2559953002/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.h:33: const int min_bitreate_change_bps_; My original idea was to use a ratio value for bit-rate and packet loss. But after some deeper thoughts about this, i decided to use no ratio for the bitrate. I agree that the changes of in small rates may be more imported, but since the bitrate is not a precises representation of BW it seams not very useful to me to log more on lower bit-rates. https://codereview.webrtc.org/2559953002/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.h:34: const float min_packet_loss_change_fraction_; This is a ratio all already. I may could change the name of the variable.
https://codereview.webrtc.org/2559953002/diff/60001/webrtc/logging/rtc_event_... File webrtc/logging/rtc_event_log/rtc_event_log.proto (right): https://codereview.webrtc.org/2559953002/diff/60001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/rtc_event_log.proto:235: message AudioNetworkAdaptorDecition { is it possible to merge this and EncoderRuntimeConfig in audio_network_adaptor/debug_dump.proto I think it would be good to import debug_dump.proto to this. WDYT
https://codereview.webrtc.org/2559953002/diff/60001/webrtc/logging/rtc_event_... File webrtc/logging/rtc_event_log/rtc_event_log.proto (right): https://codereview.webrtc.org/2559953002/diff/60001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/rtc_event_log.proto:235: message AudioNetworkAdaptorDecition { On 2016/12/13 10:52:02, minyue-webrtc wrote: > is it possible to merge this and EncoderRuntimeConfig in > audio_network_adaptor/debug_dump.proto > > I think it would be good to import debug_dump.proto to this. WDYT Done.
lgtm https://codereview.webrtc.org/2559953002/diff/80001/webrtc/logging/rtc_event_... File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/2559953002/diff/80001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/rtc_event_log.cc:447: rtc::Optional<int> bitrate_bps, can we even pass an ana_event to this function? Basically, I would hope the rtc_enent_log don't care the actual content of an ana_event. Then ANA debug dump and event log can be unified. When only need to define a criteria for adding a log.
On 2016/12/13 16:02:16, minyue-webrtc wrote: > lgtm > > https://codereview.webrtc.org/2559953002/diff/80001/webrtc/logging/rtc_event_... > File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): > > https://codereview.webrtc.org/2559953002/diff/80001/webrtc/logging/rtc_event_... > webrtc/logging/rtc_event_log/rtc_event_log.cc:447: rtc::Optional<int> > bitrate_bps, > can we even pass an ana_event to this function? Basically, I would hope the > rtc_enent_log don't care the actual content of an ana_event. > > Then ANA debug dump and event log can be unified. When only need to define a > criteria for adding a log. Sorry, I clicked wrong button, but it is not yet look good to me. I will not take it back. But please wait for my next approval before commit.
https://codereview.webrtc.org/2559953002/diff/80001/webrtc/logging/BUILD.gn File webrtc/logging/BUILD.gn (right): https://codereview.webrtc.org/2559953002/diff/80001/webrtc/logging/BUILD.gn#n... webrtc/logging/BUILD.gn:86: proto_in_dir = "//" Enlighten me - what is the reasoning with this change? https://codereview.webrtc.org/2559953002/diff/80001/webrtc/logging/rtc_event_... File webrtc/logging/rtc_event_log/mock/mock_rtc_event_log.h (right): https://codereview.webrtc.org/2559953002/diff/80001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/mock/mock_rtc_event_log.h:61: MOCK_METHOD6(LogAnaDecisionEvent, So basically, this function is here to log 6 different events? Hmm... https://codereview.webrtc.org/2559953002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc (right): https://codereview.webrtc.org/2559953002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc:41: if (last_runtime_config_) { I don't understand the mechanics here. It looks like you're checking deltas and logging only if there is a change. But you do that up to three times. So there may be three events in the proto containing *exactly* the same data? And to boot, all the fields in the events are optional so they may be set, or not. I'm sorry, but that seems both inefficient and highly ambiguous.
https://codereview.webrtc.org/2559953002/diff/80001/webrtc/logging/BUILD.gn File webrtc/logging/BUILD.gn (right): https://codereview.webrtc.org/2559953002/diff/80001/webrtc/logging/BUILD.gn#n... webrtc/logging/BUILD.gn:86: proto_in_dir = "//" To be able to include debug_dump.proto in rtc_event_log.proto both had to be compiled at the same folder level. Now both where compiled from git root. https://codereview.webrtc.org/2559953002/diff/80001/webrtc/logging/rtc_event_... File webrtc/logging/rtc_event_log/mock/mock_rtc_event_log.h (right): https://codereview.webrtc.org/2559953002/diff/80001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/mock/mock_rtc_event_log.h:61: MOCK_METHOD6(LogAnaDecisionEvent, Would you prefer to log each event separated. I implemented it this way to give the log some structure. https://codereview.webrtc.org/2559953002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc (right): https://codereview.webrtc.org/2559953002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc:41: if (last_runtime_config_) { This is partly a bug and party a feature. It should just be logged once => this is the bug. Added a unit-test and a fix. The feature is that all data is logged. This is done to be sure which ANA controller are active. All active controllers have to set the Encoder config field.
https://codereview.webrtc.org/2559953002/diff/80001/webrtc/logging/rtc_event_... File webrtc/logging/rtc_event_log/mock/mock_rtc_event_log.h (right): https://codereview.webrtc.org/2559953002/diff/80001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/mock/mock_rtc_event_log.h:61: MOCK_METHOD6(LogAnaDecisionEvent, On 2016/12/14 14:41:37, michaelt wrote: > Would you prefer to log each event separated. I implemented it this way to give > the log some structure. > > No, I don't agree. if you pass the proto event as input, it still have structure. The log method does not need to know the content of it any longer. Imagine we add other controllers in the future.
https://codereview.webrtc.org/2559953002/diff/80001/webrtc/logging/rtc_event_... File webrtc/logging/rtc_event_log/mock/mock_rtc_event_log.h (right): https://codereview.webrtc.org/2559953002/diff/80001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/mock/mock_rtc_event_log.h:61: MOCK_METHOD6(LogAnaDecisionEvent, After a offline discussion, minyue and me agreed on passing EncoderRuntimeConfig to this log function.
nice work! I have some suggestions. Plus, a naming concern (again ... my apologies) https://codereview.webrtc.org/2559953002/diff/120001/webrtc/logging/rtc_event... File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/2559953002/diff/120001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:446: event->set_type(rtclog::Event::AUDIO_NETWORK_ADAPTOR_EVENT); Slightly confusing is that the function is LogEncoderRuntimeConfig and the event type is AUDIO_NETWORK_ADAPTOR_EVENT; Can we change the event type and the function name to LogAudioEncoderConfig and AUDIO_ENCODER_CONFIG_EVENT, For brevity, I throw "runtime" away. Since the log is a time series, "runtime" is implicit. https://codereview.webrtc.org/2559953002/diff/120001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:448: if (config.bitrate_bps) how about making 448 ~ 460 a utility function, which can be also used in debug_dump_writer.cc? Then event log here does not need to care the content. where to put it? hmm, for simplicity, I don't mind first making it a static method in debug_dump_writer. The method can be called https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc (right): https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc:19: constexpr int kMinBitreateChangeBps = 5000; bitrate https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc:23: EventLogWriter::EventLogWriter(RtcEventLog* event_log) I'd like to remove this constructor, put kMinBitreateChangeBps and kMinPacketLossChangeFraction in ana_impl.cc but preferably rename a bit: kEventLogMinBitrateChangeBps and kEventLogMinPacketLossChangeFraction https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc:41: if (last_runtime_config_) { I prefer adding a private void LogEncoderRuntimeConfig(const AudioNetworkAdaptor::EncoderRuntimeConfig& config) { last_runtime_config_ = rtc::Optional<AudioNetworkAdaptor::EncoderRuntimeConfig>(config); event_log_->LogEncoderRuntimeConfig(config); } and here if (!last_runtime_config_) return LogEncoderRuntimeConfig(config); if (xxx) return LogEncoderRuntimeConfig(config); if (yyy) return LogEncoderRuntimeConfig(config); break all "||"s https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.h (right): https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.h:20: class EventLogWriter { final https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.h:23: int min_bitreate_change_bps, bitrate https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.h:31: RtcEventLog* event_log_; RtcEventLog* const https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.h:34: const float min_packet_loss_change_fraction_; move 33,34 after 31 https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc (right): https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc:20: constexpr int kMinBitreateChangeBps = 5000; add a line break before 20 https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc:43: AudioNetworkAdaptor::EncoderRuntimeConfig runtime_config; it feels that runtime_config does not need to be a state. You need it only because you wanted to create it easily. You may put runtime_config out of states, but an easier fix can be make the verification really depends on states. I suggest void Verify(const EventLogWriterStates& state) { EXPECT_CALL(state.event_log, LogEncoderRuntimeConfig(EncoderRuntimeConfigIs(state.runtime_config))) .Times(1); state.event_log_writer->MayLogEncoderRuntimeConfig(state.runtime_config); } Then everytime you want to verify, call Verify. Before Verify, you can change state. https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc:130: rtc::Optional<int>(kBitrateBps + kMinBitreateChangeBps - 1); bitrate
https://codereview.webrtc.org/2559953002/diff/120001/webrtc/logging/rtc_event... File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/2559953002/diff/120001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:446: event->set_type(rtclog::Event::AUDIO_NETWORK_ADAPTOR_EVENT); On 2016/12/20 10:39:29, minyue-webrtc wrote: > Slightly confusing is that the function is LogEncoderRuntimeConfig and the event > type is AUDIO_NETWORK_ADAPTOR_EVENT; > > Can we change the event type and the function name to > > LogAudioEncoderConfig and AUDIO_ENCODER_CONFIG_EVENT, For brevity, I throw > "runtime" away. Since the log is a time series, "runtime" is implicit. Done. https://codereview.webrtc.org/2559953002/diff/120001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:448: if (config.bitrate_bps) On 2016/12/20 10:39:29, minyue-webrtc wrote: > how about making 448 ~ 460 a utility function, which can be also used in > debug_dump_writer.cc? Then event log here does not need to care the content. > > where to put it? hmm, for simplicity, I don't mind first making it a static > method in debug_dump_writer. The method can be called Done. https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc (right): https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc:19: constexpr int kMinBitreateChangeBps = 5000; Removed https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc:23: EventLogWriter::EventLogWriter(RtcEventLog* event_log) On 2016/12/20 10:39:29, minyue-webrtc wrote: > I'd like to remove this constructor, put kMinBitreateChangeBps and > kMinPacketLossChangeFraction in ana_impl.cc > > but preferably rename a bit: kEventLogMinBitrateChangeBps and > kEventLogMinPacketLossChangeFraction Done. https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc:41: if (last_runtime_config_) { On 2016/12/20 10:39:29, minyue-webrtc wrote: > I prefer adding a > > private > void LogEncoderRuntimeConfig(const AudioNetworkAdaptor::EncoderRuntimeConfig& > config) { > last_runtime_config_ = > rtc::Optional<AudioNetworkAdaptor::EncoderRuntimeConfig>(config); > event_log_->LogEncoderRuntimeConfig(config); > } > > and here > > if (!last_runtime_config_) > return LogEncoderRuntimeConfig(config); > if (xxx) > return LogEncoderRuntimeConfig(config); > if (yyy) > return LogEncoderRuntimeConfig(config); > > break all "||"s > Done. https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.h (right): https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.h:20: class EventLogWriter { On 2016/12/20 10:39:29, minyue-webrtc wrote: > final Done. https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.h:23: int min_bitreate_change_bps, On 2016/12/20 10:39:29, minyue-webrtc wrote: > bitrate Done. https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.h:31: RtcEventLog* event_log_; On 2016/12/20 10:39:29, minyue-webrtc wrote: > RtcEventLog* const Done. https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.h:34: const float min_packet_loss_change_fraction_; On 2016/12/20 10:39:29, minyue-webrtc wrote: > move 33,34 after 31 Done. https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc (right): https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc:20: constexpr int kMinBitreateChangeBps = 5000; On 2016/12/20 10:39:29, minyue-webrtc wrote: > add a line break before 20 Done. https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc:43: AudioNetworkAdaptor::EncoderRuntimeConfig runtime_config; There are some tests which expect that log LogEncoderRuntimeConfig is not called so I can not put the expetion and the "maylog.." function in to the Verify function. I changed the interface of ExpectLogEncoderConfig to take the state. https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc:130: rtc::Optional<int>(kBitrateBps + kMinBitreateChangeBps - 1); On 2016/12/20 10:39:29, minyue-webrtc wrote: > bitrate Done.
thanks! but do you mind considering these remaining issues. https://codereview.webrtc.org/2559953002/diff/120001/webrtc/logging/rtc_event... File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/2559953002/diff/120001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:446: event->set_type(rtclog::Event::AUDIO_NETWORK_ADAPTOR_EVENT); On 2016/12/20 13:41:41, michaelt wrote: > On 2016/12/20 10:39:29, minyue-webrtc wrote: > > Slightly confusing is that the function is LogEncoderRuntimeConfig and the > event > > type is AUDIO_NETWORK_ADAPTOR_EVENT; > > > > Can we change the event type and the function name to > > > > LogAudioEncoderConfig and AUDIO_ENCODER_CONFIG_EVENT, For brevity, I throw > > "runtime" away. Since the log is a time series, "runtime" is implicit. > > Done. Almost, but we cannot drop "Audio" in the function name. https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc (right): https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc:43: AudioNetworkAdaptor::EncoderRuntimeConfig runtime_config; On 2016/12/20 13:41:41, michaelt wrote: > There are some tests which expect that log LogEncoderRuntimeConfig is not called > so I can not put the expetion and the "maylog.." function in to the Verify > function. > > I changed the interface of ExpectLogEncoderConfig to take the state. > I think that can make it worse. now, people need to read both the test and ExpectLogEncoderConfig to see how it works. how about giving an arg to Verify(bool expect_log)? https://codereview.webrtc.org/2559953002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc (right): https://codereview.webrtc.org/2559953002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc:34: if (last_logged_config_) { As mentioned in last comment. if(!last_logged_config_) return LogEncoderConfig(config); But wait, I fount that last_logged_config_ is already a bunch of optionals, why to add optional on top of it? https://codereview.webrtc.org/2559953002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc:66: last_logged_config_ = as mentioned above, do we need to add optional on AudioNetworkAdaptor::EncoderRuntimeConfig https://codereview.webrtc.org/2559953002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc (right): https://codereview.webrtc.org/2559953002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc:135: TEST(EventLogWriterTest, Log5kbpsChangeOnHightBitrateChange) { not good to hardcode 5kbps in the test time https://codereview.webrtc.org/2559953002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc:145: TEST(EventLogWriterTest, Log25PercentBandwidthChangeOnLowBitrateChange) { not good to hard code 25% in the test name.
https://codereview.webrtc.org/2559953002/diff/120001/webrtc/logging/rtc_event... File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/2559953002/diff/120001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:446: event->set_type(rtclog::Event::AUDIO_NETWORK_ADAPTOR_EVENT); Right :) Done https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc (right): https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc:43: AudioNetworkAdaptor::EncoderRuntimeConfig runtime_config; I don't like the Verify idea. Would prefer then to Move the exception back in to the Tests. https://codereview.webrtc.org/2559953002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc (right): https://codereview.webrtc.org/2559953002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc:34: if (last_logged_config_) { Oh missed that one. I made it optional since its not defined for the fist call. But you are right. It don't have to be optional. https://codereview.webrtc.org/2559953002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc:66: last_logged_config_ = On 2016/12/20 14:11:05, minyue-webrtc wrote: > as mentioned above, do we need to add optional on > AudioNetworkAdaptor::EncoderRuntimeConfig Done. https://codereview.webrtc.org/2559953002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc (right): https://codereview.webrtc.org/2559953002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc:135: TEST(EventLogWriterTest, Log5kbpsChangeOnHightBitrateChange) { On 2016/12/20 14:11:05, minyue-webrtc wrote: > not good to hardcode 5kbps in the test time Done. https://codereview.webrtc.org/2559953002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc:145: TEST(EventLogWriterTest, Log25PercentBandwidthChangeOnLowBitrateChange) { On 2016/12/20 14:11:05, minyue-webrtc wrote: > not good to hard code 25% in the test name. Done.
On 2016/12/20 14:34:04, michaelt wrote: > https://codereview.webrtc.org/2559953002/diff/120001/webrtc/logging/rtc_event... > File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): > > https://codereview.webrtc.org/2559953002/diff/120001/webrtc/logging/rtc_event... > webrtc/logging/rtc_event_log/rtc_event_log.cc:446: > event->set_type(rtclog::Event::AUDIO_NETWORK_ADAPTOR_EVENT); > Right :) > Done > > https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... > File > webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc > (right): > > https://codereview.webrtc.org/2559953002/diff/120001/webrtc/modules/audio_cod... > webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc:43: > AudioNetworkAdaptor::EncoderRuntimeConfig runtime_config; > I don't like the Verify idea. Would prefer then to Move the exception back in to > the Tests. > > https://codereview.webrtc.org/2559953002/diff/140001/webrtc/modules/audio_cod... > File webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc > (right): > > https://codereview.webrtc.org/2559953002/diff/140001/webrtc/modules/audio_cod... > webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc:34: if > (last_logged_config_) { > Oh missed that one. > > I made it optional since its not defined for the fist call. But you are right. > It don't have to be optional. > > https://codereview.webrtc.org/2559953002/diff/140001/webrtc/modules/audio_cod... > webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc:66: > last_logged_config_ = > On 2016/12/20 14:11:05, minyue-webrtc wrote: > > as mentioned above, do we need to add optional on > > AudioNetworkAdaptor::EncoderRuntimeConfig > > Done. > > https://codereview.webrtc.org/2559953002/diff/140001/webrtc/modules/audio_cod... > File > webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc > (right): > > https://codereview.webrtc.org/2559953002/diff/140001/webrtc/modules/audio_cod... > webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc:135: > TEST(EventLogWriterTest, Log5kbpsChangeOnHightBitrateChange) { > On 2016/12/20 14:11:05, minyue-webrtc wrote: > > not good to hardcode 5kbps in the test time > > Done. > > https://codereview.webrtc.org/2559953002/diff/140001/webrtc/modules/audio_cod... > webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc:145: > TEST(EventLogWriterTest, Log25PercentBandwidthChangeOnLowBitrateChange) { > On 2016/12/20 14:11:05, minyue-webrtc wrote: > > not good to hard code 25% in the test name. > > Done. LGTM % nits
minyue@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, stefan@webrtc.org - solenberg@webrtc.org
-sun@ since he is on vacation. ~~~~ +stefan@ Hi Stefan, you are the owner of logging, and logging ANA events in event log was your suggestion. ~~~~ +henrik lundin@ for the audio coding changes. https://codereview.webrtc.org/2559953002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc (right): https://codereview.webrtc.org/2559953002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc:43: std::abs(*last_logged_config_.bitrate_bps - *config.bitrate_bps) >= first bit rate will not be logged? https://codereview.webrtc.org/2559953002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc:49: if (last_logged_config_.uplink_packet_loss_fraction && here too https://codereview.webrtc.org/2559953002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc (right): https://codereview.webrtc.org/2559953002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc:133: TEST(EventLogWriterTest, LogOnFrameLengthChange) { maybe LogOn -> Log, everywhere. https://codereview.webrtc.org/2559953002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc:161: TEST(EventLogWriterTest, LogInMinBitrateChangeDistanceOnHightBitrateChange) { Hight->High and maybe LogLargerBitrateChange https://codereview.webrtc.org/2559953002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc:177: TEST(EventLogWriterTest, LogInMinBitrateChangeFractionOnLowBitrateChange) { maybe "LogLargeBitrateChangeFraction" https://codereview.webrtc.org/2559953002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc:207: TEST(EventLogWriterTest, LogBigPacketLossFractionChange) { Big->Large
audio_coding: lgtm with two nits. https://codereview.webrtc.org/2559953002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc (right): https://codereview.webrtc.org/2559953002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc:15: #include "webrtc/logging/rtc_event_log/rtc_event_log.h" Order of includes. https://codereview.webrtc.org/2559953002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc:27: RTC_CHECK(event_log_); RTC_DCHECK
stefan@webrtc.org changed reviewers: + terelius@webrtc.org
I would prefer that terelius reviews this. I don't think I know the code well enough to judge how these new types fit in the protobuf.
https://codereview.webrtc.org/2559953002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc (right): https://codereview.webrtc.org/2559953002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc:15: #include "webrtc/logging/rtc_event_log/rtc_event_log.h" On 2016/12/21 15:13:12, hlundin-webrtc wrote: > Order of includes. Done. https://codereview.webrtc.org/2559953002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc:27: RTC_CHECK(event_log_); On 2016/12/21 15:13:12, hlundin-webrtc wrote: > RTC_DCHECK Done. https://codereview.webrtc.org/2559953002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc:43: std::abs(*last_logged_config_.bitrate_bps - *config.bitrate_bps) >= Changed the impl. so first bit-rate will be logged. https://codereview.webrtc.org/2559953002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc:49: if (last_logged_config_.uplink_packet_loss_fraction && Changed the impl. so first packet_loss will be logged.
Are you planning any tools to visualize or otherwise use the new events? https://codereview.webrtc.org/2559953002/diff/180001/webrtc/logging/rtc_event... File webrtc/logging/rtc_event_log/rtc_event_log.proto (right): https://codereview.webrtc.org/2559953002/diff/180001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.proto:5: import "webrtc/modules/audio_coding/audio_network_adaptor/debug_dump.proto"; I'd prefer to keep all relevant event types in this file. It will make it easier to see what types of data are stored (e.g. reviewing log from a privacy perspective). https://codereview.webrtc.org/2559953002/diff/180001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.proto:42: AUDIO_ENCODER_CONFIG_EVENT = 12; Your use of "CONFIG" is quite different from the other configs above. The send and receive configs are construction-time settings that remain unchanged throughout the call, and they are handled separately from all other events in the log. Not sure it is worth changing the name, but have you considered something like AUDIO_NETWORK_ADAPTATION_EVENT? https://codereview.webrtc.org/2559953002/diff/180001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.proto:74: encoder_config = 12; Field tag numbers in the range [1,15] require one byte less in the serialized stream. I would therefore like to reserve them for the most frequent event types.
Hi Michael, I cannot help updating the CL as promised before your vacation. So I put an update on a separate CL. See https://codereview.webrtc.org/2609583002/ I hope it can help. https://codereview.webrtc.org/2559953002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc (right): https://codereview.webrtc.org/2559953002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc:133: TEST(EventLogWriterTest, LogOnFrameLengthChange) { On 2016/12/21 13:39:43, minyue-webrtc wrote: > maybe LogOn -> Log, everywhere. seems missed https://codereview.webrtc.org/2559953002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc:161: TEST(EventLogWriterTest, LogInMinBitrateChangeDistanceOnHightBitrateChange) { On 2016/12/21 13:39:43, minyue-webrtc wrote: > Hight->High > > and maybe > > LogLargerBitrateChange seems missed https://codereview.webrtc.org/2559953002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc:177: TEST(EventLogWriterTest, LogInMinBitrateChangeFractionOnLowBitrateChange) { On 2016/12/21 13:39:43, minyue-webrtc wrote: > maybe "LogLargeBitrateChangeFraction" seems missed https://codereview.webrtc.org/2559953002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer_unittest.cc:207: TEST(EventLogWriterTest, LogBigPacketLossFractionChange) { On 2016/12/21 13:39:43, minyue-webrtc wrote: > Big->Large seems missed https://codereview.webrtc.org/2559953002/diff/180001/webrtc/logging/rtc_event... File webrtc/logging/rtc_event_log/rtc_event_log.proto (right): https://codereview.webrtc.org/2559953002/diff/180001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.proto:5: import "webrtc/modules/audio_coding/audio_network_adaptor/debug_dump.proto"; On 2016/12/22 16:04:24, terelius wrote: > I'd prefer to keep all relevant event types in this file. It will make it easier > to see what types of data are stored (e.g. reviewing log from a privacy > perspective). Acknowledged. https://codereview.webrtc.org/2559953002/diff/180001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.proto:42: AUDIO_ENCODER_CONFIG_EVENT = 12; On 2016/12/22 16:04:24, terelius wrote: > Your use of "CONFIG" is quite different from the other configs above. The send > and receive configs are construction-time settings that remain unchanged > throughout the call, and they are handled separately from all other events in > the log. Not sure it is worth changing the name, but have you considered > something like AUDIO_NETWORK_ADAPTATION_EVENT? > Sure, I like your suggestion. https://codereview.webrtc.org/2559953002/diff/180001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.proto:74: encoder_config = 12; On 2016/12/22 16:04:24, terelius wrote: > Field tag numbers in the range [1,15] require one byte less in the serialized > stream. I would therefore like to reserve them for the most frequent event > types. ok, that is nice. I'd pick 16 then.
Uploaded changes minyue did in a separate CL. https://codereview.webrtc.org/2609583002/
Thanks. Looks much better, but I still have some comments: Could you add a function to parse the protobuf event please. There is no unit test for the protobuf encoding of ANA events. I suggest adding a test case in webrtc/logging/rtc_event_log/rtc_event_log_unittest.cc that creates an EncoderRuntimeConfig filled with random values, writes it to the log and verifies that parsing the log gives you the same EncoderRuntimeConfig you started with. Are there any plans to visualize or otherwise use the log data? (I'm not suggesting that you add that to the same CL, but I'd like to know how you are intending to use the log.) https://codereview.webrtc.org/2559953002/diff/200001/webrtc/logging/rtc_event... File webrtc/logging/rtc_event_log/rtc_event_log.proto (right): https://codereview.webrtc.org/2559953002/diff/200001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.proto:40: AUDIO_NETWORK_ADAPTATION_EVENT = 12; Possibly change to 16 in order to match the field tag number? Unsure what is best.
https://codereview.webrtc.org/2559953002/diff/200001/webrtc/logging/rtc_event... File webrtc/logging/rtc_event_log/rtc_event_log.proto (right): https://codereview.webrtc.org/2559953002/diff/200001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.proto:235: message AudioNetworkAdaptation { If one PeerConnection can have multiple audio network adaptors, you'll need something (maybe SSRC?) to identify which one changed its state.
I currently use a python script to visualize the audio network adapter dump. We will build a visualization for the event log as well. But it's not yet decide when or how to do this. https://codereview.webrtc.org/2559953002/diff/200001/webrtc/logging/rtc_event... File webrtc/logging/rtc_event_log/rtc_event_log.proto (right): https://codereview.webrtc.org/2559953002/diff/200001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.proto:235: message AudioNetworkAdaptation { At the moment a PeerConnection can just have one audio network adaptor.
Added a parser and a unittest. https://codereview.webrtc.org/2559953002/diff/200001/webrtc/logging/rtc_event... File webrtc/logging/rtc_event_log/rtc_event_log.proto (right): https://codereview.webrtc.org/2559953002/diff/200001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.proto:40: AUDIO_NETWORK_ADAPTATION_EVENT = 12; I think 16 would be ok since the next imported events will then have the right numbers.
The CQ bit was checked by michaelt@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/17696)
The CQ bit was checked by michaelt@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: ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/16380) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/21625) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/21650)
solenberg@webrtc.org changed reviewers: + solenberg@webrtc.org
https://codereview.webrtc.org/2559953002/diff/240001/webrtc/logging/BUILD.gn File webrtc/logging/BUILD.gn (right): https://codereview.webrtc.org/2559953002/diff/240001/webrtc/logging/BUILD.gn#... webrtc/logging/BUILD.gn:44: "../api:call_api", call_api only pulls in "call/audio_sink.h" directly. Is it really needed here, or are you relying on some transitive dependency? https://codereview.webrtc.org/2559953002/diff/240001/webrtc/logging/rtc_event... File webrtc/logging/rtc_event_log/rtc_event_log.proto (right): https://codereview.webrtc.org/2559953002/diff/240001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.proto:235: message AudioNetworkAdaptation { Are all the fields here dynamically changed by ANA? If not, it seems they would be more suitably placed in the AudioSendConfig as codec config params. https://codereview.webrtc.org/2559953002/diff/240001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc (right): https://codereview.webrtc.org/2559953002/diff/240001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc:12: #include <cmath> nit: math.h
https://codereview.webrtc.org/2559953002/diff/200001/webrtc/logging/rtc_event... File webrtc/logging/rtc_event_log/rtc_event_log.proto (right): https://codereview.webrtc.org/2559953002/diff/200001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.proto:235: message AudioNetworkAdaptation { On 2017/01/11 14:41:29, michaelt wrote: > At the moment a PeerConnection can just have one audio network adaptor. > Multi-party calls are not a design goal for ANA? What happens if there nevertheless are multiple AudioSendStreams? Each send stream creates one encoder, but ANA is not configured for any of them?
ANA is optional. So if some decides to use ANA for a Multi-party calls it will just not work very well (Including the logging). If you think we should create ANA for Multi-party call, then I'm clearly the wrong person to talk to.
On 2017/01/12 14:30:28, michaelt wrote: > ANA is optional. So if some decides to use ANA for a Multi-party calls it will > just not work very well (Including the logging). > If you think we should create ANA for Multi-party call, then I'm clearly the > wrong person to talk to. Ok, so ANA is not/will not be turned on by default?
https://codereview.webrtc.org/2559953002/diff/240001/webrtc/logging/BUILD.gn File webrtc/logging/BUILD.gn (right): https://codereview.webrtc.org/2559953002/diff/240001/webrtc/logging/BUILD.gn#... webrtc/logging/BUILD.gn:44: "../api:call_api", Removed i seams this is just a dependency for a old patch and not longer needed. https://codereview.webrtc.org/2559953002/diff/240001/webrtc/logging/rtc_event... File webrtc/logging/rtc_event_log/rtc_event_log.proto (right): https://codereview.webrtc.org/2559953002/diff/240001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.proto:235: message AudioNetworkAdaptation { Yes all of the fields here can be dynamically changed. https://codereview.webrtc.org/2559953002/diff/240001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc (right): https://codereview.webrtc.org/2559953002/diff/240001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/event_log_writer.cc:12: #include <cmath> On 2017/01/12 13:35:06, the sun wrote: > nit: math.h Done.
No. It will just be activate under experiment under certain conditions.
Ok, thanks for explaining. lgtm
The CQ bit was checked by michaelt@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: ios_api_framework on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_api_framework/build...)
On 2017/01/12 14:46:50, terelius wrote: > Ok, thanks for explaining. lgtm lgtm for webrtc/voice_engine/channel.cc
The CQ bit was checked by michaelt@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/21615)
The CQ bit was checked by michaelt@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 checked by michaelt@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: This issue passed the CQ dry run.
The CQ bit was checked by michaelt@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, minyue@webrtc.org, solenberg@webrtc.org, terelius@webrtc.org Link to the patchset: https://codereview.webrtc.org/2559953002/#ps320001 (title: "Revert ana dump changes")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2553413002 Patch 80001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by michaelt@webrtc.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2553413002 Patch 80001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by michaelt@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 320001, "attempt_start_ts": 1484316488330640,
"parent_rev": "bf279fc4b90d8c1c40570ddd361c69831107fbbf", "commit_rev":
"3663681b5d05682f38e75d9eaf6049299f62fb02"}
Message was sent while issue was closed.
Description was changed from ========== Log audio network adapter decisions in event log. BUG=webrtc:6845 ========== to ========== Log audio network adapter decisions in event log. BUG=webrtc:6845 Review-Url: https://codereview.webrtc.org/2559953002 Cr-Commit-Position: refs/heads/master@{#16053} Committed: https://chromium.googlesource.com/external/webrtc/+/3663681b5d05682f38e75d9ea... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:320001) as https://chromium.googlesource.com/external/webrtc/+/3663681b5d05682f38e75d9ea...
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:320001) has been created in https://codereview.webrtc.org/2631703002/ by sakal@webrtc.org. The reason for reverting is: Breaks chromium.webrtc.fyi.. |
