Configured VCMTiming with sender defining delay times.

Configured VCMTiming with sender defining delay times.

BUG=webrtc:7590
Review-Url: https://codereview.webrtc.org/2870823003
Cr-Commit-Position: refs/heads/master@{#18086}
Committed: https://chromium.googlesource.com/external/webrtc/+/b2a318bb8e699324f5953aa20d5d24b34e1a00a5

[gnish1, 2017-05-09 13:53:37]

Description was changed from

==========
abc


Set the min/max playout delay

BUG=
==========

to

==========
Configured VCMTiming with sender defining delay times.

BUG=webrtc:7590
==========

[gnish1, 2017-05-09 13:53:37]

gnish@webrtc.org changed reviewers:
+ philipel@webrtc.org

[philipel, 2017-05-09 14:34:47]

Looking good, a few comments though :)

https://codereview.webrtc.org/2870823003/diff/1/webrtc/modules/video_coding/f...
File webrtc/modules/video_coding/frame_buffer2.cc (right):

https://codereview.webrtc.org/2870823003/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_buffer2.cc:219: (minimal_playout_delay_ms >=
0) ? minimal_playout_delay_ms : -1);
What happens if you set the min/max playout delay to -1? Are those valid values?

https://codereview.webrtc.org/2870823003/diff/1/webrtc/modules/video_coding/f...
File webrtc/modules/video_coding/frame_buffer2.h (right):

https://codereview.webrtc.org/2870823003/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_buffer2.h:124: //
Please add a comment about what this function does.

https://codereview.webrtc.org/2870823003/diff/1/webrtc/modules/video_coding/f...
File webrtc/modules/video_coding/frame_buffer2_unittest.cc (right):

https://codereview.webrtc.org/2870823003/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_buffer2_unittest.cc:264: int
minimal_playout_delay_ms = -213;
Constants are defined like this "const kMinimalPlayoutDelayMs = -213"

Also, please don't use invalid values. Often people read the unittests to
understand what a particular class is suppose to do, and this might confuse
them.

https://codereview.webrtc.org/2870823003/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_buffer2_unittest.cc:265: int
maximal_playout_delay_ms = -10000;
Also a constant :)

https://codereview.webrtc.org/2870823003/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_buffer2_unittest.cc:270:
EXPECT_EQ(timing_.min_playout_delay(), -1);
In general we do EXPECT_EQ(<the expected value>, <value to test>) so when an
error occur it generates a nice error message, we should change this to
EXPECT_EQ(-1, timing_.min_playout_delay());

https://codereview.webrtc.org/2870823003/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_buffer2_unittest.cc:270:
EXPECT_EQ(timing_.min_playout_delay(), -1);
Shouldn't these values be equal to the constants defined on line 264/265?

[gnish, 2017-05-09 15:28:19]

gnish@google.com changed reviewers:
+ gnish@google.com

[philipel, 2017-05-09 16:12:59]

Just some minor style comments

https://codereview.webrtc.org/2870823003/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_buffer2.cc (right):

https://codereview.webrtc.org/2870823003/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.cc:234: UpdatePlayoutDelays(*frame);
I just realized this now, but I think we should only update the playout delay if
we actually insert the frame. All you have to do is to move this call to line
300.

https://codereview.webrtc.org/2870823003/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_buffer2.h (right):

https://codereview.webrtc.org/2870823003/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.h:124: // depending on the frame
nit: End sentence with a dot "." :)

https://codereview.webrtc.org/2870823003/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_buffer2_unittest.cc (right):

https://codereview.webrtc.org/2870823003/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2_unittest.cc:264: const int
minimalPlayoutDelayMs = 200;
minimalPlayoutDelayMs -->  kMinimalPlayoutDelayMs

Same for the one below

[gnish, 2017-05-09 16:24:13]

version 3

https://codereview.webrtc.org/2870823003/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_buffer2.cc (right):

https://codereview.webrtc.org/2870823003/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.cc:234: UpdatePlayoutDelays(*frame);
On 2017/05/09 16:12:58, philipel wrote:
> I just realized this now, but I think we should only update the playout delay
if
> we actually insert the frame. All you have to do is to move this call to line
> 300.

