Bug 93471

<rdar://problem/12057066>

+1 reproducible: http://stackoverflow.com/questions/13347947/css-filter-on-retina-display-fuzzy-images

*** Bug 106241 has been marked as a duplicate of this bug. ***

Per the duplicate, this happens even when there are now images at all.

s/now/no/.

*** Bug 106331 has been marked as a duplicate of this bug. ***

This is similar to the issue I fixed for -webkit-canvas. I suspect filters are using ImageBuffer::copyImage and need to pass the Unscaled parameter. See: https://bugs.webkit.org/show_bug.cgi?id=100611 Maybe Unscaled should be the default to prevent this in the future. And the limited clients that need Scaled should pass it instead.

I don’t think it’s copy that’s the issue. The following functions allocate buffers, always with a hardcoded device scale factor of 1: BitmapTexture::updateContents BitmapTextureImageBuffer::didReset CanvasRenderingContext2D::createCompositingBuffer CrossfadeGeneratedImage::drawPattern FilterEffect::asImageBuffer FilterEffect::createImageBufferResult FilterEffect::openCLImageToImageBuffer FilterEffectRenderer::allocateBackingStoreIfNeeded ImageBuffer::putByteArray SVGImage::drawPatternForContainer SVGRenderingContext::createImageBuffer SVGRenderingContext::createImageBufferForPattern ScratchBuffer::getScratchBuffer WebGLRenderingContext::LRUImageBufferCache::imageBuffer At least some of these functions need to pass the device scale factor, not a factor of 1.

Marking this as duplicate of bug 112717. This bug has more information what is going on and how to fix this issue. *** This bug has been marked as a duplicate of bug 112717 ***

Unduping, since this is a different issue.

Created attachment 207796 [details] Patch

This patch fixes all the shorthand filters, as well as combinations of shorthands. It also allows "referenced" filters (SVG-style filters accessed via url()) to operate at retina scale, but with some limitations: - <feDropShadow> <feGaussianBlur> <feColorMatrix> <feComponentTransfer> should all work, since this patch updates their behaviour (possibly indirectly through ImageBuffer or FilterEffect). - Other operations might revert back to 1x or produce incorrect results (say they are iterating over pixels but do not expect to see 4x as many). I've filed https://bugs.webkit.org/show_bug.cgi?id=119300 to cover cleanup.

Comment on attachment 207796 [details] Patch Attachment 207796 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/1289616

Comment on attachment 207796 [details] Patch Attachment 207796 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1273018 New failing tests: fast/hidpi/filters-shadow.html

Created attachment 207822 [details] Archive of layout-test-results from webkit-ews-04 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-04 Port: mac-mountainlion Platform: Mac OS X 10.8.4

Comment on attachment 207796 [details] Patch Attachment 207796 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1276013 New failing tests: fast/hidpi/filters-invert.html fast/hidpi/filters-blur.html fast/hidpi/filters-shadow.html fast/hidpi/filters-hue-rotate.html

Created attachment 207842 [details] Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.4

