Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1303713002:
Keep config events in RtcEventLog even if they are old. (Closed)
|
Created:
5 years, 4 months ago by terelius Modified:
5 years, 2 months ago CC:
webrtc-reviews_webrtc.org, yujie_mao (webrtc), stefan-webrtc, tterriberry_mozilla.com, andresp, perkj_webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionOld config events are no longer removed from RtcEventLog.
Time to keep old events in buffer is now changeable at runtime.
Added unit test for removing old events from buffer.
Committed: https://crrev.com/1adce14c87f8316b5c3f4ee16b8f573c8bf14761
Cr-Commit-Position: refs/heads/master@{#10302}
Patch Set 1 #Patch Set 2 : Lock before changing buffer duration #
Total comments: 22
Patch Set 3 : Reviewer comments from ivoc@ #
Total comments: 16
Patch Set 4 : Reviewer comments #Patch Set 5 : Rebase and resolve merge conflicts #Patch Set 6 : Rebase #Patch Set 7 : Rebase again #
Total comments: 1
Messages
Total messages: 30 (9 generated)
terelius@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, ivoc@webrtc.org, stefan@webrtc.org
Please have a look at this CL which fixes a problem where we lose configuration information due to only keeping events from the last 10 seconds.
Looks good! I have a few comments below. https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... File webrtc/video/rtc_event_log.cc (right): https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log.cc:173: recent_log_duration_us_ = recent_log_duration_us; Shouldn't there be some sort of purging of the buffer in case the new value is lower than the old value? (i.e. discard events that are too old due to the buffer change) https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log.cc:260: // TODO(terelius): We should use a separate event queue for config events. I guess this TODO can be removed in this CL. https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log.cc:300: // TODO(terelius): We should use a separate event queue for config events. This one as well. https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log.cc:405: config_events_.push_back(recent_log_events_.front()); I wonder if we should be checking if any of the config events that are already stored in config_events_ might become obsolete due to a newer config event. Is that something that could happen? https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_log.h File webrtc/video/rtc_event_log.h (right): https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log.h:43: virtual void SetBufferDuration(int64_t recent_log_duration_us) = 0; How useful is it to be able to change the size of the buffer at runtime? In which cases would this function be called? https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... File webrtc/video/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:482: rtc::Thread::SleepMs(50); I wonder if the sleep should be a little bit longer than the buffer duration, what if the 50 ms sleep turns out to be a bit less than 50000 us on the more accurate clock?
https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... File webrtc/video/rtc_event_log.cc (right): https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log.cc:173: recent_log_duration_us_ = recent_log_duration_us; On 2015/08/25 08:03:21, ivoc wrote: > Shouldn't there be some sort of purging of the buffer in case the new value is > lower than the old value? (i.e. discard events that are too old due to the > buffer change) The old events are purged whenever we add a new event to the queue or when we call StartLogging. https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log.cc:260: // TODO(terelius): We should use a separate event queue for config events. On 2015/08/25 08:03:21, ivoc wrote: > I guess this TODO can be removed in this CL. Yeah, that was kind of the point of the CL. :) https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log.cc:300: // TODO(terelius): We should use a separate event queue for config events. On 2015/08/25 08:03:21, ivoc wrote: > This one as well. Done. https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log.cc:405: config_events_.push_back(recent_log_events_.front()); On 2015/08/25 08:03:21, ivoc wrote: > I wonder if we should be checking if any of the config events that are already > stored in config_events_ might become obsolete due to a newer config event. Is > that something that could happen? This is a good point. I don't think it is possible to reconfigure (video) streams during runtime. However, it is possible to delete the stream and create a new one with the same SSRC which would result in two config events for the "same" stream. It is not clear whether we should resolve those ambiguities while writing the log or when we read it. Doing the check here would not completely solve the issue since we could potentially have multiple config events in the recent_log_events queue. https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_log.h File webrtc/video/rtc_event_log.h (right): https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log.h:43: virtual void SetBufferDuration(int64_t recent_log_duration_us) = 0; On 2015/08/25 08:03:21, ivoc wrote: > How useful is it to be able to change the size of the buffer at runtime? In > which cases would this function be called? At the moment it is only used for testing purposes. I want to construct a unit test where old events are removed from the queue, and waiting 10 seconds would be impractical. In the future, there might be real use cases e.g. allocating more memory for logging on high end devices. I considered passing the logging duration to the constructor, but as we discussed before we might have to change to a separate ring buffer for the RTP and RTCP packets. (Using a separate buffer would avoid constructing protobuf messages when not logging, would avoid dynamic memory allocation when queueing an event, and would avoid lock contention between the audio and video threads.) In that case, logging "duration" will be set based on memory usage rather than time, and there is no good way to change the usage of constructor arguments. https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... File webrtc/video/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:482: rtc::Thread::SleepMs(50); On 2015/08/25 08:03:21, ivoc wrote: > I wonder if the sleep should be a little bit longer than the buffer duration, > what if the 50 ms sleep turns out to be a bit less than 50000 us on the more > accurate clock? On POSIX systems it uses nanosleep() which should sleep for at least that number of nanoseconds, but on windows it uses some other sleep function. I can change to 55 ms I suppose.
https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... File webrtc/video/rtc_event_log.cc (right): https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log.cc:173: recent_log_duration_us_ = recent_log_duration_us; On 2015/08/25 16:41:44, terelius wrote: > On 2015/08/25 08:03:21, ivoc wrote: > > Shouldn't there be some sort of purging of the buffer in case the new value is > > lower than the old value? (i.e. discard events that are too old due to the > > buffer change) > > The old events are purged whenever we add a new event to the queue or when we > call StartLogging. Acknowledged. https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log.cc:405: config_events_.push_back(recent_log_events_.front()); On 2015/08/25 16:41:44, terelius wrote: > On 2015/08/25 08:03:21, ivoc wrote: > > I wonder if we should be checking if any of the config events that are already > > stored in config_events_ might become obsolete due to a newer config event. Is > > that something that could happen? > > This is a good point. > > I don't think it is possible to reconfigure (video) streams during runtime. > However, it is possible to delete the stream and create a new one with the same > SSRC which would result in two config events for the "same" stream. > > It is not clear whether we should resolve those ambiguities while writing the > log or when we read it. Doing the check here would not completely solve the > issue since we could potentially have multiple config events in the > recent_log_events queue. I think a check here would solve this problem. Even if the recent_events_ queue contains another config event that will succeed one of the ones from the config_events_ list, the one that is stored in config_events_ is still valid for any older packets that are in the recent_events_ queue, and is therefore still needed. Once the succeeding config event is too old for the recent_events_ queue, it can replace the old one in config_events_ since it is no longer useful. I'm not sure if it's better to solve this while writing or reading. I guess it also depends on how likely it is that this happens. If it happens frequently, it is not nice if the RtcEventLog starts collecting a whole list of config events that are no longer valid. If it is rare, then it's probably acceptable to leave it in. I don't know how often this happens in practice, maybe one of the other reviewers can comment? If we decide to solve it while reading, I think it would be good to document this behavior somewhere (maybe in the protobuf, or with the parsing functions?). https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_log.h File webrtc/video/rtc_event_log.h (right): https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log.h:43: virtual void SetBufferDuration(int64_t recent_log_duration_us) = 0; On 2015/08/25 16:41:44, terelius wrote: > On 2015/08/25 08:03:21, ivoc wrote: > > How useful is it to be able to change the size of the buffer at runtime? In > > which cases would this function be called? > > At the moment it is only used for testing purposes. I want to construct a unit > test where old events are removed from the queue, and waiting 10 seconds would > be impractical. In the future, there might be real use cases e.g. allocating > more memory for logging on high end devices. > > I considered passing the logging duration to the constructor, but as we > discussed before we might have to change to a separate ring buffer for the RTP > and RTCP packets. (Using a separate buffer would avoid constructing protobuf > messages when not logging, would avoid dynamic memory allocation when queueing > an event, and would avoid lock contention between the audio and video threads.) > In that case, logging "duration" will be set based on memory usage rather than > time, and there is no good way to change the usage of constructor arguments. Hmm, I don't really see why there no good way to change constructor arguments, compared to changing function arguments. To me it seems to be pretty similar.
https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_log.h File webrtc/video/rtc_event_log.h (right): https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log.h:43: virtual void SetBufferDuration(int64_t recent_log_duration_us) = 0; On 2015/08/26 08:15:28, ivoc wrote: > On 2015/08/25 16:41:44, terelius wrote: > > On 2015/08/25 08:03:21, ivoc wrote: > > > How useful is it to be able to change the size of the buffer at runtime? In > > > which cases would this function be called? > > > > At the moment it is only used for testing purposes. I want to construct a unit > > test where old events are removed from the queue, and waiting 10 seconds would > > be impractical. In the future, there might be real use cases e.g. allocating > > more memory for logging on high end devices. > > > > I considered passing the logging duration to the constructor, but as we > > discussed before we might have to change to a separate ring buffer for the RTP > > and RTCP packets. (Using a separate buffer would avoid constructing protobuf > > messages when not logging, would avoid dynamic memory allocation when queueing > > an event, and would avoid lock contention between the audio and video > threads.) > > In that case, logging "duration" will be set based on memory usage rather than > > time, and there is no good way to change the usage of constructor arguments. > > Hmm, I don't really see why there no good way to change constructor arguments, > compared to changing function arguments. To me it seems to be pretty similar. Using a function you can choose a new name, e.g. SetBufferMemorySize. You can then either remove SetBufferDuration to cause compilation errors in code that don't use the new API, or you can change SetBufferDuration to estimate the required memory like this void SetBufferDuration(seconds) { memory_size = seconds * packets_per_second * avg_packet_size SetBufferMemorySize(memory_size) }
LG. Some smaller comments. https://codereview.webrtc.org/1303713002/diff/40001/webrtc/video/rtc_event_lo... File webrtc/video/rtc_event_log.cc (right): https://codereview.webrtc.org/1303713002/diff/40001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log.cc:171: void RtcEventLogImpl::SetBufferDuration(int64_t recent_log_duration_us) { I think the function name should match the member it modifies. https://codereview.webrtc.org/1303713002/diff/40001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log.cc:195: if (event.type() == rtclog::Event::VIDEO_RECEIVER_CONFIG_EVENT || Make a helper function for checking if an event is a config event. https://codereview.webrtc.org/1303713002/diff/40001/webrtc/video/rtc_event_log.h File webrtc/video/rtc_event_log.h (right): https://codereview.webrtc.org/1303713002/diff/40001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log.h:42: // before the user calls StartLogging. The default is 10'000'000 us = 10 s While C++14 allows apostrophe as "thousands separator", we tend to write large numbers in comments using either , as separator, or no separator at all. https://codereview.webrtc.org/1303713002/diff/40001/webrtc/video/rtc_event_lo... File webrtc/video/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1303713002/diff/40001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:475: log_dumper->SetBufferDuration(50000); Comment on the unit here. 50000 ms? us? https://codereview.webrtc.org/1303713002/diff/40001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:482: rtc::Thread::SleepMs(55); ... I guess 50000 us based on this... https://codereview.webrtc.org/1303713002/diff/40001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:495: // Verify the result. Please, comment on what you are verifying. In particular regarding the RT(C)P packets.
https://codereview.webrtc.org/1303713002/diff/40001/webrtc/video/rtc_event_lo... File webrtc/video/rtc_event_log.cc (right): https://codereview.webrtc.org/1303713002/diff/40001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log.cc:195: if (event.type() == rtclog::Event::VIDEO_RECEIVER_CONFIG_EVENT || On 2015/08/28 11:16:29, hlundin-webrtc wrote: > Make a helper function for checking if an event is a config event. +1 https://codereview.webrtc.org/1303713002/diff/40001/webrtc/video/rtc_event_lo... File webrtc/video/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1303713002/diff/40001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:428: void DropOldEvents(unsigned random_seed) { either int or uint32_t?
https://codereview.webrtc.org/1303713002/diff/40001/webrtc/video/rtc_event_lo... File webrtc/video/rtc_event_log.cc (right): https://codereview.webrtc.org/1303713002/diff/40001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log.cc:171: void RtcEventLogImpl::SetBufferDuration(int64_t recent_log_duration_us) { On 2015/08/28 11:16:29, hlundin-webrtc wrote: > I think the function name should match the member it modifies. Changed the variable name to buffer_duration_us_. https://codereview.webrtc.org/1303713002/diff/40001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log.cc:195: if (event.type() == rtclog::Event::VIDEO_RECEIVER_CONFIG_EVENT || On 2015/09/03 11:13:21, stefan-webrtc (holmer) wrote: > On 2015/08/28 11:16:29, hlundin-webrtc wrote: > > Make a helper function for checking if an event is a config event. > > +1 Done. https://codereview.webrtc.org/1303713002/diff/40001/webrtc/video/rtc_event_log.h File webrtc/video/rtc_event_log.h (right): https://codereview.webrtc.org/1303713002/diff/40001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log.h:42: // before the user calls StartLogging. The default is 10'000'000 us = 10 s On 2015/08/28 11:16:29, hlundin-webrtc wrote: > While C++14 allows apostrophe as "thousands separator", we tend to write large > numbers in comments using either , as separator, or no separator at all. Though using comma as a separator does not follow any C++ standard. Would space be an acceptable substitute? https://codereview.webrtc.org/1303713002/diff/40001/webrtc/video/rtc_event_lo... File webrtc/video/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1303713002/diff/40001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:428: void DropOldEvents(unsigned random_seed) { On 2015/09/03 11:13:21, stefan-webrtc (holmer) wrote: > either int or uint32_t? Changed to unsigned int https://codereview.webrtc.org/1303713002/diff/40001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:475: log_dumper->SetBufferDuration(50000); On 2015/08/28 11:16:29, hlundin-webrtc wrote: > Comment on the unit here. 50000 ms? us? Done. https://codereview.webrtc.org/1303713002/diff/40001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:482: rtc::Thread::SleepMs(55); On 2015/08/28 11:16:29, hlundin-webrtc wrote: > ... I guess 50000 us based on this... Done. https://codereview.webrtc.org/1303713002/diff/40001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:495: // Verify the result. On 2015/08/28 11:16:29, hlundin-webrtc wrote: > Please, comment on what you are verifying. In particular regarding the RT(C)P > packets. Done.
lgtm https://codereview.webrtc.org/1303713002/diff/40001/webrtc/video/rtc_event_log.h File webrtc/video/rtc_event_log.h (right): https://codereview.webrtc.org/1303713002/diff/40001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log.h:42: // before the user calls StartLogging. The default is 10'000'000 us = 10 s On 2015/09/17 12:44:08, terelius wrote: > On 2015/08/28 11:16:29, hlundin-webrtc wrote: > > While C++14 allows apostrophe as "thousands separator", we tend to write large > > numbers in comments using either , as separator, or no separator at all. > > Though using comma as a separator does not follow any C++ standard. Would space > be an acceptable substitute? Acknowledged.
Sorry for the late reply, I kind of forgot about this one. https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... File webrtc/video/rtc_event_log.cc (right): https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log.cc:405: config_events_.push_back(recent_log_events_.front()); On 2015/08/26 08:15:28, ivoc wrote: > On 2015/08/25 16:41:44, terelius wrote: > > On 2015/08/25 08:03:21, ivoc wrote: > > > I wonder if we should be checking if any of the config events that are > already > > > stored in config_events_ might become obsolete due to a newer config event. > Is > > > that something that could happen? > > > > This is a good point. > > > > I don't think it is possible to reconfigure (video) streams during runtime. > > However, it is possible to delete the stream and create a new one with the > same > > SSRC which would result in two config events for the "same" stream. > > > > It is not clear whether we should resolve those ambiguities while writing the > > log or when we read it. Doing the check here would not completely solve the > > issue since we could potentially have multiple config events in the > > recent_log_events queue. > > I think a check here would solve this problem. Even if the recent_events_ queue > contains another config event that will succeed one of the ones from the > config_events_ list, the one that is stored in config_events_ is still valid for > any older packets that are in the recent_events_ queue, and is therefore still > needed. Once the succeeding config event is too old for the recent_events_ > queue, it can replace the old one in config_events_ since it is no longer > useful. > > I'm not sure if it's better to solve this while writing or reading. I guess it > also depends on how likely it is that this happens. If it happens frequently, it > is not nice if the RtcEventLog starts collecting a whole list of config events > that are no longer valid. If it is rare, then it's probably acceptable to leave > it in. I don't know how often this happens in practice, maybe one of the other > reviewers can comment? If we decide to solve it while reading, I think it would > be good to document this behavior somewhere (maybe in the protobuf, or with the > parsing functions?). Could you respond to this? I still think it would be better to purge old configs that are not needed to interpret any packets that appear in the log or in the recentEvents_queue, and I don't think it would be that complicated to add that feature (unless I'm missing something). https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_log.h File webrtc/video/rtc_event_log.h (right): https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log.h:43: virtual void SetBufferDuration(int64_t recent_log_duration_us) = 0; On 2015/08/26 08:43:37, terelius wrote: > On 2015/08/26 08:15:28, ivoc wrote: > > On 2015/08/25 16:41:44, terelius wrote: > > > On 2015/08/25 08:03:21, ivoc wrote: > > > > How useful is it to be able to change the size of the buffer at runtime? > In > > > > which cases would this function be called? > > > > > > At the moment it is only used for testing purposes. I want to construct a > unit > > > test where old events are removed from the queue, and waiting 10 seconds > would > > > be impractical. In the future, there might be real use cases e.g. allocating > > > more memory for logging on high end devices. > > > > > > I considered passing the logging duration to the constructor, but as we > > > discussed before we might have to change to a separate ring buffer for the > RTP > > > and RTCP packets. (Using a separate buffer would avoid constructing protobuf > > > messages when not logging, would avoid dynamic memory allocation when > queueing > > > an event, and would avoid lock contention between the audio and video > > threads.) > > > In that case, logging "duration" will be set based on memory usage rather > than > > > time, and there is no good way to change the usage of constructor arguments. > > > > Hmm, I don't really see why there no good way to change constructor arguments, > > compared to changing function arguments. To me it seems to be pretty similar. > > Using a function you can choose a new name, e.g. SetBufferMemorySize. > You can then either remove SetBufferDuration to cause compilation errors in code > that don't use the new API, or you can change SetBufferDuration to estimate the > required memory like this > > void SetBufferDuration(seconds) { > memory_size = seconds * packets_per_second * avg_packet_size > SetBufferMemorySize(memory_size) > } How about creating multiple variants of the Create function? For example a default one with a certain default buffer size, and a CreateWithBufferDuration to set a certain buffer size explicitly. If the meaning should be changed later, a new CreateWithBufferSize function can be added, and the old one can be removed. This would have similar advantages as having a function, while still having the buffer size/duration as a constant in the RtcEventLogImpl class (which I would personally prefer).
https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... File webrtc/video/rtc_event_log.cc (right): https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log.cc:405: config_events_.push_back(recent_log_events_.front()); On 2015/09/25 15:30:23, ivoc wrote: > On 2015/08/26 08:15:28, ivoc wrote: > > On 2015/08/25 16:41:44, terelius wrote: > > > On 2015/08/25 08:03:21, ivoc wrote: > > > > I wonder if we should be checking if any of the config events that are > > already > > > > stored in config_events_ might become obsolete due to a newer config > event. > > Is > > > > that something that could happen? > > > > > > This is a good point. > > > > > > I don't think it is possible to reconfigure (video) streams during runtime. > > > However, it is possible to delete the stream and create a new one with the > > same > > > SSRC which would result in two config events for the "same" stream. > > > > > > It is not clear whether we should resolve those ambiguities while writing > the > > > log or when we read it. Doing the check here would not completely solve the > > > issue since we could potentially have multiple config events in the > > > recent_log_events queue. > > > > I think a check here would solve this problem. Even if the recent_events_ > queue > > contains another config event that will succeed one of the ones from the > > config_events_ list, the one that is stored in config_events_ is still valid > for > > any older packets that are in the recent_events_ queue, and is therefore still > > needed. Once the succeeding config event is too old for the recent_events_ > > queue, it can replace the old one in config_events_ since it is no longer > > useful. > > > > I'm not sure if it's better to solve this while writing or reading. I guess it > > also depends on how likely it is that this happens. If it happens frequently, > it > > is not nice if the RtcEventLog starts collecting a whole list of config events > > that are no longer valid. If it is rare, then it's probably acceptable to > leave > > it in. I don't know how often this happens in practice, maybe one of the other > > reviewers can comment? If we decide to solve it while reading, I think it > would > > be good to document this behavior somewhere (maybe in the protobuf, or with > the > > parsing functions?). > > Could you respond to this? I still think it would be better to purge old configs > that are not needed to interpret any packets that appear in the log or in the > recentEvents_queue, and I don't think it would be that complicated to add that > feature (unless I'm missing something). So what you are proposing is checking whether there exists an older config in config_events before adding a new config for the same SSRC? The reason why I am hesitant is that we would add code for a rare event that wont be tested exhaustively, yet we would not solve the problem completely. (We could still have unused configurations for streams that are configured but never receive or send any packets. We could also have two configurations for the "same" stream if there are packets sent before the reconfiguration. Finally, if the reconfiguration is done while we are logging then everything would be written to file directly so there is no way to detect whether we have unnecessary configurations.) It is also slightly complicated to solve due to the fact that VideoSendConfig doesn't have a single SSRC, but a whole vector of them. Handling configurations with overlapping but nonidentical ssrc vectors will probably be ugly. https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_log.h File webrtc/video/rtc_event_log.h (right): https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log.h:43: virtual void SetBufferDuration(int64_t recent_log_duration_us) = 0; On 2015/09/25 15:30:23, ivoc wrote: > On 2015/08/26 08:43:37, terelius wrote: > > On 2015/08/26 08:15:28, ivoc wrote: > > > On 2015/08/25 16:41:44, terelius wrote: > > > > On 2015/08/25 08:03:21, ivoc wrote: > > > > > How useful is it to be able to change the size of the buffer at runtime? > > In > > > > > which cases would this function be called? > > > > > > > > At the moment it is only used for testing purposes. I want to construct a > > unit > > > > test where old events are removed from the queue, and waiting 10 seconds > > would > > > > be impractical. In the future, there might be real use cases e.g. > allocating > > > > more memory for logging on high end devices. > > > > > > > > I considered passing the logging duration to the constructor, but as we > > > > discussed before we might have to change to a separate ring buffer for the > > RTP > > > > and RTCP packets. (Using a separate buffer would avoid constructing > protobuf > > > > messages when not logging, would avoid dynamic memory allocation when > > queueing > > > > an event, and would avoid lock contention between the audio and video > > > threads.) > > > > In that case, logging "duration" will be set based on memory usage rather > > than > > > > time, and there is no good way to change the usage of constructor > arguments. > > > > > > Hmm, I don't really see why there no good way to change constructor > arguments, > > > compared to changing function arguments. To me it seems to be pretty > similar. > > > > Using a function you can choose a new name, e.g. SetBufferMemorySize. > > You can then either remove SetBufferDuration to cause compilation errors in > code > > that don't use the new API, or you can change SetBufferDuration to estimate > the > > required memory like this > > > > void SetBufferDuration(seconds) { > > memory_size = seconds * packets_per_second * avg_packet_size > > SetBufferMemorySize(memory_size) > > } > > How about creating multiple variants of the Create function? For example a > default one with a certain default buffer size, and a CreateWithBufferDuration > to set a certain buffer size explicitly. If the meaning should be changed later, > a new CreateWithBufferSize function can be added, and the old one can be > removed. This would have similar advantages as having a function, while still > having the buffer size/duration as a constant in the RtcEventLogImpl class > (which I would personally prefer). Following the discussions we had last week with Jiayang, I think we will have to move to a memory limit rather than a time limit. With the current implementation there is no way to construct the object based on a memory limit, so the constructor would still have to take a duration as a parameter. I suggest we hold off on this until we have the new buffers. Does this sound reasonable?
lgtm https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... File webrtc/video/rtc_event_log.cc (right): https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log.cc:405: config_events_.push_back(recent_log_events_.front()); On 2015/10/15 13:28:21, terelius wrote: > On 2015/09/25 15:30:23, ivoc wrote: > > On 2015/08/26 08:15:28, ivoc wrote: > > > On 2015/08/25 16:41:44, terelius wrote: > > > > On 2015/08/25 08:03:21, ivoc wrote: > > > > > I wonder if we should be checking if any of the config events that are > > > already > > > > > stored in config_events_ might become obsolete due to a newer config > > event. > > > Is > > > > > that something that could happen? > > > > > > > > This is a good point. > > > > > > > > I don't think it is possible to reconfigure (video) streams during > runtime. > > > > However, it is possible to delete the stream and create a new one with the > > > same > > > > SSRC which would result in two config events for the "same" stream. > > > > > > > > It is not clear whether we should resolve those ambiguities while writing > > the > > > > log or when we read it. Doing the check here would not completely solve > the > > > > issue since we could potentially have multiple config events in the > > > > recent_log_events queue. > > > > > > I think a check here would solve this problem. Even if the recent_events_ > > queue > > > contains another config event that will succeed one of the ones from the > > > config_events_ list, the one that is stored in config_events_ is still valid > > for > > > any older packets that are in the recent_events_ queue, and is therefore > still > > > needed. Once the succeeding config event is too old for the recent_events_ > > > queue, it can replace the old one in config_events_ since it is no longer > > > useful. > > > > > > I'm not sure if it's better to solve this while writing or reading. I guess > it > > > also depends on how likely it is that this happens. If it happens > frequently, > > it > > > is not nice if the RtcEventLog starts collecting a whole list of config > events > > > that are no longer valid. If it is rare, then it's probably acceptable to > > leave > > > it in. I don't know how often this happens in practice, maybe one of the > other > > > reviewers can comment? If we decide to solve it while reading, I think it > > would > > > be good to document this behavior somewhere (maybe in the protobuf, or with > > the > > > parsing functions?). > > > > Could you respond to this? I still think it would be better to purge old > configs > > that are not needed to interpret any packets that appear in the log or in the > > recentEvents_queue, and I don't think it would be that complicated to add that > > feature (unless I'm missing something). > > So what you are proposing is checking whether there exists an older config in > config_events before adding a new config for the same SSRC? > > The reason why I am hesitant is that we would add code for a rare event that > wont be tested exhaustively, yet we would not solve the problem completely. (We > could still have unused configurations for streams that are configured but never > receive or send any packets. We could also have two configurations for the > "same" stream if there are packets sent before the reconfiguration. Finally, if > the reconfiguration is done while we are logging then everything would be > written to file directly so there is no way to detect whether we have > unnecessary configurations.) > > It is also slightly complicated to solve due to the fact that VideoSendConfig > doesn't have a single SSRC, but a whole vector of them. Handling configurations > with overlapping but nonidentical ssrc vectors will probably be ugly. Okay, let's stick to the simple solution for now. We can analyze later if there actually is a problem in practice or if it's a complete non-issue (probably the latter). https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_log.h File webrtc/video/rtc_event_log.h (right): https://codereview.webrtc.org/1303713002/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log.h:43: virtual void SetBufferDuration(int64_t recent_log_duration_us) = 0; On 2015/10/15 13:28:21, terelius wrote: > On 2015/09/25 15:30:23, ivoc wrote: > > On 2015/08/26 08:43:37, terelius wrote: > > > On 2015/08/26 08:15:28, ivoc wrote: > > > > On 2015/08/25 16:41:44, terelius wrote: > > > > > On 2015/08/25 08:03:21, ivoc wrote: > > > > > > How useful is it to be able to change the size of the buffer at > runtime? > > > In > > > > > > which cases would this function be called? > > > > > > > > > > At the moment it is only used for testing purposes. I want to construct > a > > > unit > > > > > test where old events are removed from the queue, and waiting 10 seconds > > > would > > > > > be impractical. In the future, there might be real use cases e.g. > > allocating > > > > > more memory for logging on high end devices. > > > > > > > > > > I considered passing the logging duration to the constructor, but as we > > > > > discussed before we might have to change to a separate ring buffer for > the > > > RTP > > > > > and RTCP packets. (Using a separate buffer would avoid constructing > > protobuf > > > > > messages when not logging, would avoid dynamic memory allocation when > > > queueing > > > > > an event, and would avoid lock contention between the audio and video > > > > threads.) > > > > > In that case, logging "duration" will be set based on memory usage > rather > > > than > > > > > time, and there is no good way to change the usage of constructor > > arguments. > > > > > > > > Hmm, I don't really see why there no good way to change constructor > > arguments, > > > > compared to changing function arguments. To me it seems to be pretty > > similar. > > > > > > Using a function you can choose a new name, e.g. SetBufferMemorySize. > > > You can then either remove SetBufferDuration to cause compilation errors in > > code > > > that don't use the new API, or you can change SetBufferDuration to estimate > > the > > > required memory like this > > > > > > void SetBufferDuration(seconds) { > > > memory_size = seconds * packets_per_second * avg_packet_size > > > SetBufferMemorySize(memory_size) > > > } > > > > How about creating multiple variants of the Create function? For example a > > default one with a certain default buffer size, and a CreateWithBufferDuration > > to set a certain buffer size explicitly. If the meaning should be changed > later, > > a new CreateWithBufferSize function can be added, and the old one can be > > removed. This would have similar advantages as having a function, while still > > having the buffer size/duration as a constant in the RtcEventLogImpl class > > (which I would personally prefer). > > Following the discussions we had last week with Jiayang, I think we will have to > move to a memory limit rather than a time limit. With the current implementation > there is no way to construct the object based on a memory limit, so the > constructor would still have to take a duration as a parameter. > > I suggest we hold off on this until we have the new buffers. Does this sound > reasonable? Okay, sounds good.
lgtm
The CQ bit was checked by terelius@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/1303713002/#ps100001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1303713002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1303713002/100001
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/1253)
The CQ bit was checked by terelius@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org, henrik.lundin@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1303713002/#ps120001 (title: "Rebase again")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1303713002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1303713002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by terelius@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1303713002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1303713002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/1adce14c87f8316b5c3f4ee16b8f573c8bf14761 Cr-Commit-Position: refs/heads/master@{#10302}
Message was sent while issue was closed.
solenberg@webrtc.org changed reviewers: + solenberg@webrtc.org
Message was sent while issue was closed.
https://codereview.webrtc.org/1303713002/diff/120001/webrtc/call/rtc_event_lo... File webrtc/call/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1303713002/diff/120001/webrtc/call/rtc_event_lo... webrtc/call/rtc_event_log_unittest.cc:568: size_t packet_size = 1000 + rand() % 64; Avoid rand() in unit tests; it accesses global state, so if used from multiple threads, your random sequence may differ between test runs. Here's a simple PRNG you can use instead: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
