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

RESOLVED FIXED

Status

()

Core
Canvas: 2D
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
Using clip() is hurting performance on direct2d
(Assignee)

Updated

8 years ago
Summary: Use fill() instead of clip() for image painting in canvas → Use fill() instead of clip(); paint() for image painting in canvas

Updated

8 years ago
blocking2.0: --- → ?
(Assignee)

Comment 1

8 years ago
Created attachment 457165 [details] [diff] [review]
Use fill() instead of clip() when appropriate

This takes the conservative approach of only using fill() when we're sure it's correct.
Attachment #457165 - Flags: review?(vladimir)
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?
Attachment #457165 - Flags: review?(vladimir) → review+

Comment 3

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

shouldn't this be '==' instead of '='?
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

8 years ago
also, isn't it wrong to compare to floating-point numbers since this test will never return true?
We need this for performance parity with IE9.
blocking2.0: ? → betaN+
Assignee: nobody → jmuizelaar
Any reason why this hasn't landed yet (with the = fix)?

Updated

8 years ago
OS: Mac OS X → All
Hardware: x86 → All

Updated

8 years ago
Blocks: 577069
Duplicate of this bug: 581925
(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 :)
(Assignee)

Comment 10

8 years ago
This change didn't seem to fix the performance problem and I haven't had a chance to investigate why yet.
Sounds like there might be another clip set higher up the stack.
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() ??
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).
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! :)
Created attachment 461616 [details] [diff] [review]
Use fill() instead of clip() when appropriate v2

Tiny update to make it compile on current trunk.
Attachment #457165 - Attachment is obsolete: true
Attachment #461616 - Flags: review?(joe)
Attachment #461616 - Flags: review?(joe) → review+

Comment 16

8 years ago
The patch still has:
if (CurrentState().globalAlpha = 1. 
with the single = and the 1. without 0.f.
Is that correct?
(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++.
(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.
Pushed follow-up http://hg.mozilla.org/mozilla-central/rev/ee3ab2be0040.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
has anyone tested it with IE benchmark???
(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

8 years ago
For me, the results were improved.
With this patch I get nearly twice as much fps as before... :)
It seems to me that after any changes to the d2d performance , the JavaScript performance is affected negatively...
(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.
(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.
why don't we enable the d2d write capability by default on the next installment??
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.

Updated

8 years ago
Depends on: 584663
You need to log in before you can comment on or make changes to this bug.