Closed Bug 409227 Opened 17 years ago Closed 16 years ago

Multiple images within a <g> tag do not display properly when setting opacity value

Categories

(Core :: SVG, defect, P2)

All
Windows XP
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: bruce.rindahl, Assigned: roc)

References

()

Details

(Keywords: regression, testcase)

Attachments

(5 files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2007122005 Minefield/3.0b3pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2007122005 Minefield/3.0b3pre

I have multiple images as child nodes of a <g> tag.  To set the opacity of all the images I set opacity value for the <g> element.  If the value is anything less than 1 some of the tiles disappear.  In the above URL click the check box next to "Background".  Several image tiles appear.  Now set the slider bar above "Transparency" a little to the right.  Some of the tile disappear.  Click the little radio button called high to see the images appear and disappear on the fly.  This does not happen in FF2 (note FF2 really has problems with this site).  I am attaching a minimal test case.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
Open the attachment in FF3.  Open the DOM inspector and expand until you get to the "rasterLayers" tag.  Change the opacity value from 1 to 0.7 and some of the tiles will disappear.  Does not do this in FF2.
(In reply to comment #1)
> Created an attachment (id=294045) [details]
> minimal test case without scripting, etc.
> 
> Open the attachment in FF3.  Open the DOM inspector and expand until you get to
> the "rasterLayers" tag.  Change the opacity value from 1 to 0.7 and some of the
> tiles will disappear.  Does not do this in FF2.
> 

Oops.  There is no DOM inspector in FF2.  In the above URL FF2 does behave correctly after some additional navigation.
Works for me on current trunk.  Fixed by bug 406985 checkin?

Please retest with a new nightly.
(In reply to comment #3)
> Works for me on current trunk.  Fixed by bug 406985 checkin?
> 
> Please retest with a new nightly.
> 

I grabbed today's nightly that fixed bug 406985.  I waited to report until that was check-in.
 
FWIW I also see the bottom bit of the drawing disappears when I change the opacity.

Note that Fx 2.0 does have a DOM inspector you have to choose to install it though.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to comment #5)
> FWIW I also see the bottom bit of the drawing disappears when I change the
> opacity.

Exactly.  However the "bottom bit" are distinct images that align like tiles within the <g> tag
Keywords: regression, testcase
Summary: Regression - Multiple images within a <g> tag do not display properly when setting opacity value → Multiple images within a <g> tag do not display properly when setting opacity value
I think the latest cairo landing (bug 413878) fixed this. Please confirm.
Bug still there on my version.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008012804 Minefield/3.0b3pre
Bruce
How odd, I can no longer see it on.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008012809 Minefield/3.0b3pre
Or more accurately, I can't see the problem any more in the minimal testcase without scripting but the web application the URL links too does still seem to have an issue (assuming I'm clicking on the right buttons).
The minimal test case still shows the bug for me.  In the URL link if you click the 'Background' check box and then move the transparency slider part way some of the images will disappear.  If you then click the 'High' radio button the images try to come in but flash on and off.
Strange
Are we getting further regressions?  I see even less of the images in the above attachment with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12) Gecko/20080201 Firefox/2.0.0.12 than before.
In the top referenced URL the top row of images no longer display.
Thanks
Bruce, did you mean to test in Firefox 2 instead of a nightly?
Assignee: nobody → jwatt
Flags: blocking1.9?
Priority: -- → P1
Sorry
I tested both Firefox 2 and this:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008030706 Minefield/3.0b5pre
The nightly is showing less of the images than before while Firefox 2 is showing them all.
Bruce
Cool. I was a bit confused for a moment there. :-)
In this reduced testcase you still get the bottom of the image disappearing when you click (script changes the opacity) and you also get it having some really weird painting issues when you resize (especially when the window is wide but not too high).
This regression happened between 2007-08-01-05-trunk and 2007-08-10-05-trunk.

Between 2007-08-01-05-trunk and 2007-08-02-04-trunk the reduced testcase changed from working to causing the entire content area to stop painting.

http://bonsai.mozilla.org/cvsquery.cgi?module=SeaMonkeyAll&branch=HEAD&date=explicit&mindate=2007-08-01&maxdate=2007-08-02+04:00&cvsroot=/cvsroot

Between 2007-08-09-05-trunk and 2007-08-10-05-trunk the content area started painting again, but now this bug is present.

