Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2651743007:
Add metal view, shaders and renderer. (Closed)
|
Created:
3 years, 11 months ago by daniela-webrtc Modified:
3 years, 10 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd metal view, shaders and renderer.
This CL submits standalone Metal view, renderer and shader.
BUG=webrtc:7079
Review-Url: https://codereview.webrtc.org/2651743007
Cr-Commit-Position: refs/heads/master@{#16787}
Committed: https://chromium.googlesource.com/external/webrtc/+/fc8c97f950a4fead4a5debe3c838942db1834010
Patch Set 1 #
Total comments: 20
Patch Set 2 : Address comments from patchset 1 #Patch Set 3 : Add metal renderer protocol and rename implementation classes #
Total comments: 21
Patch Set 4 : Address comments #
Total comments: 21
Patch Set 5 : Address comments #Patch Set 6 : Fix compilation #Patch Set 7 : Compile metal classes only for arm cpu's #Patch Set 8 : Compile metal classes only for arm64 cpu's #Patch Set 9 : Add CoreVideo.framework dependency. #Patch Set 10 : Link metal libraries only for arm64cpu #
Total comments: 33
Patch Set 11 : Address comments #Patch Set 12 : Use webrtc::VideoRotation enum for frame rotation #Patch Set 13 : Address comments #Patch Set 14 : Rename subfolder:metal->Metal #
Total comments: 6
Patch Set 15 : Address comments #Patch Set 16 : Add whitespace that was removed with format #
Total comments: 28
Patch Set 17 : Move RTCVideoRender implementation to view #
Total comments: 22
Patch Set 18 : Minor fixes #
Messages
Total messages: 91 (60 generated)
Description was changed from ========== WIP: Add metal view, shaders and renderer. This CL submits standalone Metal view, renderer and shader. Even though the classes are not hooked up yet and are not used in code it's smart not merge this CL until the compilation scripts are done. BUG=NONE ========== to ========== WIP: Add metal view, shaders and renderer. This CL submits standalone Metal view, renderer and shader. Even though the classes are not hooked up yet and are not used in code it's smart not merge this CL until the compilation scripts are done. BUG=NONE ==========
denicija@webrtc.org changed reviewers: + kthelgason@webrtc.org, magjed@webrtc.org
First stab at implementing metal rendering. Please take a look and share your thoughts.
Looks good overall, I'm impressed! Some comments below, and I think we need to add an interface similar to webrtc/sdk/objc/Framework/Classes/RTCShader.h so that we can have different implementations for I420 and NV12, and possible other PixelBuffer color formats. https://codereview.webrtc.org/2651743007/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/metal/RTCMTLRenderer.mm (right): https://codereview.webrtc.org/2651743007/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLRenderer.mm:187: RTCLog(@"Metal error: Falied to initialize metal texture cache. Return status is %d", status); spelling nit: Failed https://codereview.webrtc.org/2651743007/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLRenderer.mm:270: lumaHeight, 0, &outTexture); Maybe add a comment for the literal 0: /* indexPlane */? https://codereview.webrtc.org/2651743007/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLRenderer.mm:276: CVBufferRelease(outTexture); Is it ok to release |outTexture| here and keep using lumaTexture? https://codereview.webrtc.org/2651743007/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLRenderer.mm:282: lumaHeight / 2, 1, &outTexture); ditto: Maybe add a comment for the literal 1: /* indexPlane */? https://codereview.webrtc.org/2651743007/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLRenderer.mm:300: - (nullable id<MTLTexture>)transformFrame:(RTCVideoFrame *)videoFrame Is this function used?
Tried to have some semi-intelligent comments, although I'm way out of my comfort zone with this code. Good job on getting this done so quickly! https://codereview.webrtc.org/2651743007/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/metal/RTCMTLRenderer.mm (right): https://codereview.webrtc.org/2651743007/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLRenderer.mm:18: float cubeVertexData[64] = static const https://codereview.webrtc.org/2651743007/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLRenderer.mm:66: // In future we might opt in to use tripple buffering method if it improves performance. I think this sentence could be made more concise: In future we might use triple buffering if it improves performance. https://codereview.webrtc.org/2651743007/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLRenderer.mm:123: - (BOOL)_setupMetal { We haven't really been doing the _methodName. For methods not part of public api put them at the bottom of the file below a #pragma mark - private https://codereview.webrtc.org/2651743007/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLRenderer.mm:150: // VERY VERY MUCHISIMO IMPORTANTE. Maybe a less flamboyant comment? :D although I do like this https://codereview.webrtc.org/2651743007/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLRenderer.mm:254: [self setupSyncVariablesForFrame:frame]; I don't see the point of extracting this into a method. Can't it just be inlined here?
https://codereview.webrtc.org/2651743007/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/metal/RTCMTLRenderer.mm (right): https://codereview.webrtc.org/2651743007/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLRenderer.mm:18: float cubeVertexData[64] = On 2017/01/24 14:17:59, kthelgason wrote: > static const Done. https://codereview.webrtc.org/2651743007/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLRenderer.mm:66: // In future we might opt in to use tripple buffering method if it improves performance. On 2017/01/24 14:18:00, kthelgason wrote: > I think this sentence could be made more concise: > In future we might use triple buffering if it improves performance. Done. https://codereview.webrtc.org/2651743007/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLRenderer.mm:123: - (BOOL)_setupMetal { On 2017/01/24 14:17:59, kthelgason wrote: > We haven't really been doing the _methodName. For methods not part of public api > put them at the bottom of the file below a #pragma mark - private Yes it's not common. I saw this in Apple's Metal examples, where they use the _method to indicate low level methods involving the GPU. I think it's a good practice to follow for Metal code because it makes it really obvious what methods involve GPU. Do you think adding a comment to clarify this would make it better? https://codereview.webrtc.org/2651743007/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLRenderer.mm:150: // VERY VERY MUCHISIMO IMPORTANTE. On 2017/01/24 14:18:00, kthelgason wrote: > Maybe a less flamboyant comment? :D although I do like this Ooops! This one slipped :D was meant to be part of the hacking code. https://codereview.webrtc.org/2651743007/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLRenderer.mm:187: RTCLog(@"Metal error: Falied to initialize metal texture cache. Return status is %d", status); On 2017/01/24 14:01:56, magjed_webrtc wrote: > spelling nit: Failed Done. https://codereview.webrtc.org/2651743007/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLRenderer.mm:254: [self setupSyncVariablesForFrame:frame]; On 2017/01/24 14:18:00, kthelgason wrote: > I don't see the point of extracting this into a method. Can't it just be inlined > here? It can be inlined but it was my personal preference as I prefer separate methods over code comments (for instance). I think it nicely conveys the intention that this is part of the code dealing with synchronization of variables. https://codereview.webrtc.org/2651743007/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLRenderer.mm:270: lumaHeight, 0, &outTexture); On 2017/01/24 14:01:56, magjed_webrtc wrote: > Maybe add a comment for the literal 0: /* indexPlane */? Good idea. I've actually added a separate variable int indexPlane that will have values 0 and 1 respectively. I think that would make the intent even clearer. Let me know what you think. https://codereview.webrtc.org/2651743007/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLRenderer.mm:276: CVBufferRelease(outTexture); On 2017/01/24 14:01:56, magjed_webrtc wrote: > Is it ok to release |outTexture| here and keep using lumaTexture? Not sure I understand the question. Are you asking if lumaTexture will be available after outTexture is released? If that's the question the answer is yes. CVMetalTextyreGetTexture returns retained object (id<MTLTexture) that will be released when it gets out of scope (i.e when this method returns). So it's perfectly safe to use it even after outTexture has been released. https://codereview.webrtc.org/2651743007/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLRenderer.mm:282: lumaHeight / 2, 1, &outTexture); On 2017/01/24 14:01:56, magjed_webrtc wrote: > ditto: Maybe add a comment for the literal 1: /* indexPlane */? ^ https://codereview.webrtc.org/2651743007/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLRenderer.mm:300: - (nullable id<MTLTexture>)transformFrame:(RTCVideoFrame *)videoFrame On 2017/01/24 14:01:56, magjed_webrtc wrote: > Is this function used? Great catch. It was part of a previous implementation I made. Will remove it.
Description was changed from ========== WIP: Add metal view, shaders and renderer. This CL submits standalone Metal view, renderer and shader. Even though the classes are not hooked up yet and are not used in code it's smart not merge this CL until the compilation scripts are done. BUG=NONE ========== to ========== WIP: Add metal view, shaders and renderer. This CL submits standalone Metal view, renderer and shader. Even though the classes are not hooked up yet and are not used in code it's smart not merge this CL until the compilation scripts are done. BUG=7079 ==========
Description was changed from ========== WIP: Add metal view, shaders and renderer. This CL submits standalone Metal view, renderer and shader. Even though the classes are not hooked up yet and are not used in code it's smart not merge this CL until the compilation scripts are done. BUG=7079 ========== to ========== WIP: Add metal view, shaders and renderer. This CL submits standalone Metal view, renderer and shader. Even though the classes are not hooked up yet and are not used in code it's smart not merge this CL until the compilation scripts are done. BUG=webrtc:7079 ==========
Description was changed from ========== WIP: Add metal view, shaders and renderer. This CL submits standalone Metal view, renderer and shader. Even though the classes are not hooked up yet and are not used in code it's smart not merge this CL until the compilation scripts are done. BUG=webrtc:7079 ========== to ========== Add metal view, shaders and renderer. This CL submits standalone Metal view, renderer and shader. BUG=webrtc:7079 ==========
I think this is pass the WIP phase now. I did further changes (added the RTCMTLRenderer protocol and nv12 renderer implementation). Please take one final look so that we can land this.
The copyright header needs updating to 2017 and the includes need to be in the correct order and with LFs in the right places. Other than that this lgtm. https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/BUILD.gn File webrtc/sdk/BUILD.gn (right): https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/BUILD.gn#newco... webrtc/sdk/BUILD.gn:54: "objc/Framework/Headers/WebRTC/RTCMTLVideoView.h", All of these changes should be in the framework target, and only for iOS right? https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/metal/RTCMTLVideoView.m (right): https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLVideoView.m:2: * Copyright 2016 The WebRTC project authors. All Rights Reserved. 2017 https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLVideoView.m:11: #import "WebRTC/RTCMTLVideoView.h" blank line between this files' header and system headers https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLVideoView.m:12: #import <Metal/Metal.h> Blank line between system headers and other project headers https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLVideoView.m:13: #import "RTCNV12Renderer.h" This import should be prefixed with WebRTC/ https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLVideoView.m:13: #import "RTCNV12Renderer.h" This import should be prefixed with WebRTC/ https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLVideoView.m:37: @end I think this file should have a .mm extension, but I'm not sure. Perhaps someone can back me up on this. https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm (right): https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm:36: break; break not needed after return https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/metal/Shaders.metal (right): https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/Shaders.metal:1: Missing copyright header
https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/BUILD.gn File webrtc/sdk/BUILD.gn (right): https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/BUILD.gn#newco... webrtc/sdk/BUILD.gn:54: "objc/Framework/Headers/WebRTC/RTCMTLVideoView.h", On 2017/02/03 09:17:23, kthelgason wrote: > All of these changes should be in the framework target, and only for iOS right? Done. https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/BUILD.gn#newco... webrtc/sdk/BUILD.gn:54: "objc/Framework/Headers/WebRTC/RTCMTLVideoView.h", On 2017/02/03 09:17:23, kthelgason wrote: > All of these changes should be in the framework target, and only for iOS right? This is the only part that's outside of the framework and not iOS exclusive. It's also redundant as the headers are part of the public common_objc_headers https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/metal/RTCMTLVideoView.m (right): https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLVideoView.m:2: * Copyright 2016 The WebRTC project authors. All Rights Reserved. On 2017/02/03 09:17:23, kthelgason wrote: > 2017 Done. https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLVideoView.m:2: * Copyright 2016 The WebRTC project authors. All Rights Reserved. On 2017/02/03 09:17:23, kthelgason wrote: > 2017 Done. https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLVideoView.m:11: #import "WebRTC/RTCMTLVideoView.h" On 2017/02/03 09:17:23, kthelgason wrote: > blank line between this files' header and system headers > > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes Done. https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLVideoView.m:11: #import "WebRTC/RTCMTLVideoView.h" On 2017/02/03 09:17:23, kthelgason wrote: > blank line between this files' header and system headers > > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes Done. https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLVideoView.m:12: #import <Metal/Metal.h> On 2017/02/03 09:17:23, kthelgason wrote: > Blank line between system headers and other project headers Done. https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLVideoView.m:12: #import <Metal/Metal.h> On 2017/02/03 09:17:23, kthelgason wrote: > Blank line between system headers and other project headers Done. https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLVideoView.m:13: #import "RTCNV12Renderer.h" On 2017/02/03 09:17:23, kthelgason wrote: > This import should be prefixed with WebRTC/ The WebRTC/ prefix works only for public framework headers and RTCNV12Renderer.h is not public header. https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/RTCMTLVideoView.m:37: @end On 2017/02/03 09:17:23, kthelgason wrote: > I think this file should have a .mm extension, but I'm not sure. Perhaps someone > can back me up on this. Since this class is pure objective-c (doesn't have any cpp or objective-cpp references) there is no need for the .mm suffix https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm (right): https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm:36: break; On 2017/02/03 09:17:23, kthelgason wrote: > break not needed after return Done. https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/metal/Shaders.metal (right): https://codereview.webrtc.org/2651743007/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/Shaders.metal:1: On 2017/02/03 09:17:23, kthelgason wrote: > Missing copyright header Done.
Looks good! Comments are just nits, but please address before landing. lgtm https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/BUILD.gn File webrtc/sdk/BUILD.gn (right): https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/BUILD.gn#newco... webrtc/sdk/BUILD.gn:318: "objc/Framework/Headers/WebRTC/RTCMTLRenderer.h", Sort these. https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.h (right): https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.h:2: * Copyright 2016 The WebRTC project authors. All Rights Reserved. 2017 https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm (right): https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm:77: int rotation; There is no need to store |rotation|, just store offset. https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm:188: if (_renderPassDescriptor) { // valid drawable. nit: Valid drawable. https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm:197: [renderEncoder setVertexBuffer:_vertexBuffer offset:sizeof(float[offset]) atIndex:0]; nit: I haven't seen that way of using sizeof. I'm more used to: offset * sizeof(float). https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm:279: if (rotation != frame.rotation) { Just do an unconditional offset = offsetForRotation(frame.rotation); https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/metal/Shaders.metal (right): https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/Shaders.metal:32: Maybe remove these empty lines between? https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/Shaders.metal:51: uv = textureCbCr.sample(s, in.texcoord).rg - float2(0.5,0.5); nit: add space between '0.5,0.5', i.e. '0.5, 0.5' https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLRenderer.h (right): https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLRenderer.h:30: * Unexpexted behavior can happen if the cleanup doesn't happen and the renderer continues nit: Unexpected https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h (right): https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h:2: * Copyright 2016 The WebRTC project authors. All Rights Reserved. 2017 https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h:36: * Unexpexted behavior can happen if the cleanup doesn't happen. nit: Unexpected
https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/BUILD.gn File webrtc/sdk/BUILD.gn (right): https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/BUILD.gn#newco... webrtc/sdk/BUILD.gn:318: "objc/Framework/Headers/WebRTC/RTCMTLRenderer.h", On 2017/02/07 12:12:41, magjed_webrtc wrote: > Sort these. Done. https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.h (right): https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.h:2: * Copyright 2016 The WebRTC project authors. All Rights Reserved. On 2017/02/07 12:12:41, magjed_webrtc wrote: > 2017 Done. https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm (right): https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm:77: int rotation; On 2017/02/07 12:12:41, magjed_webrtc wrote: > There is no need to store |rotation|, just store offset. Done. https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm:188: if (_renderPassDescriptor) { // valid drawable. On 2017/02/07 12:12:41, magjed_webrtc wrote: > nit: Valid drawable. Done. https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm:197: [renderEncoder setVertexBuffer:_vertexBuffer offset:sizeof(float[offset]) atIndex:0]; On 2017/02/07 12:12:41, magjed_webrtc wrote: > nit: I haven't seen that way of using sizeof. I'm more used to: offset * > sizeof(float). Done. https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm:279: if (rotation != frame.rotation) { On 2017/02/07 12:12:41, magjed_webrtc wrote: > Just do an unconditional offset = offsetForRotation(frame.rotation); Done. https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/metal/Shaders.metal (right): https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/Shaders.metal:32: On 2017/02/07 12:12:41, magjed_webrtc wrote: > Maybe remove these empty lines between? Done. https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/metal/Shaders.metal:51: uv = textureCbCr.sample(s, in.texcoord).rg - float2(0.5,0.5); On 2017/02/07 12:12:41, magjed_webrtc wrote: > nit: add space between '0.5,0.5', i.e. '0.5, 0.5' Done. https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLRenderer.h (right): https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLRenderer.h:30: * Unexpexted behavior can happen if the cleanup doesn't happen and the renderer continues On 2017/02/07 12:12:41, magjed_webrtc wrote: > nit: Unexpected Done. https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h (right): https://codereview.webrtc.org/2651743007/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h:36: * Unexpexted behavior can happen if the cleanup doesn't happen. On 2017/02/07 12:12:41, magjed_webrtc wrote: > nit: Unexpected Done.
The CQ bit was checked by denicija@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kthelgason@webrtc.org, magjed@webrtc.org Link to the patchset: https://codereview.webrtc.org/2651743007/#ps100001 (title: "Fix compilation")
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: ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/22477)
The CQ bit was checked by denicija@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: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
The CQ bit was checked by denicija@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: ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/17235) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/17060) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/22498)
The CQ bit was checked by denicija@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: ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/17237) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/17062) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/22500) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/21251)
The CQ bit was checked by denicija@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_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/17085)
The CQ bit was checked by denicija@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: ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...) mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/22530)
The CQ bit was checked by denicija@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: ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...)
The CQ bit was checked by denicija@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: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
The CQ bit was checked by denicija@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: This issue passed the CQ dry run.
denicija@webrtc.org changed reviewers: + tkchin@webrtc.org
@Zeke please take a look and let me know what you think.
tkchin@webrtc.org changed reviewers: + haysc@webrtc.org
first pass with some higher level comments https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.h (right): https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.h:11: #import <Foundation/Foundation.h> nit: newline after system imports https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.h:15: * Implementation of RTCMTLRenderer protocol for rendering native nv12 video frames. nit: place after ** /** Imple.. https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.h:17: @interface RTCNV12Renderer : NSObject<RTCMTLRenderer> nit: NSObject < https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.h:17: @interface RTCNV12Renderer : NSObject<RTCMTLRenderer> Should this be RTCNV12MetalRenderer? Do we have plans to compare Metal vs OpenGL perf? https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm (right): https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm:32: static inline int offsetForRotation(int rotation) { Would rotation be better as an enum? Otherwise defaults to 0 and won't know. https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm:53: @interface RTCNV12Renderer ()<MTKViewDelegate> space after () () <MTK https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm:80: - (instancetype)initWithView:(__kindof MTKView *)view { I don't think renderer should have dependency on having a view https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm:95: [self _setupView]; don't use _ in method names If they are meant to be private, stuff them in a #pragma mark - Private section https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm:113: NSBundle *frameworkBundle = [NSBundle bundleWithIdentifier:@"org.webrtc.WebRTC"]; This should be a predefined constant somewhere that's globally available https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm:227: #pragma mark - RTCVideoRenderer nit: newline after pragma https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/metal/Shaders.metal (right): https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/metal/Shaders.metal:14: using namespace metal; It is kinda burdensome to force users to include this in their bundle. Can this be done as a C define instead? I'll send you an email with a link for an example. https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLRenderer.h (right): https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLRenderer.h:21: @protocol RTCMTLRenderer<RTCVideoRenderer> RTCMTLRendere <RTC https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLRenderer.h:23: - (instancetype)initWithView:(__weak __kindof MTKView *)view; Don't put ctors in protocol decls, overly restrictive, and you can't make any guarantees on how subclasses choose to implement it. MTKView behaves like GLKView of the past Any reason not to create a composed view instead? Where you have an RTCMTLVideoView that sets up a MTKView? https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h (right): https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h:24: @interface RTCMTLVideoView : MTKView I think it is better to subclass UIView here instead. https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h:40: #pragma mark - Unavailable nit: don't need this pragma https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h:42: + (instancetype) new NS_UNAVAILABLE; nit: remove space also, ooc why is new explicitly forbidden?
https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.h (right): https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.h:11: #import <Foundation/Foundation.h> On 2017/02/10 23:32:10, tkchin_webrtc wrote: > nit: newline after system imports Done. https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.h:15: * Implementation of RTCMTLRenderer protocol for rendering native nv12 video frames. On 2017/02/10 23:32:09, tkchin_webrtc wrote: > nit: place after ** > /** Imple.. Done. https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.h:17: @interface RTCNV12Renderer : NSObject<RTCMTLRenderer> On 2017/02/10 23:32:10, tkchin_webrtc wrote: > nit: NSObject < According to the guidelines (and the Clang formatter that I use locally) there should be no space. https://google.github.io/styleguide/objcguide.xml?showone=Protocols#Protocols https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm (right): https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm:32: static inline int offsetForRotation(int rotation) { On 2017/02/10 23:32:10, tkchin_webrtc wrote: > Would rotation be better as an enum? Otherwise defaults to 0 and won't know. Done. https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm:80: - (instancetype)initWithView:(__kindof MTKView *)view { On 2017/02/10 23:32:10, tkchin_webrtc wrote: > I don't think renderer should have dependency on having a view Not sure how to avoid this dependency without exposing some internal implementation to the outside world. The _view variable needs to be setup properly in the _setupView method otherwise the render would not work properly. https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm:95: [self _setupView]; On 2017/02/10 23:32:10, tkchin_webrtc wrote: > don't use _ in method names > > If they are meant to be private, stuff them in a #pragma mark - Private section This is taken from some of the Apple examples where they use _method to denote methods involving the GPU and I thought it made sense. Wdyt? https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm:113: NSBundle *frameworkBundle = [NSBundle bundleWithIdentifier:@"org.webrtc.WebRTC"]; On 2017/02/10 23:32:10, tkchin_webrtc wrote: > This should be a predefined constant somewhere that's globally available I'll think a bit how to best implement this. Ideally it would be derived from the build target in a form of build argument or environmental variable. Do you perhaps know of some reliable way of doing this? https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm:227: #pragma mark - RTCVideoRenderer On 2017/02/10 23:32:10, tkchin_webrtc wrote: > nit: newline after pragma Done. https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/metal/Shaders.metal (right): https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/metal/Shaders.metal:14: using namespace metal; On 2017/02/10 23:32:10, tkchin_webrtc wrote: > It is kinda burdensome to force users to include this in their bundle. > Can this be done as a C define instead? > I'll send you an email with a link for an example. Yes it can be done like that as well. I managed to make it work, thanks to your pointers :) We discussed this in the team a bit as well. Basically that would simplify the build process but we might lose the biggest performance win Metal offers which is precompiling shaders in a bundled library. So we'll postpone this until we do some profiling. Also previous experience shows that big static strings might increase binary size even more than the bundled metal lib. https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLRenderer.h (right): https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLRenderer.h:23: - (instancetype)initWithView:(__weak __kindof MTKView *)view; On 2017/02/10 23:32:10, tkchin_webrtc wrote: > Don't put ctors in protocol decls, overly restrictive, and you can't make any > guarantees on how subclasses choose to implement it. > > MTKView behaves like GLKView of the past > > Any reason not to create a composed view instead? Where you have an > RTCMTLVideoView that sets up a MTKView? > > I see your point of this being restrictive and makes the API opinionated. The reason why I wanted to inject the MTKView as early as possible (in the `init`) was to have ensure control over the view setup (the setup of the view is tied to the MTLDevice object in the renderer). So it's a tradeoff between flexibility and reliability. https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h (right): https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h:24: @interface RTCMTLVideoView : MTKView On 2017/02/10 23:32:10, tkchin_webrtc wrote: > I think it is better to subclass UIView here instead. You are right. I'm already working on a follow up CL to change that. Is it OK to keep that in the separate on as it's little bit more than one liner change? https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h:42: + (instancetype) new NS_UNAVAILABLE; On 2017/02/10 23:32:10, tkchin_webrtc wrote: > nit: remove space > also, ooc why is new explicitly forbidden? Initially it was there because I did the renderer alloc in a non-designated initializer. I guess I forgot to remove this when I moved the allocation in a designated initializer. Prohibiting the init/new is no longer needed.
also nit: can we capitalize Metal in the directory name? Chuck, can you also make a pass? https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.h (right): https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.h:17: @interface RTCNV12Renderer : NSObject<RTCMTLRenderer> On 2017/02/13 12:32:06, denicija-webrtc wrote: > On 2017/02/10 23:32:10, tkchin_webrtc wrote: > > nit: NSObject < > > According to the guidelines (and the Clang formatter that I use locally) there > should be no space. > https://google.github.io/styleguide/objcguide.xml?showone=Protocols#Protocols See internal style guide (rev. 3.85). There isn't official guidance here, so we should stay consistent with our own rep and use a space. Personally I recommend following style of core Google components. https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm (right): https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm:95: [self _setupView]; On 2017/02/13 12:32:06, denicija-webrtc wrote: > On 2017/02/10 23:32:10, tkchin_webrtc wrote: > > don't use _ in method names > > > > If they are meant to be private, stuff them in a #pragma mark - Private > section > > This is taken from some of the Apple examples where they use _method to denote > methods involving the GPU and I thought it made sense. Wdyt? I would recommend against it. _ frequently is used to denote private methods in other codebases, and this could be confusing. Instead, just place GPU methods in a pragma section. https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/metal/Shaders.metal (right): https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/metal/Shaders.metal:14: using namespace metal; On 2017/02/13 12:32:06, denicija-webrtc wrote: > On 2017/02/10 23:32:10, tkchin_webrtc wrote: > > It is kinda burdensome to force users to include this in their bundle. > > Can this be done as a C define instead? > > I'll send you an email with a link for an example. > > Yes it can be done like that as well. I managed to make it work, thanks to your > pointers :) > We discussed this in the team a bit as well. > Basically that would simplify the build process but we might lose the biggest > performance win Metal offers which is precompiling shaders in a bundled library. > So we'll postpone this until we do some profiling. > Also previous experience shows that big static strings might increase binary > size even more than the bundled metal lib. I'm slightly worried about how to compile dynamic frameworks with prebuilt shaders. It may impact CocoaPod work if this is checked in right now. Bundle loading can be finicky, and i can only hope that shader loading works as expected when a binary is linked against a dynamic framework and needs access to things in its bundle. I do agree that profiling is required to determine which approach to take here - since we don't have a lot of shaders I don't know what the real perf impact is. https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLRenderer.h (right): https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLRenderer.h:23: - (instancetype)initWithView:(__weak __kindof MTKView *)view; On 2017/02/13 12:32:06, denicija-webrtc wrote: > On 2017/02/10 23:32:10, tkchin_webrtc wrote: > > Don't put ctors in protocol decls, overly restrictive, and you can't make any > > guarantees on how subclasses choose to implement it. > > > > MTKView behaves like GLKView of the past > > > > Any reason not to create a composed view instead? Where you have an > > RTCMTLVideoView that sets up a MTKView? > > > > > > I see your point of this being restrictive and makes the API opinionated. > The reason why I wanted to inject the MTKView as early as possible (in the > `init`) was to have ensure control over the view setup (the setup of the view is > tied to the MTLDevice object in the renderer). So it's a tradeoff between > flexibility and reliability. Sure, but that can be a base abstract class. In this particular case, it's odd to me to say that a Metal renderer must have a setup() method, and that it must take a kind of MTKView. This really seems to me like a concrete implementation of a kind of video renderer. In general, I don't think it is good practice for most objects to have a ctor that takes in a UIView. A UIView may or may not be actually in the view hierarchy at any given moment of time. In this rendering case, the view should be the one that decides if it wants to render or not, since it knows about when it actually draws and layouts. https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h (right): https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h:24: @interface RTCMTLVideoView : MTKView On 2017/02/13 12:32:07, denicija-webrtc wrote: > On 2017/02/10 23:32:10, tkchin_webrtc wrote: > > I think it is better to subclass UIView here instead. > > You are right. I'm already working on a follow up CL to change that. Is it OK to > keep that in the separate on as it's little bit more than one liner change? I would prefer that headers that are checked into this directory are already 100% ready to go, since this API is public and we want to maintain it gracefully. If it's still in development, can we put this file elsewhere temporarily?
On 2017/02/14 00:23:57, tkchin_webrtc wrote: > also nit: can we capitalize Metal in the directory name? Done. > Chuck, can you also make a pass? > > https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... > File webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.h (right): > > https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.h:17: @interface > RTCNV12Renderer : NSObject<RTCMTLRenderer> > On 2017/02/13 12:32:06, denicija-webrtc wrote: > > On 2017/02/10 23:32:10, tkchin_webrtc wrote: > > > nit: NSObject < > > > > According to the guidelines (and the Clang formatter that I use locally) there > > should be no space. > > https://google.github.io/styleguide/objcguide.xml?showone=Protocols#Protocols > > See internal style guide (rev. 3.85). There isn't official guidance here, so we > should stay consistent with our own rep and use a space. Personally I recommend > following style of core Google components. Yes, you are right. The docs are inconclusive so I'll follow your suggestion. I've updated everything in this CL. > https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... > File webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm (right): > > https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/metal/RTCNV12Renderer.mm:95: [self > _setupView]; > On 2017/02/13 12:32:06, denicija-webrtc wrote: > > On 2017/02/10 23:32:10, tkchin_webrtc wrote: > > > don't use _ in method names > > > > > > If they are meant to be private, stuff them in a #pragma mark - Private > > section > > > > This is taken from some of the Apple examples where they use _method to denote > > methods involving the GPU and I thought it made sense. Wdyt? > > I would recommend against it. _ frequently is used to denote private methods in > other codebases, and this could be confusing. > Instead, just place GPU methods in a pragma section. Done. > https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... > File webrtc/sdk/objc/Framework/Classes/metal/Shaders.metal (right): > > https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/metal/Shaders.metal:14: using namespace metal; > On 2017/02/13 12:32:06, denicija-webrtc wrote: > > On 2017/02/10 23:32:10, tkchin_webrtc wrote: > > > It is kinda burdensome to force users to include this in their bundle. > > > Can this be done as a C define instead? > > > I'll send you an email with a link for an example. > > > > Yes it can be done like that as well. I managed to make it work, thanks to > your > > pointers :) > > We discussed this in the team a bit as well. > > Basically that would simplify the build process but we might lose the biggest > > performance win Metal offers which is precompiling shaders in a bundled > library. > > So we'll postpone this until we do some profiling. > > Also previous experience shows that big static strings might increase binary > > size even more than the bundled metal lib. > > I'm slightly worried about how to compile dynamic frameworks with prebuilt > shaders. It may impact CocoaPod work if this is checked in right now. Bundle > loading can be finicky, and i can only hope that shader loading works as > expected when a binary is linked against a dynamic framework and needs access to > things in its bundle. > I do agree that profiling is required to determine which approach to take here - > since we don't have a lot of shaders I don't know what the real perf impact is. I'll soon share profiling stats and we can decide what approach to take. > https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... > File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLRenderer.h (right): > > https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLRenderer.h:23: - > (instancetype)initWithView:(__weak __kindof MTKView *)view; > On 2017/02/13 12:32:06, denicija-webrtc wrote: > > On 2017/02/10 23:32:10, tkchin_webrtc wrote: > > > Don't put ctors in protocol decls, overly restrictive, and you can't make > any > > > guarantees on how subclasses choose to implement it. > > > > > > MTKView behaves like GLKView of the past > > > > > > Any reason not to create a composed view instead? Where you have an > > > RTCMTLVideoView that sets up a MTKView? > > > > > > > > > > I see your point of this being restrictive and makes the API opinionated. > > The reason why I wanted to inject the MTKView as early as possible (in the > > `init`) was to have ensure control over the view setup (the setup of the view > is > > tied to the MTLDevice object in the renderer). So it's a tradeoff between > > flexibility and reliability. > > Sure, but that can be a base abstract class. In this particular case, it's odd > to me to say that a Metal renderer must have a setup() method, and that it must > take a kind of MTKView. This really seems to me like a concrete implementation > of a kind of video renderer. > > In general, I don't think it is good practice for most objects to have a ctor > that takes in a UIView. A UIView may or may not be actually in the view > hierarchy at any given moment of time. In this rendering case, the view should > be the one that decides if it wants to render or not, since it knows about when > it actually draws and layouts. I'll see how to better address this and separate the concerns of the view and the renderer. > https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... > File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h (right): > > https://codereview.webrtc.org/2651743007/diff/180001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h:24: @interface > RTCMTLVideoView : MTKView > On 2017/02/13 12:32:07, denicija-webrtc wrote: > > On 2017/02/10 23:32:10, tkchin_webrtc wrote: > > > I think it is better to subclass UIView here instead. > > > > You are right. I'm already working on a follow up CL to change that. Is it OK > to > > keep that in the separate on as it's little bit more than one liner change? > > I would prefer that headers that are checked into this directory are already > 100% ready to go, since this API is public and we want to maintain it > gracefully. > If it's still in development, can we put this file elsewhere temporarily? I've add this change as well.
I'm not very familiar with Metal, so mostly looked at style things. https://codereview.webrtc.org/2651743007/diff/260001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m (right): https://codereview.webrtc.org/2651743007/diff/260001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:43: _renderer = [[RTCNV12Renderer alloc] initWithView:_metalView]; Is this indented 1 too many spaces? https://codereview.webrtc.org/2651743007/diff/260001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.mm (right): https://codereview.webrtc.org/2651743007/diff/260001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.mm:50: // In future we might use tripple buffering method if it improves performance. s/tripple/triple/ https://codereview.webrtc.org/2651743007/diff/260001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.mm:95: if ([self setupMetal] && _device) { Why not check _device on 110 and return NO? https://codereview.webrtc.org/2651743007/diff/260001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.mm:136: // This is very important, should not be removed. Nit: why is it important? Add to comment https://codereview.webrtc.org/2651743007/diff/260001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.mm:190: nit: extra blank line here can be removed
https://codereview.webrtc.org/2651743007/diff/260001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.mm (right): https://codereview.webrtc.org/2651743007/diff/260001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.mm:136: // This is very important, should not be removed. On 2017/02/15 14:54:05, Chuck wrote: > Nit: why is it important? Add to comment It's lingering comment from before. Also I realized it's not belonging here but rather in the VideoView, so I moved it.
On 2017/02/16 15:38:05, denicija-webrtc wrote: Zeke: I think I addressed most of your comments. To summarize: - Added run-time compilation of shaders (profiling showed that run time compilation is actually ~3 ms faster than pre-bundled loading). Thanks for the tip! - Removed initializer with view from RTCMTLRenderer - Made RTCMTLVideoView inherit from UIView and contain a MTKView as property. Chuck: thanks for the review. I've addressed your comments as well.
Really wish we could do an over the shoulder CR, would probably make things go faster. Can we address the RTCMTLVideoView interface comment, and then follow up in subsequent CLs once we agree on what it should look like offline? I think this is looking good overall, thanks for your patience. https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m (right): https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:29: Probably want initWithCoder for all the folks making our view from nibs/storyboards. https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:32: #if defined(__OBJC__) && COREVIDEO_SUPPORTS_METAL ooc why __OBJC__? https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:50: #endif Probably want to return nil if metal isn't supported may want to add convenience static method to check as well somewhere. Either in our UIDevice category or as a static method on this class + (BOOL)isMetalSupported; https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.h (right): https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.h:19: @interface RTCNV12Renderer : NSObject <RTCMTLRenderer> nit: RTCMTLNV12Renderer or RTCMetalNV12Renderer So that we can have a RTCOpenGLNV12Renderer or something per other comment about shader interface though, can we hide this file for now pending final interface design? Or otherwise just fold it in. https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.mm (right): https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.mm:111: id<MTLTexture> CrCbTexture; nit _textureCache, _yTexture: _crCbTexture https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.mm:167: RTCLog(@"Metal Error: Library with source failed\n%@", libraryError); RTCLogError for errors. Here and elsewhere. https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.mm:228: commandBuffer.label = @"RTCCommandBuffer"; stuff these constants into globals at the top of the file. e.g static NSString const *kRTCMTLNV12RendererCommandBufferLabel https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.mm:265: NSLog(@"No current frame. Aborting drawinFrame:"); RTCLog, don't use NSLog (otherwise won't show up in WebRTC logstream) https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.mm:268: @autoreleasepool { why is an autoreleasepool required here? https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.mm:275: - (void)setSize:(CGSize)size { I think this is a legacy API fyi. It'd be nice to get rid of needing to have it, but not required in this CL. https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.mm:281: if (frame == NULL) { use nil instead of NULL for non-objC object pointers, use nullptr for ObjC object pointers, use nil https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.mm:292: CVMetalTextureRef outTexture; nit: initialize to nullptr https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.mm:307: CVBufferRelease(outTexture); nit: consider using a guard anyway to make the code more explicit despite CVBufferRelease being able to handle taking nullptr without crashing because most people wouldn't know that without checking the doco. up to you. https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.mm:308: outTexture = nil; nullptr for refs https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.mm:321: dispatch_async(dispatch_get_main_queue(), ^{ seems expensive to dispatch at 30fps since this is called within renderFrame Shouldn't we also be rendering at screen refresh frequency, as opposed to decoder output frequency? https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h (right): https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h:30: @property(nonatomic, readonly) id<RTCVideoRenderer> renderer; For the purposes of landing this CL, can we remove this property first, or otherwise make it private? We have a meeting next week to chat about pending work, and one of the items is how to pass in shaders, and how to make it so that it's possible to chain shaders. In this CL, can we make it so that a metal renderer view is initialized like so (basically old behavior): RTCMTLVideoView *videoView = [[RTCMTLVideoView alloc] initWithFrame:frame]; rtcVideoTrack.renderer = videoView; As opposed to the current setup where we need to also create a renderer and pass it in, which is really the creation of a shader and passing it in. We can imagine adding a new ctor to RTCMTLVideoView like initWithFrame:shader: in the future, and that should work easily with old init method (old init method just uses NV12 shader by default).
Thanks for the input, it was useful and on point. I've modified the CL and I'm happy as well now. I think we can build up from here if necessary. Let me know if you have final comments, if not I'll go ahead and land this. https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m (right): https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:29: On 2017/02/16 23:28:04, tkchin_webrtc wrote: > Probably want initWithCoder for all the folks making our view from > nibs/storyboards. Done. https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:32: #if defined(__OBJC__) && COREVIDEO_SUPPORTS_METAL On 2017/02/16 23:28:04, tkchin_webrtc wrote: > ooc why __OBJC__? Metal is available only for ObjectiveC for now. https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:50: #endif On 2017/02/16 23:28:04, tkchin_webrtc wrote: > Probably want to return nil if metal isn't supported instancetype is nonnull in the declaration. > may want to add convenience static method to check as well somewhere. Either in > our UIDevice category or as a static method on this class > > + (BOOL)isMetalSupported; Done https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.h (right): https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.h:19: @interface RTCNV12Renderer : NSObject <RTCMTLRenderer> On 2017/02/16 23:28:04, tkchin_webrtc wrote: > nit: RTCMTLNV12Renderer or RTCMetalNV12Renderer > So that we can have a RTCOpenGLNV12Renderer or something > > per other comment about shader interface though, can we hide this file for now > pending final interface design? Or otherwise just fold it in. Done. https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.mm (right): https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.mm:111: id<MTLTexture> CrCbTexture; On 2017/02/16 23:28:04, tkchin_webrtc wrote: > nit _textureCache, _yTexture: _crCbTexture Done. https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.mm:167: RTCLog(@"Metal Error: Library with source failed\n%@", libraryError); On 2017/02/16 23:28:04, tkchin_webrtc wrote: > RTCLogError for errors. Here and elsewhere. Done. https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.mm:228: commandBuffer.label = @"RTCCommandBuffer"; On 2017/02/16 23:28:04, tkchin_webrtc wrote: > stuff these constants into globals at the top of the file. > e.g > static NSString const *kRTCMTLNV12RendererCommandBufferLabel Done. https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.mm:265: NSLog(@"No current frame. Aborting drawinFrame:"); On 2017/02/16 23:28:04, tkchin_webrtc wrote: > RTCLog, don't use NSLog (otherwise won't show up in WebRTC logstream) Done. https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.mm:281: if (frame == NULL) { On 2017/02/16 23:28:04, tkchin_webrtc wrote: > use nil instead of NULL > > for non-objC object pointers, use nullptr > for ObjC object pointers, use nil Done. https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.mm:292: CVMetalTextureRef outTexture; On 2017/02/16 23:28:04, tkchin_webrtc wrote: > nit: initialize to nullptr Done. https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.mm:307: CVBufferRelease(outTexture); On 2017/02/16 23:28:04, tkchin_webrtc wrote: > nit: consider using a guard anyway to make the code more explicit despite > CVBufferRelease being able to handle taking nullptr without crashing because > most people wouldn't know that without checking the doco. up to you. Added code comment to clarify. https://codereview.webrtc.org/2651743007/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCNV12Renderer.mm:308: outTexture = nil; On 2017/02/16 23:28:04, tkchin_webrtc wrote: > nullptr for refs Done.
https://codereview.webrtc.org/2651743007/diff/320001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m (right): https://codereview.webrtc.org/2651743007/diff/320001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:56: return NO; nit: I'd prefer the return NO to be within an else branch of the ifdef. Otherwise the code might fail to compile with -Wunreachable-code.
lgtm let's land and address the renderer protocol discussion later https://codereview.webrtc.org/2651743007/diff/320001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLNV12Renderer.mm (right): https://codereview.webrtc.org/2651743007/diff/320001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLNV12Renderer.mm:90: float4 out = float4(y + 1.403 * uv.y, y - 0.344 * uv.x - 0.714 * uv.y, y + 1.770 * uv.x, 1.0); nit: I'd doco source of these constants and which color space it's using https://codereview.webrtc.org/2651743007/diff/320001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLNV12Renderer.mm:101: @interface RTCNV12Renderer () doesn't seem to be a need for this decl https://codereview.webrtc.org/2651743007/diff/320001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLNV12Renderer.mm:125: int offset; _offset https://codereview.webrtc.org/2651743007/diff/320001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLNV12Renderer.mm:239: dispatch_semaphore_signal(block_semaphore); can you doco what the semaphores accomplish? from here what I gather is that we have to wait for the previous command to finish first but then now we have to be careful because it will block the thread that we are on. Not sure what thread drawInMTKView calls back on though. https://codereview.webrtc.org/2651743007/diff/320001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLNV12Renderer.mm:272: @autoreleasepool { why is autoreleasepool needed here? to drain the command buffers faster? https://codereview.webrtc.org/2651743007/diff/320001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m (right): https://codereview.webrtc.org/2651743007/diff/320001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:21: @property(nonatomic) id<RTCMTLRenderer> renderer; nit: be consistent with doco of strong or not usually what I do is assume everything is strong since it is the default behavior (otherwise we have to annotate everything as strong, like the id<RTCMTLRenderer> above) https://codereview.webrtc.org/2651743007/diff/320001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:77: } if we know at end of configure whether or not it will work, it may be worth returning nil in the init methods if configure fails would also be helpful to log warning if metal is unavailable https://codereview.webrtc.org/2651743007/diff/320001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:78: #pragma mark - MTKViewDelegate methods nit: add blank line after pragma https://codereview.webrtc.org/2651743007/diff/320001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:82: } If this is an unexpected scenario, feel free to assert (maybe add a DCHECK equivalent somewhere. https://codereview.webrtc.org/2651743007/diff/320001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:96: if (frame == nil) { return or draw black? https://codereview.webrtc.org/2651743007/diff/320001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h (right): https://codereview.webrtc.org/2651743007/diff/320001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCMTLVideoView.h:26: nit: remove blank line?
https://codereview.webrtc.org/2651743007/diff/320001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLNV12Renderer.mm (right): https://codereview.webrtc.org/2651743007/diff/320001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLNV12Renderer.mm:90: float4 out = float4(y + 1.403 * uv.y, y - 0.344 * uv.x - 0.714 * uv.y, y + 1.770 * uv.x, 1.0); On 2017/02/22 00:22:27, tkchin_webrtc wrote: > nit: I'd doco source of these constants and which color space it's using Done. https://codereview.webrtc.org/2651743007/diff/320001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLNV12Renderer.mm:101: @interface RTCNV12Renderer () On 2017/02/22 00:22:27, tkchin_webrtc wrote: > doesn't seem to be a need for this decl Done. https://codereview.webrtc.org/2651743007/diff/320001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLNV12Renderer.mm:239: dispatch_semaphore_signal(block_semaphore); On 2017/02/22 00:22:27, tkchin_webrtc wrote: > can you doco what the semaphores accomplish? > from here what I gather is that we have to wait for the previous command to > finish first > > but then now we have to be careful because it will block the thread that we are > on. Not sure what thread drawInMTKView calls back on though. Indeed. We need to wait for the inflight (submitted on GPU) command buffer to finish before we proceed with new one. Ideally, the CPU would never have to wait on the GPU and the GPU should finish before the CPU is ready for scheduled for submission of new command buffer. https://codereview.webrtc.org/2651743007/diff/320001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLNV12Renderer.mm:272: @autoreleasepool { On 2017/02/22 00:22:27, tkchin_webrtc wrote: > why is autoreleasepool needed here? to drain the command buffers faster? Command buffers and most importantly drawables. "It is highly advisable to contain your rendering loop within an autorelease pool block to avoid possible deadlock situations with multiple drawables." https://codereview.webrtc.org/2651743007/diff/320001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m (right): https://codereview.webrtc.org/2651743007/diff/320001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:21: @property(nonatomic) id<RTCMTLRenderer> renderer; On 2017/02/22 00:22:27, tkchin_webrtc wrote: > nit: be consistent with doco of strong or not > > usually what I do is assume everything is strong since it is the default > behavior (otherwise we have to annotate everything as strong, like the > id<RTCMTLRenderer> above) Done. https://codereview.webrtc.org/2651743007/diff/320001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:56: return NO; On 2017/02/21 08:09:12, kthelgason wrote: > nit: I'd prefer the return NO to be within an else branch of the ifdef. > Otherwise the code might fail to compile with -Wunreachable-code. Done. https://codereview.webrtc.org/2651743007/diff/320001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:77: } On 2017/02/22 00:22:27, tkchin_webrtc wrote: > if we know at end of configure whether or not it will work, it may be worth > returning nil in the init methods if configure fails > would also be helpful to log warning if metal is unavailable UIView's declaration of init explicitly states nonnull is returned :/ Adding LogError for now. In future the setup would preferably be separated from the initializer. https://codereview.webrtc.org/2651743007/diff/320001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:78: #pragma mark - MTKViewDelegate methods On 2017/02/22 00:22:27, tkchin_webrtc wrote: > nit: add blank line after pragma Done. https://codereview.webrtc.org/2651743007/diff/320001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:82: } On 2017/02/22 00:22:27, tkchin_webrtc wrote: > If this is an unexpected scenario, feel free to assert (maybe add a DCHECK > equivalent somewhere. Done. https://codereview.webrtc.org/2651743007/diff/320001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Metal/RTCMTLVideoView.m:96: if (frame == nil) { On 2017/02/22 00:22:27, tkchin_webrtc wrote: > return or draw black? That functionality should not be included in the video view IMO. But I added a LogInfo.
The CQ bit was checked by denicija@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: android_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
The CQ bit was checked by denicija@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: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/21655)
The CQ bit was checked by denicija@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, kthelgason@webrtc.org, tkchin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2651743007/#ps340001 (title: "Minor fixes")
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: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/21658)
The CQ bit was checked by denicija@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: This issue passed the CQ dry run.
The CQ bit was checked by denicija@webrtc.org
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": 340001, "attempt_start_ts": 1487839420888660,
"parent_rev": "8c80c6e389d235d259a39e0114316447aa228092", "commit_rev":
"fc8c97f950a4fead4a5debe3c838942db1834010"}
Message was sent while issue was closed.
Description was changed from ========== Add metal view, shaders and renderer. This CL submits standalone Metal view, renderer and shader. BUG=webrtc:7079 ========== to ========== Add metal view, shaders and renderer. This CL submits standalone Metal view, renderer and shader. BUG=webrtc:7079 Review-Url: https://codereview.webrtc.org/2651743007 Cr-Commit-Position: refs/heads/master@{#16787} Committed: https://chromium.googlesource.com/external/webrtc/+/fc8c97f950a4fead4a5debe3c... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001) as https://chromium.googlesource.com/external/webrtc/+/fc8c97f950a4fead4a5debe3c...
Message was sent while issue was closed.
A revert of this CL (patchset #18 id:340001) has been created in https://codereview.webrtc.org/2711003004/ by denicija@webrtc.org. The reason for reverting is: Reverting due to breakage in the Google3 import.
Message was sent while issue was closed.
On 2017/02/23 09:14:50, denicija-webrtc wrote: > A revert of this CL (patchset #18 id:340001) has been created in > https://codereview.webrtc.org/2711003004/ by mailto:denicija@webrtc.org. > > The reason for reverting is: Reverting due to breakage in the Google3 import. So close! Looking forward to seeing this land. Do you have a link to the google3 breakage? I'm curious to know since the stuff I'm working on will almost certainly break it too. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
