Add external surface rendering mode

Add external surface rendering mode

In Android, some content can only be rendered directly to
SurfaceFlinger. Chrome's embedded video rendering model cannot render
this kind of content because the video should be composited with web
content in GLRenderer.

This change adds "render-video-on-external-surface" flag to enable
content to be rendered on an external surface.

* TODOs
- Handle location/size changes.
- Figure out a way to determine which content needs to be rendered
directly to SurfaceFlinger.
- Polish up messaging scheme.

BUG=7371621

[ycheo (away), 2012-12-13 05:33:43]

https://codereview.chromium.org/11442056/diff/1/content/common/media/media_pl...
File content/common/media/media_player_messages.h (right):

https://codereview.chromium.org/11442056/diff/1/content/common/media/media_pl...
content/common/media/media_player_messages.h:75: #if defined(OS_ANDROID)
Do we need this? IMO, this file seems to be only for Android.

https://codereview.chromium.org/11442056/diff/1/content/public/common/content...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/11442056/diff/1/content/public/common/content...
content/public/common/content_switches.cc:706: // Set to enable compatibility
with legacy WebView synchronous APIs.
Why do you move this here?

[danakj, 2012-12-13 05:35:42]

https://codereview.chromium.org/11442056/diff/1/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/11442056/diff/1/cc/gl_renderer.cc#newcode944
cc/gl_renderer.cc:944: } else {
if you return at the end of the if() then you don't need the else{}.

[ycheo (away), 2012-12-13 05:54:59]

https://codereview.chromium.org/11442056/diff/1/content/browser/android/media...
File content/browser/android/media_player_manager_android.h (right):

https://codereview.chromium.org/11442056/diff/1/content/browser/android/media...
content/browser/android/media_player_manager_android.h:90: int
video_view_personality_;
IMO, video_view_ has a personality, so this seems redundant.

https://codereview.chromium.org/11442056/diff/1/content/public/common/content...
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/11442056/diff/1/content/public/common/content...
content/public/common/content_switches.h:221: extern const char
kEnableWebViewSynchronousAPIs[];
You don't need to change the order of these constants.

https://codereview.chromium.org/11442056/diff/1/webkit/media/android/webmedia...
File webkit/media/android/webmediaplayer_android.h (right):

https://codereview.chromium.org/11442056/diff/1/webkit/media/android/webmedia...
webkit/media/android/webmediaplayer_android.h:146: void SetIsInVideoView(bool
is_in_video_view);
Please leave some comments.

https://codereview.chromium.org/11442056/diff/1/webkit/media/android/webmedia...
webkit/media/android/webmediaplayer_android.h:233: bool is_in_video_view_;
Please leave some comments.

[wonsik2, 2012-12-17 04:36:05]

PTAL

https://codereview.chromium.org/11442056/diff/1/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/11442056/diff/1/cc/gl_renderer.cc#newcode944
cc/gl_renderer.cc:944: } else {
On 2012/12/13 05:35:42, danakj wrote:
> if you return at the end of the if() then you don't need the else{}.

Done.

https://codereview.chromium.org/11442056/diff/1/content/browser/android/media...
File content/browser/android/media_player_manager_android.h (right):

https://codereview.chromium.org/11442056/diff/1/content/browser/android/media...
content/browser/android/media_player_manager_android.h:90: int
video_view_personality_;
On 2012/12/13 05:55:00, ycheo wrote:
> IMO, video_view_ has a personality, so this seems redundant.

If we don't track personality of a video view, we should call a JNI method to
retrieve personality from the Java video view object. I tried to avoid
unnecessary JNI calls here.

https://codereview.chromium.org/11442056/diff/1/content/common/media/media_pl...
File content/common/media/media_player_messages.h (right):

https://codereview.chromium.org/11442056/diff/1/content/common/media/media_pl...
content/common/media/media_player_messages.h:75: #if defined(OS_ANDROID)
On 2012/12/13 05:33:43, ycheo wrote:
> Do we need this? IMO, this file seems to be only for Android.

True, but other messages are relatively platform agnostic communications, but
this is Android specific communication. I wanted to mark it here.

https://codereview.chromium.org/11442056/diff/1/content/public/common/content...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/11442056/diff/1/content/public/common/content...
content/public/common/content_switches.cc:706: // Set to enable compatibility
with legacy WebView synchronous APIs.
On 2012/12/13 05:33:43, ycheo wrote:
> Why do you move this here?

For alphabetization's sake?

https://codereview.chromium.org/11442056/diff/1/content/public/common/content...
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/11442056/diff/1/content/public/common/content...
content/public/common/content_switches.h:221: extern const char
kEnableWebViewSynchronousAPIs[];
On 2012/12/13 05:55:00, ycheo wrote:
> You don't need to change the order of these constants.

Well I wanted to alphabetize the list.

https://codereview.chromium.org/11442056/diff/1/webkit/media/android/webmedia...
File webkit/media/android/webmediaplayer_android.h (right):

https://codereview.chromium.org/11442056/diff/1/webkit/media/android/webmedia...
webkit/media/android/webmediaplayer_android.h:146: void SetIsInVideoView(bool
is_in_video_view);
On 2012/12/13 05:55:00, ycheo wrote:
> Please leave some comments.

Done.

https://codereview.chromium.org/11442056/diff/1/webkit/media/android/webmedia...
webkit/media/android/webmediaplayer_android.h:233: bool is_in_video_view_;
On 2012/12/13 05:55:00, ycheo wrote:
> Please leave some comments.

Done.

[danakj, 2012-12-17 04:41:49]

+enne for the cc/ changes. i'm not sure if it's problematic that in the
early-out you're not setting the various shader locations for the program.

[ycheo (away), 2012-12-17 04:43:54]

https://codereview.chromium.org/11442056/diff/1/content/browser/android/media...
File content/browser/android/media_player_manager_android.h (right):

https://codereview.chromium.org/11442056/diff/1/content/browser/android/media...
content/browser/android/media_player_manager_android.h:90: int
video_view_personality_;
You can get this from ContentVideoView.personality_ (You may need to add a
getter to do this).

[wonsik2, 2012-12-17 05:24:55]

https://codereview.chromium.org/11442056/diff/1/content/browser/android/media...
File content/browser/android/media_player_manager_android.h (right):

https://codereview.chromium.org/11442056/diff/1/content/browser/android/media...
content/browser/android/media_player_manager_android.h:90: int
video_view_personality_;
On 2012/12/17 04:43:54, ycheo wrote:
> You can get this from ContentVideoView.personality_ (You may need to add a
> getter to do this).

Done.

[ycheo (away), 2012-12-17 06:13:01]

lgtm

[qinmin, 2012-12-17 19:34:06]

https://codereview.chromium.org/11442056/diff/8002/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/11442056/diff/8002/cc/gl_renderer.cc#newcode940
cc/gl_renderer.cc:940: // When texture_id == 0, punch a hole instead of
rendering.
nit: fix indent

https://codereview.chromium.org/11442056/diff/8002/content/browser/android/co...
File content/browser/android/content_video_view.cc (right):

https://codereview.chromium.org/11442056/diff/8002/content/browser/android/co...
content/browser/android/content_video_view.cc:36: if
(!j_content_video_view_.is_null() && personality_ != personality) {
is this going to support both playing an embedded and a fullscreen video at the
same time? Since we only have one contentVideoView per render View, I don't
think this is going to happen.

https://codereview.chromium.org/11442056/diff/8002/content/browser/android/me...
File content/browser/android/media_player_manager_android.cc (left):

https://codereview.chromium.org/11442056/diff/8002/content/browser/android/me...
content/browser/android/media_player_manager_android.cc:208: MediaPlayerBridge*
MediaPlayerManagerAndroid::GetFullscreenPlayer() {
This function name should be changed getVideoViewPlayer.

https://codereview.chromium.org/11442056/diff/8002/content/browser/android/me...
File content/browser/android/media_player_manager_android.cc (right):

https://codereview.chromium.org/11442056/diff/8002/content/browser/android/me...
content/browser/android/media_player_manager_android.cc:97: routing_id(),
player->player_id()));
Just collapse these 2 into a single message, new
MediaPlayerMsg_DidEnterVideoView(personality)

https://codereview.chromium.org/11442056/diff/8002/content/browser/android/me...
content/browser/android/media_player_manager_android.cc:151: if
(video_view_player_id_ != -1) {
no { needed

https://codereview.chromium.org/11442056/diff/8002/content/browser/android/me...
content/browser/android/media_player_manager_android.cc:152:
ExitFromVideoView(video_view_player_id_);
do we pause the player afterwards?

https://codereview.chromium.org/11442056/diff/8002/content/browser/android/me...
content/browser/android/media_player_manager_android.cc:175: if
(video_view_player_id_ != -1) {
no { needed

https://codereview.chromium.org/11442056/diff/8002/content/browser/android/me...
content/browser/android/media_player_manager_android.cc:191: Send(new
MediaPlayerMsg_DidExitVideoView(
combine this message with DidExitFullscreen

https://codereview.chromium.org/11442056/diff/8002/content/common/media/media...
File content/common/media/media_player_messages.h (right):

https://codereview.chromium.org/11442056/diff/8002/content/common/media/media...
content/common/media/media_player_messages.h:75: #if defined(OS_ANDROID)
no need for OS_ANDROID, all the messages here are for android

https://codereview.chromium.org/11442056/diff/8002/content/common/media/media...
content/common/media/media_player_messages.h:77:
IPC_MESSAGE_ROUTED1(MediaPlayerMsg_DidEnterVideoView,
combine these with DidEnterFullscreen

https://codereview.chromium.org/11442056/diff/8002/content/common/media/media...
content/common/media/media_player_messages.h:81:
IPC_MESSAGE_ROUTED1(MediaPlayerMsg_DidExitVideoView,
ditto

https://codereview.chromium.org/11442056/diff/8002/content/renderer/media/web...
File content/renderer/media/webmediaplayer_proxy_impl_android.h (right):

https://codereview.chromium.org/11442056/diff/8002/content/renderer/media/web...
content/renderer/media/webmediaplayer_proxy_impl_android.h:64: void
OnDidExitVideoView(int player_id);
remove onExitFullscreen, use OnDidExitVideoView(bool fullscreen) here

https://codereview.chromium.org/11442056/diff/8002/content/renderer/media/web...
content/renderer/media/webmediaplayer_proxy_impl_android.h:65: void
OnDidEnterVideoView(int player_id);
ditto

https://codereview.chromium.org/11442056/diff/8002/webkit/media/android/webme...
File webkit/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/11442056/diff/8002/webkit/media/android/webme...
webkit/media/android/webmediaplayer_android.cc:380: unsigned int texture_id =
texture_id_;
you also want to change the ctor of this class, to remove the calls to
stream_texture related stuff. Unless you want to support both surfaceview and
surface texture for video at the same time.

https://codereview.chromium.org/11442056/diff/8002/webkit/media/android/webme...
File webkit/media/android/webmediaplayer_impl_android.cc (right):

https://codereview.chromium.org/11442056/diff/8002/webkit/media/android/webme...
webkit/media/android/webmediaplayer_impl_android.cc:135: // We had the video
view surface connected to Android MediaPlayer,
so if i have 2 videos on the page, one is playing, and now i play the 2nd one.
The 2nd one will cause the first one to exit video view, and cause this function
to get called. Should we pause the first one?

[enne (OOO), 2012-12-17 20:38:28]

https://codereview.chromium.org/11442056/diff/8002/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/11442056/diff/8002/cc/gl_renderer.cc#newcode940
cc/gl_renderer.cc:940: // When texture_id == 0, punch a hole instead of
rendering.
What do you mean by punch a hole? Maybe I'm misunderstanding this CL, but I'm
not sure why you'd even generate a quad for this video; it sounded from the
description like you were going to composite this video external to cc.

[jamesr, 2012-12-17 22:35:15]

Where's a bug or higher level description of what this change is trying to
accomplish?  BUG=7371621 doesn't refer to an actual crbug that I can find.

[jamesr, 2012-12-17 23:44:09]

Looking around a bit it appears this is trying to accomplish a punch-under where
the compositor leaves some pixels transparent and the video is rendered by a
plane 'beneath' the compositor's output.  This isn't something we support in the
chromium compositor because it's not something that is (in general anyway)
compatible with web behavior.  In general, we need to be able to render scenes
where the video is in between other composited content.  If you are trying to
handle arbitrary content on the web, trying to render as a punch-out just won't
work no matter what code changes you make in the compositor.  On a more
practical level, this change to gl_renderer will not work even for simple cases
since the compositor may use intermediate FBOs.

If there's more information you can share about the use cases you want to handle
and the constraints you have please feel free to share them with me or with the
chrome-gpu@google.com mailing alias (internal only).

Do not land this patch as is.

[Jinsuk Kim (do not use this), 2012-12-18 00:47:53]

What you describe is correct. What our team(Google TV) is attempting to do
is to add support for rendering premium video contents (such as Netflix) on
Chrome for Android. The contents are encrypted, thus required to be
decrypted/decoded at hardware layer, and the decoded frames must stay
within the dedicated video plane as opposed to being transferred to video
surface provided by Android framework to keep them from the possibility of
piracy. This is why a "hole" needs to be punched so that the video beneath
the graphics plane will be visible to users. Please refer to the
design
doc<https://docs.google.com/a/google.com/document/d/15A62NXPiUwR3mfrQ5TpoOEkcjd8lK-xTaU3tTHNAR9M/edit>here.
Your inputs are welcome.

Ultimately this logic will be enabled on a conditional basis (only when
Chrome is running on Google TV or any other devices that need to handle
encrypted contents).

Thanks,



Jinsuk Kim | Software Engineer | jinsukkim@google.com | 82-2-531-9203




On Tue, Dec 18, 2012 at 8:44 AM, <jamesr@chromium.org> wrote:

> Looking around a bit it appears this is trying to accomplish a punch-under
> where
> the compositor leaves some pixels transparent and the video is rendered by
> a
> plane 'beneath' the compositor's output.  This isn't something we support
> in the
> chromium compositor because it's not something that is (in general anyway)
> compatible with web behavior.  In general, we need to be able to render
> scenes
> where the video is in between other composited content.  If you are trying
> to
> handle arbitrary content on the web, trying to render as a punch-out just
> won't
> work no matter what code changes you make in the compositor.  On a more
> practical level, this change to gl_renderer will not work even for simple
> cases
> since the compositor may use intermediate FBOs.
>
> If there's more information you can share about the use cases you want to
> handle
> and the constraints you have please feel free to share them with me or
> with the
> chrome-gpu@google.com mailing alias (internal only).
>
> Do not land this patch as is.
>
>
https://codereview.chromium.**org/11442056/<https://codereview.chromium.org/1...
>

[jamesr, 2012-12-19 20:30:35]

On 2012/12/18 00:47:53, jinsukkim wrote:
> What you describe is correct. What our team(Google TV) is attempting to do
> is to add support for rendering premium video contents (such as Netflix) on
> Chrome for Android. The contents are encrypted, thus required to be
> decrypted/decoded at hardware layer, and the decoded frames must stay
> within the dedicated video plane as opposed to being transferred to video
> surface provided by Android framework to keep them from the possibility of
> piracy. This is why a "hole" needs to be punched so that the video beneath
> the graphics plane will be visible to users. Please refer to the
> design
>
doc<https://docs.google.com/a/google.com/document/d/15A62NXPiUwR3mfrQ5TpoOEkcjd8lK-xTaU3tTHNAR9M/edit>here.
> Your inputs are welcome.

Many compositor and web platform features require access to the pixels (at the
very least inside the GPU), so I don't think this is a rendering mode we can
support in chromium.

I'll start a thread to discuss possible options.

[wonsik2, 2013-02-12 01:24:12]

CC'ed more people.

[wonsik2, 2013-02-14 14:56:38]

PTAL --- the GLRenderer change is separated to
https://codereview.chromium.org/12255032/ for more focused discussion.

https://codereview.chromium.org/11442056/diff/8002/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/11442056/diff/8002/cc/gl_renderer.cc#newcode940
cc/gl_renderer.cc:940: // When texture_id == 0, punch a hole instead of
rendering.
On 2012/12/17 19:34:06, qinmin wrote:
> nit: fix indent
It seems that 4 spaces are used throughout the file. Please follow up at
https://codereview.chromium.org/12255032/

https://codereview.chromium.org/11442056/diff/8002/cc/gl_renderer.cc#newcode940
cc/gl_renderer.cc:940: // When texture_id == 0, punch a hole instead of
rendering.
On 2012/12/17 20:38:28, enne wrote:
> What do you mean by punch a hole? Maybe I'm misunderstanding this CL, but I'm
> not sure why you'd even generate a quad for this video; it sounded from the
> description like you were going to composite this video external to cc.

Skipping a quad will make the background occlude the video composited outside of
cc. I added more comments: please follow up at
https://codereview.chromium.org/12255032/

https://codereview.chromium.org/11442056/diff/8002/content/browser/android/co...
File content/browser/android/content_video_view.cc (right):

https://codereview.chromium.org/11442056/diff/8002/content/browser/android/co...
content/browser/android/content_video_view.cc:36: if
(!j_content_video_view_.is_null() && personality_ != personality) {
On 2012/12/17 19:34:06, qinmin wrote:
> is this going to support both playing an embedded and a fullscreen video at
the
> same time? Since we only have one contentVideoView per render View, I don't
> think this is going to happen.

Do you think having a separate class/SurfaceView for embedded video makes more
sense? Please let me know. Thanks!

https://codereview.chromium.org/11442056/diff/8002/content/browser/android/me...
File content/browser/android/media_player_manager_android.cc (left):

https://codereview.chromium.org/11442056/diff/8002/content/browser/android/me...
content/browser/android/media_player_manager_android.cc:208: MediaPlayerBridge*
MediaPlayerManagerAndroid::GetFullscreenPlayer() {
On 2012/12/17 19:34:06, qinmin wrote:
> This function name should be changed getVideoViewPlayer. 

This indeed returns fullscreen player since it only returns the instance when it
is in fullscreen mode.

https://codereview.chromium.org/11442056/diff/8002/content/browser/android/me...
File content/browser/android/media_player_manager_android.cc (right):

https://codereview.chromium.org/11442056/diff/8002/content/browser/android/me...
content/browser/android/media_player_manager_android.cc:97: routing_id(),
player->player_id()));
On 2012/12/17 19:34:06, qinmin wrote:
> Just collapse these 2 into a single message, new
> MediaPlayerMsg_DidEnterVideoView(personality)

Done.

https://codereview.chromium.org/11442056/diff/8002/content/browser/android/me...
content/browser/android/media_player_manager_android.cc:151: if
(video_view_player_id_ != -1) {
On 2012/12/17 19:34:06, qinmin wrote:
> no { needed

Done.

https://codereview.chromium.org/11442056/diff/8002/content/browser/android/me...
content/browser/android/media_player_manager_android.cc:152:
ExitFromVideoView(video_view_player_id_);
On 2012/12/17 19:34:06, qinmin wrote:
> do we pause the player afterwards?

Done.

https://codereview.chromium.org/11442056/diff/8002/content/browser/android/me...
content/browser/android/media_player_manager_android.cc:175: if
(video_view_player_id_ != -1) {
On 2012/12/17 19:34:06, qinmin wrote:
> no { needed

Done.

https://codereview.chromium.org/11442056/diff/8002/content/browser/android/me...
content/browser/android/media_player_manager_android.cc:191: Send(new
MediaPlayerMsg_DidExitVideoView(
On 2012/12/17 19:34:06, qinmin wrote:
> combine this message with DidExitFullscreen

Done.

https://codereview.chromium.org/11442056/diff/8002/content/common/media/media...
File content/common/media/media_player_messages.h (right):

https://codereview.chromium.org/11442056/diff/8002/content/common/media/media...
content/common/media/media_player_messages.h:75: #if defined(OS_ANDROID)
On 2012/12/17 19:34:06, qinmin wrote:
> no need for OS_ANDROID, all the messages here are for android

Done.

https://codereview.chromium.org/11442056/diff/8002/content/common/media/media...
content/common/media/media_player_messages.h:77:
IPC_MESSAGE_ROUTED1(MediaPlayerMsg_DidEnterVideoView,
On 2012/12/17 19:34:06, qinmin wrote:
> combine these with DidEnterFullscreen

Done.

https://codereview.chromium.org/11442056/diff/8002/content/common/media/media...
content/common/media/media_player_messages.h:81:
IPC_MESSAGE_ROUTED1(MediaPlayerMsg_DidExitVideoView,
On 2012/12/17 19:34:06, qinmin wrote:
> ditto

Done.

https://codereview.chromium.org/11442056/diff/8002/content/renderer/media/web...
File content/renderer/media/webmediaplayer_proxy_impl_android.h (right):

https://codereview.chromium.org/11442056/diff/8002/content/renderer/media/web...
content/renderer/media/webmediaplayer_proxy_impl_android.h:64: void
OnDidExitVideoView(int player_id);
On 2012/12/17 19:34:06, qinmin wrote:
> remove onExitFullscreen, use OnDidExitVideoView(bool fullscreen) here

Done.

https://codereview.chromium.org/11442056/diff/8002/content/renderer/media/web...
content/renderer/media/webmediaplayer_proxy_impl_android.h:65: void
OnDidEnterVideoView(int player_id);
On 2012/12/17 19:34:06, qinmin wrote:
> ditto

Done.

https://codereview.chromium.org/11442056/diff/8002/webkit/media/android/webme...
File webkit/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/11442056/diff/8002/webkit/media/android/webme...
webkit/media/android/webmediaplayer_android.cc:380: unsigned int texture_id =
texture_id_;
On 2012/12/17 19:34:06, qinmin wrote:
> you also want to change the ctor of this class, to remove the calls to
> stream_texture related stuff. Unless you want to support both surfaceview and
> surface texture for video at the same time.

I think it might make sense to support them both at the same time. I'll keep the
ctor code for now.

https://codereview.chromium.org/11442056/diff/8002/webkit/media/android/webme...
File webkit/media/android/webmediaplayer_impl_android.cc (right):

https://codereview.chromium.org/11442056/diff/8002/webkit/media/android/webme...
webkit/media/android/webmediaplayer_impl_android.cc:135: // We had the video
view surface connected to Android MediaPlayer,
On 2012/12/17 19:34:06, qinmin wrote:
> so if i have 2 videos on the page, one is playing, and now i play the 2nd one.
> The 2nd one will cause the first one to exit video view, and cause this
function
> to get called. Should we pause the first one?

I thought we only allow one MediaPlayer instance to live per renderer process.
Do I need to take care of this case?

[jamesr, 2013-02-20 08:37:15]

If the video is being rendered externally, then you don't want to notify the
compositor when new frames are available since that will simply trigger another
compositing pass that does not contain any useful content updates.

[wonsik2, 2013-03-05 12:40:15]

For Android changes, please review https://codereview.chromium.org/12443003/
instead. Thanks!

Closing this issue.