New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 823172 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
no longer active
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 1
Type: Bug

Blocking:
issue 726619



Sign in to add a comment

[PIP] Tear down / exit PiP mode

Project Member Reported by apaci...@chromium.org, Mar 19 2018

Issue description

Several ways to exit PiP mode:
- User gesture to window
- Closing tab w/video
- Closing browser w/video
 
Cc: mlamouri@chromium.org
Components: Blink>Media>PictureInPicture
Also closing PIP via API.
Project Member

Comment 2 by bugdroid1@chromium.org, Apr 1 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/02897b762a9d414334439a044d4f4ee35482bfb7

commit 02897b762a9d414334439a044d4f4ee35482bfb7
Author: Jennifer Apacible <apacible@chromium.org>
Date: Sun Apr 01 01:00:49 2018

[Picture in Picture] Exit PiP from WMPI.

This clears the PiP video id from MediaWebContentsObserver.

Future work, including completely tearing down on the PiP window
side, will be added once there is messaging between the
PictureInPictureWindowController and the MediaWebContentsObserver.

BUG:  726619   823172 
Change-Id: I923acf848e71fd43ce8c2532e25bf876836bd0d1
Reviewed-on: https://chromium-review.googlesource.com/967239
Commit-Queue: apacible <apacible@chromium.org>
Reviewed-by: Frank Liberato <liberato@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547393}
[modify] https://crrev.com/02897b762a9d414334439a044d4f4ee35482bfb7/chrome/browser/overlay/overlay_surface_embedder.h
[modify] https://crrev.com/02897b762a9d414334439a044d4f4ee35482bfb7/chrome/browser/picture_in_picture/picture_in_picture_window_controller.cc
[modify] https://crrev.com/02897b762a9d414334439a044d4f4ee35482bfb7/chrome/browser/picture_in_picture/picture_in_picture_window_controller_browsertest.cc
[modify] https://crrev.com/02897b762a9d414334439a044d4f4ee35482bfb7/chrome/browser/ui/browser.cc
[modify] https://crrev.com/02897b762a9d414334439a044d4f4ee35482bfb7/content/browser/media/media_web_contents_observer.cc
[modify] https://crrev.com/02897b762a9d414334439a044d4f4ee35482bfb7/content/browser/media/media_web_contents_observer.h
[modify] https://crrev.com/02897b762a9d414334439a044d4f4ee35482bfb7/content/common/media/media_player_delegate_messages.h
[modify] https://crrev.com/02897b762a9d414334439a044d4f4ee35482bfb7/content/renderer/media/renderer_webmediaplayer_delegate.cc
[modify] https://crrev.com/02897b762a9d414334439a044d4f4ee35482bfb7/content/renderer/media/renderer_webmediaplayer_delegate.h
[modify] https://crrev.com/02897b762a9d414334439a044d4f4ee35482bfb7/content/renderer/media/stream/webmediaplayer_ms.cc
[modify] https://crrev.com/02897b762a9d414334439a044d4f4ee35482bfb7/content/renderer/media/stream/webmediaplayer_ms.h
[modify] https://crrev.com/02897b762a9d414334439a044d4f4ee35482bfb7/content/renderer/media/stream/webmediaplayer_ms_unittest.cc
[modify] https://crrev.com/02897b762a9d414334439a044d4f4ee35482bfb7/content/renderer/media_capture_from_element/html_video_element_capturer_source_unittest.cc
[modify] https://crrev.com/02897b762a9d414334439a044d4f4ee35482bfb7/media/blink/webmediaplayer_delegate.h
[modify] https://crrev.com/02897b762a9d414334439a044d4f4ee35482bfb7/media/blink/webmediaplayer_impl.cc
[modify] https://crrev.com/02897b762a9d414334439a044d4f4ee35482bfb7/media/blink/webmediaplayer_impl.h
[modify] https://crrev.com/02897b762a9d414334439a044d4f4ee35482bfb7/media/blink/webmediaplayer_impl_unittest.cc
[modify] https://crrev.com/02897b762a9d414334439a044d4f4ee35482bfb7/third_party/WebKit/Source/platform/testing/EmptyWebMediaPlayer.h
[modify] https://crrev.com/02897b762a9d414334439a044d4f4ee35482bfb7/third_party/WebKit/public/platform/WebMediaPlayer.h

