|
|
DescriptionAdd texture support to HardwareVideoEncoder.
HardwareVideoEncoderFactory can now take an EglBase.Context on creation.
When it does, it creates video encoders in texture mode. It uses the
COLOR_FormatSurface colorFormat. It passes the EglBase.Context to the
HardwareVideoEncoder.
The HardwareVideoEncoder sets up an input surface for its codec and handles
incoming frames by drawing them onto the input surface.
BUG=webrtc:7760
R=pthatcher@webrtc.org, sakal@webrtc.org
Review-Url: https://codereview.webrtc.org/2977153003 .
Cr-Commit-Position: refs/heads/master@{#19083}
Committed: https://chromium.googlesource.com/external/webrtc/+/0cf9a4a482b18eefa8caeb7f442ac4fabcdefdeb
Patch Set 1 #
Total comments: 28
Patch Set 2 : Refactoring and error handling #Patch Set 3 : Names and comments #
Total comments: 4
Patch Set 4 : Fix logging and matrix helper #Patch Set 5 : Fix logging and matrix helper #
Messages
Total messages: 17 (6 generated)
Description was changed from ========== Add texture support to HardwareVideoEncoder. HardwareVideoEncoderFactory can now take an EglBase.Context on creation. When it does, it creates video encoders in texture mode. It uses the COLOR_FormatSurface colorFormat. It passes the EglBase.Context to the HardwareVideoEncoder. The HardwareVideoEncoder sets up an input surface for its codec and handles incoming frames by drawing them onto the input surface. BUG=webrtc:7760 ========== to ========== Add texture support to HardwareVideoEncoder. HardwareVideoEncoderFactory can now take an EglBase.Context on creation. When it does, it creates video encoders in texture mode. It uses the COLOR_FormatSurface colorFormat. It passes the EglBase.Context to the HardwareVideoEncoder. The HardwareVideoEncoder sets up an input surface for its codec and handles incoming frames by drawing them onto the input surface. BUG=webrtc:7760 ==========
mellem@webrtc.org changed reviewers: + aburago@webrtc.org, pthatcher@webrtc.org, sakal@webrtc.org
https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/api/org/we... File webrtc/sdk/android/api/org/webrtc/HardwareVideoEncoderFactory.java (right): https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/HardwareVideoEncoderFactory.java:65: if (sharedContext instanceof EglBase14.Context) { I would prefer this just taking in EglBase14.Context. https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... File webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java (right): https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java:271: if (videoFrameBuffer instanceof VideoFrame.TextureBuffer) { nit: I would prefer these cases in separate methods. https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java:274: // TODO(mellem): Put this matrix manipulation in a helper. I have implemented such helper here https://chromium-review.googlesource.com/c/555516/4/webrtc/sdk/android/api/or... feel free to copy https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java:305: drawer.drawOes( nit: TextureFrame might be RGB frame in the future. Can we call drawRgb if getType is RGB? https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java:316: // No timeout. Don't block for an input buffer, drop frames if the encoder falls behind. Can we at least log a clear error if the mode doesn't match what kind of frames we receive? How hard would it be to reinitialize the encoder? https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java:322: return VideoCodecStatus.FALLBACK_SOFTWARE; Can you just replace all FALLBACK_SOTWAREs with ERRORs and we will deal with the fallback in C++. https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java:338: VideoFrame.I420Buffer i420 = videoFrameBuffer.toI420(); toI420 will return "a new instance". Therefore, we should release the i420 buffer.
https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/api/org/we... File webrtc/sdk/android/api/org/webrtc/HardwareVideoEncoderFactory.java (right): https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/HardwareVideoEncoderFactory.java:65: if (sharedContext instanceof EglBase14.Context) { On 2017/07/17 12:25:41, sakal wrote: > I would prefer this just taking in EglBase14.Context. EglBase14 is not part of the api (it's a package-private class in the src/java/... directory) and callers outside webrtc can't instantiate it. I need to take an argument that callers can actually create, or I can't reuse hardware encoding in texture mode outside webrtc. https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... File webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java (right): https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java:271: if (videoFrameBuffer instanceof VideoFrame.TextureBuffer) { On 2017/07/17 12:25:41, sakal wrote: > nit: I would prefer these cases in separate methods. Done. https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java:274: // TODO(mellem): Put this matrix manipulation in a helper. On 2017/07/17 12:25:41, sakal wrote: > I have implemented such helper here > https://chromium-review.googlesource.com/c/555516/4/webrtc/sdk/android/api/or... > > feel free to copy Done. Thanks for writing that. https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java:305: drawer.drawOes( On 2017/07/17 12:25:42, sakal wrote: > nit: TextureFrame might be RGB frame in the future. Can we call drawRgb if > getType is RGB? Done. https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java:316: // No timeout. Don't block for an input buffer, drop frames if the encoder falls behind. On 2017/07/17 12:25:42, sakal wrote: > Can we at least log a clear error if the mode doesn't match what kind of frames > we receive? How hard would it be to reinitialize the encoder? Logging added. We can actually process a texture buffer in non-texture mode (texture buffers still have toI420), just not the other way around. Or maybe we can draw the byte-buffer onto a surface right here? As it stands, it would be quite hard to re-initialize into a different mode. I would have to refactor code so that we can find an appropriate color format during init. Byte to texture would not work anyway if we're missing the EGL context. Texture to byte might not work if the codec supports texture-mode but none of our byte-buffer color formats. I'd rather not attempt it, since existing code CHECK-fails if it gets handed a frame in the wrong mode: https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/android/src/jni/a... https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/android/src/jni/a... https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java:322: return VideoCodecStatus.FALLBACK_SOFTWARE; On 2017/07/17 12:25:42, sakal wrote: > Can you just replace all FALLBACK_SOTWAREs with ERRORs and we will deal with the > fallback in C++. Done. https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java:338: VideoFrame.I420Buffer i420 = videoFrameBuffer.toI420(); On 2017/07/17 12:25:41, sakal wrote: > toI420 will return "a new instance". Therefore, we should release the i420 > buffer. Done.
https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... File webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java (right): https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java:79: // Surface objects for texture-mode encoding. Would it make sense to write a comment explaining what they are and how they interact? https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java:83: private GlRectDrawer drawer; It would be nice if these had a common name that made it clear it was for texture encoding. Perhaps something like textureContext, textureBase, textureSurface, textureDrawer? https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java:117: this.inputColorFormat = null; Can you write a comment explaining why the inputColorFormat is null when the texture shared context is non-null? https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java:164: // Surface mode. Above it was called "texture mode", not it says "surface mode". That's the same thing, I'm assuming. If so, can we pick one? https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java:190: } Is this only true when we're in texture mode? If we're not an there's so outputThread, wouldn't that be an error? https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java:218: } Would it make sense to put the 4 texture things in one "TextureModeState" class? Then it could have a release() and you'd only need one of these checks.
https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... File webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java (right): https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java:79: // Surface objects for texture-mode encoding. On 2017/07/17 22:50:19, pthatcher1 wrote: > Would it make sense to write a comment explaining what they are and how they > interact? Done. https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java:83: private GlRectDrawer drawer; On 2017/07/17 22:50:19, pthatcher1 wrote: > It would be nice if these had a common name that made it clear it was for > texture encoding. Perhaps something like textureContext, textureBase, > textureSurface, textureDrawer? Done. https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java:117: this.inputColorFormat = null; On 2017/07/17 22:50:19, pthatcher1 wrote: > Can you write a comment explaining why the inputColorFormat is null when the > texture shared context is non-null? Done. https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java:164: // Surface mode. On 2017/07/17 22:50:19, pthatcher1 wrote: > Above it was called "texture mode", not it says "surface mode". That's the same > thing, I'm assuming. If so, can we pick one? Done. https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java:190: } On 2017/07/17 22:50:19, pthatcher1 wrote: > Is this only true when we're in texture mode? If we're not an there's so > outputThread, wouldn't that be an error? No, this is just a fix for a bug that I whacked. I noticed it because I had test errors while writing texture support. If init fails, the output thread may still be null when release() is called. In that case, we can't join the output thread. https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java:218: } On 2017/07/17 22:50:19, pthatcher1 wrote: > Would it make sense to put the 4 texture things in one "TextureModeState" class? > Then it could have a release() and you'd only need one of these checks. It's possible to fail init partway through allocating these (eg. if createInputSurface throws). We need to release whatever we created. So I think each needs a separate null check.
lgtm
lgtm https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... File webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java (right): https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java:316: // No timeout. Don't block for an input buffer, drop frames if the encoder falls behind. On 2017/07/17 17:49:30, mellem wrote: > On 2017/07/17 12:25:42, sakal wrote: > > Can we at least log a clear error if the mode doesn't match what kind of > frames > > we receive? How hard would it be to reinitialize the encoder? > > Logging added. We can actually process a texture buffer in non-texture mode > (texture buffers still have toI420), just not the other way around. Or maybe we > can draw the byte-buffer onto a surface right here? > > As it stands, it would be quite hard to re-initialize into a different mode. I > would have to refactor code so that we can find an appropriate color format > during init. Byte to texture would not work anyway if we're missing the EGL > context. Texture to byte might not work if the codec supports texture-mode but > none of our byte-buffer color formats. > > I'd rather not attempt it, since existing code CHECK-fails if it gets handed a > frame in the wrong mode: > > https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/android/src/jni/a... > > https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/android/src/jni/a... We reconfigure the encoder before we hit that check: https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/android/src/jni/a... But we can implement this in a follow-up if needed. https://codereview.webrtc.org/2977153003/diff/40001/webrtc/sdk/android/api/or... File webrtc/sdk/android/api/org/webrtc/HardwareVideoEncoderFactory.java (right): https://codereview.webrtc.org/2977153003/diff/40001/webrtc/sdk/android/api/or... webrtc/sdk/android/api/org/webrtc/HardwareVideoEncoderFactory.java:68: this.sharedContext = null; I think we should log a warning at this point. https://codereview.webrtc.org/2977153003/diff/40001/webrtc/sdk/android/instru... File webrtc/sdk/android/instrumentationtests/src/org/webrtc/HardwareVideoEncoderTest.java (right): https://codereview.webrtc.org/2977153003/diff/40001/webrtc/sdk/android/instru... webrtc/sdk/android/instrumentationtests/src/org/webrtc/HardwareVideoEncoderTest.java:124: Log.w(TAG, "No hardware encoding support, skipping testEncodeYuvBuffer"); testEncodeYuvBuffer -> testEncodeTextures
https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... File webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java (right): https://codereview.webrtc.org/2977153003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/HardwareVideoEncoder.java:316: // No timeout. Don't block for an input buffer, drop frames if the encoder falls behind. On 2017/07/18 08:46:13, sakal wrote: > On 2017/07/17 17:49:30, mellem wrote: > > On 2017/07/17 12:25:42, sakal wrote: > > > Can we at least log a clear error if the mode doesn't match what kind of > > frames > > > we receive? How hard would it be to reinitialize the encoder? > > > > Logging added. We can actually process a texture buffer in non-texture mode > > (texture buffers still have toI420), just not the other way around. Or maybe > we > > can draw the byte-buffer onto a surface right here? > > > > As it stands, it would be quite hard to re-initialize into a different mode. > I > > would have to refactor code so that we can find an appropriate color format > > during init. Byte to texture would not work anyway if we're missing the EGL > > context. Texture to byte might not work if the codec supports texture-mode > but > > none of our byte-buffer color formats. > > > > I'd rather not attempt it, since existing code CHECK-fails if it gets handed a > > frame in the wrong mode: > > > > > https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/android/src/jni/a... > > > > > https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/android/src/jni/a... > > We reconfigure the encoder before we hit that check: > https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/android/src/jni/a... > > But we can implement this in a follow-up if needed. Acknowledged. https://codereview.webrtc.org/2977153003/diff/40001/webrtc/sdk/android/api/or... File webrtc/sdk/android/api/org/webrtc/HardwareVideoEncoderFactory.java (right): https://codereview.webrtc.org/2977153003/diff/40001/webrtc/sdk/android/api/or... webrtc/sdk/android/api/org/webrtc/HardwareVideoEncoderFactory.java:68: this.sharedContext = null; On 2017/07/18 08:46:13, sakal wrote: > I think we should log a warning at this point. Done. https://codereview.webrtc.org/2977153003/diff/40001/webrtc/sdk/android/instru... File webrtc/sdk/android/instrumentationtests/src/org/webrtc/HardwareVideoEncoderTest.java (right): https://codereview.webrtc.org/2977153003/diff/40001/webrtc/sdk/android/instru... webrtc/sdk/android/instrumentationtests/src/org/webrtc/HardwareVideoEncoderTest.java:124: Log.w(TAG, "No hardware encoding support, skipping testEncodeYuvBuffer"); On 2017/07/18 08:46:13, sakal wrote: > testEncodeYuvBuffer -> testEncodeTextures Done.
The CQ bit was checked by mellem@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org, sakal@webrtc.org Link to the patchset: https://codereview.webrtc.org/2977153003/#ps80001 (title: "Fix logging and matrix helper")
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
Try jobs failed on following builders: linux_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...)
Description was changed from ========== Add texture support to HardwareVideoEncoder. HardwareVideoEncoderFactory can now take an EglBase.Context on creation. When it does, it creates video encoders in texture mode. It uses the COLOR_FormatSurface colorFormat. It passes the EglBase.Context to the HardwareVideoEncoder. The HardwareVideoEncoder sets up an input surface for its codec and handles incoming frames by drawing them onto the input surface. BUG=webrtc:7760 ========== to ========== Add texture support to HardwareVideoEncoder. HardwareVideoEncoderFactory can now take an EglBase.Context on creation. When it does, it creates video encoders in texture mode. It uses the COLOR_FormatSurface colorFormat. It passes the EglBase.Context to the HardwareVideoEncoder. The HardwareVideoEncoder sets up an input surface for its codec and handles incoming frames by drawing them onto the input surface. BUG=webrtc:7760 R=pthatcher@webrtc.org, sakal@webrtc.org Review-Url: https://codereview.webrtc.org/2977153003 . Cr-Commit-Position: refs/heads/master@{#19083} Committed: https://chromium.googlesource.com/external/webrtc/+/0cf9a4a482b18eefa8caeb7f4... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as 0cf9a4a482b18eefa8caeb7f442ac4fabcdefdeb (presubmit successful). |