Bug 48545

Summary:	LLVMpipe glReadPixels Firefox hits the slow path (WebGL rendering)
Product:	Mesa	Reporter:	Liam Wilson <cosinusoidaly>
Component:	Mesa core	Assignee:	Brian Paul <brian.e.paul>
Status:	RESOLVED FIXED	QA Contact:
Severity:	normal
Priority:	medium
Version:	8.0
Hardware:	x86 (IA32)
OS:	Linux (All)
Whiteboard:
i915 platform:		i915 features:
Attachments:	do red/blue swizzle in glReadPixels add another fast path for glReadPixels

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

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.