568767 - BasicImageLayer::Paint should apply OP_SRC if layer is opaque

Status: RESOLVED WONTFIX

(Core :: Graphics, defect)

Description

[Oleg Romashin (:romaxa)]

Attachment: opaque layer

It is a bit speedup rendering

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 447941 [details] [diff] [review]
opaque layer

{} around the 'if' body

Comment 2

[Oleg Romashin (:romaxa)]

Attachment: Same thing for canvas

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually, for both this and the previous patch, you need to set the operator back to OVER afterwards.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

in response to your question on IRC, setting it back to OVER is fine, you don't need to save and restore the operator.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm, actually I take that back. Please save and restore the operator.

Comment 6

[Bas Schouten (:bas.schouten)]

Please take into account that this -severely- slows Direct2D down.

Comment 7

[Bas Schouten (:bas.schouten)]

(In reply to comment #6)
> Please take into account that this -severely- slows Direct2D down.

Actually, that's not true, if the surface is actually an RGB surface and not RGBA, it will optimize the SOURCE to an OVER. I'd be -very- surprised if the image surface backend does not optimize OVER to SOURCE in that case though.

Comment 8

[Oleg Romashin (:romaxa)]

Attachment: Add operator restore.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #8)
> Created an attachment (id=448332) [details]
> Add operator restore

What are the situations where we're painting an opaque ARGB32 surface?

Comment 10

[Oleg Romashin (:romaxa)]

Not sure if it is ARGB32 surface... in my case it was opaque youtube video plugin layer.... and when it was up/down scaled then pixman was using very expensive scale path...


I can check again and show backtrace if it is needed

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

cairo backends should be responsible for optimizing OVER to SOURCE for RGB24 surfaces where that makes sense.

BasicPlanarYCbCrImage definitely has an RGB24 surface. Dunno about Oleg's plugin layer code.

Comment 12

[Oleg Romashin (:romaxa)]

err... I'm using BasicCairoImage not YCbCr for plugins. and youtube plugin is opaque (16bpp)... and rendering that to opaque 16bpp Target...

I think that was happening with old pixman... need to check if that problem still valid.

Comment 13

[Siarhei Siamashka]

(In reply to comment #11)
> cairo backends should be responsible for optimizing OVER to SOURCE for RGB24
> surfaces where that makes sense.

This is not so trivial, considering that RGB24 surfaces with NONE repeat actually have transparent pixels around them which also need special treatment. This has been addressed in the following commit in pixman:
http://cgit.freedesktop.org/pixman/commit/?id=da6f33a798bf2ea10df610ccf1d9506d63d1a28c

Anyway, there is still some risk of not getting OVER operator optimized to SRC when using scaling with transform parameters set a bit off so that the pixels outside the source image boundaries are being fetched. For bilinear scaling this is even unavoidable. That's why using PAD repeat is generally recommended instead of NONE, and for HW accelerated drivers too:
https://bugs.freedesktop.org/show_bug.cgi?id=27954

That said, currently pixman 0.19.2 seems to have some strange performance issue if repeat is set to PAD. This must be addressed before repeat PAD can be universally recommended.

Comment 14

[Siarhei Siamashka]

(In reply to comment #13)
> That said, currently pixman 0.19.2 seems to have some strange performance issue
> if repeat is set to PAD. This must be addressed before repeat PAD can be
> universally recommended.

I think this would be the fix:
http://lists.freedesktop.org/archives/pixman/2010-September/000584.html

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Karl looked into making BasicImageLayer use EXTEND_PAD, but we haven't gotten around to it quite yet; we need to fix the scaling to snap to pixels properly. Also, there's the issue of cairo being incredibly slow with EXTEND_PAD on many X servers, due to having to fallback to avoid potential X server bugs.

I think we should deal with those issues directly rather than use this patch.

Comment 16

[Karl Tomlinson (:karlt)]

That was in Bug 581797.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 448332 [details] [diff] [review]
Add operator restore.

I think we can just do this.