Ensure ExternalSurface is idle in fullscreen video mode

Ensure ExternalSurface is idle in fullscreen video mode

When the media control progress layer updates itself
(second ticks/progress bar advances/fades in/out) on
top of fullscreen video playback, we do not need to
send OnFrameInfoUpdated() to video hole surface because
it is hidden behind the fullscreen surface.

BUG=

Committed: https://crrev.com/3567ba76d3d617b31fb771564371a7eb1b2c8966
Cr-Commit-Position: refs/heads/master@{#314498}

[william.xie1, 2015-01-28 01:15:02]

william.xie@intel.com changed reviewers:
+ qinmin@chromium.org

[william.xie1, 2015-01-28 01:15:13]

PTAL

[qinmin, 2015-01-28 19:31:03]

qinmin@chromium.org changed reviewers:
+ hugo.holgersson@sonymobile.com

[qinmin, 2015-01-28 19:31:04]

when video enters fullscreen, shouldn't we remove the java external surfaceview?
 

In that case, the updates should just update the position of the external
surfaceView without layout android views. What is this CL trying to optimize?

https://codereview.chromium.org/877253002/diff/1/content/browser/media/androi...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/877253002/diff/1/content/browser/media/androi...
content/browser/media/android/browser_media_player_manager.cc:333: if
(!web_contents_ || fullscreen_player_id_ != kInvalidMediaPlayerId)
What happens if we exit fullscreen? The external surface view will showing up in
a wrong position?

[william.xie1, 2015-01-28 23:33:05]

On 2015/01/28 19:31:04, qinmin wrote:
> when video enters fullscreen, shouldn't we remove the java external
surfaceview?
>  
> 
> In that case, the updates should just update the position of the external
> surfaceView without layout android views. What is this CL trying to optimize?
> 
>
https://codereview.chromium.org/877253002/diff/1/content/browser/media/androi...
> File content/browser/media/android/browser_media_player_manager.cc (right):
> 
>
https://codereview.chromium.org/877253002/diff/1/content/browser/media/androi...
> content/browser/media/android/browser_media_player_manager.cc:333: if
> (!web_contents_ || fullscreen_player_id_ != kInvalidMediaPlayerId)
> What happens if we exit fullscreen? The external surface view will showing up
in
> a wrong position?

Hi Min, 
If remove/add the java external surface during full/embedded switch is expensive
operations, which most times will cause flash screen and impact the user
experience.

[william.xie1, 2015-01-28 23:33:22]

https://codereview.chromium.org/877253002/diff/1/content/browser/media/androi...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/877253002/diff/1/content/browser/media/androi...
content/browser/media/android/browser_media_player_manager.cc:333: if
(!web_contents_ || fullscreen_player_id_ != kInvalidMediaPlayerId)
On 2015/01/28 19:31:04, qinmin wrote:
> What happens if we exit fullscreen? The external surface view will showing up
in
> a wrong position?

Hi Min, if we exit fullscreen, the external surface view will show up in the
original position before enter fullscreen, which is the correct behavior. 
This change is target to skip to notify external surface during the process of
entering to full screen (message is sent from
RendererMediaPlayerManager::DidCommitCompositorFrame()).

[william.xie1, 2015-01-28 23:39:02]

On 2015/01/28 23:33:05, william.xie wrote:
> On 2015/01/28 19:31:04, qinmin wrote:
> > when video enters fullscreen, shouldn't we remove the java external
> surfaceview?
> >  
> > 
> > In that case, the updates should just update the position of the external
> > surfaceView without layout android views. What is this CL trying to
optimize?
> > 
> >
>
https://codereview.chromium.org/877253002/diff/1/content/browser/media/androi...
> > File content/browser/media/android/browser_media_player_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/877253002/diff/1/content/browser/media/androi...
> > content/browser/media/android/browser_media_player_manager.cc:333: if
> > (!web_contents_ || fullscreen_player_id_ != kInvalidMediaPlayerId)
> > What happens if we exit fullscreen? The external surface view will showing
up
> in
> > a wrong position?
> 
> Hi Min, 
> If remove/add the java external surface during full/embedded switch is
expensive
> operations, which most times will cause flash screen and impact the user
> experience.
Sorry, for typo, remove "If" above.
Because remove/add a surfaceview from Android hierarchyviewer is really
expensive, is it suitable for full/embedded switch?

[Hugo Holgersson, 2015-01-29 15:24:18]

lgtm (see inline nit)

https://codereview.chromium.org/877253002/diff/1/content/browser/media/androi...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/877253002/diff/1/content/browser/media/androi...
content/browser/media/android/browser_media_player_manager.cc:323: void
BrowserMediaPlayerManager::OnFrameInfoUpdated() {
I added a print here. In fullscreen video playback, OnFrameInfoUpdated() runs
once the info layer updates (second ticks/progress bar advances/fades in/out).
OnFrameInfoUpdated() does not run at all in a paused state.

To make this clear, in the CL description I'd rather write: "When the progress
layer updates itself (second ticks/progress bar advances/fades in/out) on top of
fullscreen video playback, we do not need to send OnFrameInfoUpdated() to video
hole surface because it is hidden behind the fullscreen surface."

(Tested in com.android.browser with --force-use-overlay-embedded-video.)

[william.xie1, 2015-01-30 03:19:57]

Patchset #2 (id:20001) has been deleted

[william.xie1, 2015-01-30 03:23:22]

On 2015/01/29 15:24:18, Hugo Holgersson wrote:
> lgtm (see inline nit)
> 
>
https://codereview.chromium.org/877253002/diff/1/content/browser/media/androi...
> File content/browser/media/android/browser_media_player_manager.cc (right):
> 
>
https://codereview.chromium.org/877253002/diff/1/content/browser/media/androi...
> content/browser/media/android/browser_media_player_manager.cc:323: void
> BrowserMediaPlayerManager::OnFrameInfoUpdated() {
> I added a print here. In fullscreen video playback, OnFrameInfoUpdated() runs
> once the info layer updates (second ticks/progress bar advances/fades in/out).
> OnFrameInfoUpdated() does not run at all in a paused state.
> 
> To make this clear, in the CL description I'd rather write: "When the progress
> layer updates itself (second ticks/progress bar advances/fades in/out) on top
of
> fullscreen video playback, we do not need to send OnFrameInfoUpdated() to
video
> hole surface because it is hidden behind the fullscreen surface."
> 
> (Tested in com.android.browser with --force-use-overlay-embedded-video.)

Thank you so much, Hugo.
I updated the description per your advice to make it more accurate.

[qinmin, 2015-02-02 17:45:07]

https://codereview.chromium.org/877253002/diff/1/content/browser/media/androi...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/877253002/diff/1/content/browser/media/androi...
content/browser/media/android/browser_media_player_manager.cc:333: if
(!web_contents_ || fullscreen_player_id_ != kInvalidMediaPlayerId)
On 2015/01/28 23:33:22, william.xie wrote:
> On 2015/01/28 19:31:04, qinmin wrote:
> > What happens if we exit fullscreen? The external surface view will showing
up
> in
> > a wrong position?
> 
> Hi Min, if we exit fullscreen, the external surface view will show up in the
> original position before enter fullscreen, which is the correct behavior. 
> This change is target to skip to notify external surface during the process of
> entering to full screen (message is sent from
> RendererMediaPlayerManager::DidCommitCompositorFrame()).

what if user wrote a script to scroll the page while entering fullscreen?
Shouldn't we update the external surface position if scroll position changes?

[william.xie1, 2015-02-03 02:01:10]

On 2015/02/02 17:45:07, qinmin wrote:
>
https://codereview.chromium.org/877253002/diff/1/content/browser/media/androi...
> File content/browser/media/android/browser_media_player_manager.cc (right):
> 
>
https://codereview.chromium.org/877253002/diff/1/content/browser/media/androi...
> content/browser/media/android/browser_media_player_manager.cc:333: if
> (!web_contents_ || fullscreen_player_id_ != kInvalidMediaPlayerId)
> On 2015/01/28 23:33:22, william.xie wrote:
> > On 2015/01/28 19:31:04, qinmin wrote:
> > > What happens if we exit fullscreen? The external surface view will showing
> up
> > in
> > > a wrong position?
> > 
> > Hi Min, if we exit fullscreen, the external surface view will show up in the
> > original position before enter fullscreen, which is the correct behavior. 
> > This change is target to skip to notify external surface during the process
of
> > entering to full screen (message is sent from
> > RendererMediaPlayerManager::DidCommitCompositorFrame()).
> 
> what if user wrote a script to scroll the page while entering fullscreen?
> Shouldn't we update the external surface position if scroll position changes?

Hi Min,
 I tested the case, during auto-scroll, OnFrameInfoUpdated was called instead of
OnNotifyExternalSurface
because only the physical location( by scroll) was changed, no CSS coordinates
was changed. auto-zoon did the same.
(the physical scroll/zoom info could be gotten from mContentViewCore which was a
member of ExternalVideoSurfaceContainer)

And We only started to skip OnFrameInfoUpdated as soon as fullscreen_player_id_
is valid, which means video_view_ (full screen surface) is already created,
so user cannot aware any leg problem.

During exit full screen, once ExitFullscreen was called, fullscreen_player_id_
was set to invalid, and OnFrameInfoUpdated and OnNotifyExternalSurface were
called by render manager,
so the new rect from render manager was set to ExternalVideoSurfaceContainer and
the position of ExternalVideoSurfaceContainer was correct.

Does above flow look good?

[william.xie1, 2015-02-03 02:06:29]

On 2015/02/03 02:01:10, william.xie wrote:
> On 2015/02/02 17:45:07, qinmin wrote:
> >
>
https://codereview.chromium.org/877253002/diff/1/content/browser/media/androi...
> > File content/browser/media/android/browser_media_player_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/877253002/diff/1/content/browser/media/androi...
> > content/browser/media/android/browser_media_player_manager.cc:333: if
> > (!web_contents_ || fullscreen_player_id_ != kInvalidMediaPlayerId)
> > On 2015/01/28 23:33:22, william.xie wrote:
> > > On 2015/01/28 19:31:04, qinmin wrote:
> > > > What happens if we exit fullscreen? The external surface view will
showing
> > up
> > > in
> > > > a wrong position?
> > > 
> > > Hi Min, if we exit fullscreen, the external surface view will show up in
the
> > > original position before enter fullscreen, which is the correct behavior. 
> > > This change is target to skip to notify external surface during the
process
> of
> > > entering to full screen (message is sent from
> > > RendererMediaPlayerManager::DidCommitCompositorFrame()).
> > 
> > what if user wrote a script to scroll the page while entering fullscreen?
> > Shouldn't we update the external surface position if scroll position
changes?
> 

Sorry, update for typo

 Hi Min,
 I tested the case, during auto-scroll, OnFrameInfoUpdated was called instead of
 OnNotifyExternalSurface
 because only the physical location( by scroll) was changed, no CSS coordinates
 was changed. auto-zoon did the same.
 (the physical scroll/zoom info was got from mContentViewCore which was a
 member of ExternalVideoSurfaceContainer)
 
 And We only started to skip OnFrameInfoUpdated as soon as fullscreen_player_id_
 was valid, which means video_view_ (full screen surface) was already created,
 so user didn't aware of any lag problem.
 
 During exit full screen, once ExitFullscreen was called, fullscreen_player_id_
 was set to invalid, and OnFrameInfoUpdated and OnNotifyExternalSurface were
 called by render manager,
 so the new rect from render manager was set to ExternalVideoSurfaceContainer
and
 the position of ExternalVideoSurfaceContainer was correct.
 
 Does above flow look good?

[william.xie1, 2015-02-04 01:37:23]

updated version in patch set 2,
PTAL

[qinmin, 2015-02-04 03:10:54]

lgtm

[william.xie1, 2015-02-04 03:17:34]

The CQ bit was checked by william.xie@intel.com

[commit-bot: I haz the power, 2015-02-04 03:17:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/877253002/40001

[commit-bot: I haz the power, 2015-02-04 04:08:35]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2015-02-04 04:09:30]

Patchset 2 (id:??) landed as
https://crrev.com/3567ba76d3d617b31fb771564371a7eb1b2c8966
Cr-Commit-Position: refs/heads/master@{#314498}