Unit test out of band H264 SPS,PPS within RtpStreamReceiver.

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/+/62d02c328de2e882f580f28a6fbcbd9fd9e48b1e

[johan, 2017-01-17 16:28:17]

johan@webrtc.org changed reviewers:
+ philipel@webrtc.org, sprang@webrtc.org

[johan, 2017-01-17 16:28:53]

The CQ bit was checked by johan@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 16:28:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-17 17:33:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-17 17:33:03]

Dry run: This issue passed the CQ dry run.

[sprang_webrtc, 2017-01-18 10:05:11]

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

[johan, 2017-01-18 12:38:30]

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.

[johan, 2017-01-19 15:44:43]

PTAL

Check for expeted bitstream is in place.

[philipel, 2017-01-20 12:39:15]

lgtm

[sprang_webrtc, 2017-01-23 12:57:09]

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?

[johan, 2017-01-23 20:34:38]

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.

[johan, 2017-01-23 20:35:44]

The CQ bit was checked by johan@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 20:35:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[johan, 2017-01-24 10:42:54]

The CQ bit was checked by johan@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 10:43:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[johan, 2017-01-24 10:43:47]

The CQ bit was unchecked by johan@webrtc.org

[johan, 2017-01-24 11:31:34]

The CQ bit was checked by johan@webrtc.org

[johan, 2017-01-24 11:31:34]

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.")

[commit-bot: I haz the power, 2017-01-24 11:31:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-24 12:38:28]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1485257494512140,
"parent_rev": "822d2586fbd6af3ee421b32b86e076088283c6b5", "commit_rev":
"62d02c328de2e882f580f28a6fbcbd9fd9e48b1e"}

[commit-bot: I haz the power, 2017-01-24 12:38:30]

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...
==========

[commit-bot: I haz the power, 2017-01-24 12:38:31]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/external/webrtc/+/62d02c328de2e882f580f28a6...