Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1209563002:
Added support for keeping a buffer of the previous X seconds, to add to (Closed)
|
Created:
5 years, 6 months ago by ivoc Modified:
5 years, 6 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, hlundin-webrtc, kwiberg-webrtc, tlegrand-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdded support for keeping a buffer of the previous X seconds, to add to an AcmDump.
In addition, timestamps are now absolute instead of relative to LOG_START.
BUG=webrtc:4741
R=henrik.lundin@webrtc.org, kwiberg@webrtc.org, stefan@webrtc.org, terelius@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/241338eeb74d4c9ce963dc064ce961b63ec651aa
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed Karl's review comments. #
Total comments: 6
Patch Set 3 : Addressed Henrik's review comments. #
Total comments: 10
Patch Set 4 : Addressed Stefan's comments. #Patch Set 5 : Addressed Bjorn's review comments. #
Messages
Total messages: 16 (2 generated)
ivoc@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, stefan@webrtc.org, terelius@webrtc.org
Please have a look at this CL to keep a buffer of the last X seconds in AcmDump.
kwiberg@webrtc.org changed reviewers: + kwiberg@webrtc.org
Did you measure how fast the no-log case is now, compared to how fast is was before and the yes-log case? Also, what's the ballpark memory consumption of the 10-second deque? Large enough to matter? https://codereview.webrtc.org/1209563002/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/acm_dump.cc (right): https://codereview.webrtc.org/1209563002/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:72: void StoreToFile(ACMDumpEvent& event) EXCLUSIVE_LOCKS_REQUIRED(crit_); The convention is to use pointers for arguments that are written to. https://codereview.webrtc.org/1209563002/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:75: void RecentEvent(const ACMDumpEvent& event) EXCLUSIVE_LOCKS_REQUIRED(crit_); Maybe "AddRecentEvent"? https://codereview.webrtc.org/1209563002/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:215: file_->Flush(); This change seems unrelated to the rest of the CL. Is it?
To address your comments: we have not yet hooked the class up to do any actual logging, so it's hard to measure a performance difference. I agree that it's certainly slower, but we will have to keep an eye on any performance issues once we begin to actually log data. For audio-only logging I calculated that it would be approximately 20 kB for 10 seconds of header-only logging. I'm not sure how much extra data the BWE logging will add to that, but we can see how it works out in practice when we hook it up, and adjust the buffer time length accordingly if needed. https://codereview.webrtc.org/1209563002/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/acm_dump.cc (right): https://codereview.webrtc.org/1209563002/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:72: void StoreToFile(ACMDumpEvent& event) EXCLUSIVE_LOCKS_REQUIRED(crit_); On 2015/06/24 15:38:16, kwiberg1 wrote: > The convention is to use pointers for arguments that are written to. Okay, changed to a pointer. https://codereview.webrtc.org/1209563002/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:75: void RecentEvent(const ACMDumpEvent& event) EXCLUSIVE_LOCKS_REQUIRED(crit_); On 2015/06/24 15:38:16, kwiberg1 wrote: > Maybe "AddRecentEvent"? Okay, seems like a better name. https://codereview.webrtc.org/1209563002/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:215: file_->Flush(); On 2015/06/24 15:38:16, kwiberg1 wrote: > This change seems unrelated to the rest of the CL. Is it? It was needed for the unittest to work, but I have now rewritten the unittest in such a way that there is no longer a need to flush after every logging event.
lgtm
lgtm with commenting nits. https://codereview.webrtc.org/1209563002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/main/acm2/acm_dump.cc (right): https://codereview.webrtc.org/1209563002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:78: // Amount of time in us to record log events, before starting the actual log. Write microseconds instead of us in comments. Although us is an accepted approximation of "<greek-mu>s", I think you can spare us some confusion by not writing us. See what I did there? :) https://codereview.webrtc.org/1209563002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:126: // Add start event to the recent event list, and remove any old events from The comment is slightly confusing. I first thought that the call to LogDebugEventLocked did all of the above, but the clearing of the list happens further down, right? https://codereview.webrtc.org/1209563002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc (right): https://codereview.webrtc.org/1209563002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc:94: { Any reason to add the extra scope here? If so, please, comment on that.
Addressed Henrik's comments. https://codereview.webrtc.org/1209563002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/main/acm2/acm_dump.cc (right): https://codereview.webrtc.org/1209563002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:78: // Amount of time in us to record log events, before starting the actual log. On 2015/06/25 13:20:56, hlundin-webrtc wrote: > Write microseconds instead of us in comments. Although us is an accepted > approximation of "<greek-mu>s", I think you can spare us some confusion by not > writing us. See what I did there? :) Good point. https://codereview.webrtc.org/1209563002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:126: // Add start event to the recent event list, and remove any old events from On 2015/06/25 13:20:56, hlundin-webrtc wrote: > The comment is slightly confusing. I first thought that the call to > LogDebugEventLocked did all of the above, but the clearing of the list happens > further down, right? What I meant to say is that this call to LogDebugEventLocked will cause the LOG_START event to be added to recent_log_events_ list, and also at the same time will cause the oldest events in recent_log_events_ to be removed from that list if necessary (this all happens in LogDebugEventLocked, because it calls AddRecentEvent). This is handy because it removes unneeded events right before writing the contents of the list to the log. I will rephrase to clarify. https://codereview.webrtc.org/1209563002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc (right): https://codereview.webrtc.org/1209563002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc:94: { On 2015/06/25 13:20:56, hlundin-webrtc wrote: > Any reason to add the extra scope here? If so, please, comment on that. I added the scope so that when the AcmDump goes out of scope, the logfile is flushed right before the file is read from disk below. This avoids the need to manually flush after each write to the logfile, which Karl commented on earlier. I will add a comment to clarify.
https://codereview.webrtc.org/1209563002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/main/acm2/acm_dump.cc (right): https://codereview.webrtc.org/1209563002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:89: const webrtc::Clock* clock_ GUARDED_BY(crit_); No need to guard this since it's const, right? https://codereview.webrtc.org/1209563002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:145: rtp_event.clear_debug_event(); Isn't the event clear when it is created? https://codereview.webrtc.org/1209563002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:148: rtp_event.set_type(webrtc::ACMDumpEvent::RTP_EVENT); Would it make sense to have these in the constructor of the event instead?
https://codereview.webrtc.org/1209563002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/main/acm2/acm_dump.cc (right): https://codereview.webrtc.org/1209563002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:89: const webrtc::Clock* clock_ GUARDED_BY(crit_); On 2015/06/25 14:55:44, stefan-webrtc (holmer) wrote: > No need to guard this since it's const, right? Sounds reasonable. But then the actual pointer probably ought to be const too, if possible.
Addressed Stefan's comments. https://codereview.webrtc.org/1209563002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/main/acm2/acm_dump.cc (right): https://codereview.webrtc.org/1209563002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:89: const webrtc::Clock* clock_ GUARDED_BY(crit_); On 2015/06/25 14:55:44, stefan-webrtc (holmer) wrote: > No need to guard this since it's const, right? Okay, that makes sense, I made the pointer const as well like Karl suggested. https://codereview.webrtc.org/1209563002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:145: rtp_event.clear_debug_event(); On 2015/06/25 14:55:44, stefan-webrtc (holmer) wrote: > Isn't the event clear when it is created? You're right, this is an artifact from when we reused the same event object each time. Will remove. https://codereview.webrtc.org/1209563002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:148: rtp_event.set_type(webrtc::ACMDumpEvent::RTP_EVENT); On 2015/06/25 14:55:44, stefan-webrtc (holmer) wrote: > Would it make sense to have these in the constructor of the event instead? That would make sense, but this class is automatically generated from the protobuf file, and it doesn't have a convenient constructor like that.
lgtm https://codereview.webrtc.org/1209563002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/main/acm2/acm_dump.cc (right): https://codereview.webrtc.org/1209563002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:148: rtp_event.set_type(webrtc::ACMDumpEvent::RTP_EVENT); On 2015/06/25 15:14:48, ivoc wrote: > On 2015/06/25 14:55:44, stefan-webrtc (holmer) wrote: > > Would it make sense to have these in the constructor of the event instead? > > That would make sense, but this class is automatically generated from the > protobuf file, and it doesn't have a convenient constructor like that. Ah, that makes sense.
Not specific to this CL, but still important to discuss. The decision of where
we should write the event (file vs 10s buffer) currently looks like this:
if (CurrentlyLogging()) {
StoreToFile(&event);
} else {
StopIfNecessary();
AddRecentEvent(event);
}
I think it would be easier to read the code if we used something like this:
if (active_) {
if (clock_->time() < start_time + duration) {
StoreToFile(&event);
} else {
StopLogging();
AddRecentEvent(event);
}
} else {
AddRecentEvent(event);
}
We might also want to rename active_ to currently_logging_
Pros:
More explicit what the code does, and function names become less ambiguous if
the functions only do one thing each.
Slightly fewer function calls and conditional jumps in the typical case that
active_ is false.
Cons:
A few extra lines of code that gets repeated in several places.
Possibly less elegant because we need a lock for this piece of code rather than
for the different functions.
Thoughts?
https://codereview.webrtc.org/1209563002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/main/acm2/acm_dump.cc (right):
https://codereview.webrtc.org/1209563002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/acm_dump.cc:210:
stream_->mutable_stream(0)->Swap(event);
The stream must not contain more than one event, or we'll write a bunch of old
events to the ACMDump. Maybe add an assertion that this is the case?
Good idea. I refactored the code a bit, and put your suggested code into a seperate function (to avoid repeating it), and removed two obsolete functions. https://codereview.webrtc.org/1209563002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/main/acm2/acm_dump.cc (right): https://codereview.webrtc.org/1209563002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:210: stream_->mutable_stream(0)->Swap(event); On 2015/06/25 16:21:03, terelius wrote: > The stream must not contain more than one event, or we'll write a bunch of old > events to the ACMDump. Maybe add an assertion that this is the case? Done.
lgtm
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as 241338eeb74d4c9ce963dc064ce961b63ec651aa (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