http://bonsai.mozilla.org/cvsquery.cgi?module=SeaMonkeyAll&branch=HEAD&date=explicit&mindate=2007-08-09&maxdate=2007-08-10+05:00&cvsroot=/cvsroot
So this was broken by the update to cairo 1.5.1 (bug 383960) on the 1st August. The change from "client area stopped rendering" to the current bug was caused by the addition of the _cairo_surface_fallback_clone_similar call to _cairo_surface_clone_similar (bug 390668) on the 10th August. If I take that patch and apply it to the code as it was immediately after the cairo 1.5.1 landing, the behavior is the same (so the checkins between the 1st and the 10th are irrelevant).
Blocks: 383960
As noted above, the red and blue PNG flickers when you make small adjustments to the window height when the window has a large width but small height. I've discovered that on the occasions when the PNG fails to display, we are falling back to calling _cairo_clip_intersect_mask in _cairo_clip_clip when setting the clip for the inner <svg>. The stack looks like this:

  _cairo_clip_clip()
  _cairo_gstate_clip()
  _moz_cairo_clip_preserve()
  _moz_cairo_clip()
  gfxContext::Clip()
  nsSVGUtils::SetClipRect()
  nsSVGInnerSVGFrame::PaintSVG()

On the occasions when the PNG displays correctly we are calling _cairo_clip_intersect_region. This seems to indicate that there is something wrong with _cairo_clip_intersect_mask for certain values in the CTM.
Commenting out the nsSVGUtils::SetClipRect() call eliminates the bug.
Ah, I see what's happening! When _cairo_clip_intersect_mask gets the union of the previous and new clips, it doesn't correctly take into account the ctm of the previous clip. At least that's how it seems from observation.
OS: Windows XP → All
Hardware: PC → All
Attached patch patchSplinter Review
Turns out that this is a simple case of _cairo_clip_intersect_mask failing to set clip->mode to CAIRO_CLIP_MODE_MASK. That was quite painful to debug, but at least my level of knowledge of cairo internals is now slightly higher than zero. :-)
Attachment #308822 - Flags: superreview?(vladimir)
Attachment #308822 - Flags: review?(vladimir)
Simple patch in hand, and this is a nasty rendering bug, so marking blocking1.9+.

I also sent an email to the cairo list about this bug BTW.
Flags: blocking1.9? → blocking1.9+
(In reply to comment #22)
> 
> Turns out that this is a simple case of _cairo_clip_intersect_mask failing to
> set clip->mode to CAIRO_CLIP_MODE_MASK. That was quite painful to debug, but at
> least my level of knowledge of cairo internals is now slightly higher than
> zero. :-)
> 
Jonathan
I think my knowledge is at zero and dropping fast.  Thanks for all the work!
Bruce
Does this patch require a beta cycle?  Must it block beta 5?
Comment on attachment 308822 [details] [diff] [review]
patch

This looks fine to me, though I'd also prefer to get cworth/behdad's ok on it as well.  Can you double check that we're not leaking the region when we upgrade the clip type?  I think it should be destroyed in clip_fini regardless of the clip mode, but I'm not sure.
Attachment #308822 - Flags: superreview?(vladimir)
Attachment #308822 - Flags: superreview+
Attachment #308822 - Flags: review?(vladimir)
Attachment #308822 - Flags: review+
Comment on attachment 308822 [details] [diff] [review]
patch

Actually, hold off on landing this.

The clip code is weird -- I believe the semantics are "apply the region clip, and then if there is a clip surface, use it as a mask", regardless of the clip mode.

So there's something weird going on here; I'm trying to come up with a cairo testcase to reproduce it.
Attachment #308822 - Flags: superreview-
Attachment #308822 - Flags: superreview+
Attachment #308822 - Flags: review-
Attachment #308822 - Flags: review+
Moving to P2.  Feel free to fight me.  :)

