Add a fast path for 8888_over_565 with NEON

RESOLVED FIXED in Firefox 15

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Tracking

unspecified
mozilla16
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox15 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 626481 [details] [diff] [review]
2 pass implementation of this op
(Assignee)

Updated

5 years ago
Attachment #626481 - Flags: review?(bgirard)
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.
Attachment #626481 - Flags: review?(joe)
Attachment #626481 - Flags: review?(bgirard)
Attachment #626481 - Flags: review+
(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.
Attachment #626481 - Flags: review?(joe) → review+
(Assignee)

Comment 3

5 years ago
I meant to land this a long time ago.

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

Updated

5 years ago
Assignee: nobody → jmuizelaar
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/87dbb95cde7d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 5

5 years ago
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
Attachment #626481 - Flags: approval-mozilla-aurora?
Comment on attachment 626481 [details] [diff] [review]
2 pass implementation of this op

Looks good for aurora.
Attachment #626481 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/816d2c3ed088
status-firefox15: --- → fixed

Updated

5 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.