757878 - Add a fast path for 8888_over_565 with NEON

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: 2 pass implementation of this op

Comment 1

[Benoit Girard (:BenWa)]

Comment on attachment 626481 [details] [diff] [review]
2 pass implementation of this op

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

The style looks fine. I'm not qualified to review pixman code enough to review. I don't object with anything here.

Maybe joe can take a look?

::: gfx/cairo/libpixman/src/pixman-inlines.h
@@ +837,5 @@
> +		if (op_func != NULL)								\
> +		{										\
> +		    fetch_func ((void *)scanline_buf, (mask), (src_top), (src_bottom), (width), \
> +                        (weight_top), (weight_bottom), (vx), (unit_x), (max_vx), (zero_src));   \
> +		    ((void (*)(dst_type_t *, const mask_type_t *, const src_type_t *, int)) op_func)\

Style nit '\' alignment.

@@ +842,5 @@
> +			((dst), (mask), (src_type_t *)scanline_buf, (width));			\
> +		}										\
> +		else										\
> +		{										\
> +		    fetch_func ((void*)(dst), (mask), (src_top), (src_bottom), (width), (weight_top),  \

Style nit '\' alignment.

@@ +848,5 @@
> +		}                                                                               \
> +  } while (0)
> +
> +
> +#define SCANLINE_BUFFER_LENGTH 3072

I'm curious how you got 3072.

@@ +877,5 @@
>      pixman_fixed_t src_width_fixed;								\
>      int max_x;											\
>      pixman_bool_t need_src_extension;								\
> +                                                                                                \
> +    uint64_t stack_scanline_buffer[SCANLINE_BUFFER_LENGTH];                                     \

Why are you using uint64_t when you cast it before using it? Why not just use uint8_t and specify a bigger length?

@@ +952,5 @@
>  												\
>  	src_width_fixed = pixman_int_to_fixed (src_width);					\
>      }												\
> +                                                                                                \
> +    if (op_func != NULL && width * sizeof(src_type_t) > sizeof(stack_scanline_buffer))          \

\o/ for no more non thread safe static buffer.

@@ +1230,4 @@
>  }
>  
>  /* A workaround for old sun studio, see: https://bugs.freedesktop.org/show_bug.cgi?id=32764 */
> +#define FAST_BILINEAR_MAINLOOP_COMMON(scale_func_name, fetch_func, op_func, src_type_t, mask_type_t,\

Style nit '\' alignment.

Comment 2

[Benoit Girard (:BenWa)]

(In reply to Benoit Girard (:BenWa) from comment #1)
> @@ +877,5 @@
> >      pixman_fixed_t src_width_fixed;								\
> >      int max_x;											\
> >      pixman_bool_t need_src_extension;								\
> > +                                                                                                \
> > +    uint64_t stack_scanline_buffer[SCANLINE_BUFFER_LENGTH];                                     \
> 
> Why are you using uint64_t when you cast it before using it? Why not just
> use uint8_t and specify a bigger length?

Vlad points out this is for alignment. Never mind this comment.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

I meant to land this a long time ago.

https://hg.mozilla.org/integration/mozilla-inbound/rev/87dbb95cde7d

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/87dbb95cde7d

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 626481 [details] [diff] [review]
2 pass implementation of this op

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 754364 added a c fast path that will be taken instead of the more general NEON path. The general neon path is likely faster in many cases. This patch adds a NEON fast path that is 60x faster than the NEON general path.
User impact if declined: Slower mobile performance whenever we're drawing transparent PNGs
Testing completed (on m-c, etc.): This has been on m-c since 2012-06-14. It has passed the pixman test suite.
Risk to taking this patch (and alternatives if risky): Mobile only. This patch is fairly large but the risk is contained and should have no impact beyond the added fast path, which is used on every transparent PNG on NEON hardware. It is also very easy to backout because it is only the change in pixman between beta and m-c.
String or UUID changes made by this patch: None

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 626481 [details] [diff] [review]
2 pass implementation of this op

Looks good for aurora.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/816d2c3ed088