(In reply to comment #13) > (From update of attachment 207796 [details]) > Attachment 207796 [details] did not pass efl-wk2-ews (efl-wk2): > Output: http://webkit-queues.appspot.com/results/1289616 Internal compiler error!?

(In reply to comment #16) > (From update of attachment 207796 [details]) > Attachment 207796 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.appspot.com/results/1276013 > > New failing tests: > fast/hidpi/filters-invert.html > fast/hidpi/filters-blur.html > fast/hidpi/filters-shadow.html > fast/hidpi/filters-hue-rotate.html Not sure why these are failing since they are the tests I added for the patch.

Created attachment 208310 [details] Patch

Retrying.

Comment on attachment 208310 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208310&action=review > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:298 > calculateKernelSize(filter(), kernelSizeX, kernelSizeY, m_stdX, m_stdY); > + kernelSizeX *= filter()->filterScale(); > + kernelSizeY *= filter()->filterScale(); Shouldn't that be done in calculateKernelSize (which asks Filter or SVGFilter)? > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:301 > + paintSize.scale(filter()->filterScale()); There is an extra calculation step before entering platformApplySoftware() that calculates absolutePaintRect. The code should go there.

(In reply to comment #22) > (From update of attachment 208310 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208310&action=review > > > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:298 > > calculateKernelSize(filter(), kernelSizeX, kernelSizeY, m_stdX, m_stdY); > > + kernelSizeX *= filter()->filterScale(); > > + kernelSizeY *= filter()->filterScale(); > > Shouldn't that be done in calculateKernelSize (which asks Filter or SVGFilter)? > > > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:301 > > + paintSize.scale(filter()->filterScale()); > > There is an extra calculation step before entering platformApplySoftware() that calculates absolutePaintRect. The code should go there. Looking at the code, I think applyHorizontalScale() and VerticalScale() in Filter.h should do the adaption to retina. Just take you retina scale level into account when calculating the value. IIRC, the kernel and the image size should be scaled up automatically. Should actually be a straight forward patch.

(In reply to comment #23) > (In reply to comment #22) > > (From update of attachment 208310 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=208310&action=review > > > > > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:298 > > > calculateKernelSize(filter(), kernelSizeX, kernelSizeY, m_stdX, m_stdY); > > > + kernelSizeX *= filter()->filterScale(); > > > + kernelSizeY *= filter()->filterScale(); > > > > Shouldn't that be done in calculateKernelSize (which asks Filter or SVGFilter)? > > > > > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:301 > > > + paintSize.scale(filter()->filterScale()); > > > > There is an extra calculation step before entering platformApplySoftware() that calculates absolutePaintRect. The code should go there. > > Looking at the code, I think applyHorizontalScale() and VerticalScale() in Filter.h should do the adaption to retina. Just take you retina scale level into account when calculating the value. IIRC, the kernel and the image size should be scaled up automatically. Should actually be a straight forward patch. It's not, unfortunately. There seems to be little consistency throughout the entire filter code as to the way scaling and resolution are used. I'm not looking forward to the more complicated filters like convolution. That said, I might be able to change applyH/VScale - I'll check. IIRC I tried to do this first and there were a lot of side effects.

Comment on attachment 208310 [details] Patch Attachment 208310 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1365229 New failing tests: fast/hidpi/filters-blur.html

Created attachment 208336 [details] Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.4

(In reply to comment #24) > (In reply to comment #23) > > (In reply to comment #22) > > > (From update of attachment 208310 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=208310&action=review > > > > > > > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:298 > > > > calculateKernelSize(filter(), kernelSizeX, kernelSizeY, m_stdX, m_stdY); > > > > + kernelSizeX *= filter()->filterScale(); > > > > + kernelSizeY *= filter()->filterScale(); > > > > > > Shouldn't that be done in calculateKernelSize (which asks Filter or SVGFilter)? > > > > > > > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:301 > > > > + paintSize.scale(filter()->filterScale()); > > > > > > There is an extra calculation step before entering platformApplySoftware() that calculates absolutePaintRect. The code should go there. > > > > Looking at the code, I think applyHorizontalScale() and VerticalScale() in Filter.h should do the adaption to retina. Just take you retina scale level into account when calculating the value. IIRC, the kernel and the image size should be scaled up automatically. Should actually be a straight forward patch. > > It's not, unfortunately. There seems to be little consistency throughout the entire filter code as to the way scaling and resolution are used. I'm not looking forward to the more complicated filters like convolution. I am not talking about convolution. I think we need to revisit the implementation of convolution anyway. A lot of people worked on the code and it looks a lot messier than it used to be. I implemened applyH/VScale exactly for the use case of scaling the filter resolution. If that doesn't work, then this code is broken and needs to be fixe. Please don't make the code even more messier by doing scaling in a lot of different places, when it just should be done in one place. > > That said, I might be able to change applyH/VScale - I'll check. IIRC I tried to do this first and there were a lot of side effects. Please comment in which problems you run.

> > > Looking at the code, I think applyHorizontalScale() and VerticalScale() in Filter.h should do the adaption to retina. Just take you retina scale level into account when calculating the value. IIRC, the kernel and the image size should be scaled up automatically. Should actually be a straight forward patch. > > > > It's not, unfortunately. There seems to be little consistency throughout the entire filter code as to the way scaling and resolution are used. I'm not looking forward to the more complicated filters like convolution. > > I am not talking about convolution. I think we need to revisit the implementation of convolution anyway. A lot of people worked on the code and it looks a lot messier than it used to be. I implemened applyH/VScale exactly for the use case of scaling the filter resolution. If that doesn't work, then this code is broken and needs to be fixe. Please don't make the code even more messier by doing scaling in a lot of different places, when it just should be done in one place. Here's an example of what I mean: drop shadow uses applyH/VScale for both calculating the kernel size and offsetting the shadow. If we apply the filterScale (or resolution if you call it that), one or the other will break because they use different units. The kernel is applying to pixels, the offset is CSS. Another is displacement map. It (indirectly) uses applyH/VScale for color scaling as well as distance. I haven't verified that this will produce incorrect results yet. Maybe I'm misunderstanding the intent of things like absolutePaintRect - is it supposed to be scaled or not? You're suggesting I should do the scale before platformApplySoftware. I guess this means you think that as far as each individual filter is concerned it is always working on actual pixels, and thus applyH/VScale should be simply converting filter units into pixels. Is this right?

The layout test failures are strange. It looks like maybe i need to force a reload after changing the page scale.

(In reply to comment #28) > > > > Looking at the code, I think applyHorizontalScale() and VerticalScale() in Filter.h should do the adaption to retina. Just take you retina scale level into account when calculating the value. IIRC, the kernel and the image size should be scaled up automatically. Should actually be a straight forward patch. > > > > > > It's not, unfortunately. There seems to be little consistency throughout the entire filter code as to the way scaling and resolution are used. I'm not looking forward to the more complicated filters like convolution. > > > > I am not talking about convolution. I think we need to revisit the implementation of convolution anyway. A lot of people worked on the code and it looks a lot messier than it used to be. I implemened applyH/VScale exactly for the use case of scaling the filter resolution. If that doesn't work, then this code is broken and needs to be fixe. Please don't make the code even more messier by doing scaling in a lot of different places, when it just should be done in one place. > > Here's an example of what I mean: drop shadow uses applyH/VScale for both calculating the kernel size and offsetting the shadow. If we apply the filterScale (or resolution if you call it that), one or the other will break because they use different units. The kernel is applying to pixels, the offset is CSS. > > Another is displacement map. It (indirectly) uses applyH/VScale for color scaling as well as distance. I haven't verified that this will produce incorrect results yet. > > Maybe I'm misunderstanding the intent of things like absolutePaintRect - is it supposed to be scaled or not? You're suggesting I should do the scale before platformApplySoftware. I guess this means you think that as far as each individual filter is concerned it is always working on actual pixels, and thus applyH/VScale should be simply converting filter units into pixels. Is this right? Yes, that was the intention. absoluteRect actually defines the size in screen coordinates - the real size of your pixel buffer. In the past screen coordinates have been in CSS pixel. It worked quite well, but with HiDPI displays this is no longer true. absoluteRect needs to take the devicePixel to CSS pixel ratio into account as well. I should mention the reasoning behind it 1) with SVG Filters users actually can specify the resolution of the filtered result to speed it up during animations for instance by reducing the pixel size. 2) SVG demands pixel perfect results under all circumstances. Means if you e.g. apply a scale(10), then a blurred element should not look... well ... more blurry then expected. So we transform the filtered object into the screen coordinate space and do the filter operation there. Then we draw the result with inverse transform back on the canvas. I need to think about your use case with drop shadow. As you describe it, it actually does the right thing, doesn't it? Both, the radius as well as the offset are defined in CSS pixels. (For HTML we do not try to map local coordinates to screen coordinates sadly, which makes results more blurry on CSS transformations.) So when it produces the wrong results, then this seems to be the result of giving the wrong input to the filter.

Yeah, I was aware of the filter resolution attribute. I'll see if I can get the patch to simply update your scaling methods. I realised that my changelog comment about breaking svg filters is incorrect since I only set the filterScale (which I should rename to filterDevicePixelRatio or something) on the shorthand filters.

Hi. Any update on this? It is still broken.

Dean, there is a high demand that the fix is going to be in Safari next. Could you please take a look at this again?

Created attachment 231168 [details] Patch from dino - rebaselined

Attachment 231168 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/filters/FilterEffect.cpp:366: Declaration has space between type name and * in unsigned char *destinationPixel [whitespace/declaration] [3] ERROR: Source/WebCore/platform/graphics/filters/FilterEffect.cpp:367: Declaration has space between type name and * in unsigned char *sourcePixel [whitespace/declaration] [3] Total errors found: 2 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 231168 [details] Patch from dino - rebaselined Attachment 231168 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5451066409746432 New failing tests: svg/text/non-bmp-positioning-lists.svg fast/hidpi/filters-invert.html fast/hidpi/filters-blur.html fast/hidpi/filters-shadow.html fast/hidpi/filters-hue-rotate.html fast/hidpi/filters-reference.html fast/hidpi/filters-multiple.html

Created attachment 231176 [details] Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Created attachment 231179 [details] Patch from dino - rebaselined

Attachment 231179 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/filters/FilterEffect.cpp:366: Declaration has space between type name and * in unsigned char *destinationPixel [whitespace/declaration] [3] ERROR: Source/WebCore/platform/graphics/filters/FilterEffect.cpp:367: Declaration has space between type name and * in unsigned char *sourcePixel [whitespace/declaration] [3] Total errors found: 2 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 231179 [details] Patch from dino - rebaselined Attachment 231179 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6090052316692480 New failing tests: fast/hidpi/filters-blur.html fast/hidpi/filters-reference.html fast/hidpi/filters-hue-rotate.html fast/hidpi/filters-invert.html fast/hidpi/filters-multiple.html

Created attachment 231186 [details] Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

I think the wk2 image failures are due to subpixel. Check the difference images - it's the antialiasing boundary of the text element. I'm going to send another patch for review, with those tests marked as image diffs on wk2. Then file a bug on the subpixel errors.

Created attachment 231207 [details] Patch

Time for Dirk to review... again :)

Attachment 231207 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/filters/FilterEffect.cpp:366: Declaration has space between type name and * in unsigned char *destinationPixel [whitespace/declaration] [3] ERROR: Source/WebCore/platform/graphics/filters/FilterEffect.cpp:367: Declaration has space between type name and * in unsigned char *sourcePixel [whitespace/declaration] [3] Total errors found: 2 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.

WK2 error bugs are 132766

Committed r168577: <http://trac.webkit.org/changeset/168577>