Closed Bug 943293 Opened 6 years ago Closed 6 years ago

Add a function to readback a GL texture into a DataSourceSurface

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: nical, Assigned: pehrsons)

References

Details

(Whiteboard: [mentor=nsilva@mozilla.com][lang=c++][qa-])

Attachments

(2 files, 1 obsolete file)

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
Whiteboard: [mentor=nsilva@mozilla.com][lang=c++]
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.
Flags: needinfo?(nical.bugzilla)
Assignee: nobody → pehrsons
Flags: needinfo?(nical.bugzilla)
(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 :)
Attachment #8339172 - Flags: review?(nical.bugzilla)
Attachment #8339171 - Flags: review?(nical.bugzilla) → review+
Attachment #8339172 - Flags: review?(nical.bugzilla) → review+
Looks like you forgot to hg add GLContextUtils.h
Attachment #8339172 - Attachment is obsolete: true
Comment on attachment 8339268 [details] [diff] [review]
Add ReadBackSurface helper in GLContextUtils

Now with GLContextUtils.h ... still getting accustomed to hg.
Attachment #8339268 - Flags: review?(nical.bugzilla)
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.
Attachment #8339268 - Flags: review?(nical.bugzilla) → review+
> 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 =)
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
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Depends on: 944830
Whiteboard: [mentor=nsilva@mozilla.com][lang=c++] → [mentor=nsilva@mozilla.com][lang=c++][qa-]
You need to log in before you can comment on or make changes to this bug.