Bug 93471 - -webkit-filter prevents rendering at retina scale
: -webkit-filter prevents rendering at retina scale
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Images
: 528+ (Nightly build)
: Macintosh Mac OS X 10.8
: P2 Major
Assigned To: Dean Jackson
: InRadar
Depends on:
Blocks: 68469 119300 126398 132766
  Show dependency treegraph
 
Reported: 2012-08-08 06:01 PDT by Erik Aigner
Modified: 2014-05-10 02:30 PDT (History)
26 users (show)

See Also:


Attachments
Patch (36.00 KB, patch)
2013-07-30 19:02 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (838.62 KB, application/zip)
2013-07-31 02:48 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (1.36 MB, application/zip)
2013-07-31 05:41 PDT, Build Bot
no flags Details
Patch (35.88 KB, patch)
2013-08-07 20:05 PDT, Dean Jackson
buildbot: commit‑queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (730.16 KB, application/zip)
2013-08-08 06:04 PDT, Build Bot
no flags Details
Patch from dino - rebaselined (32.35 KB, patch)
2014-05-09 11:12 PDT, Dirk Schulze
buildbot: commit‑queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (706.69 KB, application/zip)
2014-05-09 12:10 PDT, Build Bot
no flags Details
Patch from dino - rebaselined (34.67 KB, patch)
2014-05-09 12:52 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (563.18 KB, application/zip)
2014-05-09 14:44 PDT, Build Bot
no flags Details
Patch (37.20 KB, patch)
2014-05-09 19:50 PDT, Dean Jackson
krit: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Erik Aigner 2012-08-08 06:01:01 PDT
If -webkit-filter is applied to an element and that element contains an image/background-image
that picture will not render at the correct scale on HiDPI displays.

I tested it on a MacBook Pro with Retina Display and the filters output is @1x instead of @2x
using a css selector in conjunction with a min-device-pixel-ratio/min-resolution @media query.

If no filter is applied the image renders correctly @2x
Comment 1 Radar WebKit Bug Importer 2012-08-08 10:36:27 PDT
<rdar://problem/12057066>
Comment 2 Stefan Rechsteiner 2012-11-13 02:37:44 PST
+1 reproducible: http://stackoverflow.com/questions/13347947/css-filter-on-retina-display-fuzzy-images
Comment 3 Tim Horton 2013-01-07 14:20:22 PST
*** Bug 106241 has been marked as a duplicate of this bug. ***
Comment 4 Alexey Proskuryakov 2013-01-07 14:31:26 PST
Per the duplicate, this happens even when there are now images at all.
Comment 5 Alexey Proskuryakov 2013-01-07 14:35:54 PST
s/now/no/.
Comment 6 Nico Weber 2013-01-08 09:40:00 PST
*** Bug 106331 has been marked as a duplicate of this bug. ***
Comment 7 Timothy Hatcher 2013-01-29 09:25:59 PST
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.
Comment 8 Darin Adler 2013-03-11 18:44:38 PDT
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.
Comment 9 Dirk Schulze 2013-03-20 13:04:12 PDT
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 ***
Comment 10 Stephen White 2013-03-20 13:32:28 PDT
Unduping, since this is a different issue.
Comment 11 Dean Jackson 2013-07-30 19:02:45 PDT
Created attachment 207796 [details]
Patch
Comment 12 Dean Jackson 2013-07-30 19:09:37 PDT
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 13 EFL EWS Bot 2013-07-30 20:13:59 PDT
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 14 Build Bot 2013-07-31 02:47:59 PDT
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
Comment 15 Build Bot 2013-07-31 02:48:04 PDT
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 16 Build Bot 2013-07-31 05:41:03 PDT
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
Comment 17 Build Bot 2013-07-31 05:41:08 PDT
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
Comment 18 Dean Jackson 2013-07-31 06:38:28 PDT
(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!?
Comment 19 Dean Jackson 2013-07-31 06:39:06 PDT
(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.
Comment 20 Dean Jackson 2013-08-07 20:05:39 PDT
Created attachment 208310 [details]
Patch
Comment 21 Dean Jackson 2013-08-07 20:06:26 PDT
Retrying.
Comment 22 Dirk Schulze 2013-08-08 01:34:43 PDT
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.
Comment 23 Dirk Schulze 2013-08-08 01:55:02 PDT
(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.
Comment 24 Dean Jackson 2013-08-08 02:35:10 PDT
(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 25 Build Bot 2013-08-08 06:03:55 PDT
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
Comment 26 Build Bot 2013-08-08 06:04:00 PDT
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
Comment 27 Dirk Schulze 2013-08-08 06:08:16 PDT
(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.
Comment 28 Dean Jackson 2013-08-08 14:04:04 PDT
> > > 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?
Comment 29 Dean Jackson 2013-08-08 14:48:35 PDT
The layout test failures are strange. It looks like maybe i need to force a reload after changing the page scale.
Comment 30 Dirk Schulze 2013-08-08 15:09:50 PDT
(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.
Comment 31 Dean Jackson 2013-08-08 16:06:35 PDT
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.
Comment 32 Tomasz Mikolajewski 2013-12-10 00:31:56 PST
Hi. Any update on this? It is still broken.
Comment 33 Dirk Schulze 2014-03-02 09:30:59 PST
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?
Comment 34 Dirk Schulze 2014-05-09 11:12:48 PDT
Created attachment 231168 [details]
Patch from dino - rebaselined
Comment 35 WebKit Commit Bot 2014-05-09 11:15:16 PDT
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 36 Build Bot 2014-05-09 12:10:29 PDT
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
Comment 37 Build Bot 2014-05-09 12:10:40 PDT
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
Comment 38 Dirk Schulze 2014-05-09 12:52:56 PDT
Created attachment 231179 [details]
Patch from dino - rebaselined
Comment 39 WebKit Commit Bot 2014-05-09 12:54:27 PDT
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 40 Build Bot 2014-05-09 14:44:42 PDT
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
Comment 41 Build Bot 2014-05-09 14:44:49 PDT
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
Comment 42 Dean Jackson 2014-05-09 19:39:30 PDT
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.
Comment 43 Dean Jackson 2014-05-09 19:50:37 PDT
Created attachment 231207 [details]
Patch
Comment 44 Dean Jackson 2014-05-09 19:51:31 PDT
Time for Dirk to review... again :)
Comment 45 WebKit Commit Bot 2014-05-09 19:51:33 PDT
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.
Comment 46 Dean Jackson 2014-05-09 19:52:43 PDT
WK2 error bugs are 132766
Comment 47 Dean Jackson 2014-05-10 02:30:53 PDT
Committed r168577: <http://trac.webkit.org/changeset/168577>