TabCapture: enforce active refresh if there is un-sampled compositor update

TabCapture: enforce active refresh if there is un-sampled compositor update

During tab capture, if there is no mouse movement and no local preview,
the passive refresh request will always succeed to refresh with the
last cached frame, while the compositor update will most probably be
dropped.
To capture the updated content in time, when there is un-sampled
compositor update event, we'll fail the next passive refresh request to
enforce an active refresh request.

BUG=704257
Review-Url: https://codereview.chromium.org/2770923003
Cr-Commit-Position: refs/heads/master@{#460455}
Committed: https://chromium.googlesource.com/chromium/src/+/2d7baa9276da1713abbbb01cf8b28065292302ff

[braveyao, 2017-03-22 22:48:17]

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

[commit-bot: I haz the power, 2017-03-22 22:48:53]

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

[braveyao, 2017-03-22 23:02:51]

Description was changed from

==========
TabCapture: enforce active refresh if there is un-sampled compositor update

During tab capture, if there is no mouse movement and less compositor update
(maybe due to the missing of local preview), the passive refresh request will
always succeed to refresh with the last cached frame, while the compositor
update will most probably be dropped.
To capture the updated content in time, when there is un-sampled compositor
update event, we'll fail the next passive refresh request to enforce a active
refresh request.

BUG=704257
==========

to

==========
TabCapture: enforce active refresh if there is un-sampled compositor update

During tab capture, if there is no mouse movement and no local preview,
the passive refresh request will always succeed to refresh with the
last cached frame, while the compositor update will most probably be
dropped.
To capture the updated content in time, when there is un-sampled
compositor update event, we'll fail the next passive refresh request to
enforce an active refresh request.

BUG=704257
==========

[commit-bot: I haz the power, 2017-03-23 00:51:05]

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

[commit-bot: I haz the power, 2017-03-23 00:51:05]

Dry run: This issue passed the CQ dry run.

[braveyao, 2017-03-23 00:53:01]

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

[braveyao, 2017-03-23 00:53:01]

Hi miu@, please take a look!

[miu, 2017-03-23 21:59:19]

I missed this code review in my e-mail scan today, and came up with a similar
solution (see https://bugs.chromium.org/p/chromium/issues/detail?id=704257#c4).

I was comparing the two, and there are a few things you'll want to account for:

1. The dirty flag should basically be set for all events except the two refresh
ones (i.e., mouse movement is also dirty because the mouse cursor has to be
drawn in a new location in the frame).

2. The dirty flag should be set regardless of whether the content will be
sampled. When content is not sampled, it is still dirty, and so "passive
refreshes" should not be allowed.

3. If we know a "passive refresh" will be rejected, it would be better to not
call into the sampling logic, which modifies its own internal state under the
assumption the oracle is going to go with the recommendation. Instead, we should
just reject immediately without consulting the |smoothing_sampler_|.

4. The caller of ObserveEventAndDecideCapture() might still choose not to
capture. In this case, the dirty flag should remain set. Therefore, the flag
should only be cleared once RecordCapture() is called later.

5. Later in the pipeline, if a frame capture fails to execute, we should set the
dirty flag again: The capture might have been needed to provide the latest
content to the consumer.

You can see how I dealt with all of the above in my patch. IMHO, to save time,
I'd recommend just using my patch with your unit test. :) Of course, please
scrutinize it in case there's something I got wrong.

[braveyao, 2017-03-24 16:56:23]

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

[commit-bot: I haz the power, 2017-03-24 16:56:57]

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-03-24 18:30:05]

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

[commit-bot: I haz the power, 2017-03-24 18:30:06]

Dry run: This issue passed the CQ dry run.

[braveyao, 2017-03-24 18:31:39]

Thanks so much for the patch! Mine was created in a rush...

My two cents:
1. I don't think we need to set dirty flag for mouse movement. Mouse moving will
generate countless events,which are really hard to be ignored. Check how it
recovered this bug. Also cursor is rendered instead of captured.
2. I don't think |last_event_causing_capture_| is necessary. Since we return
false to all PassiveRefresh now if dirty flag is true, there is no chance
RecordCapture will be called for PassiveRefresh. 

Then the logic can be much simpler.

PS: I missed your point 5. Thanks for the explanation. But I did realize point
2&4 during that night, but having no time to update it :)

PTAL

[miu, 2017-03-24 21:14:00]

On 2017/03/24 18:31:39, braveyao wrote:
> Thanks so much for the patch! Mine was created in a rush...
> 
> My two cents:
> 1. I don't think we need to set dirty flag for mouse movement. Mouse moving
will
> generate countless events,which are really hard to be ignored. Check how it
> recovered this bug. Also cursor is rendered instead of captured.

While mouse movement will generate a lot of events, it's the last mouse event
before the user stops moving that is the most important. Otherwise, the cursor
won't be shown pointing at the right thing (at the receiver).

Another real-world counter-example: When the mouse leaves the browser window,
the mouse cursor should be hidden. That requires capturing a new frame (i.e., a
video frame without a mouse cursor rendered on it). Even if there were several
mouse move events happening, it's the very last one as the mouse leaves the
window that is critical to making sure the consumer has the correct video.

> 2. I don't think |last_event_causing_capture_| is necessary. Since we return
> false to all PassiveRefresh now if dirty flag is true, there is no chance
> RecordCapture will be called for PassiveRefresh.

Ah yes, good point. That's fine then, but we should leave a comment in
RecordCapture() when the dirty flag is set to false.

[miu, 2017-03-24 21:17:28]

