Bug 106221 - [SVG] Cached filter results are not invalidated on repaint rect change
Summary: [SVG] Cached filter results are not invalidated on repaint rect change
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Florin Malita
URL:
Keywords:
: 106041 106748 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-01-07 07:52 PST by Florin Malita
Modified: 2013-03-12 05:14 PDT (History)
15 users (show)

See Also:


Attachments
The bottom-right blurred rect is missing in the pop-up window (752 bytes, text/html)
2013-01-07 07:52 PST, Florin Malita
no flags Details
Patch (10.38 KB, patch)
2013-01-08 08:30 PST, Florin Malita
no flags Details | Formatted Diff | Diff
Patch (15.47 KB, patch)
2013-02-14 12:37 PST, Florin Malita
no flags Details | Formatted Diff | Diff
Patch for landing (15.46 KB, patch)
2013-02-14 19:05 PST, Florin Malita
no flags Details | Formatted Diff | Diff
Patch for landing (15.37 KB, patch)
2013-02-14 19:34 PST, Florin Malita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Florin Malita 2013-01-07 07:52:56 PST
Created attachment 181509 [details]
The bottom-right blurred rect is missing in the pop-up window

https://code.google.com/p/chromium/issues/detail?id=168199

1) open the attached file and allow the pop-up
2) scroll or resize the pop-up window to expose the rightmost region
3) notice that the filtered bottom-right rect is missing

* while painting SVG content, we are checking for intersection with the repaint rect and skipping elements outside the damage region
* for filters (and possibly masks) we're caching the resulting bitmap.
* later when the new region is exposed, we're simply re-using the cached bitmap
* hence we end up not (re)drawing elements outside the original repaint region

I think we need to also store the repaint region with cached bitmap, and invalidate on change.
Comment 1 Florin Malita 2013-01-07 07:56:58 PST
(In reply to comment #0)
> I think we need to also store the repaint region with cached bitmap, and invalidate on change.

Or we could adjust the repaint region while painting filtered content to cover the whole filter region, but I'm somewhat worried about extreme cases.
Comment 2 Florin Malita 2013-01-07 08:13:27 PST
(In reply to comment #1)
> (In reply to comment #0)
> > I think we need to also store the repaint region with cached bitmap, and invalidate on change.
> 
> Or we could adjust the repaint region while painting filtered content to cover the whole filter region, but I'm somewhat worried about extreme cases.

On second thought, we are allocating a bitmap for the full filter region anyway, so at least as far as storage is concerned there's no penalty for the latter approach.
Comment 3 Florin Malita 2013-01-08 08:30:13 PST
Created attachment 181699 [details]
Patch
Comment 4 Florin Malita 2013-01-08 08:37:16 PST
(In reply to comment #3)
> Created an attachment (id=181699) [details]
> Patch

I was hoping to encapsulate the paint rect save/restore in RenderSVGResourceFilter::applyResource/postApplyResource, but that requires passing down a PaintInfo reference instead of a GraphicsContext*& and updating the signature for these virtuals.

Unfortunately, not only does to call graph for these methods fan-out to the point where tens of (virtual) method signatures need to be updated, but it also dead-ends in some places where a PaintInfo is not available at all (like GraphicsContext::drawText).
Comment 5 Florin Malita 2013-01-14 05:33:11 PST
*** Bug 106748 has been marked as a duplicate of this bug. ***
Comment 6 Build Bot 2013-01-17 05:32:34 PST
Comment on attachment 181699 [details]
Patch

Attachment 181699 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/15910930

New failing tests:
svg/filters/filter-hidden-content.svg
Comment 7 Florin Malita 2013-02-14 12:37:50 PST
Created attachment 188405 [details]
Patch

Updated to use pixel tests (the initial reftest not working on Mac).
Comment 8 Dean Jackson 2013-02-14 17:08:38 PST
Comment on attachment 188405 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188405&action=review

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:372
> +    FilterData* filterData = m_filter.get(object);
> +
> +    return filterData ? filterData->drawingRegion : FloatRect();
> +}

Super nit: remove that blank line :)
Comment 9 Florin Malita 2013-02-14 19:04:25 PST
(In reply to comment #8)
> Super nit: remove that blank line :)

Thanks, will do :)
Comment 10 Florin Malita 2013-02-14 19:05:13 PST
Created attachment 188465 [details]
Patch for landing
Comment 11 WebKit Review Bot 2013-02-14 19:07:13 PST
Comment on attachment 188465 [details]
Patch for landing

Rejecting attachment 188465 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 188465, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
platform/gtk/TestExpectations
Hunk #1 FAILED at 77.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/gtk/TestExpectations.rej
patching file LayoutTests/platform/mac/TestExpectations
patching file LayoutTests/platform/qt/TestExpectations
Hunk #1 succeeded at 1412 (offset -6 lines).
patching file LayoutTests/svg/filters/filter-hidden-content.svg

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/16527940
Comment 12 Florin Malita 2013-02-14 19:34:55 PST
Created attachment 188466 [details]
Patch for landing
Comment 13 WebKit Review Bot 2013-02-14 20:00:01 PST
Comment on attachment 188466 [details]
Patch for landing

Clearing flags on attachment: 188466

Committed r142955: <http://trac.webkit.org/changeset/142955>
Comment 14 WebKit Review Bot 2013-02-14 20:00:06 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Ryosuke Niwa 2013-02-22 12:32:50 PST
Mac baseline added in http://trac.webkit.org/changeset/143773.
Comment 16 Tim Horton 2013-03-12 05:14:20 PDT
*** Bug 106041 has been marked as a duplicate of this bug. ***