WebRTC Code Reviews
|
|
DescriptionAdd 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 #
Project "webrtc" does not have a commit queue.
Messages
Total messages: 26 (5 generated)
qiangchen@google.com changed reviewers: + wtc@chromium.org
Sign in to reply to this message.
Unit test for VCMReceiver::FrameForDecoding.
Sign in to reply to this message.
stefan@webrtc.org changed reviewers: + stefan@webrtc.org
Sign in to reply to this message.
https://webrtc-codereview.appspot.com/53629004/diff/1/webrtc/modules/video_co... File webrtc/modules/video_coding/main/source/receiver_unittest.cc (right): https://webrtc-codereview.appspot.com/53629004/diff/1/webrtc/modules/video_co... webrtc/modules/video_coding/main/source/receiver_unittest.cc:39: &event_factory_) { It looks strange that we have to pass the event_factory_ twice. https://webrtc-codereview.appspot.com/53629004/diff/1/webrtc/modules/video_co... webrtc/modules/video_coding/main/source/receiver_unittest.cc:369: // Insert Frame with render_timestamps_[useIndex_] into JitterBuffer useIndex_ doesn't exist in the code https://webrtc-codereview.appspot.com/53629004/diff/1/webrtc/modules/video_co... webrtc/modules/video_coding/main/source/receiver_unittest.cc:382: // adjust the clock back Adjust the clock back. https://webrtc-codereview.appspot.com/53629004/diff/1/webrtc/modules/video_co... webrtc/modules/video_coding/main/source/receiver_unittest.cc:389: // Insert Frame with render_timestamps_[useIndex_] into JitterBuffer s/Frame/frame and end the comment with a period "." https://webrtc-codereview.appspot.com/53629004/diff/1/webrtc/modules/video_co... webrtc/modules/video_coding/main/source/receiver_unittest.cc:398: // To Here there is no frame enter during [start_time, end_time] s/"To here..."/"Until this point"? s/enter/entering https://webrtc-codereview.appspot.com/53629004/diff/1/webrtc/modules/video_co... webrtc/modules/video_coding/main/source/receiver_unittest.cc:450: // setting from constructor of this EventFactory Reformat this comment. https://webrtc-codereview.appspot.com/53629004/diff/1/webrtc/modules/video_co... webrtc/modules/video_coding/main/source/receiver_unittest.cc:479: * */ No c-style comments. Use // comments. I'm not sure what "turn" means here? https://webrtc-codereview.appspot.com/53629004/diff/1/webrtc/modules/video_co... webrtc/modules/video_coding/main/source/receiver_unittest.cc:507: class TestVCMReceiverTiming : public ::testing::Test { VcmReceiverTimingTest instead https://webrtc-codereview.appspot.com/53629004/diff/1/webrtc/modules/video_co... webrtc/modules/video_coding/main/source/receiver_unittest.cc:520: frame_inject_event_factory_.get())) { You should be able to allocate all of these objects on the stack instead. https://webrtc-codereview.appspot.com/53629004/diff/1/webrtc/modules/video_co... webrtc/modules/video_coding/main/source/receiver_unittest.cc:548: enter_timestamps[i] = (i + 1) * 40 + (i % 10) * ((i % 2) ? 1 : -1); I think this needs some clarification :) https://webrtc-codereview.appspot.com/53629004/diff/1/webrtc/modules/video_co... webrtc/modules/video_coding/main/source/receiver_unittest.cc:563: num_frames++; ++num_frames, same at line 567
Sign in to reply to this message.
Patchset #2 (id:20001) has been deleted
Sign in to reply to this message.
Patchset #2 (id:40001) has been deleted
Sign in to reply to this message.
Hi, Stefan
Thank you for your review.
The current test is just the bottom line of FrameForDecoding that each
parameter works as intended. I'm still considering writing a stronger test that
verify the output sequence of next_render_time_ms really smooth out the
fluctuation from the enter_timestamps, as this is the main purpose of
FrameForDecoding. Do you think it is necessary? Or do you have any other
suggestions on what helpful test we can add here?
Qiang
https://webrtc-codereview.appspot.com/53629004/diff/1/webrtc/modules/video_co...
File webrtc/modules/video_coding/main/source/receiver_unittest.cc (right):
https://webrtc-codereview.appspot.com/53629004/diff/1/webrtc/modules/video_co...
webrtc/modules/video_coding/main/source/receiver_unittest.cc:369: // Insert
Frame with render_timestamps_[useIndex_] into JitterBuffer
On 2015/05/29 07:34:23, Stefan wrote:
> useIndex_ doesn't exist in the code
Done.
https://webrtc-codereview.appspot.com/53629004/diff/1/webrtc/modules/video_co...
webrtc/modules/video_coding/main/source/receiver_unittest.cc:382: // adjust the
clock back
On 2015/05/29 07:34:23, Stefan wrote:
> Adjust the clock back.
Done.
https://webrtc-codereview.appspot.com/53629004/diff/1/webrtc/modules/video_co...
webrtc/modules/video_coding/main/source/receiver_unittest.cc:389: // Insert
Frame with render_timestamps_[useIndex_] into JitterBuffer
On 2015/05/29 07:34:23, Stefan wrote:
> s/Frame/frame
> and end the comment with a period "."
Done.
https://webrtc-codereview.appspot.com/53629004/diff/1/webrtc/modules/video_co...
webrtc/modules/video_coding/main/source/receiver_unittest.cc:398: // To Here
there is no frame enter during [start_time, end_time]
On 2015/05/29 07:34:23, Stefan wrote:
> s/"To here..."/"Until this point"?
> s/enter/entering
rephrased to be more consistent with above code.
https://webrtc-codereview.appspot.com/53629004/diff/1/webrtc/modules/video_co...
webrtc/modules/video_coding/main/source/receiver_unittest.cc:450: // setting
from constructor of this EventFactory
On 2015/05/29 07:34:23, Stefan wrote:
> Reformat this comment.
Done.
https://webrtc-codereview.appspot.com/53629004/diff/1/webrtc/modules/video_co...
webrtc/modules/video_coding/main/source/receiver_unittest.cc:507: class
TestVCMReceiverTiming : public ::testing::Test {
On 2015/05/29 07:34:23, Stefan wrote:
> VcmReceiverTimingTest instead
Done.
https://webrtc-codereview.appspot.com/53629004/diff/1/webrtc/modules/video_co...
webrtc/modules/video_coding/main/source/receiver_unittest.cc:520:
frame_inject_event_factory_.get())) {
On 2015/05/29 07:34:23, Stefan wrote:
> You should be able to allocate all of these objects on the stack instead.
Done.
https://webrtc-codereview.appspot.com/53629004/diff/1/webrtc/modules/video_co...
webrtc/modules/video_coding/main/source/receiver_unittest.cc:548:
enter_timestamps[i] = (i + 1) * 40 + (i % 10) * ((i % 2) ? 1 : -1);
On 2015/05/29 07:34:23, Stefan wrote:
> I think this needs some clarification :)
Done.
Sign in to reply to this message.
Review comments on patch set 2: Thank you very much for making the effort to add a unit test for your bug fix. Most of my review comments are about coding style nits. My first impression of the two new EventWrapper subclasses was that their Wait() methods have surprising side effects. After I read the entire CL, I understood why. If you can come up with a different design to avoid the surprising side effects in Wait(), that would be great. Otherwise, I think the current design is fine and you can just add some comments to explain more. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... File webrtc/modules/video_coding/main/source/receiver.cc (right): https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver.cc:32: : VCMReceiver(timing, clock, event_factory, master, event_factory) { Please find out if it is portable to invoke a constructor like this. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... File webrtc/modules/video_coding/main/source/receiver.h (right): https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver.h:47: EventFactory* jitter_buffer_event_factory); Please add a comment to document this constructor, especially what each of the EventFactory inputs is used for. You may also point that that this is only used for unit tests. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... File webrtc/modules/video_coding/main/source/receiver_unittest.cc (right): https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:338: // Wait Call will do either of this: Nit: lowercase "call". this => these https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:341: // now+max_wait_time Nit: max_wait_time => max_time ? https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:345: class FrameInjectEvent : public webrtc::EventWrapper { Delete webrtc:: because we are in the webrtc namespace. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:357: virtual bool Reset() { return true; } Delete the Reset() method. It is no longer in the EventWrapper interface. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:359: virtual EventTypeWrapper Wait(unsigned long max_time) { Please use "override" on the virtual destructor and methods that you override. See the Style Guide: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance Explicitly annotate overrides of virtual functions or virtual destructors with an override or (less frequently) final specifier. Please make this change throughout this file. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:366: Nit: either delete this blank line, or add // in the front. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:367: // revert the clock, insert frame, as if we inserted the frame at Nit: "revert" the clock is confusing. I suggest something like "Change the clock's time to enter_timestamps_.front()" or "Turn the clock backward to enter_timestamps_.front()". https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:378: // Adjust the clock back Nit: "Adjust the clock back" is a little confusing because "back" may be misinterpreted to mean "back into the past". I suggest changing this to "Restore the clock." https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:412: // manage storage Nit: comments should have proper capitalization and punctuation. So lines 407 and 412 should read: // Just a reference. // Manage storage. But these two comments don't seem useful, so I would suggest removing them or replacing them with better comments. For example, "manage storage" doesn't tell me anything about what the timestamps are. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:421: render_timestamp); This argument is named |current_time| in StreamGenerator's header file. So I don't understand why |render_timestamp| is the current time. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:438: event_ = nullptr; Initialize these members using an initializer list: : clock_(clock), stream_generator_(stream_generator), receiver_(nullptr), event_(nullptr) { https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:442: // It is necessary to make a function here, because receiver's constructor Nit: "It is necessary to make a function here" => "The SetReceiver function is necessary" https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:443: // requires an EventFactory. To make the EventFactory back point to the back point => point back https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:450: virtual EventWrapper* CreateEvent() { List CreateEvent() (a method of the EventFactory interface) first. Then list the other methods. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:467: VCMReceiver* receiver_; We don't need the |receiver_| member. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:468: FrameInjectEvent* event_; Please try to avoid the |event_| member, which stores the most recently created event. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:473: class TurnClockEvent : public webrtc::EventWrapper { I suggest we change "TurnClock" to "AdvanceClock" or "ClockAdvance". Delete webrtc:: because we're in the webrtc namespace. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:479: virtual bool Reset() { return true; } Delete the Reset() method. It is no longer in the EventWrapper interface. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:490: class TurnClockEventFactory : public EventFactory { Change "TurnClock" to "AdvanceClock" or "ClockAdvance". https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:504: stream_generator_(0, 0, 0), Nit: I suggest passing clock_->TimeInMilliseconds() as the third argument. It's the same value, but is more meaningful. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:536: const int kNumFrames = 100; Nit: use the size_t type for object size or number of elements. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:546: // But we add a reasonable deviation on enter_timestamps to mimic Internet Nit: on => to https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:557: while (num_frames_return < kNumFrames && num_null_return < 10) { Please document what num_frames_return and num_null_return mean, and why you don't want num_null_return to exceed 10.
Sign in to reply to this message.
https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... File webrtc/modules/video_coding/main/source/receiver_unittest.cc (right): https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:576: EXPECT_EQ(end_time - start_time, kMaxWaitTime); For EXPECT_EQ, EXPECT_LE, etc., the first argument is the expected value (often a constant), and the second argument is the actual value (a variable).
Sign in to reply to this message.
https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... File webrtc/modules/video_coding/main/source/receiver.cc (right): https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver.cc:32: : VCMReceiver(timing, clock, event_factory, master, event_factory) { On 2015/05/29 22:00:31, wtc wrote: > > Please find out if it is portable to invoke a constructor like this. C++ 11 introduced the delegating constructor. So I think it is safe. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... File webrtc/modules/video_coding/main/source/receiver.h (right): https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver.h:47: EventFactory* jitter_buffer_event_factory); On 2015/05/29 22:00:31, wtc wrote: > > Please add a comment to document this constructor, especially what each of the > EventFactory inputs is used for. You may also point that that this is only used > for unit tests. Done. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... File webrtc/modules/video_coding/main/source/receiver_unittest.cc (right): https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:338: // Wait Call will do either of this: On 2015/05/29 22:00:32, wtc wrote: > > Nit: lowercase "call". > > this => these Done. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:338: // Wait Call will do either of this: On 2015/05/29 22:00:32, wtc wrote: > > Nit: lowercase "call". > > this => these Done. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:341: // now+max_wait_time On 2015/05/29 22:00:32, wtc wrote: > > Nit: max_wait_time => max_time ? Done. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:341: // now+max_wait_time On 2015/05/29 22:00:32, wtc wrote: > > Nit: max_wait_time => max_time ? Done. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:345: class FrameInjectEvent : public webrtc::EventWrapper { On 2015/05/29 22:00:32, wtc wrote: > > Delete webrtc:: because we are in the webrtc namespace. Done. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:345: class FrameInjectEvent : public webrtc::EventWrapper { On 2015/05/29 22:00:32, wtc wrote: > > Delete webrtc:: because we are in the webrtc namespace. Done. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:357: virtual bool Reset() { return true; } On 2015/05/29 22:00:32, wtc wrote: > > Delete the Reset() method. It is no longer in the EventWrapper interface. Done. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:357: virtual bool Reset() { return true; } On 2015/05/29 22:00:32, wtc wrote: > > Delete the Reset() method. It is no longer in the EventWrapper interface. Done. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:359: virtual EventTypeWrapper Wait(unsigned long max_time) { On 2015/05/29 22:00:31, wtc wrote: > > Please use "override" on the virtual destructor and methods that you override. > See the Style Guide: > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance > > Explicitly annotate overrides of virtual functions or > virtual destructors with an override or (less frequently) > final specifier. > > Please make this change throughout this file. Done. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:366: On 2015/05/29 22:00:32, wtc wrote: > > Nit: either delete this blank line, or add // in the front. Done. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:367: // revert the clock, insert frame, as if we inserted the frame at On 2015/05/29 22:00:31, wtc wrote: > > Nit: "revert" the clock is confusing. I suggest something like "Change the > clock's time to enter_timestamps_.front()" or "Turn the clock backward to > enter_timestamps_.front()". Done. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:378: // Adjust the clock back On 2015/05/29 22:00:31, wtc wrote: > > Nit: "Adjust the clock back" is a little confusing because "back" may be > misinterpreted to mean "back into the past". I suggest changing this to "Restore > the clock." Done. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:412: // manage storage On 2015/05/29 22:00:32, wtc wrote: > > Nit: comments should have proper capitalization and punctuation. So lines 407 > and 412 should read: > > // Just a reference. > > // Manage storage. > > But these two comments don't seem useful, so I would suggest removing them or > replacing them with better comments. For example, "manage storage" doesn't tell > me anything about what the timestamps are. Just removed. As before, I used int64_t*, new an Array in constructor and delete it in Destructor. No longer needed this way. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:421: render_timestamp); On 2015/05/29 22:00:31, wtc wrote: > > This argument is named |current_time| in StreamGenerator's header file. So I > don't understand why |render_timestamp| is the current time. Add a comment. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:438: event_ = nullptr; On 2015/05/29 22:00:32, wtc wrote: > > Initialize these members using an initializer list: > : clock_(clock), > stream_generator_(stream_generator), > receiver_(nullptr), > event_(nullptr) { Done. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:442: // It is necessary to make a function here, because receiver's constructor On 2015/05/29 22:00:31, wtc wrote: > > Nit: "It is necessary to make a function here" => "The SetReceiver function is > necessary" Done. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:443: // requires an EventFactory. To make the EventFactory back point to the On 2015/05/29 22:00:32, wtc wrote: > > back point => point back Done. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:450: virtual EventWrapper* CreateEvent() { On 2015/05/29 22:00:31, wtc wrote: > > List CreateEvent() (a method of the EventFactory interface) first. Then list the > other methods. Done. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:467: VCMReceiver* receiver_; On 2015/05/29 22:00:31, wtc wrote: > > We don't need the |receiver_| member. Removed the SetReceiver. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:468: FrameInjectEvent* event_; On 2015/05/29 22:00:32, wtc wrote: > > Please try to avoid the |event_| member, which stores the most recently created > event. The only way I can figure out to do this is to hard code the testing samples in FrameInjectEvent. But I think it would be better so that the caller can pass his customized testing samples in. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:473: class TurnClockEvent : public webrtc::EventWrapper { On 2015/05/29 22:00:32, wtc wrote: > > I suggest we change "TurnClock" to "AdvanceClock" or "ClockAdvance". > > Delete webrtc:: because we're in the webrtc namespace. Done. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:479: virtual bool Reset() { return true; } On 2015/05/29 22:00:31, wtc wrote: > > Delete the Reset() method. It is no longer in the EventWrapper interface. Done. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:490: class TurnClockEventFactory : public EventFactory { On 2015/05/29 22:00:31, wtc wrote: > > Change "TurnClock" to "AdvanceClock" or "ClockAdvance". Done. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:504: stream_generator_(0, 0, 0), On 2015/05/29 22:00:32, wtc wrote: > > Nit: I suggest passing clock_->TimeInMilliseconds() as the third argument. It's > the same value, but is more meaningful. Done. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:536: const int kNumFrames = 100; On 2015/05/29 22:00:32, wtc wrote: > > Nit: use the size_t type for object size or number of elements. Done. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:546: // But we add a reasonable deviation on enter_timestamps to mimic Internet On 2015/05/29 22:00:32, wtc wrote: > > Nit: on => to Done. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:557: while (num_frames_return < kNumFrames && num_null_return < 10) { On 2015/05/29 22:00:32, wtc wrote: > > Please document what num_frames_return and num_null_return mean, and why you > don't want num_null_return to exceed 10. Done. https://webrtc-codereview.appspot.com/53629004/diff/60001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:576: EXPECT_EQ(end_time - start_time, kMaxWaitTime); On 2015/05/29 22:03:14, wtc wrote: > > For EXPECT_EQ, EXPECT_LE, etc., the first argument is the expected value (often > a constant), and the second argument is the actual value (a variable). Done.
Sign in to reply to this message.
Review comments on patch set 3: Mostly coding style nits. It may be better to make VCMReceiver expose the event of the jitter buffer for testing. https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... File webrtc/modules/video_coding/main/source/receiver.h (right): https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver.h:45: // jitter buffer. Useful for unit test when you want to simulate packets Nit: unit test => unit tests https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver.h:46: // coming at given timestamps, in which case, jitter buffer's wait event Nit: "given timestamps" sounds a little strange because the constructor doesn't take timestamps as inputs. Nit: remove the comma (,) after "in which case". Nit: Add "the" before "jitter buffer's". https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver.h:47: // handler is different from that of VCMReceiver itself. Nit: "wait event handler" is confusing because there isn't really such a thing. Perhaps remove "handler"? https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... File webrtc/modules/video_coding/main/source/receiver_unittest.cc (right): https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:354: virtual bool Set() override { return true; } Remove virtual. https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:356: virtual EventTypeWrapper Wait(unsigned long max_time) override { Remove virtual. https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:408: // |current_time|. Thank you for explaining this. We don't need a comment here. However, it may be useful to document the |current_time| parameter in stream_generator.h. https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:425: std::queue<int64_t> enter_timestamps_; Nit: change "enter" to "receive" or "arrive". https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:426: std::queue<int64_t> render_timestamps_; Nit: you can also make this a std::queue of a simple struct with two members: arrive_time and render_time. This makes it clear the timestamps always come in pairs. I leave this up to you. https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:448: if (!event_) Can event_ be null?! That seems like a bug. https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:469: virtual bool Set() override { return true; } Remove virtual. https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:471: virtual EventTypeWrapper Wait(unsigned long max_time) { Remove virtual, add override. https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:484: virtual EventWrapper* CreateEvent() override { Remove virtual. https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:552: // Ideally, we should get all frames that we input in InitializeFrames Nit: add a period (.) at the end of this sentence. https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:553: // However, we should also consider the case that the receiver kills some Nit: the case that => the case where https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:555: // |kNumFrames|. To avoid infinite loop, we set an alternative stop condition Nit: add "an" before "infinite loop". https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:558: // can pass the test. Nit: change "neither can pass the test" to "and the test should fail".
Sign in to reply to this message.
Patchset #4 (id:100001) has been deleted
Sign in to reply to this message.
https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... File webrtc/modules/video_coding/main/source/receiver_unittest.cc (right): https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:354: virtual bool Set() override { return true; } On 2015/06/02 22:22:23, wtc wrote: > > Remove virtual. Done. https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:356: virtual EventTypeWrapper Wait(unsigned long max_time) override { On 2015/06/02 22:22:23, wtc wrote: > > Remove virtual. Done. https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:425: std::queue<int64_t> enter_timestamps_; On 2015/06/02 22:22:22, wtc wrote: > > Nit: change "enter" to "receive" or "arrive". Done. https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:426: std::queue<int64_t> render_timestamps_; On 2015/06/02 22:22:23, wtc wrote: > > Nit: you can also make this a std::queue of a simple struct with two members: > arrive_time and render_time. This makes it clear the timestamps always come in > pairs. I leave this up to you. Done. https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:448: if (!event_) On 2015/06/02 22:22:23, wtc wrote: > > Can event_ be null?! That seems like a bug. Changed to DCHECK https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:469: virtual bool Set() override { return true; } On 2015/06/02 22:22:23, wtc wrote: > > Remove virtual. Done. https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:471: virtual EventTypeWrapper Wait(unsigned long max_time) { On 2015/06/02 22:22:23, wtc wrote: > > Remove virtual, add override. Done. https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:484: virtual EventWrapper* CreateEvent() override { On 2015/06/02 22:22:23, wtc wrote: > > Remove virtual. Done. https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:552: // Ideally, we should get all frames that we input in InitializeFrames On 2015/06/02 22:22:22, wtc wrote: > > Nit: add a period (.) at the end of this sentence. Done. https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:553: // However, we should also consider the case that the receiver kills some On 2015/06/02 22:22:23, wtc wrote: > > Nit: the case that => the case where Done. https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:555: // |kNumFrames|. To avoid infinite loop, we set an alternative stop condition On 2015/06/02 22:22:23, wtc wrote: > > Nit: add "an" before "infinite loop". Done. https://webrtc-codereview.appspot.com/53629004/diff/80001/webrtc/modules/vide... webrtc/modules/video_coding/main/source/receiver_unittest.cc:558: // can pass the test. On 2015/06/02 22:22:22, wtc wrote: > > Nit: change "neither can pass the test" to "and the test should fail". Done.
Sign in to reply to this message.
Review comments on patch set 4: Please fix the coding style nits, run your unit test in Debug build, and upload a new patch set. Thanks! https://webrtc-codereview.appspot.com/53629004/diff/120001/webrtc/modules/vid... File webrtc/modules/video_coding/main/source/receiver.h (right): https://webrtc-codereview.appspot.com/53629004/diff/120001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver.h:46: // coming, in which case the jitter buffer's wait event is different from Nit: perhaps change "packets coming" to "incoming packets" or "packet arrivals"? https://webrtc-codereview.appspot.com/53629004/diff/120001/webrtc/modules/vid... File webrtc/modules/video_coding/main/source/receiver_unittest.cc (right): https://webrtc-codereview.appspot.com/53629004/diff/120001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:404: int64_t render_time_; Nit: struct members should not have a trailing underscore. So these should be named as follows: int64_t arrive_time; int64_t render_time; See "Struct Data Members" in the Style Guide: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Variable_Names Nit: list data members after methods, including the constructors and destructor. See "Declaration Order" in the Style Guide: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_... https://webrtc-codereview.appspot.com/53629004/diff/120001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:406: TimestampPair(int64_t arrive_time, int64_t render_time) Note: it turns out we don't need to rename the constructor arguments. But to avoid confusion, you can rename them arrive_timestamp and render_timestamp. https://webrtc-codereview.appspot.com/53629004/diff/120001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:408: }; Nit: add a blank line after this line. https://webrtc-codereview.appspot.com/53629004/diff/120001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:449: DCHECK(!event_); IMPORTANT: this should read: DCHECK(event_); Please run your unit test again in a Debug build. https://webrtc-codereview.appspot.com/53629004/diff/120001/webrtc/modules/vid... File webrtc/modules/video_coding/main/source/test/stream_generator.h (right): https://webrtc-codereview.appspot.com/53629004/diff/120001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/test/stream_generator.h:38: // |time_ms| denotes timestamp you want to put on the frame, and the unit is Nit: add "the" before "timestamp". https://webrtc-codereview.appspot.com/53629004/diff/120001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/test/stream_generator.h:40: // timestamp, and put it on the frame. Nit: omit this comma (,).
Sign in to reply to this message.
https://webrtc-codereview.appspot.com/53629004/diff/120001/webrtc/modules/vid... File webrtc/modules/video_coding/main/source/receiver.h (right): https://webrtc-codereview.appspot.com/53629004/diff/120001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver.h:46: // coming, in which case the jitter buffer's wait event is different from On 2015/06/03 22:50:05, wtc wrote: > > Nit: perhaps change "packets coming" to "incoming packets" or "packet arrivals"? Done. https://webrtc-codereview.appspot.com/53629004/diff/120001/webrtc/modules/vid... File webrtc/modules/video_coding/main/source/receiver_unittest.cc (right): https://webrtc-codereview.appspot.com/53629004/diff/120001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:404: int64_t render_time_; On 2015/06/03 22:50:06, wtc wrote: > > Nit: struct members should not have a trailing underscore. > So these should be named as follows: > > int64_t arrive_time; > int64_t render_time; > > See "Struct Data Members" in the Style Guide: > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Variable_Names > > Nit: list data members after methods, including the > constructors and destructor. > > See "Declaration Order" in the Style Guide: > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_... Done. https://webrtc-codereview.appspot.com/53629004/diff/120001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:406: TimestampPair(int64_t arrive_time, int64_t render_time) On 2015/06/03 22:50:06, wtc wrote: > > Note: it turns out we don't need to rename the constructor > arguments. But to avoid confusion, you can rename them > arrive_timestamp and render_timestamp. Done. https://webrtc-codereview.appspot.com/53629004/diff/120001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:408: }; On 2015/06/03 22:50:06, wtc wrote: > > Nit: add a blank line after this line. Done. https://webrtc-codereview.appspot.com/53629004/diff/120001/webrtc/modules/vid... File webrtc/modules/video_coding/main/source/test/stream_generator.h (right): https://webrtc-codereview.appspot.com/53629004/diff/120001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/test/stream_generator.h:38: // |time_ms| denotes timestamp you want to put on the frame, and the unit is On 2015/06/03 22:50:06, wtc wrote: > > Nit: add "the" before "timestamp". Done. https://webrtc-codereview.appspot.com/53629004/diff/120001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/test/stream_generator.h:40: // timestamp, and put it on the frame. On 2015/06/03 22:50:06, wtc wrote: > > Nit: omit this comma (,). Done.
Sign in to reply to this message.
Stefan: this CL is ready for your review now. Patch set 6 LGTM.
Sign in to reply to this message.
https://webrtc-codereview.appspot.com/53629004/diff/160001/webrtc/modules/vid... File webrtc/modules/video_coding/main/source/receiver_unittest.cc (right): https://webrtc-codereview.appspot.com/53629004/diff/160001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:363: // Insert Frame with timestamps_.front().render_time_ into JitterBuffer. Frame -> frame https://webrtc-codereview.appspot.com/53629004/diff/160001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:369: clock_->TimeInMilliseconds()); Why do we have to turn the clock back, can't we just make sure we only progress time until the next packet arrival event? https://webrtc-codereview.appspot.com/53629004/diff/160001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:383: // Insert Frame with timestamps_.front().render_time_ into JitterBuffer frame and comment says render_time_, but not the code. Actually, maybe the comment isn't really needed at all? https://webrtc-codereview.appspot.com/53629004/diff/160001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:524: TEST_F(VCMReceiverTimingTest, FrameForDecoding) { Can you better specify what this actually tests? And I would prefer to remove VCM in front of VCMReceiverTimingTest.
Sign in to reply to this message.
Did some structural change to avoid turning back the clock. https://webrtc-codereview.appspot.com/53629004/diff/160001/webrtc/modules/vid... File webrtc/modules/video_coding/main/source/receiver_unittest.cc (right): https://webrtc-codereview.appspot.com/53629004/diff/160001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:363: // Insert Frame with timestamps_.front().render_time_ into JitterBuffer. On 2015/06/04 13:26:10, Stefan wrote: > Frame -> frame N/A https://webrtc-codereview.appspot.com/53629004/diff/160001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:369: clock_->TimeInMilliseconds()); On 2015/06/04 13:26:10, Stefan wrote: > Why do we have to turn the clock back, can't we just make sure we only progress > time until the next packet arrival event? Moved this part into the clock, so we no longer need to turn back the clock. https://webrtc-codereview.appspot.com/53629004/diff/160001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:383: // Insert Frame with timestamps_.front().render_time_ into JitterBuffer On 2015/06/04 13:26:10, Stefan wrote: > frame > > and comment says render_time_, but not the code. Actually, maybe the comment > isn't really needed at all? N/A https://webrtc-codereview.appspot.com/53629004/diff/160001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:524: TEST_F(VCMReceiverTimingTest, FrameForDecoding) { On 2015/06/04 13:26:10, Stefan wrote: > Can you better specify what this actually tests? > > And I would prefer to remove VCM in front of VCMReceiverTimingTest. Added a brief description on the tests. But I think it's better to keep VCM there, as it is from the class VCMReceiver.
Sign in to reply to this message.
Review comments on patch set 7: I'm sorry I forgot to review this CL until I was about to leave work, so I only did a superficial review. https://webrtc-codereview.appspot.com/53629004/diff/180001/webrtc/modules/vid... File webrtc/modules/video_coding/main/source/receiver_unittest.cc (right): https://webrtc-codereview.appspot.com/53629004/diff/180001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:338: // A simulated clock, when time elapses, it will insert frames into the jitter Nit: please rewrite the first line in one of these ways: A simulated clock that, when time elapses, will insert frames into the jitter or A simulated clock. When time elapses, it will insert frames into the jitter https://webrtc-codereview.appspot.com/53629004/diff/180001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:345: timestamps_(), Nit: this kind of initializer that takes no argument can be omitted. https://webrtc-codereview.appspot.com/53629004/diff/180001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:348: virtual ~SimulatedClockWithFrames(){}; Change virtual to override. Please run "git cl format". It will add a space before {}. Remove the semicolon (;) after {}. https://webrtc-codereview.appspot.com/53629004/diff/180001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:394: int64_t* render_timestamps) { Please declare the method as follows: void SetFrames(const int64_t* arrive_timestamps, const int64_t* render_timestamps, size_t size) Note the use of 'const'. It is conventional to list the size parameter after the array pointer parameter. https://webrtc-codereview.appspot.com/53629004/diff/180001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:511: // returned. Please move this comment before the TEST_F line (line 506).
Sign in to reply to this message.
Hi, Stefan: Can you take a look at this? Wan-Teh is in vacation for this and next week. Qiang https://webrtc-codereview.appspot.com/53629004/diff/180001/webrtc/modules/vid... File webrtc/modules/video_coding/main/source/receiver_unittest.cc (right): https://webrtc-codereview.appspot.com/53629004/diff/180001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:338: // A simulated clock, when time elapses, it will insert frames into the jitter On 2015/06/05 23:51:37, wtc wrote: > > Nit: please rewrite the first line in one of these ways: > > A simulated clock that, when time elapses, will insert frames into the jitter > > or > > A simulated clock. When time elapses, it will insert frames into the jitter Done. https://webrtc-codereview.appspot.com/53629004/diff/180001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:345: timestamps_(), On 2015/06/05 23:51:38, wtc wrote: > > Nit: this kind of initializer that takes no argument can be omitted. Done. https://webrtc-codereview.appspot.com/53629004/diff/180001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:348: virtual ~SimulatedClockWithFrames(){}; On 2015/06/05 23:51:38, wtc wrote: > > Change virtual to override. > > Please run "git cl format". It will add a space before {}. > > Remove the semicolon (;) after {}. Done. https://webrtc-codereview.appspot.com/53629004/diff/180001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:394: int64_t* render_timestamps) { On 2015/06/05 23:51:37, wtc wrote: > > Please declare the method as follows: > > void SetFrames(const int64_t* arrive_timestamps, > const int64_t* render_timestamps, > size_t size) > > Note the use of 'const'. It is conventional to list the size parameter after the > array pointer parameter. Done. https://webrtc-codereview.appspot.com/53629004/diff/180001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:511: // returned. On 2015/06/05 23:51:38, wtc wrote: > > Please move this comment before the TEST_F line (line 506). Done.
Sign in to reply to this message.
Much better! https://webrtc-codereview.appspot.com/53629004/diff/200001/webrtc/modules/vid... File webrtc/modules/video_coding/main/source/receiver.h (right): https://webrtc-codereview.appspot.com/53629004/diff/200001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver.h:52: EventFactory* jitter_buffer_event_factory); I think I would prefer if we instead took EventWrappers as input instead of EventFactory. There's no point in having a factory now that we only have one instance from each factory. https://webrtc-codereview.appspot.com/53629004/diff/200001/webrtc/modules/vid... File webrtc/modules/video_coding/main/source/receiver_unittest.cc (right): https://webrtc-codereview.appspot.com/53629004/diff/200001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:523: arrive_timestamps[i] = (i + 1) * 40 + (i % 10) * ((i % 2) ? 1 : -1); 40 is from the 25 hz? Name that constant. https://webrtc-codereview.appspot.com/53629004/diff/200001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:543: // error, and the test should fail. You think this is worth doing? We could instead rely on the fact that a build bot will kill the test if it times out, and instead assume the code works as it should. That is, not handle a broken case in the test.
Sign in to reply to this message.
https://webrtc-codereview.appspot.com/53629004/diff/200001/webrtc/modules/vid... File webrtc/modules/video_coding/main/source/receiver.h (right): https://webrtc-codereview.appspot.com/53629004/diff/200001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver.h:52: EventFactory* jitter_buffer_event_factory); On 2015/06/09 14:38:09, Stefan wrote: > I think I would prefer if we instead took EventWrappers as input instead of > EventFactory. There's no point in having a factory now that we only have one > instance from each factory. Done. But the side effect is that we need to add a constructor for VCMJitterBuffer. https://webrtc-codereview.appspot.com/53629004/diff/200001/webrtc/modules/vid... File webrtc/modules/video_coding/main/source/receiver_unittest.cc (right): https://webrtc-codereview.appspot.com/53629004/diff/200001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:523: arrive_timestamps[i] = (i + 1) * 40 + (i % 10) * ((i % 2) ? 1 : -1); On 2015/06/09 14:38:09, Stefan wrote: > 40 is from the 25 hz? Name that constant. Done. https://webrtc-codereview.appspot.com/53629004/diff/200001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver_unittest.cc:543: // error, and the test should fail. On 2015/06/09 14:38:09, Stefan wrote: > You think this is worth doing? We could instead rely on the fact that a build > bot will kill the test if it times out, and instead assume the code works as it > should. That is, not handle a broken case in the test. Done.
Sign in to reply to this message.
looks good now, but i hope we can remove the old constructors https://webrtc-codereview.appspot.com/53629004/diff/200001/webrtc/modules/vid... File webrtc/modules/video_coding/main/source/receiver.h (right): https://webrtc-codereview.appspot.com/53629004/diff/200001/webrtc/modules/vid... webrtc/modules/video_coding/main/source/receiver.h:52: EventFactory* jitter_buffer_event_factory); On 2015/06/09 17:12:39, Qiang wrote: > On 2015/06/09 14:38:09, Stefan wrote: > > I think I would prefer if we instead took EventWrappers as input instead of > > EventFactory. There's no point in having a factory now that we only have one > > instance from each factory. > > Done. But the side effect is that we need to add a constructor for > VCMJitterBuffer. But can't we remove the old constructor? 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); Maybe this can be removed now as well? 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())) { Maybe remove this constructor and call the one below from video_receiver.cc?
Sign in to reply to this message.
My concern of signature change is that it may impact other projects. If we need to do this, we have to scan through all projects depending on WebRTC and make sure no one calls the signature we want to remove. For example, Chromium looks good, because it does not instantiate VCMJitterBuffer or VCMReceiver directly. But I'm not sure whether other projects may also depend on WebRTC. Do you have a full list of projects that uses WebRTC? On the other hand, I'm not sure whether it is a worth to do it in this CL. 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/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. 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/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.
Sign in to reply to this message.
lgtm if you prefer removing the old constructor in a separate CL. 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/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. 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/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.
Sign in to reply to this message.
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.
|
