Cast: Make vp8 3-buffer mode work

Cast: Make vp8 3-buffer mode work

Cast was originally intended to tell vp8 to only reference buffers which have been ACKed.
However, that mode was buggy and disabled. This CL fixes it again. The three-buffer mode
lets the receiver drop frames and continue, which should be quite useful if we want lower
latency.

BUG=351596
Committed: https://crrev.com/b836b7126ecb26d5c2d6eb7db591ff7c1167af71
Cr-Commit-Position: refs/heads/master@{#293243}

[Alpha Left Google, 2014-08-25 23:23:45]

https://codereview.chromium.org/339743002/diff/20001/chrome/renderer/media/ca...
File chrome/renderer/media/cast_rtp_stream.cc (right):

https://codereview.chromium.org/339743002/diff/20001/chrome/renderer/media/ca...
chrome/renderer/media/cast_rtp_stream.cc:211:
config->max_number_of_video_buffers_used = 3;
Two issues:

1. We do not want to turn on this mode by default. Maybe this should by drive by
extension. For example this can be a codec specific parameter, see here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

2. This should be a boolean instead of int.

https://codereview.chromium.org/339743002/diff/20001/media/cast/net/rtp/frame...
File media/cast/net/rtp/framer.cc (right):

https://codereview.chromium.org/339743002/diff/20001/media/cast/net/rtp/frame...
media/cast/net/rtp/framer.cc:99: 
nit: please remove empty line.

https://codereview.chromium.org/339743002/diff/20001/media/cast/net/rtp/framer.h
File media/cast/net/rtp/framer.h (right):

https://codereview.chromium.org/339743002/diff/20001/media/cast/net/rtp/frame...
media/cast/net/rtp/framer.h:52: // TODO(hubbe): Move this elsewhere.
This comment doesn't add much value. Please give more details.

https://codereview.chromium.org/339743002/diff/20001/media/cast/receiver/fram...
File media/cast/receiver/frame_receiver.cc (right):

https://codereview.chromium.org/339743002/diff/20001/media/cast/receiver/fram...
media/cast/receiver/frame_receiver.cc:239:
framer_.AckFrame(encoded_frame->frame_id);
Please add a comment to explain why frame is ACKed here instead of inside
framer.

https://codereview.chromium.org/339743002/diff/20001/media/cast/sender/vp8_en...
File media/cast/sender/vp8_encoder.cc (right):

https://codereview.chromium.org/339743002/diff/20001/media/cast/sender/vp8_en...
media/cast/sender/vp8_encoder.cc:232: uint32 latest_frame_to_reference =
last_encoded_frame_id_ - 512;
Why 512? Seems like this should be a constant and documented.

https://codereview.chromium.org/339743002/diff/20001/media/cast/sender/vp8_en...
media/cast/sender/vp8_encoder.cc:238: if (buffer_state_[i].state ==
kBufferAcked) {
This code reads to me that:

If buffer_state_[kAltRefBuffer].state == kBufferAcked then do not reference alt
ref frame.
If buffer_state_[kGoldenBuffer].state == kBufferAcked then do not reference
golden frame.
If buffer_state_[kLastBuffer].state == kBufferAcked then do not reference last
frame.

Is this correct? This seems to contradict the above comment that says we only
reference ACKed frame.

https://codereview.chromium.org/339743002/diff/20001/media/cast/sender/vp8_en...
media/cast/sender/vp8_encoder.cc:257: if (latest_frame_to_reference ==
last_encoded_frame_id_ - 512) {
Why 512? Please add a comment.

https://codereview.chromium.org/339743002/diff/20001/media/cast/sender/vp8_en...
media/cast/sender/vp8_encoder.cc:272: // 2. If there is a buffer other than the
oldest buffer
Please document the rationale for these conditions.

https://codereview.chromium.org/339743002/diff/20001/media/cast/sender/vp8_en...
media/cast/sender/vp8_encoder.cc:383: if (IsOlderFrameId(last_acked_frame_id_,
frame_id)) {
Why is this inside the for loop? I don't understand what does this if statement
do.

[hubbe, 2014-08-27 04:14:03]

I'm going to run some more tests on this, but I've uploaded the changes for now.

https://codereview.chromium.org/339743002/diff/20001/chrome/renderer/media/ca...
File chrome/renderer/media/cast_rtp_stream.cc (right):

https://codereview.chromium.org/339743002/diff/20001/chrome/renderer/media/ca...
chrome/renderer/media/cast_rtp_stream.cc:211:
config->max_number_of_video_buffers_used = 3;
1. Done
2. Agreed, but perhaps in a separate CL.

On 2014/08/25 23:23:44, Alpha wrote:
> Two issues:
> 
> 1. We do not want to turn on this mode by default. Maybe this should by drive
by
> extension. For example this can be a codec specific parameter, see here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...
> 
> 2. This should be a boolean instead of int.

https://codereview.chromium.org/339743002/diff/20001/media/cast/net/rtp/frame...
File media/cast/net/rtp/framer.cc (right):

https://codereview.chromium.org/339743002/diff/20001/media/cast/net/rtp/frame...
media/cast/net/rtp/framer.cc:99: 
On 2014/08/25 23:23:44, Alpha wrote:
> nit: please remove empty line.

Done.

https://codereview.chromium.org/339743002/diff/20001/media/cast/net/rtp/framer.h
File media/cast/net/rtp/framer.h (right):

https://codereview.chromium.org/339743002/diff/20001/media/cast/net/rtp/frame...
media/cast/net/rtp/framer.h:52: // TODO(hubbe): Move this elsewhere.
On 2014/08/25 23:23:44, Alpha wrote:
> This comment doesn't add much value. Please give more details.

Done.

https://codereview.chromium.org/339743002/diff/20001/media/cast/receiver/fram...
File media/cast/receiver/frame_receiver.cc (right):

https://codereview.chromium.org/339743002/diff/20001/media/cast/receiver/fram...
media/cast/receiver/frame_receiver.cc:239:
framer_.AckFrame(encoded_frame->frame_id);
On 2014/08/25 23:23:44, Alpha wrote:
> Please add a comment to explain why frame is ACKed here instead of inside
> framer.

Done.

https://codereview.chromium.org/339743002/diff/20001/media/cast/sender/vp8_en...
File media/cast/sender/vp8_encoder.cc (right):

https://codereview.chromium.org/339743002/diff/20001/media/cast/sender/vp8_en...
media/cast/sender/vp8_encoder.cc:232: uint32 latest_frame_to_reference =
last_encoded_frame_id_ - 512;
On 2014/08/25 23:23:44, Alpha wrote:
> Why 512? Seems like this should be a constant and documented.

Any number larger than 256 will work.

https://codereview.chromium.org/339743002/diff/20001/media/cast/sender/vp8_en...
media/cast/sender/vp8_encoder.cc:238: if (buffer_state_[i].state ==
kBufferAcked) {
On 2014/08/25 23:23:44, Alpha wrote:
> This code reads to me that:
> 
> If buffer_state_[kAltRefBuffer].state == kBufferAcked then do not reference
alt
> ref frame.
> If buffer_state_[kGoldenBuffer].state == kBufferAcked then do not reference
> golden frame.
> If buffer_state_[kLastBuffer].state == kBufferAcked then do not reference last
> frame.
> 
> Is this correct? This seems to contradict the above comment that says we only
> reference ACKed frame.

Looks like I misplaced an else somewhere...

https://codereview.chromium.org/339743002/diff/20001/media/cast/sender/vp8_en...
media/cast/sender/vp8_encoder.cc:257: if (latest_frame_to_reference ==
last_encoded_frame_id_ - 512) {
On 2014/08/25 23:23:44, Alpha wrote:
> Why 512? Please add a comment.

It just needs to be the same as the 512 above, made a constant.

https://codereview.chromium.org/339743002/diff/20001/media/cast/sender/vp8_en...
media/cast/sender/vp8_encoder.cc:272: // 2. If there is a buffer other than the
oldest buffer
On 2014/08/25 23:23:44, Alpha wrote:
> Please document the rationale for these conditions.

Done.

https://codereview.chromium.org/339743002/diff/20001/media/cast/sender/vp8_en...
media/cast/sender/vp8_encoder.cc:383: if (IsOlderFrameId(last_acked_frame_id_,
frame_id)) {
On 2014/08/25 23:23:44, Alpha wrote:
> Why is this inside the for loop? I don't understand what does this if
statement
> do.

Messed up merge, fixed.

[Alpha Left Google, 2014-08-28 21:39:33]

https://codereview.chromium.org/339743002/diff/40001/media/cast/sender/vp8_en...
File media/cast/sender/vp8_encoder.cc (right):

https://codereview.chromium.org/339743002/diff/40001/media/cast/sender/vp8_en...
media/cast/sender/vp8_encoder.cc:239: // Probably only referencing the last one
would be enough.
This comment is invalid because we only reference acked frames.

[hubbe, 2014-08-28 21:55:28]

https://codereview.chromium.org/339743002/diff/40001/media/cast/sender/vp8_en...
File media/cast/sender/vp8_encoder.cc (right):

https://codereview.chromium.org/339743002/diff/40001/media/cast/sender/vp8_en...
media/cast/sender/vp8_encoder.cc:239: // Probably only referencing the last one
would be enough.
On 2014/08/28 21:39:33, Alpha wrote:
> This comment is invalid because we only reference acked frames.

The comment says that referencing the last acked frame would be enough.
Referencing ALL acked frames probably doesn't help us very much.

[Alpha Left Google, 2014-08-28 21:59:10]

https://codereview.chromium.org/339743002/diff/40001/media/cast/sender/vp8_en...
File media/cast/sender/vp8_encoder.cc (right):

https://codereview.chromium.org/339743002/diff/40001/media/cast/sender/vp8_en...
media/cast/sender/vp8_encoder.cc:239: // Probably only referencing the last one
would be enough.
On 2014/08/28 21:55:28, hubbe wrote:
> On 2014/08/28 21:39:33, Alpha wrote:
> > This comment is invalid because we only reference acked frames.
> 
> The comment says that referencing the last acked frame would be enough.
> Referencing ALL acked frames probably doesn't help us very much.

Right. The code does allow frame to not reference anything. But the following.

But the comment "Probably only referencing the last one would be enough" is
confusing because this is not what this code do.

Also the next comment saying we allow referencing the last frame is not true. Or
am I missing something?

[hubbe, 2014-09-02 18:24:22]

https://codereview.chromium.org/339743002/diff/40001/media/cast/sender/vp8_en...
File media/cast/sender/vp8_encoder.cc (right):

https://codereview.chromium.org/339743002/diff/40001/media/cast/sender/vp8_en...
media/cast/sender/vp8_encoder.cc:239: // Probably only referencing the last one
would be enough.
On 2014/08/28 21:59:10, Alpha wrote:
> On 2014/08/28 21:55:28, hubbe wrote:
> > On 2014/08/28 21:39:33, Alpha wrote:
> > > This comment is invalid because we only reference acked frames.
> > 
> > The comment says that referencing the last acked frame would be enough.
> > Referencing ALL acked frames probably doesn't help us very much.
> 
> Right. The code does allow frame to not reference anything. But the following.
> 
> But the comment "Probably only referencing the last one would be enough" is
> confusing because this is not what this code do.
> 
> Also the next comment saying we allow referencing the last frame is not true.
Or
> am I missing something?

Comment updated.

[Alpha Left Google, 2014-09-02 18:27:10]

Thanks. LGTM.

[miu, 2014-09-02 19:22:03]

miu@chromium.org changed reviewers:
+ miu@chromium.org

[miu, 2014-09-02 19:22:04]

lgtm

Drive-by comments...

https://codereview.chromium.org/339743002/diff/60001/media/cast/sender/video_...
File media/cast/sender/video_encoder_impl_unittest.cc (right):

https://codereview.chromium.org/339743002/diff/60001/media/cast/sender/video_...
media/cast/sender/video_encoder_impl_unittest.cc:39: if (expected_frame_id_ !=
expected_last_referenced_frame_id_) {
If this assumption is no longer true, should we be introducing a new
EncodedFrame enum?

https://codereview.chromium.org/339743002/diff/60001/media/cast/sender/vp8_en...
File media/cast/sender/vp8_encoder.cc (right):

https://codereview.chromium.org/339743002/diff/60001/media/cast/sender/vp8_en...
media/cast/sender/vp8_encoder.cc:285: //    but no more than 3 times in a row.
Why 3?

[hubbe, 2014-09-02 21:39:48]

https://codereview.chromium.org/339743002/diff/60001/media/cast/sender/video_...
File media/cast/sender/video_encoder_impl_unittest.cc (right):

https://codereview.chromium.org/339743002/diff/60001/media/cast/sender/video_...
media/cast/sender/video_encoder_impl_unittest.cc:39: if (expected_frame_id_ !=
expected_last_referenced_frame_id_) {
On 2014/09/02 19:22:03, miu wrote:
> If this assumption is no longer true, should we be introducing a new
> EncodedFrame enum?

I think I'm going to try to make the dependency into a bool again. (In a
separate CL.)

https://codereview.chromium.org/339743002/diff/60001/media/cast/sender/vp8_en...
File media/cast/sender/vp8_encoder.cc (right):

https://codereview.chromium.org/339743002/diff/60001/media/cast/sender/vp8_en...
media/cast/sender/vp8_encoder.cc:285: //    but no more than 3 times in a row.
On 2014/09/02 19:22:03, miu wrote:
> Why 3?

Needs to be tuned....

[hubbe, 2014-09-03 21:34:06]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2014-09-03 21:34:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hubbe@chromium.org/339743002/120001

[commit-bot: I haz the power, 2014-09-04 00:52:35]

Committed patchset #7 (id:120001) as 0d923fea0eb819ecd6d41768d0f38ce65240f7c1

[commit-bot: I haz the power, 2014-09-10 03:29:16]

Patchset 7 (id:??) landed as
https://crrev.com/b836b7126ecb26d5c2d6eb7db591ff7c1167af71
Cr-Commit-Position: refs/heads/master@{#293243}