https://codereview.chromium.org/2770923003/diff/20001/media/capture/content/v...
File media/capture/content/video_capture_oracle.cc (right):

https://codereview.chromium.org/2770923003/diff/20001/media/capture/content/v...
media/capture/content/video_capture_oracle.cc:235: if
(last_event_causing_capture_ != kPassiveRefreshRequest)
Per discussion, this shouldn't be necessary. So, just:

  source_is_dirty_ = false;

https://codereview.chromium.org/2770923003/diff/20001/media/capture/content/v...
File media/capture/content/video_capture_oracle.h (right):

https://codereview.chromium.org/2770923003/diff/20001/media/capture/content/v...
media/capture/content/video_capture_oracle.h:156: Event
last_event_causing_capture_;
Per discussion, we don't need |last_event_causing_capture_| anymore (since a
RecordCapture() call always means the dirty flag can be cleared).

[miu, 2017-03-24 21:19:39]

https://codereview.chromium.org/2770923003/diff/20001/media/capture/content/v...
File media/capture/content/video_capture_oracle_unittest.cc (right):

https://codereview.chromium.org/2770923003/diff/20001/media/capture/content/v...
media/capture/content/video_capture_oracle_unittest.cc:354: // refresh request
to force an active refresh.
nit: s/refresh request to force/refresh request, forcing/

[braveyao, 2017-03-24 21:41:05]

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

[commit-bot: I haz the power, 2017-03-24 21:41:50]

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

[braveyao, 2017-03-24 23:12:27]

Thanks for taking my idea. All addressed! PTAL

Not important: my understanding to the MouseCursorUpdate is one of these
continuous events will definitely be sampled(i.e. every 200ms for 5fps) and will
cause a hard capture with a newly rendered cursor. They will not be omitted by
PassiveRefreshRequest if there is no other content change. So we don't need to
set dirty flag for it.
But strictly, mouse moving does mean source is dirty. So nothing wrong to set
the dirty flag too. :)

https://codereview.chromium.org/2770923003/diff/20001/media/capture/content/v...
File media/capture/content/video_capture_oracle.cc (right):

https://codereview.chromium.org/2770923003/diff/20001/media/capture/content/v...
media/capture/content/video_capture_oracle.cc:235: if
(last_event_causing_capture_ != kPassiveRefreshRequest)
On 2017/03/24 21:17:28, miu wrote:
> Per discussion, this shouldn't be necessary. So, just:
> 
>   source_is_dirty_ = false;

Done.

https://codereview.chromium.org/2770923003/diff/20001/media/capture/content/v...
File media/capture/content/video_capture_oracle.h (right):

https://codereview.chromium.org/2770923003/diff/20001/media/capture/content/v...
media/capture/content/video_capture_oracle.h:156: Event
last_event_causing_capture_;
On 2017/03/24 21:17:28, miu wrote:
> Per discussion, we don't need |last_event_causing_capture_| anymore (since a
> RecordCapture() call always means the dirty flag can be cleared).

Done.

https://codereview.chromium.org/2770923003/diff/20001/media/capture/content/v...
File media/capture/content/video_capture_oracle_unittest.cc (right):

https://codereview.chromium.org/2770923003/diff/20001/media/capture/content/v...
media/capture/content/video_capture_oracle_unittest.cc:354: // refresh request
to force an active refresh.
On 2017/03/24 21:19:39, miu wrote:
> nit: s/refresh request to force/refresh request, forcing/

Done.

[commit-bot: I haz the power, 2017-03-24 23:48:11]

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

[commit-bot: I haz the power, 2017-03-24 23:48:16]

Dry run: This issue passed the CQ dry run.

[miu, 2017-03-28 23:27:35]

PS3 lgtm

[braveyao, 2017-03-28 23:28:58]

The CQ bit was checked by braveyao@chromium.org

[commit-bot: I haz the power, 2017-03-28 23:29:56]

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-03-29 01:11:44]

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

[commit-bot: I haz the power, 2017-03-29 01:11:45]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Niklas Enbom, 2017-03-29 15:21:43]

The CQ bit was checked by niklase@chromium.org

[commit-bot: I haz the power, 2017-03-29 15:22:34]

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-03-29 18:07:00]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490800903583970,
"parent_rev": "907539c74b94bc73c5c5e6359b253723da1c731a", "commit_rev":
"2d7baa9276da1713abbbb01cf8b28065292302ff"}

[commit-bot: I haz the power, 2017-03-29 18:08:15]

Description was changed from

==========
TabCapture: enforce active refresh if there is un-sampled compositor update

During tab capture, if there is no mouse movement and no local preview,
the passive refresh request will always succeed to refresh with the
last cached frame, while the compositor update will most probably be
dropped.
To capture the updated content in time, when there is un-sampled
compositor update event, we'll fail the next passive refresh request to
enforce an active refresh request.

BUG=704257
==========

to

==========
TabCapture: enforce active refresh if there is un-sampled compositor update

During tab capture, if there is no mouse movement and no local preview,
the passive refresh request will always succeed to refresh with the
last cached frame, while the compositor update will most probably be
dropped.
To capture the updated content in time, when there is un-sampled
compositor update event, we'll fail the next passive refresh request to
enforce an active refresh request.

BUG=704257

Review-Url: https://codereview.chromium.org/2770923003
Cr-Commit-Position: refs/heads/master@{#460455}
Committed:
https://chromium.googlesource.com/chromium/src/+/2d7baa9276da1713abbbb01cf8b2...
==========

[commit-bot: I haz the power, 2017-03-29 18:08:17]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/2d7baa9276da1713abbbb01cf8b2...