1465466 - Web Replay: Show graphics when recording/replaying

Status: RESOLVED FIXED

(Core :: Graphics, enhancement)

Description

[Brian Hackett [Laid off!]]

Handling of graphics updates in Web Replay has been completely rewritten from what is in bug 1207696.  The new design places a compositor in the recording/replaying process which does all rendering for the tab and puts it in a buffer shared with the middleman process.  The middleman then relays the contents of that buffer to the compositor in the UI process via some simple layers IPDL.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: Part 1 - Use a separate compositor in recording/replaying processes.

Create an in-process compositor in recording/replaying processes.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: Part 2 - Synchronously composite into a graphics shmem buffer when recording/replaying.

When painting in a recording/replaying process, synchronously wait for the compositor thread to complete its paint before sending updates to the middleman.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: Part 3 - When recording/replaying, composite in the main thread's event loop instead of waiting for vsyncs.

Rendering based on vsync messages sent from the parent causes problems when the page is updating the DOM very fast: some updates might not be rendered, so when rewinding the graphics shown on screen do not reflect that point in the tab's execution.  With this patch, vsync messages are ignored and we trigger an artificial vsync at every turn of the main thread's event loop.

Comment 4

[Brian Hackett [Laid off!]]

Attachment: Part 4 - Send compositor updates to the UI process from the middleman.

Generate layers messages in the middleman to update graphics in the UI process whenever a paint message is received from the recording/replaying child.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: Part 5 - Don't use APZ in recording/replaying processes.

The layers messages sent by the middleman do not work with APZ in the parent process --- a single layer is sent which only covers the visible region.  This patch unsets APZ flags in GUI events sent to a recording/replaying process, allowing scrolling etc. to work.

Comment 6

[Nicolas Silva [:nical]]

Hey Brian.
I am starting to look into this, it'll take a little while for this to sink in because it's a busy time for the gfx team, but don't hesitate to ping me if you end up in a situation where your work is blocked on these reviews.
So far I appreciate the simplification of the graphics instrumentation in this version.

Comment 7

[Brian Hackett [Laid off!]]

Hi, do you know when you'll be able to look at these patches?

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8981883 [details] [diff] [review]
Part 5 - Don't use APZ in recording/replaying processes.

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

Clearing the flag like this is going to leave APZ in a bad state because it will be expecting responses that it's not going to get. It would be better to just not send the input events through APZ in the first place. I'm also missing context on how the web record/replay mechanism works in general since I didn't review any of the previous work on it - is it something that can be turned on and off while the browser is running, or is it a "run the browser in record mode" kind of thing?

Comment 9

[Nicolas Silva [:nical]]

kats: the webreplay architecture is described here: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/WebReplay thanks for chiming in.

Brian: sorry for the delay, looking into it now.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Also, technical details aside, what is the plan for this feature in terms of shipping? It introduces a huge amount of complexity, and is the sort of thing that should normally require multiple reviewers as it covers a large area of the graphics surface area. However it's hard to justify spending that kind of time on reviewing this code if it's not going to actually ship to end users.

Comment 11

