Closed Bug 915145 Opened 6 years ago Closed 6 years ago

(FxOS) canvas clipping to arbitrary path and text rendering aren't antialiased

Categories

(Core :: Canvas: 2D, defect)

26 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: evyatar, Assigned: gw280)

References

Details

(Keywords: regression)

Attachments

(2 files, 3 obsolete files)

* Clipping an image (to round it) when drawing it on canvas results in very jagged edges of the image, as it isn't being anti-aliased.
* Drawing text results in very quirky behavior - both aliased and out of place.

Please see attached HTML test page, and note the two Text tests and the "img (base64, rounded)".
Andreas this is the bug in Gecko I showed you yesterday.
Which team should this issue be assigned to?
Flags: needinfo?(gal)
Duplicate of this bug: 923714
Blocks: 1.3-e.me
Component: General → Canvas: 2D
Product: Boot2Gecko → Core
Version: unspecified → 26 Branch
This causes lots of pixelation with e.me app icons, which is not acceptable for the homescreen. Our partners are probably not going to be okay with the fact if we try to ship a homescreen with pixelated icons on e.me app icons. As such, I'm noming this to block.
Blocks: GFXB2G1.2
blocking-b2g: --- → koi?
(In reply to Jason Smith [:jsmith] from comment #3)
> This causes lots of pixelation with e.me app icons, which is not acceptable
> for the homescreen. Our partners are probably not going to be okay with the
> fact if we try to ship a homescreen with pixelated icons on e.me app icons.
> As such, I'm noming this to block.

One additional note - this causes pixelation of icons with the collections feature in 1.2 (i.e. 1.2 feature bug in gecko).
There are a lot of different approaches in the first testcase.  Can you describe exactly what the homescreen does with e.me app icons?  Likewise the collections feature, if it's different?
The different approaches are just there to show what works and what don't, and to have as many test cases possible.
As per comment 1 - the 3 tests that are "failing are:
1. Text - Games
2. Text - longer text
3. image (base64, rounded)	

I tried to include as many tests as possible to maybe help narrow the issue down.
The jagged-round image fails in the function "testImage", when passing shouldRound=true. The issue comes from these lines, which are used to round an icon (both in Collection icon and when displaying search results):
context.beginPath();
context.arc(width/2, height/2, width/2, 0, 2 * Math.PI, false);
context.clip();
Nod, the test case is very helpful, I was just curious which of the failing issues are what's used to draw.  I'll follow up.
Oh, so both of them are what we really use in the homescreen - simple text drawing, and clip to round.
Thinking about this more - I am going to hold on coming this. I will ask if UX thinks this is a blocker.
blocking-b2g: koi? → ---
(In reply to Jason Smith [:jsmith] from comment #9)
> Thinking about this more - I am going to hold on coming this. I will ask if
> UX thinks this is a blocker.

noming I meant, not coming (phone typeo).
It is possible that this is a result of Skia GL.  Try editing the b2g prefs file and forcibly disabling skia (I believe change gfx.canvas.azure.backends to just "cairo") and restarting b2g, and see what happens.
Summary: Canvas - rounding images and texts aren't AA → (FxOS) canvas clipping to arbitrary path and text rendering aren't antialiased
Great catch!

Setting the following in custom-prefs.js indeed fixed both clip AA and text rendering
user_pref('gfx.canvas.azure.backends', 'cairo');

So, knowing this, how do we move forward?
DrawTargetSkia.cpp:

#ifdef ANDROID
  mSoftClipping = false;
#else
  mSoftClipping = true;
#endif

Hilarious. I would like to have a conversation with whomever did this patch.
Flags: needinfo?(gal)
Try removing that ifdef so its just mSoftClipping = true and then do ./build.sh gecko and ./flash gecko. I think that will fix this issue.
And the winner is:

Bug 885632 - Always use soft clipping with SkiaGL r=mattwoodrow
Depends on: 885632
Blocks: 885632
No longer depends on: 885632
My patch simply preserves the existing logic there for the non-GL case and always enables it for GL. I thought it looked fishy too, but didn't know what the original argument was for disabling it.
If I remember correctly, it's because soft clipping in software Skia with RGB565 was broken. I could be misremembering, though.
Doh, should have read that other bug. Apparently this was the reason: https://bugzilla.mozilla.org/show_bug.cgi?id=716415#c7
Attached patch patch (obsolete) — Splinter Review
Attached patch and the right patch this time (obsolete) — Splinter Review
Attachment #814004 - Attachment is obsolete: true
Keywords: regression
Comment on attachment 814006 [details] [diff] [review]
and the right patch this time

Want to give this a spin and then land it? Not sure whether we use RGB565 anywhere or where you could easily force that.
Attachment #814006 - Flags: review?(gwright)
Also, with skia we seem to be using more memory for the homescreen images. That was reported separately as a bug. I guess this is software skia, so it must have some sort of cache that holds on to a copy of the images (maybe a scaled copy?).
If we don't break any tests we definitely should enable soft clipping everywhere.

Try run here: https://tbpl.mozilla.org/?tree=Try&rev=22d2cfc8c1c6

Did you have a particular reason for using a const bool for soft clipping or can we just mSoftClipping(true) in the ctor instead?
Attached patch soft-clipping (obsolete) — Splinter Review
Smaller patch
Attachment #814006 - Attachment is obsolete: true
Attachment #814006 - Flags: review?(gwright)
Attachment #814477 - Flags: review?(snorp)
Comment on attachment 814477 [details] [diff] [review]
soft-clipping

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

::: gfx/2d/DrawTargetSkia.cpp
@@ +134,1 @@
>    mSoftClipping = true;

can just put mSoftClipping(true) in the member initializer
Attachment #814477 - Flags: review?(snorp) → review+
This will cause memory to be allocated for the member since you are guaranteed to be able to do &member. Not a big deal, but thats why I didn't do that.
Well I guess the real question is, do we ever want this to be preffable in the future? If not, then let's just remove the variable altogether and use true at all the callsites.
I will sr- any patch that tries to use non-AA-ed clipping on mobile, in case that answers your question ;) My patch meant to do that in a readable way.
Nuke the variable, use soft clipping always. Don't give the impression that we're ever going to support not doing soft clipping.
Attachment #814477 - Attachment is obsolete: true
Attachment #814492 - Flags: review?(snorp)
Attachment #814492 - Flags: review?(snorp) → review+
https://hg.mozilla.org/mozilla-central/rev/4b21fe65487a
Assignee: nobody → gwright
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Duplicate of this bug: 916309
We should get this patch on Aurora. Can you nominate for approval?
well I'm on gaia, not gecko, so I wouldn't even know how to nominate for Aurora :)
Comment on attachment 814492 [details] [diff] [review]
soft-clipping take 3



[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or IDL/UUID changes made by this patch:
Attachment #814492 - Flags: approval-mozilla-aurora?
NI on George to help fill the approval request form in comment 35.
Flags: needinfo?(gwright)
[Approval Request Comment]
Bug caused by (feature/regressing bug #): not a regression, always been this behaviour
User impact if declined: bad rendering when viewing many pages using <canvas>
Testing completed (on m-c, etc.): been on m-c for a long time
Risk to taking this patch (and alternatives if risky): no risk
String or IDL/UUID changes made by this patch: none
Flags: needinfo?(gwright)
Attachment #814492 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.