Closed Bug 1035045 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Graphics, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: boris, Assigned: boris)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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
Blocks: 959118
Attachment #8451436 - Flags: feedback?(mtseng)
HI Morris,

I added Google protocol buffer according to your previous patches. Could you please give me some feedback? Thanks.
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?
Attachment #8451436 - Flags: feedback?(mtseng) → feedback+
(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.
(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.
(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.
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
Attachment #8451436 - Attachment is obsolete: true
(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
Attachment #8453576 - Flags: review?(dglastonbury)
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 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.
Attachment #8453576 - Flags: review?(dglastonbury) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6808a2c6eab4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.