1035045 - [LayerScope] Send packets to the layerscope viewer by using google protocol buffer

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Boris Chiou [:boris]]

Use google protocol buffer to handle the package and then send to the viewer, and our viewer can use this 3rd-party library: https://github.com/dcodeIO/ProtoBuf.js

Comment 1

[Boris Chiou [:boris]]

Attachment: [WIP] Handle packets by the protocol buffer (v1)

Comment 2

[Boris Chiou [:boris]]

Attachment: Handle packets by the protocol buffer (v2)

Use google protocol buffer to handle our package in LayerScope.
Note: LayerScopePacket.pb.h and LayerScopePacket.pb.cc were
generated by version 2.4.1.

Comment 3

[Boris Chiou [:boris]]

HI Morris,

I added Google protocol buffer according to your previous patches. Could you please give me some feedback? Thanks.

Comment 4

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8451436 [details] [diff] [review]
Handle packets by the protocol buffer (v2)

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

Looks good to me. Excellent work!

::: gfx/layers/LayerScope.cpp
@@ +357,5 @@
>  };
>  
>  StaticAutoPtr<LayerScopeWebSocketManager> WebSocketHelper::sWebSocketManager;
>  
>  class DebugGLData : public LinkedListElement<DebugGLData> {

Maybe we should create a base class which only contains mDataType (maybe contains mContextAddress as well?) and let DebugGLData, DebugGLTextureData, DebugGLColorData derive this base class. Thus we don't need to fill TypePacket and send it in each derived class manually. The code will also looks more logical.

@@ +399,5 @@
>  
> +        uint32_t hSize = header.ByteSize();
> +        uint32_t pSize = packet.ByteSize();
> +        nsAutoArrayPtr<uint8_t> hData(new uint8_t[hSize]);
> +        nsAutoArrayPtr<uint8_t> pData(new uint8_t[pSize]);

I have no idea but does it have any method to remove those extra allocations?

Comment 5

[Boris Chiou [:boris]]

(In reply to Morris Tseng [:mtseng] from comment #4)
> Comment on attachment 8451436 [details] [diff] [review]
> Handle packets by the protocol buffer (v2)
> 
> Review of attachment 8451436 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me. Excellent work!
> 
> ::: gfx/layers/LayerScope.cpp
> @@ +357,5 @@
> >  };
> >  
> >  StaticAutoPtr<LayerScopeWebSocketManager> WebSocketHelper::sWebSocketManager;
> >  
> >  class DebugGLData : public LinkedListElement<DebugGLData> {
> 
> Maybe we should create a base class which only contains mDataType (maybe
> contains mContextAddress as well?) and let DebugGLData, DebugGLTextureData,
> DebugGLColorData derive this base class. Thus we don't need to fill
> TypePacket and send it in each derived class manually. The code will also
> looks more logical.
Good idea.

> 
> @@ +399,5 @@
> >  
> > +        uint32_t hSize = header.ByteSize();
> > +        uint32_t pSize = packet.ByteSize();
> > +        nsAutoArrayPtr<uint8_t> hData(new uint8_t[hSize]);
> > +        nsAutoArrayPtr<uint8_t> pData(new uint8_t[pSize]);
> 
> I have no idea but does it have any method to remove those extra allocations?

nsIOutputStream is different from the OutputStream in google::protobuf::io, so I cannot directly write the data to the nsIOutputStream from google protobuf. It looks like that we still need a temporary memory to hold the data(ex. an array). Actually, I really hate the extra memory allocation and copy.

Comment 6

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #5)
> (In reply to Morris Tseng [:mtseng] from comment #4)
> > Comment on attachment 8451436 [details] [diff] [review]
> > Handle packets by the protocol buffer (v2)
> > 
> > Review of attachment 8451436 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks good to me. Excellent work!
> > 
> > ::: gfx/layers/LayerScope.cpp
> > @@ +357,5 @@
> > >  };
> > >  
> > >  StaticAutoPtr<LayerScopeWebSocketManager> WebSocketHelper::sWebSocketManager;
> > >  
> > >  class DebugGLData : public LinkedListElement<DebugGLData> {
> > 
> > Maybe we should create a base class which only contains mDataType (maybe
> > contains mContextAddress as well?) and let DebugGLData, DebugGLTextureData,
> > DebugGLColorData derive this base class. Thus we don't need to fill
> > TypePacket and send it in each derived class manually. The code will also
> > looks more logical.
> Good idea.
> 
> > 
> > @@ +399,5 @@
> > >  
> > > +        uint32_t hSize = header.ByteSize();
> > > +        uint32_t pSize = packet.ByteSize();
> > > +        nsAutoArrayPtr<uint8_t> hData(new uint8_t[hSize]);
> > > +        nsAutoArrayPtr<uint8_t> pData(new uint8_t[pSize]);
> > 
> > I have no idea but does it have any method to remove those extra allocations?
> 
> nsIOutputStream is different from the OutputStream in google::protobuf::io,
> so I cannot directly write the data to the nsIOutputStream from google
> protobuf. It looks like that we still need a temporary memory to hold the
> data(ex. an array). Actually, I really hate the extra memory allocation and
> copy.

One possible way is to implement my own outputstream class which is inherited from nsIOutputStream and google::protobuf::io::ZeroCopyOutputStream, and I have to implement all the interfaces which ZeroCopyOutputStream needs.

Comment 7

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to Boris Chiou [:boris] from comment #6)
> (In reply to Boris Chiou [:boris] from comment #5)
> > (In reply to Morris Tseng [:mtseng] from comment #4)
> > > Comment on attachment 8451436 [details] [diff] [review]
> > > Handle packets by the protocol buffer (v2)
> > > 
> > > Review of attachment 8451436 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > @@ +399,5 @@
> > > >  
> > > > +        uint32_t hSize = header.ByteSize();
> > > > +        uint32_t pSize = packet.ByteSize();
> > > > +        nsAutoArrayPtr<uint8_t> hData(new uint8_t[hSize]);
> > > > +        nsAutoArrayPtr<uint8_t> pData(new uint8_t[pSize]);
> > > 
> > > I have no idea but does it have any method to remove those extra allocations?
> > 
> > nsIOutputStream is different from the OutputStream in google::protobuf::io,
> > so I cannot directly write the data to the nsIOutputStream from google
> > protobuf. It looks like that we still need a temporary memory to hold the
> > data(ex. an array). Actually, I really hate the extra memory allocation and
> > copy.
> 
> One possible way is to implement my own outputstream class which is
> inherited from nsIOutputStream and
> google::protobuf::io::ZeroCopyOutputStream, and I have to implement all the
> interfaces which ZeroCopyOutputStream needs.
Nice!! This should be work.

Comment 8

[Boris Chiou [:boris]]

Attachment: Handle packets by the protocol buffer (v3)

Use google protocol buffer to handle our package in LayerScope.
Note: LayerScopePacket.pb.h and LayerScopePacket.pb.cc were
generated by version 2.4.1.
In the patch, I reorganized the message structrure.

try result:
https://tbpl.mozilla.org/?tree=Try&rev=372351777f36

Comment 9

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #8)
> Created attachment 8453576 [details] [diff] [review]
> Handle packets by the protocol buffer (v3)
> 
> Use google protocol buffer to handle our package in LayerScope.
> Note: LayerScopePacket.pb.h and LayerScopePacket.pb.cc were
> generated by version 2.4.1.
> In the patch, I reorganized the message structrure.
> 
> try result:
> https://tbpl.mozilla.org/?tree=Try&rev=372351777f36

Reference viewer:
https://github.com/BorisChiou/layerscope/tree/1035045
http://people.mozilla.org/~bchiou/layerscope/layerview.html

Comment 10

[Boris Chiou [:boris]]

Comment on attachment 8453576 [details] [diff] [review]
Handle packets by the protocol buffer (v3)

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

::: toolkit/components/protobuf/moz.build
@@ +3,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +EXPORTS.google.protobuf += [

Google protocol buffer generates code automatically and the prefix of include paths is "google/protobuf/*", instead of "protobuf/google/protobuf/*".
ex: #include <google/protobuf/stubs/common.h> in *.pb.h

Therefore, I suggest we should revise this export structure.

Comment 11

[Boris Chiou [:boris]]

Comment on attachment 8453576 [details] [diff] [review]
Handle packets by the protocol buffer (v3)

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

::: gfx/layers/protobuf/LayerScopePacket.proto
@@ +35,5 @@
> +  required DataType type = 1;
> +
> +  optional FramePacket frame = 2;
> +  optional ColorPacket color = 3;
> +  optional TexturePacket texture = 4;

In order to implement the protocol buffer polymorphism, I think "extend" is better than "optional". However, "extend" is not fully supported in dcodeIO/Protobuf.js, so I use "optional" to achieve my purpose.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6808a2c6eab4

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6808a2c6eab4