Priority: P1 → P2
(In reply to comment #26)
> I think it should be destroyed in clip_fini regardless of the clip mode,
> but I'm not sure.

It's _cairo_clip_reset, and I can confirm that it cleans up all member without looking to see what type of clip it is.
This is win32-only; I can't reproduce with any of the testcases under Linux or under OSX.
OS: All → Windows XP
(In reply to comment #28)
> Moving to P2.  Feel free to fight me.  :)
> 

If this means not blocking FF3b5 no problem.  If this means not blocking FF3 then I have an issue.  This is a regression, and it seems to break a vital feature in SVG - namely the ability to see attributes on a large number of elements at once via grouping.
It means not blocking FF3b5.  It means it will still block the final release.  
I just took a look at the cairo clipping code. At first, it seems
obvious what needs to be done in _cairo_clip_clip, (testing the
clip->mode to descide which functions to call and then "upgrading" it
as needed based on the results).

But then there's this _cairo_clip_init_deep_copy function which
surprisingly does nothing(!) if the clip->mode of the source and
destination objects don't match:

    if (other->mode != clip->mode) {
        /* We should reapply the original clip path in this case, and let          
         * whatever the right handling is happen */
    } else {
         ... actual copying code here ...
    }

Currently, these always match since we set the clip->mode based on the
surface capabilities. But then if we start doing the clip->mode
upgrade then I'm afraid we'll break _cairo_clip_init_deep_copy.

The code above is Vladimir's originally, so he might have some
comment. But my inclination is to not touch anything that's apparently
this fragile so close before cairo's 1.6 release.

Vlad?

-Carl
Argh, I'd forgotten about this -- do we have a minimized testcase yet?  I tried to create one and could never get things into a broken state.
The second attachment in this bug is a fairly simple testcase isn't it?

I don't know whether bug 313508 is related - you do the same thing to cause it i.e. resize the window. It has a relatively simple testcase now but is not fixed by jwatt's patch here.
A minimized cairo-only testcase, rather; that's what jwatt and I were talking about previously.
I just got back, but I won't get to this in the next day or two. Please take it if you have the cycles Vlad. Carl said there's some linux tool to get the cairo calls out of mozilla. I can't recall its name off the top of my head, but it sounded like you could use it to get the C calls from linux and then compile them as a testcase on Windows.

Carl: thanks for your comments.
Stealing for now
Assignee: jwatt → roc
Version: unspecified → Trunk
The cairo clip-surface path is a trap IMHO, it's horribly slow and doesn't do what you want with pixel values at the clip-path boundary, anyway.
(In reply to comment #39)
> The cairo clip-surface path is a trap IMHO, it's horribly slow and doesn't do
> what you want with pixel values at the clip-path boundary, anyway.

What does that mean? In a case like:

     cairo_arc (cr, ...);
     cairo_clip (cr);

The surface-based clipping does exactly what I want with pixel values
at the boundary, (and elsewhere too).

There is a trap that people can get that behavior when they meant to
ask for a pixel-region clip. But I haven't seen a good proposal to fix
that.

It's hard for me to tell what the core of this bug report
is. Obviously, we've found that _cairo_clip_clip is doing the wrong
thing when a clip gets upgraded to a surface-based clip. And we need
to fix that.

Are you also saying that there's a bug in this case that leads to the
surface-based clip in the first place? Is there mozilla code that fell
into the trap described above?

-Carl
Sorry, I'm just griping while looking for the underlying bug.

The problem I was referring to, though, is that if you clip to the rectangle (0.5, 0, 1.0, 1.0), and then fill with black twice, then assuming a white background the resulting pixel is 75% black when it should be only 50% black. It's the seams problem all over again.

To avoid that, you should just push_group inside every non-pixel-aligned clip, which also happens to solve the performance problems too. We should probably change our SVG code to do that. But I want to find the underlying cairo bug first.
It's definitely a geometry bug, i.e. not the clip mode.

When we come to paint the image, gstate->clip has a surface_rect of (0,0,125,225) ... but region.rgn.extents is (0,-24,125,225). I guess that explains why a strip at the bottom doesn't paint --- it's clipped by the region.

For regular surfaces I think having a negative Y in the extents makes no sense. I'll try to track that down.
No wait, we get that negative clip Y when we create a temporary surface for the opacity, that's OK.
Sorry, I misread the extents --- it's x2/y2, not width/height, so those numbers are OK.
Attached patch fixSplinter Review
This is it. It's obvious once you see it; we want an exact copy of other->surface, so we want to start the copy at its origin.
Attachment #315898 - Flags: review?(vladimir)
Whiteboard: [needs review]
Comment on attachment 315898 [details] [diff] [review]
fix

simple cairo regression, well understood fix.

Vlad, I assume you'll take this upstream?

I'll work on a reftest.
Attachment #315898 - Flags: approval1.9?
Whiteboard: [needs review] → [needs approval]
Comment on attachment 315898 [details] [diff] [review]
fix

a=beltzner, thanks for doing the reftest as well, roc
Attachment #315898 - Flags: approval1.9? → approval1.9+
Attached patch reftestSplinter Review
Finally managed to concoct a reftest.
Excellent! Thanks for digging into this roc!
Whiteboard: [needs approval] → [needs landing]
Checked in with reftest.
Flags: in-testsuite+
Whiteboard: [needs landing]
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Confirming this patch fixes both of the above testcases and my original page.  Thanks!
Bruce
And working with:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041707 Minefield/3.0pre

Thanks again!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: