Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1308223002:
Android: Add new renderer SurfaceViewRenderer (Closed)
|
Created:
5 years, 4 months ago by magjed_webrtc Modified:
5 years, 3 months ago Reviewers:
AlexG CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), Andrew MacDonald Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAndroid: Add new renderer SurfaceViewRenderer
BUG=webrtc:4742, webrtc:4910
R=glaznev@webrtc.org
Committed: https://crrev.com/c06a716d5941e8c60229afdd8720cd9c45178374
Cr-Commit-Position: refs/heads/master@{#9922}
Patch Set 1 : Reland from https://codereview.webrtc.org/1257043004 #Patch Set 2 : Rebase canApplyRotation and GlRectDrawer.uploadYuvData() #Patch Set 3 : Add RendererEvents support #Patch Set 4 : Fix issue webrtc:4910 #
Total comments: 18
Patch Set 5 : Addressing comments and rebasing against RendererCommon.getSamplingMatrix() #Patch Set 6 : add TODO to investigate how to release() safely #Messages
Total messages: 11 (3 generated)
magjed@webrtc.org changed reviewers: + glaznev@webrtc.org
Alex - Please take a look. This is a rebased reland of SurfaceViewRenderer, but without using it for AppRTCDemo yet. I fixed issue webrtc:4910 by always conforming to the onMeasure() constraints. This also means I have to flush the SurfaceView with a black frame when someone requests a resize.
Ping
https://codereview.webrtc.org/1308223002/diff/60001/talk/app/webrtc/java/andr... File talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java (right): https://codereview.webrtc.org/1308223002/diff/60001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:57: // Dedicated render thread. Synchronized on |this|. I would add separate lock instead of |this| for clarity similar to other 2 locks used below. https://codereview.webrtc.org/1308223002/diff/60001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:75: // Most recent measurement specification from onMeasure(). Please add more comments on these different dimension - when do they updated and what are their target value. Something like: surfaceWidth/Height is size of underlying surface buffer? Updated in .... After update should be equal to layoutWidth/Height? It is not very clear for me what are all these dimensions used for (though I understand they are necessary :)) https://codereview.webrtc.org/1308223002/diff/60001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:158: if (yuvTextures != null) { I think this is not necessary - Android should handle it by itself http://developer.android.com/reference/android/opengl/GLSurfaceView.Renderer.... - "When the EGL context is lost, all OpenGL resources (such as textures) that are associated with that context will be automatically deleted" When you called GlRectDrawer.release() and deleted shaders in current renderer it sometime caused GL crash and distorted Activity on KK devices in some rear cases. https://codereview.webrtc.org/1308223002/diff/60001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:213: Log.d(TAG, "Dropping frame - previous frame has not been rendered yet."); nit: this may cause too frequent log spamming for devices with slow rendering performance. Having periodic statistics on frame drops is probably enough. https://codereview.webrtc.org/1308223002/diff/60001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:338: runOnRenderThreadDelayed(renderFrameRunnable, 1); I think it is better to just exclude this call - this looks like some hack especially with 1 ms timeout. If surface is not updated - probably better to wait for next frame and still display black frame. But this is up to you. https://codereview.webrtc.org/1308223002/diff/60001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:346: synchronized (layoutLock) { Can this lock be unified with the previous one one line 328? https://codereview.webrtc.org/1308223002/diff/60001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:363: Log.d(TAG, "Postponing rendering until surface size is updated."); glClear() here? https://codereview.webrtc.org/1308223002/diff/60001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:453: synchronized (statisticsLock) { nit: no need in this synchronize - it is already called from synchronized block. But may be better to leave it for clarity.
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
https://codereview.webrtc.org/1308223002/diff/60001/talk/app/webrtc/java/andr... File talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java (right): https://codereview.webrtc.org/1308223002/diff/60001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:57: // Dedicated render thread. Synchronized on |this|. On 2015/09/10 00:09:08, AlexG wrote: > I would add separate lock instead of |this| for clarity similar to other 2 locks > used below. Done. https://codereview.webrtc.org/1308223002/diff/60001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:75: // Most recent measurement specification from onMeasure(). On 2015/09/10 00:09:08, AlexG wrote: > Please add more comments on these different dimension - when do they updated and > what are their target value. > Something like: surfaceWidth/Height is size of underlying surface buffer? > Updated in .... After update should be equal to layoutWidth/Height? > It is not very clear for me what are all these dimensions used for (though I > understand they are necessary :)) Done. https://codereview.webrtc.org/1308223002/diff/60001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:158: if (yuvTextures != null) { On 2015/09/10 00:09:08, AlexG wrote: > I think this is not necessary - Android should handle it by itself > http://developer.android.com/reference/android/opengl/GLSurfaceView.Renderer.... > - "When the EGL context is lost, all OpenGL resources (such as textures) that > are associated with that context will be automatically deleted" > > When you called GlRectDrawer.release() and deleted shaders in current renderer > it sometime caused GL crash and distorted Activity on KK devices in some rear > cases. Thanks, I didn't know this. I'm still worried that the automatic cleanup may happen too late because |EGLContext sharedContext| is outside this class, and is e.g. only lost in Activity.onDestroy() for AppRTCDemo. I think it might be a valid use case to init() and release() multiple renderers during the lifetime of |sharedContext|. I looked at https://codereview.webrtc.org/1322703004/. I think that issue is caused by calling VideoRendererGui.dispose() after calling super.onDestroy(). The current code looks like this: public class CallActivity extends Activity … @Override protected void onDestroy() { … super.onDestroy(); … VideoRendererGui.dispose(); } I think we need to release the resources in the reverse order they were created, i.e.: @Override public void onCreate(Bundle savedInstanceState) { super.onCreate(savedInstanceState); VideoRendererGui.setView(...); } @Override public void onDestroy() { VideoRendererGui.dispose(); super.onDestroy(); } https://codereview.webrtc.org/1308223002/diff/60001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:213: Log.d(TAG, "Dropping frame - previous frame has not been rendered yet."); On 2015/09/10 00:09:08, AlexG wrote: > nit: this may cause too frequent log spamming for devices with slow rendering > performance. Having periodic statistics on frame drops is probably enough. Done. https://codereview.webrtc.org/1308223002/diff/60001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:338: runOnRenderThreadDelayed(renderFrameRunnable, 1); On 2015/09/10 00:09:08, AlexG wrote: > I think it is better to just exclude this call - this looks like some hack > especially with 1 ms timeout. If surface is not updated - probably better to > wait for next frame and still display black frame. But this is up to you. Done, I removed it. https://codereview.webrtc.org/1308223002/diff/60001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:346: synchronized (layoutLock) { On 2015/09/10 00:09:08, AlexG wrote: > Can this lock be unified with the previous one one line 328? Done, I moved this into a separate function checkConsistentLayout(). https://codereview.webrtc.org/1308223002/diff/60001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:363: Log.d(TAG, "Postponing rendering until surface size is updated."); On 2015/09/10 00:09:08, AlexG wrote: > glClear() here? Done, part of checkConsistentLayout() refactoring now. https://codereview.webrtc.org/1308223002/diff/60001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:453: synchronized (statisticsLock) { On 2015/09/10 00:09:08, AlexG wrote: > nit: no need in this synchronize - it is already called from synchronized block. > But may be better to leave it for clarity. Acknowledged. I will keep it for clarity and in case the code changes in the future.
lgtm https://codereview.webrtc.org/1308223002/diff/60001/talk/app/webrtc/java/andr... File talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java (right): https://codereview.webrtc.org/1308223002/diff/60001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:158: if (yuvTextures != null) { On 2015/09/10 13:38:28, magjed_webrtc wrote: > On 2015/09/10 00:09:08, AlexG wrote: > > I think this is not necessary - Android should handle it by itself > > > http://developer.android.com/reference/android/opengl/GLSurfaceView.Renderer.... > > - "When the EGL context is lost, all OpenGL resources (such as textures) that > > are associated with that context will be automatically deleted" > > > > When you called GlRectDrawer.release() and deleted shaders in current renderer > > it sometime caused GL crash and distorted Activity on KK devices in some rear > > cases. > > Thanks, I didn't know this. I'm still worried that the automatic cleanup may > happen too late because |EGLContext sharedContext| is outside this class, and is > e.g. only lost in Activity.onDestroy() for AppRTCDemo. I think it might be a > valid use case to init() and release() multiple renderers during the lifetime of > |sharedContext|. > > I looked at https://codereview.webrtc.org/1322703004/. I think that issue is > caused by calling VideoRendererGui.dispose() after calling super.onDestroy(). > The current code looks like this: > public class CallActivity extends Activity > … > @Override > protected void onDestroy() { > … > super.onDestroy(); > … > VideoRendererGui.dispose(); > } > I think we need to release the resources in the reverse order they were created, > i.e.: > @Override > public void onCreate(Bundle savedInstanceState) { > super.onCreate(savedInstanceState); > VideoRendererGui.setView(...); > } > @Override > public void onDestroy() { > VideoRendererGui.dispose(); > super.onDestroy(); > } I think I tried both - calling dispose() before super.onDestroy() made things better. I may be wrong, but seems like when onDestroy() is executing Activity and its controls are still alive - GL rendering may still happen even during onDestroy() execution.
Message was sent while issue was closed.
Committed patchset #6 (id:140001) manually as c06a716d5941e8c60229afdd8720cd9c45178374 (presubmit successful).
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/c06a716d5941e8c60229afdd8720cd9c45178374 Cr-Commit-Position: refs/heads/master@{#9922}
Message was sent while issue was closed.
https://codereview.webrtc.org/1308223002/diff/60001/talk/app/webrtc/java/andr... File talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java (right): https://codereview.webrtc.org/1308223002/diff/60001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:158: if (yuvTextures != null) { On 2015/09/10 23:31:50, AlexG wrote: > On 2015/09/10 13:38:28, magjed_webrtc wrote: > > On 2015/09/10 00:09:08, AlexG wrote: > > > I think this is not necessary - Android should handle it by itself > > > > > > http://developer.android.com/reference/android/opengl/GLSurfaceView.Renderer.... > > > - "When the EGL context is lost, all OpenGL resources (such as textures) > that > > > are associated with that context will be automatically deleted" > > > > > > When you called GlRectDrawer.release() and deleted shaders in current > renderer > > > it sometime caused GL crash and distorted Activity on KK devices in some > rear > > > cases. > > > > Thanks, I didn't know this. I'm still worried that the automatic cleanup may > > happen too late because |EGLContext sharedContext| is outside this class, and > is > > e.g. only lost in Activity.onDestroy() for AppRTCDemo. I think it might be a > > valid use case to init() and release() multiple renderers during the lifetime > of > > |sharedContext|. > > > > I looked at https://codereview.webrtc.org/1322703004/. I think that issue is > > caused by calling VideoRendererGui.dispose() after calling super.onDestroy(). > > The current code looks like this: > > public class CallActivity extends Activity > > … > > @Override > > protected void onDestroy() { > > … > > super.onDestroy(); > > … > > VideoRendererGui.dispose(); > > } > > I think we need to release the resources in the reverse order they were > created, > > i.e.: > > @Override > > public void onCreate(Bundle savedInstanceState) { > > super.onCreate(savedInstanceState); > > VideoRendererGui.setView(...); > > } > > @Override > > public void onDestroy() { > > VideoRendererGui.dispose(); > > super.onDestroy(); > > } > > I think I tried both - calling dispose() before super.onDestroy() made things > better. I may be wrong, but seems like when onDestroy() is executing Activity > and its controls are still alive - GL rendering may still happen even during > onDestroy() execution. I added a TODO - I will try to investigate this. |