Project Member

Comment 3 by bugdroid1@chromium.org, Apr 6 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f6d55b110ae7ea1a49c131d647f279e8a57d1813

commit f6d55b110ae7ea1a49c131d647f279e8a57d1813
Author: Jennifer Apacible <apacible@chromium.org>
Date: Fri Apr 06 04:23:40 2018

[Picture in Picture] Add ExitPipCB callback for WMPI teardown.

Currently, we run the PipSurfaceInfoCB callback in WMPI with an empty
viz::SurfaceId, which is not allowed. This change adds a new mojo call
to explicitly exit from Picture-in-Picture mode from the WMPI side.

BUG:  823172 
Change-Id: I81bb71a3853325bcf31a56f8929d4aba17f66248
Reviewed-on: https://chromium-review.googlesource.com/996468
Commit-Queue: apacible <apacible@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548668}
[modify] https://crrev.com/f6d55b110ae7ea1a49c131d647f279e8a57d1813/chrome/browser/picture_in_picture/picture_in_picture_window_controller.cc
[modify] https://crrev.com/f6d55b110ae7ea1a49c131d647f279e8a57d1813/chrome/browser/ui/browser.cc
[modify] https://crrev.com/f6d55b110ae7ea1a49c131d647f279e8a57d1813/chrome/browser/ui/browser.h
[modify] https://crrev.com/f6d55b110ae7ea1a49c131d647f279e8a57d1813/content/browser/frame_host/render_frame_host_delegate.h
[modify] https://crrev.com/f6d55b110ae7ea1a49c131d647f279e8a57d1813/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/f6d55b110ae7ea1a49c131d647f279e8a57d1813/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/f6d55b110ae7ea1a49c131d647f279e8a57d1813/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/f6d55b110ae7ea1a49c131d647f279e8a57d1813/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/f6d55b110ae7ea1a49c131d647f279e8a57d1813/content/common/frame.mojom
[modify] https://crrev.com/f6d55b110ae7ea1a49c131d647f279e8a57d1813/content/public/browser/web_contents_delegate.cc
[modify] https://crrev.com/f6d55b110ae7ea1a49c131d647f279e8a57d1813/content/public/browser/web_contents_delegate.h
[modify] https://crrev.com/f6d55b110ae7ea1a49c131d647f279e8a57d1813/content/renderer/media/media_factory.cc
[modify] https://crrev.com/f6d55b110ae7ea1a49c131d647f279e8a57d1813/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/f6d55b110ae7ea1a49c131d647f279e8a57d1813/content/renderer/render_frame_impl.h
[modify] https://crrev.com/f6d55b110ae7ea1a49c131d647f279e8a57d1813/content/test/test_render_frame.cc
[modify] https://crrev.com/f6d55b110ae7ea1a49c131d647f279e8a57d1813/media/blink/webmediaplayer_impl.cc
[modify] https://crrev.com/f6d55b110ae7ea1a49c131d647f279e8a57d1813/media/blink/webmediaplayer_impl.h
[modify] https://crrev.com/f6d55b110ae7ea1a49c131d647f279e8a57d1813/media/blink/webmediaplayer_impl_unittest.cc
[modify] https://crrev.com/f6d55b110ae7ea1a49c131d647f279e8a57d1813/media/blink/webmediaplayer_params.cc
[modify] https://crrev.com/f6d55b110ae7ea1a49c131d647f279e8a57d1813/media/blink/webmediaplayer_params.h

Blocking: 726619
Project Member

Comment 5 by bugdroid1@chromium.org, Apr 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5cd9a7e373b918a58a709d8abb6ac1c23cf4103e

commit 5cd9a7e373b918a58a709d8abb6ac1c23cf4103e
Author: Jennifer Apacible <apacible@chromium.org>
Date: Tue Apr 17 03:08:06 2018

