576169 - Use fill() instead of clip(); paint() for image painting in canvas

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Using clip() is hurting performance on direct2d

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: Use fill() instead of clip() when appropriate

This takes the conservative approach of only using fill() when we're sure it's correct.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 457165 [details] [diff] [review]
Use fill() instead of clip() when appropriate

Sure, but seems like a copout :-)  Shouldn't we make this work for other operators as well?

Comment 3

[Deniz Sarikaya]

> Created attachment 457165 [details] [diff] [review]
> Use fill() instead of clip() when appropriate
> 
>  if (CurrentState().globalAlpha = 1. 

shouldn't this be '==' instead of '='?

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 457165 [details] [diff] [review]
Use fill() instead of clip() when appropriate

Er yes, quite.  Also make that "1." a "1.0f", because the "." with no 0 broke my head enough that I didn't see the lack of == :-)

Comment 5

[Deniz Sarikaya]

also, isn't it wrong to compare to floating-point numbers since this test will never return true?

Comment 6

[Joe Drew (not getting mail)]

We need this for performance parity with IE9.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Any reason why this hasn't landed yet (with the = fix)?

Comment 9

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #7)
> Any reason why this hasn't landed yet (with the = fix)?

This has also been asked several times on Mozillazine. If anyone has any information, we'd love to hear it :)

Comment 10

[Jeff Muizelaar [:jrmuizel]]

This change didn't seem to fix the performance problem and I haven't had a chance to investigate why yet.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Sounds like there might be another clip set higher up the stack.

Comment 12

[lukaswrites.bugzilla]

i think , this is crucial bug ( should be set to blocking2.0:beta3) I am wondering why the performance of d2d in beta 1 could surpass the beta2? does it use clip() or fill() ??

Comment 13

[[not reading bugmail]]

Luke there was another D2D bug that fixed some issues, although slowed the performance last month.  This is why your seen a beta slowdown.  This bug is open to work on fixing part of the performance regression and hasn't been resolved (see comment 10).

Comment 14

[Bas Schouten (:bas.schouten)]

Since the fixing of bug 583033 this fix seems to work and performance seems to be back to normal in my tests. We should probably land it! :)

Comment 15

[Bas Schouten (:bas.schouten)]

Attachment: Use fill() instead of clip() when appropriate v2

Tiny update to make it compile on current trunk.

Comment 16

[Alfred Kayser]

The patch still has:
if (CurrentState().globalAlpha = 1. 
with the single = and the 1. without 0.f.
Is that correct?

Comment 17

[Bas Schouten (:bas.schouten)]

(In reply to comment #16)
> The patch still has:
> if (CurrentState().globalAlpha = 1. 
> with the single = and the 1. without 0.f.
> Is that correct?

As far as I know that's perfectly legal C++.

Comment 18

[Bas Schouten (:bas.schouten)]

(In reply to comment #16)
> The patch still has:
> if (CurrentState().globalAlpha = 1. 
> with the single = and the 1. without 0.f.
> Is that correct?

Ugh, I neglected to notice vlad's earlier review comment. I'll work on fixing that.

Comment 19

[Bas Schouten (:bas.schouten)]

Pushed follow-up http://hg.mozilla.org/mozilla-central/rev/ee3ab2be0040.

Comment 20

[lukaswrites.bugzilla]

has anyone tested it with IE benchmark???

Comment 21

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #20)
> has anyone tested it with IE benchmark???

After doing a quick test on the IE Fishank benchmark the results were pretty much as before - I am getting consistently good 60fps on 250 again.

Comment 22

[Deniz Sarikaya]

For me, the results were improved.
With this patch I get nearly twice as much fps as before... :)

Comment 23

[lukaswrites.bugzilla]

It seems to me that after any changes to the d2d performance , the JavaScript performance is affected negatively...

Comment 24

[[not reading bugmail]]

(In reply to comment #20)
> has anyone tested it with IE benchmark???

(In reply to comment #23)
> It seems to me that after any changes to the d2d performance, the JavaScript
> performance is affected negatively..

Fyi. The FishIE performance discussion was actually bug 577069; a bug this blocked.  Anything else, file another bug please.

Comment 25

[lukaswrites.bugzilla]

(In reply to comment #24)
> (In reply to comment #20)
> > has anyone tested it with IE benchmark???
> 
> (In reply to comment #23)
> > It seems to me that after any changes to the d2d performance, the JavaScript
> > performance is affected negatively..
> 
> Fyi. The FishIE performance discussion was actually bug 577069; a bug this
> blocked.  Anything else, file another bug please.

the comment#23 is not about FishIETanks benchmark though, according to my own test, JavaScript performance regressed while trying to do some chromeexperiments.com tests.

Comment 26

[lukaswrites.bugzilla]

why don't we enable the d2d write capability by default on the next installment??

Comment 27

[[not reading bugmail]]

Can you file another bug, no need to add further comments here.

Also please, if your not familiar with bugzilla, Your last question is off topic, has nothing to do with this bug and your spamming emails.  This is not a general d2d discussion, nor a forum.  This bug is resolved and only for a specific code change.  

If you wish to discuss, I suggest you seek help on Mozillazine forums before making further comments.