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)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
firefox27 --- wontfix
firefox28 + wontfix
firefox29 + wontfix
firefox30 - wontfix
firefox31 --- wontfix
firefox45 --- wontfix
firefox46 --- fixed
firefox47 --- fixed
firefox48 --- fixed
firefox-esr38 --- wontfix
firefox-esr45 --- wontfix

People

(Reporter: yury, Unassigned)

References

Details

(Keywords: regression, Whiteboard: [pdfjs-c-rendering])

Attachments

(2 files)

Attached file Minimal test case to replicate the issue (deleted) —
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
Blocks: 928123
Keywords: regression
Thanks! Agreed that this should block release.
Looks like we already know it's caused by bug 928123.
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)
Benoit, can you prepare a back out ? (even if it might be too late for 28) thanks
Attached patch This should be a beta backout. (deleted) — — Splinter Review
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 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.
(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?
(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.
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
(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.
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.
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.
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
Moving to backlog then per comment 13 - can't reproduce on 1.3.
blocking-b2g: 1.3? → backlog
Attachment #8385814 - Flags: feedback?(bgirard)
Any chance to see that bug fixed for 29? Thanks
I can confirm this bug still exists on Firefox 28.0 on Ubuntu 12.04.
wontfixing again as it's now too late to land this in FF29. we will track for one more cycle.
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.
I can now firm this is fixed in Firefox 31.0 on Ubuntu 12.04.
Attachment #8385814 - Flags: feedback?(bas)
add tracking flag for backlog.
tracking-b2g: --- → +
blocking-b2g: backlog → ---
tracking-b2g: + → ---
Closing this bug based on comment 22. Please reopen if this is still an issue.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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?
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.
Re-open to verify resolution on Fedora for newer version of Firefox. See comment 26.
Status: RESOLVED → REOPENED
Keywords: verifyme
Resolution: FIXED → ---
David or Bruce, does this still reproduce for you?
Flags: needinfo?(davejohansen)
Flags: needinfo?(bruce)
I can confirm on Firefox 45.0.2 on Ubuntu 12.04 that the image from comment 13 still shows a black box.
Also, Firefox 45.0.2 on Fedora 23 and Firefox 38.7.0 on CentOS 7 show a black box.
Flags: needinfo?(davejohansen)
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
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 ago9 years ago
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
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.

Attachment

General

Created:
Updated:
Size: