Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2456873002:
Android EglRenderer: Add Bitmap frame listener functionality (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. |
DescriptionAndroid EglRenderer: Add Bitmap frame listener functionality.
BUG=webrtc:6470
Committed: https://crrev.com/fb0c573263c619b7e67d4eddef0d5354590d340a
Cr-Commit-Position: refs/heads/master@{#14921}
Patch Set 1 : First review round. #Patch Set 2 : Add spaces. #
Total comments: 12
Patch Set 3 : Address magjed's comments #1. #Patch Set 4 : Add comments to the test and change some names. #Patch Set 5 : Remove unneeded glBindTexture, better error in test when device screen is off. #
Total comments: 30
Patch Set 6 : Changes according to magjed's comments. #2 #Patch Set 7 : Remove unneeded native method signature. #Patch Set 8 : Change no checkNoGLES2Error message. #
Total comments: 25
Patch Set 9 : Changes according to magjed's comments. #3 #Patch Set 10 : Speculatively increase timeout #Patch Set 11 : Move notifyCallbacks outside rending time measurement period. #
Messages
Total messages: 29 (18 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== WIP. Android EglRenderer: Add Bitmap frame listener functionality BUG=webrtc:6470 ========== to ========== WIP. Android EglRenderer: Add Bitmap frame listener functionality BUG=webrtc:6470 ==========
sakal@webrtc.org changed reviewers: + magjed@webrtc.org
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Description was changed from ========== WIP. Android EglRenderer: Add Bitmap frame listener functionality BUG=webrtc:6470 ========== to ========== Android EglRenderer: Add Bitmap frame listener functionality. BUG=webrtc:6470 ==========
PTAL
https://codereview.webrtc.org/2456873002/diff/140001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/EglRenderer.java (right): https://codereview.webrtc.org/2456873002/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/EglRenderer.java:94: private YuvConverter yuvConverter; Remove this now since you didn't end up using it. https://codereview.webrtc.org/2456873002/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/EglRenderer.java:128: private final int[] bitmapTexture = new int[1]; Keep this as an int instead of int[1]. Same for bitmapFramebuffer. https://codereview.webrtc.org/2456873002/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/EglRenderer.java:253: GLES20.glDeleteFramebuffers(1, bitmapFramebuffer, 0); Add 'if (bitmapFramebuffer[0] != 0)' check here. Same for bitmapTexture. https://codereview.webrtc.org/2456873002/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/EglRenderer.java:589: // Notify callbacks. Make temporary copy of callback list to avoid Move this block of code into a helper function. Also abort early if frameListeners is empty. Also, you need to synchronize on frameListenerLock. https://codereview.webrtc.org/2456873002/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/EglRenderer.java:597: final float[] bitmapMatrix = RendererCommon.multiplyMatrices( Move this outside the loop. https://codereview.webrtc.org/2456873002/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/EglRenderer.java:615: if (scaledWidth != bitmapTextureWidth || scaledHeight != bitmapTextureHeight) { Wait a minute... Maybe you can use the helper class GlTextureFrameBuffer for this?
https://codereview.webrtc.org/2456873002/diff/140001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/EglRenderer.java (right): https://codereview.webrtc.org/2456873002/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/EglRenderer.java:94: private YuvConverter yuvConverter; On 2016/11/01 18:06:46, magjed_webrtc wrote: > Remove this now since you didn't end up using it. Done. https://codereview.webrtc.org/2456873002/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/EglRenderer.java:128: private final int[] bitmapTexture = new int[1]; On 2016/11/01 18:06:46, magjed_webrtc wrote: > Keep this as an int instead of int[1]. Same for bitmapFramebuffer. Done. https://codereview.webrtc.org/2456873002/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/EglRenderer.java:253: GLES20.glDeleteFramebuffers(1, bitmapFramebuffer, 0); On 2016/11/01 18:06:46, magjed_webrtc wrote: > Add 'if (bitmapFramebuffer[0] != 0)' check here. Same for bitmapTexture. "glDeleteFramebuffers silently ignores 0's and names that do not correspond to existing framebuffer objects." Anyway, it is irrelevant now since I changed the code to use GlTextureFrameBuffer. https://codereview.webrtc.org/2456873002/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/EglRenderer.java:589: // Notify callbacks. Make temporary copy of callback list to avoid On 2016/11/01 18:06:46, magjed_webrtc wrote: > Move this block of code into a helper function. Also abort early if > frameListeners is empty. Also, you need to synchronize on frameListenerLock. Done. https://codereview.webrtc.org/2456873002/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/EglRenderer.java:597: final float[] bitmapMatrix = RendererCommon.multiplyMatrices( On 2016/11/01 18:06:46, magjed_webrtc wrote: > Move this outside the loop. Done. https://codereview.webrtc.org/2456873002/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/EglRenderer.java:615: if (scaledWidth != bitmapTextureWidth || scaledHeight != bitmapTextureHeight) { On 2016/11/01 18:06:46, magjed_webrtc wrote: > Wait a minute... Maybe you can use the helper class GlTextureFrameBuffer for > this? Done.
https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/EglRenderer.java (right): https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/EglRenderer.java:600: tmpList = new ArrayList<>(frameListeners); nit: add this first instead: if (frameListeners.isEmpty()) { return; } and remove the 'tmpList.size() == 0' below. https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/EglRenderer.java:630: drawer.drawYuv(yuvTextures, bitmapMatrix, frame.rotatedWidth(), frame.rotatedHeight(), 0, 0, nit: add literal comments, i.e. '0 /* viewportX */'. https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/EglRenderer.java:637: final IntBuffer bitmapBuffer = IntBuffer.allocate(scaledWidth * scaledHeight); Maybe this would be faster: final IntBuffer bitmapBuffer = ByteBuffer.allocateDirect(scaledWidth * scaledHeight).asIntBuffer(); Then we know that the buffer is direct and that no copy will be made to JNI. You could use GetDirectBufferAddress instead of GetIntArrayElements. https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/EglRenderer.java:646: final int[] bitmapArray = bitmapBuffer.array(); I think if we use .array(), we should also use arrayOffset(). arrayOffset() is probably 0 on at least 99% of all devices, but this might be a nasty bug. You can send the offset to nativeABGRToARGB and also use this version of Bitmap.createBitmap: Bitmap.createBitmap(int[] colors, int offset, int stride, int width, int height, Bitmap.Config config) Another variant would be to use: bitmap = Bitmap.createBitmap(int width, int height, Bitmap.Config config); bitmap.copyPixelsFromBuffer(bitmapBuffer); https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/androidtests/... File webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java (right): https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:27: final String TAG = "EglRendererTest"; Make these variables static. https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:54: private static class TestFrameListener implements EglRenderer.FrameListener { Do we really need a list of Bitmaps? Can't we have just one, and change getBitmaps() to public synchronized Bitmap getBitmaps(long timeoutMs) where we wait until we receive a Bitmap, or return null if we time out. Then we can remove the unconditional waits in the tests. https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:70: SurfaceView surfaceView; Instead of a SurfaceView, you can get a SurfaceTexture by doing this: oesTextureId = GlUtil.generateTexture(GLES11Ext.GL_TEXTURE_EXTERNAL_OES); surfaceTexture = new SurfaceTexture(oesTextureId); Then you can use the surfaceTexture in eglRenderer.createEglSurface(). https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:82: eglRenderer.init(null, EglBase.CONFIG_RGBA, new GlRectDrawer()); nit: comment this null, i.e. 'null /* sharedContext */' https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:120: private void checkBitmap(Bitmap bitmap, float scale) { nit: Make this function static. https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:134: private float linearSample(ByteBuffer data, int x, int y) { nit: Make this function static. https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:134: private float linearSample(ByteBuffer data, int x, int y) { I would like to pass in stride, width, and height of the ByteBuffer and make the x and y parameters float in range [0, 1]. So the '(x + 0.5f) / TEST_FRAME_WIDTH' will be done by the caller. I think that's clearer even if it's slightly more code. https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:137: float coordX = (x + 0.5f) / TEST_FRAME_WIDTH * (TEST_FRAME_WIDTH / 2f); nit: Make these final https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:140: int lowX = (int) Math.floor(coordX - 0.5f); I would like this to be something like final int lowIndexX = (int) Math.max(0, Math.floor(x * (width - 1))); https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:208: Logging.d(TAG, "Expected bitmap content: " + Arrays.toString(expectedRGBA)); These seems like excessive logs. I think it's better to only log when it's failing. https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:214: if (value < expected - 2 || value > expected + 2) { nit: This is cleaner; Math.abs(value - expected) > 2.
Patchset #6 (id:220001) has been deleted
https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/EglRenderer.java (right): https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/EglRenderer.java:600: tmpList = new ArrayList<>(frameListeners); On 2016/11/02 12:00:19, magjed_webrtc wrote: > nit: add this first instead: > if (frameListeners.isEmpty()) { > return; > } > and remove the 'tmpList.size() == 0' below. Done. https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/EglRenderer.java:630: drawer.drawYuv(yuvTextures, bitmapMatrix, frame.rotatedWidth(), frame.rotatedHeight(), 0, 0, On 2016/11/02 12:00:19, magjed_webrtc wrote: > nit: add literal comments, i.e. '0 /* viewportX */'. Done. https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/EglRenderer.java:637: final IntBuffer bitmapBuffer = IntBuffer.allocate(scaledWidth * scaledHeight); On 2016/11/02 12:00:19, magjed_webrtc wrote: > Maybe this would be faster: > final IntBuffer bitmapBuffer = ByteBuffer.allocateDirect(scaledWidth * > scaledHeight).asIntBuffer(); > Then we know that the buffer is direct and that no copy will be made to JNI. You > could use GetDirectBufferAddress instead of GetIntArrayElements. The problem was ByteBuffer converted to IntBuffer wouldn't return an array. I changed the code to use copyPixelsFromBuffer. Mysteriously, this removed the need for nativeABGRToARGB. https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/EglRenderer.java:646: final int[] bitmapArray = bitmapBuffer.array(); On 2016/11/02 12:00:19, magjed_webrtc wrote: > I think if we use .array(), we should also use arrayOffset(). arrayOffset() is > probably 0 on at least 99% of all devices, but this might be a nasty bug. You > can send the offset to nativeABGRToARGB and also use this version of > Bitmap.createBitmap: > Bitmap.createBitmap(int[] colors, int offset, int stride, int width, int height, > Bitmap.Config config) > > Another variant would be to use: > bitmap = Bitmap.createBitmap(int width, int height, Bitmap.Config config); > bitmap.copyPixelsFromBuffer(bitmapBuffer); Acknowledged. https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/androidtests/... File webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java (right): https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:27: final String TAG = "EglRendererTest"; On 2016/11/02 12:00:20, magjed_webrtc wrote: > Make these variables static. Done. https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:54: private static class TestFrameListener implements EglRenderer.FrameListener { On 2016/11/02 12:00:19, magjed_webrtc wrote: > Do we really need a list of Bitmaps? Can't we have just one, and change > getBitmaps() to > public synchronized Bitmap getBitmaps(long timeoutMs) > where we wait until we receive a Bitmap, or return null if we time out. Then we > can remove the unconditional waits in the tests. Done. https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:70: SurfaceView surfaceView; On 2016/11/02 12:00:20, magjed_webrtc wrote: > Instead of a SurfaceView, you can get a SurfaceTexture by doing this: > oesTextureId = GlUtil.generateTexture(GLES11Ext.GL_TEXTURE_EXTERNAL_OES); > surfaceTexture = new SurfaceTexture(oesTextureId); > Then you can use the surfaceTexture in eglRenderer.createEglSurface(). Done. https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:82: eglRenderer.init(null, EglBase.CONFIG_RGBA, new GlRectDrawer()); On 2016/11/02 12:00:19, magjed_webrtc wrote: > nit: comment this null, i.e. 'null /* sharedContext */' Done. https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:120: private void checkBitmap(Bitmap bitmap, float scale) { On 2016/11/02 12:00:19, magjed_webrtc wrote: > nit: Make this function static. Done. https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:134: private float linearSample(ByteBuffer data, int x, int y) { On 2016/11/02 12:00:19, magjed_webrtc wrote: > nit: Make this function static. Done. https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:137: float coordX = (x + 0.5f) / TEST_FRAME_WIDTH * (TEST_FRAME_WIDTH / 2f); On 2016/11/02 12:00:20, magjed_webrtc wrote: > nit: Make these final Done. https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:140: int lowX = (int) Math.floor(coordX - 0.5f); On 2016/11/02 12:00:20, magjed_webrtc wrote: > I would like this to be something like > final int lowIndexX = (int) Math.max(0, Math.floor(x * (width - 1))); Discussed offline. https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:208: Logging.d(TAG, "Expected bitmap content: " + Arrays.toString(expectedRGBA)); On 2016/11/02 12:00:19, magjed_webrtc wrote: > These seems like excessive logs. I think it's better to only log when it's > failing. Done. https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:214: if (value < expected - 2 || value > expected + 2) { On 2016/11/02 12:00:19, magjed_webrtc wrote: > nit: This is cleaner; Math.abs(value - expected) > 2. Done.
I just have a few nit comments left. Will stamp this CL once they are addressed. https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/EglRenderer.java (right): https://codereview.webrtc.org/2456873002/diff/200001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/EglRenderer.java:637: final IntBuffer bitmapBuffer = IntBuffer.allocate(scaledWidth * scaledHeight); On 2016/11/03 12:27:41, sakal wrote: > On 2016/11/02 12:00:19, magjed_webrtc wrote: > > Maybe this would be faster: > > final IntBuffer bitmapBuffer = ByteBuffer.allocateDirect(scaledWidth * > > scaledHeight).asIntBuffer(); > > Then we know that the buffer is direct and that no copy will be made to JNI. > You > > could use GetDirectBufferAddress instead of GetIntArrayElements. > > The problem was ByteBuffer converted to IntBuffer wouldn't return an array. I > changed the code to use copyPixelsFromBuffer. Mysteriously, this removed the > need for nativeABGRToARGB. Alright, a bit scary though. I hope Java guarantees that we will get the same result regardless if the architecture is big or little endian (I think so). https://codereview.webrtc.org/2456873002/diff/280001/webrtc/api/androidtests/... File webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java (right): https://codereview.webrtc.org/2456873002/diff/280001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:70: public synchronized boolean waitForBitmap(int timeout) throws InterruptedException { nit: add ms suffix https://codereview.webrtc.org/2456873002/diff/280001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:99: eglRenderer.surfaceSizeChanged(1 /* width */, 1 /* height */); nit: move this below createEglSurface (that's the call sequence in production code). https://codereview.webrtc.org/2456873002/diff/280001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:106: int[] textureArray = new int[] {oesTextureId}; nit: just inline this https://codereview.webrtc.org/2456873002/diff/280001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:134: final int lowX = (int) Math.floor(coordX - 0.5f); Maybe add 'index' to the variable name, i.e. lowIndexX? https://codereview.webrtc.org/2456873002/diff/280001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:139: final float highSampleX = coordX - lowX - 0.5f; I would prefer to call these variables 'weight' instead of 'sample', i.e. highWeightX. https://codereview.webrtc.org/2456873002/diff/280001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:145: final int sampleCoordLowX = Math.max(0, lowY); You use lowY here, but it should be lowX. https://codereview.webrtc.org/2456873002/diff/280001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:145: final int sampleCoordLowX = Math.max(0, lowY); I would actually prefer to reuse lowX/lowIndexX here, i.e. make it non-final and clamp it here. https://codereview.webrtc.org/2456873002/diff/280001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:155: return (lowSampleY * lowYValue + highSampleY * highYValue) / 255f - 0.5f; I don't think the -0.5f should be part of this function. It's an implementation detail of YUV->RGB conversion. https://codereview.webrtc.org/2456873002/diff/280001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:183: final float rC = Math.max(0f, Math.min(1f, yC + 1.403f * vC)); I would like to introduce a small helper function: // Convert float color value in range [0f, 1f] to byte. private static byte saturatedFloatToByte(float c) { return (byte) Math.round(255f * Math.max(0, Math.min(c, 1))); } https://codereview.webrtc.org/2456873002/diff/280001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:209: if (Math.abs(value - expected) > 2) { So, we still have to accept +/- 2? (That's alright, I'm just curious). https://codereview.webrtc.org/2456873002/diff/280001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:227: eglRenderer.addFrameListener(testFrameListener, 0f); comment /* scaleFactor */ https://codereview.webrtc.org/2456873002/diff/280001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:229: assertEquals(true, testFrameListener.waitForBitmap(RENDER_WAIT_MS)); assertTrue https://codereview.webrtc.org/2456873002/diff/280001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:237: assertEquals(false, testFrameListener.waitForBitmap(RENDER_WAIT_MS)); assertFalse
https://codereview.webrtc.org/2456873002/diff/280001/webrtc/api/androidtests/... File webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java (right): https://codereview.webrtc.org/2456873002/diff/280001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:70: public synchronized boolean waitForBitmap(int timeout) throws InterruptedException { On 2016/11/03 13:10:06, magjed_webrtc wrote: > nit: add ms suffix Done. https://codereview.webrtc.org/2456873002/diff/280001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:99: eglRenderer.surfaceSizeChanged(1 /* width */, 1 /* height */); On 2016/11/03 13:10:06, magjed_webrtc wrote: > nit: move this below createEglSurface (that's the call sequence in production > code). Done. https://codereview.webrtc.org/2456873002/diff/280001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:106: int[] textureArray = new int[] {oesTextureId}; On 2016/11/03 13:10:06, magjed_webrtc wrote: > nit: just inline this Done. https://codereview.webrtc.org/2456873002/diff/280001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:134: final int lowX = (int) Math.floor(coordX - 0.5f); On 2016/11/03 13:10:06, magjed_webrtc wrote: > Maybe add 'index' to the variable name, i.e. lowIndexX? Done. https://codereview.webrtc.org/2456873002/diff/280001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:139: final float highSampleX = coordX - lowX - 0.5f; On 2016/11/03 13:10:06, magjed_webrtc wrote: > I would prefer to call these variables 'weight' instead of 'sample', i.e. > highWeightX. Done. https://codereview.webrtc.org/2456873002/diff/280001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:145: final int sampleCoordLowX = Math.max(0, lowY); On 2016/11/03 13:10:06, magjed_webrtc wrote: > You use lowY here, but it should be lowX. Done, I modified the test data to try to catch these kind of problems in the future. https://codereview.webrtc.org/2456873002/diff/280001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:155: return (lowSampleY * lowYValue + highSampleY * highYValue) / 255f - 0.5f; On 2016/11/03 13:10:06, magjed_webrtc wrote: > I don't think the -0.5f should be part of this function. It's an implementation > detail of YUV->RGB conversion. Done. https://codereview.webrtc.org/2456873002/diff/280001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:183: final float rC = Math.max(0f, Math.min(1f, yC + 1.403f * vC)); On 2016/11/03 13:10:06, magjed_webrtc wrote: > I would like to introduce a small helper function: > // Convert float color value in range [0f, 1f] to byte. > private static byte saturatedFloatToByte(float c) { > return (byte) Math.round(255f * Math.max(0, Math.min(c, 1))); > } Done. https://codereview.webrtc.org/2456873002/diff/280001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:209: if (Math.abs(value - expected) > 2) { On 2016/11/03 13:10:06, magjed_webrtc wrote: > So, we still have to accept +/- 2? (That's alright, I'm just curious). Not anymore, x vs y bug caused this. Now it is +/- 1. https://codereview.webrtc.org/2456873002/diff/280001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:227: eglRenderer.addFrameListener(testFrameListener, 0f); On 2016/11/03 13:10:06, magjed_webrtc wrote: > comment /* scaleFactor */ Done. https://codereview.webrtc.org/2456873002/diff/280001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:229: assertEquals(true, testFrameListener.waitForBitmap(RENDER_WAIT_MS)); On 2016/11/03 13:10:06, magjed_webrtc wrote: > assertTrue Done. https://codereview.webrtc.org/2456873002/diff/280001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java:237: assertEquals(false, testFrameListener.waitForBitmap(RENDER_WAIT_MS)); On 2016/11/03 13:10:06, magjed_webrtc wrote: > assertFalse Done.
lgtm
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_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/18266)
The CQ bit was checked by sakal@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org Link to the patchset: https://codereview.webrtc.org/2456873002/#ps340001 (title: "Move notifyCallbacks outside rending time measurement period.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Android EglRenderer: Add Bitmap frame listener functionality. BUG=webrtc:6470 ========== to ========== Android EglRenderer: Add Bitmap frame listener functionality. BUG=webrtc:6470 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Android EglRenderer: Add Bitmap frame listener functionality. BUG=webrtc:6470 ========== to ========== Android EglRenderer: Add Bitmap frame listener functionality. BUG=webrtc:6470 Committed: https://crrev.com/fb0c573263c619b7e67d4eddef0d5354590d340a Cr-Commit-Position: refs/heads/master@{#14921} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/fb0c573263c619b7e67d4eddef0d5354590d340a Cr-Commit-Position: refs/heads/master@{#14921} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
