1045957 - Improve GLReadTexImageHelper

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Kelsey Gilbert [:jgilbert]]

In fixing bug 1045955, I found a number of potential issues in GLReadTexImageHelper.cpp. It turns out they were unrelated, but I fixed most of them in this patch.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: fix-readback

Comment 2

[Kelsey Gilbert [:jgilbert]]

  https://tbpl.mozilla.org/?tree=Try&rev=1a06b41a9eba

Comment 3

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8464409 [details] [diff] [review]
fix-readback

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

::: gfx/gl/GLReadTexImageHelper.cpp
@@ +22,5 @@
> +Swap<uint8_t>(uint8_t& a, uint8_t& b)
> +{
> +    a ^= b;
> +    b ^= a;
> +    a ^= b;

Please don't use XOR swapping, see http://en.wikipedia.org/wiki/XOR_swap_algorithm#Reasons_for_avoidance_in_practice

::: gfx/gl/GLScreenBuffer.cpp
@@ +445,5 @@
>      }
>      MOZ_ASSERT(nextSurf);
>  
> +    bool ret = Attach(nextSurf, size);
> +    return ret;

Why this change?

::: gfx/gl/SharedSurfaceANGLE.cpp
@@ +6,5 @@
>  #include "SharedSurfaceANGLE.h"
>  
>  #include "GLContextEGL.h"
>  #include "GLLibraryEGL.h"
> +#include "GLReadTexImageHelper.h"

And this one?

Comment 4

[Kelsey Gilbert [:jgilbert]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> Comment on attachment 8464409 [details] [diff] [review]
> fix-readback
> 
> Review of attachment 8464409 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLReadTexImageHelper.cpp
> @@ +22,5 @@
> > +Swap<uint8_t>(uint8_t& a, uint8_t& b)
> > +{
> > +    a ^= b;
> > +    b ^= a;
> > +    a ^= b;
> 
> Please don't use XOR swapping, see
> http://en.wikipedia.org/wiki/
> XOR_swap_algorithm#Reasons_for_avoidance_in_practice
Great, that's what I thought, though this was pre-existing code. (I just moved it)
> 
> ::: gfx/gl/GLScreenBuffer.cpp
> @@ +445,5 @@
> >      }
> >      MOZ_ASSERT(nextSurf);
> >  
> > +    bool ret = Attach(nextSurf, size);
> > +    return ret;
> 
> Why this change?
Oops, was left over from debugging, when I needed to inject something after Attach.
> 
> ::: gfx/gl/SharedSurfaceANGLE.cpp
> @@ +6,5 @@
> >  #include "SharedSurfaceANGLE.h"
> >  
> >  #include "GLContextEGL.h"
> >  #include "GLLibraryEGL.h"
> > +#include "GLReadTexImageHelper.h"
> 
> And this one?
Oops, shouldn't be there.

Comment 5

[Kelsey Gilbert [:jgilbert]]

Attachment: fix-readback

r=mattwoodrow

Comment 6

[Kelsey Gilbert [:jgilbert]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/477ba7091557

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/477ba7091557

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Talos says this improved performance by 16.6% on the WebGL terrain talos test on WinXP slaves. Congratulations!

https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/rq5y8PGdrGk

Comment 9

[Avi Halachmi (:avih)]

Nice! :)

Any idea why it only affected XP?

http://graphs.mozilla.org/graph.html#tests=[[325,131,37],[325,63,31],[325,63,33],[325,63,25],[325,63,24]]&sel=1405032582445.972,1406764610418&displayrange=30&datatype=running

Comment 10

[Avi Halachmi (:avih)]

Hmm.. actually, it appears to have affected only non-PGO and only XP...

http://graphs.mozilla.org/graph.html#tests=[[325,131,37],[325,63,37]]&sel=1405032582445.972,1406764610418&displayrange=30&datatype=running

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

Because on Windows with D3D10 layers (Vista/7/8 with good enough drivers/hw) we don't read back WebGL frames, we pass them directly to the compositor.

But on XP we have to read back every WebGL frame, so we have that slow GPU -> CPU -> GPU round trip, and Jeff's patch here just made it significantly less slow. It matters a lot, because that slow readback code path is what at least a third of our users still use (and in particular, all of our WinXP users) and depending on machines and applications may just be fast enough to make WebGL useful.

Comment 12

[Kelsey Gilbert [:jgilbert]]

FWIW, I didn't even do the full optimization here, so there's further room for improvement.

Comment 13

[Avi Halachmi (:avih)]

(In reply to Avi Halachmi (:avih) from comment #10)
> Hmm.. actually, it appears to have affected only non-PGO and only XP...
> 
> http://graphs.mozilla.org/graph.html#tests=[[325,131,37],[325,63,
> 37]]&sel=1405032582445.972,1406764610418&displayrange=30&datatype=running

Glad to be proven wrong on the "only non-PGO" thingy. It appears that the PGO improvement appeared a bit later and after I posted this, maybe due to the lower frequency of PGO builds compared to non-PGO?

Anyway, good patch!

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Jeff Gilbert [:jgilbert] from comment #12)
> FWIW, I didn't even do the full optimization here, so there's further room
> for improvement.

Please do! As you can see, now that we have some WebGL Talos coverage, such improvements are actually measured and tracked, so I think that such work should now be much easier to justify prioritizing. (It always was important but we used to lack a way to measure improvement).