Closed Bug 570294 Opened 14 years ago Closed 14 years ago

Publish layer trees to other processes (basically, slowly)

Categories

(Core :: Graphics, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: cjones, Assigned: cjones)

References

Details

Attachments

(15 files, 27 obsolete files)

2.45 KB, patch
joe
: review+
Details | Diff | Splinter Review
5.06 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
637.21 KB, image/png
Details
17.23 KB, patch
Details | Diff | Splinter Review
9.95 KB, patch
Details | Diff | Splinter Review
17.38 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
9.95 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
10.35 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
28.74 KB, patch
vlad
: review+
Details | Diff | Splinter Review
23.40 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
11.90 KB, patch
Details | Diff | Splinter Review
8.35 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
14.87 KB, patch
roc
: review+
Details | Diff | Splinter Review
25.26 KB, patch
Details | Diff | Splinter Review
10.57 KB, patch
Details | Diff | Splinter Review
This bug covers implementing the bare essentials of sharing layer trees among one (!) parent/child process pair.  Sharing layer trees "usefully", efficiently, and between >2 levels of processes is work for followup bugs.
(Sneaking this in here.)
Assignee: nobody → jones.chris.g
Attachment #449413 - Flags: review?(joe)
Attached patch part d: IPC protocol for Layers (obsolete) — Splinter Review
Attachment #449416 - Flags: superreview?(roc)
Attachment #449416 - Flags: review?(joe)
Let me know if vlad is a better reviewer of the canvas bits.
Attachment #449419 - Flags: superreview?(roc)
Attachment #449419 - Flags: review?(joe)
Attached patch (For testing) Compositor goop (obsolete) — Splinter Review
Initial stab at a compositor process, used for testing.
With all patches in this bug and its dependencies applied, and the patches in bug 567421, you can set NSPR_LOG_MODULES="Layers:5" to see basic layers updates published to the compositor on MOZ_IPC platforms (all Tier Is).
On Gtk2 systems, this patch allows one to view browser-process widgets synchronized with windows in the compositor process.  (Mostly for personal testing purposes.)
I'll start filing followup bugs for fennectrolysis etc. stuff next week.
Attachment #449413 - Flags: review?(joe) → review+
(I'm going to hold off on filing more dependent bugs until the basic approach of these patches gets at least the equivalent of feedback+.  Don't want to waste anyone's work.)
roc suggested some fundamental API changes that are going to propagate from part c on down.  Mass review cancellation forthcoming.  (Next round, I'll hold off on review requests for the implementation bits until I get sr on the revised API ;) .)

One thing I neglected to mention in comment 0 is that these patches live at
 http://hg.mozilla.org/users/cjones_mozilla.com/mcmq
and depend on a '.hg/patches/layers' directory or symlink containing
 http://hg.mozilla.org/users/rocallahan_mozilla.com/layers-patches/
Attachment #449415 - Attachment is obsolete: true
Attachment #449415 - Flags: superreview?(roc)
Attachment #449415 - Flags: review?(bas.schouten)
Attachment #449417 - Attachment is obsolete: true
Attachment #449417 - Flags: superreview?(roc)
Attachment #449417 - Flags: review?(joe)
Attachment #449418 - Attachment is obsolete: true
Attachment #449418 - Flags: superreview?(roc)
Attachment #449418 - Flags: review?(joe)
Attachment #449419 - Attachment is obsolete: true
Attachment #449419 - Flags: superreview?(roc)
Attachment #449419 - Flags: review?(joe)
Attachment #449420 - Attachment is obsolete: true
Attachment #449420 - Flags: review?(roc)
Attachment #449421 - Attachment is obsolete: true
Attachment #449421 - Flags: review?(roc)
Comment on attachment 449416 [details] [diff] [review]
part d: IPC protocol for Layers

This patch didn't change, but I'll start from scratch for everyone's sanity.
Attachment #449416 - Attachment is obsolete: true
Attachment #449416 - Flags: superreview?(roc)
Attachment #449416 - Flags: review?(joe)
Attachment #449422 - Attachment is obsolete: true
Attachment #449423 - Attachment is obsolete: true
Attachment #449424 - Attachment is obsolete: true
Attachment #450048 - Flags: feedback?(roc)
Attachment #450048 - Flags: feedback?(joe)
Attached patch part d: IPC protocol for Layers (obsolete) — Splinter Review
I'd like to get feedback+ up to here before working on the later patches anymore.
Attachment #450050 - Flags: feedback?(roc)
Attachment #450050 - Flags: feedback?(joe)
Comment on attachment 450048 [details] [diff] [review]
part c: C++ part of Layers IPC interface

Tentative feedback+, depending on the outcome of the protocol discussion and the below discussion.

Something I don't like in here is that GetShadowManager() returns a PLayersChild*. It's not 100% clear to me yet, as I haven't seen all the C++ code, but unless PLayers also implements ShadowLayerManager, there's a mismatch there.
Attachment #450048 - Flags: feedback?(joe) → feedback+
Comment on attachment 450050 [details] [diff] [review]
part d: IPC protocol for Layers

I don't like the Update(Edit[] edits) API. An alternative that I've come up with is having all the individual functions along with BeginUpdate/EndUpdate. This'd allow a simple API with the ability to be atomic at the same time.

This could also be done in a way to allow non-atomicity, with all functions throwing an exception/returning error if BeginUpdate() hadn't been called before, and all operations being queued up until EndUpdate() had been called.

Another option to disallow compound operations would be to have each operation return an Edit, and then have an Update() function that pushed them. That's morally equivalent to what we have now, but at least requires less manual work to set up Edits.
Attachment #450050 - Flags: feedback?(joe) → feedback-
(In reply to comment #28)
> (From update of attachment 450050 [details] [diff] [review])
> I don't like the Update(Edit[] edits) API.

Perhaps this is a semantic issue, but Update() is only used in the implementations of ShadowLayerForwarder and ShadowLayersParent (which receives Update() from Forwarder).  No other code in gfx/layers or elsewhere would use it.  So I'm loath to call it an API.

> An alternative that I've come up
> with is having all the individual functions along with BeginUpdate/EndUpdate.
> This'd allow a simple API with the ability to be atomic at the same time.
> 

What do you mean by "all the individual functions"? --- all the Layers API methods as IPC messages?  By BeginUpdate/EndUpdate, do you have in mind IPC messages that correspond to BeginTransaction/EndTransaction in the layers API?

> This could also be done in a way to allow non-atomicity,

Wait, do we want to allow non-atomicity?

> with all functions
> throwing an exception/returning error if BeginUpdate() hadn't been called
> before, and all operations being queued up until EndUpdate() had been called.
> 

Questions
 - I'm somewhat confused by the above, but if we want to prevent non-atomic transactions, why offer an abusable API when we could prevent atomicity violations statically?
 - what's an "operation"?
 - where are they queued?
 - (see above re: EndUpdate.)

Assuming that
 - an "operation" means what is currently called |Op*|, created in response to a Layers-API IPC message
 - operations are queued on the parent side
 - EndUpdate is an IPC message that means "EndTransaction()" and applies all the queued "operations"
then how does this improve on the current approach?

From my perspective, again assuming I've understand correctly, what's different about this is that the subclassed Layers methods would look like, e.g.

 (i) BasicManager()->SendRemoveChild(...) --or-- (ii) mShadow->SendRemoveChild(...)

rather than

 BasicManager()->AddEdit(OpRemoveChild(...))

For both (i) and (ii) above, the parent side would be more complicated, because we'd need handlers for each Send[operation]() message.  We would still need something akin to the Op* structs to represent the queued operations, and we would still need the giant switch() statement because of the queue.  Additionally, this implementation would be more expensive than the current, because we'd send IPC-message-per-operation rather than IPC-message-per-transaction.  This isn't necessarily an a priori concern since we don't have profiling data, but sending an IPC message isn't free.

Maybe there's a compromise --- if what bothers you with the current approach is that the "batching" implementation detail leaks out of ShadowLayerForwarder, what if we instead gave ShadowLayerForwarder a method-per-Layers-operation, so that we could write

 BasicManager()->RemoveChild(...)

instead of

 BasicManager()->AddEdit(OpRemoveChild(...))

Any better?

> Another option to disallow compound operations would be to have each operation
> return an Edit, and then have an Update() function that pushed them. That's
> morally equivalent to what we have now, but at least requires less manual work
> to set up Edits.

What do you mean by "disallow compound operations?"  What's an "operation" here?  What code would return the Edit?
(In reply to comment #27)
> (From update of attachment 450048 [details] [diff] [review])
> Something I don't like in here is that GetShadowManager() returns a
> PLayersChild*. It's not 100% clear to me yet, as I haven't seen all the C++
> code, but unless PLayers also implements ShadowLayerManager, there's a mismatch
> there.

PLayersChild is just the C++ interface to the child side of the PLayers protocol.  You can think of it as a handle to a ShadowLayerManager in the parent process.  That is, |GetShadowManager()| means "get the handle to the ShadowLayerManager in the parent process".  I agree that this is confusing, but IMHO it's about par for the course wrt IPC.

If you object to "leaking" the PLayersChild* from GetShadowManager() (totally fair), there are certainly alternate solutions to its use at the GetShadowManager() call sites.
(In reply to comment #29)
> (In reply to comment #28)
> > (From update of attachment 450050 [details] [diff] [review] [details])
> > I don't like the Update(Edit[] edits) API.
> 
> Perhaps this is a semantic issue, but Update() is only used in the
> implementations of ShadowLayerForwarder and ShadowLayersParent (which receives
> Update() from Forwarder).  No other code in gfx/layers or elsewhere would use
> it.  So I'm loath to call it an API.

We should be beautiful at the high AND low levels! Shoot for the moon! :)

> > An alternative that I've come up
> > with is having all the individual functions along with BeginUpdate/EndUpdate.
> > This'd allow a simple API with the ability to be atomic at the same time.
> > 
> 
> What do you mean by "all the individual functions"? --- all the Layers API
> methods as IPC messages? 

The things in the Edit union, which are logically operations/functions.

> By BeginUpdate/EndUpdate, do you have in mind IPC
> messages that correspond to BeginTransaction/EndTransaction in the layers API?

Potentially, yes.

> > This could also be done in a way to allow non-atomicity,
> 
> Wait, do we want to allow non-atomicity?

Dunno!

> 
> > with all functions
> > throwing an exception/returning error if BeginUpdate() hadn't been called
> > before, and all operations being queued up until EndUpdate() had been called.
> > 
> 
> Questions
>  - I'm somewhat confused by the above, but if we want to prevent non-atomic
> transactions, why offer an abusable API when we could prevent atomicity
> violations statically?

Because the statically atomic version is harder to use.

>  - what's an "operation"?

An Edit.

>  - where are they queued?

The ether! (the PLayers instance)

>  - (see above re: EndUpdate.)

Not sure what you mean by this.

> Assuming that
>  - an "operation" means what is currently called |Op*|, created in response to
> a Layers-API IPC message
>  - operations are queued on the parent side
>  - EndUpdate is an IPC message that means "EndTransaction()" and applies all
> the queued "operations"
> then how does this improve on the current approach?

A single-operation API, even one used from the low level as this one is, makes it harder to understand what's going on. It's harder to read the code that calls it, harder to read the code that implements it, and necessitates a big switch statement in the middle. I was trying to get rid of the "hard to read the code that calls it" bit.

> Maybe there's a compromise --- if what bothers you with the current approach is
> that the "batching" implementation detail leaks out of ShadowLayerForwarder,
> what if we instead gave ShadowLayerForwarder a method-per-Layers-operation, so
> that we could write
> 
>  BasicManager()->RemoveChild(...)
> 
> instead of
> 
>  BasicManager()->AddEdit(OpRemoveChild(...))
> 
> Any better?

Yes!
 
> > Another option to disallow compound operations would be to have each operation
> > return an Edit, and then have an Update() function that pushed them. That's
> > morally equivalent to what we have now, but at least requires less manual work
> > to set up Edits.
> 
> What do you mean by "disallow compound operations?"  What's an "operation"
> here?  What code would return the Edit?

Here is what happened in the office as I wrote that:
Joe: "What's the opposite of atomic?"
Shaver: "Compound."

"disallow compound operations" = "make everything atomic."

The Edit could be returned by the function itself, so you have:

vector<Edit> edits;
edits.push_back(BasicManager()->RemoveChild(...));
// ...
BasicManager()->Update(edits);

I'm not in love with it, but it's a little better, I guess.

(In reply to comment #30)
> (In reply to comment #27)
> > (From update of attachment 450048 [details] [diff] [review] [details])
> > Something I don't like in here is that GetShadowManager() returns a
> > PLayersChild*. It's not 100% clear to me yet, as I haven't seen all the C++
> > code, but unless PLayers also implements ShadowLayerManager, there's a mismatch
> > there.
> 
> PLayersChild is just the C++ interface to the child side of the PLayers
> protocol.  You can think of it as a handle to a ShadowLayerManager in the
> parent process.  That is, |GetShadowManager()| means "get the handle to the
> ShadowLayerManager in the parent process".  I agree that this is confusing, but
> IMHO it's about par for the course wrt IPC.
> 
> If you object to "leaking" the PLayersChild* from GetShadowManager() (totally
> fair), there are certainly alternate solutions to its use at the
> GetShadowManager() call sites.

You could just call it GetLayers(), and I would have no problem with it. I just don't like calling it GetShadowManager() when it doesn't return anything of the sort.
Attachment #450048 - Attachment is obsolete: true
Attachment #450050 - Attachment is obsolete: true
Attachment #450051 - Attachment is obsolete: true
Attachment #450052 - Attachment is obsolete: true
Attachment #450053 - Attachment is obsolete: true
Attachment #450054 - Attachment is obsolete: true
Attachment #450055 - Attachment is obsolete: true
Attachment #450057 - Attachment is obsolete: true
Attachment #450058 - Attachment is obsolete: true
Attachment #450310 - Flags: feedback?(roc)
Attachment #450310 - Flags: feedback?(joe)
Attachment #450048 - Flags: feedback?(roc)
Attachment #450050 - Flags: feedback?(roc)
Moved all the IPDL-level stuff into ShadowLayers.cpp.
Attachment #450312 - Flags: feedback?(joe)
Attachment #450310 - Flags: feedback?(joe) → feedback+
Comment on attachment 450310 [details] [diff] [review]
part c: C++ part of Layers IPC interface

>+   * XXX should the receiving process blit updates from the new front
>+   * buffer to the previous front buffer (new back buffer) while it has
>+   * access to the new front buffer?  Or is it better to fill the
>+   * updates bits in anew on the new back buffer?

Imagining that the parent process has a texture that represents this layer, it's probably best for us to just do a glTexSubImage2D-alike to publish the dirty rect to the existing layer's representation, rather than overwriting the whole thing or (worse) creating a whole new texture. Plus, if it's software-based, a littler blit is better than a bigger one!
Attachment #450311 - Flags: feedback?(joe) → feedback+
Comment on attachment 450312 [details] [diff] [review]
part e: Implement the "forwarder" side of IPC layers

do like
Attachment #450312 - Flags: feedback?(joe) → feedback+
Attachment #449414 - Flags: superreview?(roc) → superreview+
Oh, I think shadowlayers should be using PRBool instead of bool here, if possible.
Attachment #449414 - Flags: review?(bas.schouten) → review+
Attachment #450312 - Attachment is obsolete: true
Attachment #450313 - Attachment is obsolete: true
Attachment #450314 - Attachment is obsolete: true
Attachment #450316 - Attachment is obsolete: true
Attachment #450317 - Attachment is obsolete: true
Attachment #450318 - Attachment is obsolete: true
Attachment #450319 - Attachment is obsolete: true
Attachment #450320 - Attachment is obsolete: true
Attachment #453666 - Flags: superreview?(vladimir)
Attachment #453666 - Flags: review?
Attachment #453666 - Flags: review? → review?(bas.schouten)
https://developer.mozilla.org/en/IPDL might be useful for this.  (Warning: somewhat out of date.)
Attachment #453667 - Flags: superreview?(vladimir)
Attachment #453667 - Flags: review?(jmuizelaar)
Note: this is a slow impl.  It will be made fast(er) in a followup bug.
Attachment #453671 - Flags: superreview?(vladimir)
Note: this is a slow impl.  It will be made fast(er) in a followup bug.
Attachment #453672 - Flags: review?(bas.schouten)
Note: this is a slow impl.  It will be made fast(er) in a followup bug.

(This applies on top of the refactoring in bug 573829 because it makes things a little cleaner IMHO.)
Attachment #453673 - Flags: review?(roc)
Comment on attachment 453668 [details] [diff] [review]
part e: Implement the "forwarder" side of IPC layers

>+  nsAutoTArray<Edit, 10> cset;
>+  cset.SetCapacity(mTxn->mCset.size());

The SetCapacity is unneeded. SetCapacity and AppendElements both call EnsureCapacity.

I don't really have great picture of how this code will all be used, but overall it seems reasonably uncontroversial to me.
Attachment #453668 - Flags: review?(jmuizelaar) → review+
Comment on attachment 453667 [details] [diff] [review]
part d: IPC protocol for Layers

Likewise.
Attachment #453667 - Flags: review?(jmuizelaar) → review+
The general overview of where this code is headed is
  https://wiki.mozilla.org/Gecko:CrossProcessLayers
and a little more detail lives at
  https://wiki.mozilla.org/Gecko:CrossProcessLayers#Implementation

Not sure if this helps, please ping for more info.
Comment on attachment 453672 [details] [diff] [review]
part i: Publish BasicImageLayer pixels to remote processes (slowly)

One nit: the instances of NULL should probably be nsnull again.

>+void
>+BasicShadowableImageLayer::Paint(gfxContext* aContext,
>+                                 LayerManager::DrawThebesLayerCallback aCallback,
>+                                 void* aCallbackData,
>+                                 float aOpacity)
>+{
>+  gfxIntSize oldSize = mSize;
>+  nsRefPtr<gfxPattern> pat = GetAndPaintCurrentImage(aContext, aOpacity);
>+  if (!pat || !HasShadow())
>+    return;
>+
>+  if (oldSize != mSize) {
>+    NS_ASSERTION(oldSize == gfxIntSize(0, 0), "video changed size?");

As far as I know we allow videos to change size don't we?

>+    if (mFrontSurface)
>+      BasicManager()->ShadowLayerManager::DestroySharedSurface(mFrontSurface);

Nit: We should probably be consistent with the coding style guide and do { } for single line ifs.

>
Attachment #453672 - Flags: review?(bas.schouten) → review+
Comment on attachment 453666 [details] [diff] [review]
part c: C++ part of Layers IPC interface

Looks good to me! Although I dread the day we have to try to make this system fast :-).

>diff --git a/gfx/layers/Makefile.in b/gfx/layers/Makefile.in
>--- a/gfx/layers/Makefile.in
>+++ b/gfx/layers/Makefile.in
>@@ -42,16 +42,20 @@ VPATH       = \
>   $(srcdir) \
>   $(srcdir)/basic \
>   $(srcdir)/opengl \
>   $(srcdir)/d3d9 \
>   $(NULL)
> 
> include $(DEPTH)/config/autoconf.mk
> 
>+ifdef MOZ_IPC
>+DIRS += ipc
>+endif

At the request of Ted and others we did not add more recursive Makefiles (due to their terrible-ness). Is there a reason we can't include the ipc files the same way we do for the other directories?
Attachment #453666 - Flags: review?(bas.schouten) → review+
(In reply to comment #59)
> (From update of attachment 453672 [details] [diff] [review])
> One nit: the instances of NULL should probably be nsnull again.
> 
> >+void
> >+BasicShadowableImageLayer::Paint(gfxContext* aContext,
> >+                                 LayerManager::DrawThebesLayerCallback aCallback,
> >+                                 void* aCallbackData,
> >+                                 float aOpacity)
> >+{
> >+  gfxIntSize oldSize = mSize;
> >+  nsRefPtr<gfxPattern> pat = GetAndPaintCurrentImage(aContext, aOpacity);
> >+  if (!pat || !HasShadow())
> >+    return;
> >+
> >+  if (oldSize != mSize) {
> >+    NS_ASSERTION(oldSize == gfxIntSize(0, 0), "video changed size?");
> 
> As far as I know we allow videos to change size don't we?
> 

I didn't know, left in the assertion to check.  Rob?
(In reply to comment #60)
> (From update of attachment 453666 [details] [diff] [review])
> Looks good to me! Although I dread the day we have to try to make this system
> fast :-).
> 

Why?

> >diff --git a/gfx/layers/Makefile.in b/gfx/layers/Makefile.in
> >--- a/gfx/layers/Makefile.in
> >+++ b/gfx/layers/Makefile.in
> >@@ -42,16 +42,20 @@ VPATH       = \
> >   $(srcdir) \
> >   $(srcdir)/basic \
> >   $(srcdir)/opengl \
> >   $(srcdir)/d3d9 \
> >   $(NULL)
> > 
> > include $(DEPTH)/config/autoconf.mk
> > 
> >+ifdef MOZ_IPC
> >+DIRS += ipc
> >+endif
> 
> At the request of Ted and others we did not add more recursive Makefiles (due
> to their terrible-ness). Is there a reason we can't include the ipc files the
> same way we do for the other directories?

For the shadow layers code we should be fine; I used a subdir to make the IPC enable/disable switch cleaner.  We will need a subdirectory for the compositor process though, because our build system can't handle compiling a library and executable with the same Makefile.
Comment on attachment 453670 [details] [diff] [review]
part g: Implement basic ShadowLayers and share basic layers with remote processes.

Hrm, maybe we should put the BasicShadowable code into a separate file, to prevent this one from getting crowded and to make it easier to look things up.

>+  PLayerChild* shadow = aMgr->ConstructShadowFor(aLayer);
>+  // XXX error handling
>+  NS_ABORT_IF_FALSE(!!shadow, "failed to create shadow");

Not sure how the macro is setup, but I'm pretty sure !! isn't needed here.

>+BasicShadowLayerManager::~BasicShadowLayerManager()
>+{
>+//  if (HasShadowManager())
>+//    PLayersChild::Send__delete__(mShadow);

Presumably this shouldn't be commented out?
Attachment #453670 - Flags: review?(bas.schouten) → review+
(In reply to comment #63)
> (From update of attachment 453670 [details] [diff] [review])
> Hrm, maybe we should put the BasicShadowable code into a separate file, to
> prevent this one from getting crowded and to make it easier to look things up.
> 

That would require a nontrivial amount of reorg, because there's code shared between "regular" and shadow basic layers that's only declared in BasicLayers.cpp.  I'm not against that though, but I think it should be done in a followup.

> >+BasicShadowLayerManager::~BasicShadowLayerManager()
> >+{
> >+//  if (HasShadowManager())
> >+//    PLayersChild::Send__delete__(mShadow);
> 
> Presumably this shouldn't be commented out?

Hm yes.
(In reply to comment #61)
> > As far as I know we allow videos to change size don't we?
> 
> I didn't know, left in the assertion to check.  Rob?

Videos can change size in theory, but we currently don't support that.
(In reply to comment #62)
> (In reply to comment #60)
> > (From update of attachment 453666 [details] [diff] [review] [details])
> > Looks good to me! Although I dread the day we have to try to make this system
> > fast :-).
> > 
> 
> Why?

Can't put my finger on it precisely, it may turn out to be no problem at all! 

It's just a general feeling that moving all this information around in such a performance critical area, and doing it so that we get similar performance to when everything lives in the same memory space will be very difficult. But I'm sure we'll figure it out.
(In reply to comment #64)
> (In reply to comment #63)
> > (From update of attachment 453670 [details] [diff] [review] [details])
> > Hrm, maybe we should put the BasicShadowable code into a separate file, to
> > prevent this one from getting crowded and to make it easier to look things up.
> > 
> 
> That would require a nontrivial amount of reorg, because there's code shared
> between "regular" and shadow basic layers that's only declared in
> BasicLayers.cpp.  I'm not against that though, but I think it should be done in
> a followup.

Sounds good to me!
(In reply to comment #63)
> (From update of attachment 453670 [details] [diff] [review])
> >+BasicShadowLayerManager::~BasicShadowLayerManager()
> >+{
> >+//  if (HasShadowManager())
> >+//    PLayersChild::Send__delete__(mShadow);
> 
> Presumably this shouldn't be commented out?

Ah, now I remember: The problem here is that at this point in the patch series, PLayers is a top-level (unmanaged) protocol, so it doesn't have a __delete__() message (__delete__() doesn't really make sense for top-level actors).  Once PLayers is hooked into PCompositor/PContent/PPlugin, it'll no longer be top-level and so get __delete__().  I added a comment in the source to this effect.
Comment on attachment 453666 [details] [diff] [review]
part c: C++ part of Layers IPC interface

Would like some more documentation on the various members of ShadowLayerForwarder; e.g. what do the various args to Created*Buffer mean, and also document things like that PaintedImage only handles RGB data currently etc.
Attachment #453666 - Flags: superreview?(vladimir) → superreview+
Comment on attachment 453667 [details] [diff] [review]
part d: IPC protocol for Layers


>+struct OpCreateThebesLayer     { PLayer shadow; };
>+struct OpCreateContainerLayer  { PLayer shadow; };
>+struct OpCreateImageLayer      { PLayer shadow; };
>+struct OpCreateColorLayer      { PLayer shadow; };
>+struct OpCreateCanvasLayer     { PLayer shadow; };

Change these guys to "PLayer layer;" and below, as we discussed on irc for clarity

>+// For the "buffer creation" operations, we send a temporary "dummy
>+// front" that doesn't contain valid pixel data.  The OpCreate should
>+// be followed by an OpPaint that will set things straight by sending
>+// the tempFront back to the producer process as the back buffer.

Update "doesn't contain valid pixel data" as per our irc discussion... maybe something like "we send an initial empty front buffer, so that we can swap it back when we update it".  Maybe also s/tempFront/initialFront/ in the structs as well.
Attachment #453667 - Flags: superreview?(vladimir) → superreview+
(In reply to comment #69)
> Comment on attachment 453666 [details] [diff] [review]
> part c: C++ part of Layers IPC interface
> 
> Would like some more documentation on the various members of
> ShadowLayerForwarder; e.g. what do the various args to Created*Buffer mean, and
> also document things like that PaintedImage only handles RGB data currently
> etc.

Done.

(In reply to comment #70)
> Comment on attachment 453667 [details] [diff] [review]
> part d: IPC protocol for Layers
> 
> 
> >+struct OpCreateThebesLayer     { PLayer shadow; };
> >+struct OpCreateContainerLayer  { PLayer shadow; };
> >+struct OpCreateImageLayer      { PLayer shadow; };
> >+struct OpCreateColorLayer      { PLayer shadow; };
> >+struct OpCreateCanvasLayer     { PLayer shadow; };
> 
> Change these guys to "PLayer layer;" and below, as we discussed on irc for
> clarity
> 
> >+// For the "buffer creation" operations, we send a temporary "dummy
> >+// front" that doesn't contain valid pixel data.  The OpCreate should
> >+// be followed by an OpPaint that will set things straight by sending
> >+// the tempFront back to the producer process as the back buffer.
> 
> Update "doesn't contain valid pixel data" as per our irc discussion... maybe
> something like "we send an initial empty front buffer, so that we can swap it
> back when we update it".  Maybe also s/tempFront/initialFront/ in the structs
> as well.

Done.
Comment on attachment 453669 [details] [diff] [review]
part f: Implement the "manager" side of IPC layers

Looks fine, though a few comments:

>+void
>+ShadowLayerParent::Bind(already_AddRefed<Layer> layer)

Hmm, I don't really like the already_AddRefed<Layer> argument here.  It seems pretty weird and isn't really standard usage (looks like the only place we use it as an arg is xpconnect, which isn't really a bastion of good code style :-).

I see that it's called below as Bind(...->Create().get()) -- would you mind if this just became a bare pointer, and you split up the bind function into two lines?  Those lines have ugly indentation anyway as a result of being long, so splitting them up into a nsRefPtr<> assignment and then passing the ptr to Bind would look cleaner anyway.  It involves an extra addref and release, but I don't have a better suggestion...

>+bool
>+ShadowLayerParent::Recv__delete__()
>+{
>+  Layer* layer;
>+  mLayer.forget(&layer);
>+  NS_RELEASE(layer);

What's wrong with |mLayer = nsnull;| ?

>+//--------------------------------------------------
>+// Convenience accessors
>+static ShadowLayerParent*
>+cast(const PLayerParent* in)

hmm, I was going to suggest calling this something like "as_...", but that ... being shadowlayerparent isn't really all that great.  'cast' is pretty generic, but meh, I guess it's local here.

>+bool
>+ShadowLayersParent::RecvUpdate(const nsTArray<Edit>& cset,
>+                               nsTArray<EditReply>* reply)
>+{
>+  MOZ_LAYERS_LOG(("[ParentSide] recieved txn with %d edits", cset.Length()));
>+
>+  EditReplyVector replyv;
>+
>+  layer_manager()->BeginTransactionWithTarget(NULL);
>+
>+  for (EditArray::index_type i = 0; i < cset.Length(); ++i) {
>+    const Edit& edit = cset[i];
>+
>+    switch (edit.type()) {
>+      // Create* ops
>+    case Edit::TOpCreateThebesLayer: {
>+      MOZ_LAYERS_LOG(("[ParentSide] CreateThebesLayer"));
>+
>+      AsShadowLayer(edit.get_OpCreateThebesLayer())->Bind(
>+        layer_manager()->CreateShadowThebesLayer().get());

Yeah, see comment above, I think this would be cleaner (and eaiser to read) split up into twolines, e.g.:

nsRefPtr<mumble> l = layer_manager()->CreateShadowMumbleLayer();
AsShadowLayer(edit.get_OpCreateThebesLayer())->Bind(l);


>+      unused << thebes->Swap(new gfxSharedImageSurface(otb.tempFront()),
>+                             otb.bufferRect(),
>+                             nsIntPoint(0, 0));

I had no idea we had an unused, neat.
Attachment #453669 - Flags: review?(vladimir) → review+
Comment on attachment 453670 [details] [diff] [review]
part g: Implement basic ShadowLayers and share basic layers with remote processes.

>+  BasicShadowLayerManager* BasicManager()
>+  {
>+    return static_cast<BasicShadowLayerManager*>(mManager);
>+  }

This confused me for a while -- call this ShadowManager()?  We know it'll always be Basic already, and calling it BasicManager made me wonder why we were making calls on it from Shadowable layers, since presumably we were already in the layer manager somewhere...

>+class BasicShadowThebesLayer : public ShadowThebesLayer, BasicImplData {
>+public:
>+  BasicShadowThebesLayer(BasicShadowLayerManager* aLayerManager) :
>+    ShadowThebesLayer(aLayerManager, static_cast<BasicImplData*>(this))
>+  {
>+    MOZ_COUNT_CTOR(BasicShadowThebesLayer);
>+  }
>+  virtual ~BasicShadowThebesLayer()
>+  {
>+    MOZ_COUNT_DTOR(BasicShadowThebesLayer);
>+  }
>+
>+  virtual already_AddRefed<gfxSharedImageSurface>
>+  Swap(gfxSharedImageSurface* aNewFront,
>+       const nsIntRect& aBufferRect,
>+       const nsIntPoint& aRotation)
>+  { return NULL; }
>+
>+  virtual void Paint(gfxContext* aContext,
>+                     LayerManager::DrawThebesLayerCallback aCallback,
>+                     void* aCallbackData,
>+                     float aOpacity)
>+  {}

^ I take it that this just isn't implemented yet? (as with the other layer classes at this point)  Maybe add some warnings, so there's no confusion in case someone tries to use them?  Or am I missing something?

>+  aLayer->SetShadow(shadow);
>+  (aMgr->*aMethod)(aLayer);

Yay, instance method pointers!  Now we're having fun!

>+void
>+BasicShadowLayerManager::SetRoot(Layer* aLayer)
>+{
>+  if (HasShadowManager() && mRoot != aLayer)
>+    ShadowLayerForwarder::SetRoot(Hold(aLayer));
>+  BasicLayerManager::SetRoot(aLayer);
>+}

Are there no side effects expected/allowed from setting the root layer?  Seems like we should either do it on both or on neither just to make sure we're consistent, if mRoot == aLayer.

Also, roc would say that you should have { }'s around even the single-line if clauses (here and elsewhere).  I sorta tend to agree; I tend to fight that only for very trivial clause statements (generally only "return X;"), but up to you.
Attachment #453670 - Flags: superreview?(vladimir) → superreview+
Comment on attachment 453671 [details] [diff] [review]
part h: Publish BasicCanvasLayer pixels to remote processes (slowly)

> BasicLayerManager::~BasicLayerManager()
> {
>   NS_ASSERTION(!InTransaction(), "Died during transaction?");
>+
>+  if (mRoot) {
>+    Layer* root;
>+    mRoot.forget(&root);
>+    NS_RELEASE(root);
>+  }

Seriously, why do you hate "mRoot = nsnull;"?  1 line compared to 5! :)

>+already_AddRefed<gfxSharedImageSurface>
>+BasicShadowCanvasLayer::Swap(gfxSharedImageSurface* newFront)
>+{
>+  nsRefPtr<gfxSharedImageSurface> tmp = mFrontSurface;
>+  mFrontSurface = newFront;
>+  return tmp.forget();
>+}

Hmm, I thought you could juse use swap(), but you need to actually take a ref on newFront.  However, I believe this would save you an addref/release:

  gfxSharedImageSurface *tmp = mFrontSurface.forget();
  mFrontSurface = newFront;
  return tmp;
Attachment #453671 - Flags: superreview?(vladimir) → superreview+
(In reply to comment #72)
> Comment on attachment 453669 [details] [diff] [review]
> part f: Implement the "manager" side of IPC layers
> 
> Looks fine, though a few comments:
> 
> >+void
> >+ShadowLayerParent::Bind(already_AddRefed<Layer> layer)
> 
> Hmm, I don't really like the already_AddRefed<Layer> argument here.  It seems
> pretty weird and isn't really standard usage (looks like the only place we use
> it as an arg is xpconnect, which isn't really a bastion of good code style :-).
> 
> I see that it's called below as Bind(...->Create().get()) -- would you mind if
> this just became a bare pointer, and you split up the bind function into two
> lines?  Those lines have ugly indentation anyway as a result of being long, so
> splitting them up into a nsRefPtr<> assignment and then passing the ptr to Bind
> would look cleaner anyway.  It involves an extra addref and release, but I
> don't have a better suggestion...
> 

OK.

> >+bool
> >+ShadowLayerParent::Recv__delete__()
> >+{
> >+  Layer* layer;
> >+  mLayer.forget(&layer);
> >+  NS_RELEASE(layer);
> 
> What's wrong with |mLayer = nsnull;| ?
> 

Oops, |mLayer| used to be a bare pointer, and I didn't fix this code correctly when that changed.  Thanks.

> >+bool
> >+ShadowLayersParent::RecvUpdate(const nsTArray<Edit>& cset,
> >+                               nsTArray<EditReply>* reply)
> >+{
> >+  MOZ_LAYERS_LOG(("[ParentSide] recieved txn with %d edits", cset.Length()));
> >+
> >+  EditReplyVector replyv;
> >+
> >+  layer_manager()->BeginTransactionWithTarget(NULL);
> >+
> >+  for (EditArray::index_type i = 0; i < cset.Length(); ++i) {
> >+    const Edit& edit = cset[i];
> >+
> >+    switch (edit.type()) {
> >+      // Create* ops
> >+    case Edit::TOpCreateThebesLayer: {
> >+      MOZ_LAYERS_LOG(("[ParentSide] CreateThebesLayer"));
> >+
> >+      AsShadowLayer(edit.get_OpCreateThebesLayer())->Bind(
> >+        layer_manager()->CreateShadowThebesLayer().get());
> 
> Yeah, see comment above, I think this would be cleaner (and eaiser to read)
> split up into twolines, e.g.:
> 
> nsRefPtr<mumble> l = layer_manager()->CreateShadowMumbleLayer();
> AsShadowLayer(edit.get_OpCreateThebesLayer())->Bind(l);
> 

OK.

> 
> >+      unused << thebes->Swap(new gfxSharedImageSurface(otb.tempFront()),
> >+                             otb.bufferRect(),
> >+                             nsIntPoint(0, 0));
> 
> I had no idea we had an unused, neat.

FYI, it was created to suppress gcc's __attribute__((warn_unused_result)) warnings, which |(void) foo();| does not.
(In reply to comment #73)
> Comment on attachment 453670 [details] [diff] [review]
> part g: Implement basic ShadowLayers and share basic layers with remote
> processes.
> 
> >+  BasicShadowLayerManager* BasicManager()
> >+  {
> >+    return static_cast<BasicShadowLayerManager*>(mManager);
> >+  }
> 
> This confused me for a while -- call this ShadowManager()?  We know it'll
> always be Basic already, and calling it BasicManager made me wonder why we were
> making calls on it from Shadowable layers, since presumably we were already in
> the layer manager somewhere...
> 

OK.

> >+class BasicShadowThebesLayer : public ShadowThebesLayer, BasicImplData {
> >+public:
> >+  BasicShadowThebesLayer(BasicShadowLayerManager* aLayerManager) :
> >+    ShadowThebesLayer(aLayerManager, static_cast<BasicImplData*>(this))
> >+  {
> >+    MOZ_COUNT_CTOR(BasicShadowThebesLayer);
> >+  }
> >+  virtual ~BasicShadowThebesLayer()
> >+  {
> >+    MOZ_COUNT_DTOR(BasicShadowThebesLayer);
> >+  }
> >+
> >+  virtual already_AddRefed<gfxSharedImageSurface>
> >+  Swap(gfxSharedImageSurface* aNewFront,
> >+       const nsIntRect& aBufferRect,
> >+       const nsIntPoint& aRotation)
> >+  { return NULL; }
> >+
> >+  virtual void Paint(gfxContext* aContext,
> >+                     LayerManager::DrawThebesLayerCallback aCallback,
> >+                     void* aCallbackData,
> >+                     float aOpacity)
> >+  {}
> 
> ^ I take it that this just isn't implemented yet? (as with the other layer
> classes at this point)  Maybe add some warnings, so there's no confusion in
> case someone tries to use them?  Or am I missing something?
> 

It's implemented in part j, and the other Paint() methods are in parts h and i.

> >+  aLayer->SetShadow(shadow);
> >+  (aMgr->*aMethod)(aLayer);
> 
> Yay, instance method pointers!  Now we're having fun!
> 

Templated, and generated by a macro too!  Triple word score for me.

> >+void
> >+BasicShadowLayerManager::SetRoot(Layer* aLayer)
> >+{
> >+  if (HasShadowManager() && mRoot != aLayer)
> >+    ShadowLayerForwarder::SetRoot(Hold(aLayer));
> >+  BasicLayerManager::SetRoot(aLayer);
> >+}
> 
> Are there no side effects expected/allowed from setting the root layer?  Seems
> like we should either do it on both or on neither just to make sure we're
> consistent, if mRoot == aLayer.
> 

No, but ...  What this code is doing is trying to help a transaction-forwarding optimization: if nothing changed between BeginUpdate()/EndUpdate(), then the sync Update() to compositor isn't done.  The sync Update() isn't free by any means, so it would be a shame to pay that for what reduces to a no-op.  I'll put the |mRoot = aLayer;| assignment inside the if block there.

> Also, roc would say that you should have { }'s around even the single-line if
> clauses (here and elsewhere).  I sorta tend to agree; I tend to fight that only
> for very trivial clause statements (generally only "return X;"), but up to you.

That's fine, when in Rome and so forth.
(In reply to comment #74)
> Comment on attachment 453671 [details] [diff] [review]
> part h: Publish BasicCanvasLayer pixels to remote processes (slowly)
> 
> > BasicLayerManager::~BasicLayerManager()
> > {
> >   NS_ASSERTION(!InTransaction(), "Died during transaction?");
> >+
> >+  if (mRoot) {
> >+    Layer* root;
> >+    mRoot.forget(&root);
> >+    NS_RELEASE(root);
> >+  }
> 
> Seriously, why do you hate "mRoot = nsnull;"?  1 line compared to 5! :)
> 

Haha, I don't know what I was thinking here.

> >+already_AddRefed<gfxSharedImageSurface>
> >+BasicShadowCanvasLayer::Swap(gfxSharedImageSurface* newFront)
> >+{
> >+  nsRefPtr<gfxSharedImageSurface> tmp = mFrontSurface;
> >+  mFrontSurface = newFront;
> >+  return tmp.forget();
> >+}
> 
> Hmm, I thought you could juse use swap(), but you need to actually take a ref
> on newFront.  However, I believe this would save you an addref/release:
> 
>   gfxSharedImageSurface *tmp = mFrontSurface.forget();
>   mFrontSurface = newFront;
>   return tmp;

Wfm.
FTR, bug 570625, bug 580781, and bug 580784 are filed on making the pixel forwarding not suck (for 2D content).
No longer depends on: 598874
You need to log in before you can comment on or make changes to this bug.