WebRTC Code Reviews
Help | Chromium Project | Sign in
(82)

Issue 53629004: Add a unittest for VCMReceiver::FrameForDecoding. Mainly test the time control algorithm.

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 8 months ago by Qiang
Modified:
3 years, 8 months ago
Reviewers:
wtc, Stefan
CC:
webrtc-reviews_webrtc.org, tterriberry, Stefan, mflodman
Base URL:
https://chromium.googlesource.com/external/webrtc.git@master
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

Add a unittest for VCMReceiver::FrameForDecoding. Mainly test the time control algorithm.

Patch Set 1 #

Total comments: 19

Patch Set 2 : Coding Style Fix #

Total comments: 56

Patch Set 3 : Coding Style Fix #

Total comments: 28

Patch Set 4 : Coding Style Fix, Comment Rephrase #

Total comments: 13

Patch Set 5 : Coding style fix #

Patch Set 6 : Typo fix for DCHECK #

Total comments: 8

Patch Set 7 : Avoid turning back the clock. #

Total comments: 10

Patch Set 8 : Comment Fix #

Total comments: 7

Patch Set 9 : Remove Unnecessary Factory Use #

Total comments: 8

Patch Set 10 : Remove a constructor #

Unified diffs Side-by-side diffs Delta from patch set Stats (+249 lines, -12 lines) Patch
M webrtc/modules/video_coding/main/source/jitter_buffer.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -2 lines 0 comments Download
M webrtc/modules/video_coding/main/source/jitter_buffer.cc View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -2 lines 0 comments Download
M webrtc/modules/video_coding/main/source/jitter_buffer_unittest.cc View 1 2 3 4 5 6 7 8 9 2 chunks +6 lines, -3 lines 0 comments Download
M webrtc/modules/video_coding/main/source/receiver.h View 1 2 3 4 5 6 7 8 1 chunk +10 lines, -0 lines 0 comments Download
M webrtc/modules/video_coding/main/source/receiver.cc View 1 2 3 4 5 6 7 8 1 chunk +14 lines, -2 lines 0 comments Download
M webrtc/modules/video_coding/main/source/receiver_unittest.cc View 1 2 3 4 5 6 7 8 2 chunks +208 lines, -0 lines 0 comments Download
M webrtc/modules/video_coding/main/source/test/stream_generator.h View 1 2 3 4 1 chunk +4 lines, -1 line 0 comments Download
M webrtc/modules/video_coding/main/source/test/stream_generator.cc View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
Trybot results: Sign in to try more bots
Project "webrtc" does not have a commit queue.

Messages

