Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
132233042:
Enable the embedded L1/EME support in WebView. (Closed)
|
Created:
6 years, 11 months ago by ycheo (away) Modified:
6 years, 10 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, Jinsuk Kim, qinmin, Ignacio Solla, Yaron Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionEnable the embedded L1/EME support in WebView.
- Add ExternalVideoSurfaceContainer which handles the external surface for the hole punching in WebView.
- Refactor the callbacks of ContentViewClient on the hole-punching to make them have the single purpose per each function.
BUG=329447
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252571
Patch Set 1 : ExternalVideoSurfaceContainer.java from Clank.git for the reference. #Patch Set 2 : #Patch Set 3 : #
Total comments: 10
Patch Set 4 : Addressed boliu's comments. #Patch Set 5 : Upload error. #Patch Set 6 : #
Total comments: 25
Patch Set 7 : Addressed boliu's comments. #
Total comments: 18
Patch Set 8 : Addressed boliu's comments. #
Total comments: 23
Patch Set 9 : Addressed boliu's comments. #
Total comments: 13
Patch Set 10 : Addressed boliu's comments and rebased. #
Total comments: 11
Patch Set 11 : Addressed tedchoc's comments. #Patch Set 12 : Added ExternalVideoSurfaceDelegate.java. #
Total comments: 8
Patch Set 13 : Addressed tedchoc's comments. #Patch Set 14 : Fix nits. #
Total comments: 6
Patch Set 15 : Addressed tedchoc's comments. #Patch Set 16 : Rebased. #Patch Set 17 #Patch Set 18 : Fixed the findbugs warning. #
Total comments: 4
Patch Set 19 : Addressed boliu's comments. #Patch Set 20 : Rebased. #Messages
Total messages: 52 (0 generated)
PTAL. @qinmin: overall @yfriedman: content/public/android/ @boliue: android_webview/ @erikwright: android_webview/DEPS Thanks!
+benm I wasn't in that meeting last November so don't know what we agreed on, but I'm really concerned about adding a compile time flag and diverging from chrome proper's implementation. (Haven't done a detailed pass yet)
One consequence of this is that the video does not stay in sync with the web contents during scroll/zoom. This will look really bad in apps. Have you given any thought about how to resolve that? Maybe it's possible do this synchronously in webview. The true scroll/zoom is in java on the UI thread and updated only during layout (in theory). Maybe we could re-layout the surface views when webview itself is being layed out to keep things in sync. I haven't looked through all the VIDEO_HOLE-guarded code yet. But it's kind of hard to comment on them since those files are not modified here. Are there any tests? Unfortunately, upstream (ie chromium) instrumentation tests only runs in software, so the video layer will not draw. But maybe hole punching can work in software mode? If not, we'll need tests inside android code. https://codereview.chromium.org/132233042/diff/90001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ExternalVideoSurfaceContainer.java (right): https://codereview.chromium.org/132233042/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ExternalVideoSurfaceContainer.java:16: public class ExternalVideoSurfaceContainer implements SurfaceHolder.Callback { This class should be under android_webview https://codereview.chromium.org/132233042/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ExternalVideoSurfaceContainer.java:27: // TODO(wonsik): extend it to multiple surfaces. Need to fix this first before turning it on by default We can have multiple videos in a single webview. We can also have multiple webviews in a single process, so either this class needs to be per webview instance, or the mContentViewCore var has to go. https://codereview.chromium.org/132233042/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ExternalVideoSurfaceContainer.java:67: mSurfaceView.requestLayout(); factor these 4 lines into a helper method https://codereview.chromium.org/132233042/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ExternalVideoSurfaceContainer.java:81: public void notifyGeometryChange(int playerId, RectF rect) { Need to be very careful that this does not cause a repaint storm. By the time ContentViewCore.updateFrameInfo is called, that webview frame has already been drawn. If we are being careless and laying out the surface view causes the webview itself to repaint, then we could get into a repaint storm. I think at minimum, we need to early out if nothing has changed. https://codereview.chromium.org/132233042/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ExternalVideoSurfaceContainer.java:86: if (mRect == null || mContentViewCore == null || mSurfaceView == null) return; This is extremely confusing. Can we refactor setting mRect into requestExternalVideoSurface, and then this method would not need playerId or rect parameters.
Also how do you manually test EME video?
On 2014/01/22 23:18:35, boliu wrote: > Also how do you manually test EME video? Hi Bo, For the background, you can refer to the following discussion: https://groups.google.com/a/google.com/forum/#!topic/chrome-gpu/eIM1RwarUmk Currently, I found some bug in my CL and I'm fixing it. I'll upload the patchset again with the fix and reflecting your comments. Thanks, Yuncheol
Hi Bo, For the sync issue during scroll/zoom, we're very well aware of your concerns. and luckily the symptoms are not very noticeable in my tests. VIDEO_HOLE code has been used already in Chrome for TV and Eureka. Currently, I don't have any good idea on how to test this function. If you can give some advice or examples regarding this, I would appreciate it. Instead, I summarized the manual test instructions here: http://goo.gl/AGN2wl. I hope this could be helpful to you. Thanks, Yuncheol https://codereview.chromium.org/132233042/diff/90001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ExternalVideoSurfaceContainer.java (right): https://codereview.chromium.org/132233042/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ExternalVideoSurfaceContainer.java:16: public class ExternalVideoSurfaceContainer implements SurfaceHolder.Callback { On 2014/01/22 23:01:07, boliu wrote: > This class should be under android_webview Done. https://codereview.chromium.org/132233042/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ExternalVideoSurfaceContainer.java:27: // TODO(wonsik): extend it to multiple surfaces. On 2014/01/22 23:01:07, boliu wrote: > Need to fix this first before turning it on by default > > We can have multiple videos in a single webview. > > We can also have multiple webviews in a single process, so either this class > needs to be per webview instance, or the mContentViewCore var has to go. The followings are our backgrounds not to support the multiple instances. 1. This hole-punching logic works only for MSE/EME video, not for the normal media playback. 2. To support the multiple SurfaceView seems impractical, because z-order between SurfaceViewes is non-deterministic. 3. Most usecases of MSE/EME video is to play the premium video (that's why it's encrypted) and so it's unlikely to play the multiple MSE/EME videoes at the same time. 4. The hole-punching itself is a stop-gap solution until the proper solution is suggested, so we don't want to put too much efforts on this. https://codereview.chromium.org/132233042/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ExternalVideoSurfaceContainer.java:67: mSurfaceView.requestLayout(); On 2014/01/22 23:01:07, boliu wrote: > factor these 4 lines into a helper method During the refactoring the duplication was gone. https://codereview.chromium.org/132233042/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ExternalVideoSurfaceContainer.java:81: public void notifyGeometryChange(int playerId, RectF rect) { On 2014/01/22 23:01:07, boliu wrote: > Need to be very careful that this does not cause a repaint storm. > > By the time ContentViewCore.updateFrameInfo is called, that webview frame has > already been drawn. If we are being careless and laying out the surface view > causes the webview itself to repaint, then we could get into a repaint storm. > > I think at minimum, we need to early out if nothing has changed. I found that we don't need the call from ContentViewCore.updateFrameInfo any longer. So I removed the call. https://codereview.chromium.org/132233042/diff/90001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ExternalVideoSurfaceContainer.java:86: if (mRect == null || mContentViewCore == null || mSurfaceView == null) return; On 2014/01/22 23:01:07, boliu wrote: > This is extremely confusing. Can we refactor setting mRect into > requestExternalVideoSurface, and then this method would not need playerId or > rect parameters. Refactored.
This can be split into 3 parts: 1) Define VIDEO_HOLE. Split that out into a separate CL 2) Add kMediaDrmEnableNonCompositing switch. We need to find a different way to do this. 3) The rest. I think this can land after a few rounds more of review. Given this won't be turned on widely, I don't care that much about automated testing anymore. It's still in your best interest turn through all of the code and make sure things work ok. I think the only way to automate that is downstream cts tests, since that's where VIDEO_HOLE will be defined. I'll leave how much you want to do there to your judgement. The internal thread mentioned "sandwiching". Wondering what assumptions are made there. There are two cases, 1) the video layer is in front of another layer, and 2) the webview is in front of another view. For 1), you set needs_blending of the trasparent quad to false, but that doesn't necessarily mean DrawQuad::ShouldDrawWithBlending will return false. So does hole punching assumes that ShouldDrawWithBlending will return false in that case? Also I'm not sure what happens when there are multiple render surfaces below the video layer. For 2), I see setZOrderOnTop(false) is removed in the latest patch set (not sure if intentional or not). Either way, does this case work? https://codereview.chromium.org/132233042/diff/260001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java (right): https://codereview.chromium.org/132233042/diff/260001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:39: protected void dispatchDraw(Canvas canvas) {} // Do nothing What does this do exactly? From the public docs, overriding this should skip drawing the children of this view, but there is no children to the SurfaceView. Or is this is making an assumption about the implementation of SurfaceView? Can you document the assumptions? https://codereview.chromium.org/132233042/diff/260001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:43: // TODO(wonsik): extend it to multiple surfaces. On 2014/01/24 11:14:35, Yuncheol Heo wrote: > On 2014/01/22 23:01:07, boliu wrote: > > Need to fix this first before turning it on by default > > > > We can have multiple videos in a single webview. > > > > We can also have multiple webviews in a single process, so either this class > > needs to be per webview instance, or the mContentViewCore var has to go. > > The followings are our backgrounds not to support the multiple instances. > 1. This hole-punching logic works only for MSE/EME video, not for the normal > media playback. > 2. To support the multiple SurfaceView seems impractical, because z-order > between SurfaceViewes is non-deterministic. Ok. Should mention this in the comment. > 3. Most usecases of MSE/EME video is to play the premium video (that's why it's > encrypted) and so it's unlikely to play the multiple MSE/EME videoes at the same > time. > 4. The hole-punching itself is a stop-gap solution until the proper solution is > suggested, so we don't want to put too much efforts on this. Even if we don't support multiple eme videos, we should still define what should happen in this case. Should the first video keep playing? Should the second video kick the first video off? Either way, document your decision. I think right now if there are two videos, they'll just keep releasing each other's surface. https://codereview.chromium.org/132233042/diff/260001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:46: private ContentViewCore mContentViewCore = null; This reference is very scary because it will never be garbage collected if we forget to null it out, and looking at the code, it's not clear we do in "all the cases" (there are a lot). Same goes for mSurfaceView, which might internally hold on to a reference to the webview. Can we put these references in a per-webview object (like AwContentViewClient) and then find another way to maintain the "only 1 surface view" invariant? Maybe just keep count, or use WeakReference? https://codereview.chromium.org/132233042/diff/260001/android_webview/lib/mai... File android_webview/lib/main/aw_main_delegate.cc (right): https://codereview.chromium.org/132233042/diff/260001/android_webview/lib/mai... android_webview/lib/main/aw_main_delegate.cc:86: cl->AppendSwitch(switches::kMediaDrmEnableNonCompositing); Given all the caveats, we can't turn EME on in general for all of webview. We'll need to find a different way to turn this on for the apps that want this feature. And need to make sure that it remains off for all other apps. Also a note that this doesn't link in chromium component build. https://codereview.chromium.org/132233042/diff/260001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/132233042/diff/260001/build/common.gypi#newco... build/common.gypi:2485: 'defines': ['VIDEO_HOLE=1'], This has implications for webview due to all the new code we are going to compile in. Can't just land this blindly. Can you upload a no-op CL touching each instance of VIDEO_HOLE in the code base, so I can review them? (Say add a blank line after each VIDEO_HOLE instance) https://codereview.chromium.org/132233042/diff/260001/content/browser/media/a... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/132233042/diff/260001/content/browser/media/a... content/browser/media/android/browser_media_player_manager.cc:590: ReleaseExternalVideoSurface(player_id); Maybe question for the media owners. Why is this needed for both OnReleaseResources and OnDestroyPlayer? Shouldn't one be enough? https://codereview.chromium.org/132233042/diff/260001/content/browser/media/a... File content/browser/media/android/browser_media_player_manager.h (right): https://codereview.chromium.org/132233042/diff/260001/content/browser/media/a... content/browser/media/android/browser_media_player_manager.h:156: void ReleaseExternalVideoSurface(int player_id); should be private https://codereview.chromium.org/132233042/diff/260001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java (right): https://codereview.chromium.org/132233042/diff/260001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:165: ContentViewCore contentViewCore, int playerId, boolean isRequest, RectF rect) { This is going in the wrong direction imo. If I understand this correctly, this call is now serving 4 purposes: 1) Request a video surface (when isRequest is true) 2) Update the location of the video element in relation to the page (rect) 3) Update the scroll position/zoom of the page 4) Release the surface (when rect is empty) Just split them into 4 calls, and properly document each one. Similar for the generic "notifyExternalSurface" method. Then the code will be much more understandable. Sorry I didn't make that clear in the last round of review. https://codereview.chromium.org/132233042/diff/260001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (left): https://codereview.chromium.org/132233042/diff/260001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2450: getContentViewClient().onGeometryChanged(-1, null); So what happens when the page scrolls/zooms now? It's not clear to me why this is no longer needed https://codereview.chromium.org/132233042/diff/260001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/132233042/diff/260001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3148: this, playerId, isRequest, new RectF(x, y, x + width, y + height)); This is a bit of a layering violation here. We shouldn't need to pass |this| up to the client. ContentViewCores and Clients are 1-to-1. The details of how to get hold of the ContentViewCore reference in AwContentViewClient can stay in android_webview code
https://codereview.chromium.org/132233042/diff/260001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java (right): https://codereview.chromium.org/132233042/diff/260001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:43: // TODO(wonsik): extend it to multiple surfaces. On 2014/01/24 21:56:32, boliu wrote: > Even if we don't support multiple eme videos, we should still define what should > happen in this case. Should the first video keep playing? Should the second > video kick the first video off? Either way, document your decision. > > I think right now if there are two videos, they'll just keep releasing each > other's surface. Misread the code. I think right now the second video kicks the first video off. https://codereview.chromium.org/132233042/diff/260001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (left): https://codereview.chromium.org/132233042/diff/260001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2450: getContentViewClient().onGeometryChanged(-1, null); On 2014/01/24 21:56:32, boliu wrote: > So what happens when the page scrolls/zooms now? It's not clear to me why this > is no longer needed Never mind. Found the renderer code to update geometry on commit.
https://codereview.chromium.org/132233042/diff/260001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (left): https://codereview.chromium.org/132233042/diff/260001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2450: getContentViewClient().onGeometryChanged(-1, null); On 2014/01/25 01:27:39, boliu wrote: > On 2014/01/24 21:56:32, boliu wrote: > > So what happens when the page scrolls/zooms now? It's not clear to me why this > > is no longer needed > > Never mind. Found the renderer code to update geometry on commit. Err, scratch that. Renderer commit is only updating geometry, not page scroll position. Original question stands.
Hi Bo, I refactored the whole code according to your guidance. Could you take a look at this if your intention is reflected correctly? Thanks, Yuncheol https://codereview.chromium.org/132233042/diff/260001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java (right): https://codereview.chromium.org/132233042/diff/260001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:39: protected void dispatchDraw(Canvas canvas) {} // Do nothing On 2014/01/24 21:56:32, boliu wrote: > What does this do exactly? From the public docs, overriding this should skip > drawing the children of this view, but there is no children to the SurfaceView. > > Or is this is making an assumption about the implementation of SurfaceView? Can > you document the assumptions? Done. https://codereview.chromium.org/132233042/diff/260001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:43: // TODO(wonsik): extend it to multiple surfaces. On 2014/01/25 01:27:39, boliu wrote: > On 2014/01/24 21:56:32, boliu wrote: > > Even if we don't support multiple eme videos, we should still define what > should > > happen in this case. Should the first video keep playing? Should the second > > video kick the first video off? Either way, document your decision. > > > > I think right now if there are two videos, they'll just keep releasing each > > other's surface. > > Misread the code. I think right now the second video kicks the first video off. Yes, the second video kicks the first one off. https://codereview.chromium.org/132233042/diff/260001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:46: private ContentViewCore mContentViewCore = null; On 2014/01/24 21:56:32, boliu wrote: > This reference is very scary because it will never be garbage collected if we > forget to null it out, and looking at the code, it's not clear we do in "all the > cases" (there are a lot). > > Same goes for mSurfaceView, which might internally hold on to a reference to the > webview. > > Can we put these references in a per-webview object (like AwContentViewClient) > and then find another way to maintain the "only 1 surface view" invariant? Maybe > just keep count, or use WeakReference? Done. https://codereview.chromium.org/132233042/diff/260001/android_webview/lib/mai... File android_webview/lib/main/aw_main_delegate.cc (right): https://codereview.chromium.org/132233042/diff/260001/android_webview/lib/mai... android_webview/lib/main/aw_main_delegate.cc:86: cl->AppendSwitch(switches::kMediaDrmEnableNonCompositing); On 2014/01/24 21:56:32, boliu wrote: > Given all the caveats, we can't turn EME on in general for all of webview. > > We'll need to find a different way to turn this on for the apps that want this > feature. And need to make sure that it remains off for all other apps. > > Also a note that this doesn't link in chromium component build. Removed. https://codereview.chromium.org/132233042/diff/260001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/132233042/diff/260001/build/common.gypi#newco... build/common.gypi:2485: 'defines': ['VIDEO_HOLE=1'], On 2014/01/24 21:56:32, boliu wrote: > This has implications for webview due to all the new code we are going to > compile in. Can't just land this blindly. Can you upload a no-op CL touching > each instance of VIDEO_HOLE in the code base, so I can review them? (Say add a > blank line after each VIDEO_HOLE instance) I'll send you another CL. https://codereview.chromium.org/132233042/diff/260001/content/browser/media/a... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/132233042/diff/260001/content/browser/media/a... content/browser/media/android/browser_media_player_manager.cc:590: ReleaseExternalVideoSurface(player_id); On 2014/01/24 21:56:32, boliu wrote: > Maybe question for the media owners. > > Why is this needed for both OnReleaseResources and OnDestroyPlayer? Shouldn't > one be enough? OnDestoryPlayer doesn't call OnReleaseResources. @qinmin, Is this intended behavior? https://codereview.chromium.org/132233042/diff/260001/content/browser/media/a... File content/browser/media/android/browser_media_player_manager.h (right): https://codereview.chromium.org/132233042/diff/260001/content/browser/media/a... content/browser/media/android/browser_media_player_manager.h:156: void ReleaseExternalVideoSurface(int player_id); On 2014/01/24 21:56:32, boliu wrote: > should be private Done. https://codereview.chromium.org/132233042/diff/260001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java (right): https://codereview.chromium.org/132233042/diff/260001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:165: ContentViewCore contentViewCore, int playerId, boolean isRequest, RectF rect) { On 2014/01/24 21:56:32, boliu wrote: > This is going in the wrong direction imo. If I understand this correctly, this > call is now serving 4 purposes: > > 1) Request a video surface (when isRequest is true) > 2) Update the location of the video element in relation to the page (rect) > 3) Update the scroll position/zoom of the page > 4) Release the surface (when rect is empty) > > Just split them into 4 calls, and properly document each one. Similar for the > generic "notifyExternalSurface" method. Then the code will be much more > understandable. > > Sorry I didn't make that clear in the last round of review. Done. https://codereview.chromium.org/132233042/diff/260001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (left): https://codereview.chromium.org/132233042/diff/260001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2450: getContentViewClient().onGeometryChanged(-1, null); On 2014/01/25 04:50:13, boliu wrote: > On 2014/01/25 01:27:39, boliu wrote: > > On 2014/01/24 21:56:32, boliu wrote: > > > So what happens when the page scrolls/zooms now? It's not clear to me why > this > > > is no longer needed > > > > Never mind. Found the renderer code to update geometry on commit. > > Err, scratch that. Renderer commit is only updating geometry, not page scroll > position. Original question stands. I was wrong. In case of the scroll, this was not needed, but in the zoom, this is needed. https://codereview.chromium.org/132233042/diff/260001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/132233042/diff/260001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3148: this, playerId, isRequest, new RectF(x, y, x + width, y + height)); On 2014/01/24 21:56:32, boliu wrote: > This is a bit of a layering violation here. We shouldn't need to pass |this| up > to the client. ContentViewCores and Clients are 1-to-1. > > The details of how to get hold of the ContentViewCore reference in > AwContentViewClient can stay in android_webview code Done.
On 2014/01/24 21:56:31, boliu wrote: > This can be split into 3 parts: > > 1) Define VIDEO_HOLE. Split that out into a separate CL > 2) Add kMediaDrmEnableNonCompositing switch. We need to find a different way to > do this. > 3) The rest. I think this can land after a few rounds more of review. > > Given this won't be turned on widely, I don't care that much about automated > testing anymore. It's still in your best interest turn through all of the code > and make sure things work ok. I think the only way to automate that is > downstream cts tests, since that's where VIDEO_HOLE will be defined. I'll leave > how much you want to do there to your judgement. > > The internal thread mentioned "sandwiching". Wondering what assumptions are made > there. There are two cases, 1) the video layer is in front of another layer, and > 2) the webview is in front of another view. > > For 1), you set needs_blending of the trasparent quad to false, but that doesn't > necessarily mean DrawQuad::ShouldDrawWithBlending will return false. So does > hole punching assumes that ShouldDrawWithBlending will return false in that > case? Also I'm not sure what happens when there are multiple render surfaces > below the video layer. @wonsik, Could you answer this question? > > For 2), I see setZOrderOnTop(false) is removed in the latest patch set (not sure > if intentional or not). Either way, does this case work? I removed it, because it is the default setting. > > https://codereview.chromium.org/132233042/diff/260001/android_webview/java/sr... > File > android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java > (right): > > https://codereview.chromium.org/132233042/diff/260001/android_webview/java/sr... > android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:39: > protected void dispatchDraw(Canvas canvas) {} // Do nothing > What does this do exactly? From the public docs, overriding this should skip > drawing the children of this view, but there is no children to the SurfaceView. > > Or is this is making an assumption about the implementation of SurfaceView? Can > you document the assumptions? > > https://codereview.chromium.org/132233042/diff/260001/android_webview/java/sr... > android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:43: > // TODO(wonsik): extend it to multiple surfaces. > On 2014/01/24 11:14:35, Yuncheol Heo wrote: > > On 2014/01/22 23:01:07, boliu wrote: > > > Need to fix this first before turning it on by default > > > > > > We can have multiple videos in a single webview. > > > > > > We can also have multiple webviews in a single process, so either this class > > > needs to be per webview instance, or the mContentViewCore var has to go. > > > > The followings are our backgrounds not to support the multiple instances. > > 1. This hole-punching logic works only for MSE/EME video, not for the normal > > media playback. > > 2. To support the multiple SurfaceView seems impractical, because z-order > > between SurfaceViewes is non-deterministic. > > Ok. Should mention this in the comment. > > > 3. Most usecases of MSE/EME video is to play the premium video (that's why > it's > > encrypted) and so it's unlikely to play the multiple MSE/EME videoes at the > same > > time. > > 4. The hole-punching itself is a stop-gap solution until the proper solution > is > > suggested, so we don't want to put too much efforts on this. > > Even if we don't support multiple eme videos, we should still define what should > happen in this case. Should the first video keep playing? Should the second > video kick the first video off? Either way, document your decision. > > I think right now if there are two videos, they'll just keep releasing each > other's surface. > > https://codereview.chromium.org/132233042/diff/260001/android_webview/java/sr... > android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:46: > private ContentViewCore mContentViewCore = null; > This reference is very scary because it will never be garbage collected if we > forget to null it out, and looking at the code, it's not clear we do in "all the > cases" (there are a lot). > > Same goes for mSurfaceView, which might internally hold on to a reference to the > webview. > > Can we put these references in a per-webview object (like AwContentViewClient) > and then find another way to maintain the "only 1 surface view" invariant? Maybe > just keep count, or use WeakReference? > > https://codereview.chromium.org/132233042/diff/260001/android_webview/lib/mai... > File android_webview/lib/main/aw_main_delegate.cc (right): > > https://codereview.chromium.org/132233042/diff/260001/android_webview/lib/mai... > android_webview/lib/main/aw_main_delegate.cc:86: > cl->AppendSwitch(switches::kMediaDrmEnableNonCompositing); > Given all the caveats, we can't turn EME on in general for all of webview. > > We'll need to find a different way to turn this on for the apps that want this > feature. And need to make sure that it remains off for all other apps. > > Also a note that this doesn't link in chromium component build. > > https://codereview.chromium.org/132233042/diff/260001/build/common.gypi > File build/common.gypi (right): > > https://codereview.chromium.org/132233042/diff/260001/build/common.gypi#newco... > build/common.gypi:2485: 'defines': ['VIDEO_HOLE=1'], > This has implications for webview due to all the new code we are going to > compile in. Can't just land this blindly. Can you upload a no-op CL touching > each instance of VIDEO_HOLE in the code base, so I can review them? (Say add a > blank line after each VIDEO_HOLE instance) > > https://codereview.chromium.org/132233042/diff/260001/content/browser/media/a... > File content/browser/media/android/browser_media_player_manager.cc (right): > > https://codereview.chromium.org/132233042/diff/260001/content/browser/media/a... > content/browser/media/android/browser_media_player_manager.cc:590: > ReleaseExternalVideoSurface(player_id); > Maybe question for the media owners. > > Why is this needed for both OnReleaseResources and OnDestroyPlayer? Shouldn't > one be enough? > > https://codereview.chromium.org/132233042/diff/260001/content/browser/media/a... > File content/browser/media/android/browser_media_player_manager.h (right): > > https://codereview.chromium.org/132233042/diff/260001/content/browser/media/a... > content/browser/media/android/browser_media_player_manager.h:156: void > ReleaseExternalVideoSurface(int player_id); > should be private > > https://codereview.chromium.org/132233042/diff/260001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java > (right): > > https://codereview.chromium.org/132233042/diff/260001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:165: > ContentViewCore contentViewCore, int playerId, boolean isRequest, RectF rect) { > This is going in the wrong direction imo. If I understand this correctly, this > call is now serving 4 purposes: > > 1) Request a video surface (when isRequest is true) > 2) Update the location of the video element in relation to the page (rect) > 3) Update the scroll position/zoom of the page > 4) Release the surface (when rect is empty) > > Just split them into 4 calls, and properly document each one. Similar for the > generic "notifyExternalSurface" method. Then the code will be much more > understandable. > > Sorry I didn't make that clear in the last round of review. > > https://codereview.chromium.org/132233042/diff/260001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (left): > > https://codereview.chromium.org/132233042/diff/260001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2450: > getContentViewClient().onGeometryChanged(-1, null); > So what happens when the page scrolls/zooms now? It's not clear to me why this > is no longer needed > > https://codereview.chromium.org/132233042/diff/260001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (right): > > https://codereview.chromium.org/132233042/diff/260001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3148: > this, playerId, isRequest, new RectF(x, y, x + width, y + height)); > This is a bit of a layering violation here. We shouldn't need to pass |this| up > to the client. ContentViewCores and Clients are 1-to-1. > > The details of how to get hold of the ContentViewCore reference in > AwContentViewClient can stay in android_webview code
https://codereview.chromium.org/132233042/diff/260001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java (right): https://codereview.chromium.org/132233042/diff/260001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:165: ContentViewCore contentViewCore, int playerId, boolean isRequest, RectF rect) { On 2014/01/28 13:08:53, Yuncheol Heo wrote: > On 2014/01/24 21:56:32, boliu wrote: > > This is going in the wrong direction imo. If I understand this correctly, this > > call is now serving 4 purposes: > > > > 1) Request a video surface (when isRequest is true) > > 2) Update the location of the video element in relation to the page (rect) > > 3) Update the scroll position/zoom of the page > > 4) Release the surface (when rect is empty) > > > > Just split them into 4 calls, and properly document each one. Similar for the > > generic "notifyExternalSurface" method. Then the code will be much more > > understandable. > > > > Sorry I didn't make that clear in the last round of review. > > Done. I splited the functions within the browser process. As I checked the history, we've merged the function 1) and 2) to reduce the IPC.
Getting there. My sandwiching question in #8 is still unanswered. https://codereview.chromium.org/132233042/diff/410001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java (right): https://codereview.chromium.org/132233042/diff/410001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:39: // Disable the hole-punching in SurfaceView.dispatchDraw(). This comment still doesn't explain to me how this "disables hole-punching". https://codereview.chromium.org/132233042/diff/410001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:49: private WeakReference<SurfaceView> mSurfaceView = new WeakReference<SurfaceView>(null); The problem we are trying to solve is that ExternalVideoSurfaceContainer should not prevent any of the per-webview objects (AwContents/AwContentViewClient/ContentViewCore) from being garbage collected. So how about splitting the tracking part (the global logic of keeping the "only one surface view") and everything else (which is per-webview) Roughly something like // This class is global class ExternalVideoTracker { void NewContainerCreated(ExternalVideoContainer newContainer) { if (mCurrentContainer) { mCurrentContainer.release(); } mCurrentContainer = newContainer; } WeakReference<ExternalVideoContainer> mCurrentContainer; } // This class is per-webview class ExternalVideoContainer { SurfaceView mSurfaceView; ContentViewCore mContentViewCore; int mPlayerId; // Other strong refs as needed } AwContentViewClient can create the ExternalVideoContainer and can hold to it with a strong reference, and the rest of the logic can move to ExternalVideoContainer. So this way there is only one WeakReference, much easier to understand and reason about. WDYT? https://codereview.chromium.org/132233042/diff/410001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:113: mX = Float.NaN; // to make sure to lay out the surface view. Can we have a reset method that sets everything, not just mX, to sentinel values? https://codereview.chromium.org/132233042/diff/410001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java (right): https://codereview.chromium.org/132233042/diff/410001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:166: public void onRequestExternalVideoSurface( Need javadocs for all public methods. Should mention these are only used for video hole punching. https://codereview.chromium.org/132233042/diff/410001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:167: ViewGroup containerView, int playerId, SurfaceHolder.Callback callback) { ViewGroup is passed from android_webview/ to content/. There is no need to pass it up. Just pass whatever you need into the AwContentViewClient from AwContents, no need to go through the content layer. No need to pass up RenderCoordinates either. ContentViewCore already has an accessor for it, so just call the accessor whenever you need to. https://codereview.chromium.org/132233042/diff/410001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/132233042/diff/410001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2388: Log.d(TAG, "updateFrameInfo: scroll " + scrollOffsetX + ", " + scrollOffsetY Remove debug log https://codereview.chromium.org/132233042/diff/410001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2454: getContentViewClient().onExternalVideoSurfaceRenderCoordinatesChanged( On 2014/01/28 13:08:53, Yuncheol Heo wrote: > I was wrong. > In case of the scroll, this was not needed, but in the zoom, this is needed. I think the test page you were using caused commits on scroll and that's why it was not needed. But in general, scrolls can be all done on the compositor thread and not lead to any commits. Just FYI. https://codereview.chromium.org/132233042/diff/410001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3152: private static class ExternalVideoSurfaceHolderCallback implements SurfaceHolder.Callback { Why did you split this with where the surface view reference is held? This is implementation and should definitely be somewhere in android_webview/.
https://codereview.chromium.org/132233042/diff/260001/content/browser/media/a... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/132233042/diff/260001/content/browser/media/a... content/browser/media/android/browser_media_player_manager.cc:590: ReleaseExternalVideoSurface(player_id); OnReleaseResources happens at tab switching, or app goes back ground. And the video element is still there. OnDestroyPlayer() happens when video element goes away (or src attribute changes), so it is a different from the OnReleaseResources(). On 2014/01/24 21:56:32, boliu wrote: > Maybe question for the media owners. > > Why is this needed for both OnReleaseResources and OnDestroyPlayer? Shouldn't > one be enough?
On 2014/01/28 13:10:46, Yuncheol Heo wrote: > On 2014/01/24 21:56:31, boliu wrote: > > This can be split into 3 parts: > > > > 1) Define VIDEO_HOLE. Split that out into a separate CL > > 2) Add kMediaDrmEnableNonCompositing switch. We need to find a different way > to > > do this. > > 3) The rest. I think this can land after a few rounds more of review. > > > > Given this won't be turned on widely, I don't care that much about automated > > testing anymore. It's still in your best interest turn through all of the code > > and make sure things work ok. I think the only way to automate that is > > downstream cts tests, since that's where VIDEO_HOLE will be defined. I'll > leave > > how much you want to do there to your judgement. > > > > The internal thread mentioned "sandwiching". Wondering what assumptions are > made > > there. There are two cases, 1) the video layer is in front of another layer, > and > > 2) the webview is in front of another view. > > > > For 1), you set needs_blending of the trasparent quad to false, but that > doesn't > > necessarily mean DrawQuad::ShouldDrawWithBlending will return false. So does > > hole punching assumes that ShouldDrawWithBlending will return false in that > > case? Yes. > > Also I'm not sure what happens when there are multiple render surfaces > > below the video layer. If a video hole renders on a render surface and the surface is composed later for a whole page, the video hole doesn't penetrate through render surfaces. > @wonsik, > Could you answer this question?
https://codereview.chromium.org/132233042/diff/410001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java (right): https://codereview.chromium.org/132233042/diff/410001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:39: // Disable the hole-punching in SurfaceView.dispatchDraw(). On 2014/01/28 19:54:31, boliu wrote: > This comment still doesn't explain to me how this "disables hole-punching". Sorry, I can't get your point. What this does is just to override the method not to run the line 343: https://android.googlesource.com/platform/frameworks/base/+/master/core/java/... Could you suggest the comment? https://codereview.chromium.org/132233042/diff/410001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:49: private WeakReference<SurfaceView> mSurfaceView = new WeakReference<SurfaceView>(null); On 2014/01/28 19:54:31, boliu wrote: > The problem we are trying to solve is that ExternalVideoSurfaceContainer should > not prevent any of the per-webview objects > (AwContents/AwContentViewClient/ContentViewCore) from being garbage collected. > So how about splitting the tracking part (the global logic of keeping the "only > one surface view") and everything else (which is per-webview) > > Roughly something like > > // This class is global > class ExternalVideoTracker { > void NewContainerCreated(ExternalVideoContainer newContainer) { > if (mCurrentContainer) { > mCurrentContainer.release(); > } > mCurrentContainer = newContainer; > } > WeakReference<ExternalVideoContainer> mCurrentContainer; > } > > // This class is per-webview > class ExternalVideoContainer { > SurfaceView mSurfaceView; > ContentViewCore mContentViewCore; > int mPlayerId; > // Other strong refs as needed > } > > AwContentViewClient can create the ExternalVideoContainer and can hold to it > with a strong reference, and the rest of the logic can move to > ExternalVideoContainer. So this way there is only one WeakReference, much easier > to understand and reason about. WDYT? Great advice, Thanks! https://codereview.chromium.org/132233042/diff/410001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:113: mX = Float.NaN; // to make sure to lay out the surface view. On 2014/01/28 19:54:31, boliu wrote: > Can we have a reset method that sets everything, not just mX, to sentinel > values? Done. https://codereview.chromium.org/132233042/diff/410001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java (right): https://codereview.chromium.org/132233042/diff/410001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:166: public void onRequestExternalVideoSurface( On 2014/01/28 19:54:31, boliu wrote: > Need javadocs for all public methods. Should mention these are only used for > video hole punching. Done. https://codereview.chromium.org/132233042/diff/410001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:167: ViewGroup containerView, int playerId, SurfaceHolder.Callback callback) { On 2014/01/28 19:54:31, boliu wrote: > ViewGroup is passed from android_webview/ to content/. There is no need to pass > it up. Just pass whatever you need into the AwContentViewClient from AwContents, > no need to go through the content layer. > > No need to pass up RenderCoordinates either. ContentViewCore already has an > accessor for it, so just call the accessor whenever you need to. Done. https://codereview.chromium.org/132233042/diff/410001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/132233042/diff/410001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2388: Log.d(TAG, "updateFrameInfo: scroll " + scrollOffsetX + ", " + scrollOffsetY On 2014/01/28 19:54:31, boliu wrote: > Remove debug log Done. https://codereview.chromium.org/132233042/diff/410001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2454: getContentViewClient().onExternalVideoSurfaceRenderCoordinatesChanged( On 2014/01/28 19:54:31, boliu wrote: > On 2014/01/28 13:08:53, Yuncheol Heo wrote: > > I was wrong. > > In case of the scroll, this was not needed, but in the zoom, this is needed. > > I think the test page you were using caused commits on scroll and that's why it > was not needed. But in general, scrolls can be all done on the compositor thread > and not lead to any commits. Just FYI. Thanks for the info. https://codereview.chromium.org/132233042/diff/410001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3152: private static class ExternalVideoSurfaceHolderCallback implements SurfaceHolder.Callback { On 2014/01/28 19:54:31, boliu wrote: > Why did you split this with where the surface view reference is held? > > This is implementation and should definitely be somewhere in android_webview/. Done.
Almost there. Only blocking thing is dealing with switching ContentViewCore in pop up webview case. Everything else is for style/clarity. https://codereview.chromium.org/132233042/diff/410001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java (right): https://codereview.chromium.org/132233042/diff/410001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:39: // Disable the hole-punching in SurfaceView.dispatchDraw(). On 2014/02/04 11:12:10, Yuncheol Heo wrote: > On 2014/01/28 19:54:31, boliu wrote: > > This comment still doesn't explain to me how this "disables hole-punching". > > Sorry, I can't get your point. > > What this does is just to override the method not to run the line 343: > https://android.googlesource.com/platform/frameworks/base/+/master/core/java/... > > Could you suggest the comment? Something like "SurfaceView.dispatchDraw implementation punches a hole in the view hierarchy. Disable this by making this a no-op." It's not obvious and not even documented that SurfaceView.dispatchDraw punches a hole. So always good to explicitly call out assumptions we make. https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java (right): https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:91: mExternalVideoSurfaceContainer = new ExternalVideoSurfaceContainer(contentViewCore); Let's make this setContentViewCore, and lazily create ExternalVideoSurfaceContainer when it's first needed. We have to make sure that if VIDEO_HOLE is not defined, then the code added in this patch should have no affect on behavior or performance. Also the AwContents-ContentViewCore relationship is not permanent. The ContentViewCore can change for a pop up webview. So we need to deal with that case. I think releasing any surface view in the old ContentViewCore is probably good enough. https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:103: if (mExternalVideoSurfaceContainer != null) { Do we need null these checks here and in PositionChanged? Aren't these methods guaranteed to be called after Request? https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:530: contentViewCore.setZoomControlsDelegate(zoomControlsDelegate); Let's move the contentViewClient.setContentViewCore call here. https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java (right): https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:23: public class ExternalVideoSurfaceContainer implements SurfaceHolder.Callback { You can put a comment here that you guys are owning this file, like next to some of the VIDEO_HOLE defines. https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:47: private int mPlayerId = INVALID_PLAYER_ID; Let me try to enumerate all the states this class is keeping track of, and see if any can be simplified. 1) Whether we have a valid player, currently determined by mPlayerId == INVALID_PLAYER_ID. Currently this maps to onRequest/onRelease, with the snag that onRequest can be called twice with different IDs. 2) Whether we have a surface view, ie mSurfaceView == null. 3) Whether mSurfaceView is added as child of container view. Currently, this is the same as 2) 4) Whether surface is attached to the media player, ie attach/detachExternalVideoSurface. Nothing explicitly tracks this, so I think right now it might be possible to say call detachExternalVideoSurface twice on the same player id. A few proposals for you to consider: a) Split apart 2 and 3, and merge 3 and 4, ie add/remove surface view as child at the same time that the surface is attached/detached from the video player. And also explicitly track this state to avoid repeated calls. b) Merge 1) and 2). When onRequest is called when mPlayerId is valid, go through all the motions of tearing down the current player, ie detach surface player and remove child, then set mPlayerId to invalid, and release the surface view. Then start from scratch again for the new player. Then the state machine looks something like: No player <-> Has player, has surface view, pending surface ready <-> surface ready and attached to player, surface view added as child https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:52: private float mX = Float.NaN; nit: Call initializeCurrentPositionOfSurfaceView in constructor, then don't need to set NaN here. https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:100: private void setActiveContainer() { Make this static and use an explicit argument instead of "this". Also if sActiveContainer == this, then this method can be a no-op. https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:110: private void releaseActiveContainer() { Make this static too. Call it something like releaseIfActiveContainer(Container container) https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:113: sActiveContainer.clear(); This can just be setActiveContainer(null) after refactor above. https://codereview.chromium.org/132233042/diff/430001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java (right): https://codereview.chromium.org/132233042/diff/430001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:169: public void onRequestExternalVideoSurface(int playerId) { Can we define some relationships between callback pair: onRequestExternalVideoSurface onReleaseExternalVideoSurface and call pair: attachExternalVideoSurface detachExternalVideoSurface For callbacks, do we have the guarantee that onRelease will always be called if onRequest is called? Will onRelease be called if I call detachExternalVideoSurface before it? For calls, attachExternalVideoSurface should only be called between onRequest and onRelease, right? Can detach be called after onRelease? Just need some clarifying comments about how these should be used, for example if another content embedder wants to implement video hole.
https://codereview.chromium.org/132233042/diff/410001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java (right): https://codereview.chromium.org/132233042/diff/410001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:39: // Disable the hole-punching in SurfaceView.dispatchDraw(). On 2014/02/04 21:10:08, boliu wrote: > On 2014/02/04 11:12:10, Yuncheol Heo wrote: > > On 2014/01/28 19:54:31, boliu wrote: > > > This comment still doesn't explain to me how this "disables hole-punching". > > > > Sorry, I can't get your point. > > > > What this does is just to override the method not to run the line 343: > > > https://android.googlesource.com/platform/frameworks/base/+/master/core/java/... > > > > Could you suggest the comment? > > Something like "SurfaceView.dispatchDraw implementation punches a hole in the > view hierarchy. Disable this by making this a no-op." > > It's not obvious and not even documented that SurfaceView.dispatchDraw punches a > hole. So always good to explicitly call out assumptions we make. Thanks for the suggestion. https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java (right): https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:91: mExternalVideoSurfaceContainer = new ExternalVideoSurfaceContainer(contentViewCore); On 2014/02/04 21:10:08, boliu wrote: > Let's make this setContentViewCore, and lazily create > ExternalVideoSurfaceContainer when it's first needed. We have to make sure that > if VIDEO_HOLE is not defined, then the code added in this patch should have no > affect on behavior or performance. > > Also the AwContents-ContentViewCore relationship is not permanent. The > ContentViewCore can change for a pop up webview. So we need to deal with that > case. I think releasing any surface view in the old ContentViewCore is probably > good enough. Done. https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:103: if (mExternalVideoSurfaceContainer != null) { On 2014/02/04 21:10:08, boliu wrote: > Do we need null these checks here and in PositionChanged? Aren't these methods > guaranteed to be called after Request? You're right, but, the change by the upper comment introduces the necessity of these checks. https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:530: contentViewCore.setZoomControlsDelegate(zoomControlsDelegate); On 2014/02/04 21:10:08, boliu wrote: > Let's move the contentViewClient.setContentViewCore call here. Hmm, it looks strange. In the scope of this method, we can't know whether contentViewCore will be assigned to mContentViewCore yet. https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java (right): https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:23: public class ExternalVideoSurfaceContainer implements SurfaceHolder.Callback { On 2014/02/04 21:10:08, boliu wrote: > You can put a comment here that you guys are owning this file, like next to some > of the VIDEO_HOLE defines. Done. https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:47: private int mPlayerId = INVALID_PLAYER_ID; On 2014/02/04 21:10:08, boliu wrote: > Let me try to enumerate all the states this class is keeping track of, and see > if any can be simplified. > > 1) Whether we have a valid player, currently determined by mPlayerId == > INVALID_PLAYER_ID. Currently this maps to onRequest/onRelease, with the snag > that onRequest can be called twice with different IDs. > > 2) Whether we have a surface view, ie mSurfaceView == null. > > 3) Whether mSurfaceView is added as child of container view. Currently, this is > the same as 2) > > 4) Whether surface is attached to the media player, ie > attach/detachExternalVideoSurface. Nothing explicitly tracks this, so I think > right now it might be possible to say call detachExternalVideoSurface twice on > the same player id. > > A few proposals for you to consider: > > a) Split apart 2 and 3, and merge 3 and 4, ie add/remove surface view as child > at the same time that the surface is attached/detached from the video player. > And also explicitly track this state to avoid repeated calls. > > b) Merge 1) and 2). When onRequest is called when mPlayerId is valid, go through > all the motions of tearing down the current player, ie detach surface player and > remove child, then set mPlayerId to invalid, and release the surface view. Then > start from scratch again for the new player. > > Then the state machine looks something like: > No player <-> Has player, has surface view, pending surface ready <-> surface > ready and attached to player, surface view added as child Thanks for the suggestion. But, we can't do a), because the callback can be called after the SurfaceView is attached. Done for b). https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:52: private float mX = Float.NaN; On 2014/02/04 21:10:08, boliu wrote: > nit: Call initializeCurrentPositionOfSurfaceView in constructor, then don't need > to set NaN here. Done. https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:100: private void setActiveContainer() { On 2014/02/04 21:10:08, boliu wrote: > Make this static and use an explicit argument instead of "this". > > Also if sActiveContainer == this, then this method can be a no-op. Done. https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:110: private void releaseActiveContainer() { On 2014/02/04 21:10:08, boliu wrote: > Make this static too. Call it something like releaseIfActiveContainer(Container > container) Done. https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:113: sActiveContainer.clear(); On 2014/02/04 21:10:08, boliu wrote: > This can just be setActiveContainer(null) after refactor above. Done. https://codereview.chromium.org/132233042/diff/430001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java (right): https://codereview.chromium.org/132233042/diff/430001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:169: public void onRequestExternalVideoSurface(int playerId) { On 2014/02/04 21:10:08, boliu wrote: > Can we define some relationships between > > callback pair: > onRequestExternalVideoSurface > onReleaseExternalVideoSurface > > and call pair: > attachExternalVideoSurface > detachExternalVideoSurface > > For callbacks, do we have the guarantee that onRelease will always be called if > onRequest is called? Will onRelease be called if I call > detachExternalVideoSurface before it? yes. > > For calls, attachExternalVideoSurface should only be called between onRequest > and onRelease, right? Can detach be called after onRelease? yes. but, attachExternalVideoSurface() and detachExternalVideoSurface() also can be called not only when onRequest or onRelease, but also when the surface will be hidden or re-shown (by moving the activity to the foreground and the background). > > Just need some clarifying comments about how these should be used, for example > if another content embedder wants to implement video hole. Done.
lgtm % last few comments https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:530: contentViewCore.setZoomControlsDelegate(zoomControlsDelegate); On 2014/02/05 06:17:05, Yuncheol Heo wrote: > On 2014/02/04 21:10:08, boliu wrote: > > Let's move the contentViewClient.setContentViewCore call here. > > Hmm, it looks strange. > In the scope of this method, we can't know whether contentViewCore will be > assigned to mContentViewCore yet. Actually this method is static, nothing here says contentViewClient is an instance var of the object. I guess the strange bit that the caller is passing an instance var as contentViewClient. But there's reason for that too. Ok to leave this as is. https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java (right): https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:47: private int mPlayerId = INVALID_PLAYER_ID; On 2014/02/05 06:17:05, Yuncheol Heo wrote: > On 2014/02/04 21:10:08, boliu wrote: > > Let me try to enumerate all the states this class is keeping track of, and see > > if any can be simplified. > > > > 1) Whether we have a valid player, currently determined by mPlayerId == > > INVALID_PLAYER_ID. Currently this maps to onRequest/onRelease, with the snag > > that onRequest can be called twice with different IDs. > > > > 2) Whether we have a surface view, ie mSurfaceView == null. > > > > 3) Whether mSurfaceView is added as child of container view. Currently, this > is > > the same as 2) > > > > 4) Whether surface is attached to the media player, ie > > attach/detachExternalVideoSurface. Nothing explicitly tracks this, so I think > > right now it might be possible to say call detachExternalVideoSurface twice on > > the same player id. > > > > A few proposals for you to consider: > > > > a) Split apart 2 and 3, and merge 3 and 4, ie add/remove surface view as child > > at the same time that the surface is attached/detached from the video player. > > And also explicitly track this state to avoid repeated calls. > > > > b) Merge 1) and 2). When onRequest is called when mPlayerId is valid, go > through > > all the motions of tearing down the current player, ie detach surface player > and > > remove child, then set mPlayerId to invalid, and release the surface view. > Then > > start from scratch again for the new player. > > > > Then the state machine looks something like: > > No player <-> Has player, has surface view, pending surface ready <-> surface > > ready and attached to player, surface view added as child > > Thanks for the suggestion. > But, we can't do a), because the callback can be called after the SurfaceView is > attached. You mean the surfaceCreated callback? Can we move attaching the SurfaceView to view tree to the surfaceCreated callback then? > Done for b). https://codereview.chromium.org/132233042/diff/480001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java (right): https://codereview.chromium.org/132233042/diff/480001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:101: if (mExternalVideoSurfaceContainer == null && mContentViewCore != null) { mContentViewCore can never be null when this is called https://codereview.chromium.org/132233042/diff/480001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:104: if (mExternalVideoSurfaceContainer != null) { Won't need this check after removing the mContentViewCore check above. https://codereview.chromium.org/132233042/diff/480001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:111: if (mExternalVideoSurfaceContainer != null) { Can you add a comment here and below that the check should only apply for the pop up webview case, ie mContentViewCore changes midway? https://codereview.chromium.org/132233042/diff/480001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java (right): https://codereview.chromium.org/132233042/diff/480001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:72: setActiveContainer(this); style: I think it's clearer to handle mPlayerId != INVALID_PLAYER_ID here explicitly rather than implicitly through the tear down in setActiveContainer. Then setActiveContainer can be no-op if sActiveContainer == this. So sActiveContainer keeps track of active container, but the active player is tracked in the container itself. https://codereview.chromium.org/132233042/diff/480001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:126: // SurfaceHoder.surfaceCreated() will be called after the SurfaceView is attached to nice https://codereview.chromium.org/132233042/diff/480001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:134: mContentViewCore.getContainerView().removeView(mSurfaceView); Do we need to check if mSurfaceView is already null? https://codereview.chromium.org/132233042/diff/480001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java (right): https://codereview.chromium.org/132233042/diff/480001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:168: // 2) Whenever the size of the video element is changed, it'll notify through or when video element is moved in the page
https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java (right): https://codereview.chromium.org/132233042/diff/430001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:47: private int mPlayerId = INVALID_PLAYER_ID; On 2014/02/05 07:19:14, boliu wrote: > On 2014/02/05 06:17:05, Yuncheol Heo wrote: > > On 2014/02/04 21:10:08, boliu wrote: > > > Let me try to enumerate all the states this class is keeping track of, and > see > > > if any can be simplified. > > > > > > 1) Whether we have a valid player, currently determined by mPlayerId == > > > INVALID_PLAYER_ID. Currently this maps to onRequest/onRelease, with the snag > > > that onRequest can be called twice with different IDs. > > > > > > 2) Whether we have a surface view, ie mSurfaceView == null. > > > > > > 3) Whether mSurfaceView is added as child of container view. Currently, this > > is > > > the same as 2) > > > > > > 4) Whether surface is attached to the media player, ie > > > attach/detachExternalVideoSurface. Nothing explicitly tracks this, so I > think > > > right now it might be possible to say call detachExternalVideoSurface twice > on > > > the same player id. > > > > > > A few proposals for you to consider: > > > > > > a) Split apart 2 and 3, and merge 3 and 4, ie add/remove surface view as > child > > > at the same time that the surface is attached/detached from the video > player. > > > And also explicitly track this state to avoid repeated calls. > > > > > > b) Merge 1) and 2). When onRequest is called when mPlayerId is valid, go > > through > > > all the motions of tearing down the current player, ie detach surface player > > and > > > remove child, then set mPlayerId to invalid, and release the surface view. > > Then > > > start from scratch again for the new player. > > > > > > Then the state machine looks something like: > > > No player <-> Has player, has surface view, pending surface ready <-> > surface > > > ready and attached to player, surface view added as child > > > > Thanks for the suggestion. > > But, we can't do a), because the callback can be called after the SurfaceView > is > > attached. > > You mean the surfaceCreated callback? Can we move attaching the SurfaceView to > view tree to the surfaceCreated callback then? > > > Done for b). > Sorry, let me explain this more clearly. The surfaceCreated callback will be triggered after the SurfaceView to the container view. So, if we move the attaching to the callback, then, the callback can't be triggered. https://codereview.chromium.org/132233042/diff/480001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java (right): https://codereview.chromium.org/132233042/diff/480001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:101: if (mExternalVideoSurfaceContainer == null && mContentViewCore != null) { On 2014/02/05 07:19:15, boliu wrote: > mContentViewCore can never be null when this is called Done. Instead, I added the assertion in the constructor. https://codereview.chromium.org/132233042/diff/480001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:104: if (mExternalVideoSurfaceContainer != null) { On 2014/02/05 07:19:15, boliu wrote: > Won't need this check after removing the mContentViewCore check above. Done. https://codereview.chromium.org/132233042/diff/480001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:111: if (mExternalVideoSurfaceContainer != null) { On 2014/02/05 07:19:15, boliu wrote: > Can you add a comment here and below that the check should only apply for the > pop up webview case, ie mContentViewCore changes midway? Done. https://codereview.chromium.org/132233042/diff/480001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java (right): https://codereview.chromium.org/132233042/diff/480001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:72: setActiveContainer(this); On 2014/02/05 07:19:15, boliu wrote: > style: I think it's clearer to handle mPlayerId != INVALID_PLAYER_ID here > explicitly rather than implicitly through the tear down in setActiveContainer. > Then setActiveContainer can be no-op if sActiveContainer == this. So > sActiveContainer keeps track of active container, but the active player is > tracked in the container itself. Done. https://codereview.chromium.org/132233042/diff/480001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:134: mContentViewCore.getContainerView().removeView(mSurfaceView); On 2014/02/05 07:19:15, boliu wrote: > Do we need to check if mSurfaceView is already null? I don't think it can happen, because removeSurfaceView() is called only from setActiveContainer() and it will not call removeSurfaceView() if the active container points null. https://codereview.chromium.org/132233042/diff/480001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java (right): https://codereview.chromium.org/132233042/diff/480001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:168: // 2) Whenever the size of the video element is changed, it'll notify through On 2014/02/05 07:19:15, boliu wrote: > or when video element is moved in the page Done.
PTAL. @yfriedman: content/browser/android/ content/public/android/java/src/org/chromium/content/browser/ @sky: content/browser/media/android/browser_media_player_manager.cc content/browser/web_contents/
I'm not a good owner for the android side of things. There should be android owners, if not update the OWNERS file appropriately.
Just a question. https://codereview.chromium.org/132233042/diff/650001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java (right): https://codereview.chromium.org/132233042/diff/650001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:30: // So we need to disable its hole-punching logic. What does it mean that WebView does hole-punching by itself? Is this because WebView already has a second SurfaceView (ie. inside ContentViewRenderView) to render the WebContents and you're afraid that by punching a second hole you might clear the controls? I'm just trying to understand whether we need something similar for our own content embedder.
https://codereview.chromium.org/132233042/diff/650001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java (right): https://codereview.chromium.org/132233042/diff/650001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:30: // So we need to disable its hole-punching logic. Not owner of video hole code, so my answer might not be authoritative... On 2014/02/05 23:47:00, igsolla wrote: > What does it mean that WebView does hole-punching by itself? It means compositor is already leaving the rectangle where the video would be blank, so there is no need for this surface view to do it: https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/video_la... > Is this because > WebView already has a second SurfaceView (ie. inside ContentViewRenderView) FYI Webview rendering is different from chrome and doesn't use ContentViewRenderView, but that doesn't change anything you are asking here. > to > render the WebContents and you're afraid that by punching a second hole you > might clear the controls? I believe yes > I'm just trying to understand whether we need > something similar for our own content embedder. Compositor (ie video_layer_impl.cc) is used in content shell too, so you should have a hole in ContentViewRenderView already
PTAL, tedchoc@ content/browser/android/ content/browser/web_contents/ xhwang@ content/browser/media/android/
You need more CL description about what you are doing in this CL. content/browser/media/android/ rubberstamp lgtm
On 2014/02/08 05:16:37, xhwang wrote: > You need more CL description about what you are doing in this CL. > > content/browser/media/android/ rubberstamp lgtm Updated the CL description.
+qinmin because this touches media stuff. overall, this seems fine. Nits, but lgtm for OWNERS https://codereview.chromium.org/132233042/diff/650001/content/browser/web_con... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/132233042/diff/650001/content/browser/web_con... content/browser/web_contents/web_contents_view_android.cc:62: void WebContentsViewAndroid::ReleaseExternalVideoSurface(int player_id) { blank line between methods https://codereview.chromium.org/132233042/diff/650001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java (right): https://codereview.chromium.org/132233042/diff/650001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:164: // Please contact ycheo@chromium.org (or wonsik@) if you have any questions on the following. Not something for this change, but I'd like to see this split out of ContentViewClient. It seems like this enough functionality to deserve it's own delegate. https://codereview.chromium.org/132233042/diff/650001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:170: // 3) Whenever the page to contain the video element is scrolled or zoomed, "the page [that] contain[s]" https://codereview.chromium.org/132233042/diff/650001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:173: // 5) If the player want not to use the surface any more, it'll call If the player [no longer needs] the surface anymore, ... https://codereview.chromium.org/132233042/diff/650001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/132233042/diff/650001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3271: playerId, new RectF(x, y, x + width, y + height)); what coordinate space are these values (css pixels, absolute screen pixels)? Any reason you are converting this to a RectF? It will result in a java alloc that unless you need, I would probably drop it and pass the 4 values.
@boliu, @tedchoc, After refactoring, ContentViewCore owns ExternalVideoSurfaceContainers now. IMO, it looks fine. Could you double-check if it's okay? Thanks, Yuncheol https://codereview.chromium.org/132233042/diff/650001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java (right): https://codereview.chromium.org/132233042/diff/650001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:164: // Please contact ycheo@chromium.org (or wonsik@) if you have any questions on the following. On 2014/02/11 18:05:47, Ted C wrote: > Not something for this change, but I'd like to see this split out of > ContentViewClient. It seems like this enough functionality to deserve it's own > delegate. Done. https://codereview.chromium.org/132233042/diff/650001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:170: // 3) Whenever the page to contain the video element is scrolled or zoomed, On 2014/02/11 18:05:47, Ted C wrote: > "the page [that] contain[s]" Done. https://codereview.chromium.org/132233042/diff/650001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:173: // 5) If the player want not to use the surface any more, it'll call On 2014/02/11 18:05:47, Ted C wrote: > If the player [no longer needs] the surface anymore, ... Done. https://codereview.chromium.org/132233042/diff/650001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/132233042/diff/650001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3271: playerId, new RectF(x, y, x + width, y + height)); On 2014/02/11 18:05:47, Ted C wrote: > what coordinate space are these values (css pixels, absolute screen pixels)? css pixels, updates in the javadoc. > > Any reason you are converting this to a RectF? It will result in a java alloc > that unless you need, I would probably drop it and pass the 4 values. Done.
Missing ExternalVideoSurfaceDelegate.java file?
On 2014/02/14 17:40:39, boliu wrote: > Missing ExternalVideoSurfaceDelegate.java file? Oops!, Sorry, I added the missing file.
Still lgtm Nice clena up :)
The plumbing through WebContentsViewAndroid is the one remaining thing I think maybe should be fixed for this change. Pulling out the stuff from ContentViewCore would be too big for this and was just a suggestion for a follow up change to further make this a bit cleaner. https://codereview.chromium.org/132233042/diff/970001/content/browser/media/a... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/132233042/diff/970001/content/browser/media/a... content/browser/media/android/browser_media_player_manager.cc:338: static_cast<WebContentsViewAndroid*>(web_contents_->GetView()); Any reason you are going through WebContentsViewAndroid instead of calling these methods on the ContentViewCore directly? ContentViewCoreImpl* content_view_core = GetContentViewCore(); if (content_view_core) content_view_core->ReleaseExternalVideoSurface(player_id); Seems like you could remove the methods entirely from the WebContentsViewAndroid. https://codereview.chromium.org/132233042/diff/970001/content/browser/web_con... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/132233042/diff/970001/content/browser/web_con... content/browser/web_contents/web_contents_view_android.cc:61: } still missing blank lines between the methods. https://codereview.chromium.org/132233042/diff/970001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/132233042/diff/970001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2121: public void setExternalVideoSurfaceDelegate(ExternalVideoSurfaceDelegate delegate) { although other places don't have it, all public methods should have javadoc. https://codereview.chromium.org/132233042/diff/970001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2581: mExternalVideoSurfaceDelegate.onRenderCoordinatesChanged(); Hmm...this is so close to not needing to be in ContentViewCore at all. I wonder if this could be moved to a WebContents helper (i.e. just hung off of a WebContents using the SupportsUserData trick). Then all the JNI plumbing could be moved out of content view core, which is already a dumping ground.
To reviewers, Please hold the review for a while, I'm refactoring the code as Ted recommended. I'll upload it soon as soon as I finish it. Thanks, Yuncheol
To reviews, Thanks to tedchoc, I refactored the code, and IMO it looks quite better. Please continue to review. I added some reviewers for the newly added files as follows: @erikwright: android_webview/DEPS @brettw: content/content_browser.gypi https://codereview.chromium.org/132233042/diff/970001/content/browser/media/a... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/132233042/diff/970001/content/browser/media/a... content/browser/media/android/browser_media_player_manager.cc:338: static_cast<WebContentsViewAndroid*>(web_contents_->GetView()); On 2014/02/14 18:32:52, Ted C wrote: > Any reason you are going through WebContentsViewAndroid instead of calling these > methods on the ContentViewCore directly? > > ContentViewCoreImpl* content_view_core = GetContentViewCore(); > if (content_view_core) > content_view_core->ReleaseExternalVideoSurface(player_id); > > Seems like you could remove the methods entirely from the > WebContentsViewAndroid. After refactoring, we don't need to apply this. https://codereview.chromium.org/132233042/diff/970001/content/browser/web_con... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/132233042/diff/970001/content/browser/web_con... content/browser/web_contents/web_contents_view_android.cc:61: } On 2014/02/14 18:32:52, Ted C wrote: > still missing blank lines between the methods. Removed. https://codereview.chromium.org/132233042/diff/970001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/132233042/diff/970001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2121: public void setExternalVideoSurfaceDelegate(ExternalVideoSurfaceDelegate delegate) { On 2014/02/14 18:32:52, Ted C wrote: > although other places don't have it, all public methods should have javadoc. Removed. https://codereview.chromium.org/132233042/diff/970001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2581: mExternalVideoSurfaceDelegate.onRenderCoordinatesChanged(); On 2014/02/14 18:32:52, Ted C wrote: > Hmm...this is so close to not needing to be in ContentViewCore at all. > > I wonder if this could be moved to a WebContents helper (i.e. just hung off of a > WebContents using the SupportsUserData trick). Then all the JNI plumbing could > be moved out of content view core, which is already a dumping ground. Done.
lgtm
lgtm w/ a few comments https://codereview.chromium.org/132233042/diff/1090001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java (right): https://codereview.chromium.org/132233042/diff/1090001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:19: import java.lang.ref.WeakReference; one too many spaces after import https://codereview.chromium.org/132233042/diff/1090001/content/public/browser... File content/public/browser/android/external_video_surface_container.h (right): https://codereview.chromium.org/132233042/diff/1090001/content/public/browser... content/public/browser/android/external_video_surface_container.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/132233042/diff/1090001/content/public/browser... content/public/browser/android/external_video_surface_container.h:20: class CONTENT_EXPORT ExternalVideoSurfaceContainer { Some documentation on this class would be good.
https://codereview.chromium.org/132233042/diff/1090001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java (right): https://codereview.chromium.org/132233042/diff/1090001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:19: import java.lang.ref.WeakReference; On 2014/02/18 20:03:14, Ted C wrote: > one too many spaces after import Done. https://codereview.chromium.org/132233042/diff/1090001/content/public/browser... File content/public/browser/android/external_video_surface_container.h (right): https://codereview.chromium.org/132233042/diff/1090001/content/public/browser... content/public/browser/android/external_video_surface_container.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/02/18 20:03:14, Ted C wrote: > no (c) Done. https://codereview.chromium.org/132233042/diff/1090001/content/public/browser... content/public/browser/android/external_video_surface_container.h:20: class CONTENT_EXPORT ExternalVideoSurfaceContainer { On 2014/02/18 20:03:14, Ted C wrote: > Some documentation on this class would be good. Done.
Doesn't need to block this CL landing, but is it possible for a user agent to detect whether holepunching is available or not? Would it be wise to add a mechanism?
On 2014/02/19 12:31:05, benm wrote: > Doesn't need to block this CL landing, but is it possible for a user agent to > detect whether holepunching is available or not? Would it be wise to add a > mechanism? IMO, it is not need, because usually the premium contents partners (like Netflix, Amazon) want to control the distribution of their app per the model basis: they'll enable their apps only for the model approved by themselves. Currently the model is already included in UA and that could be enough.
@brettw, ping!
.gypi lgtm
erickwright@ seems OOO. @joth, Could you check the change on android_webview/DEPS? Thanks, Yuncheol
@joi, Could you check the change on android_webview/DEPS? Thanks, Yuncheol
https://codereview.chromium.org/132233042/diff/1360001/android_webview/DEPS File android_webview/DEPS (right): https://codereview.chromium.org/132233042/diff/1360001/android_webview/DEPS#n... android_webview/DEPS:1: # Please include joth@ and (joi@ or erikwright@) on the review for any changes Turns out joi/erikwright were added to coordinate with component effort, way back when the src/android_webview directory was first created: https://chromiumcodereview.appspot.com/10825155/ Not sure how relevant this is still, since android_webview is pretty stable now. And joth has already left the team. https://codereview.chromium.org/132233042/diff/1360001/android_webview/DEPS#n... android_webview/DEPS:13: "+media/base/media_switches.h", # For media command line switches. move this to android_webview/lib/DEPS
https://codereview.chromium.org/132233042/diff/1360001/android_webview/DEPS File android_webview/DEPS (right): https://codereview.chromium.org/132233042/diff/1360001/android_webview/DEPS#n... android_webview/DEPS:1: # Please include joth@ and (joi@ or erikwright@) on the review for any changes On 2014/02/20 23:43:10, boliu wrote: > Turns out joi/erikwright were added to coordinate with component effort, way > back when the src/android_webview directory was first created: > https://chromiumcodereview.appspot.com/10825155/ Not sure how relevant this is > still, since android_webview is pretty stable now. > > And joth has already left the team. Thank for letting me know this. https://codereview.chromium.org/132233042/diff/1360001/android_webview/DEPS#n... android_webview/DEPS:13: "+media/base/media_switches.h", # For media command line switches. On 2014/02/20 23:43:10, boliu wrote: > move this to android_webview/lib/DEPS Done.
aw DEPS lgtm. I think you don't need to wait for Joi for DEPS, but doesn't hurt either if you are not desperate for time. Also noticed you added changes build/common.gypi and lgtm for that as well. But remember to announce these additional changes next time.
android_webview/lib/DEPS LGTM
The CQ bit was checked by ycheo@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ycheo@chromium.org/132233042/1480001
Message was sent while issue was closed.
Change committed as 252571 |
