Bug 87695

Created attachment 144449 [details] Patch

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

Created attachment 144452 [details] Patch

(In reply to comment #3) > Created an attachment (id=144452) [details] > Patch I would rather implement the blurs using two passes: vertical and horizontal. That way you minimize the number of texture accesses.

Created attachment 144611 [details] Patch

Comment on attachment 144611 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144611&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:220 > + int leftOutset, topOutset, bottomOutset, rightOutset; Not webkit style > Tools/qmake/mkspecs/features/unix/default_post.prf:1 > -# ------------------------------------------------------------------- > +# ------------------------------------------------------------------- Why?

(In reply to comment #6) > (From update of attachment 144611 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144611&action=review > > > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:220 > > + int leftOutset, topOutset, bottomOutset, rightOutset; > > Not webkit style Right... > > Tools/qmake/mkspecs/features/unix/default_post.prf:1 > > -# ------------------------------------------------------------------- > > +# ------------------------------------------------------------------- > > Why? Oops :)

Comment on attachment 144611 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144611&action=review Looks good. The single thing that I don't like is the duplicated code, but I don't see a nice way to optimize it either. > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:533 > + program->prepare(filter, pass, sourceTexture.contentSize(), static_cast<const BitmapTextureGL&>(contentTexture).id()); This cast doesn't look nice, but it seems like the whole file does that. > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:423 > + vec2 coord = v_texCoord + vec2(r * u_blurRadius, 0.); This line is the only difference between the two passes and there's a lot of duplicated code. I'm not sure how to make this look better though, but I think you should at least add a comment explaining that (makes the later maintenance a little easier). You could get away with just one shader if you had vec2 u_blurRadius and just multiplied r with it instead, but it might have slight performance impact. > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:430 > + vec4 dist = I suppose that a for loop would be unrolled at compile time anyway and would make the code look better. > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:452 > + float g = Isn't this one just a constant at the end? I suppose the compiler would just optimize it anyway. > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:591 > + sampleAlpha(0.) + Ditto about the for loop. > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:776 > + radius = blur.stdDeviation().percent(); There's a better way to treat this: floatValueForLength in css/LengthFunctions.h. Also, that's what FilterEffectRenderer.cpp is using. > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:812 > +PassRefPtr<StandardFilterProgram> TextureMapperShaderManager::getShaderForFilter(const FilterOperation& filter, int pass) I think there's no need to have "pass" as a signed int. unsigned should be sufficient. > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:830 > +int TextureMapperShaderManager::getPassesForFilter(const FilterOperation& operation) const int -> unsigned ?

Created attachment 145157 [details] Patch

Comment on attachment 145157 [details] Patch Attachment 145157 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12869390 New failing tests: http/tests/media/media-source/video-media-source-event-attributes.html

Created attachment 145209 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

(In reply to comment #10) > (From update of attachment 145157 [details]) > Attachment 145157 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/12869390 > > New failing tests: > http/tests/media/media-source/video-media-source-event-attributes.html This has to be flaky.

Comment on attachment 145157 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145157&action=review Maybe you could say a couple words about why the blur can be done in one pass, while the drop shadow (which is essentially a blur and a source-in composite of the shadow color) must be done in two passes. Other than that I have a few comments, which are mostly just nit-picky things. > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:563 > + m_textureMapper->drawFiltered((i || j) ? *source.get() : contentTexture, contentTexture, *filter, j); I think you can dereference a RefPtr directly, because it has an operator overload: (from RefPtr.h) T& operator*() const { return *m_ptr; } This seems to be the only caller of ::drawFiltered, so I'm surprised that ::drawFiltered doesn't just take pointers instead of having to dereference everything here. > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:222 > + int leftOutset, topOutset, bottomOutset, rightOutset; > + if (m_state.filters.hasOutsets()) { > + m_state.filters.getOutsets(topOutset, rightOutset, bottomOutset, leftOutset); Perhaps leftOutset, etc could be declared inside the conditional here? > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:233 > + Nit: extra newline here. > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:322 > + for (int i = 0; i < filters.size(); ++i) { > + switch (filters.operations().at(i)->getOperationType()) { > + case FilterOperation::DROP_SHADOW: > + return true; > + default: > + break; > + } > + } > + > + return false; Is this completely dependent on the number of passes? If so perhaps that could just check the number of passes. > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:508 > + { > + // Normal distribution formula, when the mean is 0 and the standard deviation is 1. > + return pow(e, -pow(v, 2.) / 2.) / (sqrt(2. * pi)); > + } > + > + lowp float sampleAlpha(float r) I think that 'v' and 'r' deserve to be full words here. > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:528 > + lowp vec4 sourceOver(lowp vec4 front, lowp vec4 back) Being most familiar with Cairo, the names 'source' and 'destination' seem more apt than front and back to me. > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:560 > + GLchar log[100]; > + GLint len; > glGetShaderInfoLog(fragmentShader, 100, &len, log); > + printf("%s\n", log); Perhaps it makes sense to use the WTF logging facilities for this? (Logging.h in platform)

(In reply to comment #13) > (From update of attachment 145157 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145157&action=review > > Maybe you could say a couple words about why the blur can be done in one pass, while the drop shadow (which is essentially a blur and a source-in composite of the shadow color) must be done in two passes. Other than that I have a few comments, which are mostly just nit-picky things. The blur is done in two passes, but uses the same shader for both. We need different shaders for the drop-shadow passes because in the second pass we also blend the content texture. > > > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:563 > > + m_textureMapper->drawFiltered((i || j) ? *source.get() : contentTexture, contentTexture, *filter, j); > > I think you can dereference a RefPtr directly, because it has an operator overload: > > (from RefPtr.h) > T& operator*() const { return *m_ptr; } > > This seems to be the only caller of ::drawFiltered, so I'm surprised that ::drawFiltered doesn't just take pointers instead of having to dereference everything here. > > > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:222 > > + int leftOutset, topOutset, bottomOutset, rightOutset; > > + if (m_state.filters.hasOutsets()) { > > + m_state.filters.getOutsets(topOutset, rightOutset, bottomOutset, leftOutset); > > Perhaps leftOutset, etc could be declared inside the conditional here? > > > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:233 > > + > > Nit: extra newline here. > > > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:322 > > + for (int i = 0; i < filters.size(); ++i) { > > + switch (filters.operations().at(i)->getOperationType()) { > > + case FilterOperation::DROP_SHADOW: > > + return true; > > + default: > > + break; > > + } > > + } > > + > > + return false; > > Is this completely dependent on the number of passes? If so perhaps that could just check the number of passes. No. It's very specific to the shader - blur has two passes, but doesn't need the content texture. Specifically in the drop-shadow shader we need to composite the original image on top of the blurred shadow. I'll take care of the nitpicks :)

Created attachment 146341 [details] Patch

Comment on attachment 146341 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146341&action=review Looks pretty good, r=me if you fix the comments. > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:221 > + int leftOutset, topOutset, bottomOutset, rightOutset; Not webkit style really > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:314 > + // The drop-shadow filter requires the contentn texture, because it needs to composite it spelling issue > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:505 > + Remove newline > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:530 > + lowp float gaussian(lowp float value) > + { > + // Normal distribution formula, when the mean is 0 and the standard deviation is 1. > + return pow(e, -pow(value, 2.) / 2.) / (sqrt(2. * pi)); > + } As this is defined in various places, would it be possible to share it somehow? > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:751 > +unsigned TextureMapperShaderManager::getPassesForFilter(const FilterOperation& operation) const getPassesRequiredForFilter?

\> > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:530 > > + lowp float gaussian(lowp float value) > > + { > > + // Normal distribution formula, when the mean is 0 and the standard deviation is 1. > > + return pow(e, -pow(value, 2.) / 2.) / (sqrt(2. * pi)); > > + } > > As this is defined in various places, would it be possible to share it somehow? I don't have a clean way of doing it at this point, I'll add FIXMEs.

Created attachment 146581 [details] Patch for landing

Comment on attachment 146581 [details] Patch for landing Rejecting attachment 146581 [details] from commit-queue. New failing tests: perf/object-keys.html Full output: http://queues.webkit.org/results/12907999

Created attachment 146610 [details] Archive of layout-test-results from ec2-cq-01 The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: ec2-cq-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 146581 [details] Patch for landing Flaky bot.

Comment on attachment 146581 [details] Patch for landing Clearing flags on attachment: 146581 Committed r119849: <http://trac.webkit.org/changeset/119849>

All reviewed patches have been landed. Closing bug.