Total messages: 26 (5 generated)
Qiang
Unit test for VCMReceiver::FrameForDecoding.
3 years, 8 months ago (2015-05-28 21:13:33 UTC) #2
Stefan
https://webrtc-codereview.appspot.com/53629004/diff/1/webrtc/modules/video_coding/main/source/receiver_unittest.cc File webrtc/modules/video_coding/main/source/receiver_unittest.cc (right): https://webrtc-codereview.appspot.com/53629004/diff/1/webrtc/modules/video_coding/main/source/receiver_unittest.cc#newcode39 webrtc/modules/video_coding/main/source/receiver_unittest.cc:39: &event_factory_) { It looks strange that we have to ...
3 years, 8 months ago (2015-05-29 07:34:23 UTC) #4
Qiang
Hi, Stefan Thank you for your review. The current test is just the bottom line ...
3 years, 8 months ago (2015-05-29 18:12:10 UTC) #7
wtc
Review comments on patch set 2: Thank you very much for making the effort to ...
3 years, 8 months ago (2015-05-29 22:00:32 UTC) #8
wtc
https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/video_coding/main/source/receiver_unittest.cc File webrtc/modules/video_coding/main/source/receiver_unittest.cc (right): https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/video_coding/main/source/receiver_unittest.cc#newcode576 webrtc/modules/video_coding/main/source/receiver_unittest.cc:576: EXPECT_EQ(end_time - start_time, kMaxWaitTime); For EXPECT_EQ, EXPECT_LE, etc., the ...
3 years, 8 months ago (2015-05-29 22:03:14 UTC) #9
Qiang
https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/video_coding/main/source/receiver.cc File webrtc/modules/video_coding/main/source/receiver.cc (right): https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/video_coding/main/source/receiver.cc#newcode32 webrtc/modules/video_coding/main/source/receiver.cc:32: : VCMReceiver(timing, clock, event_factory, master, event_factory) { On 2015/05/29 ...
3 years, 8 months ago (2015-06-01 21:47:52 UTC) #10
wtc
Review comments on patch set 3: Mostly coding style nits. It may be better to ...
3 years, 8 months ago (2015-06-02 22:22:23 UTC) #11
Qiang
https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/video_coding/main/source/receiver_unittest.cc File webrtc/modules/video_coding/main/source/receiver_unittest.cc (right): https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/video_coding/main/source/receiver_unittest.cc#newcode354 webrtc/modules/video_coding/main/source/receiver_unittest.cc:354: virtual bool Set() override { return true; } On ...
3 years, 8 months ago (2015-06-03 17:53:46 UTC) #13
wtc
Review comments on patch set 4: Please fix the coding style nits, run your unit ...
3 years, 8 months ago (2015-06-03 22:50:06 UTC) #14
Qiang
https://webrtc-codereview.appspot.com/53629004/diff/120001/webrtc/modules/video_coding/main/source/receiver.h File webrtc/modules/video_coding/main/source/receiver.h (right): https://webrtc-codereview.appspot.com/53629004/diff/120001/webrtc/modules/video_coding/main/source/receiver.h#newcode46 webrtc/modules/video_coding/main/source/receiver.h:46: // coming, in which case the jitter buffer's wait ...
3 years, 8 months ago (2015-06-04 00:11:29 UTC) #15
wtc
Stefan: this CL is ready for your review now. Patch set 6 LGTM.
3 years, 8 months ago (2015-06-04 01:05:27 UTC) #16
Stefan
https://webrtc-codereview.appspot.com/53629004/diff/160001/webrtc/modules/video_coding/main/source/receiver_unittest.cc File webrtc/modules/video_coding/main/source/receiver_unittest.cc (right): https://webrtc-codereview.appspot.com/53629004/diff/160001/webrtc/modules/video_coding/main/source/receiver_unittest.cc#newcode363 webrtc/modules/video_coding/main/source/receiver_unittest.cc:363: // Insert Frame with timestamps_.front().render_time_ into JitterBuffer. Frame -> ...
3 years, 8 months ago (2015-06-04 13:26:10 UTC) #17
Qiang
Did some structural change to avoid turning back the clock. https://webrtc-codereview.appspot.com/53629004/diff/160001/webrtc/modules/video_coding/main/source/receiver_unittest.cc File webrtc/modules/video_coding/main/source/receiver_unittest.cc (right): https://webrtc-codereview.appspot.com/53629004/diff/160001/webrtc/modules/video_coding/main/source/receiver_unittest.cc#newcode363 ...
3 years, 8 months ago (2015-06-05 16:26:28 UTC) #18
wtc
Review comments on patch set 7: I'm sorry I forgot to review this CL until ...
3 years, 8 months ago (2015-06-05 23:51:38 UTC) #19
Qiang
Hi, Stefan: Can you take a look at this? Wan-Teh is in vacation for this ...
3 years, 8 months ago (2015-06-08 16:08:40 UTC) #20
Stefan
Much better! https://webrtc-codereview.appspot.com/53629004/diff/200001/webrtc/modules/video_coding/main/source/receiver.h File webrtc/modules/video_coding/main/source/receiver.h (right): https://webrtc-codereview.appspot.com/53629004/diff/200001/webrtc/modules/video_coding/main/source/receiver.h#newcode52 webrtc/modules/video_coding/main/source/receiver.h:52: EventFactory* jitter_buffer_event_factory); I think I would prefer ...
3 years, 8 months ago (2015-06-09 14:38:10 UTC) #21
Qiang
https://webrtc-codereview.appspot.com/53629004/diff/200001/webrtc/modules/video_coding/main/source/receiver.h File webrtc/modules/video_coding/main/source/receiver.h (right): https://webrtc-codereview.appspot.com/53629004/diff/200001/webrtc/modules/video_coding/main/source/receiver.h#newcode52 webrtc/modules/video_coding/main/source/receiver.h:52: EventFactory* jitter_buffer_event_factory); On 2015/06/09 14:38:09, Stefan wrote: > I ...
3 years, 8 months ago (2015-06-09 17:12:39 UTC) #22
Stefan
looks good now, but i hope we can remove the old constructors https://webrtc-codereview.appspot.com/53629004/diff/200001/webrtc/modules/video_coding/main/source/receiver.h File webrtc/modules/video_coding/main/source/receiver.h ...
3 years, 8 months ago (2015-06-10 08:01:01 UTC) #23
Qiang
My concern of signature change is that it may impact other projects. If we need ...
3 years, 8 months ago (2015-06-10 16:04:35 UTC) #24
Stefan
lgtm if you prefer removing the old constructor in a separate CL. https://webrtc-codereview.appspot.com/53629004/diff/220001/webrtc/modules/video_coding/main/source/jitter_buffer.h File webrtc/modules/video_coding/main/source/jitter_buffer.h ...
3 years, 8 months ago (2015-06-11 10:24:24 UTC) #25
Qiang
3 years, 8 months ago (2015-06-12 16:52:52 UTC) #26
Removed the unused constructor of jitter buffer. 
Keep the one in VCMReceiver, as that will be more consistent with VideoReceiver.

