Closed Bug 1257939 Opened 4 years ago Closed 4 years ago

VerifyRGBXFormat assertion/crash with Skia content on Windows in layers TextureClient

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: lsalzman, Assigned: lsalzman)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files)

When we clear buffers for a texture client, we were doing a memset(0), which is not correct for the RGBX case. We need to make sure that only the RGB channels are 0, but the X channel is opaque alpha.

This causes us to hit the assertion in a lot of cases.
This patch fixes two issues:

It fixes the main issue by using std::fill<uint32_t> instead of memset, so we can fill with the appropriate 0xFF000000 pixel value.

Secondly, this fixes the TextureClient buffer initialization in general to treat the data as garbage/uninitialized when we don't clear, by passing this info down into Skia so that Skia doesn't try to verify the contents of a buffer that was never initialized, and will be shortly after explicitly cleared by whatever will overwrite it later. In almost all cases when we create a draw target from this texture client buffer, we are explicitly clearing it right after this, so this saves on the cost of doing an unnecessary initialization of the alpha channel.
Attachment #8732309 - Flags: review?(mchang)
Comment on attachment 8732309 [details] [diff] [review]
initialize BGRX alpha channel to opaque when clearing and ignore uninitialized alpha in texture clients

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

::: gfx/layers/BufferTexture.cpp
@@ +407,5 @@
>  
>    if ((aAllocFlags & ALLOC_CLEAR_BUFFER) ||
>        (aAllocFlags & ALLOC_CLEAR_BUFFER_BLACK)) {
> +    if (aFormat == gfx::SurfaceFormat::B8G8R8X8) {
> +      std::fill_n(reinterpret_cast<uint32_t*>(buf), bufSize / sizeof(uint32_t), 0xFF000000);

nit: Do we care about big/little endian here?
Attachment #8732309 - Flags: review?(mchang) → review+
(In reply to Mason Chang [:mchang] from comment #2)
> Comment on attachment 8732309 [details] [diff] [review]
> initialize BGRX alpha channel to opaque when clearing and ignore
> uninitialized alpha in texture clients
> 
> Review of attachment 8732309 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/BufferTexture.cpp
> @@ +407,5 @@
> >  
> >    if ((aAllocFlags & ALLOC_CLEAR_BUFFER) ||
> >        (aAllocFlags & ALLOC_CLEAR_BUFFER_BLACK)) {
> > +    if (aFormat == gfx::SurfaceFormat::B8G8R8X8) {
> > +      std::fill_n(reinterpret_cast<uint32_t*>(buf), bufSize / sizeof(uint32_t), 0xFF000000);
> 
> nit: Do we care about big/little endian here?

In this case the 0xFF000000 takes care of it, because where we say B8G8R8X8 we really mean X8R8G8B8_UINT32, so the alpha channel will be in the right place on either endianness. The problem is all of the gfx/layers code is "consistently broken" right now in that we always mean the _UINT32 interpretation even though the B8G8R8X8 enum doesn't mean that everywhere else outside layers/moz2d. That is a time bomb we will have to solve for layers and moz2 separately at some point in the future.
https://hg.mozilla.org/mozilla-central/rev/2da6cf4f1870
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
I did various microbenchmarks that tested common variants of SSE store optimizations for implementing memset (including aligned and unaligned stores), memset itself, the autovectorized vanilla C code, and rep stosd variants.

In my microbenchmark, pretty much everything took 17 seconds, whereas rep stosd was the only clear victor taking 12 seconds.

Conveniently, libyuv already implements X86/NEON optimizations for 32 bit fills, so let's just use that directly here.
Attachment #8762244 - Flags: review?(gwright)
Attachment #8762244 - Flags: review?(gwright) → review+
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3154f444e340
use libyuv::ARGBRect for initializing buffers in layers. r=gwright
You need to log in before you can comment on or make changes to this bug.