Bug 48545 - LLVMpipe glReadPixels Firefox hits the slow path (WebGL rendering)
LLVMpipe glReadPixels Firefox hits the slow path (WebGL rendering)
Status: RESOLVED FIXED
Product: Mesa
Classification: Unclassified
Component: Mesa core
8.0
x86 (IA32) Linux (All)
: medium normal
Assigned To: Brian Paul
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-11 03:51 UTC by Liam Wilson
Modified: 2012-04-17 16:43 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
do red/blue swizzle in glReadPixels (1.81 KB, patch)
2012-04-12 21:06 UTC, Brian Paul
Details | Splinter Review
add another fast path for glReadPixels (2.80 KB, patch)
2012-04-16 08:53 UTC, Brian Paul
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Liam Wilson 2012-04-11 03:51:50 UTC
I've reported this bug to Mozilla (https://bugzilla.mozilla.org/show_bug.cgi?id=743585), but I think Mesa may be needlessly going through the glReadPixels slow path so I thought I'd best report the bug here too.

I think what's happening is Firefox is creating a RGBA backing store of some kind, and then attempting to read it back BGRA. This is then in turn causing fast_read_rgba_pixels_memcpy (in /src/mesa/main/readpix.c) to fail and instead fall back to the slow path. As Firefox will read back the whole frame buffer each frame, this signifcantly damages performance.

I assume another fast path could be added which does the read back with a swizzle? In the mozilla bug report I included a patch to mesa that does this (which is a horrible hack). I'd be happy to try and turn that hack into a more general solution. Is that worth doing? If so, what's the best line of attack?
Comment 1 Brian Paul 2012-04-12 21:06:44 UTC
Created attachment 59889 [details] [review]
do red/blue swizzle in glReadPixels

Can you try this patch for Mesa/master?  If it works I can back port it to the 8.0 branch.
Comment 2 Liam Wilson 2012-04-15 12:29:46 UTC
(In reply to comment #1)
> Created attachment 59889 [details] [review] [review]
> do red/blue swizzle in glReadPixels
> 
> Can you try this patch for Mesa/master?  If it works I can back port it to the
> 8.0 branch.

Applied the patch with master. Built with:

 ./configure --disable-dri  --disable-egl --enable-xlib-glx --with-gallium-drivers=swrast

This does boost performance when the backing store is MESA_FORMAT_RGBA8888_REV . I found that this is not always the case with Firefox. If you initialise your webgl context with canvas.getContext("experimental-webgl",{alpha: false }) you end up with an MESA_FORMAT_XRGB8888 render buffer.
Comment 3 Brian Paul 2012-04-16 08:53:14 UTC
Created attachment 60068 [details] [review]
add another fast path for glReadPixels

Here's an updated patch that handles the MESA_FORMAT_XRGB888 buffer format.  I'm assuming that the format/type params to glReadPixels are still GL_BGRA/GL_UNSIGNED_INT_8_8_8_8_REV.
Comment 4 Liam Wilson 2012-04-17 05:50:46 UTC
(In reply to comment #3)
> Created attachment 60068 [details] [review] [review]
> add another fast path for glReadPixels
> 
> Here's an updated patch that handles the MESA_FORMAT_XRGB888 buffer format. 
> I'm assuming that the format/type params to glReadPixels are still
> GL_BGRA/GL_UNSIGNED_INT_8_8_8_8_REV.

The patch seems to work if I change 

if (rb->Format == MESA_FORMAT_XRGB8888_REV ...

to 

if (rb->Format == MESA_FORMAT_XRGB8888  ....

Incidentally, couldn't llvmpipe do all this for you? From what I can tell, there are a load of routines generated from lp_tile_soa.py that can do all the conversion for you. I think these conversion routines are used to generate the buffer returned by ctx->Driver.MapRenderbuffer anyway. Or would it be a case of moving the glReadPixels implementaion into Gallium (in the case of llvmpipe)? If so, I'm not sure if I have the skills to do it myself, but I'll have a look around and see what I can do.
Comment 5 Brian Paul 2012-04-17 09:43:11 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > Created attachment 60068 [details] [review] [review] [review]
> > add another fast path for glReadPixels
> > 
> > Here's an updated patch that handles the MESA_FORMAT_XRGB888 buffer format. 
> > I'm assuming that the format/type params to glReadPixels are still
> > GL_BGRA/GL_UNSIGNED_INT_8_8_8_8_REV.
> 
> The patch seems to work if I change 
> 
> if (rb->Format == MESA_FORMAT_XRGB8888_REV ...
> 
> to 
> 
> if (rb->Format == MESA_FORMAT_XRGB8888  ....

OK, thanks, I guess I did a bad copy&paste.


> Incidentally, couldn't llvmpipe do all this for you? From what I can tell,
> there are a load of routines generated from lp_tile_soa.py that can do all the
> conversion for you. I think these conversion routines are used to generate the
> buffer returned by ctx->Driver.MapRenderbuffer anyway. Or would it be a case of
> moving the glReadPixels implementaion into Gallium (in the case of llvmpipe)?
> If so, I'm not sure if I have the skills to do it myself, but I'll have a look
> around and see what I can do.

Doing this change in readpix.c means that all drivers can benefit.  Besides, the MapRenderbuffer call has no way of telling the driver that a specific surface format is desired.

I'll clean up my patch and post it for review before committing.  I think I can back-port this to the 8.0 branch too.
Comment 6 Brian Paul 2012-04-17 16:43:16 UTC
Patch committed to mesa with commit a5e95a419e4f6ad93e35a960113d97ae2de27476.
This includes a better approach to the swizzling code which in my test was 44% faster than before.

Also applied to the 8.0 branch: 49ed43b6de98482c898334a9abfc574720391c9f