|
|
Created:
5 years, 2 months ago by perkj_webrtc Modified:
5 years, 1 month ago CC:
webrtc-reviews_webrtc.org, yujie_mao (webrtc), Andrew MacDonald, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionThis cl add support to encode from textures to MediaCodecVideoEncoder.
This has also partly been reviewed in https://codereview.webrtc.org/1375953002/.
BUG=webrtc:4993
TBR=glaznew@webrtc.org
Committed: https://crrev.com/30e918278c8e0221ebbb24727fca90676da77220
Cr-Commit-Position: refs/heads/master@{#10725}
Patch Set 1 #Patch Set 2 : #
Total comments: 14
Patch Set 3 : Addressed Alex comments. #
Total comments: 1
Patch Set 4 : Fixed tests #
Total comments: 26
Patch Set 5 : Addressed magjeds comments #
Total comments: 3
Patch Set 6 : Worked on test. #Patch Set 7 : REbased #Patch Set 8 : Fixed factory. Deprecated setVideoHwAccelerationOptions #
Messages
Total messages: 26 (8 generated)
perkj@webrtc.org changed reviewers: + magjed@webrtc.org
please?
perkj@webrtc.org changed reviewers: + glaznev@webrtc.org
Alex, here are the encoder changes I would like to land.
https://codereview.webrtc.org/1403713002/diff/20001/talk/app/webrtc/java/andr... File talk/app/webrtc/java/android/org/webrtc/EglBase.java (right): https://codereview.webrtc.org/1403713002/diff/20001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/EglBase.java:215: // See https://android.googlesource.com/platform/frameworks/native/+/tools_r22.2/ope... nit: alignment? https://codereview.webrtc.org/1403713002/diff/20001/talk/app/webrtc/java/jni/... File talk/app/webrtc/java/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/1403713002/diff/20001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:435: use_surface_) != WEBRTC_VIDEO_CODEC_OK) { false instead of use_surface_ so it will be similar to InitEncodeOnCodecThread call? https://codereview.webrtc.org/1403713002/diff/20001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:605: input_frame); nit: input_frame can fit previous line? https://codereview.webrtc.org/1403713002/diff/20001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:700: NativeHandleImpl* handle = How do you handle slow encoder cases here if encoding time is too high? Byte buffer encoding will start to drop input frames. What texture encoding will do? https://codereview.webrtc.org/1403713002/diff/20001/talk/app/webrtc/java/jni/... File talk/app/webrtc/java/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/1403713002/diff/20001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/peerconnection_jni.cc:1275: if (encoder_factory) { May be add a separate egl_context parameters to configure texture decoding and texture encoding separately (which should be the same if both encode and decode to texture is enabled)? https://codereview.webrtc.org/1403713002/diff/20001/talk/app/webrtc/java/src/... File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoEncoder.java (right): https://codereview.webrtc.org/1403713002/diff/20001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoEncoder.java:212: public static boolean isVp8HwSupportedUsingTextures() { Are these 2 functions called from native code or from any app code or you are planning to use them in the future? https://codereview.webrtc.org/1403713002/diff/20001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoEncoder.java:362: drawer.release(); nit: drawer = null and same for 2 checks below?
Patchset #4 (id:60001) has been deleted
PTAL https://codereview.webrtc.org/1403713002/diff/20001/talk/app/webrtc/java/andr... File talk/app/webrtc/java/android/org/webrtc/EglBase.java (right): https://codereview.webrtc.org/1403713002/diff/20001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/EglBase.java:215: // See https://android.googlesource.com/platform/frameworks/native/+/tools_r22.2/ope... On 2015/10/14 23:48:27, AlexG wrote: > nit: alignment? I remove this for now since I have not find a reason to call EGLExt.eglPresentationTimeANDROID. I asked the hangouts folks about when EGLExt.eglPresentationTimeANDROID is needed. (it requires EGL14 so I would like to do it in a follow up cl if it is required. We then must use EGL14 on some SDK versions and EGL10 on others) https://codereview.webrtc.org/1403713002/diff/20001/talk/app/webrtc/java/jni/... File talk/app/webrtc/java/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/1403713002/diff/20001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:435: use_surface_) != WEBRTC_VIDEO_CODEC_OK) { On 2015/10/14 23:48:27, AlexG wrote: > false instead of use_surface_ so it will be similar to InitEncodeOnCodecThread > call? Done. https://codereview.webrtc.org/1403713002/diff/20001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:605: input_frame); On 2015/10/14 23:48:27, AlexG wrote: > nit: input_frame can fit previous line? Done. https://codereview.webrtc.org/1403713002/diff/20001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:700: NativeHandleImpl* handle = On 2015/10/14 23:48:27, AlexG wrote: > How do you handle slow encoder cases here if encoding time is too high? Byte > buffer encoding will start to drop input frames. What texture encoding will do? The same. Dropping happens before this function is called in EncodeOnCodecThread https://codereview.webrtc.org/1403713002/diff/20001/talk/app/webrtc/java/jni/... File talk/app/webrtc/java/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/1403713002/diff/20001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/peerconnection_jni.cc:1275: if (encoder_factory) { On 2015/10/14 23:48:27, AlexG wrote: > May be add a separate egl_context parameters to configure texture decoding and > texture encoding separately (which should be the same if both encode and decode > to texture is enabled)? Done. Added a new method PeerConnectionFactory_nativeSetVideoHwAccelerationOptions2 (2) since the jni compiler does not like to functions with the same name. https://codereview.webrtc.org/1403713002/diff/20001/talk/app/webrtc/java/src/... File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoEncoder.java (right): https://codereview.webrtc.org/1403713002/diff/20001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoEncoder.java:212: public static boolean isVp8HwSupportedUsingTextures() { On 2015/10/14 23:48:27, AlexG wrote: > Are these 2 functions called from native code or from any app code or you are > planning to use them in the future? they are here so that an app can ask if the hw codec can use with textures. If we don't add them - is there a risk that a hw encoder does not support encoding from textures? I guess not and in that case we can remove them and rely on isH264HwSupported and the vp8 version. Wdyt? https://codereview.webrtc.org/1403713002/diff/20001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoEncoder.java:362: drawer.release(); On 2015/10/14 23:48:27, AlexG wrote: > nit: drawer = null and same for 2 checks below? Done. https://codereview.webrtc.org/1403713002/diff/40001/talk/app/webrtc/java/andr... File talk/app/webrtc/java/android/org/webrtc/EglBase.java (right): https://codereview.webrtc.org/1403713002/diff/40001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/EglBase.java:96: * We have to wrap Surface in a SurfaceHolder because for some reason eglCreateWindowSurface This is landed separately by magjed
And please take a look at this one if you have time.
https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/androidte... File talk/app/webrtc/androidtests/src/org/webrtc/MediaCodecVideoEncoderTest.java (right): https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/androidte... talk/app/webrtc/androidtests/src/org/webrtc/MediaCodecVideoEncoderTest.java:36: import android.util.Log; The import order is wrong. "Import statements are divided into the following groups, in this order, with each group separated by a single blank line: All static imports in a single group com.google imports (only if this source file is in the com.google package space) Third-party imports, one group per top-level package, in ASCII sort order for example: android, com, junit, org, sun java imports javax imports" https://google.github.io/styleguide/javaguide.html#s3.3.3-import-ordering-and... https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/androidte... talk/app/webrtc/androidtests/src/org/webrtc/MediaCodecVideoEncoderTest.java:45: public static void testReInitializeUsingByteBuffer() { Why did you modify this test in this CL? And why did you remove the previous simpler test with just init() and release()? https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/androidte... talk/app/webrtc/androidtests/src/org/webrtc/MediaCodecVideoEncoderTest.java:131: final EglBase eglOesBase = new EglBase(EGL10.EGL_NO_CONTEXT, EglBase.ConfigType.PLAIN); You must make |eglOesBase| current, otherwise it has no purpose. Replace with this code: Final EglBase eglOesBase = new EglBase(sharedContext, EglBase.ConfigType.PIXEL_BUFFER); eglOesBase.createDummyPbufferSurface(); eglOesBase.makeCurrent(); https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/andr... File talk/app/webrtc/java/android/org/webrtc/EglBase.java (right): https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/EglBase.java:32: import android.graphics.Rect; Revert these changes. https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/EglBase.java:96: * We have to wrap Surface in a SurfaceHolder because for some reason eglCreateWindowSurface Rebase, I have landed this change already in a separate CL. https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/andr... File talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java (right): https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:177: eglBase.createSurface(getHolder().getSurface()); Rebase against master. https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/jni/... File talk/app/webrtc/java/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:367: false /*use_surface*/)); /* use_surface */ https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:499: use_surface_ = true; This looks strange, can you only enable surface encode, but not disable? Why not 'use_surface_ = use_surface;' outside if-statement? https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:647: bool reconfigure_due_to_size = Why did you make |reconfigure_due_to_size| mutable :P https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/jni/... File talk/app/webrtc/java/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/peerconnection_jni.cc:1309: JOW(void, PeerConnectionFactory_nativeSetVideoHwAccelerationOptions2)( Why do we need two versions of nativeSetVideoHwAccelerationOptions? https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/src/... File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoEncoder.java (right): https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoEncoder.java:192: for (int codecColorFormat : capabilities.colorFormats) { This looks like duplicated code. I suggest replacing the |useSurface| argument with a |supportedColorList| argument instead. Rename static |supportedColorList| to |supportedYuvColorList| and add private static final int[] supportedSurfaceColorList = { CodecCapabilities.COLOR_FormatSurface } Then in isVp8HwSupported(), pass |supportedYuvColorList|, and in isVp8HwSupportedUsingTextures(), pass |supportedSurfaceColorList|. https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoEncoder.java:269: (useSurface ? "True" : "False")); Just print useSurface, you don't need to convert it to a string manually.
PTAL https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/androidte... File talk/app/webrtc/androidtests/src/org/webrtc/MediaCodecVideoEncoderTest.java (right): https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/androidte... talk/app/webrtc/androidtests/src/org/webrtc/MediaCodecVideoEncoderTest.java:36: import android.util.Log; On 2015/11/18 13:12:52, magjed_webrtc wrote: > The import order is wrong. > "Import statements are divided into the following groups, in this order, with > each group separated by a single blank line: > > All static imports in a single group > com.google imports (only if this source file is in the com.google package space) > Third-party imports, one group per top-level package, in ASCII sort order > for example: android, com, junit, org, sun > java imports > javax imports" > https://google.github.io/styleguide/javaguide.html#s3.3.3-import-ordering-and... We strictly don't do https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/androidte... talk/app/webrtc/androidtests/src/org/webrtc/MediaCodecVideoEncoderTest.java:45: public static void testReInitializeUsingByteBuffer() { On 2015/11/18 13:12:52, magjed_webrtc wrote: > Why did you modify this test in this CL? And why did you remove the previous > simpler test with just init() and release()? this is how we use it from c++ sometimes. It felt unnecessary to test both init release and init release init release... https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/androidte... talk/app/webrtc/androidtests/src/org/webrtc/MediaCodecVideoEncoderTest.java:131: final EglBase eglOesBase = new EglBase(EGL10.EGL_NO_CONTEXT, EglBase.ConfigType.PLAIN); On 2015/11/18 13:12:52, magjed_webrtc wrote: > You must make |eglOesBase| current, otherwise it has no purpose. Replace with > this code: > Final EglBase eglOesBase = > new EglBase(sharedContext, EglBase.ConfigType.PIXEL_BUFFER); > eglOesBase.createDummyPbufferSurface(); > eglOesBase.makeCurrent(); not sure I understand. https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/andr... File talk/app/webrtc/java/android/org/webrtc/EglBase.java (right): https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/EglBase.java:96: * We have to wrap Surface in a SurfaceHolder because for some reason eglCreateWindowSurface On 2015/11/18 13:12:52, magjed_webrtc wrote: > Rebase, I have landed this change already in a separate CL. I will rebase and revert all this once the decoder patch has landed. https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/andr... File talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java (right): https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:177: eglBase.createSurface(getHolder().getSurface()); On 2015/11/18 13:12:52, magjed_webrtc wrote: > Rebase against master. dito- this will go away once the decoder patch has landed. https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/jni/... File talk/app/webrtc/java/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:367: false /*use_surface*/)); On 2015/11/18 13:12:52, magjed_webrtc wrote: > /* use_surface */ Done. https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:499: use_surface_ = true; On 2015/11/18 13:12:52, magjed_webrtc wrote: > This looks strange, can you only enable surface encode, but not disable? Why not > 'use_surface_ = use_surface;' outside if-statement? that is strange - but works. I reset in Reset... Anyway, fixed. https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:647: bool reconfigure_due_to_size = On 2015/11/18 13:12:52, magjed_webrtc wrote: > Why did you make |reconfigure_due_to_size| mutable :P .... https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/jni/... File talk/app/webrtc/java/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/peerconnection_jni.cc:1309: JOW(void, PeerConnectionFactory_nativeSetVideoHwAccelerationOptions2)( On 2015/11/18 13:12:52, magjed_webrtc wrote: > Why do we need two versions of nativeSetVideoHwAccelerationOptions? This is on request by glaznew. We can then have separate context for decoder -renderer / capturer - encoder - local renderer. It does not compile if I name this PeerConnectionFactory_nativeSetVideoHwAccelerationOptions as well. And we can not remove the other until all clients use the new version. https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/src/... File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoEncoder.java (right): https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoEncoder.java:192: for (int codecColorFormat : capabilities.colorFormats) { On 2015/11/18 13:12:52, magjed_webrtc wrote: > This looks like duplicated code. I suggest replacing the |useSurface| argument > with a |supportedColorList| argument instead. Rename static |supportedColorList| > to |supportedYuvColorList| and add > private static final int[] supportedSurfaceColorList = { > CodecCapabilities.COLOR_FormatSurface > } > Then in isVp8HwSupported(), pass |supportedYuvColorList|, and in > isVp8HwSupportedUsingTextures(), pass |supportedSurfaceColorList|. nice. yes https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoEncoder.java:269: (useSurface ? "True" : "False")); On 2015/11/18 13:12:52, magjed_webrtc wrote: > Just print useSurface, you don't need to convert it to a string manually. Done.
https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/androidte... File talk/app/webrtc/androidtests/src/org/webrtc/MediaCodecVideoEncoderTest.java (right): https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/androidte... talk/app/webrtc/androidtests/src/org/webrtc/MediaCodecVideoEncoderTest.java:45: public static void testReInitializeUsingByteBuffer() { On 2015/11/18 14:51:20, perkj1 wrote: > On 2015/11/18 13:12:52, magjed_webrtc wrote: > > Why did you modify this test in this CL? And why did you remove the previous > > simpler test with just init() and release()? > > this is how we use it from c++ sometimes. It felt unnecessary to test both init > release and init release init release... Yeah maybe. I don't know much about our testing 'style guide', but it's nice to have small tests that only test one behaviour. You can probably ask phoglund@ what the preferred way is - overlapping small tests or a few big tests. https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/androidte... talk/app/webrtc/androidtests/src/org/webrtc/MediaCodecVideoEncoderTest.java:131: final EglBase eglOesBase = new EglBase(EGL10.EGL_NO_CONTEXT, EglBase.ConfigType.PLAIN); On 2015/11/18 14:51:20, perkj1 wrote: > On 2015/11/18 13:12:52, magjed_webrtc wrote: > > You must make |eglOesBase| current, otherwise it has no purpose. Replace with > > this code: > > Final EglBase eglOesBase = > > new EglBase(sharedContext, EglBase.ConfigType.PIXEL_BUFFER); > > eglOesBase.createDummyPbufferSurface(); > > eglOesBase.makeCurrent(); > > not sure I understand. First, you need a current EGLContext for all GLES calls. I'm not sure if another EGLContext is current here, or if you are relying on undefined behaviour and generating a texture outside an EGLContext. Also, if you don't make |eglOesBase| current, you don't create |oesTextureId| in the |eglOesBase| namespace. Then the encoder can't access it in encoder.encodeTexture(), because it is not a valid texture id in the encode EGLContext namespace. I think this test is currently sampling from an invalid texture. https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/jni/... File talk/app/webrtc/java/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/1403713002/diff/80001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/peerconnection_jni.cc:1309: JOW(void, PeerConnectionFactory_nativeSetVideoHwAccelerationOptions2)( On 2015/11/18 14:51:21, perkj1 wrote: > On 2015/11/18 13:12:52, magjed_webrtc wrote: > > Why do we need two versions of nativeSetVideoHwAccelerationOptions? > > > This is on request by glaznew. We can then have separate context for decoder > -renderer / capturer - encoder - local renderer. > > It does not compile if I name this > PeerConnectionFactory_nativeSetVideoHwAccelerationOptions as well. > > And we can not remove the other until all clients use the new version. The purpose with separate contexts is still unclear to me. Is the purpose to be able to enable/disable texture support separately for encode/decode? Why don't you call encoder_factory->SetEGLContext() in the previous nativeSetVideoHwAccelerationOptions() version? https://codereview.webrtc.org/1403713002/diff/100001/talk/app/webrtc/androidt... File talk/app/webrtc/androidtests/src/org/webrtc/MediaCodecVideoEncoderTest.java (right): https://codereview.webrtc.org/1403713002/diff/100001/talk/app/webrtc/androidt... talk/app/webrtc/androidtests/src/org/webrtc/MediaCodecVideoEncoderTest.java:135: int oesTextureId = GlUtil.generateTexture(GLES11Ext.GL_TEXTURE_EXTERNAL_OES); This texture still does not have any data associated with it, and the width and height is undefined. For any ordinary texture, you could just fill it with a glTexImage2D() call, but GL_TEXTURE_EXTERNAL_OES is different and not easy to generate. You can look at how it's done in GlRectDrawerTest.testOesRendering(). I'm not sure how much you want to do in this test, but you are probably sampling and encoding from an undefined texture currently.
https://codereview.webrtc.org/1403713002/diff/100001/talk/app/webrtc/androidt... File talk/app/webrtc/androidtests/src/org/webrtc/MediaCodecVideoEncoderTest.java (right): https://codereview.webrtc.org/1403713002/diff/100001/talk/app/webrtc/androidt... talk/app/webrtc/androidtests/src/org/webrtc/MediaCodecVideoEncoderTest.java:142: assertTrue(encoder.encodeTexture(true, oesTextureId, RendererCommon.identityMatrix(), Another thing, can you call GlUtil.checkNoGLES2Error() here? Hopefully you can catch if you are sending an undefined texture id as input.
https://codereview.webrtc.org/1403713002/diff/100001/talk/app/webrtc/androidt... File talk/app/webrtc/androidtests/src/org/webrtc/MediaCodecVideoEncoderTest.java (right): https://codereview.webrtc.org/1403713002/diff/100001/talk/app/webrtc/androidt... talk/app/webrtc/androidtests/src/org/webrtc/MediaCodecVideoEncoderTest.java:142: assertTrue(encoder.encodeTexture(true, oesTextureId, RendererCommon.identityMatrix(), On 2015/11/18 15:47:31, magjed_webrtc wrote: > Another thing, can you call GlUtil.checkNoGLES2Error() here? Hopefully you can > catch if you are sending an undefined texture id as input. Done.
Patchset #7 (id:140001) has been deleted
PTAL
lgtm
Thanks - Alex - do you want to take a look?
Description was changed from ========== This cl add support to encode from textures to MediaCodecVideoEncoder. This has also partly been reviewed in https://codereview.webrtc.org/1375953002/. BUG=webrtc:4993 ========== to ========== This cl add support to encode from textures to MediaCodecVideoEncoder. This has also partly been reviewed in https://codereview.webrtc.org/1375953002/. BUG=webrtc:4993 TBR=glaznew@webrtc.org ==========
The CQ bit was checked by perkj@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1403713002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1403713002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by perkj@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1403713002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1403713002/180001
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/30e918278c8e0221ebbb24727fca90676da77220 Cr-Commit-Position: refs/heads/master@{#10725} |