Fix for issue 684766 Old/erroneous frame feedback during tab capture

Fix for issue 684766 Old/erroneous frame feedback during tab capture

VideoCaptureController was not updating the |frame_feedback_id| when new frames 
were arriving. This resulted in frame feedback being reported back to producing 
devices being stuck with the |frame_feedback_id|s that came with the initial 
sharing of the buffers.

The existing unit test did not catch this bug, because it tested the 
feedback mechanism for a sequence of only 2 frames, which happens to be the same
as the number of buffers being shared, resulting in the issue not surfacing.

In addition to a fix, this CL adds a new test case that verifies that the 
frame feedback mechanism works as intended for longer sequences of frames.

BUG=684766
TEST=content_unittests --gtest_filter="VideoCaptureControllerTest.*"

Review-Url: https://codereview.chromium.org/2659653002
Cr-Commit-Position: refs/heads/master@{#447262}
Committed: https://chromium.googlesource.com/chromium/src/+/6ce92793fa6e71801c77b7770c66eeb76799df30

[chfremer, 2017-01-26 19:18:16]

Description was changed from

==========
Fix for issue 684766 Old/erroneous frame feedback during tab capture

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

to

==========
Fix for issue 684766 Old/erroneous frame feedback during tab capture

VideoCaptureController was not updating the |frame_feedback_id| when new frames 
were arriving. This resulted in frame feedback being reported back to producing 
devices being stuck with the |frame_feedback_id|s that came with the initial 
sharing of the buffers.

The existing unit test did not catch this bug, because it tested the 
feedback mechanism for a sequence of only 2 frames, which happens to be the same
as the number of buffers being shared, resulting in the issue not surfacing.

In addition to a fix, this CL adds a new test case that verifies that the 
frame feedback mechanism works as intended for longer sequences of frames.

BUG=684766
TEST=content_unittests --gtest_filter="VideoCaptureControllerTest.*"
==========

[chfremer, 2017-01-26 19:18:23]

The CQ bit was checked by chfremer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 19:19:16]

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

[chfremer, 2017-01-26 19:19:40]

chfremer@chromium.org changed reviewers:
+ mcasas@chromium.org, miu@chromium.org

[chfremer, 2017-01-26 19:19:40]

miu@: PTAL
mcasas@: FYI

[commit-bot: I haz the power, 2017-01-26 20:35:25]

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

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

Dry run: This issue passed the CQ dry run.

[mcasas, 2017-01-30 15:36:34]

https://codereview.chromium.org/2659653002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/2659653002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:394:
buffer_state.SetFrameFeedbackId(buffer.frame_feedback_id());
This is confusing.  buffer.frame_feedback_id() is used to
set Buffer's |frame_feedback_id_| in l.101, so what's the
point in re-pasting this value if the insert(std::make_pair())
fails? More to the point, how could a construction+
insert(std::make_pair()) fail? (Note that BufferState is a POD,
right? std::is_pod<VideoCaptureController::BufferState>::value
verifies).

[chfremer, 2017-01-30 21:16:46]

Patchset #3 (id:40001) has been deleted

[chfremer, 2017-01-30 21:18:27]

PTAL

https://codereview.chromium.org/2659653002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/2659653002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:394:
buffer_state.SetFrameFeedbackId(buffer.frame_feedback_id());
On 2017/01/30 15:36:34, mcasas wrote:
> This is confusing.  buffer.frame_feedback_id() is used to
> set Buffer's |frame_feedback_id_| in l.101, so what's the
> point in re-pasting this value if the insert(std::make_pair())
> fails? More to the point, how could a construction+
> insert(std::make_pair()) fail? (Note that BufferState is a POD,
> right? std::is_pod<VideoCaptureController::BufferState>::value
> verifies).

Please see PatchSet#2 and let me know if it makes things clearer.

[miu, 2017-01-30 23:32:34]

lgtm (I was going to ask if this also works for resurrected frames, but I was
able to walk through the code to confirm it should.)

[mcasas, 2017-01-30 23:54:01]

On 2017/01/30 23:32:34, miu wrote:
> lgtm (I was going to ask if this also works for resurrected frames, but I was
> able to walk through the code to confirm it should.)

lgtm

[chfremer, 2017-01-31 00:30:50]

Thanks for the review!

miu@, since you mentioned the buffer resurrect case, I noticed that this code
path is covered by neither VideoCaptureControllerTest nor
VideoCaptureDeviceClientTest, which is a bit scary. 

Since I feel you shouldn't have to manually read through the code to confirm
that it is working, I slightly modified the test case added in this CL to
exercise ResurrectLastOutputBuffer as well.

I assume your sign-offs are still good even with this change. Please let me know
if you have any objections.

[chfremer, 2017-01-31 00:30:59]

The CQ bit was checked by chfremer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 00:31:36]

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

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

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

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

Dry run: This issue passed the CQ dry run.

[chfremer, 2017-01-31 17:35:20]

The CQ bit was checked by chfremer@chromium.org

[chfremer, 2017-01-31 17:35:20]

The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org,
miu@chromium.org
Link to the patchset: https://codereview.chromium.org/2659653002/#ps60001
(title: "Exercise ResurrectLastOutputBuffer() and apply git cl format")

[commit-bot: I haz the power, 2017-01-31 17:35:40]

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

[commit-bot: I haz the power, 2017-01-31 17:42:01]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485884120413000,
"parent_rev": "2445daf09f96a27c16e93f20c40eb30acb4a6fa9", "commit_rev":
"6ce92793fa6e71801c77b7770c66eeb76799df30"}

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

Description was changed from

==========
Fix for issue 684766 Old/erroneous frame feedback during tab capture

VideoCaptureController was not updating the |frame_feedback_id| when new frames 
were arriving. This resulted in frame feedback being reported back to producing 
devices being stuck with the |frame_feedback_id|s that came with the initial 
sharing of the buffers.

The existing unit test did not catch this bug, because it tested the 
feedback mechanism for a sequence of only 2 frames, which happens to be the same
as the number of buffers being shared, resulting in the issue not surfacing.

In addition to a fix, this CL adds a new test case that verifies that the 
frame feedback mechanism works as intended for longer sequences of frames.

BUG=684766
TEST=content_unittests --gtest_filter="VideoCaptureControllerTest.*"
==========

to

==========
Fix for issue 684766 Old/erroneous frame feedback during tab capture

VideoCaptureController was not updating the |frame_feedback_id| when new frames 
were arriving. This resulted in frame feedback being reported back to producing 
devices being stuck with the |frame_feedback_id|s that came with the initial 
sharing of the buffers.

The existing unit test did not catch this bug, because it tested the 
feedback mechanism for a sequence of only 2 frames, which happens to be the same
as the number of buffers being shared, resulting in the issue not surfacing.

In addition to a fix, this CL adds a new test case that verifies that the 
frame feedback mechanism works as intended for longer sequences of frames.

BUG=684766
TEST=content_unittests --gtest_filter="VideoCaptureControllerTest.*"

Review-Url: https://codereview.chromium.org/2659653002
Cr-Commit-Position: refs/heads/master@{#447262}
Committed:
https://chromium.googlesource.com/chromium/src/+/6ce92793fa6e71801c77b7770c66...
==========

[commit-bot: I haz the power, 2017-01-31 17:42:35]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/6ce92793fa6e71801c77b7770c66...