Calling OpenGL from the renderer process.

Calling OpenGL from the renderer process
- Added ability for renderer processes to render to a real window (Windows only so far).
- Added ability to create offscreen frame buffer objects that can be resized later.
- OpenGL context can have a "parent" context that can access its last swapped back buffer through a texture ID.
- Moved code to establish GPU channel from RenderWidget to RenderThread.
- Changed way service size command buffer object lifetimes are managed.
TEST=trybot and visual verification that OpenGL can clear the browser window to magenta.
BUG=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=42679

[apatrick_chromium, 2010-03-19 22:51:42]

This still needs quite a bit of work but I would appreciate some early feedback.
It doesn't even build on linux and mac yet. Though, I have been able to clear
the render view to magenta using glClear() from the renderer process on windows.

Suggest gman looks at stuff under gpu/ and kbr and darin look at stuff under
chrome/.

darin, can you please pay particular attention to the code that attempts to
prevent a renderer process from issuing arbitrary OpenGL calls to any window on
the desktop :) It's in chrome/gpu/gpu_channel.cc and
chrome/browser/renderer_host/render_widget_host_view_win.cc. I'm sure there's a
better way of approaching this.

Thanks.

[greggman, 2010-03-22 02:30:59]

http://codereview.chromium.org/1136006/diff/21002/35009
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/1136006/diff/21002/35009#newcode918
gpu/command_buffer/service/gles2_cmd_decoder.cc:918: DCHECK(id_ == 0);
DCHECK_EQ?

