Bug 98989

Created attachment 168138 [details] Patch

Alex + Max may have comments here.

(In reply to comment #2) > Alex + Max may have comments here. Thanks. I'm looking forward!

Comment on attachment 168138 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168138&action=review Looks good! Thanks for the patch. > Source/WebCore/ChangeLog:16 > + CustomFilterGlobalContext and CustomFilterOperation, and it is possible if we I would remove the dependency on the CustomFilterOperation. Most probably the platform code has no access to it. Anyway, FECustomFilter had no dependency on that, so please remove it. > Source/WebCore/ChangeLog:21 > + No new tests. Covered by css1/filters/custom nit: typo css1 -> css3 > Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:55 > +PassRefPtr<CustomFilterRenderer> CustomFilterRenderer::create(CustomFilterGlobalContext* customFilterGlobalContext, CustomFilterOperation* operation) This layer should not use the CustomFilterOperation directly. I would rather keep the same constructor as the FECustomFilter had before. > Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:148 > +bool CustomFilterRenderer::render(Uint8ClampedArray* source, Uint8ClampedArray* destination, const IntSize& size) I think this one could have a better name to explain that it would read back the pixels. I suppose at some point you will add one to execute only on textures, so that the Accelerated Pipeline doesn't read back. > Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:474 > +static void orthogonalProjectionMatrix(TransformationMatrix& matrix, float left, float right, float bottom, float top) > +{ > + ASSERT(matrix.isIdentity()); > + I think static methods would better be placed at the beginning of the file. > Source/WebCore/platform/graphics/filters/CustomFilterRenderer.h:67 > + enum CustomFilterDrawType { > + NEEDS_INPUT_TEXTURE, > + NO_INPUT_TEXTURE I know you just copy-pasted this one, but it seems like NO_INPUT_TEXTURE is never used. Can you please remove CustomFilterDrawType type? I think it was intended to help implementing a renderer that has the input already as a texture instead of just some bytes in the memory. For the accelerated compositor you will need a different render method that will take a source texture ID and a destination texture ID, so maybe we could add it back in a different form at that time. > Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:44 > +FECustomFilter::FECustomFilter(Filter* filter, PassRefPtr<CustomFilterRenderer> renderer) I think FECustomFilter should create its own CustomFilterRenderer and not be passed one. CustomFilterRenderer would then be just an implementation detail.

(In reply to comment #4) > (From update of attachment 168138 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=168138&action=review Thank you for your review! > > Source/WebCore/ChangeLog:16 > > + CustomFilterGlobalContext and CustomFilterOperation, and it is possible if we > > I would remove the dependency on the CustomFilterOperation. Most probably the platform code has no access to it. Anyway, FECustomFilter had no dependency on that, so please remove it. Ok. I will make FECustomFilter have no dependency on CustomFilterOperation. I think it is better for FECustomFilter to not know CustomFilterOperation. IMHO, each GraphicsLayer uses FilterOperation directly. Moreover, TextureMapperLayer and TextureMaperGL heavily use FilterOperation, too. > > Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:148 > > +bool CustomFilterRenderer::render(Uint8ClampedArray* source, Uint8ClampedArray* destination, const IntSize& size) > > I think this one could have a better name to explain that it would read back the pixels. I suppose at some point you will add one to execute only on textures, so that the Accelerated Pipeline doesn't read back. You're right. The name is poor. I'll more refactor CustomFilterRenderer for the Accelerated Pipeline. > > Source/WebCore/platform/graphics/filters/CustomFilterRenderer.h:67 > > + enum CustomFilterDrawType { > > + NEEDS_INPUT_TEXTURE, > > + NO_INPUT_TEXTURE > > I know you just copy-pasted this one, but it seems like NO_INPUT_TEXTURE is never used. Can you please remove CustomFilterDrawType type? I think it was intended to help implementing a renderer that has the input already as a texture instead of just some bytes in the memory. Yes, I can and I want :) I want to file and post another bug removing CustomFilterDrawType which this bug depends on. I have a question. There is the CustomFilterProgramType enumeration in CustomFilterProgramInfo.h. Is it also useless? enum CustomFilterProgramType { PROGRAM_TYPE_NO_ELEMENT_TEXTURE, PROGRAM_TYPE_BLENDS_ELEMENT_TEXTURE }; CustomFilterProgramType is set only in StyleResolver. PassRefPtr<CustomFilterOperation> StyleResolver::createCustomFilterOperation(WebKitCSSFilterValue* filterValue) { ... CustomFilterProgramType programType = PROGRAM_TYPE_NO_ELEMENT_TEXTURE; ... if (fragmentShaderOrMixFunction->isWebKitCSSMixFunctionValue()) { programType = PROGRAM_TYPE_BLENDS_ELEMENT_TEXTURE; ... } I don't understand why only mix(<uri> ..) needs the DOM element texture. I think just <uri> fragmentShader also needs the DOM element texture. http://dvcs.w3.org/hg/FXTF/raw-file/tip/filters/index.html#fragmentShader-attribute Could I remove CustomFilterProgramType also?

(In reply to comment #5) > > I know you just copy-pasted this one, but it seems like NO_INPUT_TEXTURE is never used. Can you please remove CustomFilterDrawType type? I think it was intended to help implementing a renderer that has the input already as a texture instead of just some bytes in the memory. > I don't understand why only mix(<uri> ..) needs the DOM element texture. I think just <uri> fragmentShader also needs the DOM element texture. > http://dvcs.w3.org/hg/FXTF/raw-file/tip/filters/index.html#fragmentShader-attribute > > Could I remove CustomFilterProgramType also? I understand my old question is wrong after reading spec: "38.2.1. Fragment Shaders Differences with GLSL." http://dvcs.w3.org/hg/FXTF/raw-file/tip/filters/index.html#fragment-shaders-differences-with-glsl I understand <uri> fragmentShader can not use the DOM element texture. <uri> fragmentShader can just render a simple shader like "gl_FragColor = vec4(1.0, 0.0, 0.0, 1.0);". Am I right? As you mentioned, we just need to remove CustomFilterDrawType. I'll do that.

Created attachment 169381 [details] Patch

(In reply to comment #4) > (From update of attachment 168138 [details]) > I would remove the dependency on the CustomFilterOperation. Most probably the platform code has no access to it. Anyway, FECustomFilter had no dependency on that, so please remove it. Done. > nit: typo css1 -> css3 Done. > This layer should not use the CustomFilterOperation directly. I would rather keep the same constructor as the FECustomFilter had before. Done. > > Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:148 > > +bool CustomFilterRenderer::render(Uint8ClampedArray* source, Uint8ClampedArray* destination, const IntSize& size) > > I think this one could have a better name to explain that it would read back the pixels. I suppose at some point you will add one to execute only on textures, so that the Accelerated Pipeline doesn't read back. The previous patch had made FECustomFilter delegate uploading texture, rendering, managing fbo and reading back pixel to CustomFilterRenderer. However, this patch only delegates rendering css shaders to CustomFilterRenderer because Accelerated Compositing needs CustomFilterRenderer only to render css shaders. FECustomFilter has jobs: uploading texture, managing fbo and reading back pixel as it has done. CustomFilterRenderer::paint will be used by both FECustomFilter and Accelerated Compositing. > I know you just copy-pasted this one, but it seems like NO_INPUT_TEXTURE is never used. Can you please remove CustomFilterDrawType type? I think it was intended to help implementing a renderer that has the input already as a texture instead of just some bytes in the memory. > > For the accelerated compositor you will need a different render method that will take a source texture ID and a destination texture ID, so maybe we could add it back in a different form at that time. Done. > I think FECustomFilter should create its own CustomFilterRenderer and not be passed one. CustomFilterRenderer would then be just an implementation detail. Done.

I enabled CSS Shaders on Texture Mapper in Bug 98990. It is the first enabling CSS Shaders on Accelerated Compositing.

Comment on attachment 169381 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169381&action=review Thanks! Looks good, I like the new approach where the CustomFilterRenderer is not messing around with textures anymore. I have a few minor comments. Going this way, it looks like CustomFilterRenderer could be really decoupled from the Global Context and that would simplify a bunch of the other checks. > Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:76 > +PassRefPtr<CustomFilterRenderer> CustomFilterRenderer::create(CustomFilterGlobalContext* customFilterGlobalContext, const CustomFilterProgramInfo& programInfo, const CustomFilterParameterList& parameters, > + unsigned meshRows, unsigned meshColumns, CustomFilterOperation::MeshBoxType meshBoxType, CustomFilterOperation::MeshType meshType) > +{ I would remove the need to pass the customFIlterGlobalContext at this point. It looks like you only need the context and the validated program. Let's make this class as simple as possible, so that it can be embedded anywhere. > Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:116 > + m_contextSize = size; Can you add a fixme and a new bug to remove m_contextSize from the CustomFilterRenderer? I think we would need something like CustomFilterRendererState that will contain the size and maybe other parameters in the future. We should pass that to bindProgramBuffers instead of storing it. > Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:145 > + // FIXME: Sharing the mesh would just save the time needed to upload it to the GPU, so I assume we could > + // benchmark that for performance. > + // https://bugs.webkit.org/show_bug.cgi?id=88429 > + m_mesh = CustomFilterMesh::create(m_context.get(), m_meshColumns, m_meshRows, FloatRect(0, 0, 1, 1), m_meshType); Move this to a separate method called initializeMeshIfNeeded(), so that it is not guarded by the first return. It should not be an issue, but looks weird to have it in a method called initializeCompiledProgram. Also, make the call after checking that the compiled program is initialized in prepareForDrawing. > Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:68 > + RefPtr<FECustomFilter> feCustomFilter = adoptRef(new FECustomFilter(filter, customFilterGlobalContext, programInfo, parameters, meshRows, meshColumns, meshBoxType, meshType)); It looks like you could remove the dependency on customFilterGlobalContext and just pass in the validated program + the GraphicsContext3D directly. That will simplify a bunch of the checks we do here. > Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:72 > + ASSERT(feCustomFilter->m_context.get()); Right now the context is never null, but it might be in the future if shaders are disabled after a GPU reset. > Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:-122 > - if (!m_context) At this point there is no guarantee that the m_context is not 0. I think you should put this back in.

Created attachment 170116 [details] Patch

Comment on attachment 169381 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169381&action=review >> Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:76 >> +{ > > I would remove the need to pass the customFIlterGlobalContext at this point. It looks like you only need the context and the validated program. Let's make this class as simple as possible, so that it can be embedded anywhere. Thanks for your careful review! Done. >> Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:116 >> + m_contextSize = size; > > Can you add a fixme and a new bug to remove m_contextSize from the CustomFilterRenderer? I think we would need something like CustomFilterRendererState that will contain the size and maybe other parameters in the future. We should pass that to bindProgramBuffers instead of storing it. Done. I filed Bug 100107. >> Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:145 >> + m_mesh = CustomFilterMesh::create(m_context.get(), m_meshColumns, m_meshRows, FloatRect(0, 0, 1, 1), m_meshType); > > Move this to a separate method called initializeMeshIfNeeded(), so that it is not guarded by the first return. It should not be an issue, but looks weird to have it in a method called initializeCompiledProgram. > > Also, make the call after checking that the compiled program is initialized in prepareForDrawing. Done. >> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:68 >> + RefPtr<FECustomFilter> feCustomFilter = adoptRef(new FECustomFilter(filter, customFilterGlobalContext, programInfo, parameters, meshRows, meshColumns, meshBoxType, meshType)); > > It looks like you could remove the dependency on customFilterGlobalContext and just pass in the validated program + the GraphicsContext3D directly. That will simplify a bunch of the checks we do here. Done. You are right. The bunch of the checks are only in createCustomFilterEffect now. >> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:72 >> + ASSERT(feCustomFilter->m_context.get()); > > Right now the context is never null, but it might be in the future if shaders are disabled after a GPU reset. Done. >> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:-122 >> - if (!m_context) > > At this point there is no guarantee that the m_context is not 0. I think you should put this back in. I put ASSERT(m_context) because we have guarantee that the m_context is not 0. We set m_context in the constructor of FECustomFilter. createCustomFilterEffect in FilterEffectRenderer.cpp dose not create FECustomFilter if the m_context is 0. Moreover, many methods of FECustomFilter already use the m_context without the checks. IMHO if we put this back in, other folks will be confused when reading here.

Comment on attachment 170116 [details] Patch Attachment 170116 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14486902

Created attachment 170234 [details] Patch

(In reply to comment #14) > Created an attachment (id=170234) [details] > Patch I fixed build fail on mac due to CustomFilterRenderer.cpp:79:173: error: unused parameter 'programInfo' [-Werror,-Wunused-parameter]

Comment on attachment 170234 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170234&action=review > Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:79 > +CustomFilterRenderer::CustomFilterRenderer(PassRefPtr<GraphicsContext3D> context, PassRefPtr<CustomFilterValidatedProgram> validatedProgram, const CustomFilterProgramInfo&, const CustomFilterParameterList& parameters, Do we need to pass in CustomFilterProgramInfo separately? Can ask for validatedProgram->programInfo() instead? > Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:97 > + return m_validatedProgram->programInfo().programType() == PROGRAM_TYPE_BLENDS_ELEMENT_TEXTURE; Like this. > Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:47 > +FECustomFilter::FECustomFilter(Filter* filter, PassRefPtr<GraphicsContext3D> context, PassRefPtr<CustomFilterValidatedProgram> validatedProgram, const CustomFilterProgramInfo& programInfo, const CustomFilterParameterList& parameters, Ditto regarding CustomFilterProgramInfo.

(In reply to comment #16) > (From update of attachment 170234 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=170234&action=review > > > Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:79 > > +CustomFilterRenderer::CustomFilterRenderer(PassRefPtr<GraphicsContext3D> context, PassRefPtr<CustomFilterValidatedProgram> validatedProgram, const CustomFilterProgramInfo&, const CustomFilterParameterList& parameters, > > Do we need to pass in CustomFilterProgramInfo separately? Can ask for validatedProgram->programInfo() instead? Oops, I was not careful. We don't need CustomFilterProgramInfo in CustomFilterRenderer. Thank you. I'll update.

Created attachment 170249 [details] Patch

(In reply to comment #17) > (In reply to comment #16) > > (From update of attachment 170234 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=170234&action=review > > > > > Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:79 > > > +CustomFilterRenderer::CustomFilterRenderer(PassRefPtr<GraphicsContext3D> context, PassRefPtr<CustomFilterValidatedProgram> validatedProgram, const CustomFilterProgramInfo&, const CustomFilterParameterList& parameters, > > > > Do we need to pass in CustomFilterProgramInfo separately? Can ask for validatedProgram->programInfo() instead? > > Oops, I was not careful. We don't need CustomFilterProgramInfo in CustomFilterRenderer. Thank you. I'll update. I removed CustomFilterProgramInfo in CustomFilterRenderer. Thank Max. In addition, I removed unused two headers in CustomFilterRenderer.

(In reply to comment #19) > I removed CustomFilterProgramInfo in CustomFilterRenderer. Thank Max. > In addition, I removed unused two headers in CustomFilterRenderer. Nice. Thanks Huang :)

(In reply to comment #20) > (In reply to comment #19) > > I removed CustomFilterProgramInfo in CustomFilterRenderer. Thank Max. > > In addition, I removed unused two headers in CustomFilterRenderer. > > Nice. Thanks Huang :) Thank you, too! :) I'll ask dino to review.

Comment on attachment 170249 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170249&action=review This looks great. Just two very minor nits. Sorry this means another submission - you'll hopefully be getting commit access very soon :) > Source/WebCore/ChangeLog:10 > + rendering css shaders to CustomFilterRenderer. Super Nit "css" -> "CSS" > Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp:196 > + // The viewport is a box with the size of 1 unit, so we are scalling up here to make sure that translations happen using real pixel Typo "scaling"

Created attachment 171422 [details] Patch

(In reply to comment #22) > (From update of attachment 170249 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=170249&action=review > > This looks great. Just two very minor nits. Sorry this means another submission - you'll hopefully be getting commit access very soon :) Thanks for your review! I hope so :) > Super Nit "css" -> "CSS" > Typo "scaling" Done. I rename CustomFilterRenderer::draw() from CustomFilterRenderer::paint() because Chiculita seems to prefer draw to paint. right? :)

(In reply to comment #24) > (In reply to comment #22) > > (From update of attachment 170249 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=170249&action=review > I rename CustomFilterRenderer::draw() from CustomFilterRenderer::paint() because Chiculita seems to prefer draw to paint. right? :) Hm, I don't have a preference, but looking at the code we've used "draw" a lot in this class. Maybe we could change that to "paint" at a later time in order to be more consistent with the rest of WebKit. But that's a different patch. I can set you the commit-queue flag if needed.

(In reply to comment #25) > (In reply to comment #24) > > (In reply to comment #22) > Hm, I don't have a preference, but looking at the code we've used "draw" a lot in this class. Maybe we could change that to "paint" at a later time in order to be more consistent with the rest of WebKit. But that's a different patch. Hi, alex. I just recalled CCCustomFilterRenderer::draw(). > I can set you the commit-queue flag if needed. If you do, I'll be happy!

Comment on attachment 171422 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171422&action=review > Source/WebCore/ChangeLog:36 > + (WebCore::CustomFilterRenderer::paint): This is wrong now :( Alex, could you land this manually (sorry, I can't do it right now). No need to go through another review.

Comment on attachment 171422 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171422&action=review >> Source/WebCore/ChangeLog:36 >> + (WebCore::CustomFilterRenderer::paint): > > This is wrong now :( Alex, could you land this manually (sorry, I can't do it right now). No need to go through another review. Oops. Sorry. Thanks Alex in advance.

(In reply to comment #28) > (From update of attachment 171422 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171422&action=review > > >> Source/WebCore/ChangeLog:36 > >> + (WebCore::CustomFilterRenderer::paint): > > > > This is wrong now :( Alex, could you land this manually (sorry, I can't do it right now). No need to go through another review. > > Oops. Sorry. Thanks Alex in advance. I'm rebasing and landing it manually. Thanks for the patch!

Landed manually in http://trac.webkit.org/changeset/132920.

Thank alex, dino and max for review and commit. I'm happy because of contributing to an awesome feature, which is CSS Shaders. :)

(In reply to comment #31) > Thank alex, dino and max for review and commit. I'm happy because of contributing to an awesome feature, which is CSS Shaders. :) You're very welcome. Keep the patches coming :)