Last Comment Bug 740557 - Add a Copy-On-Write surface wrapper (gfxReusableSurface)
: Add a Copy-On-Write surface wrapper (gfxReusableSurface)
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Mac OS X
-- normal (vote)
: mozilla14
Assigned To: Benoit Girard (:BenWa)
: Milan Sreckovic [:milan]
Depends on:
Blocks: 739679
  Show dependency treegraph
Reported: 2012-03-29 12:49 PDT by Benoit Girard (:BenWa)
Modified: 2012-04-19 12:29 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (5.47 KB, patch)
2012-03-29 12:49 PDT, Benoit Girard (:BenWa)
jmuizelaar: review-
Details | Diff | Splinter Review
patch v2 (5.94 KB, patch)
2012-04-12 14:03 PDT, Benoit Girard (:BenWa)
jmuizelaar: review-
Details | Diff | Splinter Review
patch-v3 (6.38 KB, patch)
2012-04-17 16:23 PDT, Benoit Girard (:BenWa)
jmuizelaar: review+
Details | Diff | Splinter Review

Description User image Benoit Girard (:BenWa) 2012-03-29 12:49:46 PDT
Created attachment 610652 [details] [diff] [review]

This surface is meant to be used for sharing copy on write surface between content and the compositor.
Comment 1 User image Jeff Muizelaar [:jrmuizel] 2012-03-29 13:35:15 PDT
Comment on attachment 610652 [details] [diff] [review]

I'm not thrilled with some of the naming, but don't have a bunch of suggestions.

We should do away with the WriteLock stuff, it doesn't seem to be needed.

One of the things that's not clear about this stuff is that it needs to be used in a rather restrictive way. i.e. all ReadLocking has to happen on the same thread as GetWritable. I'm not sure how to make this more obvious in the interface, but it should be at least documented.

Further, we shouldn't allow access to the gfxImageSurface from a different thread. I suggest that we only have methods for decreasing the read count and getting the data/size/stride.

We can't delete gfxImageSurface from another thread, and I'm not sure how the gfxImageSurface's reference is managed by the gfxCoWSurfaceWrapper.

Another thing I don't like is the sort of duplication with reference count and the read count.
Comment 2 User image Benoit Girard (:BenWa) 2012-04-12 14:03:00 PDT
Created attachment 614555 [details] [diff] [review]
patch v2
Comment 3 User image Benoit Girard (:BenWa) 2012-04-17 10:44:18 PDT
review ping
Comment 4 User image Jeff Muizelaar [:jrmuizel] 2012-04-17 15:35:46 PDT
Comment on attachment 614555 [details] [diff] [review]
patch v2

Review of attachment 614555 [details] [diff] [review]:

You'll need to make sure you delete this on the main thread.

::: gfx/thebes/gfxReusableSurfaceWrapper.cpp
@@ +21,5 @@
> +{
> +  NS_ABORT_IF_FALSE(mReadCount == 0, "Should not be locked when released");
> +  MOZ_COUNT_DTOR(gfxReusableSurfaceWrapper);
> +  oC--;
> +  printf_stderr("oC %u\n", oC);

Make sure you take this out.

@@ +48,5 @@
> +    return this;
> +  }
> +
> +  // Something else is reading the surface, copy it
> +  gfxImageSurface* copySurface = new gfxImageSurface(mSurface->GetSize(), mSurface->Format());

You shouldn't initialize this surface.

::: gfx/thebes/gfxReusableSurfaceWrapper.h
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at */
> +

Fix the name here
Comment 5 User image Benoit Girard (:BenWa) 2012-04-17 16:23:04 PDT
Created attachment 615937 [details] [diff] [review]
Comment 8 User image Benoit Girard (:BenWa) 2012-04-19 12:29:03 PDT
GFXCOWSURFACEWRAPPER didn't get fixed (must of not got qref properly), still need to fix this.

Note You need to log in before you can comment on or make changes to this bug.