[Brian Hackett [Laid off!]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> Also, technical details aside, what is the plan for this feature in terms of
> shipping? It introduces a huge amount of complexity, and is the sort of
> thing that should normally require multiple reviewers as it covers a large
> area of the graphics surface area. However it's hard to justify spending
> that kind of time on reviewing this code if it's not going to actually ship
> to end users.

Once the reviews are completed (see bug 1422587 and its dependent bugs) web replay will land and initially ship to nightly Mac users.  Eventually we'll support other platforms and releases, but need to take one step at a time.

Feel free to reassign reviews if you want, I don't know who would best know the different parts of the graphics code this is touching.  As for complexity, which parts are you concerned with?  I've been trying pretty hard to minimize the changes needed to Gecko to support web replay, and feel that the changes to the graphics code are much simpler than they were in bug 1207696, but I'm sure there are more simplifications that could be done.

Comment 12

[Brian Hackett [Laid off!]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> Comment on attachment 8981883 [details] [diff] [review]
> Part 5 - Don't use APZ in recording/replaying processes.
> 
> Review of attachment 8981883 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Clearing the flag like this is going to leave APZ in a bad state because it
> will be expecting responses that it's not going to get. It would be better
> to just not send the input events through APZ in the first place. I'm also
> missing context on how the web record/replay mechanism works in general
> since I didn't review any of the previous work on it - is it something that
> can be turned on and off while the browser is running, or is it a "run the
> browser in record mode" kind of thing?

Whether we are recording or not is specified at the level of content processes: only content processes can be recorded, and a UI process might be working with both recorded and unrecorded content processes.  The ContentParent knows whether its child is being recorded, but so far we haven't had to make any modifications to the UI process' behavior based on whether a child is recording or not.  That might need to change to deal with APZ, though, I'll work on a new patch.

Comment 13

[Brian Hackett [Laid off!]]

Attachment: Part 5 - Don't use APZ when recording/replaying tabs are rendered.

This patch ignores input events in APZ whenever any recording/replaying tabs are being rendered.  It would be nice to do this without the global TabParent state but I didn't see a simple way to determine which tab the input event will eventually go to.  The layer ID for the AsyncPanZoomController seemed like one option but (a) that would need to be tested in several different places, and (b) there isn't actually an AsyncPanZoomController created for the recording/replaying tab, since its layers are not treated by APZ as scrollable.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

(In reply to Brian Hackett (:bhackett) from comment #13)
> This patch ignores input events in APZ whenever any recording/replaying tabs
> are being rendered.  It would be nice to do this without the global
> TabParent state but I didn't see a simple way to determine which tab the
> input event will eventually go to. 

Yeah I guess for your purposes it's a bit of a catch-22 problem. You can't determine which tab the input event is going to go to without doing the hit-testing stuff and that happens after/during APZ processing of the input event.

> The layer ID for the
> AsyncPanZoomController seemed like one option but (a) that would need to be
> tested in several different places, and (b) there isn't actually an
> AsyncPanZoomController created for the recording/replaying tab, since its
> layers are not treated by APZ as scrollable.

Ok. In that case, since this patch effectively disables APZ when there are any record/replaying tabs, it might be cleaner to just return false from gfxPlatform::AsyncPanZoomEnabled(). That will disable all of the APZ machinery more reliably. Your current patch also has the issue that on Windows with the GPU process, ContentParent isn't in the same process as APZCTreeManager, so what you have now wouldn't work there. Since you're only targeting Mac for the first version it's not a big deal but disabling it in gfxPlatform::AsyncPanZoomEnabled() would solve that problem too. What do you think?

Comment 15

[Brian Hackett [Laid off!]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #14)
> > The layer ID for the
> > AsyncPanZoomController seemed like one option but (a) that would need to be
> > tested in several different places, and (b) there isn't actually an
> > AsyncPanZoomController created for the recording/replaying tab, since its
> > layers are not treated by APZ as scrollable.
> 
> Ok. In that case, since this patch effectively disables APZ when there are
> any record/replaying tabs, it might be cleaner to just return false from
> gfxPlatform::AsyncPanZoomEnabled(). That will disable all of the APZ
> machinery more reliably. Your current patch also has the issue that on
> Windows with the GPU process, ContentParent isn't in the same process as
> APZCTreeManager, so what you have now wouldn't work there. Since you're only
> targeting Mac for the first version it's not a big deal but disabling it in
> gfxPlatform::AsyncPanZoomEnabled() would solve that problem too. What do you
> think?

Hmm, I just tried changing AsyncPanZoomEnabled() as you suggest but ran into a couple problems:

- When opening a window without a recording/replaying tab and then starting a recording/replaying tab, we still go through APZCTreeManager::ReceiveInputEvent and the recording/replaying tab receives input events with mHandledByAPZ set.

- When starting a recording/replaying tab, opening a new window, and then closing the recording/replaying tab, events on the new window do *not* go through APZCTreeManager::ReceiveInputEvent, as if APZ state was never (and will not be) set up for that new window.

It seems to me that changing AsyncPanZoomEnabled() to return false is too heavyweight for this application; we want the APZ machinery to be set up as normal, but we don't want input events sent to recording/replaying tabs to go through APZ, so that they can handle scrolling etc. internally.

Comment 16

[Brian Hackett [Laid off!]]

Attachment: Part 5 - Don't use APZ when recording/replaying tabs are rendered.

This patch is still based on APZCTreeManager::ReceiveInputEvent, but tidies up the TabParent implementation a bit and makes sure that when recording/replaying tabs are destroyed they remove themselves from the counter.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8988335 [details] [diff] [review]
Part 5 - Don't use APZ when recording/replaying tabs are rendered.

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

r+ for the APZ side.

::: dom/ipc/TabParent.cpp
@@ +2928,5 @@
>    // RenderLayers requests are ignored.
>    mLayerTreeEpoch++;
>  
> +  if (Manager()->AsContentParent()->IsRecordingOrReplaying()) {
> +    SetIsVisibleRecordReplayTab(aEnabled);

I don't know if SetRenderLayers is the right place for this, since that gets set to true even for tabs that are not actually visible (e.g. during tab warming). f?mconley for if he has a better place to put this.

Comment 18

[Nicolas Silva [:nical]]

Comment on attachment 8981882 [details] [diff] [review]
Part 4 - Send compositor updates to the UI process from the middleman.

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

::: toolkit/recordreplay/ipc/ParentGraphics.cpp
@@ +127,5 @@
> +
> +#define TRY(op) do { if (!(op)) MOZ_CRASH(#op); } while (false)
> +
> +static void
> +UpdateBrowserGraphics(dom::TabChild* aBrowser, const PaintMessage& aMsg)

I am not super fond of manipulating the layers protocol. It'd be easy for simple modifications anywhere in the layers code to break this. Have you tried creating an actual layer tree?

Comment 19

[Nicolas Silva [:nical]]

About the vsync stuff: I gave it a "let's not block experiments on this" sort of r+. If WebReplay turns into a long term feature it would be worth figuring out a way to not have WebReplay specific logic/requirements/assumptions there, because scheduling frames is subtly complicated and has implications in all parts of the graphics stack. ASAP compositing exists for testing purposes so if it has bugs we would normally not mind too much as long as it doesn't prevent us from testing what we use it for.
My naive reaction is that we should be able to throttle or frame production from the replaying side or drop frames if they come in too fast. After all we already do that for normal web content. I'm sure the reality is more complicated than that but it's probably something to add to the list of things to figure out sooner or later.

It's not clear from this set of patches what your plan is for WebRender. Unfortunately there's a whole bunch of Layers specific code which won't work if WebRender is enabled so I suppose your initial implementation will disable the record/replay functionality if WebRender is enabled? I think that WebRender should be easier to integrate with a record/replay infrastructure (since it's based on recording the display list and moving all of paining to the compositor side), but there's going to be a lot of things to re-implement since it interfaces with the compositor very differently.

Sorry again for the delay.

Comment 20

[Brian Hackett [Laid off!]]

(In reply to Nicolas Silva [:nical] from comment #18)
> Comment on attachment 8981882 [details] [diff] [review]
> Part 4 - Send compositor updates to the UI process from the middleman.
> 
> Review of attachment 8981882 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/recordreplay/ipc/ParentGraphics.cpp
> @@ +127,5 @@
> > +
> > +#define TRY(op) do { if (!(op)) MOZ_CRASH(#op); } while (false)
> > +
> > +static void
> > +UpdateBrowserGraphics(dom::TabChild* aBrowser, const PaintMessage& aMsg)
> 
> I am not super fond of manipulating the layers protocol. It'd be easy for
> simple modifications anywhere in the layers code to break this. Have you
> tried creating an actual layer tree?

I haven't, though I don't think I understand the difference between the layers we're constructing here and a typical content layer tree.

One idea I had was to avoid dealing with layers messages entirely in this code by creating a <canvas> in the middleman process, into which we can blit the graphics data and send the layers and painting data to the UI process via the normal rendering paths.  This seems like a more stable design in the long run, and I'll experiment and see if I can get it to work.

Comment 21

[Nicolas Silva [:nical]]

> I haven't, though I don't think I understand the difference between the layers we're constructing here and a typical content layer tree.

I mean creating a ClientLayerManager, ClientContainerLayer, etc which would internally do the stuff your patch does and have the same structure. This is just a matter of manipulating the higher level of abstraction and avoid touching the internal details of the layers protocols.

Comment 22

[Brian Hackett [Laid off!]]

(In reply to Nicolas Silva [:nical] from comment #19)
> About the vsync stuff: I gave it a "let's not block experiments on this"
> sort of r+. If WebReplay turns into a long term feature it would be worth
> figuring out a way to not have WebReplay specific
> logic/requirements/assumptions there, because scheduling frames is subtly
> complicated and has implications in all parts of the graphics stack. ASAP
> compositing exists for testing purposes so if it has bugs we would normally
> not mind too much as long as it doesn't prevent us from testing what we use
> it for.
> My naive reaction is that we should be able to throttle or frame production
> from the replaying side or drop frames if they come in too fast. After all
> we already do that for normal web content. I'm sure the reality is more
> complicated than that but it's probably something to add to the list of
> things to figure out sooner or later.

Well, from the user's perspective, dropping frames when recording/replaying is problematic because rewinding can expose the fact that frames were dropped: you can rewind to a particular point where you expect to see certain graphics on the screen, but if the frame showing those graphics was dropped and you see an earlier frame instead, it gets pretty confusing.  Is there a better way, maybe, to make sure we don't drop frames?

> It's not clear from this set of patches what your plan is for WebRender.
> Unfortunately there's a whole bunch of Layers specific code which won't work
> if WebRender is enabled so I suppose your initial implementation will
> disable the record/replay functionality if WebRender is enabled? I think
> that WebRender should be easier to integrate with a record/replay
> infrastructure (since it's based on recording the display list and moving
> all of paining to the compositor side), but there's going to be a lot of
> things to re-implement since it interfaces with the compositor very
> differently.

Right now there isn't a plan for WebRender.  I have a pretty limited understanding of WebRender (I read http://www.masonchang.com/blog/2016/7/18/a-short-walkthrough-of-webrender-2, which was linked from https://wiki.mozilla.org/Platform/GFX/Quantum_Render#On_WebRender) and I don't know how it interacts with e10s.  Is the issue that when WebRender is used a different IPDL/IPC mechanism is used to convey graphics data from the content process to the UI/GPU process?

What I'm hoping will work in the long term is that we'll continue to be able to use a BasicCompositor or similar (not one that uses OpenGL or interacts with the GPU) in the recording/replaying process, and have code in the middleman to send the rendered data to the UI/GPU process that will work regardless of what graphics options are set.  Using normal pathways in the middleman to render that data should help with the latter, i.e. the <canvas> idea from comment 20.

Comment 23

[Brian Hackett [Laid off!]]

(In reply to Nicolas Silva [:nical] from comment #21)
> > I haven't, though I don't think I understand the difference between the layers we're constructing here and a typical content layer tree.
> 
> I mean creating a ClientLayerManager, ClientContainerLayer, etc which would
> internally do the stuff your patch does and have the same structure. This is
> just a matter of manipulating the higher level of abstraction and avoid
> touching the internal details of the layers protocols.

OK, thanks, I'll try the <canvas> idea first and if that doesn't work I'll rewrite this patch to use these classes rather than sending raw layer messages.

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8988335 [details] [diff] [review]
Part 5 - Don't use APZ when recording/replaying tabs are rendered.

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

::: dom/ipc/TabParent.cpp
@@ +2928,5 @@
>    // RenderLayers requests are ignored.
>    mLayerTreeEpoch++;
>  
> +  if (Manager()->AsContentParent()->IsRecordingOrReplaying()) {
> +    SetIsVisibleRecordReplayTab(aEnabled);

kats is right - we'll sometimes do the tab rendering and uploading even if the tab hasn't moved into the foreground.

A better place might be here: https://searchfox.org/mozilla-central/source/dom/ipc/TabChild.cpp#2558

The DocShell is only supposed to become active once it reaches the foreground.

Comment 25

[Brian Hackett [Laid off!]]

Attachment: Part 5 - Don't use APZ when recording/replaying tabs are rendered.

Thanks, here is an updated patch, r? for the DOM changes (the APZ stuff is the same).

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8989554 [details] [diff] [review]
Part 5 - Don't use APZ when recording/replaying tabs are rendered.

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

TabParent changes seem reasonable to me. Thanks!

Comment 27

[Brian Hackett [Laid off!]]

Attachment: Part 4 - Composite using a canvas in the middleman process.

This patch avoids all the manipulation of graphics layers in the previous patch by running a JS module in the middleman process which fills in a canvas element using the data which the child process supplied.

Comment 28

[Andrew McCreight [:mccr8]]

Comment on attachment 8990043 [details] [diff] [review]
Part 4 - Composite using a canvas in the middleman process.

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

Nical, could you please review Channel.h and ParentGraphics.cpp? For the latter, specifically the method UpdateGraphicsInUIProcess, starting from "size_t width = gLastPaint.ref().mWidth" down until "JSObject* bufferObject =". Thanks.

::: devtools/server/actors/replay/graphics.js
@@ +25,5 @@
> +
> +  canvas.width = width;
> +  canvas.height = height;
> +
> +  // If there is a scale for this window, then the graphics will already have

I don't know anything about this kind of code, so if you want a good review of it you'll have to ask somebody else.

@@ +47,5 @@
> +// to draw.
> +function Update(buffer, width, height)
> +{
> +  try {
> +    // Paint to all windows we can find. Hopefully there is only one.

I guess the other parts of WebReplay enforce that there's only a single top level window in the middleman process? Should there be some error or exception if there's more than one?

::: toolkit/recordreplay/ipc/ParentForwarding.cpp
@@ +107,5 @@
>        type == dom::PBrowser::Msg_PRenderFrameConstructor__ID ||
>        type == dom::PBrowser::Msg_RenderLayers__ID ||
> +      type == dom::PBrowser::Msg_UpdateDimensions__ID ||
> +      // This message performs some graphics related initialization.
> +      type == dom::PBrowser::Msg_LoadURL__ID ||

What's going on with these changes? I don't see any additional code that deals with these messages. Was this just something missing from another patch?

::: toolkit/recordreplay/ipc/ParentGraphics.cpp
@@ +89,5 @@
> +  }
> +
> +  // Make sure there is a sandbox which is running the graphics JS module.
> +  if (!gGraphicsSandbox) {
> +    dom::AutoJSAPI jsapi;

Please wrap this into a static InitGraphicSandbox method.

@@ +118,5 @@
> +  }
> +
> +  AutoSafeJSContext cx;
> +  JSAutoRealm ac(cx, *gGraphicsSandbox);
> +

A graphics person should really review this code starting from around here.

@@ +123,5 @@
> +  size_t width = gLastPaint.ref().mWidth;
> +  size_t height = gLastPaint.ref().mHeight;
> +
> +  size_t stride = layers::ImageDataSerializer::ComputeRGBStride(gSurfaceFormat, width);
> +  MOZ_RELEASE_ASSERT(width * 4 <= stride);

This is all derived from a message from the other process, right? I think you should use CheckedInt all over the place because there's lots of opportunities for integer overflow here.

@@ +124,5 @@
> +  size_t height = gLastPaint.ref().mHeight;
> +
> +  size_t stride = layers::ImageDataSerializer::ComputeRGBStride(gSurfaceFormat, width);
> +  MOZ_RELEASE_ASSERT(width * 4 <= stride);
> +  MOZ_RELEASE_ASSERT(height * stride <= GraphicsMemorySize);

I assume GraphicsMemorySize also is the size of gGraphicsMemory?

Comment 29

[Nicolas Silva [:nical]]

Comment on attachment 8990043 [details] [diff] [review]
Part 4 - Composite using a canvas in the middleman process.

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

::: toolkit/recordreplay/ipc/ParentGraphics.cpp
@@ +133,5 @@
> +  // sent us.
> +  void* memory = gGraphicsMemory;
> +  if (stride != width * 4) {
> +    if (!gBufferMemory) {
> +      gBufferMemory = malloc(GraphicsMemorySize);

I suppose this is actually gGraphicsMemorySize?

@@ +137,5 @@
> +      gBufferMemory = malloc(GraphicsMemorySize);
> +    }
> +    memory = gBufferMemory;
> +    for (size_t y = 0; y < height; y++) {
> +      char* src = (char*)gGraphicsMemory + y * stride;

It looks like we need so form of synchronization for the access to some the globals since they are accessed from multiple threads (If only to please TSAN and friends).

@@ +141,5 @@
> +      char* src = (char*)gGraphicsMemory + y * stride;
> +      char* dst = (char*)gBufferMemory + y * width * 4;
> +      memcpy(dst, src, width * 4);
> +    }
> +  }

Please add checks that the various global pointers gGraphicsMemory aren't null before reading/writing to them.
I'd also feel better if we had checks that the allocation gBufferMemory points to is big enough (it's not obvious to me what enforces that right now).
What deallocates gBufferMemory?

@@ +144,5 @@
> +    }
> +  }
> +
> +  JSObject* bufferObject =
> +    JS_NewArrayBufferWithExternalContents(cx, width * height * 4, memory);

What's the life cycle of this guy? It hangs on to a buffer it doesn't own, is there a guarantee that it will be desroyed before the buffer?

Comment 30

[Brian Hackett [Laid off!]]

(In reply to Andrew McCreight [:mccr8] from comment #28)
> @@ +47,5 @@
> > +// to draw.
> > +function Update(buffer, width, height)
> > +{
> > +  try {
> > +    // Paint to all windows we can find. Hopefully there is only one.
> 
> I guess the other parts of WebReplay enforce that there's only a single top
> level window in the middleman process? Should there be some error or
> exception if there's more than one?

We don't enforce this, but right now the focus is on recording/replaying content processes with a single tab, which (I think) should only have one window.  This code is being defensive because the previous version of this patch had some strange issues I don't understand where a content process with one tab would still have multiple PBrowserChild instances; it wasn't obvious how to determine which one to paint to so we just painted to all the browsers we could find.

> ::: toolkit/recordreplay/ipc/ParentForwarding.cpp
> @@ +107,5 @@
> >        type == dom::PBrowser::Msg_PRenderFrameConstructor__ID ||
> >        type == dom::PBrowser::Msg_RenderLayers__ID ||
> > +      type == dom::PBrowser::Msg_UpdateDimensions__ID ||
> > +      // This message performs some graphics related initialization.
> > +      type == dom::PBrowser::Msg_LoadURL__ID ||
> 
> What's going on with these changes? I don't see any additional code that
> deals with these messages. Was this just something missing from another
> patch?

These messages need to be received by the TabChild in the middleman process so that its window can be rendered correctly.  UpdateDimensions sets the dimensions which the puppet widget uses, and LoadURL includes a ShowInfo with scale and other rendering information.  With the previous version of the patch we never rendered the window in the middleman process (we constructed layers messages manually and sent those to the UI process directly).

> @@ +124,5 @@
> > +  size_t height = gLastPaint.ref().mHeight;
> > +
> > +  size_t stride = layers::ImageDataSerializer::ComputeRGBStride(gSurfaceFormat, width);
> > +  MOZ_RELEASE_ASSERT(width * 4 <= stride);
> > +  MOZ_RELEASE_ASSERT(height * stride <= GraphicsMemorySize);
> 
> I assume GraphicsMemorySize also is the size of gGraphicsMemory?

Yes, this is correct.

Comment 31

[Brian Hackett [Laid off!]]

(In reply to Nicolas Silva [:nical] from comment #29)
> Comment on attachment 8990043 [details] [diff] [review]
> Part 4 - Composite using a canvas in the middleman process.
> 
> Review of attachment 8990043 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/recordreplay/ipc/ParentGraphics.cpp
> @@ +133,5 @@
> > +  // sent us.
> > +  void* memory = gGraphicsMemory;
> > +  if (stride != width * 4) {
> > +    if (!gBufferMemory) {
> > +      gBufferMemory = malloc(GraphicsMemorySize);
> 
> I suppose this is actually gGraphicsMemorySize?

No, GraphicsMemorySize is the fixed size of the shmem buffer used to send graphics data from the child to the middleman process (4096 * 4096 * 4).  I'll rename gGraphicsMemorySize in ChildIPC.cpp to be less confusing.

> @@ +137,5 @@
> > +      gBufferMemory = malloc(GraphicsMemorySize);
> > +    }
> > +    memory = gBufferMemory;
> > +    for (size_t y = 0; y < height; y++) {
> > +      char* src = (char*)gGraphicsMemory + y * stride;
> 
> It looks like we need so form of synchronization for the access to some the
> globals since they are accessed from multiple threads (If only to please
> TSAN and friends).

Hmm, these globals should only be accessed on the main thread.  I'll add more assertions.

> @@ +141,5 @@
> > +      char* src = (char*)gGraphicsMemory + y * stride;
> > +      char* dst = (char*)gBufferMemory + y * width * 4;
> > +      memcpy(dst, src, width * 4);
> > +    }
> > +  }
> 
> Please add checks that the various global pointers gGraphicsMemory aren't
> null before reading/writing to them.
> I'd also feel better if we had checks that the allocation gBufferMemory
> points to is big enough (it's not obvious to me what enforces that right
> now).

The check that gGraphicsMemory (and gBufferMemory, since it has the same allocated size) is large enough is done in the child process, in DrawTargetForRemoteDrawing in part 2 of this bug.  Right now we just crash if the render size is too large to fit in the buffer.

> What deallocates gBufferMemory?

gBufferMemory and gGraphicsMemory are never freed.

> @@ +144,5 @@
> > +    }
> > +  }
> > +
> > +  JSObject* bufferObject =
> > +    JS_NewArrayBufferWithExternalContents(cx, width * height * 4, memory);
> 
> What's the life cycle of this guy? It hangs on to a buffer it doesn't own,
> is there a guarantee that it will be desroyed before the buffer?

The array buffer won't be destroyed until it is GC'ed, but since gGraphicsMemory and gBufferMemory are not freed this should be OK.

Comment 32

[Nicolas Silva [:nical]]

Comment on attachment 8990043 [details] [diff] [review]
Part 4 - Composite using a canvas in the middleman process.

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

I don't want to block the prototype on this, but leaking a large buffer (even if it's a one-time leak), is not something I'm super fond of in the long run. This patch set will need another review by anyone in the graphics team whenever we decide to turn WebReplay into an official feature that rides the trains.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e4f6f3985d0
Part 2 - Synchronously composite into a graphics shmem buffer when recording/replaying, r=nical.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ac654301741
Part 4 - Send compositor updates to the UI process from the middleman, r=mccr8,nical.

Comment 34

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7e4f6f3985d0
https://hg.mozilla.org/mozilla-central/rev/1ac654301741

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a243446f634c
Part 1 - Use a separate compositor in recording/replaying processes, r=nical.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8eb5cd519eda
Part 3 - When recording/replaying, composite in the main thread's event loop instead of waiting for vsyncs, r=nical.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d1551e0a5f2
Part 5 - Don't use APZ in recording/replaying processes, r=kats,mconley.

Comment 36

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a243446f634c
https://hg.mozilla.org/mozilla-central/rev/8eb5cd519eda
https://hg.mozilla.org/mozilla-central/rev/4d1551e0a5f2