Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1460703002:
Implement AndroidTextureBuffer::NativeToI420. (Closed)
|
Created:
5 years, 1 month ago by nisse-webrtc Modified:
5 years ago Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionImplement NativeToI420Buffer in C++, calling java SurfaceTextureHelper, new method .textureToYUV, to
do the conversion using an opengl fragment shader.
BUG=webrtc:4993
Committed: https://crrev.com/c490e01bd1bd4a0d754ed5f746b95ac03136346f
Cr-Commit-Position: refs/heads/master@{#10972}
Patch Set 1 #Patch Set 2 : Install SurfaceTextureHelper pointer into the c++ AndroidTextureBuffer. #Patch Set 3 : Implement .textureToRGBA #Patch Set 4 : Rebased #Patch Set 5 : GPU conversion to YUV now working. #Patch Set 6 : Rebase and related fixes. #Patch Set 7 : Get a shared ref to the java SurfaceTectureHelper. #
Total comments: 28
Patch Set 8 : Deleted now unused ConvertRGBA code. #Patch Set 9 : Addressed Magnus' comments. #
Total comments: 23
Patch Set 10 : Rebase, EglBase refactoring was landed separately. #Patch Set 11 : Addressed perkj's comments. #Patch Set 12 : Minor tweak, whitespace, drop logging.h include. #
Total comments: 10
Patch Set 13 : Comment improvements and some cleanup. #
Total comments: 4
Patch Set 14 : Addressed nits #Patch Set 15 : Added testcase. Fix lost detachCurrent calls. #Patch Set 16 : Deleted unused variable #
Total comments: 12
Patch Set 17 : Testcase cleanup. Revert EglBase helper method. #
Total comments: 2
Patch Set 18 : Fix comment nits. #Messages
Total messages: 40 (15 generated)
Description was changed from ========== Add SurfaceTextureHelper as argument to onTextureFrameAvailable, and down the chain. Dummy NativeToI420Buffer implementation, in C++. Enable texture-mode video capture. Depends on 1440343002. ========== to ========== Add SurfaceTextureHelper as argument to onTextureFrameAvailable, and down the chain. Dummy NativeToI420Buffer implementation, in C++. Enable texture-mode video capture. Depends on 1440343002. ==========
nisse@webrtc.org changed reviewers: + perkj@webrtc.org
Description was changed from ========== Add SurfaceTextureHelper as argument to onTextureFrameAvailable, and down the chain. Dummy NativeToI420Buffer implementation, in C++. Enable texture-mode video capture. Depends on 1440343002. ========== to ========== Add SurfaceTextureHelper as argument to onTextureFrameAvailable, and down the chain. Implement NativeToI420Buffer in C++, calling java SurfaceTectureHelper, new method .textureToRGBA. Enable texture-mode video capture. ==========
Conversion now working, but slow. Rebased, after perkj's related changes were landed.
Description was changed from ========== Add SurfaceTextureHelper as argument to onTextureFrameAvailable, and down the chain. Implement NativeToI420Buffer in C++, calling java SurfaceTectureHelper, new method .textureToRGBA. Enable texture-mode video capture. ========== to ========== Add SurfaceTextureHelper as argument to onTextureFrameAvailable, and down the chain. Implement NativeToI420Buffer in C++, calling java SurfaceTextureHelper, new method .textureToYUV, to do the conversion using an opengl fragment shader. Enable texture-mode video capture. ==========
nisse@webrtc.org changed reviewers: + magjed@webrtc.org
Conversion seems to work now, and with reasonable speed. Next, I'll rebase, and then it needs some cleanup.
Now AndroidVideoCapturerJni keeps a reference to the java SurfaceTextureHelper, and this reference is copied into the frame objects (via NativeHandleImpl) without any additional calls to NewGlobalRef/DeleteGlobalRef.
https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/and... File talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java (right): https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:102: static private class ConvertRGBA { I don't want to land a ConvertToRGBA until we need it, so you can remove it from this CL. https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:214: + " texture2D(oesTex, interp_tc - 1.5 * xUnit).rgb);\n" The alpha value from the texture2D should always be 1, so just do a full dot, i.e. dot(coeffs, texture2D(oesTex, interp_tc). You can also experiment with: gl_FragColor = mat4(texture2D(oesTex, interp_tc - 1.5 * xUnit), texture2D(oesTex, interp_tc - 0.5 * xUnit), texture2D(oesTex, interp_tc + 0.5 * xUnit), texture2D(oesTex, interp_tc + 1.5 * xUnit)) * coeffs; https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:231: try { You don't need this try-finally. Detaching the EGLContext is not critical when a RuntimeException is thrown. https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:251: int width, int height, int stride, int textureId, float [] transformMatrix) { why do you need a stride value? width/height should be enough? https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:252: synchronized(this) { If the whole function is synchronized on |this|, you can just write public void synchronized convert() instead. https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:260: "Invalid stride, must be a multiple of 8"); why? https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:295: throw new IllegalStateException("ConvertYUV.convert called with too small buffer"); IllegalArgument https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:386: eglBase.makeCurrent(); shader.release() need an EGLContext current, so you need to move makeCurrent() above. https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/jni... File talk/app/webrtc/java/jni/androidvideocapturer_jni.cc (right): https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/androidvideocapturer_jni.cc:237: capturer->GetSurfaceHelper())); If this is the only use of GetSurfaceHelper(), I would prefer if you changed OnTextureFrame() instead to take |j_oes_texture_id| and |j_transform_matrix| as argument, and create the NativeHandleImpl inside that function where you have private access. https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/jni... File talk/app/webrtc/java/jni/native_handle_impl.cc (right): https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/native_handle_impl.cc:55: // TODO(nisse): Hold on to java float array instead, since it is Yes, but then you need a ScopedGlobalRef or similar. I prefer to copy the floats for simplicity. If we want to reduce overhead, we can reduce the 4x4 matrix to 2x3. https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/native_handle_impl.cc:91: // deleted by its destructor callback. Yes, use scoped_ptr here... https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/native_handle_impl.cc:103: rtc::Bind(&webrtc::AlignedFree, y_data)); ...and y_data.release() here. Btw, you need to change the name |y_data| now. https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/jni... File talk/app/webrtc/java/jni/native_handle_impl.h (right): https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/native_handle_impl.h:46: // Raw object pointer, relying on AndroidVideoCapturerJni to keep a Can't you place the |surface_texture_helper| in AndroidTextureBuffer instead?
Deleted the unused RGBA-related code. More fixes coming. https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/and... File talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java (right): https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:102: static private class ConvertRGBA { On 2015/12/03 12:33:58, magjed_webrtc wrote: > I don't want to land a ConvertToRGBA until we need it, so you can remove it from > this CL. I'll do. I wonder if it's worth the effort to keep the code around in some local branch, though. https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:251: int width, int height, int stride, int textureId, float [] transformMatrix) { On 2015/12/03 12:33:58, magjed_webrtc wrote: > why do you need a stride value? width/height should be enough? To allow for some additional alignment, and at the same time avoid duplicating the code which selects the stride. https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:252: synchronized(this) { On 2015/12/03 12:33:58, magjed_webrtc wrote: > If the whole function is synchronized on |this|, you can just write public void > synchronized convert() instead. Acknowledged. https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:260: "Invalid stride, must be a multiple of 8"); On 2015/12/03 12:33:58, magjed_webrtc wrote: > why? Because we do a split in half for drawing u and v, and the v part must start at an x coordinate which is a multiple of 4. I ought to write a comment somewhere explaining the layout. https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/jni... File talk/app/webrtc/java/jni/native_handle_impl.cc (right): https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/native_handle_impl.cc:55: // TODO(nisse): Hold on to java float array instead, since it is On 2015/12/03 12:33:58, magjed_webrtc wrote: > Yes, but then you need a ScopedGlobalRef or similar. I prefer to copy the floats > for simplicity. If we want to reduce overhead, we can reduce the 4x4 matrix to > 2x3. If we add a reference to the java array, can we delete the C++ sampling_matrix? Or do we need both? https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/native_handle_impl.cc:103: rtc::Bind(&webrtc::AlignedFree, y_data)); On 2015/12/03 12:33:58, magjed_webrtc wrote: > ...and y_data.release() here. Btw, you need to change the name |y_data| now. Rename, why and to what? https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/jni... File talk/app/webrtc/java/jni/native_handle_impl.h (right): https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/native_handle_impl.h:46: // Raw object pointer, relying on AndroidVideoCapturerJni to keep a On 2015/12/03 12:33:58, magjed_webrtc wrote: > Can't you place the |surface_texture_helper| in AndroidTextureBuffer instead? I can. And then the GetSurfaceHelper method is no longer needed. I don't understand why things are divided between AndroidTextureBuffer and NativeHandleImpl. Maybe there is, or was, a good reason.
https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/and... File talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java (right): https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:102: static private class ConvertRGBA { On 2015/12/03 13:35:20, nisse-webrtc wrote: > On 2015/12/03 12:33:58, magjed_webrtc wrote: > > I don't want to land a ConvertToRGBA until we need it, so you can remove it > from > > this CL. > > I'll do. I wonder if it's worth the effort to keep the code around in some local > branch, though. Yes, do that :) It is also saved in the patch history here. https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/jni... File talk/app/webrtc/java/jni/native_handle_impl.cc (right): https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/native_handle_impl.cc:55: // TODO(nisse): Hold on to java float array instead, since it is On 2015/12/03 13:35:20, nisse-webrtc wrote: > On 2015/12/03 12:33:58, magjed_webrtc wrote: > > Yes, but then you need a ScopedGlobalRef or similar. I prefer to copy the > floats > > for simplicity. If we want to reduce overhead, we can reduce the 4x4 matrix to > > 2x3. > > If we add a reference to the java array, can we delete the C++ sampling_matrix? > Or do we need both? Yes, then we wouldn't need the C++ sampling_matrix, but please don't add a reference to the java array, I'm sure it will just complicate things in the future. It's just a few bytes, like width and height, it's simpler to copy those variables than to use global refs/reference counting. https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/native_handle_impl.cc:103: rtc::Bind(&webrtc::AlignedFree, y_data)); On 2015/12/03 13:35:20, nisse-webrtc wrote: > On 2015/12/03 12:33:58, magjed_webrtc wrote: > > ...and y_data.release() here. Btw, you need to change the name |y_data| now. > > Rename, why and to what? Why? - Because it is not just y data anymore. To what? - Maybe yuv_data?
Description was changed from ========== Add SurfaceTextureHelper as argument to onTextureFrameAvailable, and down the chain. Implement NativeToI420Buffer in C++, calling java SurfaceTextureHelper, new method .textureToYUV, to do the conversion using an opengl fragment shader. Enable texture-mode video capture. ========== to ========== Implement NativeToI420Buffer in C++, calling java SurfaceTextureHelper, new method .textureToYUV, to do the conversion using an opengl fragment shader. ==========
https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/and... File talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java (right): https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:231: try { On 2015/12/03 12:33:58, magjed_webrtc wrote: > You don't need this try-finally. Detaching the EGLContext is not critical when a > RuntimeException is thrown. Done. https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:252: synchronized(this) { On 2015/12/03 12:33:58, magjed_webrtc wrote: > If the whole function is synchronized on |this|, you can just write public void > synchronized convert() instead. Done. https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:295: throw new IllegalStateException("ConvertYUV.convert called with too small buffer"); On 2015/12/03 12:33:58, magjed_webrtc wrote: > IllegalArgument Done. https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:386: eglBase.makeCurrent(); On 2015/12/03 12:33:58, magjed_webrtc wrote: > shader.release() need an EGLContext current, so you need to move makeCurrent() > above. Done. https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/jni... File talk/app/webrtc/java/jni/native_handle_impl.cc (right): https://codereview.webrtc.org/1460703002/diff/120001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/native_handle_impl.cc:91: // deleted by its destructor callback. On 2015/12/03 12:33:58, magjed_webrtc wrote: > Yes, use scoped_ptr here... Done. Not sure if it's an improvement, though. We allocate the buffer, then pass ownership of it to the frame. And at the same time we also wrap it in a java ByteBuffer, which hopefully isn't used beyond the lifetime of the pixel data. Maybe it would be cleaner to arrange so that the data is owned by the ByteBuffer, and let the frame "own" a reference to that buffer.
https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/and... File talk/app/webrtc/java/android/org/webrtc/EglBase.java (left): https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/EglBase.java:170: if (configType == ConfigType.PIXEL_BUFFER) { Why remove these logs? https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/EglBase.java:190: if (configType != ConfigType.PIXEL_BUFFER) { dito? Why remove? https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/and... File talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java (right): https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:102: static private class ConvertYUV { rename YuvConverter? https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:153: // + " gl_FragColor = coeffs * mat4(\n" remove code you don't use. If you want, you can write a comment before gl_FragColor and explain why you do what you do. https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:199: synchronized public void convert(ByteBuffer buf, not for use outside webrtc.org. Remove public. https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:201: // TODO(nisse): For now only produces the Y plane. Really? https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:221: Logging.e(TAG, "width: " + width + ", height: " + height + remove per frame logging https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:466: Logging.e(TAG, "textureToYUV called"); remove logging. https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/jni... File talk/app/webrtc/java/jni/androidmediadecoder_jni.cc (right): https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:642: } revert change https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/jni... File talk/app/webrtc/java/jni/androidvideocapturer_jni.cc (right): https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/androidvideocapturer_jni.cc:234: capturer->OnTextureFrame(j_width, j_height, j_timestamp, revert change https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/jni... File talk/app/webrtc/java/jni/native_handle_impl.cc (right): https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/native_handle_impl.cc:28: #include <android/log.h> remove this include. https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/native_handle_impl.cc:76: #define TAG "AndroidTextureBuffer" remove https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/native_handle_impl.cc:90: rtc::scoped_ptr<uint8_t, webrtc::AlignedFreeDeleter> yuv_data( Should we have AndroidTextureBuffer take pointer to a I420BufferPool as well so that we don' have to allocate each frame?
I've rebased, since the EglBase refactoring was landed previously. If you still want to replace the configType checks with something else, note that those changes are no longer part of this cl. https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/and... File talk/app/webrtc/java/android/org/webrtc/EglBase.java (left): https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/EglBase.java:170: if (configType == ConfigType.PIXEL_BUFFER) { On 2015/12/07 12:53:50, perkj1 wrote: > Why remove these logs? Because the configType enum no longer exists. To keep these checks, we'd need to either query the underlying opengl object (not sure how), or check ourselves for relevant attributes on the passed in configAttributes. What do you suggest?
https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/and... File talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java (right): https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:102: static private class ConvertYUV { On 2015/12/07 12:53:50, perkj1 wrote: > rename YuvConverter? Done. https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:153: // + " gl_FragColor = coeffs * mat4(\n" On 2015/12/07 12:53:50, perkj1 wrote: > remove code you don't use. If you want, you can write a comment before > gl_FragColor and explain why you do what you do. Done. https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:199: synchronized public void convert(ByteBuffer buf, On 2015/12/07 12:53:50, perkj1 wrote: > not for use outside http://webrtc.org. Remove public. Done. https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:201: // TODO(nisse): For now only produces the Y plane. On 2015/12/07 12:53:50, perkj1 wrote: > Really? Removed. https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/jni... File talk/app/webrtc/java/jni/androidmediadecoder_jni.cc (right): https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:642: } On 2015/12/07 12:53:50, perkj1 wrote: > revert change Done. https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/jni... File talk/app/webrtc/java/jni/androidvideocapturer_jni.cc (right): https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/androidvideocapturer_jni.cc:234: capturer->OnTextureFrame(j_width, j_height, j_timestamp, On 2015/12/07 12:53:50, perkj1 wrote: > revert change Done. https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/jni... File talk/app/webrtc/java/jni/native_handle_impl.cc (right): https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/native_handle_impl.cc:28: #include <android/log.h> On 2015/12/07 12:53:50, perkj1 wrote: > remove this include. Done. And removed corresponding logging. https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/native_handle_impl.cc:90: rtc::scoped_ptr<uint8_t, webrtc::AlignedFreeDeleter> yuv_data( On 2015/12/07 12:53:50, perkj1 wrote: > Should we have AndroidTextureBuffer take pointer to a I420BufferPool as well so > that we don' have to allocate each frame? Note that we use WrappedI420Buffer. To use a plain I420Buffer requires changes to support the special layout with the same stride for Y, U and v. To avoid a large allocation for eac frame, I think it might be cleanest to have a pool of java ByteBuffer which are reused, and let java own the storage. Creating a new WrappedI420Buffer per frame should be cheap. Can we have more than one frame alive at a time? We only have one texture anyway, right? In case allocation/gc overhead matters, I guess that a cache of at most one free buffer should be simple and quite effective.
https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/jni... File talk/app/webrtc/java/jni/native_handle_impl.cc (right): https://codereview.webrtc.org/1460703002/diff/160001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/native_handle_impl.cc:90: rtc::scoped_ptr<uint8_t, webrtc::AlignedFreeDeleter> yuv_data( On 2015/12/08 11:44:51, nisse-webrtc wrote: > On 2015/12/07 12:53:50, perkj1 wrote: > > Should we have AndroidTextureBuffer take pointer to a I420BufferPool as well > so > > that we don' have to allocate each frame? > > Note that we use WrappedI420Buffer. To use a plain I420Buffer requires changes > to support the special layout with the same stride for Y, U and v. > > To avoid a large allocation for eac frame, I think it might be cleanest to have > a pool of java ByteBuffer which are reused, and let java own the storage. > Creating a new WrappedI420Buffer per frame should be cheap. > > Can we have more than one frame alive at a time? We only have one texture > anyway, right? > > In case allocation/gc overhead matters, I guess that a cache of at most one free > buffer should be simple and quite effective. Yes, we can have memory backed frames active at the same time. But, yes, there can only be one texture buffer used outside the camera at once. https://codereview.webrtc.org/1460703002/diff/220001/talk/app/webrtc/java/and... File talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java (right): https://codereview.webrtc.org/1460703002/diff/220001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:218: // Reuse surface, if possible. TODO(nisse): Add an eglBase I would recommend to do this now or remove the todo. Todos have way of never getting fixed if they are not immedately attended to. I am fine with the way it is. https://codereview.webrtc.org/1460703002/diff/220001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:232: GlUtil.checkNoGLES2Error("Initialize fragment shader uniform values."); Can we skip checking for error on every line. Just check at the bottom of this method. https://codereview.webrtc.org/1460703002/diff/220001/talk/app/webrtc/java/jni... File talk/app/webrtc/java/jni/native_handle_impl.cc (right): https://codereview.webrtc.org/1460703002/diff/220001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/native_handle_impl.cc:51: // TODO(nisse): Hold on to java float array instead, since it is remove this todo. It will not be true when I land the cvo patch. https://codereview.webrtc.org/1460703002/diff/220001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/native_handle_impl.cc:76: int y_width = (width()+3) / 4; please add comment about the memory layout. https://codereview.webrtc.org/1460703002/diff/220001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/native_handle_impl.cc:82: // data is deleted by the frame's destructor callback. Add TODO to not allocate on each frame. I think we should look into using a pool here as we talked about earlier.
https://codereview.webrtc.org/1460703002/diff/220001/talk/app/webrtc/java/and... File talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java (right): https://codereview.webrtc.org/1460703002/diff/220001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:218: // Reuse surface, if possible. TODO(nisse): Add an eglBase On 2015/12/09 09:28:28, perkj1 wrote: > I would recommend to do this now or remove the todo. Todos have way of never > getting fixed if they are not immedately attended to. I am fine with the way it > is. I'm adding a helper function, withPbufferSurface. Or is it better to extend createPbufferSurface to reuse or recreate the surface as needed? https://codereview.webrtc.org/1460703002/diff/220001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:232: GlUtil.checkNoGLES2Error("Initialize fragment shader uniform values."); On 2015/12/09 09:28:28, perkj1 wrote: > Can we skip checking for error on every line. Just check at the bottom of this > method. Done. https://codereview.webrtc.org/1460703002/diff/220001/talk/app/webrtc/java/jni... File talk/app/webrtc/java/jni/native_handle_impl.cc (right): https://codereview.webrtc.org/1460703002/diff/220001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/native_handle_impl.cc:51: // TODO(nisse): Hold on to java float array instead, since it is On 2015/12/09 09:28:28, perkj1 wrote: > remove this todo. It will not be true when I land the cvo patch. Done. https://codereview.webrtc.org/1460703002/diff/220001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/native_handle_impl.cc:76: int y_width = (width()+3) / 4; On 2015/12/09 09:28:28, perkj1 wrote: > please add comment about the memory layout. Done, but I put the comment in SurfaceTextureHelper.java instead. https://codereview.webrtc.org/1460703002/diff/220001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/native_handle_impl.cc:82: // data is deleted by the frame's destructor callback. On 2015/12/09 09:28:28, perkj1 wrote: > Add TODO to not allocate on each frame. I think we should look into using a pool > here as we talked about earlier. Done.
lgtm if the nits are addressed. https://codereview.webrtc.org/1460703002/diff/240001/talk/app/webrtc/java/and... File talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java (right): https://codereview.webrtc.org/1460703002/diff/240001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:284: // Only a single call to at the end, when operations are complete. remove this comment please, it is not needed. https://codereview.webrtc.org/1460703002/diff/240001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:292: synchronized public void release() { not public please. https://codereview.webrtc.org/1460703002/diff/240001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:330: // yuvConverter is assign once s/ assigned https://codereview.webrtc.org/1460703002/diff/240001/talk/app/webrtc/java/jni... File talk/app/webrtc/java/jni/native_handle_impl.cc (right): https://codereview.webrtc.org/1460703002/diff/240001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/native_handle_impl.cc:111: memset(u_data, 0x80, stride * uv_height); remove
Description was changed from ========== Implement NativeToI420Buffer in C++, calling java SurfaceTextureHelper, new method .textureToYUV, to do the conversion using an opengl fragment shader. ========== to ========== Implement NativeToI420Buffer in C++, calling java SurfaceTextureHelper, new method .textureToYUV, to do the conversion using an opengl fragment shader. BUG=webrtc:4993 ==========
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1460703002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1460703002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/11221)
New testcase.
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1460703002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1460703002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_compile_dbg/build...)
not lgtm .... But should be easy to fix. https://codereview.webrtc.org/1460703002/diff/300001/talk/app/webrtc/androidt... File talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java (right): https://codereview.webrtc.org/1460703002/diff/300001/talk/app/webrtc/androidt... talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java:170: * {@code threshold}. Maybe belongs in MoreAsserts.java. */ Remove comment Maybe belongs.... https://codereview.webrtc.org/1460703002/diff/300001/talk/app/webrtc/androidt... talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java:171: public static void assertClose(int threshold, int expected, int actual) { Move up to before the first test. https://codereview.webrtc.org/1460703002/diff/300001/talk/app/webrtc/androidt... talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java:177: static public void assertEqualsButOne(int expected, int actual) { public static? https://codereview.webrtc.org/1460703002/diff/300001/talk/app/webrtc/androidt... talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java:177: static public void assertEqualsButOne(int expected, int actual) { This looks like assertClose but with tresh hold 1. Remove method? https://codereview.webrtc.org/1460703002/diff/300001/talk/app/webrtc/androidt... talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java:184: public static void testTexturetoYUV() throws InterruptedException { Move to the end of this file. https://codereview.webrtc.org/1460703002/diff/300001/talk/app/webrtc/androidt... talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java:235: if (buffer.position() % width < width/2) Add a comment here to about the memory layout please. https://codereview.webrtc.org/1460703002/diff/300001/talk/app/webrtc/java/and... File talk/app/webrtc/java/android/org/webrtc/EglBase.java (right): https://codereview.webrtc.org/1460703002/diff/300001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/EglBase.java:260: public void withPbufferSurface(int width, int height) { I would prefer if put this this helper method where you use it. If you add something here you must also do the same in EglBase14 and it seems unnecessary. If this is a helper where you use it I would call it createSurfaceOfSize or something similar.
https://codereview.webrtc.org/1460703002/diff/300001/talk/app/webrtc/androidt... File talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java (right): https://codereview.webrtc.org/1460703002/diff/300001/talk/app/webrtc/androidt... talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java:170: * {@code threshold}. Maybe belongs in MoreAsserts.java. */ On 2015/12/10 10:15:52, perkj1 wrote: > Remove comment Maybe belongs.... Done. https://codereview.webrtc.org/1460703002/diff/300001/talk/app/webrtc/androidt... talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java:177: static public void assertEqualsButOne(int expected, int actual) { On 2015/12/10 10:15:52, perkj1 wrote: > This looks like assertClose but with tresh hold 1. Remove method? assertEqualsButOne is a left-over. Deleted. https://codereview.webrtc.org/1460703002/diff/300001/talk/app/webrtc/androidt... talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java:184: public static void testTexturetoYUV() throws InterruptedException { On 2015/12/10 10:15:53, perkj1 wrote: > Move to the end of this file. Done. https://codereview.webrtc.org/1460703002/diff/300001/talk/app/webrtc/androidt... talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java:235: if (buffer.position() % width < width/2) On 2015/12/10 10:15:53, perkj1 wrote: > Add a comment here to about the memory layout please. Done. https://codereview.webrtc.org/1460703002/diff/300001/talk/app/webrtc/java/and... File talk/app/webrtc/java/android/org/webrtc/EglBase.java (right): https://codereview.webrtc.org/1460703002/diff/300001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/EglBase.java:260: public void withPbufferSurface(int width, int height) { On 2015/12/10 10:15:53, perkj1 wrote: > I would prefer if put this this helper method where you use it. If you add > something here you must also do the same in EglBase14 and it seems unnecessary. > > If this is a helper where you use it I would call it createSurfaceOfSize or > something similar. I'll delete this method and inline the logic where it is used. It's only one place (used to be two, with textureToRGBA).
lgtm with nits https://codereview.webrtc.org/1460703002/diff/310001/talk/app/webrtc/androidt... File talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java (right): https://codereview.webrtc.org/1460703002/diff/310001/talk/app/webrtc/androidt... talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java:406: // data the left and 8 bytes of V data on the right. s/ to https://codereview.webrtc.org/1460703002/diff/310001/talk/app/webrtc/java/and... File talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java (right): https://codereview.webrtc.org/1460703002/diff/310001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:248: // Reuse surface, if possible. s Create new pBuffferSurface with the correct size if needed.
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from perkj@webrtc.org Link to the patchset: https://codereview.webrtc.org/1460703002/#ps330001 (title: "Fix comment nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1460703002/330001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1460703002/330001
Message was sent while issue was closed.
Description was changed from ========== Implement NativeToI420Buffer in C++, calling java SurfaceTextureHelper, new method .textureToYUV, to do the conversion using an opengl fragment shader. BUG=webrtc:4993 ========== to ========== Implement NativeToI420Buffer in C++, calling java SurfaceTextureHelper, new method .textureToYUV, to do the conversion using an opengl fragment shader. BUG=webrtc:4993 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:330001)
Message was sent while issue was closed.
Description was changed from ========== Implement NativeToI420Buffer in C++, calling java SurfaceTextureHelper, new method .textureToYUV, to do the conversion using an opengl fragment shader. BUG=webrtc:4993 ========== to ========== Implement NativeToI420Buffer in C++, calling java SurfaceTextureHelper, new method .textureToYUV, to do the conversion using an opengl fragment shader. BUG=webrtc:4993 Committed: https://crrev.com/c490e01bd1bd4a0d754ed5f746b95ac03136346f Cr-Commit-Position: refs/heads/master@{#10972} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/c490e01bd1bd4a0d754ed5f746b95ac03136346f Cr-Commit-Position: refs/heads/master@{#10972} |