[Picture in Picture] Handle ending Picture-in-Picture from user gesture.

This consolidates some of the similar exit-Picture-in-Picture behavior
from both WebMediaPlayerImpl and PictureInPictureWindowController sides.

When exiting from WMPI, we need to run an additional callback to signal
that the window should be closed. From user gesture, the window tear
down has already been handled.

BUG:  823172 
Change-Id: I4ee6e03a519a7564f7eb8b195f2dc7ddeacf4a88
Reviewed-on: https://chromium-review.googlesource.com/1012521
Commit-Queue: apacible <apacible@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551224}
[modify] https://crrev.com/5cd9a7e373b918a58a709d8abb6ac1c23cf4103e/media/blink/webmediaplayer_impl.cc

Project Member

Comment 6 by bugdroid1@chromium.org, May 1 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e20a9bd2b8148d3e867773a135768324b2439ea8

commit e20a9bd2b8148d3e867773a135768324b2439ea8
Author: Jennifer Apacible <apacible@chromium.org>
Date: Tue May 01 07:01:20 2018

[Picture in Picture] Pause active videos when leaving PiP mode.

There are two instances where PictureInPictureWindowController
is torn down:
1. Tab is closed (controller is tied to the tab's WebContents). In this
case, the player is also deleted, so there are no behavioral concerns.
2. A video on a different tab enters Picture-in-Picture mode. In this
case, the player from the earlier PiP'd tab may still be playing in that
tab, meaning there may be two(+) videos playing.

This change would pause the initial video, as the user has indicated
they want to PiP a new video.

This behavior will be up for further discussion with UX.

BUG:  823172 
Change-Id: Iccda65eecea859f4e6fee291ff8ae511304d6ca5
Reviewed-on: https://chromium-review.googlesource.com/1034031
Commit-Queue: apacible <apacible@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555013}
[modify] https://crrev.com/e20a9bd2b8148d3e867773a135768324b2439ea8/content/browser/picture_in_picture/picture_in_picture_window_controller_impl.cc

Project Member

Comment 7 by bugdroid1@chromium.org, May 1 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/661130e01c1f86bb5db527ad09089903c61ab303

commit 661130e01c1f86bb5db527ad09089903c61ab303
Author: Ned Nguyen <nednguyen@google.com>
Date: Tue May 01 14:57:31 2018

Revert "[Picture in Picture] Pause active videos when leaving PiP mode."

This reverts commit e20a9bd2b8148d3e867773a135768324b2439ea8.

Reason for revert: suspect break PictureInPictureWindowControllerBrowserTest.CreationAndVisibility test

