976322 - 'source-in' operator results in wrong color when 'white' is used

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Yury Delendik (:yury)]

Attachment: Minimal test case to replicate the issue

The following fragment produces black (instead of white):

  maskCtx.fillRect(10,10,40,40);
  maskCtx.globalCompositeOperation = 'source-in';
  // maskCtx.fillStyle = 'rgb(254, 255, 255)'; // works
  maskCtx.fillStyle = 'rgb(255, 255, 255)';
  maskCtx.fillRect(0, 0, maskCanvas.width, maskCanvas.height);

Worked in FF26:

Last good revision: 4e7d1e2c93a6
First bad revision: 77f70432fdcd
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4e7d1e2c93a6&tochange=77f70432fdcd


The first bad revision is:
changeset:   151385:fc7cc3c1dccf
user:        Benoit Girard <[e-mail]>
date:        Thu Oct 17 19:08:20 2013 -0400
summary:     Bug 928123 - Avoid PushGroup during simple FillRect. r=Bas

Comment 1

[Benoit Girard (:BenWa)]

Thanks! Agreed that this should block release.

Comment 2

[Benoit Girard (:BenWa)]

Looks like we already know it's caused by bug 928123.

Comment 3

[Benoit Girard (:BenWa)]

Bas the patch in bug 928123 was largely dictated by you (I should of got jeff to review it). Can you easily spot the mistake since I don't understand this stuff very well? Note that this PushGroup is a regression in Azure. If not I'll back it out and well live with the performance regression until we can investigate it further.

Comment 4

[Sylvestre Ledru [:Sylvestre]]

Benoit, can you prepare a back out ? (even if it might be too late for 28) thanks

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: This should be a beta backout.

I think this patch should back out the original change from bug 928123 on Linux.

Comment 6

[Benoit Girard (:BenWa)]

Comment on attachment 8385814 [details] [diff] [review]
This should be a beta backout.

Review of attachment 8385814 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/DrawTargetCairo.cpp
@@ +702,5 @@
>    cairo_rectangle(mContext, aRect.x, aRect.y, aRect.Width(), aRect.Height());
>  
>    bool pathBoundsClip = false;
>  
> +#ifndef MOZ_X11

Why is this ifdef? I suspect this logic needs to be checked. Unless I'm missing something I think we should just temporarily disable this everywhere and come back to it soon since this pushgroup is important to remove.

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Benoit Girard (:BenWa) from comment #6)
> Comment on attachment 8385814 [details] [diff] [review]
> This should be a beta backout.
> 
> Review of attachment 8385814 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/DrawTargetCairo.cpp
> @@ +702,5 @@
> >    cairo_rectangle(mContext, aRect.x, aRect.y, aRect.Width(), aRect.Height());
> >  
> >    bool pathBoundsClip = false;
> >  
> > +#ifndef MOZ_X11
> 
> Why is this ifdef? I suspect this logic needs to be checked. Unless I'm
> missing something I think we should just temporarily disable this everywhere
> and come back to it soon since this pushgroup is important to remove.

We only see the problem on Linux, right?  And your comment 3 talks about a performance regression.  Neither one of these two was something I wanted to take lightly.  Can we quantify the performance regression?  Will it be on all platforms?  Are we worried about it?  Also, 28 has branched in B2G 1.3, if this change is needed on all platforms, would we want to update that branch as well, and where would we expect performance regressions on 1.3?

Comment 8

[Benoit Girard (:BenWa)]

