Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
3 years, 11 months ago by johan Modified:
3 years, 11 months ago Reviewers:
philipel, sprang_webrtc CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionUnit test out of band H264 SPS,PPS within RtpStreamReceiver.
This CL introduces a dedicated unit test for webrtc::RtpStreamReceiver.
Focus of this CL is testing RtpStreamReciver::OnReceivedPayloadData().
Dependencies with virtual interfaces are (g)mocked, non-virtual
dependencies are instantiated.
This CL is chained to https://codereview.webrtc.org/2638933002/ .
BUG=webrtc:5948
Review-Url: https://codereview.webrtc.org/2641463002
Cr-Commit-Position: refs/heads/master@{#16240}
Committed: https://chromium.googlesource.com/external/webrtc/+/62d02c328de2e882f580f28a6fbcbd9fd9e48b1e
Patch Set 1 #
Total comments: 14
Patch Set 2 : Merge changes from https://codereview.webrtc.org/2638933002/#ps20001 . #Patch Set 3 : Review feedback (without data verifcation) #Patch Set 4 : Add bitstream expectations and rebase on https://codereview.webrtc.org/2638933002/ . #
Total comments: 1
Patch Set 5 : Rebase on master, use ScopedFieldTrials and resolve nits. #Patch Set 6 : Copyright of new file should be 2017. #Patch Set 7 : Rebase. #Patch Set 8 : Rebase. #
Created: 3 years, 11 months ago
Messages
Total messages: 23 (14 generated)
johan@webrtc.org changed reviewers: + philipel@webrtc.org, sprang@webrtc.org
The CQ bit was checked by johan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2641463002/diff/1/webrtc/video/rtp_stream_recei... File webrtc/video/rtp_stream_receiver.cc (right): https://codereview.webrtc.org/2641463002/diff/1/webrtc/video/rtp_stream_recei... webrtc/video/rtp_stream_receiver.cc:677: << " payload type: " << (unsigned int)payload_type; nit: prefer uint32_t alias to unsigned int https://codereview.webrtc.org/2641463002/diff/1/webrtc/video/rtp_stream_recei... File webrtc/video/rtp_stream_receiver_unittest.cc (right): https://codereview.webrtc.org/2641463002/diff/1/webrtc/video/rtp_stream_recei... webrtc/video/rtp_stream_receiver_unittest.cc:32: const char new_jb_enabled[] = "WebRTC-NewVideoJitterBuffer/Enabled/"; I'd change the name to something like kNewJitterBufferFieldTrialEnabled just to be extra obvious. https://codereview.webrtc.org/2641463002/diff/1/webrtc/video/rtp_stream_recei... webrtc/video/rtp_stream_receiver_unittest.cc:70: // InitFieldTrials has to done before creation of rtp_stream_receiver_. nit: s/has to done/has to be done Or you can move rtp_stream_receiver_.reset(...) to the SetUp() method. https://codereview.webrtc.org/2641463002/diff/1/webrtc/video/rtp_stream_recei... webrtc/video/rtp_stream_receiver_unittest.cc:132: } nit: \n https://codereview.webrtc.org/2641463002/diff/1/webrtc/video/rtp_stream_recei... webrtc/video/rtp_stream_receiver_unittest.cc:153: EXPECT_CALL(mock_on_complete_frame_callback_, DoOnCompleteFrame(testing::_)); add "using ::testing::_;" above https://codereview.webrtc.org/2641463002/diff/1/webrtc/video/rtp_stream_recei... webrtc/video/rtp_stream_receiver_unittest.cc:159: EXPECT_CALL(mock_on_complete_frame_callback_, DoOnCompleteFrame(testing::_)); When do we expect it? If it's a result of rtp_stream_receiver_->OnReceivedPayloadData(), I suggest moving the EXPECT_CALL to just before that. Can we determine what the frame will contain, so that we can use actual data instead of _ ? https://codereview.webrtc.org/2641463002/diff/1/webrtc/video/rtp_stream_recei... webrtc/video/rtp_stream_receiver_unittest.cc:188: EXPECT_CALL(mock_on_complete_frame_callback_, DoOnCompleteFrame(testing::_)); Same here https://codereview.webrtc.org/2641463002/diff/1/webrtc/video/rtp_stream_recei... webrtc/video/rtp_stream_receiver_unittest.cc:189: constexpr int payload_type = 99; kPayloadType
https://codereview.webrtc.org/2641463002/diff/1/webrtc/video/rtp_stream_recei... File webrtc/video/rtp_stream_receiver.cc (right): https://codereview.webrtc.org/2641463002/diff/1/webrtc/video/rtp_stream_recei... webrtc/video/rtp_stream_receiver.cc:677: << " payload type: " << (unsigned int)payload_type; On 2017/01/18 10:05:10, språng wrote: > nit: prefer uint32_t alias to unsigned int This one makes the LOG ostream interpret 'payload_type' as number instead of char. It's a hack anyway. But ok, going with uint32_t. https://codereview.webrtc.org/2641463002/diff/1/webrtc/video/rtp_stream_recei... File webrtc/video/rtp_stream_receiver_unittest.cc (right): https://codereview.webrtc.org/2641463002/diff/1/webrtc/video/rtp_stream_recei... webrtc/video/rtp_stream_receiver_unittest.cc:70: // InitFieldTrials has to done before creation of rtp_stream_receiver_. On 2017/01/18 10:05:10, språng wrote: > nit: s/has to done/has to be done Ack > Or you can move rtp_stream_receiver_.reset(...) to the SetUp() method. Well, reviewers of other modules asked me to do stuff in constructors instead of SetUp() methods, whenever possible. Would be nice to have a common guideline here... https://codereview.webrtc.org/2641463002/diff/1/webrtc/video/rtp_stream_recei... webrtc/video/rtp_stream_receiver_unittest.cc:153: EXPECT_CALL(mock_on_complete_frame_callback_, DoOnCompleteFrame(testing::_)); On 2017/01/18 10:05:10, språng wrote: > add "using ::testing::_;" above Acknowledged. https://codereview.webrtc.org/2641463002/diff/1/webrtc/video/rtp_stream_recei... webrtc/video/rtp_stream_receiver_unittest.cc:159: EXPECT_CALL(mock_on_complete_frame_callback_, DoOnCompleteFrame(testing::_)); On 2017/01/18 10:05:10, språng wrote: > When do we expect it? If it's a result of > rtp_stream_receiver_->OnReceivedPayloadData(), I suggest moving the EXPECT_CALL > to just before that. OK > Can we determine what the frame will contain, so that we can use actual data > instead of _ ? It should be possible to determine the expected Frame's bitstream. I will create a patchset for the other comments first, and figure out how to verify data in this callback afterwards.
PTAL Check for expeted bitstream is in place.
lgtm
lgtm % comments https://codereview.webrtc.org/2641463002/diff/1/webrtc/video/rtp_stream_recei... File webrtc/video/rtp_stream_receiver.cc (right): https://codereview.webrtc.org/2641463002/diff/1/webrtc/video/rtp_stream_recei... webrtc/video/rtp_stream_receiver.cc:677: << " payload type: " << (unsigned int)payload_type; On 2017/01/18 12:38:29, johan wrote: > On 2017/01/18 10:05:10, språng wrote: > > nit: prefer uint32_t alias to unsigned int > > This one makes the LOG ostream interpret 'payload_type' as number instead of > char. It's a hack anyway. But ok, going with uint32_t. Right, so int would work as well. Also, style guide says to use static_cast instead of c style :/ https://codereview.webrtc.org/2641463002/diff/1/webrtc/video/rtp_stream_recei... File webrtc/video/rtp_stream_receiver_unittest.cc (right): https://codereview.webrtc.org/2641463002/diff/1/webrtc/video/rtp_stream_recei... webrtc/video/rtp_stream_receiver_unittest.cc:70: // InitFieldTrials has to done before creation of rtp_stream_receiver_. On 2017/01/18 12:38:29, johan wrote: > On 2017/01/18 10:05:10, språng wrote: > > nit: s/has to done/has to be done > Ack > > > Or you can move rtp_stream_receiver_.reset(...) to the SetUp() method. > Well, reviewers of other modules asked me to do stuff in constructors instead of > SetUp() methods, whenever possible. Would be nice to have a common guideline > here... I don't have super strong preferences, but if the initialization ordering risks breaking stuff I would lean towards a way of making it quite clear. You could also look into using a ScopedFieldTrials as member, then you could bring this all back into the initializer list. https://codereview.webrtc.org/2641463002/diff/60001/webrtc/video/rtp_stream_r... File webrtc/video/rtp_stream_receiver_unittest.cc (right): https://codereview.webrtc.org/2641463002/diff/60001/webrtc/video/rtp_stream_r... webrtc/video/rtp_stream_receiver_unittest.cc:77: << frame->size(); Are you expecting this to happen? I think no one will look at the logs for unit tests, maybe an EXPECT_EQ would be better?
On 2017/01/23 12:57:09, språng wrote: > lgtm % comments > > https://codereview.webrtc.org/2641463002/diff/1/webrtc/video/rtp_stream_recei... > File webrtc/video/rtp_stream_receiver.cc (right): > > https://codereview.webrtc.org/2641463002/diff/1/webrtc/video/rtp_stream_recei... > webrtc/video/rtp_stream_receiver.cc:677: << " payload type: " << (unsigned > int)payload_type; > On 2017/01/18 12:38:29, johan wrote: > > On 2017/01/18 10:05:10, språng wrote: > > > nit: prefer uint32_t alias to unsigned int > > > > This one makes the LOG ostream interpret 'payload_type' as number instead of > > char. It's a hack anyway. But ok, going with uint32_t. > > Right, so int would work as well. > Also, style guide says to use static_cast instead of c style :/ Oups, fixing it. > > https://codereview.webrtc.org/2641463002/diff/1/webrtc/video/rtp_stream_recei... > File webrtc/video/rtp_stream_receiver_unittest.cc (right): > > https://codereview.webrtc.org/2641463002/diff/1/webrtc/video/rtp_stream_recei... > webrtc/video/rtp_stream_receiver_unittest.cc:70: // InitFieldTrials has to done > before creation of rtp_stream_receiver_. > On 2017/01/18 12:38:29, johan wrote: > > On 2017/01/18 10:05:10, språng wrote: > > > nit: s/has to done/has to be done > > Ack > > > > > Or you can move rtp_stream_receiver_.reset(...) to the SetUp() method. > > Well, reviewers of other modules asked me to do stuff in constructors instead > of > > SetUp() methods, whenever possible. Would be nice to have a common guideline > > here... > > I don't have super strong preferences, but if the initialization ordering risks > breaking stuff I would lean towards a way of making it quite clear. > > You could also look into using a ScopedFieldTrials as member, then you could > bring this all back into the initializer list. I like the ScopedFieldTrial. Prevents this test case from having impact on the following ones. > > https://codereview.webrtc.org/2641463002/diff/60001/webrtc/video/rtp_stream_r... > File webrtc/video/rtp_stream_receiver_unittest.cc (right): > > https://codereview.webrtc.org/2641463002/diff/60001/webrtc/video/rtp_stream_r... > webrtc/video/rtp_stream_receiver_unittest.cc:77: << frame->size(); > Are you expecting this to happen? I think no one will look at the logs for unit > tests, maybe an EXPECT_EQ would be better? It is a safe guard for engineers editing the test case. Ack on the EXPECT_EQ.
The CQ bit was checked by johan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was checked by johan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by johan@webrtc.org
The CQ bit was checked by johan@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org, sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/2641463002/#ps140001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1485257494512140,
"parent_rev": "822d2586fbd6af3ee421b32b86e076088283c6b5", "commit_rev":
"62d02c328de2e882f580f28a6fbcbd9fd9e48b1e"}
Message was sent while issue was closed.
Description was changed from ========== Unit test out of band H264 SPS,PPS within RtpStreamReceiver. This CL introduces a dedicated unit test for webrtc::RtpStreamReceiver. Focus of this CL is testing RtpStreamReciver::OnReceivedPayloadData(). Dependencies with virtual interfaces are (g)mocked, non-virtual dependencies are instantiated. This CL is chained to https://codereview.webrtc.org/2638933002/ . BUG=webrtc:5948 ========== to ========== Unit test out of band H264 SPS,PPS within RtpStreamReceiver. This CL introduces a dedicated unit test for webrtc::RtpStreamReceiver. Focus of this CL is testing RtpStreamReciver::OnReceivedPayloadData(). Dependencies with virtual interfaces are (g)mocked, non-virtual dependencies are instantiated. This CL is chained to https://codereview.webrtc.org/2638933002/ . BUG=webrtc:5948 Review-Url: https://codereview.webrtc.org/2641463002 Cr-Commit-Position: refs/heads/master@{#16240} Committed: https://chromium.googlesource.com/external/webrtc/+/62d02c328de2e882f580f28a6... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/external/webrtc/+/62d02c328de2e882f580f28a6... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
