Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1311093005:
Android GlRectDrawer: Add fragment shader for RGB(A) textures (Closed)
|
Created:
5 years, 3 months ago by magjed_webrtc Modified:
5 years, 3 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), Andrew MacDonald Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAndroid GlRectDrawer: Add fragment shader for RGB(A) textures
Add third shader type for RGB(A) and refactor according to the Rule of three.
BUG=webrtc:4742
R=hbos@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/ed4224fbdaca23a9ba593d07d6809a0943f73528
Patch Set 1 #
Total comments: 17
Patch Set 2 : addressing hbos@ comments #Messages
Total messages: 10 (2 generated)
magjed@webrtc.org changed reviewers: + tommi@webrtc.org
tommi - Please take a look.
https://codereview.webrtc.org/1311093005/diff/1/talk/app/webrtc/java/android/... File talk/app/webrtc/java/android/org/webrtc/GlRectDrawer.java (right): https://codereview.webrtc.org/1311093005/diff/1/talk/app/webrtc/java/android/... talk/app/webrtc/java/android/org/webrtc/GlRectDrawer.java:89: + "}\n"; I'm afraid I have no clue what I'm reviewing here :) well, I sort of know what it is, but I don't have the chops to review it unfortunately. Is there someone else that would be suitable?
magjed@webrtc.org changed reviewers: + hbos@webrtc.org
hbos - Please take a look. https://codereview.webrtc.org/1311093005/diff/1/talk/app/webrtc/java/android/... File talk/app/webrtc/java/android/org/webrtc/GlRectDrawer.java (right): https://codereview.webrtc.org/1311093005/diff/1/talk/app/webrtc/java/android/... talk/app/webrtc/java/android/org/webrtc/GlRectDrawer.java:89: + "}\n"; On 2015/09/01 17:06:19, tommi (webrtc) wrote: > I'm afraid I have no clue what I'm reviewing here :) well, I sort of know what > it is, but I don't have the chops to review it unfortunately. Is there someone > else that would be suitable? I don't know. hbos@ knows about this, but he is not an owner. I can also ask glaznev@, but I'm already spamming him with CL:s every day.
I can stamp it if hbos also takes a look
https://codereview.webrtc.org/1311093005/diff/1/talk/app/webrtc/java/android/... File talk/app/webrtc/java/android/org/webrtc/GlRectDrawer.java (right): https://codereview.webrtc.org/1311093005/diff/1/talk/app/webrtc/java/android/... talk/app/webrtc/java/android/org/webrtc/GlRectDrawer.java:52: "varying vec2 interp_tc;\n" nit: fix indent so that all code lines align (either extra space on first line or + on right side) https://codereview.webrtc.org/1311093005/diff/1/talk/app/webrtc/java/android/... talk/app/webrtc/java/android/org/webrtc/GlRectDrawer.java:64: "precision mediump float;\n" nit: indent https://codereview.webrtc.org/1311093005/diff/1/talk/app/webrtc/java/android/... talk/app/webrtc/java/android/org/webrtc/GlRectDrawer.java:82: "precision mediump float;\n" nit: indent https://codereview.webrtc.org/1311093005/diff/1/talk/app/webrtc/java/android/... talk/app/webrtc/java/android/org/webrtc/GlRectDrawer.java:145: GLES20.glBindTexture(GLES20.GL_TEXTURE_2D, 0); Same reason for unbinding as in drawOes? Add comment about it here too? https://codereview.webrtc.org/1311093005/diff/1/talk/app/webrtc/java/android/... talk/app/webrtc/java/android/org/webrtc/GlRectDrawer.java:160: drawRectangle(texMatrix); No need to unbind texture after draw here like in drawOes (and drawRgb)? I suppose the unbinding is a precaution that is only necessary when the textures used are updateTexImage() type of textures, and this is a kind of general "draw rectangle" class that shouldn't need to know about the type of textures. If I'm reasoning correctly it seems to me like we should either always take the precaution and (sometimes unnecessarily) unbind or we should make it an option that you control from the outside. https://codereview.webrtc.org/1311093005/diff/1/talk/app/webrtc/java/android/... talk/app/webrtc/java/android/org/webrtc/GlRectDrawer.java:184: GLES20.glUniform1i(shader.getUniformLocation("v_tex"), 2); You've forgot to do this for "rgb_tex" for RGB_FRAGMENT_SHADER_STRING. (It seems to default to 0 and work anyway, but is this mandated by the GL specs that it will default that way?) Set rgb_tex to 0 in the RGB case. https://codereview.webrtc.org/1311093005/diff/1/talk/app/webrtc/java/android/... talk/app/webrtc/java/android/org/webrtc/GlRectDrawer.java:205: public void release() { This will never be called and you know it ;D (i kid)
hbos - Please take another look. https://codereview.webrtc.org/1311093005/diff/1/talk/app/webrtc/java/android/... File talk/app/webrtc/java/android/org/webrtc/GlRectDrawer.java (right): https://codereview.webrtc.org/1311093005/diff/1/talk/app/webrtc/java/android/... talk/app/webrtc/java/android/org/webrtc/GlRectDrawer.java:52: "varying vec2 interp_tc;\n" On 2015/09/02 08:01:28, hbos wrote: > nit: fix indent so that all code lines align (either extra space on first line > or + on right side) Done, I managed to follow the hbos@ style guide without violating the official style guide. FYI: 4.5.1 Where to break 1. When a line is broken at a non-assignment operator the break comes before the symbol. 4.6.3 Horizontal alignment: never required Terminology Note: Horizontal alignment is the practice of adding a variable number of additional spaces in your code with the goal of making certain tokens appear directly below certain other tokens on previous lines. This practice is permitted, but is never required by Google Style. It is not even required to maintain horizontal alignment in places where it was already used. https://codereview.webrtc.org/1311093005/diff/1/talk/app/webrtc/java/android/... talk/app/webrtc/java/android/org/webrtc/GlRectDrawer.java:160: drawRectangle(texMatrix); On 2015/09/02 08:01:28, hbos wrote: > No need to unbind texture after draw here like in drawOes (and drawRgb)? > > I suppose the unbinding is a precaution that is only necessary when the textures > used are updateTexImage() type of textures, and this is a kind of general "draw > rectangle" class that shouldn't need to know about the type of textures. > If I'm reasoning correctly it seems to me like we should either always take the > precaution and (sometimes unnecessarily) unbind or we should make it an option > that you control from the outside. Only OES textures can be used in updateTexImage(), but maybe it is necessary to bind/unbind whenever we use the textures from different threads. I changed so we always unbind as a precaution, given how many hours we spent debugging the OES texture problem. I think it's a negligible performance difference anyway. https://codereview.webrtc.org/1311093005/diff/1/talk/app/webrtc/java/android/... talk/app/webrtc/java/android/org/webrtc/GlRectDrawer.java:184: GLES20.glUniform1i(shader.getUniformLocation("v_tex"), 2); On 2015/09/02 08:01:28, hbos wrote: > You've forgot to do this for "rgb_tex" for RGB_FRAGMENT_SHADER_STRING. (It seems > to default to 0 and work anyway, but is this mandated by the GL specs that it > will default that way?) Set rgb_tex to 0 in the RGB case. Yes, it is mandated by the GL specs that all uniforms without initializers are assigned a default value of 0. But maybe it's more clear to do it explicitly. I added it for oes_tex as well. https://codereview.webrtc.org/1311093005/diff/1/talk/app/webrtc/java/android/... talk/app/webrtc/java/android/org/webrtc/GlRectDrawer.java:205: public void release() { On 2015/09/02 08:01:28, hbos wrote: > This will never be called and you know it ;D (i kid) True, I can remove this in a follow-up CL.
lgtm https://codereview.webrtc.org/1311093005/diff/1/talk/app/webrtc/java/android/... File talk/app/webrtc/java/android/org/webrtc/GlRectDrawer.java (right): https://codereview.webrtc.org/1311093005/diff/1/talk/app/webrtc/java/android/... talk/app/webrtc/java/android/org/webrtc/GlRectDrawer.java:52: "varying vec2 interp_tc;\n" On 2015/09/02 09:02:24, magjed_webrtc wrote: > On 2015/09/02 08:01:28, hbos wrote: > > nit: fix indent so that all code lines align (either extra space on first line > > or + on right side) > > Done, I managed to follow the hbos@ style guide without violating the official > style guide. FYI: > 4.5.1 Where to break > 1. When a line is broken at a non-assignment operator the break comes before the > symbol. > > 4.6.3 Horizontal alignment: never required > Terminology Note: Horizontal alignment is the practice of adding a variable > number of additional spaces in your code with the goal of making certain tokens > appear directly below certain other tokens on previous lines. > > This practice is permitted, but is never required by Google Style. It is not > even required to maintain horizontal alignment in places where it was already > used. hbos@ style guide best style guide. https://codereview.webrtc.org/1311093005/diff/1/talk/app/webrtc/java/android/... talk/app/webrtc/java/android/org/webrtc/GlRectDrawer.java:160: drawRectangle(texMatrix); On 2015/09/02 09:02:24, magjed_webrtc wrote: > On 2015/09/02 08:01:28, hbos wrote: > > No need to unbind texture after draw here like in drawOes (and drawRgb)? > > > > I suppose the unbinding is a precaution that is only necessary when the > textures > > used are updateTexImage() type of textures, and this is a kind of general > "draw > > rectangle" class that shouldn't need to know about the type of textures. > > If I'm reasoning correctly it seems to me like we should either always take > the > > precaution and (sometimes unnecessarily) unbind or we should make it an option > > that you control from the outside. > > Only OES textures can be used in updateTexImage(), but maybe it is necessary to > bind/unbind whenever we use the textures from different threads. I changed so we > always unbind as a precaution, given how many hours we spent debugging the OES > texture problem. I think it's a negligible performance difference anyway. Acknowledged. https://codereview.webrtc.org/1311093005/diff/1/talk/app/webrtc/java/android/... talk/app/webrtc/java/android/org/webrtc/GlRectDrawer.java:184: GLES20.glUniform1i(shader.getUniformLocation("v_tex"), 2); On 2015/09/02 09:02:24, magjed_webrtc wrote: > On 2015/09/02 08:01:28, hbos wrote: > > You've forgot to do this for "rgb_tex" for RGB_FRAGMENT_SHADER_STRING. (It > seems > > to default to 0 and work anyway, but is this mandated by the GL specs that it > > will default that way?) Set rgb_tex to 0 in the RGB case. > > Yes, it is mandated by the GL specs that all uniforms without initializers are > assigned a default value of 0. But maybe it's more clear to do it explicitly. I > added it for oes_tex as well. Acknowledged. https://codereview.webrtc.org/1311093005/diff/1/talk/app/webrtc/java/android/... talk/app/webrtc/java/android/org/webrtc/GlRectDrawer.java:205: public void release() { On 2015/09/02 09:02:24, magjed_webrtc wrote: > On 2015/09/02 08:01:28, hbos wrote: > > This will never be called and you know it ;D (i kid) > > True, I can remove this in a follow-up CL. sgtm
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as ed4224fbdaca23a9ba593d07d6809a0943f73528 (presubmit successful). |
