Closed Bug 448831 Opened 12 years ago Closed 11 years ago

DOM changes to filters don't work

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

VERIFIED FIXED

People

(Reporter: longsonr, Assigned: longsonr)

References

Details

(Keywords: regression, testcase, verified1.9.1)

Attachments

(3 files, 4 obsolete files)

Attached patch patch (obsolete) — Splinter Review
change the href on an image filter and nothing happens. Setting it to a relative URL and expecting it to resolve via xml:base doesn't work either.
Attachment #331998 - Flags: superreview?(roc)
Attachment #331998 - Flags: review?(roc)
+nsIntRect
+nsSVGFEFloodElement::ComputeChangeBBox(const nsTArray<nsIntRect>& aSourceChangeBoxes,
+                                       const nsSVGFilterInstance& aInstance)
+{
+  return GetMaxRect();
+}

Why have you added this? feFlood doesn't have any inputs so this should always return an empty rect (which is what the default implementation does). same for nsSVGFETurbulenceElement and nsSVGFEImageElement.

Why not merge GetSrc and LoadImage somehow, since we always call GetSrc to use with LoadImage.

Attached patch merged GetSrc and LoadImage (obsolete) — Splinter Review
(In reply to comment #1)
> Why have you added this? feFlood doesn't have any inputs so this should always
> return an empty rect (which is what the default implementation does). same for
> nsSVGFETurbulenceElement and nsSVGFEImageElement.
> 

When you change a filter's attributes via the DOM you get this call stack...

nsSVGFilterProperty::DoUpdate
  nsSVGFilterProperty::UpdateRect
    nsSVGFilterFrame::GetInvalidationRegion
      instance->ComputeOutputDirtyRect
         ComputeResultChangeBoxes
            info->mFE->ComputeChangeBBox as you say returns an empty rect
         info->mResultChangeBox is now an empty rect
     aDirtyRect is now an empty rect so no refresh happens

The included reftests demonstrate this issue.

> Why not merge GetSrc and LoadImage somehow, since we always call GetSrc to use
> with LoadImage.
> 

Good idea, done.
Attachment #331998 - Attachment is obsolete: true
Attachment #332038 - Flags: superreview?(roc)
Attachment #332038 - Flags: review?(roc)
Attachment #331998 - Flags: superreview?(roc)
Attachment #331998 - Flags: review?(roc)
Attached image reftest1
Attached image reftest2
Comment on attachment 332038 [details] [diff] [review]
merged GetSrc and LoadImage

I do think this is wrong now but I don't know what the right thing to do is.

I'll break out the other changes into a different fix.
Attachment #332038 - Flags: superreview?(roc)
Attachment #332038 - Flags: review?(roc)
Attachment #332038 - Attachment is obsolete: true
Raised bug 448938 for the href issues.

It seems that there are two ways filters change. Either their input changes which is catered for or there are DOM changes to the filter attributes which seems to be optimised out at the moment.

Summary: dynamic changes to filters without an in1 don't work → DOM changes to filters don't work
nsSVGFilterFrame::GetInvalidationRegion tells us how much to repaint when the source changes. We need a new method that tells us how much to repaint when the filter itself changes, and then call that method instead in the right places.
Attached patch patch (obsolete) — Splinter Review
Here's a stab at it.
Attachment #332899 - Flags: superreview?(roc)
Attachment #332899 - Flags: review?(roc)
GetCoveredRegion shouldn't need an aRect parameter --- just pass null for aDirtyInputRect, ComputeCoveredRect isn't going to use it anyway. Otherwise, looks great.
Attached patch address review comments (obsolete) — Splinter Review
Attachment #332899 - Attachment is obsolete: true
Attachment #333370 - Flags: superreview?(roc)
Attachment #333370 - Flags: review?(roc)
Attachment #332899 - Flags: superreview?(roc)
Attachment #332899 - Flags: review?(roc)
Attached patch alternative fixSplinter Review
Sorry to mention this now, but I just realized I have an alternative patch from my branch which should also fix the bug. How do you feel about this?

This patch also prepares some stuff so that filters can be used with non-SVG sources.
Attachment #333383 - Flags: superreview?(mats.palmgren)
Attachment #333383 - Flags: review?(longsonr)
Comment on attachment 333383 [details] [diff] [review]
alternative fix

>-  // Compute filter effects region as per spec
>-  if (units == nsIDOMSVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX) {
>-    if (!bbox)
>-      return NS_OK;
>+  gfxRect filterArea = nsSVGUtils::GetRelativeRect(units,
>+    &filter->mLengthAttributes[nsSVGFilterElement::X], bbox, aTarget);
>+  filterArea.RoundOut();

There is no GetRelativeRect method in my copy of nsSVGUtils I think you need to submit another patch with that included.

>+nsRect
>+nsSVGFilterFrame::GetSourceForInvalidArea(nsIFrame *aTarget, const nsRect& aRect)

This has no callers. Is this part of the non-SVG preparation.

>+nsRect
>+nsSVGFilterFrame::GetFilterBBox(nsIFrame *aTarget, const nsRect *aSourceBBox)

The only caller to this passes null as aSourceBBox. Were you intending to use this argument at some later date? It should go if not.

>+{
>+  nsRect result;
>+
>+  nsAutoPtr<nsSVGFilterInstance> instance;
>+  nsresult rv = CreateInstance(aTarget, nsnull,
>+    nsnull, nsnull, aSourceBBox, getter_Transfers(instance));
>+  if (NS_SUCCEEDED(rv)) {
>+    if (!instance) {
>+      // The filter draws nothing, so the bbox is empty.
>+      result = nsRect();
>+    } else {
>+      // We've passed in the source's dirty area so the instance knows about it.
>+      // Now we can ask the instance to compute the area of the filter output
>+      // that's dirty.

This comment is wrong. Too much cut and paste here.

>diff --git a/layout/svg/base/src/nsSVGFilterFrame.h b/layout/svg/base/src/nsSVGFilterFrame.h
>--- a/layout/svg/base/src/nsSVGFilterFrame.h
>+++ b/layout/svg/base/src/nsSVGFilterFrame.h
>@@ -33,48 +33,86 @@
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #ifndef __NS_SVGFILTERFRAME_H__
> #define __NS_SVGFILTERFRAME_H__
> 
> #include "nsRect.h"
>+#include "nsSVGUtils.h"

Can't you get away with adding class nsSVGRenderState; etc?

>+
>+nsresult
>+nsSVGFilterInstance::ComputeSourceNeededRect(nsIntRect* aDirty)

The only caller to this is the method above that has no callers itself.
Sorry, this is trying to be too clever. I should just submit a single patch that does the clip-path/mask/filter integration and that includes these changes. I'll do that tomorrow.
Attachment #333383 - Flags: review?(longsonr) → review-
Attachment #333383 - Flags: superreview?(mats.palmgren)
Comment on attachment 333383 [details] [diff] [review]
alternative fix

I guess this patch is obsolete (now part of bug 450340?)
bug 450340 would fix this yes; and I think roc would rather get that one through review than this one.
Attachment #333370 - Attachment is obsolete: true
Attachment #333370 - Flags: superreview?(roc)
Attachment #333370 - Flags: review?(roc)
Fixed by check in for bug 450340

Reftests added in 6554dc6c2dfe
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Flags: wanted1.9.1? → wanted1.9.1+
Keywords: fixed1.9.1
verified FIXED on builds: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090414 Minefield/3.6a1pre ID:20090414030735

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090414 Shiretoko/3.5b4pre ID:20090414035212
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.