http://codereview.chromium.org/1136006/diff/21002/35009#newcode938
gpu/command_buffer/service/gles2_cmd_decoder.cc:938: // TODO(apatrick): The
OpenGL spec doesn't actually document that data can
It's in the spec
(http://www.opengl.org/documentation/specs/version2.1/glspec21.pdf) section
3.8.1, last paragraph.

http://codereview.chromium.org/1136006/diff/21002/35009#newcode956
gpu/command_buffer/service/gles2_cmd_decoder.cc:956: DCHECK(id_ != 0);
DCHECK_NE?

http://codereview.chromium.org/1136006/diff/21002/35009#newcode984
gpu/command_buffer/service/gles2_cmd_decoder.cc:984: DCHECK(id_ == 0);
DCHECK_EQ?

http://codereview.chromium.org/1136006/diff/21002/35009#newcode1020
gpu/command_buffer/service/gles2_cmd_decoder.cc:1020: DCHECK(id_ == 0);
DCHECK_EQ?

Here an below.  I know some versions of google's lint will warn about these.

http://codereview.chromium.org/1136006/diff/21002/35009#newcode1059
gpu/command_buffer/service/gles2_cmd_decoder.cc:1059: glClear(buffers);
Do you need to call glScissor, glClearColor, glClearDepthf, glClearStencil,
glColorMask, glDepthMask, glStencilMask before glClear? and of course restore
them?

It's fine to do that in another CL and I'm happy to do the wrapping. Just add a
TODO.

Alternatively, we could create some other GL context for the sole purpose of
keeping it's own state and use that context to clear.

http://codereview.chromium.org/1136006/diff/21002/35009#newcode2607
gpu/command_buffer/service/gles2_cmd_decoder.cc:2607: memset(pixels, 7,
pixels_size);
Is this some test left over?

http://codereview.chromium.org/1136006/diff/21002/35009#newcode3427
gpu/command_buffer/service/gles2_cmd_decoder.cc:3427: if (bound_framebuffer_ !=
0)
Is this check correct? I would think it would be safe to call SwapBuffers even
if bound_framebuffer_ is not zero.  

It seems valid GL to do 

--Render To backbuffer 
--Bind to user FBO
--Swap Buffers (will take the backbuffer, not the fbo)

[apatrick_chromium, 2010-03-22 20:30:58]

http://codereview.chromium.org/1136006/diff/21002/35009
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/1136006/diff/21002/35009#newcode938
gpu/command_buffer/service/gles2_cmd_decoder.cc:938: // TODO(apatrick): The
OpenGL spec doesn't actually document that data can
On 2010/03/22 02:30:59, greggman wrote:
> It's in the spec
> (http://www.opengl.org/documentation/specs/version2.1/glspec21.pdf) section
> 3.8.1, last paragraph.

Done.

http://codereview.chromium.org/1136006/diff/21002/35009#newcode956
gpu/command_buffer/service/gles2_cmd_decoder.cc:956: DCHECK(id_ != 0);
On 2010/03/22 02:30:59, greggman wrote:
> DCHECK_NE?

Done.

http://codereview.chromium.org/1136006/diff/21002/35009#newcode984
gpu/command_buffer/service/gles2_cmd_decoder.cc:984: DCHECK(id_ == 0);
On 2010/03/22 02:30:59, greggman wrote:
> DCHECK_EQ?

Done.

http://codereview.chromium.org/1136006/diff/21002/35009#newcode1020
gpu/command_buffer/service/gles2_cmd_decoder.cc:1020: DCHECK(id_ == 0);
On 2010/03/22 02:30:59, greggman wrote:
> DCHECK_EQ?
> 
> Here an below.  I know some versions of google's lint will warn about these.

Done.

http://codereview.chromium.org/1136006/diff/21002/35009#newcode1059
gpu/command_buffer/service/gles2_cmd_decoder.cc:1059: glClear(buffers);
That's up to the caller. In this case the caller uses a special FBO with known
state that is not exposed to the client to ensure that the scissor rect, clear
color, etc are appropriate.

http://codereview.chromium.org/1136006/diff/21002/35009#newcode2607
gpu/command_buffer/service/gles2_cmd_decoder.cc:2607: memset(pixels, 7,
pixels_size);
Yes, done.

http://codereview.chromium.org/1136006/diff/21002/35009#newcode3427
gpu/command_buffer/service/gles2_cmd_decoder.cc:3427: if (bound_framebuffer_ !=
0)
On 2010/03/22 02:30:59, greggman wrote:
> Is this check correct? I would think it would be safe to call SwapBuffers
even
> if bound_framebuffer_ is not zero.  
> 
> It seems valid GL to do 
> 
> --Render To backbuffer 
> --Bind to user FBO
> --Swap Buffers (will take the backbuffer, not the fbo)
> 

Done.

[greggman, 2010-03-23 02:09:28]

LGTM

http://codereview.chromium.org/1136006/diff/21002/35009
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/1136006/diff/21002/35009#newcode1059
gpu/command_buffer/service/gles2_cmd_decoder.cc:1059: glClear(buffers);
On 2010/03/22 20:30:59, apatrick_chromium wrote:
> That's up to the caller. In this case the caller uses a special FBO with known
> state that is not exposed to the client to ensure that the scissor rect, clear
> color, etc are appropriate.


I wasn't aware those states are per FBO. Thought they were global states. I'm
99% sure they are as I had to fix glScissor issues with RenderSurface in O3D but
my memory could be faulty.

[apatrick_chromium, 2010-03-23 18:43:26]

darin, kbr: ready for review of the rest of the code under chrome/.

http://codereview.chromium.org/1136006/diff/21002/35009
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/1136006/diff/21002/35009#newcode1059
gpu/command_buffer/service/gles2_cmd_decoder.cc:1059: glClear(buffers);
The docs seem ambiguous. I'll just have to try it.

[Ken Russell (switch to Gerrit), 2010-03-23 20:40:05]

Generally looks good. A few comments. You'll also need to merge with
http://codereview.chromium.org/1095005 which was just checked in.

http://codereview.chromium.org/1136006/diff/40004/9023
File chrome/common/gpu_messages_internal.h (right):

http://codereview.chromium.org/1136006/diff/40004/9023#newcode110
chrome/common/gpu_messages_internal.h:110: int32, /* parent_route_id */
Could you document the parent_route_id and parent_texture_id parameters better
to indicate how their optional nature works? I hope that having a parent is not
strictly required, at least in the initial implementation -- otherwise
bootstrapping WebGL on the Mac will break.

Also, could you group the parameters better? Perhaps width, height,
parent_route_id, parent_texture_id, route_id or something similar. This has
implications for APIs all the way down to the GPU process code, unfortunately.

http://codereview.chromium.org/1136006/diff/40004/9046
File chrome/gpu/gpu_thread.h (right):

http://codereview.chromium.org/1136006/diff/40004/9046#newcode42
chrome/gpu/gpu_thread.h:42: typedef base::hash_map<int,
scoped_refptr<GpuChannel> > GpuChannelMap;
int32 to match gpu_channel.h?

http://codereview.chromium.org/1136006/diff/40004/9035
File chrome/renderer/ggl/ggl.h (right):

http://codereview.chromium.org/1136006/diff/40004/9035#newcode42
chrome/renderer/ggl/ggl.h:42: // contexts frame buffer that is updated every
time SwapBuffers is called. It is
contexts -> context's

http://codereview.chromium.org/1136006/diff/40004/9035#newcode59
chrome/renderer/ggl/ggl.h:59: // respect to the parent context.
Specify return value if parent context is NULL.

http://codereview.chromium.org/1136006/diff/40004/9011
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/1136006/diff/40004/9011#newcode1535
gpu/command_buffer/service/gles2_cmd_decoder.cc:1535: if (pending_width_ <= 0 ||
pending_width_ <= 0) {
Second pending_width_ should be pending_height_, but the logic is wrong. If both
are > 0 then neither is <= 0.

http://codereview.chromium.org/1136006/diff/40004/9011#newcode1593
gpu/command_buffer/service/gles2_cmd_decoder.cc:1593: #elif defined(OS_LINUX)
Please add TODO to implement sharing offscreen context with parent.

http://codereview.chromium.org/1136006/diff/40004/9011#newcode1597
gpu/command_buffer/service/gles2_cmd_decoder.cc:1597: #elif defined(OS_MACOSX)
Please add TODO to think about how sharing with parent will work on this
platform.

http://codereview.chromium.org/1136006/diff/40004/9007
File gpu/command_buffer/service/gles2_cmd_decoder.h (right):

http://codereview.chromium.org/1136006/diff/40004/9007#newcode86
gpu/command_buffer/service/gles2_cmd_decoder.h:86: // Set the initial size of or
resize an offscreen frame buffer.
This comment seems out of date now that the initial size is specified during
initialization.

[apatrick_chromium, 2010-03-23 21:48:35]

http://codereview.chromium.org/1136006/diff/40004/9023
File chrome/common/gpu_messages_internal.h (right):

http://codereview.chromium.org/1136006/diff/40004/9023#newcode110
chrome/common/gpu_messages_internal.h:110: int32, /* parent_route_id */
Done. Yes an offscreen context can be used without a parent context.

http://codereview.chromium.org/1136006/diff/40004/9046
File chrome/gpu/gpu_thread.h (right):

http://codereview.chromium.org/1136006/diff/40004/9046#newcode42
chrome/gpu/gpu_thread.h:42: typedef base::hash_map<int,
scoped_refptr<GpuChannel> > GpuChannelMap;
On 2010/03/23 20:40:05, kbr wrote:
> int32 to match gpu_channel.h?
> 

The convention in Chrome appears to be to use int for renderer IDs and int32 for
IPC routing IDs. This is not entirely consistent though. Trying to not make
things worse!

http://codereview.chromium.org/1136006/diff/40004/9035
File chrome/renderer/ggl/ggl.h (right):

http://codereview.chromium.org/1136006/diff/40004/9035#newcode42
chrome/renderer/ggl/ggl.h:42: // contexts frame buffer that is updated every
time SwapBuffers is called. It is
On 2010/03/23 20:40:05, kbr wrote:
> contexts -> context's
> 

Done.

http://codereview.chromium.org/1136006/diff/40004/9035#newcode59
chrome/renderer/ggl/ggl.h:59: // respect to the parent context.
On 2010/03/23 20:40:05, kbr wrote:
> Specify return value if parent context is NULL.
> 

Done.

http://codereview.chromium.org/1136006/diff/40004/9011
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/1136006/diff/40004/9011#newcode1535
gpu/command_buffer/service/gles2_cmd_decoder.cc:1535: if (pending_width_ <= 0 ||
pending_width_ <= 0) {
On 2010/03/23 20:40:05, kbr wrote:
> Second pending_width_ should be pending_height_, but the logic is wrong. If
both
> are > 0 then neither is <= 0.
> 

Done.

http://codereview.chromium.org/1136006/diff/40004/9011#newcode1593
gpu/command_buffer/service/gles2_cmd_decoder.cc:1593: #elif defined(OS_LINUX)
On 2010/03/23 20:40:05, kbr wrote:
> Please add TODO to implement sharing offscreen context with parent.
> 

Done.

http://codereview.chromium.org/1136006/diff/40004/9011#newcode1597
gpu/command_buffer/service/gles2_cmd_decoder.cc:1597: #elif defined(OS_MACOSX)
On 2010/03/23 20:40:05, kbr wrote:
> Please add TODO to think about how sharing with parent will work on this
> platform.
> 

Done.

http://codereview.chromium.org/1136006/diff/40004/9007
File gpu/command_buffer/service/gles2_cmd_decoder.h (right):

http://codereview.chromium.org/1136006/diff/40004/9007#newcode86
gpu/command_buffer/service/gles2_cmd_decoder.h:86: // Set the initial size of or
resize an offscreen frame buffer.
On 2010/03/23 20:40:05, kbr wrote:
> This comment seems out of date now that the initial size is specified during
> initialization.
> 

Done.

[Ken Russell (switch to Gerrit), 2010-03-23 22:15:23]

LGTM

http://codereview.chromium.org/1136006/diff/83001/84009
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/1136006/diff/83001/84009#newcode1599
gpu/command_buffer/service/gles2_cmd_decoder.cc:1599: DCHECK(!offscreen);
Since I think there is a check higher up about "windowed mode" initialization
not being implemented on non-Windows platforms yet, I have a feeling GPU process
initialization is going to be broken on non-Windows platforms. If this seems
likely then please send an email to people likely to be affected so we are all
aware of the potential breakage and can patch it up as quickly as possible.

[apatrick_chromium, 2010-03-23 22:24:59]

+dspringer
+pinkerton

No need to review everything. Please let me know if you see anything that will
break mac. My expectation is that the existing GPU plugin will continue to work
on mac with this change but some work will be needed to get this new non-plugin
based model working on mac.

[greggman, 2010-03-24 08:49:02]

http://codereview.chromium.org/1136006/diff/21002/35009
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/1136006/diff/21002/35009#newcode1059
gpu/command_buffer/service/gles2_cmd_decoder.cc:1059: glClear(buffers);
On 2010/03/23 18:43:26, apatrick_chromium wrote:
> The docs seem ambiguous. I'll just have to try it.

I just tried it. The settings are global. This only gets called once in a while
right (init and resize?)   In that case it would be best just to query the
values and then restore. 

For glScissor you can just use glDisable(GL_SCISSOR_TEST) and restore that
instead of changing the scissor setting.

[darin (slow to review), 2010-03-24 15:03:48]

This is really cool stuff.  Just some minor things:

http://codereview.chromium.org/1136006/diff/83001/84042
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/1136006/diff/83001/84042#newcode302
chrome/browser/renderer_host/render_widget_host_view_win.cc:302:
L"ChromiumRendererId",
perhaps this string should be moved to chrome/common/chrome_constants.h since it
is hardcoded here and elsewhere.

http://codereview.chromium.org/1136006/diff/83001/84025
File chrome/gpu/gpu_channel.cc (right):

http://codereview.chromium.org/1136006/diff/83001/84025#newcode141
chrome/gpu/gpu_channel.cc:141: int view_renderer_id = reinterpret_cast<int>(
nice.  we might want to get aggressive toward a renderer that sends a bogus IPC.
 from the browser process, if we receive a malformed message, we have code that
will kill the renderer.  you might want to add a TODO about doing something
similar here.  it would be good if we had a facility in the GPU process to catch
malformed input from a renderer and take the action of killing the renderer in
such cases.

http://codereview.chromium.org/1136006/diff/83001/84025#newcode171
chrome/gpu/gpu_channel.cc:171: DCHECK(it != stubs_.end());
is there any kind of race condition here?  what if the parent is being
destroyed?  i admit i haven't fully thought this case through...

http://codereview.chromium.org/1136006/diff/83001/84043
File chrome/gpu/gpu_thread.cc (right):

http://codereview.chromium.org/1136006/diff/83001/84043#newcode61
chrome/gpu/gpu_thread.cc:61: // TODO(apatrick): figure out when to remove
channels from the map. They
we should be able to monitor our IPC channel between the GPU process and the
renderer.  when we see the pipe close, we can then cleanup this map.  take a
look at IPC::ChannelProxy::MessageFilter.

http://codereview.chromium.org/1136006/diff/83001/84035
File chrome/renderer/command_buffer_proxy.cc (right):

http://codereview.chromium.org/1136006/diff/83001/84035#newcode186
chrome/renderer/command_buffer_proxy.cc:186: void
CommandBufferProxy::ResizeOffscreenFrameBuffer(int width, int height) {
same comment about using gfx::Size

http://codereview.chromium.org/1136006/diff/83001/84034
File chrome/renderer/ggl/ggl.cc (right):

http://codereview.chromium.org/1136006/diff/83001/84034#newcode111
chrome/renderer/ggl/ggl.cc:111: DCHECK(width >= 0 && height >= 0);
gfx::Size has an IsEmpty method that can be used here.

http://codereview.chromium.org/1136006/diff/83001/84033
File chrome/renderer/ggl/ggl.h (right):

http://codereview.chromium.org/1136006/diff/83001/84033#newcode48
chrome/renderer/ggl/ggl.h:48: int width,
same nit about using gfx::Size

http://codereview.chromium.org/1136006/diff/83001/84033#newcode56
chrome/renderer/ggl/ggl.h:56: void ResizeOffscreenContext(Context* context, int
width, int height);
ditto

http://codereview.chromium.org/1136006/diff/83001/84032
File chrome/renderer/gpu_channel_host.h (right):

http://codereview.chromium.org/1136006/diff/83001/84032#newcode60
chrome/renderer/gpu_channel_host.h:60: int width,
nit: how about using gfx::Size when we need to pass around a (width, height)
pair?  you can directly pass gfx::Size over an IPC message too.

http://codereview.chromium.org/1136006/diff/83001/84030
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/1136006/diff/83001/84030#newcode699
chrome/renderer/render_thread.cc:699: CHECK(Send(new
ViewHostMsg_EstablishGpuChannel()));
Send can fail if we are in the process of shutting down.

http://codereview.chromium.org/1136006/diff/83001/84038
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/1136006/diff/83001/84038#newcode406
chrome/renderer/render_widget.cc:406: //   
RenderThread::current()->EstablishGpuChannel();
so in this case, we would simply not paint?  i assume there would be some signal
later on that indicates when we should try to paint?  RenderWidget::PaintRect
calls only come in if there was some damage initiated by WebKit or the embedder.
 we can't count on that happening in a timely fashion though.

http://codereview.chromium.org/1136006/diff/83001/84038#newcode409
chrome/renderer/render_widget.cc:409: //  ggl::Context* view_context =
ggl::CreateViewContext(channel,
wow, this is pretty exciting.  so, if we can use GL directly like this, then
perhaps it means that we could greatly simplify the WebGraphicsContext3D
interface to merely provide bridging to GGL.  that would mean that we could
allow code in WebKit to play with OpenGL directly much as you are proposing down
below.  is that something you had considered?

[apatrick_chromium, 2010-03-25 19:43:08]

Ready for round 2.

http://codereview.chromium.org/1136006/diff/83001/84042
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/1136006/diff/83001/84042#newcode302
chrome/browser/renderer_host/render_widget_host_view_win.cc:302:
L"ChromiumRendererId",
On 2010/03/24 15:03:49, darin wrote:
> perhaps this string should be moved to chrome/common/chrome_constants.h since
it
> is hardcoded here and elsewhere.

Done.

http://codereview.chromium.org/1136006/diff/83001/84025
File chrome/gpu/gpu_channel.cc (right):

http://codereview.chromium.org/1136006/diff/83001/84025#newcode141
chrome/gpu/gpu_channel.cc:141: int view_renderer_id = reinterpret_cast<int>(
On 2010/03/24 15:03:49, darin wrote:
> nice.  we might want to get aggressive toward a renderer that sends a bogus
IPC.
>  from the browser process, if we receive a malformed message, we have code
that
> will kill the renderer.  you might want to add a TODO about doing something
> similar here.  it would be good if we had a facility in the GPU process to
catch
> malformed input from a renderer and take the action of killing the renderer
in
> such cases.

Done.

http://codereview.chromium.org/1136006/diff/83001/84025#newcode171
chrome/gpu/gpu_channel.cc:171: DCHECK(it != stubs_.end());
The GPU process is single threaded.

http://codereview.chromium.org/1136006/diff/83001/84043
File chrome/gpu/gpu_thread.cc (right):

http://codereview.chromium.org/1136006/diff/83001/84043#newcode61
chrome/gpu/gpu_thread.cc:61: // TODO(apatrick): figure out when to remove
channels from the map. They
Thanks. I'll do that in a future CL.

http://codereview.chromium.org/1136006/diff/83001/84035
File chrome/renderer/command_buffer_proxy.cc (right):

http://codereview.chromium.org/1136006/diff/83001/84035#newcode186
chrome/renderer/command_buffer_proxy.cc:186: void
CommandBufferProxy::ResizeOffscreenFrameBuffer(int width, int height) {
On 2010/03/24 15:03:49, darin wrote:
> same comment about using gfx::Size

Done.

http://codereview.chromium.org/1136006/diff/83001/84034
File chrome/renderer/ggl/ggl.cc (right):

http://codereview.chromium.org/1136006/diff/83001/84034#newcode111
chrome/renderer/ggl/ggl.cc:111: DCHECK(width >= 0 && height >= 0);
On 2010/03/24 15:03:49, darin wrote:
> gfx::Size has an IsEmpty method that can be used here.

The semantics are not the same.

http://codereview.chromium.org/1136006/diff/83001/84032
File chrome/renderer/gpu_channel_host.h (right):

http://codereview.chromium.org/1136006/diff/83001/84032#newcode60
chrome/renderer/gpu_channel_host.h:60: int width,
On 2010/03/24 15:03:49, darin wrote:
> nit: how about using gfx::Size when we need to pass around a (width, height)
> pair?  you can directly pass gfx::Size over an IPC message too.

Done.

http://codereview.chromium.org/1136006/diff/83001/84030
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/1136006/diff/83001/84030#newcode699
chrome/renderer/render_thread.cc:699: CHECK(Send(new
ViewHostMsg_EstablishGpuChannel()));
On 2010/03/24 15:03:49, darin wrote:
> Send can fail if we are in the process of shutting down.

Done.

http://codereview.chromium.org/1136006/diff/83001/84038
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/1136006/diff/83001/84038#newcode406
chrome/renderer/render_widget.cc:406: //   
RenderThread::current()->EstablishGpuChannel();
kbr's upcoming CL has a solution for establishing a channel synchronously.

http://codereview.chromium.org/1136006/diff/83001/84038#newcode409
chrome/renderer/render_widget.cc:409: //  ggl::Context* view_context =
ggl::CreateViewContext(channel,
I would like to do that.

http://codereview.chromium.org/1136006/diff/83001/84009
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/1136006/diff/83001/84009#newcode1599
gpu/command_buffer/service/gles2_cmd_decoder.cc:1599: DCHECK(!offscreen);
The existing GPU plugin model should continue to work. That doesn't count as
"offscreen" as far as this assertion is concerned, even on mac.

[darin (slow to review), 2010-03-25 23:47:08]

I have no further comments.  LGTM2