Original change's description:
> [Picture in Picture] Pause active videos when leaving PiP mode.
> 
> There are two instances where PictureInPictureWindowController
> is torn down:
> 1. Tab is closed (controller is tied to the tab's WebContents). In this
> case, the player is also deleted, so there are no behavioral concerns.
> 2. A video on a different tab enters Picture-in-Picture mode. In this
> case, the player from the earlier PiP'd tab may still be playing in that
> tab, meaning there may be two(+) videos playing.
> 
> This change would pause the initial video, as the user has indicated
> they want to PiP a new video.
> 
> This behavior will be up for further discussion with UX.
> 
> BUG:  823172 
> Change-Id: Iccda65eecea859f4e6fee291ff8ae511304d6ca5
> Reviewed-on: https://chromium-review.googlesource.com/1034031
> Commit-Queue: apacible <apacible@chromium.org>
> Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#555013}

TBR=mlamouri@chromium.org,apacible@chromium.org

Change-Id: Ic74635ab4e66fec1d8524d3e2e2fa272d5a353af
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/1036926
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Commit-Queue: Ned Nguyen <nednguyen@google.com>
Cr-Commit-Position: refs/heads/master@{#555038}
[modify] https://crrev.com/661130e01c1f86bb5db527ad09089903c61ab303/content/browser/picture_in_picture/picture_in_picture_window_controller_impl.cc

Project Member

Comment 8 by bugdroid1@chromium.org, May 2 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/87ee01237a14e74e1bf7f6e4ee24ba3ec1e9e13a

commit 87ee01237a14e74e1bf7f6e4ee24ba3ec1e9e13a
Author: Jennifer Apacible <apacible@chromium.org>
Date: Wed May 02 16:14:51 2018

[Picture-in-Picture] Treat WebContents-scoped controller per-tab.

Currently, the PictureInPictureWindowController is reset and torn down
whenever a new video goes into Picture-in-Picture mode. This is less
than ideal since the PictureInPictureWindowController is attached and
scoped to the lifetime of a WebContents via WebContentsUserData.

Rather, we should allow the natural teardown of the controller when
the initiator tabs is destroyed.

BUG:  823172 
Change-Id: I7e13b8bf45899fd4ff4b3dbd34781d04ab15dc1d
Reviewed-on: https://chromium-review.googlesource.com/1034194
Commit-Queue: apacible <apacible@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555415}
[modify] https://crrev.com/87ee01237a14e74e1bf7f6e4ee24ba3ec1e9e13a/chrome/browser/ui/browser.cc
[modify] https://crrev.com/87ee01237a14e74e1bf7f6e4ee24ba3ec1e9e13a/chrome/browser/ui/browser.h
[modify] https://crrev.com/87ee01237a14e74e1bf7f6e4ee24ba3ec1e9e13a/content/browser/picture_in_picture/picture_in_picture_window_controller_impl.cc
[modify] https://crrev.com/87ee01237a14e74e1bf7f6e4ee24ba3ec1e9e13a/content/browser/picture_in_picture/picture_in_picture_window_controller_impl.h
[modify] https://crrev.com/87ee01237a14e74e1bf7f6e4ee24ba3ec1e9e13a/content/public/browser/picture_in_picture_window_controller.h

Project Member

Comment 9 by bugdroid1@chromium.org, May 2 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/73edf5e8f14d396cc21b1a7b58f12ca86b7c99e6

commit 73edf5e8f14d396cc21b1a7b58f12ca86b7c99e6
Author: Jennifer Apacible <apacible@chromium.org>
Date: Wed May 02 16:15:34 2018

[Reland] [Picture in Picture] Pause active videos when leaving PiP mode.

There are two instances where PictureInPictureWindowController
is torn down:
1. Tab is closed (controller is tied to the tab's WebContents). In this
case, the player is also deleted, so there are no behavioral concerns.
2. A video on a different tab enters Picture-in-Picture mode. In this
case, the player from the earlier PiP'd tab may still be playing in that
tab, meaning there may be two(+) videos playing.

This change would pause the initial video, as the user has indicated
they want to PiP a new video.

This behavior will be up for further discussion with UX.

BUG:  823172 
Change-Id: I3e1a58f496f5814a4a8aae40df3de23cd0ee6702
Reviewed-on: https://chromium-review.googlesource.com/1038303
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Commit-Queue: apacible <apacible@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555416}
[modify] https://crrev.com/73edf5e8f14d396cc21b1a7b58f12ca86b7c99e6/content/browser/picture_in_picture/picture_in_picture_window_controller_impl.cc

Project Member

Comment 10 by bugdroid1@chromium.org, May 2 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7a1e33e57709c1dc41957c22b508ad0a1657c6de

commit 7a1e33e57709c1dc41957c22b508ad0a1657c6de
Author: Jennifer Apacible <apacible@chromium.org>
Date: Wed May 02 21:56:50 2018

[Picture in Picture] Exit PiP on media player on controller teardown.

When PictureInPictureWindowController is torn down, we should send a
signal to exit Picture-in-Picture for the client, or media player. This
will take the player

The case where PictureInPictureWindowController is torn down without
the client's initiator WebContents tab closing is when a video in
another tab enters Picture-in-Picture mode.

BUG:  823172 
Change-Id: Id370c9ba088e1ad47b82939cee0df44cd0566233
Reviewed-on: https://chromium-review.googlesource.com/1033467
Commit-Queue: apacible <apacible@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555568}
[modify] https://crrev.com/7a1e33e57709c1dc41957c22b508ad0a1657c6de/content/browser/picture_in_picture/picture_in_picture_window_controller_impl.cc

