Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2842453002:
ObjC: Split out I420 texture uploading into separate class (Closed)
|
Created:
3 years, 8 months ago by magjed_webrtc Modified:
3 years, 8 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionObjC: Split out I420 texture uploading into separate class
This CL splits out the code in RTCI420Shader.mm for uploading byte
buffers to textures into its own class RTCI420TextureCache. The purpose
is to prepare for allowing clients to inject their own shaders.
RTCI420TextureCache will be used in the generic code, while the actual
shaders in RTCI420Shader will be customizable by the client.
BUG=webrtc:7473
Review-Url: https://codereview.webrtc.org/2842453002
Cr-Commit-Position: refs/heads/master@{#17882}
Committed: https://chromium.googlesource.com/external/webrtc/+/8245a8542944b1b1c41dba6f61aa2d72cfe79536
Patch Set 1 #
Total comments: 2
Patch Set 2 : Remove comment. #
Total comments: 4
Patch Set 3 : Make uploadFrameToTextures return void #
Messages
Total messages: 28 (20 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: ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/23911) mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/20968) mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/25084)
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_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/25112)
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 #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== ObjC: Split out I420 texture uploading into separate class This CL splits out the code in RTCI420Shader.mm for uploading byte buffers to textures into its own class RTCI420TextureCache. The purpose is to prepare for allowing clients to inject their own shaders. RTCI420TextureCache will be used in the generic code, while the actual shaders in RTCI420Shader will be customizable by the client. BUG=webrtc:7473 ========== to ========== ObjC: Split out I420 texture uploading into separate class This CL splits out the code in RTCI420Shader.mm for uploading byte buffers to textures into its own class RTCI420TextureCache. The purpose is to prepare for allowing clients to inject their own shaders. RTCI420TextureCache will be used in the generic code, while the actual shaders in RTCI420Shader will be customizable by the client. BUG=webrtc:7473 ==========
magjed@webrtc.org changed reviewers: + denicija@webrtc.org, sakal@webrtc.org
Please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
So it the purpose that clients would reuse the code in the texture cache? Shouldn't it have a public header then? https://codereview.webrtc.org/2842453002/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCI420TextureCache.mm (right): https://codereview.webrtc.org/2842453002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCI420TextureCache.mm:17: // |kNumTextures| must not exceed 8, which is the limit in OpenGLES2. Two sets nit: I don't think this comment is relevant anymore. I think referred to the limitation of GL_TEXTURE7 being the highest texture index. Now we always use GL_TEXTURE0-2 so it is not a problem anymore.
On 2017/04/25 15:19:21, sakal wrote:
> So it the purpose that clients would reuse the code in the texture cache?
> Shouldn't it have a public header then?
>
My plan is to use the texture cache in RTCEAGLVideoView, and pass the uploaded
textures to an injectable shader delegate. Something like this:
@protocol RTCEAGLVideoViewShaderDelegate
/** Callback for I420 frames. Each plane is given as a texture. */
- (void)videoView:(RTCEAGLVideoView *)videoView
didReceiveFrameWithRotation:(RTCVideoRotation)rotation
yPlane:(GLuint)yPlane
uPlane:(GLuint)uPlane
vPlane:(GLuint)vPlane;
/** Callback for NV12 frames. Each plane is given as a texture. */
- (void)videoView:(RTCEAGLVideoView *)videoView
didReceiveFrameWithRotation:(RTCVideoRotation)rotation
yPlane:(GLuint)yPlane
uvPlane:(GLuint)uvPlane;
@end
Then the client only has to care about rendering textures (this is similar to
the RendererCommon.GlDrawer in Android).
An alternative approach is to add the texture cache to the public headers and
push the responsibility of uploading frames to the clients. Then the shader
interface would look something like the existing:
@protocol RTCShader <NSObject>
- (BOOL)drawFrame:(RTCVideoFrame *)frame;
@end
This wasn't the plan, but we can discuss pros and cons of the approaches
tomorrow.
https://codereview.webrtc.org/2842453002/diff/40001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/RTCI420TextureCache.mm (right):
https://codereview.webrtc.org/2842453002/diff/40001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/RTCI420TextureCache.mm:17: // |kNumTextures|
must not exceed 8, which is the limit in OpenGLES2. Two sets
On 2017/04/25 15:19:21, sakal wrote:
> nit: I don't think this comment is relevant anymore. I think referred to the
> limitation of GL_TEXTURE7 being the highest texture index. Now we always use
> GL_TEXTURE0-2 so it is not a problem anymore.
Done. I removed that sentence.
On 2017/04/25 17:44:30, magjed_webrtc wrote: > On 2017/04/25 15:19:21, sakal wrote: > > So it the purpose that clients would reuse the code in the texture cache? > > Shouldn't it have a public header then? > > > My plan is to use the texture cache in RTCEAGLVideoView, and pass the uploaded > textures to an injectable shader delegate. Something like this: > > @protocol RTCEAGLVideoViewShaderDelegate > > /** Callback for I420 frames. Each plane is given as a texture. */ > - (void)videoView:(RTCEAGLVideoView *)videoView > didReceiveFrameWithRotation:(RTCVideoRotation)rotation > yPlane:(GLuint)yPlane > uPlane:(GLuint)uPlane > vPlane:(GLuint)vPlane; > > /** Callback for NV12 frames. Each plane is given as a texture. */ > - (void)videoView:(RTCEAGLVideoView *)videoView > didReceiveFrameWithRotation:(RTCVideoRotation)rotation > yPlane:(GLuint)yPlane > uvPlane:(GLuint)uvPlane; > > @end > > Then the client only has to care about rendering textures (this is similar to > the RendererCommon.GlDrawer in Android). > > An alternative approach is to add the texture cache to the public headers and > push the responsibility of uploading frames to the clients. Then the shader > interface would look something like the existing: > > @protocol RTCShader <NSObject> > > - (BOOL)drawFrame:(RTCVideoFrame *)frame; > > @end > > This wasn't the plan, but we can discuss pros and cons of the approaches > tomorrow. > > https://codereview.webrtc.org/2842453002/diff/40001/webrtc/sdk/objc/Framework... > File webrtc/sdk/objc/Framework/Classes/RTCI420TextureCache.mm (right): > > https://codereview.webrtc.org/2842453002/diff/40001/webrtc/sdk/objc/Framework... > webrtc/sdk/objc/Framework/Classes/RTCI420TextureCache.mm:17: // |kNumTextures| > must not exceed 8, which is the limit in OpenGLES2. Two sets > On 2017/04/25 15:19:21, sakal wrote: > > nit: I don't think this comment is relevant anymore. I think referred to the > > limitation of GL_TEXTURE7 being the highest texture index. Now we always use > > GL_TEXTURE0-2 so it is not a problem anymore. > > Done. I removed that sentence. No this fine, I just didn't see the roadmap. lgtm
lgtm with two small remarks. https://codereview.webrtc.org/2842453002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCI420Shader.mm (right): https://codereview.webrtc.org/2842453002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCI420Shader.mm:91: [textureCache uploadFrameToTextures:frame]; What happens if the result of `uploadFrameToTextures` is NO? Do we not care for that use case anymore? https://codereview.webrtc.org/2842453002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCI420TextureCache.mm (right): https://codereview.webrtc.org/2842453002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCI420TextureCache.mm:148: return YES; Maybe change the method signature to return void? Seems like this returns YES always.
https://codereview.webrtc.org/2842453002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCI420Shader.mm (right): https://codereview.webrtc.org/2842453002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCI420Shader.mm:91: [textureCache uploadFrameToTextures:frame]; On 2017/04/26 08:00:57, daniela-webrtc wrote: > What happens if the result of `uploadFrameToTextures` is NO? Do we not care for > that use case anymore? Like you noticed in your other comment, it's not possible. https://codereview.webrtc.org/2842453002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCI420TextureCache.mm (right): https://codereview.webrtc.org/2842453002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCI420TextureCache.mm:148: return YES; On 2017/04/26 08:00:57, daniela-webrtc wrote: > Maybe change the method signature to return void? Seems like this returns YES > always. Yes! Thanks for pointing it out.
The CQ bit was checked by magjed@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sakal@webrtc.org, denicija@webrtc.org Link to the patchset: https://codereview.webrtc.org/2842453002/#ps70001 (title: "Make uploadFrameToTextures return void")
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": 70001, "attempt_start_ts": 1493196056814180,
"parent_rev": "bb08c3e29656fafe8a2d5d16ec4a62db49689f8a", "commit_rev":
"8245a8542944b1b1c41dba6f61aa2d72cfe79536"}
Message was sent while issue was closed.
Description was changed from ========== ObjC: Split out I420 texture uploading into separate class This CL splits out the code in RTCI420Shader.mm for uploading byte buffers to textures into its own class RTCI420TextureCache. The purpose is to prepare for allowing clients to inject their own shaders. RTCI420TextureCache will be used in the generic code, while the actual shaders in RTCI420Shader will be customizable by the client. BUG=webrtc:7473 ========== to ========== ObjC: Split out I420 texture uploading into separate class This CL splits out the code in RTCI420Shader.mm for uploading byte buffers to textures into its own class RTCI420TextureCache. The purpose is to prepare for allowing clients to inject their own shaders. RTCI420TextureCache will be used in the generic code, while the actual shaders in RTCI420Shader will be customizable by the client. BUG=webrtc:7473 Review-Url: https://codereview.webrtc.org/2842453002 Cr-Commit-Position: refs/heads/master@{#17882} Committed: https://chromium.googlesource.com/external/webrtc/+/8245a8542944b1b1c41dba6f6... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:70001) as https://chromium.googlesource.com/external/webrtc/+/8245a8542944b1b1c41dba6f6... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
