Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
103583005:
Restart fullscreen video playback when switching back from background (Closed)
|
Created:
7 years ago by kbalazs Modified:
6 years, 10 months ago CC:
chromium-reviews, fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org, android-webview-reviews_chromium.org, Peter Beverloo, scherkus (not reviewing), lgombos Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionRestart fullscreen video playback when switching back from background
This cl is supposed to improve user experience. The idea is that it
is likely that the intention of the user is to continue video playback
when switching back to the browser. Additionally this unifies the behavior
of switching back from background and from device lock. The new behavior
matches with Safari on IOS and Chrome on desktop.
Now we destroy the fullscreen view when the application is stopped and
recreate it when the application comes back to foreground.
Bug=323697
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242570
Patch Set 1 #
Total comments: 12
Patch Set 2 : renamings according to review #
Total comments: 2
Patch Set 3 : removed the buggy changes #Patch Set 4 : removed one more buggy line and the fullscreen_player_is_released_ member which is not necessary now #
Total comments: 1
Patch Set 5 : new approach, handle everything at browser side #Patch Set 6 : handle everything at browser side #
Total comments: 4
Patch Set 7 : fix comments, keep surfaceDestroyed #
Total comments: 2
Patch Set 8 : fixed nits #
Total comments: 2
Patch Set 9 : fixed scope comment #Patch Set 10 : fixed comment without whitespace noise #
Total comments: 2
Messages
Total messages: 39 (0 generated)
https://codereview.chromium.org/103583005/diff/1/content/renderer/media/andro... File content/renderer/media/android/renderer_media_player_manager.cc (left): https://codereview.chromium.org/103583005/diff/1/content/renderer/media/andro... content/renderer/media/android/renderer_media_player_manager.cc:326: void RendererMediaPlayerManager::ReleaseVideoResources() { I don't think this is right. This function is not only for fullscreen video, it also works for embedded video. After your change, now it only pauses the fullscreen video， and not actually releasing the MediaPlayer resources. This will cause issues for other apps. We should still release the media player, and reacquire the resource when we restart the app. Calling MediaPlayerBridge.SetVideoSurface() will automatically create the new MediaPlayer, so the only thing we need to do is call enterfullscreen() when app restarts.
https://codereview.chromium.org/103583005/diff/1/content/renderer/media/andro... File content/renderer/media/android/renderer_media_player_manager.cc (left): https://codereview.chromium.org/103583005/diff/1/content/renderer/media/andro... content/renderer/media/android/renderer_media_player_manager.cc:326: void RendererMediaPlayerManager::ReleaseVideoResources() { On 2013/12/05 23:02:34, qinmin wrote: > I don't think this is right. This function is not only for fullscreen video, it > also works for embedded video. After your change, now it only pauses the > fullscreen video， and not actually releasing the MediaPlayer resources. This > will cause issues for other apps. > We should still release the media player, and reacquire the resource when we > restart the app. Calling MediaPlayerBridge.SetVideoSurface() will automatically > create the new MediaPlayer, so the only thing we need to do is call > enterfullscreen() when app restarts. I think you misread it, there is no early return for the fullscreen player, it will still be released in the loop below :)
https://codereview.chromium.org/103583005/diff/1/android_webview/native/aw_co... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/103583005/diff/1/android_webview/native/aw_co... android_webview/native/aw_contents.cc:738: if (cvc) { remove nested loops. if(!cvc) return; if(paused) ... else ... https://codereview.chromium.org/103583005/diff/1/content/browser/android/cont... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/103583005/diff/1/content/browser/android/cont... content/browser/android/content_view_core_impl.h:66: virtual void ResumeVideo() OVERRIDE; s/ResumeVideo/ResumeFullscreenVideo/ https://codereview.chromium.org/103583005/diff/1/content/common/view_messages.h File content/common/view_messages.h (right): https://codereview.chromium.org/103583005/diff/1/content/common/view_messages... content/common/view_messages.h:1352: // Sent by the browser when we should pause/resume video playback. pause is for all the videos, and resume is only for fullscreen video, i would separate the comments of these 2. https://codereview.chromium.org/103583005/diff/1/content/common/view_messages... content/common/view_messages.h:1354: IPC_MESSAGE_ROUTED0(ViewMsg_ResumeVideo); s/ResumeVideo/ResumeFullscreenVideo/ https://codereview.chromium.org/103583005/diff/1/content/renderer/media/andro... File content/renderer/media/android/renderer_media_player_manager.cc (left): https://codereview.chromium.org/103583005/diff/1/content/renderer/media/andro... content/renderer/media/android/renderer_media_player_manager.cc:326: void RendererMediaPlayerManager::ReleaseVideoResources() { ah...sorry, i thought you were removing the original function. But we should not rename this function to PauseVideo(). All the video resources are freed by this funtion, and we have a separate pause() function inside WebMediaPlayerAndroid and MediaPlayerBridge, we should not mix the usage of these two. On 2013/12/05 23:36:22, kbalazs wrote: > On 2013/12/05 23:02:34, qinmin wrote: > > I don't think this is right. This function is not only for fullscreen video, > it > > also works for embedded video. After your change, now it only pauses the > > fullscreen video， and not actually releasing the MediaPlayer resources. This > > will cause issues for other apps. > > We should still release the media player, and reacquire the resource when we > > restart the app. Calling MediaPlayerBridge.SetVideoSurface() will > automatically > > create the new MediaPlayer, so the only thing we need to do is call > > enterfullscreen() when app restarts. > > I think you misread it, there is no early return for the fullscreen player, it > will still be released in the loop below :) https://codereview.chromium.org/103583005/diff/1/content/renderer/media/andro... File content/renderer/media/android/renderer_media_player_manager.h (right): https://codereview.chromium.org/103583005/diff/1/content/renderer/media/andro... content/renderer/media/android/renderer_media_player_manager.h:119: void ResumeVideo(); s/rResumeVideo/ResumeFullscreenVideo/ https://codereview.chromium.org/103583005/diff/1/content/renderer/render_view... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/103583005/diff/1/content/renderer/render_view... content/renderer/render_view_impl.cc:1860: void RenderViewImpl::OnResumeVideo() { s/OnResumeVideo/OnResumeFullscreenVideo/ https://codereview.chromium.org/103583005/diff/1/content/renderer/render_view... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/103583005/diff/1/content/renderer/render_view... content/renderer/render_view_impl.h:998: void OnResumeVideo(); s/OnResumeVideo/OnResumeFullscreenVideo/ need to be more specific here. The OnPauseVideo() releases all the video players. However, the OnResumeVideo only resumes the fullscreen video.
I renamed ResumeVideo to ResumeFullscreenVideo (everywhere). I't still not perfect because it's odd that it is not symmetric to PauseVideo and I don't want to rename PauseVideo to something ridiculously verbose, say, ReleaseVideoResourcesAndSuspendFullscreenVideo. Ideally these should be routed through RenderViewImpl::OnWasShown/OnWasHidden but that does not work for the webview (maybe AwContents::SetIsPaused should trigger it?).
https://codereview.chromium.org/103583005/diff/1/android_webview/native/aw_co... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/103583005/diff/1/android_webview/native/aw_co... android_webview/native/aw_contents.cc:738: if (cvc) { On 2013/12/06 00:10:13, qinmin wrote: > remove nested loops. > > if(!cvc) > return; > > if(paused) > ... > else > ... Done. https://codereview.chromium.org/103583005/diff/1/content/renderer/media/andro... File content/renderer/media/android/renderer_media_player_manager.cc (left): https://codereview.chromium.org/103583005/diff/1/content/renderer/media/andro... content/renderer/media/android/renderer_media_player_manager.cc:326: void RendererMediaPlayerManager::ReleaseVideoResources() { Done, I moved the new code into SuspendFullscreenVideo. On 2013/12/06 00:10:13, qinmin wrote: > ah...sorry, i thought you were removing the original function. But we should > not rename this function to PauseVideo(). All the video resources are freed by > this funtion, and we have a separate pause() function inside > WebMediaPlayerAndroid and MediaPlayerBridge, we should not mix the usage of > these two. > > On 2013/12/05 23:36:22, kbalazs wrote: > > On 2013/12/05 23:02:34, qinmin wrote: > > > I don't think this is right. This function is not only for fullscreen video, > > it > > > also works for embedded video. After your change, now it only pauses the > > > fullscreen video， and not actually releasing the MediaPlayer resources. This > > > will cause issues for other apps. > > > We should still release the media player, and reacquire the resource when we > > > restart the app. Calling MediaPlayerBridge.SetVideoSurface() will > > automatically > > > create the new MediaPlayer, so the only thing we need to do is call > > > enterfullscreen() when app restarts. > > > > I think you misread it, there is no early return for the fullscreen player, it > > will still be released in the loop below :) >
https://codereview.chromium.org/103583005/diff/20001/content/browser/media/an... File content/browser/media/android/browser_media_player_manager.cc (left): https://codereview.chromium.org/103583005/diff/20001/content/browser/media/an... content/browser/media/android/browser_media_player_manager.cc:453: if (video_view_.get()) { I don't think you can remove this. Try the swap-fs example on videotestsuite.appspot.com. It is possible that the website changes the src attribute when playing a video in fullscreen, so that it can play ads before the content. In this case, the first player will be destroyed without destroying the contentVideoView. https://codereview.chromium.org/103583005/diff/20001/content/browser/media/an... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/103583005/diff/20001/content/browser/media/an... content/browser/media/android/browser_media_player_manager.cc:465: video_view_.reset(); you should not do this. The MediaPlayer should detach from the surfaceview first before the latter is destroyed. Otherwise, some old android builds will throw exceptions. This call should just hint the ContentVideoView to destroy the surfaceView. When SurfaceDestroyed() gets called, it will ask the MediaPlayer to set the surface to NULL. Calling video_view_.reset will make java side unable to callback the native side.
On 2013/12/07 01:17:03, qinmin wrote: > https://codereview.chromium.org/103583005/diff/20001/content/browser/media/an... > File content/browser/media/android/browser_media_player_manager.cc (left): > > https://codereview.chromium.org/103583005/diff/20001/content/browser/media/an... > content/browser/media/android/browser_media_player_manager.cc:453: if > (video_view_.get()) { > I don't think you can remove this. Try the swap-fs example on > http://videotestsuite.appspot.com. > It is possible that the website changes the src attribute when playing a video > in fullscreen, so that it can play ads before the content. In this case, the > first player will be destroyed without destroying the contentVideoView. > > https://codereview.chromium.org/103583005/diff/20001/content/browser/media/an... > File content/browser/media/android/browser_media_player_manager.cc (right): > > https://codereview.chromium.org/103583005/diff/20001/content/browser/media/an... > content/browser/media/android/browser_media_player_manager.cc:465: > video_view_.reset(); > you should not do this. The MediaPlayer should detach from the surfaceview first > before the latter is destroyed. Otherwise, some old android builds will throw > exceptions. > > This call should just hint the ContentVideoView to destroy the surfaceView. When > SurfaceDestroyed() gets called, it will ask the MediaPlayer to set the surface > to NULL. Calling video_view_.reset will make java side unable to callback the > native side. Thanks for pointing this out, I was unaware of that. I made this change because I had some problem with exiting and reentering the video, I'm gonna see how can I fix that.
On 2013/12/09 18:38:43, kbalazs wrote: > On 2013/12/07 01:17:03, qinmin wrote: > > > https://codereview.chromium.org/103583005/diff/20001/content/browser/media/an... > > File content/browser/media/android/browser_media_player_manager.cc (left): > > > > > https://codereview.chromium.org/103583005/diff/20001/content/browser/media/an... > > content/browser/media/android/browser_media_player_manager.cc:453: if > > (video_view_.get()) { > > I don't think you can remove this. Try the swap-fs example on > > http://videotestsuite.appspot.com. > > It is possible that the website changes the src attribute when playing a video > > in fullscreen, so that it can play ads before the content. In this case, the > > first player will be destroyed without destroying the contentVideoView. > > > > > https://codereview.chromium.org/103583005/diff/20001/content/browser/media/an... > > File content/browser/media/android/browser_media_player_manager.cc (right): > > > > > https://codereview.chromium.org/103583005/diff/20001/content/browser/media/an... > > content/browser/media/android/browser_media_player_manager.cc:465: > > video_view_.reset(); > > you should not do this. The MediaPlayer should detach from the surfaceview > first > > before the latter is destroyed. Otherwise, some old android builds will throw > > exceptions. > > > > This call should just hint the ContentVideoView to destroy the surfaceView. > When > > SurfaceDestroyed() gets called, it will ask the MediaPlayer to set the surface > > to NULL. Calling video_view_.reset will make java side unable to callback the > > native side. > > Thanks for pointing this out, I was unaware of that. I made this change because > I had some problem with exiting and reentering the video, I'm gonna see how can > I fix that. Turned out I don't need those changes at all. I had problems with a previous version of the patch but now it works fine without that.
On 2013/12/09 19:20:17, kbalazs wrote: > On 2013/12/09 18:38:43, kbalazs wrote: > > On 2013/12/07 01:17:03, qinmin wrote: > > > > > > https://codereview.chromium.org/103583005/diff/20001/content/browser/media/an... > > > File content/browser/media/android/browser_media_player_manager.cc (left): > > > > > > > > > https://codereview.chromium.org/103583005/diff/20001/content/browser/media/an... > > > content/browser/media/android/browser_media_player_manager.cc:453: if > > > (video_view_.get()) { > > > I don't think you can remove this. Try the swap-fs example on > > > http://videotestsuite.appspot.com. > > > It is possible that the website changes the src attribute when playing a > video > > > in fullscreen, so that it can play ads before the content. In this case, the > > > first player will be destroyed without destroying the contentVideoView. > > > > > > > > > https://codereview.chromium.org/103583005/diff/20001/content/browser/media/an... > > > File content/browser/media/android/browser_media_player_manager.cc (right): > > > > > > > > > https://codereview.chromium.org/103583005/diff/20001/content/browser/media/an... > > > content/browser/media/android/browser_media_player_manager.cc:465: > > > video_view_.reset(); > > > you should not do this. The MediaPlayer should detach from the surfaceview > > first > > > before the latter is destroyed. Otherwise, some old android builds will > throw > > > exceptions. > > > > > > This call should just hint the ContentVideoView to destroy the surfaceView. > > When > > > SurfaceDestroyed() gets called, it will ask the MediaPlayer to set the > surface > > > to NULL. Calling video_view_.reset will make java side unable to callback > the > > > native side. > > > > Thanks for pointing this out, I was unaware of that. I made this change > because > > I had some problem with exiting and reentering the video, I'm gonna see how > can > > I fix that. > > Turned out I don't need those changes at all. I had problems with a previous > version of the patch but now it works fine without that. I tried this CL. While playing the fullscreen video, if I click the home button, and then switch back to chrome, the fullscreen video is not restarted. This does not matches the CL description.
https://codereview.chromium.org/103583005/diff/60001/content/renderer/media/a... File content/renderer/media/android/renderer_media_player_manager.h (right): https://codereview.chromium.org/103583005/diff/60001/content/renderer/media/a... content/renderer/media/android/renderer_media_player_manager.h:116: // Requests the player that is currently entered fullscreen (if any) nit: s/is/has/
> I tried this CL. While playing the fullscreen video, if I click the home button, > and then switch back to chrome, the fullscreen video is not restarted. > This does not matches the CL description. Thanks for testing it. For me it works. I'm using a Samsung Galaxy Note 2 with Android 4.3 and trying some demo content like http://www.w3.org/2010/05/video/mediaevents.html or http://camendesign.com/code/video_for_everybody/test.html. What device and test content do you use?
On 2013/12/10 00:23:06, kbalazs wrote: > > I tried this CL. While playing the fullscreen video, if I click the home > button, > > and then switch back to chrome, the fullscreen video is not restarted. > > This does not matches the CL description. > > Thanks for testing it. For me it works. I'm using a Samsung Galaxy Note 2 with > Android 4.3 and trying some demo content like > http://www.w3.org/2010/05/video/mediaevents.html or > http://camendesign.com/code/video_for_everybody/test.html. What device and test > content do you use? I am using nexus 4 android 4.4. simply go to videotestsuite.appspot.com, run the play-fs test. Hit the soft home button while video is playing in fullscreen, and then hit the soft switch button to switch back to chrome
On 2013/12/10 00:50:33, qinmin wrote: > On 2013/12/10 00:23:06, kbalazs wrote: > > > I tried this CL. While playing the fullscreen video, if I click the home > > button, > > > and then switch back to chrome, the fullscreen video is not restarted. > > > This does not matches the CL description. > > > > Thanks for testing it. For me it works. I'm using a Samsung Galaxy Note 2 with > > Android 4.3 and trying some demo content like > > http://www.w3.org/2010/05/video/mediaevents.html or > > http://camendesign.com/code/video_for_everybody/test.html. What device and > test > > content do you use? > > I am using nexus 4 android 4.4. simply go to http://videotestsuite.appspot.com, run the > play-fs test. > Hit the soft home button while video is playing in fullscreen, and then hit the > soft switch button to switch back to chrome It also work for me with this example. I hope I can set up an emulator with 4.4 and look at what's going wrong.
I'm not very lucky with the emulator, it does not work very well and I cannot play videos in it at all. I'm trying to fix. If you have some time, could you be so kind and run this patch with some tracing and send me the output? I prepared a patch with logs: https://gist.github.com/balazs/7902132. I would like to look at the output of "adb logcat chromium:e ContentVideoView:e". Just if you really have time :)
On 2013/12/10 23:18:44, kbalazs wrote: > I'm not very lucky with the emulator, it does not work very well and I cannot > play videos in it at all. I'm trying to fix. > > If you have some time, could you be so kind and run this patch with some tracing > and send me the output? I prepared a patch with logs: > https://gist.github.com/balazs/7902132. I would like to look at the output of > "adb logcat chromium:e ContentVideoView:e". Just if you really have time :) When hitting the home button, the surfaceview got destroyed first. So a didExitFullscreen IPC is sent to the RendererMediaPlayerManager. The manager will set the fullscreen_player_id_ to -1. Then ResumeFullscreenVideo will record that player_id as the paused_fullscreen_player_id_. Upon resuming, since paused_fullscreen_player_id_ is -1, nothing will happen.
On 2013/12/11 02:09:59, qinmin wrote: > On 2013/12/10 23:18:44, kbalazs wrote: > > I'm not very lucky with the emulator, it does not work very well and I cannot > > play videos in it at all. I'm trying to fix. > > > > If you have some time, could you be so kind and run this patch with some > tracing > > and send me the output? I prepared a patch with logs: > > https://gist.github.com/balazs/7902132. I would like to look at the output of > > "adb logcat chromium:e ContentVideoView:e". Just if you really have time :) > > When hitting the home button, the surfaceview got destroyed first. So a > didExitFullscreen IPC is sent to the RendererMediaPlayerManager. > The manager will set the fullscreen_player_id_ to -1. > Then ResumeFullscreenVideo will record that player_id as the > paused_fullscreen_player_id_. > Upon resuming, since paused_fullscreen_player_id_ is -1, nothing will happen. I see, thanks for pointing out. I think we can remove handling of surfaceDestroyed since with the patch we kill the view from C++ anyway.
On 2013/12/11 18:07:41, kbalazs wrote: > On 2013/12/11 02:09:59, qinmin wrote: > > On 2013/12/10 23:18:44, kbalazs wrote: > > > I'm not very lucky with the emulator, it does not work very well and I > cannot > > > play videos in it at all. I'm trying to fix. > > > > > > If you have some time, could you be so kind and run this patch with some > > tracing > > > and send me the output? I prepared a patch with logs: > > > https://gist.github.com/balazs/7902132. I would like to look at the output > of > > > "adb logcat chromium:e ContentVideoView:e". Just if you really have time :) > > > > When hitting the home button, the surfaceview got destroyed first. So a > > didExitFullscreen IPC is sent to the RendererMediaPlayerManager. > > The manager will set the fullscreen_player_id_ to -1. > > Then ResumeFullscreenVideo will record that player_id as the > > paused_fullscreen_player_id_. > > Upon resuming, since paused_fullscreen_player_id_ is -1, nothing will happen. > > I see, thanks for pointing out. I think we can remove handling of > surfaceDestroyed since with the patch we kill the view from C++ anyway. We need to handle the surfaceDestroyed properly, since that is triggered by user and we have to detach the surfaceview from the MediaPlayer before the surface view is totally gone. BTW, this CL may have some impact on Media Source implementations, need to test it with some MSE demos.
On 2013/12/11 18:33:14, qinmin wrote: > On 2013/12/11 18:07:41, kbalazs wrote: > > On 2013/12/11 02:09:59, qinmin wrote: > > > On 2013/12/10 23:18:44, kbalazs wrote: > > > > I'm not very lucky with the emulator, it does not work very well and I > > cannot > > > > play videos in it at all. I'm trying to fix. > > > > > > > > If you have some time, could you be so kind and run this patch with some > > > tracing > > > > and send me the output? I prepared a patch with logs: > > > > https://gist.github.com/balazs/7902132. I would like to look at the output > > of > > > > "adb logcat chromium:e ContentVideoView:e". Just if you really have time > :) > > > > > > When hitting the home button, the surfaceview got destroyed first. So a > > > didExitFullscreen IPC is sent to the RendererMediaPlayerManager. > > > The manager will set the fullscreen_player_id_ to -1. > > > Then ResumeFullscreenVideo will record that player_id as the > > > paused_fullscreen_player_id_. > > > Upon resuming, since paused_fullscreen_player_id_ is -1, nothing will > happen. > > > > I see, thanks for pointing out. I think we can remove handling of > > surfaceDestroyed since with the patch we kill the view from C++ anyway. > > We need to handle the surfaceDestroyed properly, since that is triggered by user > and we have to detach the surfaceview from the MediaPlayer before the surface > view is totally gone. Right, but ContentViewCore::PauseVideo is always called before that happens so we can also destroy the view at that point from C++. PauseVideo is also triggered by the user and it is called when going background as well as when locking the device. The latter is not true for surfaceDestroyed. > BTW, this CL may have some impact on Media Source implementations, need to test > it with some MSE demos. Sure, I will check that as well. Actually there is one more problem with the patch. When becoming visible again, it first sends the message to the renderer which sends back the message to (re)enter fullscreen. If that does not happen fast enough than the user can see the page before reentering fullscreen which is confusing and looks like a bug. On my device it happens sometimes (usually not though). So I think a better approach would be to shut down the fullscreen player at browser side without telling it to the renderer. When becoming visible we can relaunch it and reassign the surface to the player, so the renderer doesn't need to be involved. So I'm gonna destroy the view when PauseVideo is called and remove the handler of surfaceDestroyed. The problem with surfaceDestroyed is that it is not called when the device get's locked. I would like to do the exact same thing for both cases. It also has the advantage that we can release more resources on device lock because we destroy the view.
Could you review the latest patch?
https://codereview.chromium.org/103583005/diff/100001/content/browser/android... File content/browser/android/content_video_view.cc (right): https://codereview.chromium.org/103583005/diff/100001/content/browser/android... content/browser/android/content_video_view.cc:200: Java_ContentVideoView_createContentVideoView(env, nit:fix the indentation https://codereview.chromium.org/103583005/diff/100001/content/browser/android... File content/browser/android/content_video_view.h (right): https://codereview.chromium.org/103583005/diff/100001/content/browser/android... content/browser/android/content_video_view.h:92: ContentViewCoreImpl* content_view_core); no need for the input param, you can get that from manager_ inside this function. https://codereview.chromium.org/103583005/diff/100001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java (right): https://codereview.chromium.org/103583005/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:338: // We expect the native side to call destroyContentVideoView This is not right. When user hit the home button, SurfaceDestroyed should be called before the native side call exitFullscreen.
https://codereview.chromium.org/103583005/diff/100001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java (right): https://codereview.chromium.org/103583005/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:338: // We expect the native side to call destroyContentVideoView On 2013/12/18 02:09:46, qinmin wrote: > This is not right. When user hit the home button, SurfaceDestroyed should be > called before the native side call exitFullscreen. If the user calls ContentView.onHide in it's activity's onStop than PauseVideo will be called before the view is destroyed. I think surfaceDestroyed cannot be called before onStop returns. So this is the case with test_shell and content_shell and I expect that all clients should do that. However we can keep it to make sure it's destroyed even if ContentView.onHide is never called.
lgtm % nits https://codereview.chromium.org/103583005/diff/120001/content/browser/android... File content/browser/android/content_video_view.h (right): https://codereview.chromium.org/103583005/diff/120001/content/browser/android... content/browser/android/content_video_view.h:20: class ContentViewCoreImpl; nit: this is no longer needed https://codereview.chromium.org/103583005/diff/120001/content/browser/android... content/browser/android/content_video_view.h:105: enum FullscreenState { for enums, all the letters are capitalized. So use ENTERED, SUSPENDED,RESUME instead
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/103583005/140001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
@bulach: I'd like you to do a second review on this
On 2013/12/19 16:21:11, kbalazs wrote: > @bulach: I'd like you to do a second review on this Nevermind, I didn't see you are out of office. Ted, could you? :)
Could someone sign-off? (missing owner lgtm).
lgtm for OWNERS w/ scope question https://codereview.chromium.org/103583005/diff/140001/content/browser/android... File content/browser/android/content_video_view.h (right): https://codereview.chromium.org/103583005/diff/140001/content/browser/android... content/browser/android/content_video_view.h:90: JavaObjectWeakGlobalRef CreateJavaObject(); why does this need to be public?
https://codereview.chromium.org/103583005/diff/140001/content/browser/android... File content/browser/android/content_video_view.h (right): https://codereview.chromium.org/103583005/diff/140001/content/browser/android... content/browser/android/content_video_view.h:90: JavaObjectWeakGlobalRef CreateJavaObject(); On 2013/12/26 17:54:06, Ted C wrote: > why does this need to be public? It doesn't :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/103583005/180001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/103583005/240001
Message was sent while issue was closed.
Change committed as 242570
Message was sent while issue was closed.
late lgtm, thanks! sorry for the delay, just a couple of nits below: https://codereview.chromium.org/103583005/diff/240001/content/browser/android... File content/browser/android/content_video_view.cc (right): https://codereview.chromium.org/103583005/diff/240001/content/browser/android... content/browser/android/content_video_view.cc:202: env, nit: indent should be just +4 from the beginning of the line. https://codereview.chromium.org/103583005/diff/240001/content/browser/android... File content/browser/android/content_video_view.h (right): https://codereview.chromium.org/103583005/diff/240001/content/browser/android... content/browser/android/content_video_view.h:34: ContentVideoView(BrowserMediaPlayerManager* manager); nit: explicit
Message was sent while issue was closed.
On 2014/01/06 15:44:02, bulach_ooo_until_feb_2 wrote: > late lgtm, thanks! > > sorry for the delay, just a couple of nits below: > > https://codereview.chromium.org/103583005/diff/240001/content/browser/android... > File content/browser/android/content_video_view.cc (right): > > https://codereview.chromium.org/103583005/diff/240001/content/browser/android... > content/browser/android/content_video_view.cc:202: env, > nit: indent should be just +4 from the beginning of the line. > > https://codereview.chromium.org/103583005/diff/240001/content/browser/android... > File content/browser/android/content_video_view.h (right): > > https://codereview.chromium.org/103583005/diff/240001/content/browser/android... > content/browser/android/content_video_view.h:34: > ContentVideoView(BrowserMediaPlayerManager* manager); > nit: explicit I am not very comfortable with this change now. There is no guaranteed ordering between ContentVideoView.OnSurfaceDestroyed() and ContentViewCoreImpl::PauseVideo(). When hitting the home button on nexus 5, for example, the former is called first. And when hitting the screen lock, the latter is called first.
Message was sent while issue was closed.
On 2014/01/31 20:36:08, qinmin wrote: > On 2014/01/06 15:44:02, bulach_ooo_until_feb_2 wrote: > > late lgtm, thanks! > > > > sorry for the delay, just a couple of nits below: > > > > > https://codereview.chromium.org/103583005/diff/240001/content/browser/android... > > File content/browser/android/content_video_view.cc (right): > > > > > https://codereview.chromium.org/103583005/diff/240001/content/browser/android... > > content/browser/android/content_video_view.cc:202: env, > > nit: indent should be just +4 from the beginning of the line. > > > > > https://codereview.chromium.org/103583005/diff/240001/content/browser/android... > > File content/browser/android/content_video_view.h (right): > > > > > https://codereview.chromium.org/103583005/diff/240001/content/browser/android... > > content/browser/android/content_video_view.h:34: > > ContentVideoView(BrowserMediaPlayerManager* manager); > > nit: explicit > > I am not very comfortable with this change now. There is no guaranteed ordering > between ContentVideoView.OnSurfaceDestroyed() and > ContentViewCoreImpl::PauseVideo(). > When hitting the home button on nexus 5, for example, the former is called > first. > And when hitting the screen lock, the latter is called first. OnSurfaceDestroyed is not called when hitting the lock button at all. I'm going to check the order of the calls on my device and think about whether the ordering you experience can cause problem.
Message was sent while issue was closed.
Something has definitely changed somewhere. I see that now surfaceDestroyed is called on lock. And with the home button, surfaceDestroyed is called first. In this case the fullscreen restarting code in this patch is not functional. I am 100% sure that this was not the case when I last tested this. Do you know about something that could cause this? Maybe we build with newer sdk version? So far I did not see any code change that would explain this. Anyways, I am going to rethink the patch.
Message was sent while issue was closed.
I think at the time I was writing the patch, ContentViewCore::onHide/onShow was called in turn of main activity onPause/onResume. This is not happening now. In fact onShow is not called when I launch content shell which looks like a bug to me. I'm still digging what has changed.
Message was sent while issue was closed.
I was trying to fix this but was not successful. So the case when it does not work well is when the process gets terminated while it is in the background. This can be reproduced with a developer option "Do not keep activities" (which was turned on for me this time as it turned out...). I am going to revert the original patch. I tried to fix this but somehow I cannot make the fullscreen view reinitialized when the process is recreated. For some reason the surfaceCreated for the VideoSurfaceView is never called and the fullscreen view does not go visible. I will show you the WIP patch, I hope someone with deeper Android knowledge can help. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
