Last Comment Bug 757878 - Add a fast path for 8888_over_565 with NEON
: Add a fast path for 8888_over_565 with NEON
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla16
Assigned To: Jeff Muizelaar [:jrmuizel]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-23 09:44 PDT by Jeff Muizelaar [:jrmuizel]
Modified: 2012-08-14 08:31 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
2 pass implementation of this op (27.17 KB, patch)
2012-05-23 09:44 PDT, Jeff Muizelaar [:jrmuizel]
b56girard: review+
joe: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Jeff Muizelaar [:jrmuizel] 2012-05-23 09:44:22 PDT
Created attachment 626481 [details] [diff] [review]
2 pass implementation of this op
Comment 1 Benoit Girard (:BenWa) 2012-05-25 08:33:45 PDT
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) 2012-05-25 08:53:30 PDT
(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] 2012-06-13 14:10:50 PDT
I meant to land this a long time ago.

https://hg.mozilla.org/integration/mozilla-inbound/rev/87dbb95cde7d
Comment 4 Ed Morley [:emorley] 2012-06-14 02:46:34 PDT
https://hg.mozilla.org/mozilla-central/rev/87dbb95cde7d
Comment 5 Jeff Muizelaar [:jrmuizel] 2012-06-26 17:08:02 PDT
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 2012-06-27 15:35:13 PDT
Comment on attachment 626481 [details] [diff] [review]
2 pass implementation of this op

Looks good for aurora.
Comment 7 Jeff Muizelaar [:jrmuizel] 2012-07-03 12:19:05 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/816d2c3ed088

Note You need to log in before you can comment on or make changes to this bug.