cast: Support for low-latency interactive mode

cast: Support for low-latency interactive mode

Add support to detect the presence of user interaction
while playing non-animated content and then adapt the
target playout latency accordingly.

We leverage the detection of animation content added for ZeroConfig and switch to a low-latency target playout mode
when there is sufficient user interaction and the content
being played is not detected to be animation content.

The goal is to address clear interactive user cases (slides
presentation etc.,) while keeping the impact on animated content watching experience minimal.

Testing involved switching between static and animated content while interacting and using a UDP proxy to observe the target playout time reduce during interaction with low-frame rate content and observe it go up once the sender observes a drop while playing animated content.

BUG=405339
Committed: https://crrev.com/70bcae439e4adfdecbc508787d2334a88f7f7984
Cr-Commit-Position: refs/heads/master@{#364160}

[Irfan, 2015-12-01 18:09:27]

Description was changed from

==========
cast: Add support to detect interactive mode

Add support to detect the presence of user interaction
while playing non-animated content. We leverage the
detection of animation content added for ZeroConfig
and switch to a low-latency target playout mode
when there is sufficient user interaction and the content
being played is not detected to be animation content.

The goal is to address clear interactive user cases (slides
presentation etc.,) while keeping the impact on animated content
watching experience minimal.

Testing involved switching between static and animated content
while interacting and using a UDP proxy to observe the target
playout time reduce during interaction with low-frame rate content
and observe it go up once the sender observes a drop while playing
animated content.

BUG=405339
==========

to

==========
cast: Add support to detect interactive mode

Add support to detect the presence of user interaction
while playing non-animated content. We leverage the
detection of animation content added for ZeroConfig
and switch to a low-latency target playout mode
when there is sufficient user interaction and the content
being played is not detected to be animation content.

The goal is to address clear interactive user cases (slides
presentation etc.,) while keeping the impact on animated content
watching experience minimal.

Testing involved switching between static and animated content
while interacting and using a UDP proxy to observe the target
playout time reduce during interaction with low-frame rate content
and observe it go up once the sender observes a drop while playing
animated content.

BUG=405339
==========

[Irfan, 2015-12-01 18:09:27]

isheriff@chromium.org changed reviewers:
+ miu@chromium.org, xjz@chromium.org

[Irfan, 2015-12-01 18:09:34]

PTAL

[Irfan, 2015-12-01 18:26:23]

Description was changed from

==========
cast: Add support to detect interactive mode

Add support to detect the presence of user interaction
while playing non-animated content. We leverage the
detection of animation content added for ZeroConfig
and switch to a low-latency target playout mode
when there is sufficient user interaction and the content
being played is not detected to be animation content.

The goal is to address clear interactive user cases (slides
presentation etc.,) while keeping the impact on animated content
watching experience minimal.

Testing involved switching between static and animated content
while interacting and using a UDP proxy to observe the target
playout time reduce during interaction with low-frame rate content
and observe it go up once the sender observes a drop while playing
animated content.

BUG=405339
==========

to

==========
cast: Support for low-latency interactive mode

Add support to detect the presence of user interaction
while playing non-animated content and then adapt the 
target playout latency accordingly.

We leverage the detection of animation content added for ZeroConfig and switch
to a low-latency target playout mode
when there is sufficient user interaction and the content
being played is not detected to be animation content.

The goal is to address clear interactive user cases (slides
presentation etc.,) while keeping the impact on animated content watching
experience minimal.

Testing involved switching between static and animated content while interacting
and using a UDP proxy to observe the target playout time reduce during
interaction with low-frame rate content and observe it go up once the sender
observes a drop while playing animated content.

BUG=405339
==========

[miu, 2015-12-01 21:15:27]

https://codereview.chromium.org/1484403002/diff/1/content/browser/media/captu...
File content/browser/media/capture/web_contents_video_capture_device.cc (right):

https://codereview.chromium.org/1484403002/diff/1/content/browser/media/captu...
content/browser/media/capture/web_contents_video_capture_device.cc:378:
frame->metadata()->SetBoolean(media::VideoFrameMetadata::ANIMATION_CONTENT,
In-line with other comments (video_sender.cc), I think it'd be more powerful to
provide timestamps in the metadata instead of booleans.  Specifically:

1. They should be TimeDeltas on the same timeline as the VideoFrame timestamps
themselves.

2. It seems simplest/sufficient to just make these the timestamp of the last
frame where the condition was true.

3. I'd suggest moving all time-comparison logic into this method so it's clear
they are specific to WCVCD capture.  So, for example, instead of a
ui_event_detected() method, I'd suggest a time_of_last_ui_event() accessor that
returns a TimeTicks.  Likewise, instead of IsAnimationActive(), a
time_animation_was_last_detected() accessor.

4. They metadata entries should not be set if state is unknown (i.e.,
|cursor_renderer| is null).

See other comments...

https://codereview.chromium.org/1484403002/diff/1/media/capture/content/video...
File media/capture/content/video_capture_oracle.cc (right):

https://codereview.chromium.org/1484403002/diff/1/media/capture/content/video...
media/capture/content/video_capture_oracle.cc:309: return
duration.InMicroseconds() < kDebouncingPeriodForAnimatedContentMicros;
I don't think this does what you intended.  The kDebouncingPeriod is something
orthogonal.  It was meant to prevent unintended resolution increases as the
"capability" signals fluctuate.

The VCOracle only sets |last_time_animation_was_detected_| after the
AnimatedContentSampler detects animation, and that takes 1-2 seconds' worth of
events to trigger.  So, I think you can just use that.

Overall, I suggest an inline accessor for |last_time_animation_was_detected_|
and plumb this timestamp through to video_sender.cc.

https://codereview.chromium.org/1484403002/diff/1/media/cast/sender/frame_sen...
File media/cast/sender/frame_sender.cc (right):

https://codereview.chromium.org/1484403002/diff/1/media/cast/sender/frame_sen...
media/cast/sender/frame_sender.cc:43: min_playout_delay_(min_playout_delay),
Sanity-check: Can min_playout_delay ever be zero?  Should we at least do
something like this here: std::max(50 milliseconds, min_playout_delay)

https://codereview.chromium.org/1484403002/diff/1/media/cast/sender/frame_sen...
File media/cast/sender/frame_sender.h (right):

https://codereview.chromium.org/1484403002/diff/1/media/cast/sender/frame_sen...
media/cast/sender/frame_sender.h:24: const int
kInteractiveModeStartPlayoutTimeMs = 120;
Rather than as constants, these should be plumbed-in as Audio/VideoSenderConfig,
and all the way up through to the extension API.  See media/cast/cast_config.h,
chrome/common/extensions/api/cast_streaming_rtp_stream.idl, and
chrome/renderer/media/cast_rtp_stream.cc.  This will allow us to run experiments
to determine ideal values, and also to turn on/off this new feature with Cast
Extension releases.

https://codereview.chromium.org/1484403002/diff/1/media/cast/sender/video_sen...
File media/cast/sender/video_sender.cc (right):

https://codereview.chromium.org/1484403002/diff/1/media/cast/sender/video_sen...
media/cast/sender/video_sender.cc:179: user_interaction_count_++;
IIUC, this is counting the number of frames where USER_INTERACTION was true. 
However, frame rate can fluctuate wildly, anywhere from zero (for long periods
of time) to the max frame rate.

I think it'd be better to use time-based thresholds.  If the VideoFrameMetadata
provided timestamps, it would be easy to determine how long to wait before
toggling between the lower or higher playout delay, by comparing the current
VideoFrame's timestamp to the two metadata timestamps.  Something like:

  base::TimeDelta last_time_animation_detected;
  base::TimeDelta last_time_user_interaction_detected;
  video_frame->metadata()->Get...
  video_frame->metadata()->Get...
  const bool is_animating =
      (video_frame->timestamp() - last_time_animation_detected) < "3 seconds";
  const bool is_user_interacting = ...ditto...;
  // ...logic to determine whether to make playout change here...

https://codereview.chromium.org/1484403002/diff/1/media/cast/sender/video_sen...
media/cast/sender/video_sender.cc:182: !animation_content_ &&
!interactive_mode_) {
Perhaps we should only consider the "user interaction" signal, and snap to the
lower playout delay regardless of whether content is animating?  For example, if
the user is seeking a video with a mouse drag on the "scrub bar," they'd
probably want to be using low-latency mirroring.  Thoughts on this?

https://codereview.chromium.org/1484403002/diff/1/media/cast/sender/video_sen...
media/cast/sender/video_sender.cc:194: 
BTW--What about audio?  If we change video's playout delay, we'll lose A/V sync
unless audio is also changed.

OTOH, this will require some refactoring: Something like a
SetTargetPlayoutDelay() method added to CastSenderImpl, which then propagates
the playout delay changes to both AudioSender and VideoSender.

[Irfan, 2015-12-02 22:32:44]

https://codereview.chromium.org/1484403002/diff/1/content/browser/media/captu...
File content/browser/media/capture/web_contents_video_capture_device.cc (right):

https://codereview.chromium.org/1484403002/diff/1/content/browser/media/captu...
content/browser/media/capture/web_contents_video_capture_device.cc:378:
frame->metadata()->SetBoolean(media::VideoFrameMetadata::ANIMATION_CONTENT,
On 2015/12/01 21:15:27, miu wrote:
> In-line with other comments (video_sender.cc), I think it'd be more powerful
to
> provide timestamps in the metadata instead of booleans.  Specifically:
> 
> 1. They should be TimeDeltas on the same timeline as the VideoFrame timestamps
> themselves.
> 
> 2. It seems simplest/sufficient to just make these the timestamp of the last
> frame where the condition was true.
> 
> 3. I'd suggest moving all time-comparison logic into this method so it's clear
> they are specific to WCVCD capture.  So, for example, instead of a
> ui_event_detected() method, I'd suggest a time_of_last_ui_event() accessor
that
> returns a TimeTicks.  Likewise, instead of IsAnimationActive(), a
> time_animation_was_last_detected() accessor.
> 
> 4. They metadata entries should not be set if state is unknown (i.e.,
> |cursor_renderer| is null).
> 
> See other comments...

As discussed will look at moving to a single boolean instead

https://codereview.chromium.org/1484403002/diff/1/media/capture/content/video...
File media/capture/content/video_capture_oracle.cc (right):

https://codereview.chromium.org/1484403002/diff/1/media/capture/content/video...
media/capture/content/video_capture_oracle.cc:309: return
duration.InMicroseconds() < kDebouncingPeriodForAnimatedContentMicros;
On 2015/12/01 21:15:27, miu wrote:
> I don't think this does what you intended.  The kDebouncingPeriod is something
> orthogonal.  It was meant to prevent unintended resolution increases as the
> "capability" signals fluctuate.
> 
> The VCOracle only sets |last_time_animation_was_detected_| after the
> AnimatedContentSampler detects animation, and that takes 1-2 seconds' worth of
> events to trigger.  So, I think you can just use that.
> 
> Overall, I suggest an inline accessor for |last_time_animation_was_detected_|
> and plumb this timestamp through to video_sender.cc.

As discussed, this has the same intention. Will be moving the logic out though.

https://codereview.chromium.org/1484403002/diff/1/media/cast/sender/frame_sen...
File media/cast/sender/frame_sender.cc (right):

https://codereview.chromium.org/1484403002/diff/1/media/cast/sender/frame_sen...
media/cast/sender/frame_sender.cc:43: min_playout_delay_(min_playout_delay),
On 2015/12/01 21:15:27, miu wrote:
> Sanity-check: Can min_playout_delay ever be zero?  Should we at least do
> something like this here: std::max(50 milliseconds, min_playout_delay)

I have gone to previous logic since if there is no min provided, I want to
disable the dynamic playout delay adjustment.

If there is just max provided (400) like now, we will basically end up with
fixed playout delay at all time.

https://codereview.chromium.org/1484403002/diff/1/media/cast/sender/frame_sen...
File media/cast/sender/frame_sender.h (right):

https://codereview.chromium.org/1484403002/diff/1/media/cast/sender/frame_sen...
media/cast/sender/frame_sender.h:24: const int
kInteractiveModeStartPlayoutTimeMs = 120;
On 2015/12/01 21:15:27, miu wrote:
> Rather than as constants, these should be plumbed-in as
Audio/VideoSenderConfig,
> and all the way up through to the extension API.  See
media/cast/cast_config.h,
> chrome/common/extensions/api/cast_streaming_rtp_stream.idl, and
> chrome/renderer/media/cast_rtp_stream.cc.  This will allow us to run
experiments
> to determine ideal values, and also to turn on/off this new feature with Cast
> Extension releases.

I thought about this and have gone with following:

- We can levarage min as the starting point for interactive mode (since a value
less than that does not make much sense)

- Added a new animated playout delay value that is in between min and max so we
can adjust for movie playback dynamically

https://codereview.chromium.org/1484403002/diff/1/media/cast/sender/video_sen...
File media/cast/sender/video_sender.cc (right):

https://codereview.chromium.org/1484403002/diff/1/media/cast/sender/video_sen...
media/cast/sender/video_sender.cc:179: user_interaction_count_++;
On 2015/12/01 21:15:27, miu wrote:
> IIUC, this is counting the number of frames where USER_INTERACTION was true. 
> However, frame rate can fluctuate wildly, anywhere from zero (for long periods
> of time) to the max frame rate.
> 
> I think it'd be better to use time-based thresholds.  If the
VideoFrameMetadata
> provided timestamps, it would be easy to determine how long to wait before
> toggling between the lower or higher playout delay, by comparing the current
> VideoFrame's timestamp to the two metadata timestamps.  Something like:
> 
>   base::TimeDelta last_time_animation_detected;
>   base::TimeDelta last_time_user_interaction_detected;
>   video_frame->metadata()->Get...
>   video_frame->metadata()->Get...
>   const bool is_animating =
>       (video_frame->timestamp() - last_time_animation_detected) < "3 seconds";
>   const bool is_user_interacting = ...ditto...;
>   // ...logic to determine whether to make playout change here...

This has moved to capture side now.

https://codereview.chromium.org/1484403002/diff/1/media/cast/sender/video_sen...
media/cast/sender/video_sender.cc:194: 
On 2015/12/01 21:15:27, miu wrote:
> BTW--What about audio?  If we change video's playout delay, we'll lose A/V
sync
> unless audio is also changed.
> 
> OTOH, this will require some refactoring: Something like a
> SetTargetPlayoutDelay() method added to CastSenderImpl, which then propagates
> the playout delay changes to both AudioSender and VideoSender.

This already works as you describe :-)

[Irfan, 2015-12-02 22:37:55]

PTAL

- Moved UI activity detection in the web contents capture code
- Added a new playout settings to be fed in from extension code. This will
  be used as the starting value for playing animated content.
- We also leverage the minimum value when we detect interactive content.
  As of today, min and max are the same (400). We need a seperate change
  on extension that chooses appropriate values for min, max and animation start

https://codereview.chromium.org/1484403002/diff/1/media/cast/sender/video_sen...
File media/cast/sender/video_sender.cc (right):

https://codereview.chromium.org/1484403002/diff/1/media/cast/sender/video_sen...
media/cast/sender/video_sender.cc:182: !animation_content_ &&
!interactive_mode_) {
On 2015/12/01 21:15:27, miu wrote:
> Perhaps we should only consider the "user interaction" signal, and snap to the
> lower playout delay regardless of whether content is animating?  For example,
if
> the user is seeking a video with a mouse drag on the "scrub bar," they'd
> probably want to be using low-latency mirroring.  Thoughts on this?

As discussed f2f, I think we should be more conservative for user interaction on
animated content

[miu, 2015-12-03 21:26:23]

This is much cleaner, and thanks for making the activity tracker (separate from
the CursorRenderer).  Most of my comments are pedantic details:

https://codereview.chromium.org/1484403002/diff/20001/chrome/renderer/extensi...
File chrome/renderer/extensions/cast_streaming_native_handler.cc (right):

https://codereview.chromium.org/1484403002/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/cast_streaming_native_handler.cc:94: :
ext_params.max_latency;
I think the "else" clause of this expression will return nullptr casted to a
zero integer.  Instead, should we just use a default, bounded within min and
max?  Something like:

  if (cast_params->min_latency_ms > cast_params->max_latency_ms)
    return false;

  ...

  if (cast_params->min_latency_ms <= 0) {
    cast_params->animated_latency_ms = max_latency_ms;
  } else {
    cast_params->animated_latency_ms =
        std::min(cast_params->max_latency_ms,
std::max(cast_params->min_latency_ms, kDefaultAnimatedLatencyMillis));
  }

https://codereview.chromium.org/1484403002/diff/20001/chrome/renderer/media/c...
File chrome/renderer/media/cast_rtp_stream.h (right):

https://codereview.chromium.org/1484403002/diff/20001/chrome/renderer/media/c...
chrome/renderer/media/cast_rtp_stream.h:51: int animated_latency_ms;
Please initialize this in the constructor.

Or, if you want to do a little quick clean-up, you could eliminate the
constructor and use default initialization for all these members (recently
approved Chromium style).  Meaning, the struct would look like:

  struct CastRtpPayloadParams {
    int payload_type = 127;
    int max_latency_ms = 1000;
    int min_latency_ms = 120;
    ...
  };

https://codereview.chromium.org/1484403002/diff/20001/content/browser/media/c...
File content/browser/media/capture/ui_activity_tracker.h (right):

https://codereview.chromium.org/1484403002/diff/20001/content/browser/media/c...
content/browser/media/capture/ui_activity_tracker.h:16: class CONTENT_EXPORT
UiActivityTracker {
naming nit: Based on what this does, consider something like
WindowActivityTracker.

https://codereview.chromium.org/1484403002/diff/20001/content/browser/media/c...
content/browser/media/capture/ui_activity_tracker.h:21: virtual bool
uiInteractionActive() const = 0;
style: No camel case.  Suggestion: IsUiInteractionActive() or
IsUserInteracting()

https://codereview.chromium.org/1484403002/diff/20001/content/browser/media/c...
content/browser/media/capture/ui_activity_tracker.h:24: virtual void reset() =
0;
Because it's non-inlined (and virtual), style requires: Reset()

https://codereview.chromium.org/1484403002/diff/20001/content/browser/media/c...
File content/browser/media/capture/ui_activity_tracker_aura.h (right):

https://codereview.chromium.org/1484403002/diff/20001/content/browser/media/c...
content/browser/media/capture/ui_activity_tracker_aura.h:33: void
OnEvent(ui::Event* event) final;
nit: This method and OnWindowDestroying() should probably be private.

https://codereview.chromium.org/1484403002/diff/20001/content/browser/media/c...
File content/browser/media/capture/web_contents_video_capture_device.cc (right):

https://codereview.chromium.org/1484403002/diff/20001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device.cc:402: bool
FrameSubscriber::IsInteractiveModeActive() {
naming nit: To aid those unfamiliar with this code, how about something like:
IsUserInteractingWithContent()

https://codereview.chromium.org/1484403002/diff/20001/media/base/video_frame_...
File media/base/video_frame_metadata.h (right):

https://codereview.chromium.org/1484403002/diff/20001/media/base/video_frame_...
media/base/video_frame_metadata.h:84: // with is adjusting end-to-end latency
based on whether the mode is turned
For a bit more clarity for those unfamiliar with mirroring concepts:

s/with is adjusting/with is remoting content: adjusting/

and

s/based on whether the mode is turned on or off/down to help the user better
coordinate their actions/

https://codereview.chromium.org/1484403002/diff/20001/media/base/video_frame_...
media/base/video_frame_metadata.h:88: INTERACTIVE_MODE,
1. naming nit: How about INTERACTIVE_CONTENT?  (i.e., focus is on this
VideoFrame's media content, not an overall systemic "mode")

2. Please move this up so it's in alphabetical order.

https://codereview.chromium.org/1484403002/diff/20001/media/cast/sender/frame...
File media/cast/sender/frame_sender.cc (right):

https://codereview.chromium.org/1484403002/diff/20001/media/cast/sender/frame...
media/cast/sender/frame_sender.cc:66: VLOG(1) << "min latency " <<
min_playout_delay_.InMilliseconds()
nit: VLOG(1) << SENDER_SSRC << ...and the rest...

https://codereview.chromium.org/1484403002/diff/20001/media/cast/sender/frame...
File media/cast/sender/frame_sender.h (right):

https://codereview.chromium.org/1484403002/diff/20001/media/cast/sender/frame...
media/cast/sender/frame_sender.h:116: // The |target_playout delay_| is the
current delay that is adaptively
Nice comment!  :)

https://codereview.chromium.org/1484403002/diff/20001/media/cast/sender/frame...
media/cast/sender/frame_sender.h:123: base::TimeDelta min_playout_delay_;
nit: Looks like min/max/animated_playout_delay_ could be made const.

https://codereview.chromium.org/1484403002/diff/20001/media/cast/sender/video...
File media/cast/sender/video_sender.h (right):

https://codereview.chromium.org/1484403002/diff/20001/media/cast/sender/video...
media/cast/sender/video_sender.h:101: // Interactive mode as indicated by the
web content capture logic.
This concepts mentioned in this comment is an abstraction violation.  Thinking
about what this bool controls in VideoSender, you could call it:
low_latency_mode_;

[Irfan, 2015-12-04 22:45:37]

https://codereview.chromium.org/1484403002/diff/20001/chrome/renderer/extensi...
File chrome/renderer/extensions/cast_streaming_native_handler.cc (right):

https://codereview.chromium.org/1484403002/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/cast_streaming_native_handler.cc:94: :
ext_params.max_latency;
On 2015/12/03 21:26:22, miu wrote:
> I think the "else" clause of this expression will return nullptr casted to a
> zero integer.  Instead, should we just use a default, bounded within min and
> max?  Something like:
> 
>   if (cast_params->min_latency_ms > cast_params->max_latency_ms)
>     return false;
> 
>   ...
> 
>   if (cast_params->min_latency_ms <= 0) {
>     cast_params->animated_latency_ms = max_latency_ms;
>   } else {
>     cast_params->animated_latency_ms =
>         std::min(cast_params->max_latency_ms,
> std::max(cast_params->min_latency_ms, kDefaultAnimatedLatencyMillis));
>   }

The else part of the expression is returning an int. max_latency is a mandatory
parameter (and hence an int). If there are no animation/min params passed, they
default to the max value.

https://codereview.chromium.org/1484403002/diff/20001/chrome/renderer/media/c...
File chrome/renderer/media/cast_rtp_stream.h (right):

https://codereview.chromium.org/1484403002/diff/20001/chrome/renderer/media/c...
chrome/renderer/media/cast_rtp_stream.h:51: int animated_latency_ms;
On 2015/12/03 21:26:22, miu wrote:
> Please initialize this in the constructor.
> 
> Or, if you want to do a little quick clean-up, you could eliminate the
> constructor and use default initialization for all these members (recently
> approved Chromium style).  Meaning, the struct would look like:
> 
>   struct CastRtpPayloadParams {
>     int payload_type = 127;
>     int max_latency_ms = 1000;
>     int min_latency_ms = 120;
>     ...
>   };

Done.

https://codereview.chromium.org/1484403002/diff/20001/content/browser/media/c...
File content/browser/media/capture/ui_activity_tracker.h (right):

https://codereview.chromium.org/1484403002/diff/20001/content/browser/media/c...
content/browser/media/capture/ui_activity_tracker.h:16: class CONTENT_EXPORT
UiActivityTracker {
On 2015/12/03 21:26:22, miu wrote:
> naming nit: Based on what this does, consider something like
> WindowActivityTracker.

Done.

https://codereview.chromium.org/1484403002/diff/20001/content/browser/media/c...
content/browser/media/capture/ui_activity_tracker.h:21: virtual bool
uiInteractionActive() const = 0;
On 2015/12/03 21:26:22, miu wrote:
> style: No camel case.  Suggestion: IsUiInteractionActive() or
> IsUserInteracting()

Done.

https://codereview.chromium.org/1484403002/diff/20001/content/browser/media/c...
content/browser/media/capture/ui_activity_tracker.h:24: virtual void reset() =
0;
On 2015/12/03 21:26:22, miu wrote:
> Because it's non-inlined (and virtual), style requires: Reset()

Done.

https://codereview.chromium.org/1484403002/diff/20001/content/browser/media/c...
File content/browser/media/capture/ui_activity_tracker_aura.h (right):

https://codereview.chromium.org/1484403002/diff/20001/content/browser/media/c...
content/browser/media/capture/ui_activity_tracker_aura.h:33: void
OnEvent(ui::Event* event) final;
On 2015/12/03 21:26:22, miu wrote:
> nit: This method and OnWindowDestroying() should probably be private.

Done.

https://codereview.chromium.org/1484403002/diff/20001/content/browser/media/c...
File content/browser/media/capture/web_contents_video_capture_device.cc (right):

https://codereview.chromium.org/1484403002/diff/20001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device.cc:402: bool
FrameSubscriber::IsInteractiveModeActive() {
On 2015/12/03 21:26:23, miu wrote:
> naming nit: To aid those unfamiliar with this code, how about something like:
> IsUserInteractingWithContent()

Done.

https://codereview.chromium.org/1484403002/diff/20001/media/base/video_frame_...
File media/base/video_frame_metadata.h (right):

https://codereview.chromium.org/1484403002/diff/20001/media/base/video_frame_...
media/base/video_frame_metadata.h:84: // with is adjusting end-to-end latency
based on whether the mode is turned
On 2015/12/03 21:26:23, miu wrote:
> For a bit more clarity for those unfamiliar with mirroring concepts:
> 
> s/with is adjusting/with is remoting content: adjusting/
> 
> and
> 
> s/based on whether the mode is turned on or off/down to help the user better
> coordinate their actions/

Done.

https://codereview.chromium.org/1484403002/diff/20001/media/base/video_frame_...
media/base/video_frame_metadata.h:88: INTERACTIVE_MODE,
On 2015/12/03 21:26:23, miu wrote:
> 1. naming nit: How about INTERACTIVE_CONTENT?  (i.e., focus is on this
> VideoFrame's media content, not an overall systemic "mode")
> 
> 2. Please move this up so it's in alphabetical order.

Done.

https://codereview.chromium.org/1484403002/diff/20001/media/cast/sender/frame...
File media/cast/sender/frame_sender.cc (right):

https://codereview.chromium.org/1484403002/diff/20001/media/cast/sender/frame...
media/cast/sender/frame_sender.cc:66: VLOG(1) << "min latency " <<
min_playout_delay_.InMilliseconds()
On 2015/12/03 21:26:23, miu wrote:
> nit: VLOG(1) << SENDER_SSRC << ...and the rest...

Done.

https://codereview.chromium.org/1484403002/diff/20001/media/cast/sender/frame...
File media/cast/sender/frame_sender.h (right):

https://codereview.chromium.org/1484403002/diff/20001/media/cast/sender/frame...
media/cast/sender/frame_sender.h:123: base::TimeDelta min_playout_delay_;
On 2015/12/03 21:26:23, miu wrote:
> nit: Looks like min/max/animated_playout_delay_ could be made const.

Done.

https://codereview.chromium.org/1484403002/diff/20001/media/cast/sender/video...
File media/cast/sender/video_sender.h (right):

https://codereview.chromium.org/1484403002/diff/20001/media/cast/sender/video...
media/cast/sender/video_sender.h:101: // Interactive mode as indicated by the
web content capture logic.
On 2015/12/03 21:26:23, miu wrote:
> This concepts mentioned in this comment is an abstraction violation.  Thinking
> about what this bool controls in VideoSender, you could call it:
> low_latency_mode_;

Done.

[Irfan, 2015-12-04 23:09:32]

PTAL

[Irfan, 2015-12-04 23:10:14]

isheriff@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Irfan, 2015-12-04 23:10:37]

+Devlin, for owner review

[Irfan, 2015-12-04 23:12:56]

PS #4 adds the renamed files I forgot to commit

[Irfan, 2015-12-04 23:38:47]

Patchset #5 (id:80001) has been deleted

[Irfan, 2015-12-07 18:04:56]

Patchset #4 (id:60001) has been deleted

[Irfan, 2015-12-07 19:53:04]

PTAL. PS#5 fixes a bug caught by test (animated latency was not being set to max
when uninitialized) and adds some logs.

[Devlin, 2015-12-07 20:14:06]

extensions lgtm

[miu, 2015-12-08 00:16:16]

https://codereview.chromium.org/1484403002/diff/20001/chrome/renderer/extensi...
File chrome/renderer/extensions/cast_streaming_native_handler.cc (right):

https://codereview.chromium.org/1484403002/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/cast_streaming_native_handler.cc:94: :
ext_params.max_latency;
On 2015/12/04 22:45:36, Irfan wrote:
> On 2015/12/03 21:26:22, miu wrote:
> > I think the "else" clause of this expression will return nullptr casted to a
> > zero integer.  Instead, should we just use a default, bounded within min and
> > max?  Something like:
> > 
> >   if (cast_params->min_latency_ms > cast_params->max_latency_ms)
> >     return false;
> > 
> >   ...
> > 
> >   if (cast_params->min_latency_ms <= 0) {
> >     cast_params->animated_latency_ms = max_latency_ms;
> >   } else {
> >     cast_params->animated_latency_ms =
> >         std::min(cast_params->max_latency_ms,
> > std::max(cast_params->min_latency_ms, kDefaultAnimatedLatencyMillis));
> >   }
> 
> The else part of the expression is returning an int. max_latency is a
mandatory
> parameter (and hence an int). If there are no animation/min params passed,
they
> default to the max value.

Acknowledged.

https://codereview.chromium.org/1484403002/diff/120001/chrome/renderer/media/...
File chrome/renderer/media/cast_rtp_stream.h (right):

https://codereview.chromium.org/1484403002/diff/120001/chrome/renderer/media/...
chrome/renderer/media/cast_rtp_stream.h:88: CastRtpPayloadParams();
Is the ctor still needed?  I see the defaults added to the members above, but no
change to, or removal of, the ctor.

https://codereview.chromium.org/1484403002/diff/120001/content/browser/media/...
File content/browser/media/capture/web_contents_video_capture_device.cc (right):

https://codereview.chromium.org/1484403002/diff/120001/content/browser/media/...
content/browser/media/capture/web_contents_video_capture_device.cc:440:
window_activity_tracker_.reset(
This looks like it'll work for all Aura platforms, not just Linux.  Do you want
to move this out?

https://codereview.chromium.org/1484403002/diff/120001/content/browser/media/...
File content/browser/media/capture/window_activity_tracker_aura.h (right):

https://codereview.chromium.org/1484403002/diff/120001/content/browser/media/...
content/browser/media/capture/window_activity_tracker_aura.h:43: // The number
of UI events detected so far. In case of continuous events
nit: add newline above this line

https://codereview.chromium.org/1484403002/diff/120001/media/base/video_frame...
File media/base/video_frame_metadata.h (right):

https://codereview.chromium.org/1484403002/diff/120001/media/base/video_frame...
media/base/video_frame_metadata.h:58: // user better coordinate their actions
nit: Period at end of sentence.

https://codereview.chromium.org/1484403002/diff/120001/media/cast/sender/vide...
File media/cast/sender/video_sender.cc (right):

https://codereview.chromium.org/1484403002/diff/120001/media/cast/sender/vide...
media/cast/sender/video_sender.cc:207: // In case of low latency mode, we prefer
frame drops over increasing
nit: IMO, this comment is more useful in the header file where the
|low_latency_mode_| member is declared.

https://codereview.chromium.org/1484403002/diff/120001/media/cast/sender/vide...
media/cast/sender/video_sender.cc:210: // In case we detect user is no more in
an low latency mode and there is
nit: s/no more/no longer/

[Irfan, 2015-12-08 01:30:19]

https://codereview.chromium.org/1484403002/diff/120001/chrome/renderer/media/...
File chrome/renderer/media/cast_rtp_stream.h (right):

https://codereview.chromium.org/1484403002/diff/120001/chrome/renderer/media/...
chrome/renderer/media/cast_rtp_stream.h:88: CastRtpPayloadParams();
On 2015/12/08 00:16:16, miu wrote:
> Is the ctor still needed?  I see the defaults added to the members above, but
no
> change to, or removal of, the ctor.

Yeah, I get a style complaint along the lines of the class being too complex to
lack a constructur.

https://codereview.chromium.org/1484403002/diff/120001/content/browser/media/...
File content/browser/media/capture/web_contents_video_capture_device.cc (right):

https://codereview.chromium.org/1484403002/diff/120001/content/browser/media/...
content/browser/media/capture/web_contents_video_capture_device.cc:440:
window_activity_tracker_.reset(
On 2015/12/08 00:16:16, miu wrote:
> This looks like it'll work for all Aura platforms, not just Linux.  Do you
want
> to move this out?

Done.

https://codereview.chromium.org/1484403002/diff/120001/content/browser/media/...
File content/browser/media/capture/window_activity_tracker_aura.h (right):

https://codereview.chromium.org/1484403002/diff/120001/content/browser/media/...
content/browser/media/capture/window_activity_tracker_aura.h:43: // The number
of UI events detected so far. In case of continuous events
On 2015/12/08 00:16:16, miu wrote:
> nit: add newline above this line

Done.

https://codereview.chromium.org/1484403002/diff/120001/media/base/video_frame...
File media/base/video_frame_metadata.h (right):

https://codereview.chromium.org/1484403002/diff/120001/media/base/video_frame...
media/base/video_frame_metadata.h:58: // user better coordinate their actions
On 2015/12/08 00:16:16, miu wrote:
> nit: Period at end of sentence.

Done.

https://codereview.chromium.org/1484403002/diff/120001/media/cast/sender/vide...
File media/cast/sender/video_sender.cc (right):

https://codereview.chromium.org/1484403002/diff/120001/media/cast/sender/vide...
media/cast/sender/video_sender.cc:207: // In case of low latency mode, we prefer
frame drops over increasing
On 2015/12/08 00:16:16, miu wrote:
> nit: IMO, this comment is more useful in the header file where the
> |low_latency_mode_| member is declared.

Also added in header.

https://codereview.chromium.org/1484403002/diff/120001/media/cast/sender/vide...
media/cast/sender/video_sender.cc:210: // In case we detect user is no more in
an low latency mode and there is
On 2015/12/08 00:16:16, miu wrote:
> nit: s/no more/no longer/

Done.

[Irfan, 2015-12-08 01:36:54]

PTAL

[miu, 2015-12-08 02:04:59]

lgtm

https://codereview.chromium.org/1484403002/diff/140001/content/browser/media/...
File content/browser/media/capture/web_contents_video_capture_device.cc (right):

https://codereview.chromium.org/1484403002/diff/140001/content/browser/media/...
content/browser/media/capture/web_contents_video_capture_device.cc:442: #if
defined(USE_AURA)
BTW--Is there a bug to track adding the implementation CursorRenderer and
WindowActivityTracker for the other desktop platforms?  Suggest you include a
TODO comment with a http://crbug.com/###### link in the code here.

[Irfan, 2015-12-08 15:24:12]

https://codereview.chromium.org/1484403002/diff/140001/content/browser/media/...
File content/browser/media/capture/web_contents_video_capture_device.cc (right):

https://codereview.chromium.org/1484403002/diff/140001/content/browser/media/...
content/browser/media/capture/web_contents_video_capture_device.cc:442: #if
defined(USE_AURA)
On 2015/12/08 02:04:59, miu wrote:
> BTW--Is there a bug to track adding the implementation CursorRenderer and
> WindowActivityTracker for the other desktop platforms?  Suggest you include a
> TODO comment with a http://crbug.com/###### link in the code here.

Done.

[Irfan, 2015-12-08 22:59:23]

The CQ bit was checked by isheriff@chromium.org

[Irfan, 2015-12-08 22:59:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, miu@chromium.org
Link to the patchset: https://codereview.chromium.org/1484403002/#ps160001
(title: "Add crbug in comments")

[commit-bot: I haz the power, 2015-12-08 23:01:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1484403002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1484403002/160001

[commit-bot: I haz the power, 2015-12-08 23:31:17]

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

[commit-bot: I haz the power, 2015-12-08 23:31:18]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Irfan, 2015-12-08 23:39:03]

isheriff@chromium.org changed reviewers:
+ dalecurtis@chromium.org, jam@chromium.org

[Irfan, 2015-12-08 23:39:49]

dalecurtis@, please look at media/base/ changes for LGTM

jam@, please look at content/ changes for LGTM

[jam, 2015-12-09 00:00:06]

On 2015/12/08 23:39:49, Irfan wrote:
> dalecurtis@, please look at media/base/ changes for LGTM
> 
> jam@, please look at content/ changes for LGTM

content/browser/BUILD.gn lgtm

[DaleCurtis, 2015-12-09 21:00:04]

lgtm

[Irfan, 2015-12-09 21:07:17]

The CQ bit was checked by isheriff@chromium.org

[Irfan, 2015-12-09 21:07:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, jam@chromium.org, miu@chromium.org
Link to the patchset: https://codereview.chromium.org/1484403002/#ps180001
(title: "Rebased")

[commit-bot: I haz the power, 2015-12-09 21:07:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1484403002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1484403002/180001

[commit-bot: I haz the power, 2015-12-09 21:41:37]

Description was changed from

==========
cast: Support for low-latency interactive mode

Add support to detect the presence of user interaction
while playing non-animated content and then adapt the 
target playout latency accordingly.

We leverage the detection of animation content added for ZeroConfig and switch
to a low-latency target playout mode
when there is sufficient user interaction and the content
being played is not detected to be animation content.

The goal is to address clear interactive user cases (slides
presentation etc.,) while keeping the impact on animated content watching
experience minimal.

Testing involved switching between static and animated content while interacting
and using a UDP proxy to observe the target playout time reduce during
interaction with low-frame rate content and observe it go up once the sender
observes a drop while playing animated content.

BUG=405339
==========

to

==========
cast: Support for low-latency interactive mode

Add support to detect the presence of user interaction
while playing non-animated content and then adapt the 
target playout latency accordingly.

We leverage the detection of animation content added for ZeroConfig and switch
to a low-latency target playout mode
when there is sufficient user interaction and the content
being played is not detected to be animation content.

The goal is to address clear interactive user cases (slides
presentation etc.,) while keeping the impact on animated content watching
experience minimal.

Testing involved switching between static and animated content while interacting
and using a UDP proxy to observe the target playout time reduce during
interaction with low-frame rate content and observe it go up once the sender
observes a drop while playing animated content.

BUG=405339
==========

[commit-bot: I haz the power, 2015-12-09 21:41:38]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2015-12-09 21:42:32]

Description was changed from

==========
cast: Support for low-latency interactive mode

Add support to detect the presence of user interaction
while playing non-animated content and then adapt the 
target playout latency accordingly.

We leverage the detection of animation content added for ZeroConfig and switch
to a low-latency target playout mode
when there is sufficient user interaction and the content
being played is not detected to be animation content.

The goal is to address clear interactive user cases (slides
presentation etc.,) while keeping the impact on animated content watching
experience minimal.

Testing involved switching between static and animated content while interacting
and using a UDP proxy to observe the target playout time reduce during
interaction with low-frame rate content and observe it go up once the sender
observes a drop while playing animated content.

BUG=405339
==========

to

==========
cast: Support for low-latency interactive mode

Add support to detect the presence of user interaction
while playing non-animated content and then adapt the
target playout latency accordingly.

We leverage the detection of animation content added for ZeroConfig and switch
to a low-latency target playout mode
when there is sufficient user interaction and the content
being played is not detected to be animation content.

The goal is to address clear interactive user cases (slides
presentation etc.,) while keeping the impact on animated content watching
experience minimal.

Testing involved switching between static and animated content while interacting
and using a UDP proxy to observe the target playout time reduce during
interaction with low-frame rate content and observe it go up once the sender
observes a drop while playing animated content.

BUG=405339

Committed: https://crrev.com/70bcae439e4adfdecbc508787d2334a88f7f7984
Cr-Commit-Position: refs/heads/master@{#364160}
==========

[commit-bot: I haz the power, 2015-12-09 21:42:33]

Patchset 8 (id:??) landed as
https://crrev.com/70bcae439e4adfdecbc508787d2334a88f7f7984
Cr-Commit-Position: refs/heads/master@{#364160}