Something like: TemporaryRef<DataSourceSurface> ReadBackSurface(GLContext* aCtx, GLuint aTexture) It should not be a member of GLContext since we are trying to slim GLContext down. Let's place it in a separate file like GLContextUtils.h/.cpp

So, as we discussed on IRC I have created GLContextUtils::ReadBackSurface which internally uses GLContext::GetTexImage and copies the gfxImageSurface I get from there into a DataSourceSurface. Now, that copying.. I looked but couldn't find any helper method already doing that for me, so I went ahead and created gfxImageSurface::CopyTo. It compiles well and follows the pattern already there in CopyFrom which does the inverse of what I want. What do you think of that approach? Are there simpler ways? You could assign me on the bug as well.

(In reply to Andreas Pehrson [:pehrsons] from comment #1) > So, as we discussed on IRC I have created GLContextUtils::ReadBackSurface > which internally uses GLContext::GetTexImage and copies the gfxImageSurface > I get from there into a DataSourceSurface. > > Now, that copying.. I looked but couldn't find any helper method already > doing that for me, so I went ahead and created gfxImageSurface::CopyTo. It > compiles well and follows the pattern already there in CopyFrom which does > the inverse of what I want. What do you think of that approach? Are there > simpler ways? > > You could assign me on the bug as well. Sounds reasonable, the other way is to just memcpy things in ReadBackSurface without adding code in gfxImageSurface but it's probable other Moz2Dification tasks may need that kind of convenience function so factoring it in gfxImageSurface can prove to be usefull. It doesn't sound like a scary amount of code to add :)

Looks like you forgot to hg add GLContextUtils.h

Comment on attachment 8339268 [details] [diff] [review] Add ReadBackSurface helper in GLContextUtils Now with GLContextUtils.h ... still getting accustomed to hg.

Comment on attachment 8339268 [details] [diff] [review] Add ReadBackSurface helper in GLContextUtils Review of attachment 8339268 [details] [diff] [review]: ----------------------------------------------------------------- R=me with ReadBackSurface just as a function instead of a static method. ::: gfx/gl/GLContextUtils.h @@ +21,5 @@ > +class GLContextUtils { > +public: > + static TemporaryRef<gfx::DataSourceSurface> > + ReadBackSurface(GLContext* aContext, GLuint aTexture, bool aYInvert, SurfaceFormat aFormat); > +}; I would prefer that ReadBackSurface be just a free function. It doesn't have a state so it doesn't need a class.

> I would prefer that ReadBackSurface be just a free function. It doesn't have > a state so it doesn't need a class. If you agree with this, and to avoid the bugzilla overhead I can make this small change my self and land as soon as the try push is green (it will still be your name on the commit).

nical, sure! Please go ahead. Try _should_ become green now =)

https://hg.mozilla.org/integration/mozilla-inbound/rev/d96c86f2b4d4 https://hg.mozilla.org/integration/mozilla-inbound/rev/b44b4be6d6cb

Comment on attachment 8339268 [details] [diff] [review] Add ReadBackSurface helper in GLContextUtils Review of attachment 8339268 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/GLContextUtils.h @@ +6,5 @@ > + > +#ifndef GLCONTEXTUTILS_H_ > +#define GLCONTEXTUTILS_H_ > + > +#include "GLContext.h" Reviewer tip: GLContext.h is a very expensive header to include. We just went through the work of stopping including it in other headers where possible. Here including GLContextTypes.h is enough, or even better, just forward-declare GLContext. Fixing it.

https://hg.mozilla.org/mozilla-central/rev/d96c86f2b4d4 https://hg.mozilla.org/mozilla-central/rev/b44b4be6d6cb