Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2835203003:
ObjC: Split out NV12 texture uploading into separate class (Closed)
|
Created:
3 years, 8 months ago by magjed_webrtc Modified:
3 years, 7 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionObjC: Split out NV12 texture uploading into separate class
This CL splits out the code in RTCNV12Shader.mm for uploading byte
buffers to textures into its own class RTCNV12TextureCache. The purpose
is to prepare for allowing clients to inject their own shaders.
RTCNV12TextureCache will be used in the generic code, while the actual
shaders in RTCNV12Shader will be customizable by the client.
BUG=webrtc:7473
Review-Url: https://codereview.webrtc.org/2835203003
Cr-Commit-Position: refs/heads/master@{#17902}
Committed: https://chromium.googlesource.com/external/webrtc/+/15f0c6815d089640230e625a4a9faa53b7dd7f62
Patch Set 1 #
Total comments: 12
Patch Set 2 : Re-upload with high similarity #
Total comments: 4
Patch Set 3 : Address comments. #
Total comments: 2
Patch Set 4 : CFRelease textureOut #
Messages
Total messages: 31 (21 generated)
The CQ bit was checked by magjed@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: mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/25086)
The CQ bit was checked by magjed@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 checked by magjed@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/...
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== ObjC: Split out NV12 texture uploading into separate class This CL splits out the code in RTCNV12Shader.mm for uploading byte buffers to textures into its own class RTCNV12TextureCache. The purpose is to prepare for allowing clients to inject their own shaders. RTCNV12TextureCache will be used in the generic code, while the actual shaders in RTCNV12Shader will be customizable by the client. BUG=webrtc:7473 ========== to ========== ObjC: Split out NV12 texture uploading into separate class This CL splits out the code in RTCNV12Shader.mm for uploading byte buffers to textures into its own class RTCNV12TextureCache. The purpose is to prepare for allowing clients to inject their own shaders. RTCNV12TextureCache will be used in the generic code, while the actual shaders in RTCNV12Shader will be customizable by the client. BUG=webrtc:7473 ==========
magjed@webrtc.org changed reviewers: + denicija@webrtc.org, sakal@webrtc.org
Please take a look.
https://codereview.webrtc.org/2835203003/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCNV12TextureCache.h (right): https://codereview.webrtc.org/2835203003/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCNV12TextureCache.h:11: #import "WebRTC/RTCVideoFrame.h" Can we forward declare this? https://codereview.webrtc.org/2835203003/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCNV12TextureCache.h:20: - (instancetype)initWithContext:(EAGLContext *)context; You could use Nullability audits (NS_ASSUME_NONNULL_BEGIN) and (NS_ASSUME_NONNULL_END) and then specify that this can return nullable instance like so - (nullable instancetype)initWithContext:(EAGLContext *)context https://codereview.webrtc.org/2835203003/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCNV12TextureCache.m (left): https://codereview.webrtc.org/2835203003/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCNV12TextureCache.m:104: RTC_CHECK(pixelFormat == kCVPixelFormatType_420YpCbCr8BiPlanarFullRange || We no longer want to check the pixel format? https://codereview.webrtc.org/2835203003/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCNV12TextureCache.m (right): https://codereview.webrtc.org/2835203003/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCNV12TextureCache.m:13: #import "RTCShader+Private.h" Is this needed? https://codereview.webrtc.org/2835203003/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCNV12TextureCache.m:17: CVOpenGLESTextureRef _yTexture; It's bit weird that the ivar and the property have different types. Perhaps instead of properties it would be better to expose the textures as methods?
https://codereview.webrtc.org/2835203003/diff/100001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCNV12TextureCache.m (right): https://codereview.webrtc.org/2835203003/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCNV12TextureCache.m:51: const int lumaWidth = CVPixelBufferGetWidthOfPlane(pixelBuffer, 0); nit: Can you make 0 and 1 here constants. https://codereview.webrtc.org/2835203003/diff/100001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCNativeNV12Shader.mm (left): https://codereview.webrtc.org/2835203003/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCNativeNV12Shader.mm:15: #if TARGET_OS_IPHONE This is no also supported on Mac? Is the idea to provide RTCVC12TextureCache implementation for Mac as well?
Patchset #3 (id:120001) has been deleted
https://codereview.webrtc.org/2835203003/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCNV12TextureCache.h (right): https://codereview.webrtc.org/2835203003/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCNV12TextureCache.h:11: #import "WebRTC/RTCVideoFrame.h" On 2017/04/26 08:56:23, daniela-webrtc wrote: > Can we forward declare this? Good idea! Done. https://codereview.webrtc.org/2835203003/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCNV12TextureCache.h:20: - (instancetype)initWithContext:(EAGLContext *)context; On 2017/04/26 08:56:23, daniela-webrtc wrote: > You could use Nullability audits (NS_ASSUME_NONNULL_BEGIN) and > (NS_ASSUME_NONNULL_END) and then specify that this can return nullable instance > like so > - (nullable instancetype)initWithContext:(EAGLContext *)context Done. https://codereview.webrtc.org/2835203003/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCNV12TextureCache.m (left): https://codereview.webrtc.org/2835203003/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCNV12TextureCache.m:104: RTC_CHECK(pixelFormat == kCVPixelFormatType_420YpCbCr8BiPlanarFullRange || On 2017/04/26 08:56:23, daniela-webrtc wrote: > We no longer want to check the pixel format? I did a straight-forward conversion to NSAssert (I don't want to make this an Objective C++ file just for RTC_CHECK), but then I got compile errors for "unused variable pixelFormat" on release builds... So I decided to just drop this check, it's not that important. https://codereview.webrtc.org/2835203003/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCNV12TextureCache.m (right): https://codereview.webrtc.org/2835203003/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCNV12TextureCache.m:13: #import "RTCShader+Private.h" On 2017/04/26 08:56:23, daniela-webrtc wrote: > Is this needed? It's needed for RTC_PIXEL_FORMAT, but since this file is iOS only, I can replace that with hardcoded GL_LUMINANCE instead and remove this include. I did that now. https://codereview.webrtc.org/2835203003/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCNV12TextureCache.m:17: CVOpenGLESTextureRef _yTexture; On 2017/04/26 08:56:23, daniela-webrtc wrote: > It's bit weird that the ivar and the property have different types. Perhaps > instead of properties it would be better to expose the textures as methods? I agree it's a bit weird, and I don't know the conventions in ObjC. I think it looks nicer for users of the class to be able to use the dot syntax, and it would be consistent with the I420TextureCache. I changed the name of the ivar slightly so it's not exactly the same as the property. Is that enough? https://codereview.webrtc.org/2835203003/diff/100001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCNV12TextureCache.m (right): https://codereview.webrtc.org/2835203003/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCNV12TextureCache.m:51: const int lumaWidth = CVPixelBufferGetWidthOfPlane(pixelBuffer, 0); On 2017/04/26 09:11:58, sakal wrote: > nit: Can you make 0 and 1 here constants. Done (it's a named argument to a function now). https://codereview.webrtc.org/2835203003/diff/100001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCNativeNV12Shader.mm (left): https://codereview.webrtc.org/2835203003/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCNativeNV12Shader.mm:15: #if TARGET_OS_IPHONE On 2017/04/26 09:11:58, sakal wrote: > This is no also supported on Mac? Is the idea to provide RTCVC12TextureCache > implementation for Mac as well? It's not supported on Mac. I tried to generalize this code to Mac, but it turned out non-trivial. It's not high priority, so I don't have any near terms plans to fix it for Mac.
lgtm https://codereview.webrtc.org/2835203003/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCNV12TextureCache.m (left): https://codereview.webrtc.org/2835203003/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCNV12TextureCache.m:104: RTC_CHECK(pixelFormat == kCVPixelFormatType_420YpCbCr8BiPlanarFullRange || On 2017/04/26 13:14:41, magjed_webrtc wrote: > On 2017/04/26 08:56:23, daniela-webrtc wrote: > > We no longer want to check the pixel format? > > I did a straight-forward conversion to NSAssert (I don't want to make this an > Objective C++ file just for RTC_CHECK), but then I got compile errors for > "unused variable pixelFormat" on release builds... So I decided to just drop > this check, it's not that important. Acknowledged. https://codereview.webrtc.org/2835203003/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCNV12TextureCache.m (right): https://codereview.webrtc.org/2835203003/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCNV12TextureCache.m:17: CVOpenGLESTextureRef _yTexture; On 2017/04/26 13:14:41, magjed_webrtc wrote: > On 2017/04/26 08:56:23, daniela-webrtc wrote: > > It's bit weird that the ivar and the property have different types. Perhaps > > instead of properties it would be better to expose the textures as methods? > > I agree it's a bit weird, and I don't know the conventions in ObjC. I think it > looks nicer for users of the class to be able to use the dot syntax, and it > would be consistent with the I420TextureCache. I changed the name of the ivar > slightly so it's not exactly the same as the property. Is that enough? Yes this is better. And yes, you are right exposing the textures as properties is better approach then methods so this looks good.
https://codereview.webrtc.org/2835203003/diff/140001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCNV12TextureCache.m (right): https://codereview.webrtc.org/2835203003/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCNV12TextureCache.m:53: *textureOut = nil; I think we should call CFRelease if textureOut is not already nil?
https://codereview.webrtc.org/2835203003/diff/140001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCNV12TextureCache.m (right): https://codereview.webrtc.org/2835203003/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCNV12TextureCache.m:53: *textureOut = nil; On 2017/04/26 13:31:56, sakal wrote: > I think we should call CFRelease if textureOut is not already nil? Looks like it. Done.
lgtm
The CQ bit was checked by magjed@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from denicija@webrtc.org Link to the patchset: https://codereview.webrtc.org/2835203003/#ps160001 (title: "CFRelease textureOut")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1493280538665560,
"parent_rev": "2fc04769faec0031a202963eaeb602420a082c07", "commit_rev":
"15f0c6815d089640230e625a4a9faa53b7dd7f62"}
Message was sent while issue was closed.
Description was changed from ========== ObjC: Split out NV12 texture uploading into separate class This CL splits out the code in RTCNV12Shader.mm for uploading byte buffers to textures into its own class RTCNV12TextureCache. The purpose is to prepare for allowing clients to inject their own shaders. RTCNV12TextureCache will be used in the generic code, while the actual shaders in RTCNV12Shader will be customizable by the client. BUG=webrtc:7473 ========== to ========== ObjC: Split out NV12 texture uploading into separate class This CL splits out the code in RTCNV12Shader.mm for uploading byte buffers to textures into its own class RTCNV12TextureCache. The purpose is to prepare for allowing clients to inject their own shaders. RTCNV12TextureCache will be used in the generic code, while the actual shaders in RTCNV12Shader will be customizable by the client. BUG=webrtc:7473 Review-Url: https://codereview.webrtc.org/2835203003 Cr-Commit-Position: refs/heads/master@{#17902} Committed: https://chromium.googlesource.com/external/webrtc/+/15f0c6815d089640230e625a4... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:160001) as https://chromium.googlesource.com/external/webrtc/+/15f0c6815d089640230e625a4... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