Project Member

Comment 11 by bugdroid1@chromium.org, May 8 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3174ddaf19b542e2c43265866a5a179827a890d6

commit 3174ddaf19b542e2c43265866a5a179827a890d6
Author: Jennifer Apacible <apacible@chromium.org>
Date: Tue May 08 05:04:41 2018

[Picture in Picture] Close window when starting PiP in another tab.

Currently, we don't close the existing Picture-in-Picture window when
the a new tab (i.e. content::WebContents) initiates Picture-in-Picture
mode. This change tears down Picture-in-Picture mode for the current
video before opening a new one.

This change also pauses the video on user gesture to close. When the
Picture-in-Picture window is closed, the video in tab will not continue
playing, even if it was playing in the window.

BUG:  823172 
Change-Id: I15e4ada29c6447fd15751503e1ad41868a89e71a
Reviewed-on: https://chromium-review.googlesource.com/1044752
Commit-Queue: apacible <apacible@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556692}
[modify] https://crrev.com/3174ddaf19b542e2c43265866a5a179827a890d6/chrome/browser/ui/browser.cc
[modify] https://crrev.com/3174ddaf19b542e2c43265866a5a179827a890d6/content/browser/picture_in_picture/picture_in_picture_window_controller_impl.cc
[modify] https://crrev.com/3174ddaf19b542e2c43265866a5a179827a890d6/content/browser/picture_in_picture/picture_in_picture_window_controller_impl.h

Project Member

Comment 12 by bugdroid1@chromium.org, May 8 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8358509dc2d56e464581ad0ab6b3743e01fb18a0

commit 8358509dc2d56e464581ad0ab6b3743e01fb18a0
Author: Jennifer Apacible <apacible@chromium.org>
Date: Tue May 08 23:46:23 2018

[Picture in Picture] Support PiPing multiple videos on the same tab.

Currently, only one video can be gracefully sent to Picture-in-Picture
mode from each initiator content::WebContents. Media controls do not
work as they are only set once. This change allows us to enter
Picture-in-Picture from more than one video on the same initiator,
though one at a time.

Another patch handles pausing the previously PiP'd video.

BUG:  823172   726619 
Change-Id: Ie9eae108c5c8ce902458aa520b6553042096fb7a
Reviewed-on: https://chromium-review.googlesource.com/1044773
Commit-Queue: apacible <apacible@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557012}
[modify] https://crrev.com/8358509dc2d56e464581ad0ab6b3743e01fb18a0/content/browser/picture_in_picture/picture_in_picture_window_controller_impl.cc

Labels: -Pri-2 M-68 Pri-1
Any work remaining here? should we close?
Status: Fixed (was: Assigned)
Project Member

Comment 15 by bugdroid1@chromium.org, Jun 7 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b4b972a4e5f6135f27049aede9dea948f0df57d4

commit b4b972a4e5f6135f27049aede9dea948f0df57d4
Author: Jennifer Apacible <apacible@chromium.org>
Date: Thu Jun 07 04:41:02 2018

[Picture in Picture] Exit Picture-in-Picture on renderer crash.

Currently, if a WebContents has an active Picture-in-Picture video,
there is no tear down if the renderer crashes. This means that if the
tab crashes, the Picture-in-Picture window is left hanging. This patch
ensures that the WebContentsDelegate exits Picture-in-Picture, if
available.

Bug:  823172 
Change-Id: I47ecd98795a835e7fa8d886d2f4aaa1afce90313
Reviewed-on: https://chromium-review.googlesource.com/1088320
Commit-Queue: apacible <apacible@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565182}
[modify] https://crrev.com/b4b972a4e5f6135f27049aede9dea948f0df57d4/content/browser/web_contents/web_contents_impl.cc

Sign in to add a comment