Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2510413002:
Set surface view surface size to minimum of the layout size and frame size. (Closed)
|
Created:
4 years, 1 month ago by sakal Modified:
4 years, 1 month ago Reviewers:
magjed_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionSet surface view surface size to minimum of the layout size and frame size.
This improves the performance of SurfaceViewRenderer. This feature is added
behind a flag for now because it can be buggy on some devices.
BUG=webrtc:6470
Committed: https://crrev.com/b1e6d5efa67edda6b06377b1b9cb50d295514a3d
Cr-Commit-Position: refs/heads/master@{#15181}
Patch Set 1 : Fix #
Total comments: 15
Patch Set 2 : Changes according to magjed's comments. #
Total comments: 2
Patch Set 3 : Changes according to magjed's comments. #2 #
Messages
Total messages: 35 (25 generated)
Description was changed from ========== Set surface view surface size. BUG= ========== to ========== Set surface view surface size to minimum of the layout size and frame size. This improves the performance of SurfaceViewRenderer. BUG= ==========
Description was changed from ========== Set surface view surface size to minimum of the layout size and frame size. This improves the performance of SurfaceViewRenderer. BUG= ========== to ========== Set surface view surface size to minimum of the layout size and frame size. This improves the performance of SurfaceViewRenderer. BUG=webrtc:6470 ==========
sakal@webrtc.org changed reviewers: + magjed@webrtc.org
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
The CQ bit was checked by sakal@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_mips_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_mips_db...) android_compile_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...) android_compile_x86_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_rel...)
Patchset #1 (id:100001) has been deleted
The CQ bit was checked by sakal@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2510413002/diff/120001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java (right): https://codereview.webrtc.org/2510413002/diff/120001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java:50: private SurfaceHolder surfaceHolder; You don't need to store this, you can just call SurfaceView.getHolder() instead. It's fine to call functions on the SurfaceHolder (but not the Surface) even after SurfaceDestroyed. https://codereview.webrtc.org/2510413002/diff/120001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java:159: if (surfaceHolder != null && rotatedFrameWidth != 0 && rotatedFrameHeight != 0) { Maybe add getHeight() != 0 as well, I think it's allowed to have zero layout size. https://codereview.webrtc.org/2510413002/diff/120001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java:172: final int surfaceWidth = Math.min(getWidth(), drawnFrameWidth); Maybe we should store surfaceWidth/Height as member variables and only call setFixedSize when they change. https://codereview.webrtc.org/2510413002/diff/120001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java:177: surfaceHolder.setFixedSize(surfaceWidth, surfaceHeight); I have tried to use setFixedSize before, and it's buggy on some devices. I think we need to have a whitelist/blacklist for this before we enable it per default. I would like to put this feature under a flag for now that defaults to false, e.g. 'public void enableFixedSurfaceSize(boolean enable)', and if it's false we don't call setFixedSize. You can disable fixed size by calling SurfaceHolder.setSizeFromLayout(). https://codereview.webrtc.org/2510413002/diff/120001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java:243: updateSurfaceSize(); This is too early to call updateSurfaceSize(), the layout size has not changed yet. You should call updateSurfaceSize() in onLayout() instead.
Patchset #2 (id:140001) has been deleted
https://codereview.webrtc.org/2510413002/diff/120001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java (right): https://codereview.webrtc.org/2510413002/diff/120001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java:50: private SurfaceHolder surfaceHolder; On 2016/11/21 10:16:29, magjed_webrtc wrote: > You don't need to store this, you can just call SurfaceView.getHolder() instead. > It's fine to call functions on the SurfaceHolder (but not the Surface) even > after SurfaceDestroyed. Done. https://codereview.webrtc.org/2510413002/diff/120001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java:159: if (surfaceHolder != null && rotatedFrameWidth != 0 && rotatedFrameHeight != 0) { On 2016/11/21 10:16:29, magjed_webrtc wrote: > Maybe add getHeight() != 0 as well, I think it's allowed to have zero layout > size. Done. https://codereview.webrtc.org/2510413002/diff/120001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java:172: final int surfaceWidth = Math.min(getWidth(), drawnFrameWidth); On 2016/11/21 10:16:29, magjed_webrtc wrote: > Maybe we should store surfaceWidth/Height as member variables and only call > setFixedSize when they change. Done. https://codereview.webrtc.org/2510413002/diff/120001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java:177: surfaceHolder.setFixedSize(surfaceWidth, surfaceHeight); On 2016/11/21 10:16:29, magjed_webrtc wrote: > I have tried to use setFixedSize before, and it's buggy on some devices. I think > we need to have a whitelist/blacklist for this before we enable it per default. > I would like to put this feature under a flag for now that defaults to false, > e.g. 'public void enableFixedSurfaceSize(boolean enable)', and if it's false we > don't call setFixedSize. You can disable fixed size by calling > SurfaceHolder.setSizeFromLayout(). Done. https://codereview.webrtc.org/2510413002/diff/120001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java:209: updateSurfaceSize(); What do you think about removing this? I think it should be safe and it seemed to reduce the flickering in AppRTCMobile to my eye. https://codereview.webrtc.org/2510413002/diff/120001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java:243: updateSurfaceSize(); On 2016/11/21 10:16:29, magjed_webrtc wrote: > This is too early to call updateSurfaceSize(), the layout size has not changed > yet. You should call updateSurfaceSize() in onLayout() instead. I believe we need to keep this because layout might not change even though the frame size changes.
https://codereview.webrtc.org/2510413002/diff/120001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java (right): https://codereview.webrtc.org/2510413002/diff/120001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java:178: eglRenderer.surfaceSizeChanged(surfaceWidth, surfaceHeight); I don't think we should call eglRenderer.surfaceSizeChanged from here. https://codereview.webrtc.org/2510413002/diff/120001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java:209: updateSurfaceSize(); On 2016/11/21 15:59:46, sakal wrote: > What do you think about removing this? I think it should be safe and it seemed > to reduce the flickering in AppRTCMobile to my eye. Surface size changes shouldn't trigger new surface size changes. You should just call eglRenderer.surfaceSizeChanged here (and not from anywhere else I think). https://codereview.webrtc.org/2510413002/diff/160001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java (right): https://codereview.webrtc.org/2510413002/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java:50: private float layoutAspectRatio; We don't need to store this, so I think we shouldn't.
https://codereview.webrtc.org/2510413002/diff/120001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java (right): https://codereview.webrtc.org/2510413002/diff/120001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java:178: eglRenderer.surfaceSizeChanged(surfaceWidth, surfaceHeight); On 2016/11/21 17:08:02, magjed_webrtc wrote: > I don't think we should call eglRenderer.surfaceSizeChanged from here. I added it here to reduces glitchiness. Moved it to surfaceChanged per offline discussion. https://codereview.webrtc.org/2510413002/diff/120001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java:209: updateSurfaceSize(); On 2016/11/21 17:08:02, magjed_webrtc wrote: > On 2016/11/21 15:59:46, sakal wrote: > > What do you think about removing this? I think it should be safe and it seemed > > to reduce the flickering in AppRTCMobile to my eye. > > Surface size changes shouldn't trigger new surface size changes. You should just > call eglRenderer.surfaceSizeChanged here (and not from anywhere else I think). Done. https://codereview.webrtc.org/2510413002/diff/160001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java (right): https://codereview.webrtc.org/2510413002/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java:50: private float layoutAspectRatio; On 2016/11/21 17:08:02, magjed_webrtc wrote: > We don't need to store this, so I think we shouldn't. Done.
lgtm
The CQ bit was checked by sakal@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== Set surface view surface size to minimum of the layout size and frame size. This improves the performance of SurfaceViewRenderer. BUG=webrtc:6470 ========== to ========== Set surface view surface size to minimum of the layout size and frame size. This improves the performance of SurfaceViewRenderer. This feature is added behind a flag for now because it can be buggy on some devices. BUG=webrtc:6470 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_dbg/builds/8507)
The CQ bit was checked by sakal@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1479808061885520,
"parent_rev": "3e90662969596a535c9f88d5a938476676772385", "commit_rev":
"2853f100e6628597ec9a091adf726ade2a9320b7"}
Message was sent while issue was closed.
Description was changed from ========== Set surface view surface size to minimum of the layout size and frame size. This improves the performance of SurfaceViewRenderer. This feature is added behind a flag for now because it can be buggy on some devices. BUG=webrtc:6470 ========== to ========== Set surface view surface size to minimum of the layout size and frame size. This improves the performance of SurfaceViewRenderer. This feature is added behind a flag for now because it can be buggy on some devices. BUG=webrtc:6470 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Set surface view surface size to minimum of the layout size and frame size. This improves the performance of SurfaceViewRenderer. This feature is added behind a flag for now because it can be buggy on some devices. BUG=webrtc:6470 ========== to ========== Set surface view surface size to minimum of the layout size and frame size. This improves the performance of SurfaceViewRenderer. This feature is added behind a flag for now because it can be buggy on some devices. BUG=webrtc:6470 Committed: https://crrev.com/b1e6d5efa67edda6b06377b1b9cb50d295514a3d Cr-Commit-Position: refs/heads/master@{#15181} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b1e6d5efa67edda6b06377b1b9cb50d295514a3d Cr-Commit-Position: refs/heads/master@{#15181} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
