Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1216613002:
Extend full stack tests with more stats (Closed)
|
Created:
5 years, 6 months ago by sprang_webrtc Modified:
5 years, 5 months ago Reviewers:
pbos-webrtc 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. |
DescriptionExtend full stack tests with more stats
BUG=
R=pbos@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/085856cd35c3ec7d5b73d6788465868bbd99ae26
Patch Set 1 #Patch Set 2 : Formatting #
Total comments: 10
Patch Set 3 : Comment #Patch Set 4 : Addressed comments #Patch Set 5 : Cleanup #
Total comments: 8
Patch Set 6 : Addressed comments #Patch Set 7 : Fixed race, addressed comments #Messages
Total messages: 10 (1 generated)
sprang@webrtc.org changed reviewers: + pbos@webrtc.org
https://codereview.webrtc.org/1216613002/diff/20001/webrtc/video/full_stack.cc File webrtc/video/full_stack.cc (right): https://codereview.webrtc.org/1216613002/diff/20001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:170: encoded_frame_sizes_[timestamp] += Why do you need a += here if you remove the stats further down? This looks racy if you want to catch all the combined simulcast layers. https://codereview.webrtc.org/1216613002/diff/20001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:197: VideoSendStream::Stats send_stats = send_stats_.front(); You can pop this one after AddFrameComparison so you don't need a temporary, up to you though. https://codereview.webrtc.org/1216613002/diff/20001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:413: encode_frame_rate_.AddSample(comparison.send_stats.encode_frame_rate); Does this send_stats even necessarily correspond to the same frame? Otherwise I see no reason to add it here, can't we just add samples from when we call GetStats in the first place?
https://codereview.webrtc.org/1216613002/diff/20001/webrtc/video/full_stack.cc File webrtc/video/full_stack.cc (right): https://codereview.webrtc.org/1216613002/diff/20001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:170: encoded_frame_sizes_[timestamp] += On 2015/06/28 18:35:31, pbos-webrtc wrote: > Why do you need a += here if you remove the stats further down? This looks racy > if you want to catch all the combined simulcast layers. Is simulcast used in the full stack tests? Then how is send_times_ not racy? The reason for the += is to capture the encoded size of a frame if it is fragmented across several rtp packets. I'd rather have used VideoSenderStream.post_encode_callback but EncodedFrameObserver takes an EncodedFrame which doesn't give us anything to identify it by... Any other ideas of how to solve this? If it's even needed - I did this before adding the send stream stats, perhaps the media send bitrate is enough.. https://codereview.webrtc.org/1216613002/diff/20001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:197: VideoSendStream::Stats send_stats = send_stats_.front(); On 2015/06/28 18:35:32, pbos-webrtc wrote: > You can pop this one after AddFrameComparison so you don't need a temporary, up > to you though. Done. https://codereview.webrtc.org/1216613002/diff/20001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:413: encode_frame_rate_.AddSample(comparison.send_stats.encode_frame_rate); On 2015/06/28 18:35:31, pbos-webrtc wrote: > Does this send_stats even necessarily correspond to the same frame? Otherwise I > see no reason to add it here, can't we just add samples from when we call > GetStats in the first place? The reason I did this was that I wanted samples from the same time frame as the the other stats. If we do .AddSample when we capture a frame we might be adding data points for a while after the we actually stop adding frame comparisons.
https://codereview.webrtc.org/1216613002/diff/20001/webrtc/video/full_stack.cc File webrtc/video/full_stack.cc (right): https://codereview.webrtc.org/1216613002/diff/20001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:170: encoded_frame_sizes_[timestamp] += On 2015/07/06 11:19:00, språng wrote: > On 2015/06/28 18:35:31, pbos-webrtc wrote: > > Why do you need a += here if you remove the stats further down? This looks > racy > > if you want to catch all the combined simulcast layers. > > Is simulcast used in the full stack tests? Then how is send_times_ not racy? > The reason for the += is to capture the encoded size of a frame if it is > fragmented across several rtp packets. I'd rather have used > VideoSenderStream.post_encode_callback but EncodedFrameObserver takes an > EncodedFrame which doesn't give us anything to identify it by... > Any other ideas of how to solve this? > If it's even needed - I did this before adding the send stream stats, perhaps > the media send bitrate is enough.. It won't be popped unless it's rendered, right? That requires all packets to be delivered/given up. For one stream that's fine, we won't try to render it twice. For simulcast we have 3 separate streams but one of them can be rendered before the last one is even sent. https://codereview.webrtc.org/1216613002/diff/20001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:413: encode_frame_rate_.AddSample(comparison.send_stats.encode_frame_rate); On 2015/07/06 11:19:00, språng wrote: > On 2015/06/28 18:35:31, pbos-webrtc wrote: > > Does this send_stats even necessarily correspond to the same frame? Otherwise > I > > see no reason to add it here, can't we just add samples from when we call > > GetStats in the first place? > > The reason I did this was that I wanted samples from the same time frame as the > the other stats. If we do .AddSample when we capture a frame we might be adding > data points for a while after the we actually stop adding frame comparisons. Ok, so this essentially replaces a separate thread doing GetStats() every k ms, and should not be corresponding to frame samples? I'm wondering whether we should do that, WDYT?
https://codereview.webrtc.org/1216613002/diff/20001/webrtc/video/full_stack.cc File webrtc/video/full_stack.cc (right): https://codereview.webrtc.org/1216613002/diff/20001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:170: encoded_frame_sizes_[timestamp] += On 2015/07/06 11:36:00, pbos-webrtc wrote: > On 2015/07/06 11:19:00, språng wrote: > > On 2015/06/28 18:35:31, pbos-webrtc wrote: > > > Why do you need a += here if you remove the stats further down? This looks > > racy > > > if you want to catch all the combined simulcast layers. > > > > Is simulcast used in the full stack tests? Then how is send_times_ not racy? > > The reason for the += is to capture the encoded size of a frame if it is > > fragmented across several rtp packets. I'd rather have used > > VideoSenderStream.post_encode_callback but EncodedFrameObserver takes an > > EncodedFrame which doesn't give us anything to identify it by... > > Any other ideas of how to solve this? > > If it's even needed - I did this before adding the send stream stats, perhaps > > the media send bitrate is enough.. > > It won't be popped unless it's rendered, right? That requires all packets to be > delivered/given up. For one stream that's fine, we won't try to render it twice. > For simulcast we have 3 separate streams but one of them can be rendered before > the last one is even sent. True, what I'm saying is that send_times is racy as well then? Are we using simulcast in these test? Anyway, I removed this and instead have the pre_decode_callback directly add a sample. This might not be entirely correct, but it'll have to be good enough. https://codereview.webrtc.org/1216613002/diff/20001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:413: encode_frame_rate_.AddSample(comparison.send_stats.encode_frame_rate); On 2015/07/06 11:36:00, pbos-webrtc wrote: > On 2015/07/06 11:19:00, språng wrote: > > On 2015/06/28 18:35:31, pbos-webrtc wrote: > > > Does this send_stats even necessarily correspond to the same frame? > Otherwise > > I > > > see no reason to add it here, can't we just add samples from when we call > > > GetStats in the first place? > > > > The reason I did this was that I wanted samples from the same time frame as > the > > the other stats. If we do .AddSample when we capture a frame we might be > adding > > data points for a while after the we actually stop adding frame comparisons. > > Ok, so this essentially replaces a separate thread doing GetStats() every k ms, > and should not be corresponding to frame samples? I'm wondering whether we > should do that, WDYT? Sure. I added a check for frames_captured_ instead to limit scope.
lgtm https://codereview.webrtc.org/1216613002/diff/80001/webrtc/video/full_stack.cc File webrtc/video/full_stack.cc (right): https://codereview.webrtc.org/1216613002/diff/80001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:304: default: Do you need default: here? (Is this an enum?) https://codereview.webrtc.org/1216613002/diff/80001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:311: if (frames_recorded_ >= frames_to_process_) Does this mean that we're done? If so add a comment saying why listening for done_ is not enough. If you're worried about adding one sample too many, then don't bother I think. :) https://codereview.webrtc.org/1216613002/diff/80001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:406: PrintResult("encode_frame_rate_", encode_frame_rate_, " fps"); encode_frame_rate (drop the _ suffix) https://codereview.webrtc.org/1216613002/diff/80001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:407: PrintResult("avg_encode_time", avg_encode_time_ms, " ms"); Should this be called avg_ ? All of these are averages, even though this is an average of averages.
Addressed comments and fixed a race in the test code. Not sure if you wanna take another pass? https://codereview.webrtc.org/1216613002/diff/80001/webrtc/video/full_stack.cc File webrtc/video/full_stack.cc (right): https://codereview.webrtc.org/1216613002/diff/80001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:304: default: On 2015/07/21 20:47:49, pbos-webrtc wrote: > Do you need default: here? (Is this an enum?) It's an enum. I'd like to get a compile-time error instead, but that doesn't seem to happen :( https://codereview.webrtc.org/1216613002/diff/80001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:311: if (frames_recorded_ >= frames_to_process_) On 2015/07/21 20:47:49, pbos-webrtc wrote: > Does this mean that we're done? If so add a comment saying why listening for > done_ is not enough. If you're worried about adding one sample too many, then > don't bother I think. :) Done. https://codereview.webrtc.org/1216613002/diff/80001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:406: PrintResult("encode_frame_rate_", encode_frame_rate_, " fps"); On 2015/07/21 20:47:49, pbos-webrtc wrote: > encode_frame_rate (drop the _ suffix) Done. https://codereview.webrtc.org/1216613002/diff/80001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:407: PrintResult("avg_encode_time", avg_encode_time_ms, " ms"); On 2015/07/21 20:47:49, pbos-webrtc wrote: > Should this be called avg_ ? All of these are averages, even though this is an > average of averages. I'll drop the avg. Just copied the name from the stats struct.
lgtm
Message was sent while issue was closed.
Committed patchset #7 (id:120001) manually as 085856cd35c3ec7d5b73d6788465868bbd99ae26 (presubmit successful). |