(In reply to Milan Sreckovic [:milan] from comment #7)
> (In reply to Benoit Girard (:BenWa) from comment #6)
> > Comment on attachment 8385814 [details] [diff] [review]
> > This should be a beta backout.
> > 
> > Review of attachment 8385814 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/2d/DrawTargetCairo.cpp
> > @@ +702,5 @@
> > >    cairo_rectangle(mContext, aRect.x, aRect.y, aRect.Width(), aRect.Height());
> > >  
> > >    bool pathBoundsClip = false;
> > >  
> > > +#ifndef MOZ_X11
> > 
> > Why is this ifdef? I suspect this logic needs to be checked. Unless I'm
> > missing something I think we should just temporarily disable this everywhere
> > and come back to it soon since this pushgroup is important to remove.
> 
> We only see the problem on Linux, right? 

It feels to me like the code is incorrect with, at least, operator source-in when we use azure cairo to draw. I'd imagine b2g/fennec would also be affect with this bug and some configuration. Apparently we don't use cairo azure on windows canvas anymore.

> And your comment 3 talks about a
> performance regression.  Neither one of these two was something I wanted to
> take lightly.  Can we quantify the performance regression?

It was causing us to skip frames IIRC so maybe say 5ms rasterize time regressions on some b2g apps.

> Will it be on
> all platforms?  Are we worried about it? 

Assuming it only happens with 'unsual operators' then the regression might be restricted to linux+mobile canvas.

> Also, 28 has branched in B2G 1.3,
> if this change is needed on all platforms, would we want to update that
> branch as well, and where would we expect performance regressions on 1.3?

Ideally we want correct canvas on all b2g. It's really sucky when authors can't use something like an operator because it's randomly buggy in one b2g version.

Fixing this might less work then backing it out IMO.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

If we're going to correct this for 1.3, we need to make sure it reproduces there.  Could QA check if this is a problem on 1.3 (or 1.4)?  We can then change the platform setting.

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Benoit Girard (:BenWa) from comment #8)
> ...
> 
> Ideally we want correct canvas on all b2g. It's really sucky when authors
> can't use something like an operator because it's randomly buggy in one b2g
> version.
> 
> Fixing this might less work then backing it out IMO.

Oh, I didn't look at this in detail before - you're saying we're handling source-in incorrectly, rather than we're making the fast path choice incorrectly.  I'll see if I can find anything today.

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

We're at final beta for FF28 today, if this does need to get on b2g 1.3 it can perhaps be uplifted there separately.

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

It is a special case, and the most common over and source are not affected, so I think we're fine with wontfix for 28.

Comment 13

[Peter Bylenga [:PBylenga]]

This issue does not reproduce on today's Buri v1.3, navigated to https://bug976322.bugzilla.mozilla.org/attachment.cgi?id=8380956 and the box was white.

v1.3 Environmental Variables:
Device: Buri v1.3 MOZ
BuildID: 20140310004001
Gaia: 78c30d7ed6f6e30337d6c05453b867f5e97e42bc
Gecko: 00f249d54bda
Version: 28.0
Firmware Version: v1.2-device.cfg

Comment 14

[Jason Smith [:jsmith]]

Moving to backlog then per comment 13 - can't reproduce on 1.3.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Any chance to see that bug fixed for 29? Thanks

Comment 16

[Bruce Momjian]

I can confirm this bug still exists on Firefox 28.0 on Ubuntu 12.04.

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

wontfixing again as it's now too late to land this in FF29.  we will track for one more cycle.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

No longer tracking as this is not looking like a priority and we've shipped it several times now.  Low risk uplift nominations welcome when available.

Comment 20

[Yury Delendik (:yury)]

Few more reports:

https://github.com/mozilla/pdf.js/issues/4073
https://github.com/mozilla/pdf.js/issues/4171
https://github.com/mozilla/pdf.js/issues/4852

Comment 21

[Yury Delendik (:yury)]

https://github.com/mozilla/pdf.js/issues/4648#issuecomment-46979696

Comment 22

[Bruce Momjian]

I can now firm this is fixed in Firefox 31.0 on Ubuntu 12.04.

Comment 23

[Bobby Chien]

add tracking flag for backlog.

Comment 25

[u279076]

Closing this bug based on comment 22. Please reopen if this is still an issue.

Comment 26

[David Johansen]

Using Firefox 41.0.2 on Fedora 22, the test from comment 13 is still a black box and the issue from bug 1122363 is still happening. It does not appear that I have permission to reopen this bug, so could someone please reopen it against Firefox 41?

Comment 27

[Bruce Momjian]

I can confirm comment 26 that the box appears black, but on Ubuntu 12.04 'eog' and 'display' also show the PNG image with the box as black.  I can say that before comment 22, all my PDFs appeared with black backgrounds, e.g. http://momjian.us/main/writings/pgsql/sharding.pdf, and now they are fine.  So, I would say the PDF display is fixed, and I am not sure the PNG is even broken as other tools display it the same as Firefox.

Comment 28

[Yury Delendik (:yury)]

Re-open to verify resolution on Fedora for newer version of Firefox. See comment 26.

Comment 29

[u279076]

David or Bruce, does this still reproduce for you?

Comment 30

[Bruce Momjian]

I can confirm on Firefox 45.0.2 on Ubuntu 12.04 that the image from comment 13 still shows a black box.

Comment 31

[David Johansen]

Also, Firefox 45.0.2 on Fedora 23 and Firefox 38.7.0 on CentOS 7 show a black box.

Comment 32

[u279076]

Thanks guys. I confirm this is reproducible in a Fedora 23 VM and that this regressed in Firefox 27, however it looks like this is resolved in today's Nightly build. I'm going to check to see where/when this was resolved.

Comment 33

[u279076]

I found the fix in Firefox 46 using mozregression:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f36a4a4ff73f1184f569fd9152b98fe8d8d3866a&tochange=cea2883034cbb4485c1ee0047cd6a7cfe4b9b652

I've additionally confirmed this is fixed with Firefox 46 Beta 11. I am closing this bug as fixed since Firefox 46 is released tomorrow. Please reopen if this continues to reproduce for you after updating.

Thanks everyone for your help.

Comment 34

[Bruce Momjian]

I can confirm on Firefox 46 on Ubuntu 12.04 that the image from comment 13 _now_ shows a white box.  :-)