|
|
Created:
5 years, 4 months ago by ivoc Modified:
5 years, 3 months ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, rwolff_gocast.it, yujie_mao (webrtc), stefan-webrtc, Andrew MacDonald, henrika_webrtc, hlundin-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, mflodman, kwiberg-webrtc, perkj_webrtc, andresp Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionHooked up RtcEventLog. It lives in Voice Engine and pointers are propagated to ACM and Call.
An option was added to voe_cmd_test to make a RtcEventLog dump.
BUG=webrtc:4741
Committed: https://crrev.com/b04965ccf83c2bc6e2758abab9bea0c18551a54c
Cr-Commit-Position: refs/heads/master@{#9901}
Patch Set 1 : Initial version #
Total comments: 2
Patch Set 2 : Added nullptr check after call to GetInterface(), as suggested by Terelius@ #Patch Set 3 : Moved GetEventLog from VoEBase to VoECodec, based on suggestion from Minyue@ #Patch Set 4 : Added missing dependencies to gn/gyp build files and suppressed warning in ACM gn file. #Patch Set 5 : Rebased to latest revision of webrtc to resolve patching conflict on try bots. #
Total comments: 19
Patch Set 6 : Addressed Henrik's review comments. #
Total comments: 2
Patch Set 7 : Processed more comments from Henrik. #
Total comments: 30
Patch Set 8 : All SetEventLog() functions have been removed, the pointer is now propagated through the constructo… #
Total comments: 12
Patch Set 9 : Moved RtcEventLog from SharedData to ChannelManager. #
Total comments: 5
Patch Set 10 : Added comment regarding pointer lifetime on the VoECodec sub-API. #
Total comments: 5
Patch Set 11 : Added a unittest in codec_test.cc, to test the integration in VoE. #
Total comments: 5
Patch Set 12 : Changed EXCEPT to ASSERT for the check to see if a logfile is created. #Patch Set 13 : Rebase #
Total comments: 7
Patch Set 14 : Addressed Stefan's review comments. #Patch Set 15 : Rebase #Patch Set 16 : Another rebase #Messages
Total messages: 83 (21 generated)
ivoc@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, henrikg@webrtc.org, stefan@webrtc.org, terelius@webrtc.org
Please have a look at this CL to hook up RtcEventLog.
Nice to have this hooked up! https://codereview.webrtc.org/1267683002/diff/1/webrtc/video/call.cc File webrtc/video/call.cc (right): https://codereview.webrtc.org/1267683002/diff/1/webrtc/video/call.cc#newcode182 webrtc/video/call.cc:182: voe_base->Release(); I don't understand what 'sub-API' means in the documentation for GetInterface and Release, so I have no idea whether it is safe to call Release while still holding a pointer to the object. It seems to go against the idea of reference counting... In any case, GetInterface is allowed to fail and return a null pointer according to the documentation. I think it we should test for this before setting event_log_ abd calling Release
https://codereview.webrtc.org/1267683002/diff/1/webrtc/video/call.cc File webrtc/video/call.cc (right): https://codereview.webrtc.org/1267683002/diff/1/webrtc/video/call.cc#newcode182 webrtc/video/call.cc:182: voe_base->Release(); On 2015/07/30 12:34:43, terelius wrote: > I don't understand what 'sub-API' means in the documentation for GetInterface > and Release, so I have no idea whether it is safe to call Release while still > holding a pointer to the object. It seems to go against the idea of reference > counting... > > In any case, GetInterface is allowed to fail and return a null pointer according > to the documentation. I think it we should test for this before setting > event_log_ abd calling Release Good point on null pointer, I added a check. About the GetInterface() and Release() calls, I still think this is the way the VoE sub-APIs are supposed to be used, based on other examples in the codebase. I suggest we wait for Henrik to return from his vacation and have a look at this, and leave it as is for now.
The CQ bit was checked by ivoc@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1267683002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1267683002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_gn_rel/builds/4818)
The CQ bit was checked by ivoc@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1267683002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1267683002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_clang/builds/6860) android_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_rel/builds/4916) ios_arm64 on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64/builds/3675) linux on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux/builds/8847) linux_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/8619) linux_gn on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_gn/builds/4868) linux_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_gn_rel/builds/4825) linux_tsan2 on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/6038)
Looks good in general. I only have a few minor comments. https://codereview.webrtc.org/1267683002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/BUILD.gn (right): https://codereview.webrtc.org/1267683002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/BUILD.gn:62: # Suppress warnings from Chrome's Clang plugins. Please, file a new bug about this specific suppression, explain why it was needed (copy-paste the error message), and assign to pbos. https://codereview.webrtc.org/1267683002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.h (right): https://codereview.webrtc.org/1267683002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.h:334: RtcEventLog* event_log_; I wonder if you should protect the pointer (not the RtcEventLog itself, since it is already thread-safe, but the address) with a lock. There is a risk that you are using and changing the pointer at the same time. https://codereview.webrtc.org/1267683002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/main/interface/audio_coding_module.h (right): https://codereview.webrtc.org/1267683002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/interface/audio_coding_module.h:985: // Set an RtcEventLog object to enable logging of debug events inside the Sets... https://codereview.webrtc.org/1267683002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/interface/audio_coding_module.h:986: // audio coding module. Explain that calling this function is optional, i.e., that the default means not having an event log, which is fine too. https://codereview.webrtc.org/1267683002/diff/80001/webrtc/video/call.cc File webrtc/video/call.cc (right): https://codereview.webrtc.org/1267683002/diff/80001/webrtc/video/call.cc#newc... webrtc/video/call.cc:529: event_log_->LogRtcpPacket(false, media_type, packet, length); This looks like the same RTCP packet being logged multiple times, but I guess that stream->DeliverRtcp will only return true once across both these for loops, correct? https://codereview.webrtc.org/1267683002/diff/80001/webrtc/video/call.cc#newc... webrtc/video/call.cc:548: event_log_->LogRtpHeader(true, media_type, packet, 20, length); What is "20"? https://codereview.webrtc.org/1267683002/diff/80001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1267683002/diff/80001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.h:452: // Set an RtcEventLog object to enable logging of debug events in the Audio Sets... https://codereview.webrtc.org/1267683002/diff/80001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.h:453: // Coding Module. Also, is nullptr a valid input value? Does that stop the logging in a controlled manner?
https://codereview.webrtc.org/1267683002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/BUILD.gn (right): https://codereview.webrtc.org/1267683002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/BUILD.gn:62: # Suppress warnings from Chrome's Clang plugins. On 2015/08/04 09:14:47, hlundin-webrtc wrote: > Please, file a new bug about this specific suppression, explain why it was > needed (copy-paste the error message), and assign to pbos. Done, issue URL: https://code.google.com/p/webrtc/issues/detail?id=4895 https://codereview.webrtc.org/1267683002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.h (right): https://codereview.webrtc.org/1267683002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.h:334: RtcEventLog* event_log_; On 2015/08/04 09:14:47, hlundin-webrtc wrote: > I wonder if you should protect the pointer (not the RtcEventLog itself, since it > is already thread-safe, but the address) with a lock. There is a risk that you > are using and changing the pointer at the same time. I didn't think about that, but I guess you're right. I added a lock for the pointer. https://codereview.webrtc.org/1267683002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/main/interface/audio_coding_module.h (right): https://codereview.webrtc.org/1267683002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/interface/audio_coding_module.h:985: // Set an RtcEventLog object to enable logging of debug events inside the On 2015/08/04 09:14:47, hlundin-webrtc wrote: > Sets... Done. https://codereview.webrtc.org/1267683002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/interface/audio_coding_module.h:986: // audio coding module. On 2015/08/04 09:14:47, hlundin-webrtc wrote: > Explain that calling this function is optional, i.e., that the default means not > having an event log, which is fine too. Done. https://codereview.webrtc.org/1267683002/diff/80001/webrtc/video/call.cc File webrtc/video/call.cc (right): https://codereview.webrtc.org/1267683002/diff/80001/webrtc/video/call.cc#newc... webrtc/video/call.cc:529: event_log_->LogRtcpPacket(false, media_type, packet, length); On 2015/08/04 09:14:47, hlundin-webrtc wrote: > This looks like the same RTCP packet being logged multiple times, but I guess > that stream->DeliverRtcp will only return true once across both these for loops, > correct? That is what I was hoping for, but I am not entirely sure about this. I changed the implementation to avoid the issue altogether https://codereview.webrtc.org/1267683002/diff/80001/webrtc/video/call.cc#newc... webrtc/video/call.cc:548: event_log_->LogRtpHeader(true, media_type, packet, 20, length); On 2015/08/04 09:14:47, hlundin-webrtc wrote: > What is "20"? It indicates that only the first 20 bytes of the packet should be logged, which is a crude way to ensure that no payload data will be logged. Terelius@ is working on a CL to split the packet into header and payload properly (i.e. respecting header extensions) inside the RtcEventLog class, so this is just a temporary crutch (hence the TODO). https://codereview.webrtc.org/1267683002/diff/80001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1267683002/diff/80001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.h:452: // Set an RtcEventLog object to enable logging of debug events in the Audio On 2015/08/04 09:14:47, hlundin-webrtc wrote: > Sets... Done. https://codereview.webrtc.org/1267683002/diff/80001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.h:453: // Coding Module. On 2015/08/04 09:14:47, hlundin-webrtc wrote: > Also, is nullptr a valid input value? Does that stop the logging in a controlled > manner? It is valid to set a nullptr to stop logging. Added a comment here as well as in the ACM header.
Good. A few more comments, then I will be happy. https://codereview.webrtc.org/1267683002/diff/80001/webrtc/video/call.cc File webrtc/video/call.cc (right): https://codereview.webrtc.org/1267683002/diff/80001/webrtc/video/call.cc#newc... webrtc/video/call.cc:529: event_log_->LogRtcpPacket(false, media_type, packet, length); On 2015/08/05 07:39:04, ivoc wrote: > On 2015/08/04 09:14:47, hlundin-webrtc wrote: > > This looks like the same RTCP packet being logged multiple times, but I guess > > that stream->DeliverRtcp will only return true once across both these for > loops, > > correct? > > That is what I was hoping for, but I am not entirely sure about this. I changed > the implementation to avoid the issue altogether I think this made things worse. Keep it as it was. We need to understand what happens here. https://codereview.webrtc.org/1267683002/diff/80001/webrtc/video/call.cc#newc... webrtc/video/call.cc:548: event_log_->LogRtpHeader(true, media_type, packet, 20, length); On 2015/08/05 07:39:04, ivoc wrote: > On 2015/08/04 09:14:47, hlundin-webrtc wrote: > > What is "20"? > > It indicates that only the first 20 bytes of the packet should be logged, which > is a crude way to ensure that no payload data will be logged. > Terelius@ is working on a CL to split the packet into header and payload > properly (i.e. respecting header extensions) inside the RtcEventLog class, so > this is just a temporary crutch (hence the TODO). Acknowledged. https://codereview.webrtc.org/1267683002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.h (right): https://codereview.webrtc.org/1267683002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.h:335: RtcEventLog* event_log_ GUARDED_BY(event_log_crit_sect_); Perhaps name it event_log_ptr_crit_sect_ to clarify that it is the pointer that is protected, not the pointee.
https://codereview.webrtc.org/1267683002/diff/80001/webrtc/video/call.cc File webrtc/video/call.cc (right): https://codereview.webrtc.org/1267683002/diff/80001/webrtc/video/call.cc#newc... webrtc/video/call.cc:529: event_log_->LogRtcpPacket(false, media_type, packet, length); On 2015/08/05 08:32:13, hlundin-webrtc wrote: > On 2015/08/05 07:39:04, ivoc wrote: > > On 2015/08/04 09:14:47, hlundin-webrtc wrote: > > > This looks like the same RTCP packet being logged multiple times, but I > guess > > > that stream->DeliverRtcp will only return true once across both these for > > loops, > > > correct? > > > > That is what I was hoping for, but I am not entirely sure about this. I > changed > > the implementation to avoid the issue altogether > > I think this made things worse. Keep it as it was. We need to understand what > happens here. Ok, reverted to the previous version. https://codereview.webrtc.org/1267683002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.h (right): https://codereview.webrtc.org/1267683002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.h:335: RtcEventLog* event_log_ GUARDED_BY(event_log_crit_sect_); On 2015/08/05 08:32:13, hlundin-webrtc wrote: > Perhaps name it event_log_ptr_crit_sect_ to clarify that it is the pointer that > is protected, not the pointee. Done.
webrtc/modules/audio_coding: lgtm
https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:455: void SetEventLog(RtcEventLog* event_log); Use a separate function for disabling the logging. It makes it clearer. https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/incl... File webrtc/voice_engine/include/voe_codec.h (right): https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/incl... webrtc/voice_engine/include/voe_codec.h:135: // Get a pointer to the event logging object associated with this Voice Is the event logging object related to codec(s) only? Should this function perhaps be in another "sub-API"? https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/incl... webrtc/voice_engine/include/voe_codec.h:137: virtual RtcEventLog* GetEventLog() = 0; Instead of getting a pointer and operate directly on that object, add functions here that do what we need. E.g. Enable, Disable. The internal object should be hidden. https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/shar... File webrtc/voice_engine/shared_data.cc (right): https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/shar... webrtc/voice_engine/shared_data.cc:34: _eventLog(RtcEventLog::Create()), Create the RtcEventLog object when needed, i.e. when the logging is enabled.
https://codereview.webrtc.org/1267683002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc (right): https://codereview.webrtc.org/1267683002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc:150: event_log_ptr_crit_sect_(CriticalSectionWrapper::CreateCriticalSection()), I got the following comment from pbos@ concering CriticalSectionWrapper "Use a rtc::CriticalSection (this doesn't require additional heap allocation)." "We're trying to move off webrtc::CriticalSectionWrapper." What's the policy here? Another question is whether we need a lock in the first place. If i understood the argument, the point is to prevent anyone from changing the pointer itself (and not the event log object) while someone else is reading it. Could this be solved in some other way, e.g. by making the pointer itself const? https://codereview.webrtc.org/1267683002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc:744: if (event_log_) I thought the reason for having a no-op implementation of the RtcEventLog was so that would not have to do these checks everywhere we want to log something.
https://codereview.webrtc.org/1267683002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc (right): https://codereview.webrtc.org/1267683002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc:150: event_log_ptr_crit_sect_(CriticalSectionWrapper::CreateCriticalSection()), On 2015/08/05 10:35:42, terelius wrote: > I got the following comment from pbos@ concering CriticalSectionWrapper > "Use a rtc::CriticalSection (this doesn't require additional heap allocation)." > "We're trying to move off webrtc::CriticalSectionWrapper." > What's the policy here? > > Another question is whether we need a lock in the first place. If i understood > the argument, the point is to prevent anyone from changing the pointer itself > (and not the event log object) while someone else is reading it. Could this be > solved in some other way, e.g. by making the pointer itself const? Yes, that was my concern. If you can make the design such that the pointer is initialized in the constructors initialization list, and that it will never change, then go ahead. But that means that you cannot deregister the event logger by changing to a nullptr. https://codereview.webrtc.org/1267683002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc:744: if (event_log_) On 2015/08/05 10:35:42, terelius wrote: > I thought the reason for having a no-op implementation of the RtcEventLog was so > that would not have to do these checks everywhere we want to log something. Having a no-op implementation is not the same as not having a valid pointer.
https://codereview.webrtc.org/1267683002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc (right): https://codereview.webrtc.org/1267683002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc:150: event_log_ptr_crit_sect_(CriticalSectionWrapper::CreateCriticalSection()), On 2015/08/05 11:02:06, hlundin-webrtc wrote: > On 2015/08/05 10:35:42, terelius wrote: > > I got the following comment from pbos@ concering CriticalSectionWrapper > > "Use a rtc::CriticalSection (this doesn't require additional heap > allocation)." > > "We're trying to move off webrtc::CriticalSectionWrapper." > > What's the policy here? > > > > Another question is whether we need a lock in the first place. If i understood > > the argument, the point is to prevent anyone from changing the pointer itself > > (and not the event log object) while someone else is reading it. Could this be > > solved in some other way, e.g. by making the pointer itself const? > > Yes, that was my concern. If you can make the design such that the pointer is > initialized in the constructors initialization list, and that it will never > change, then go ahead. But that means that you cannot deregister the event > logger by changing to a nullptr. So which option do we prefer? I would like to hear your opinions before addressing this, since this would require a rewrite of about 70% of this CL and also affects how I should address some of the other comments. I agree that it would be nice to get rid of the lock here, on the other hand being able to disable logging by setting a nullptr could also come in handy some day. https://codereview.webrtc.org/1267683002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc:744: if (event_log_) On 2015/08/05 11:02:06, hlundin-webrtc wrote: > On 2015/08/05 10:35:42, terelius wrote: > > I thought the reason for having a no-op implementation of the RtcEventLog was > so > > that would not have to do these checks everywhere we want to log something. > > Having a no-op implementation is not the same as not having a valid pointer. If I'm not mistaken, the ACM is also used standalone (without a corresponding voice engine). So it's hard to avoid checking this pointer, since in the standalone case there probably won't be a possibility to log. https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/incl... File webrtc/voice_engine/include/voe_codec.h (right): https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/incl... webrtc/voice_engine/include/voe_codec.h:135: // Get a pointer to the event logging object associated with this Voice On 2015/08/05 09:44:35, Henrik Grunell (webrtc) wrote: > Is the event logging object related to codec(s) only? Should this function > perhaps be in another "sub-API"? I was not entirely sure which of the sub-APIs is most appropriate for this functionality. Initially I chose VoEBase, but Minyue@ said VoECodec would be more appropriate because the actual logging is done in the ACM, and most of the functions that relate to ACM are in VoECodec. If you have a better idea, feel free to share. https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/incl... webrtc/voice_engine/include/voe_codec.h:137: virtual RtcEventLog* GetEventLog() = 0; On 2015/08/05 09:44:35, Henrik Grunell (webrtc) wrote: > Instead of getting a pointer and operate directly on that object, add functions > here that do what we need. E.g. Enable, Disable. The internal object should be > hidden. We need more functionality than just enable/disable. The pointer that is obtained from this function is used in Call.cc to log RTP packets, RTCP packets and video send/receive configs. We would have to basically replicate the whole interface from the rtc_event_log.h header here, is that really desirable?
https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:3457: void Channel::SetEventLog(RtcEventLog* event_log) { Where is RtcEventLog defined? https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/incl... File webrtc/voice_engine/include/voe_codec.h (right): https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/incl... webrtc/voice_engine/include/voe_codec.h:135: // Get a pointer to the event logging object associated with this Voice On 2015/08/06 10:57:51, ivoc wrote: > On 2015/08/05 09:44:35, Henrik Grunell (webrtc) wrote: > > Is the event logging object related to codec(s) only? Should this function > > perhaps be in another "sub-API"? > > I was not entirely sure which of the sub-APIs is most appropriate for this > functionality. Initially I chose VoEBase, but Minyue@ said VoECodec would be > more appropriate because the actual logging is done in the ACM, and most of the > functions that relate to ACM are in VoECodec. If you have a better idea, feel > free to share. Right, it's given to |audio_coding_| in the channel. This seems to be the right place. https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/incl... webrtc/voice_engine/include/voe_codec.h:137: virtual RtcEventLog* GetEventLog() = 0; On 2015/08/06 10:57:52, ivoc wrote: > On 2015/08/05 09:44:35, Henrik Grunell (webrtc) wrote: > > Instead of getting a pointer and operate directly on that object, add > functions > > here that do what we need. E.g. Enable, Disable. The internal object should be > > hidden. > > We need more functionality than just enable/disable. The pointer that is > obtained from this function is used in Call.cc to log RTP packets, RTCP packets > and video send/receive configs. We would have to basically replicate the whole > interface from the rtc_event_log.h header here, is that really desirable? OK. In that case I'd prefer the VoE user to have ownership of that object and pass it to VoE. Let'd discuss offline tomorrow.
https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:3457: void Channel::SetEventLog(RtcEventLog* event_log) { On 2015/08/06 15:30:45, Henrik Grunell (webrtc) wrote: > Where is RtcEventLog defined? Found it. https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:455: void SetEventLog(RtcEventLog* event_log); On 2015/08/05 09:44:35, Henrik Grunell (webrtc) wrote: > Use a separate function for disabling the logging. It makes it clearer. Please address this. https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/incl... File webrtc/voice_engine/include/voe_codec.h (right): https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/incl... webrtc/voice_engine/include/voe_codec.h:137: virtual RtcEventLog* GetEventLog() = 0; On 2015/08/06 15:30:45, Henrik Grunell (webrtc) wrote: > On 2015/08/06 10:57:52, ivoc wrote: > > On 2015/08/05 09:44:35, Henrik Grunell (webrtc) wrote: > > > Instead of getting a pointer and operate directly on that object, add > > functions > > > here that do what we need. E.g. Enable, Disable. The internal object should > be > > > hidden. > > > > We need more functionality than just enable/disable. The pointer that is > > obtained from this function is used in Call.cc to log RTP packets, RTCP > packets > > and video send/receive configs. We would have to basically replicate the whole > > interface from the rtc_event_log.h header here, is that really desirable? > > OK. In that case I'd prefer the VoE user to have ownership of that object and > pass it to VoE. Let'd discuss offline tomorrow. We talked offline and since (1) the path in libjingle between VoE and Call is quite long with many layers, (2) the event log will eventually be moved completely to Call, and (3) VoE outlives Call, I think it's OK to do it this way. https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/shar... File webrtc/voice_engine/shared_data.cc (right): https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/shar... webrtc/voice_engine/shared_data.cc:34: _eventLog(RtcEventLog::Create()), On 2015/08/05 09:44:35, Henrik Grunell (webrtc) wrote: > Create the RtcEventLog object when needed, i.e. when the logging is enabled. Since we keep the Get function in the VoECodec API, create the event log object at first Get.
https://codereview.webrtc.org/1267683002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc (right): https://codereview.webrtc.org/1267683002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc:744: if (event_log_) On 2015/08/06 10:57:51, ivoc wrote: > On 2015/08/05 11:02:06, hlundin-webrtc wrote: > > On 2015/08/05 10:35:42, terelius wrote: > > > I thought the reason for having a no-op implementation of the RtcEventLog > was > > so > > > that would not have to do these checks everywhere we want to log something. > > > > Having a no-op implementation is not the same as not having a valid pointer. > > If I'm not mistaken, the ACM is also used standalone (without a corresponding > voice engine). So it's hard to avoid checking this pointer, since in the > standalone case there probably won't be a possibility to log. Good point Ivo. It would still be able to point at the no-op implementation I think, though perhaps that's not worth it. https://codereview.webrtc.org/1267683002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc:744: if (event_log_) On 2015/08/05 11:02:06, hlundin-webrtc wrote: > On 2015/08/05 10:35:42, terelius wrote: > > I thought the reason for having a no-op implementation of the RtcEventLog was > so > > that would not have to do these checks everywhere we want to log something. > > Having a no-op implementation is not the same as not having a valid pointer. Indeed. However, the proposed checks do not guarantee a "valid" pointer, merely a non-null pointer. In other words, we test whether someone has explicitly disabled logging by setting the pointer to null which (for most purposes) is equivalent to pointing at the no-op implementation. https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:455: void SetEventLog(RtcEventLog* event_log); On 2015/08/07 11:30:03, Henrik Grunell (webrtc) wrote: > On 2015/08/05 09:44:35, Henrik Grunell (webrtc) wrote: > > Use a separate function for disabling the logging. It makes it clearer. > > Please address this. The main way to stop logging is to call StopLogging in RtcEventLog. Manipulating the pointer was never intended as the primary method for changing logging behavior. https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/shar... File webrtc/voice_engine/shared_data.cc (right): https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/shar... webrtc/voice_engine/shared_data.cc:34: _eventLog(RtcEventLog::Create()), On 2015/08/07 11:30:03, Henrik Grunell (webrtc) wrote: > On 2015/08/05 09:44:35, Henrik Grunell (webrtc) wrote: > > Create the RtcEventLog object when needed, i.e. when the logging is enabled. > > Since we keep the Get function in the VoECodec API, create the event log object > at first Get. The idea is that we should be able to log "back in time", i.e. we should be able to store events that happened before the user enabled logging. I have no idea when Get is normally called, but creating the logging object at an unspecified time during the call might cause a failure to log early events.
I fear we are over-designing this. Right now we have an abstract base, but no run-time polymorphism. We have at least three different ways to disable logging. Based on reviewer comments, the general trend seems to lean towards an even more dynamic implementation. All of this requires pointers, which requires locks, which requires extra code. I wouldn't mind the complexity if there was a clear reason for it, but at this point we don't even know how large performance hit we'll take from logging every packet/header or how the user will enable logging. I'd suggest looking for a simpler solution instead of the most general one.
Thanks for the feedback. I agree with the general sentiment of your comment, so I will rewrite this CL to avoid the Set functions and use the constructor instead to pass a const pointer (which doesn't require a lock) to the ACM. The performance impact of the logging should definitely be investigated further in the near future.
https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:455: void SetEventLog(RtcEventLog* event_log); On 2015/08/11 09:04:34, terelius wrote: > On 2015/08/07 11:30:03, Henrik Grunell (webrtc) wrote: > > On 2015/08/05 09:44:35, Henrik Grunell (webrtc) wrote: > > > Use a separate function for disabling the logging. It makes it clearer. > > > > Please address this. > > The main way to stop logging is to call StopLogging in RtcEventLog. Manipulating > the pointer was never intended as the primary method for changing logging > behavior. OK, well, the comment says differently. :) We need to decide if it can be disabled with (if so I prefer a separate disable function + assert in the set function that it wasn't set before) or not. https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/shar... File webrtc/voice_engine/shared_data.cc (right): https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/shar... webrtc/voice_engine/shared_data.cc:34: _eventLog(RtcEventLog::Create()), On 2015/08/11 09:04:34, terelius wrote: > On 2015/08/07 11:30:03, Henrik Grunell (webrtc) wrote: > > On 2015/08/05 09:44:35, Henrik Grunell (webrtc) wrote: > > > Create the RtcEventLog object when needed, i.e. when the logging is enabled. > > > > Since we keep the Get function in the VoECodec API, create the event log > object > > at first Get. > > The idea is that we should be able to log "back in time", i.e. we should be able > to store events that happened before the user enabled logging. I have no idea > when Get is normally called, but creating the logging object at an unspecified > time during the call might cause a failure to log early events. That would mean that logging is enabled from the start really. What happens when you "enable logging"? How far back is stored?
https://codereview.webrtc.org/1267683002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel_manager.cc (right): https://codereview.webrtc.org/1267683002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_manager.cc:63: RtcEventLog* const event_log) { Will we ever use another event log than the one given in the ctor? Could we let ChannelManager own it and remove it from SharedData?
https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/shar... File webrtc/voice_engine/shared_data.cc (right): https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/shar... webrtc/voice_engine/shared_data.cc:34: _eventLog(RtcEventLog::Create()), On 2015/08/12 12:38:50, Henrik Grunell (webrtc) wrote: > On 2015/08/11 09:04:34, terelius wrote: > > On 2015/08/07 11:30:03, Henrik Grunell (webrtc) wrote: > > > On 2015/08/05 09:44:35, Henrik Grunell (webrtc) wrote: > > > > Create the RtcEventLog object when needed, i.e. when the logging is > enabled. > > > > > > Since we keep the Get function in the VoECodec API, create the event log > > object > > > at first Get. > > > > The idea is that we should be able to log "back in time", i.e. we should be > able > > to store events that happened before the user enabled logging. I have no idea > > when Get is normally called, but creating the logging object at an unspecified > > time during the call might cause a failure to log early events. > > That would mean that logging is enabled from the start really. What happens when > you "enable logging"? How far back is stored? Poorly explained by me. The event log has an internal buffer which stores the recent history. The default history is 10 seconds, but it is possible that this should be increased to 30 seconds or so, depending on what memory usage we get in real calls. When the user presses the "Send feedback" button, the recent history is written to a file and any future events are appended to the same file. It is important that the event log exists when the ReceiveStreams/SendStreams (the corresponding audio object is voe, right?) are created since we need to store configuration information. Without the configuration information we can't parse the RTP headers.
All the SetEventLog functions have been removed now, the pointer to the RtcEventLog object is now propagated through the constructors of ChannelManager, Channel to the ACM. This avoids the use of the Critical Section to protect the pointer in the ACM, because the pointer is const. https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:455: void SetEventLog(RtcEventLog* event_log); On 2015/08/12 12:38:50, Henrik Grunell (webrtc) wrote: > On 2015/08/11 09:04:34, terelius wrote: > > On 2015/08/07 11:30:03, Henrik Grunell (webrtc) wrote: > > > On 2015/08/05 09:44:35, Henrik Grunell (webrtc) wrote: > > > > Use a separate function for disabling the logging. It makes it clearer. > > > > > > Please address this. > > > > The main way to stop logging is to call StopLogging in RtcEventLog. > Manipulating > > the pointer was never intended as the primary method for changing logging > > behavior. > > OK, well, the comment says differently. :) We need to decide if it can be > disabled with (if so I prefer a separate disable function + assert in the set > function that it wasn't set before) or not. The SetEventLog function is removed in the latest version (the pointer is now propagated through the constructors), so making a StopLogging function here does not make sense any more in this new design. https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/shar... File webrtc/voice_engine/shared_data.cc (right): https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/shar... webrtc/voice_engine/shared_data.cc:34: _eventLog(RtcEventLog::Create()), On 2015/08/12 13:05:05, terelius wrote: > On 2015/08/12 12:38:50, Henrik Grunell (webrtc) wrote: > > On 2015/08/11 09:04:34, terelius wrote: > > > On 2015/08/07 11:30:03, Henrik Grunell (webrtc) wrote: > > > > On 2015/08/05 09:44:35, Henrik Grunell (webrtc) wrote: > > > > > Create the RtcEventLog object when needed, i.e. when the logging is > > enabled. > > > > > > > > Since we keep the Get function in the VoECodec API, create the event log > > > object > > > > at first Get. > > > > > > The idea is that we should be able to log "back in time", i.e. we should be > > able > > > to store events that happened before the user enabled logging. I have no > idea > > > when Get is normally called, but creating the logging object at an > unspecified > > > time during the call might cause a failure to log early events. > > > > That would mean that logging is enabled from the start really. What happens > when > > you "enable logging"? How far back is stored? > > Poorly explained by me. The event log has an internal buffer which stores the > recent history. The default history is 10 seconds, but it is possible that this > should be increased to 30 seconds or so, depending on what memory usage we get > in real calls. > When the user presses the "Send feedback" button, the recent history is written > to a file and any future events are appended to the same file. > > It is important that the event log exists when the ReceiveStreams/SendStreams > (the corresponding audio object is voe, right?) are created since we need to > store configuration information. Without the configuration information we can't > parse the RTP headers. In the latest version the pointer to RtcEventLog is propagated through the constructor, so it's hard to avoid initializing the RtcEventLog object within the constructor as well. https://codereview.webrtc.org/1267683002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel_manager.cc (right): https://codereview.webrtc.org/1267683002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_manager.cc:63: RtcEventLog* const event_log) { On 2015/08/12 12:43:45, Henrik Grunell (webrtc) wrote: > Will we ever use another event log than the one given in the ctor? Could we let > ChannelManager own it and remove it from SharedData? That is certainly an option, do you think it would be better to let ChannelManager own it than SharedData?
I can't find any tests for this except the command line test. We need some test(s), preferably unit test + integration (is VoE auto_test used? Run in bots?). https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:455: void SetEventLog(RtcEventLog* event_log); On 2015/08/12 13:16:41, ivoc wrote: > On 2015/08/12 12:38:50, Henrik Grunell (webrtc) wrote: > > On 2015/08/11 09:04:34, terelius wrote: > > > On 2015/08/07 11:30:03, Henrik Grunell (webrtc) wrote: > > > > On 2015/08/05 09:44:35, Henrik Grunell (webrtc) wrote: > > > > > Use a separate function for disabling the logging. It makes it clearer. > > > > > > > > Please address this. > > > > > > The main way to stop logging is to call StopLogging in RtcEventLog. > > Manipulating > > > the pointer was never intended as the primary method for changing logging > > > behavior. > > > > OK, well, the comment says differently. :) We need to decide if it can be > > disabled with (if so I prefer a separate disable function + assert in the set > > function that it wasn't set before) or not. > > The SetEventLog function is removed in the latest version (the pointer is now > propagated through the constructors), so making a StopLogging function here does > not make sense any more in this new design. Acknowledged. https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/shar... File webrtc/voice_engine/shared_data.cc (right): https://codereview.webrtc.org/1267683002/diff/120001/webrtc/voice_engine/shar... webrtc/voice_engine/shared_data.cc:34: _eventLog(RtcEventLog::Create()), On 2015/08/12 13:05:05, terelius wrote: > On 2015/08/12 12:38:50, Henrik Grunell (webrtc) wrote: > > On 2015/08/11 09:04:34, terelius wrote: > > > On 2015/08/07 11:30:03, Henrik Grunell (webrtc) wrote: > > > > On 2015/08/05 09:44:35, Henrik Grunell (webrtc) wrote: > > > > > Create the RtcEventLog object when needed, i.e. when the logging is > > enabled. > > > > > > > > Since we keep the Get function in the VoECodec API, create the event log > > > object > > > > at first Get. > > > > > > The idea is that we should be able to log "back in time", i.e. we should be > > able > > > to store events that happened before the user enabled logging. I have no > idea > > > when Get is normally called, but creating the logging object at an > unspecified > > > time during the call might cause a failure to log early events. > > > > That would mean that logging is enabled from the start really. What happens > when > > you "enable logging"? How far back is stored? > > Poorly explained by me. The event log has an internal buffer which stores the > recent history. The default history is 10 seconds, but it is possible that this > should be increased to 30 seconds or so, depending on what memory usage we get > in real calls. > When the user presses the "Send feedback" button, the recent history is written > to a file and any future events are appended to the same file. > > It is important that the event log exists when the ReceiveStreams/SendStreams > (the corresponding audio object is voe, right?) are created since we need to > store configuration information. Without the configuration information we can't > parse the RTP headers. OK, then it makes sense to create it at start. https://codereview.webrtc.org/1267683002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1267683002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:752: _timeStamp(0), // This is just an offset, RTP module will add it's own Nit: Put the comment above, follows the style above. https://codereview.webrtc.org/1267683002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1267683002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:174: RtcEventLog* const event_log, Remove const. https://codereview.webrtc.org/1267683002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:178: RtcEventLog* const event_log, Same here. https://codereview.webrtc.org/1267683002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel_manager.cc (right): https://codereview.webrtc.org/1267683002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_manager.cc:63: RtcEventLog* const event_log) { On 2015/08/12 13:16:41, ivoc wrote: > On 2015/08/12 12:43:45, Henrik Grunell (webrtc) wrote: > > Will we ever use another event log than the one given in the ctor? Could we > let > > ChannelManager own it and remove it from SharedData? > > That is certainly an option, do you think it would be better to let > ChannelManager own it than SharedData? I think the raw pointer should be avoided if not necessary, which it isn't then. If we let SharedData or ChannelManager own doesn't matter that much. https://codereview.webrtc.org/1267683002/diff/140001/webrtc/voice_engine/incl... File webrtc/voice_engine/include/voe_codec.h (right): https://codereview.webrtc.org/1267683002/diff/140001/webrtc/voice_engine/incl... webrtc/voice_engine/include/voe_codec.h:136: // Engine. Nit: "VoiceEngine" (no space)
There are some pretty extensive unittests for the RtcEventLog class in webrtc/video/rtc_event_log_unittest.cc . Do you think it's useful to add more tests for the integration as well? https://codereview.webrtc.org/1267683002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel_manager.cc (right): https://codereview.webrtc.org/1267683002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_manager.cc:63: RtcEventLog* const event_log) { On 2015/08/12 15:29:53, Henrik Grunell (webrtc) wrote: > On 2015/08/12 13:16:41, ivoc wrote: > > On 2015/08/12 12:43:45, Henrik Grunell (webrtc) wrote: > > > Will we ever use another event log than the one given in the ctor? Could we > > let > > > ChannelManager own it and remove it from SharedData? > > > > That is certainly an option, do you think it would be better to let > > ChannelManager own it than SharedData? > > I think the raw pointer should be avoided if not necessary, which it isn't then. > If we let SharedData or ChannelManager own doesn't matter that much. I'm not sure what you mean exactly by avoiding the raw pointer, do you mean passing a scoped_ptr or scoped_ref_ptr instead in the constructors?
https://codereview.webrtc.org/1267683002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel_manager.cc (right): https://codereview.webrtc.org/1267683002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_manager.cc:63: RtcEventLog* const event_log) { On 2015/08/13 07:49:23, ivoc wrote: > On 2015/08/12 15:29:53, Henrik Grunell (webrtc) wrote: > > On 2015/08/12 13:16:41, ivoc wrote: > > > On 2015/08/12 12:43:45, Henrik Grunell (webrtc) wrote: > > > > Will we ever use another event log than the one given in the ctor? Could > we > > > let > > > > ChannelManager own it and remove it from SharedData? > > > > > > That is certainly an option, do you think it would be better to let > > > ChannelManager own it than SharedData? > > > > I think the raw pointer should be avoided if not necessary, which it isn't > then. > > If we let SharedData or ChannelManager own doesn't matter that much. > > I'm not sure what you mean exactly by avoiding the raw pointer, do you mean > passing a scoped_ptr or scoped_ref_ptr instead in the constructors? I mean having a scoped_ptr only at one place only.
https://codereview.webrtc.org/1267683002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel_manager.cc (right): https://codereview.webrtc.org/1267683002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_manager.cc:63: RtcEventLog* const event_log) { On 2015/08/13 08:13:55, Henrik Grunell (webrtc) wrote: > On 2015/08/13 07:49:23, ivoc wrote: > > On 2015/08/12 15:29:53, Henrik Grunell (webrtc) wrote: > > > On 2015/08/12 13:16:41, ivoc wrote: > > > > On 2015/08/12 12:43:45, Henrik Grunell (webrtc) wrote: > > > > > Will we ever use another event log than the one given in the ctor? Could > > we > > > > let > > > > > ChannelManager own it and remove it from SharedData? > > > > > > > > That is certainly an option, do you think it would be better to let > > > > ChannelManager own it than SharedData? > > > > > > I think the raw pointer should be avoided if not necessary, which it isn't > > then. > > > If we let SharedData or ChannelManager own doesn't matter that much. > > > > I'm not sure what you mean exactly by avoiding the raw pointer, do you mean > > passing a scoped_ptr or scoped_ref_ptr instead in the constructors? > > I mean having a scoped_ptr only at one place only. Ah okay, that is a good point. I moved the RtcEventLog from SharedData to ChannelManager to solve this.
Ivo, do you have any more changes pending to this CL? https://codereview.webrtc.org/1267683002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/BUILD.gn (right): https://codereview.webrtc.org/1267683002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/BUILD.gn:66: Do you remember what construct caused the clang error? If it is not too complex, we might be able to fix the source instead of disabling clang warnings for the entire ACM.
Final two comments. https://codereview.webrtc.org/1267683002/diff/160001/webrtc/voice_engine/incl... File webrtc/voice_engine/include/voe_codec.h (right): https://codereview.webrtc.org/1267683002/diff/160001/webrtc/voice_engine/incl... webrtc/voice_engine/include/voe_codec.h:136: // Engine. Also document the lifetime here. Until VoE is destroyed? https://codereview.webrtc.org/1267683002/diff/160001/webrtc/voice_engine/incl... webrtc/voice_engine/include/voe_codec.h:137: virtual RtcEventLog* GetEventLog() = 0; Just to verify: is the event log thread safe?
No changes are pending on my side for this CL, I'm merely trying to appease the reviewers :) https://codereview.webrtc.org/1267683002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/BUILD.gn (right): https://codereview.webrtc.org/1267683002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/BUILD.gn:66: On 2015/08/14 13:00:44, terelius wrote: > Do you remember what construct caused the clang error? If it is not too complex, > we might be able to fix the source instead of disabling clang warnings for the > entire ACM. Well, in the ACM we include rtc_event_log.h, which includes video_send_stream.h and video_receive_stream.h, which actually cause clang errors (I copied this suppression code from one of the video build files). I filed a bug for this, which I assigned to pbos@, see: https://code.google.com/p/webrtc/issues/detail?id=4895 . Once that bug is resolved, this suppression should be removed. https://codereview.webrtc.org/1267683002/diff/160001/webrtc/voice_engine/incl... File webrtc/voice_engine/include/voe_codec.h (right): https://codereview.webrtc.org/1267683002/diff/160001/webrtc/voice_engine/incl... webrtc/voice_engine/include/voe_codec.h:136: // Engine. On 2015/08/14 13:11:20, Henrik Grunell (webrtc) wrote: > Also document the lifetime here. Until VoE is destroyed? Done. https://codereview.webrtc.org/1267683002/diff/160001/webrtc/voice_engine/incl... webrtc/voice_engine/include/voe_codec.h:137: virtual RtcEventLog* GetEventLog() = 0; On 2015/08/14 13:11:20, Henrik Grunell (webrtc) wrote: > Just to verify: is the event log thread safe? Yes, it is indeed thread safe.
I think you should add a test in voice_engine/test/auto_test/standard/codec_test.cc. At least check that we get a valid pointer and that anything at all is written. https://codereview.webrtc.org/1267683002/diff/160001/webrtc/voice_engine/incl... File webrtc/voice_engine/include/voe_codec.h (right): https://codereview.webrtc.org/1267683002/diff/160001/webrtc/voice_engine/incl... webrtc/voice_engine/include/voe_codec.h:137: virtual RtcEventLog* GetEventLog() = 0; On 2015/08/14 13:23:13, ivoc wrote: > On 2015/08/14 13:11:20, Henrik Grunell (webrtc) wrote: > > Just to verify: is the event log thread safe? > > Yes, it is indeed thread safe. Acknowledged.
https://codereview.webrtc.org/1267683002/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/main/interface/audio_coding_module.h (right): https://codereview.webrtc.org/1267683002/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/main/interface/audio_coding_module.h:94: RtcEventLog* event_log; What's the difference between AudioCodingModule and AudioCodingModuleImpl? Any chance we can use RtcEventLog* const here too?
https://codereview.webrtc.org/1267683002/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/main/interface/audio_coding_module.h (right): https://codereview.webrtc.org/1267683002/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/main/interface/audio_coding_module.h:94: RtcEventLog* event_log; On 2015/08/14 19:12:47, terelius wrote: > What's the difference between AudioCodingModule and AudioCodingModuleImpl? Any > chance we can use RtcEventLog* const here too? The former is the interface, the latter is the implementation. https://codereview.webrtc.org/1267683002/diff/180001/webrtc/voice_engine/voe_... File webrtc/voice_engine/voe_base_impl.cc (left): https://codereview.webrtc.org/1267683002/diff/180001/webrtc/voice_engine/voe_... webrtc/voice_engine/voe_base_impl.cc:423: This is an unnecessary diff.
I added a simple unittest to codec_test.cc, it tests if the GetEventLog() function returns a valid pointer, and if it is possible to write a log file using that pointer. https://codereview.webrtc.org/1267683002/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/main/interface/audio_coding_module.h (right): https://codereview.webrtc.org/1267683002/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/main/interface/audio_coding_module.h:94: RtcEventLog* event_log; On 2015/08/17 11:04:02, hlundin-webrtc wrote: > On 2015/08/14 19:12:47, terelius wrote: > > What's the difference between AudioCodingModule and AudioCodingModuleImpl? Any > > chance we can use RtcEventLog* const here too? > > The former is the interface, the latter is the implementation. The reason why the pointer is not const here in this Config struct, is that it would make it impossible to set, except with a special constructor (which doesn't exist right now). Since this struct is only used during initialization I thought it is not a big deal to make it non-const.
lgtm https://codereview.webrtc.org/1267683002/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/main/interface/audio_coding_module.h (right): https://codereview.webrtc.org/1267683002/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/main/interface/audio_coding_module.h:94: RtcEventLog* event_log; On 2015/08/21 11:45:51, ivoc wrote: > On 2015/08/17 11:04:02, hlundin-webrtc wrote: > > On 2015/08/14 19:12:47, terelius wrote: > > > What's the difference between AudioCodingModule and AudioCodingModuleImpl? > Any > > > chance we can use RtcEventLog* const here too? > > > > The former is the interface, the latter is the implementation. > > The reason why the pointer is not const here in this Config struct, is that it > would make it impossible to set, except with a special constructor (which > doesn't exist right now). Since this struct is only used during initialization I > thought it is not a big deal to make it non-const. Ok.
Still lgtm.
https://codereview.webrtc.org/1267683002/diff/200001/webrtc/voice_engine/test... File webrtc/voice_engine/test/auto_test/standard/codec_test.cc (right): https://codereview.webrtc.org/1267683002/diff/200001/webrtc/voice_engine/test... webrtc/voice_engine/test/auto_test/standard/codec_test.cc:204: event_log->StopLogging(); Is there maybe any VoE function call that should render a log entry? I'd like to also make VoE log something and verify the contents of the log file. Just calling a function would be simplest if possible. https://codereview.webrtc.org/1267683002/diff/200001/webrtc/voice_engine/test... webrtc/voice_engine/test/auto_test/standard/codec_test.cc:208: EXPECT_TRUE(event_file); I think you can just assert here, if it couldn't be opened, we won't do anything after anyway.
https://codereview.webrtc.org/1267683002/diff/200001/webrtc/voice_engine/test... File webrtc/voice_engine/test/auto_test/standard/codec_test.cc (right): https://codereview.webrtc.org/1267683002/diff/200001/webrtc/voice_engine/test... webrtc/voice_engine/test/auto_test/standard/codec_test.cc:204: event_log->StopLogging(); On 2015/08/24 09:03:03, Henrik Grunell (webrtc) wrote: > Is there maybe any VoE function call that should render a log entry? I'd like to > also make VoE log something and verify the contents of the log file. Just > calling a function would be simplest if possible. There are currently no functions on the VoE API that result in anything being logged. The only function that results in a log entry on the audio side is one of the functions of the AudioCodingModule, but it is not exposed in the VoE interface. The logfile will currently have at least a LogStart event, so it is possible to parse it and validate that at least the LogStart event is written successfully. Let me know if you think this should be added. The reason I chose not to do this is that it would mean more includes and dependencies (for parsing), and it overlaps with the unit tests in rtc_event_log_unittest.cc, where writing logfiles and parsing the resulting files is extensively validated, so I'm not sure how valuable it would be to replicate similar tests here. https://codereview.webrtc.org/1267683002/diff/200001/webrtc/voice_engine/test... webrtc/voice_engine/test/auto_test/standard/codec_test.cc:208: EXPECT_TRUE(event_file); On 2015/08/24 09:03:02, Henrik Grunell (webrtc) wrote: > I think you can just assert here, if it couldn't be opened, we won't do anything > after anyway. Done.
LGTM. Thanks for adding the test! https://codereview.webrtc.org/1267683002/diff/200001/webrtc/voice_engine/test... File webrtc/voice_engine/test/auto_test/standard/codec_test.cc (right): https://codereview.webrtc.org/1267683002/diff/200001/webrtc/voice_engine/test... webrtc/voice_engine/test/auto_test/standard/codec_test.cc:204: event_log->StopLogging(); On 2015/08/24 12:17:29, ivoc wrote: > On 2015/08/24 09:03:03, Henrik Grunell (webrtc) wrote: > > Is there maybe any VoE function call that should render a log entry? I'd like > to > > also make VoE log something and verify the contents of the log file. Just > > calling a function would be simplest if possible. > > There are currently no functions on the VoE API that result in anything being > logged. The only function that results in a log entry on the audio side is one > of the functions of the AudioCodingModule, but it is not exposed in the VoE > interface. > > The logfile will currently have at least a LogStart event, so it is possible to > parse it and validate that at least the LogStart event is written successfully. > Let me know if you think this should be added. > > The reason I chose not to do this is that it would mean more includes and > dependencies (for parsing), and it overlaps with the unit tests in > rtc_event_log_unittest.cc, where writing logfiles and parsing the resulting > files is extensively validated, so I'm not sure how valuable it would be to > replicate similar tests here. If there's other tests that covers those I'm fine with this. The only thing missing is testing that we correctly pass the object into ACM, which could have been tested that way. It's also something trivial to test, so I'm fine without that.
ivoc@webrtc.org changed reviewers: + pbos@webrtc.org
The CQ bit was checked by ivoc@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1267683002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1267683002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux/builds/9247) linux_gn on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_gn/builds/5227) linux_tsan2 on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/6397)
The CQ bit was checked by ivoc@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1267683002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1267683002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android on tryserver.webrtc (JOB_TIMED_OUT, no build URL) mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
https://codereview.webrtc.org/1267683002/diff/240001/webrtc/video/call.cc File webrtc/video/call.cc (right): https://codereview.webrtc.org/1267683002/diff/240001/webrtc/video/call.cc#new... webrtc/video/call.cc:529: event_log_->LogRtcpPacket(false, media_type, packet, length); Maybe move these two logs down to right before we return and only have one: if (rtcp_delivered && event_log_) event_log_->LogRtcpPacket(...); ? https://codereview.webrtc.org/1267683002/diff/240001/webrtc/video/call.cc#new... webrtc/video/call.cc:548: event_log_->LogRtpHeader(true, media_type, packet, 20, length); For RTCP we only log packets which were successfully given to the stream object. Here we seem to log no matter what. Shouldn't we do the same thing?
Thanks for the comments. See my responses below. https://codereview.webrtc.org/1267683002/diff/240001/webrtc/video/call.cc File webrtc/video/call.cc (right): https://codereview.webrtc.org/1267683002/diff/240001/webrtc/video/call.cc#new... webrtc/video/call.cc:529: event_log_->LogRtcpPacket(false, media_type, packet, length); On 2015/09/07 11:04:37, stefan-webrtc (holmer) wrote: > Maybe move these two logs down to right before we return and only have one: > > if (rtcp_delivered && event_log_) > event_log_->LogRtcpPacket(...); > > ? > That's possible, but then we lose information about if the packet it on the send or receive side, at least that's how I understand the difference between the first and the second loop. Notice that the first argument is true in the first call, and false in the second. https://codereview.webrtc.org/1267683002/diff/240001/webrtc/video/call.cc#new... webrtc/video/call.cc:548: event_log_->LogRtpHeader(true, media_type, packet, 20, length); On 2015/09/07 11:04:37, stefan-webrtc (holmer) wrote: > For RTCP we only log packets which were successfully given to the stream object. > Here we seem to log no matter what. Shouldn't we do the same thing? That sounds like a good idea. I changed the code along those lines.
lgtm
https://codereview.webrtc.org/1267683002/diff/240001/webrtc/video/call.cc File webrtc/video/call.cc (right): https://codereview.webrtc.org/1267683002/diff/240001/webrtc/video/call.cc#new... webrtc/video/call.cc:529: event_log_->LogRtcpPacket(false, media_type, packet, length); On 2015/09/07 11:51:14, ivoc wrote: > On 2015/09/07 11:04:37, stefan-webrtc (holmer) wrote: > > Maybe move these two logs down to right before we return and only have one: > > > > if (rtcp_delivered && event_log_) > > event_log_->LogRtcpPacket(...); > > > > ? > > > > That's possible, but then we lose information about if the packet it on the send > or receive side, at least that's how I understand the difference between the > first and the second loop. Notice that the first argument is true in the first > call, and false in the second. I rely on knowing whether the packet in incoming or outgoing, to separate the bandwidth estimates. I think the entire function should be rewritten and based on the TODO, pbos thinks so too. If that is the case, do we want to split the function into DeliverIncomingRtcp and DeliverOutgoingRtcp and assume that the caller knows whether he is sending or receiving the packet? https://codereview.webrtc.org/1267683002/diff/240001/webrtc/video/call.cc#new... webrtc/video/call.cc:548: event_log_->LogRtpHeader(true, media_type, packet, 20, length); On 2015/09/07 11:04:37, stefan-webrtc (holmer) wrote: > For RTCP we only log packets which were successfully given to the stream object. > Here we seem to log no matter what. Shouldn't we do the same thing? Is it common to not find a stream object? If so, then we should probably log the packet anyway since it takes up bandwidth. Perhaps add a protobuf field indicating that a certain packet was undeliverable?
https://codereview.webrtc.org/1267683002/diff/240001/webrtc/video/call.cc File webrtc/video/call.cc (right): https://codereview.webrtc.org/1267683002/diff/240001/webrtc/video/call.cc#new... webrtc/video/call.cc:548: event_log_->LogRtpHeader(true, media_type, packet, 20, length); On 2015/09/07 12:32:44, terelius wrote: > On 2015/09/07 11:04:37, stefan-webrtc (holmer) wrote: > > For RTCP we only log packets which were successfully given to the stream > object. > > Here we seem to log no matter what. Shouldn't we do the same thing? > > Is it common to not find a stream object? If so, then we should probably log the > packet anyway since it takes up bandwidth. Perhaps add a protobuf field > indicating that a certain packet was undeliverable? It shouldn't be common at all I think. I think we can leave that for future improvements in that case. :)
The CQ bit was checked by ivoc@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1267683002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1267683002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_drmemory_light on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by ivoc@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from terelius@webrtc.org, henrik.lundin@webrtc.org, henrikg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1267683002/#ps260001 (title: "Addressed Stefan's review comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1267683002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1267683002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/656)
lgtm for talk/
The CQ bit was checked by ivoc@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1267683002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1267683002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by ivoc@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1267683002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1267683002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_drmemory_light on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by ivoc@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from terelius@webrtc.org, henrik.lundin@webrtc.org, pbos@webrtc.org, stefan@webrtc.org, henrikg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1267683002/#ps300001 (title: "Another rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1267683002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1267683002/300001
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/b04965ccf83c2bc6e2758abab9bea0c18551a54c Cr-Commit-Position: refs/heads/master@{#9901} |