https://webrtc-codereview.appspot.com/53629004/diff/220001/webrtc/modules/vid...
File webrtc/modules/video_coding/main/source/jitter_buffer.h (right):

https://webrtc-codereview.appspot.com/53629004/diff/220001/webrtc/modules/vid...
webrtc/modules/video_coding/main/source/jitter_buffer.h:80: EventFactory*
event_factory);
On 2015/06/11 10:24:24, Stefan wrote:
> On 2015/06/10 16:04:35, Qiang wrote:
> > On 2015/06/10 08:01:00, Stefan wrote:
> > > Maybe this can be removed now as well?
> > 
> > Removing a signature will require changes to all its references. In WebRTC,
> > there are two unittests referring VCMJitterBuffer. Theoretically, there can
> also
> > be references to it from other projects depending on WebRTC. I'm not sure
> > whether it is a worth here to make such a big change.
> 
> I don't think there is anything else using the jitter buffer directly, so that
> should be safe as long as we keep the video coding modules interface intact.

Done.

https://webrtc-codereview.appspot.com/53629004/diff/220001/webrtc/modules/vid...
File webrtc/modules/video_coding/main/source/receiver.cc (right):

https://webrtc-codereview.appspot.com/53629004/diff/220001/webrtc/modules/vid...
webrtc/modules/video_coding/main/source/receiver.cc:36:
rtc::scoped_ptr<EventWrapper>(event_factory->CreateEvent())) {
On 2015/06/11 10:24:24, Stefan wrote:
> On 2015/06/10 16:04:35, Qiang wrote:
> > On 2015/06/10 08:01:01, Stefan wrote:
> > > Maybe remove this constructor and call the one below from
video_receiver.cc?
> > 
> > Similar to jitter_buffer.h. Signature change may impact other projects.
> 
> Also at this level the only user of this class is the VCM itself, so should be
> fine.

When looking at the code of VideoReceiver, I think it is better to keep this
constructor here, as it is used twice for creating events.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 245c2c2-tainted