Closed
Bug 976322
Opened 11 years ago
Closed 9 years ago
'source-in' operator results in wrong color when 'white' is used
Categories
(Core :: Graphics, defect)
Tracking
()
People
(Reporter: yury, Unassigned)
References
Details
(Keywords: regression, Whiteboard: [pdfjs-c-rendering])
Attachments
(2 files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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 <b56girard@gmail.com>
date: Thu Oct 17 19:08:20 2013 -0400
summary: Bug 928123 - Avoid PushGroup during simple FillRect. r=Bas
Reporter | ||
Updated•11 years ago
|
Blocks: 928123
tracking-firefox28:
--- → ?
tracking-firefox29:
--- → ?
tracking-firefox30:
--- → ?
Keywords: regression
Comment 1•11 years ago
|
||
Thanks! Agreed that this should block release.
Updated•11 years ago
|
status-firefox27:
--- → wontfix
status-firefox28:
--- → affected
status-firefox29:
--- → affected
status-firefox30:
--- → affected
Updated•11 years ago
|
Keywords: regressionwindow-wanted
Comment 2•11 years ago
|
||
Looks like we already know it's caused by bug 928123.
Keywords: regressionwindow-wanted
Comment 3•11 years ago
|
||
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.
Flags: needinfo?(bas)
Comment 4•11 years ago
|
||
Benoit, can you prepare a back out ? (even if it might be too late for 28) thanks
Comment 5•11 years ago
|
||
I think this patch should back out the original change from bug 928123 on Linux.
Attachment #8385814 -
Flags: feedback?(bgirard)
Attachment #8385814 -
Flags: feedback?(bas)
Flags: needinfo?(bas)
Comment 6•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
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.
blocking-b2g: --- → 1.3?
Keywords: qawanted
Comment 10•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
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
Keywords: qawanted
Comment 14•11 years ago
|
||
Moving to backlog then per comment 13 - can't reproduce on 1.3.
blocking-b2g: 1.3? → backlog
Updated•11 years ago
|
Attachment #8385814 -
Flags: feedback?(bgirard)
Comment 15•11 years ago
|
||
Any chance to see that bug fixed for 29? Thanks
Comment 16•11 years ago
|
||
I can confirm this bug still exists on Firefox 28.0 on Ubuntu 12.04.
Comment 17•11 years ago
|
||
wontfixing again as it's now too late to land this in FF29. we will track for one more cycle.
status-firefox31:
--- → affected
Comment 19•10 years ago
|
||
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.
Reporter | ||
Comment 20•10 years ago
|
||
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
Whiteboard: [pdfjs-c-rendering]
Reporter | ||
Comment 21•10 years ago
|
||
Comment 22•10 years ago
|
||
I can now firm this is fixed in Firefox 31.0 on Ubuntu 12.04.
Updated•10 years ago
|
Attachment #8385814 -
Flags: feedback?(bas)
Assignee | ||
Updated•10 years ago
|
blocking-b2g: backlog → ---
Updated•10 years ago
|
tracking-b2g:
+ → ---
Comment 25•9 years ago
|
||
Closing this bug based on comment 22. Please reopen if this is still an issue.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment 26•9 years ago
|
||
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•9 years ago
|
||
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.
Reporter | ||
Comment 28•9 years ago
|
||
Re-open to verify resolution on Fedora for newer version of Firefox. See comment 26.
Comment 29•9 years ago
|
||
David or Bruce, does this still reproduce for you?
Flags: needinfo?(davejohansen)
Flags: needinfo?(bruce)
Comment 30•9 years ago
|
||
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•9 years ago
|
||
Also, Firefox 45.0.2 on Fedora 23 and Firefox 38.7.0 on CentOS 7 show a black box.
Flags: needinfo?(davejohansen)
Comment 32•9 years ago
|
||
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.
Flags: needinfo?(bruce)
Keywords: verifyme
Comment 33•9 years ago
|
||
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.
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
status-firefox45:
--- → wontfix
status-firefox46:
--- → fixed
status-firefox47:
--- → fixed
status-firefox48:
--- → fixed
status-firefox-esr38:
--- → wontfix
status-firefox-esr45:
--- → wontfix
Resolution: --- → FIXED
Summary: 'source-in' operator results in wrong color when 'white' is used on Ubuntu → 'source-in' operator results in wrong color when 'white' is used
Comment 34•9 years ago
|
||
I can confirm on Firefox 46 on Ubuntu 12.04 that the image from comment 13 _now_ shows a white box. :-)
You need to log in
before you can comment on or make changes to this bug.
Description
•