Done.

https://codereview.webrtc.org/2870823003/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_buffer2.h (right):

https://codereview.webrtc.org/2870823003/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.h:124: // depending on the frame
On 2017/05/09 16:12:59, philipel wrote:
> nit: End sentence with a dot "." :)

Done.

https://codereview.webrtc.org/2870823003/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_buffer2_unittest.cc (right):

https://codereview.webrtc.org/2870823003/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2_unittest.cc:264: const int
minimalPlayoutDelayMs = 200;
On 2017/05/09 16:12:59, philipel wrote:
> minimalPlayoutDelayMs -->  kMinimalPlayoutDelayMs
> 
> Same for the one below

Done.

[philipel, 2017-05-10 08:07:20]

philipel@webrtc.org changed reviewers:
+ stefan@webrtc.org
- gnish@google.com

[philipel, 2017-05-10 08:07:21]

lgtm

Stefan, PTAL

[philipel, 2017-05-10 11:24:41]

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

[philipel, 2017-05-10 11:24:41]

Eric, PTAL at this super awesome CL

[sprang_webrtc, 2017-05-10 13:58:10]

https://codereview.webrtc.org/2870823003/diff/30001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/encoded_frame.h (right):

https://codereview.webrtc.org/2870823003/diff/30001/webrtc/modules/video_codi...
webrtc/modules/video_coding/encoded_frame.h:54: 
Do you really need this? Looks like it's only used in one test.

https://codereview.webrtc.org/2870823003/diff/30001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_buffer2.cc (right):

https://codereview.webrtc.org/2870823003/diff/30001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.cc:45:
inter_frame_delay_(clock_->TimeInMilliseconds()),
While you're here, can you update the initializer list to include
last_decoded_frame_timestamp_?

https://codereview.webrtc.org/2870823003/diff/30001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.cc:213: int maximal_playout_delay_ms =
-1;
No need to initialize these temporaries. In fact, maybe change them to something
like:

PlayoutDelay playout_delay = frame.EncodedImage().playout_delay_;
Then use that struct instead below.

[gnish, 2017-05-10 15:24:54]

gnish@google.com changed reviewers:
+ gnish@google.com

[sprang_webrtc, 2017-05-10 15:29:47]

lgtm
Btw, it's nice if you reply (even if just with a "done" click) to each comment
just so it's easier to keep track of what's been addressed or not. :)

[gnish, 2017-05-10 15:32:51]

On 2017/05/10 15:29:47, sprang_webrtc wrote:
> lgtm
> Btw, it's nice if you reply (even if just with a "done" click) to each comment
> just so it's easier to keep track of what's been addressed or not. :)

Sure,will take that into account.

[gnish, 2017-05-10 15:45:50]

The CQ bit was checked by gnish@google.com

[gnish, 2017-05-10 15:45:50]

The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2870823003/#ps50001 (title:
"empty")

[commit-bot: I haz the power, 2017-05-10 15:45:57]

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-05-10 16:21:33]

CQ is committing da patch.

Bot data: {"patchset_id": 50001, "attempt_start_ts": 1494431150037650,
"parent_rev": "2d9d21f32831b7e5db2ecbc8cc3504d3599a9cb6", "commit_rev":
"b2a318bb8e699324f5953aa20d5d24b34e1a00a5"}

[commit-bot: I haz the power, 2017-05-10 16:21:37]

Description was changed from

==========
Configured VCMTiming with sender defining delay times.

BUG=webrtc:7590
==========

to

==========
Configured VCMTiming with sender defining delay times.

BUG=webrtc:7590

Review-Url: https://codereview.webrtc.org/2870823003
Cr-Commit-Position: refs/heads/master@{#18086}
Committed:
https://chromium.googlesource.com/external/webrtc/+/b2a318bb8e699324f5953aa20...
==========

[commit-bot: I haz the power, 2017-05-10 16:21:38]

Committed patchset #4 (id:50001) as
https://chromium.googlesource.com/external/webrtc/+/b2a318bb8e699324f5953aa20...