View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0032979Open CASCADEOCCT:Data Exchangepublic2022-05-17 10:492022-08-05 21:15
Reporterkgv Assigned Tobugmaster  
PrioritynormalSeverityfeature 
Status reviewedResolutionopen 
Target Version7.7.0 
Summary0032979: Data Exchange, RWGltf_CafWriter - support multi-threaded Draco compression
DescriptionDraco compression might be time-consuming on large models. Multi-threaded encoding might be helpful.
TagsNo tags attached.
Test case number

Attached Files

  • performance.JPG (85,219 bytes)
  • performance_new.JPG (85,301 bytes)

Activities

msv

2022-05-18 17:20

developer   ~0108490

It might be useful to add the boolean option MultiThread to DracoParameters, and involve multi-threading only when it is on.

git

2022-07-26 14:08

administrator   ~0109999

Branch CR32979 has been created by ichesnokova.

SHA-1: fb2c70cc7076988c1ca16e4486742dffc9df8c5b


Detailed log of new commits:

Author: ichesnok
Date: Tue Jul 26 14:06:58 2022 +0300

    0032979: Data Exchange, RWGltf_CafWriter - support multi-threaded Draco compression
    
    'MultiThread' field was added to structure RWGltf_DracoParameters for using multithreading.
    Class CafWriter_DracoEncodingFunctor was added for multithreaded compression.

git

2022-07-27 09:49

administrator   ~0110013

Branch CR32979 has been updated forcibly by ichesnokova.

SHA-1: f89bdae5d445bbe9d3dc36ec0df08164472f6489

git

2022-07-28 08:53

administrator   ~0110028

Branch CR32979 has been updated forcibly by ichesnokova.

SHA-1: 927e4f5632b464d86801ba9623d2505a93b672ca

git

2022-07-28 14:34

administrator   ~0110055

Branch CR32979 has been updated forcibly by ichesnokova.

SHA-1: a00c3ac237e97f3d79b8dcdd87ca75fa619c282d

ichesnokova

2022-07-29 16:34

developer   ~0110075

Branch CR32979 is ready for review.
Tests: http://jenkins-test-occt.nnov.opencascade.com/view/CR32979-master-ichesnok/view/ALL/

kgv

2022-07-29 16:53

developer   ~0110078

@ichesnokova,
It could be useful adding at least one test case for verifying multi-threaded mode.

Attaching performance comparison results would be also interesting.

-    aDracoEncoder.SetSpeedOptions (myDracoParameters.CompressionLevel, myDracoParameters.CompressionLevel);
+    aDracoEncoder.SetSpeedOptions(myDracoParameters.CompressionLevel, myDracoParameters.CompressionLevel);
...
-        Message::SendFail (TCollection_AsciiString("File '") + myBinFileNameFull + "' cannot be written");
+        Message::SendFail(TCollection_AsciiString("File '") + myBinFileNameFull + "' cannot be written");

Please revert unrelated cosmetic changes and re-visit OCCT Coding Style suggestions (space is expected before opening bracket).

+  bool MultiThread;         //!< flag to use multithreading (FALSE by default)

Consider using consistent naming - see similar property in RWGltf_CafReader:
> Standard_Boolean myToParallel; //!< flag to use multithreading; FALSE by default
> .
> //! Return TRUE if multithreaded optimizations are allowed; FALSE by default.
> bool ToParallel() const { return myToParallel; }

Not sure if it is for good or bad putting this flag into Draco-specific options instead of a `RWGltf_CafWriter`, @msv please share your suggestion.

 
+class RWGltf_CafWriter::CafWriter_DracoEncodingFunctor
+{

Cosmetic description would be useful.

+    draco::Mesh aMesh;
+    if (aCurrentMesh->NodesVec.empty())
+    {
+      return;
+    }

It looks strange to declare an unused variable `aMesh` before termination condition.

+#include <OSD_ThreadPool.hxx>
 #include <OSD_Timer.hxx>

Alphabetical order is preferred ('i' precedes 'h').

msv

2022-07-29 17:15

developer   ~0110082

>Alphabetical order is preferred ('i' precedes 'h').
You are mistaking. 'h' precedes 'i'.

kgv

2022-07-29 17:21

developer   ~0110083

@msv, my bad.

msv

2022-08-01 11:53

developer   ~0110094

src/RWGltf/RWGltf_CafWriter.hxx
There is no necessity to put the class CafWriter_DracoEncodingFunctor into RWGltf_CafWriter. Please revert this change.

src/RWGltf/RWGltf_CafWriter.cxx
174: Name the class DracoEncodingFunctor and put it into unnamed namespace.
179: This parameter should be const.
212: You are using default constructor, then copy constructor. Simplify as:
    std::shared_ptr<draco::EncoderBuffer> anEncoderBuffer = std::make_shared<draco::EncoderBuffer>();

221: concurrent call of the method Next() will lead to data races. You must create a new progress range for each mesh item beforehand. Put mesh and progress range in a structure.
609: aPSentryBin is initialized only for 4 progress units, but now a new progress is added.
869: For this simple case it is better to use OSD_Parallel::For, like this:
    OSD_Parallel::For (0, int(aMeshes.size()), aFunctor, !myDracoParameters.MultiThread);

git

2022-08-02 10:44

administrator   ~0110099

Branch CR32979_1 has been created by ichesnokova.

SHA-1: acf484e0082e69758aa259fe83b84da752fb398c


Detailed log of new commits:

Author: ichesnok
Date: Tue Jul 26 14:06:58 2022 +0300

    0032979: Data Exchange, RWGltf_CafWriter - support multi-threaded Draco compression
    
    'MultiThread' field was added to structure RWGltf_DracoParameters for using multithreading.
    Class CafWriter_DracoEncodingFunctor was added for multithreaded compression.

ichesnokova

2022-08-02 14:07

developer   ~0110105

Branch CR32979_1 is ready for review.
Tests: http://jenkins-test-occt.nnov.opencascade.com/view/CR32979-master-ichesnok/view/COMPARE/
performance.JPG (85,219 bytes)

kgv

2022-08-02 14:17

developer   ~0110106

@ichesnokova,

you may put preformatted ASCII table directly into comment using HTML tag 'pre'.

msv

2022-08-02 18:38

developer   ~0110116

Please organize progressing like in the class BRepExtrema_DistShapeShape.cxx

git

2022-08-03 10:01

administrator   ~0110119

Branch CR32979_1 has been updated forcibly by ichesnokova.

SHA-1: dd9a6d2d3b2f7bef05a4c0e9bc9062f8cd95248f

ichesnokova

2022-08-03 13:40

developer   ~0110121

Branch CR32979_1 is ready for review.
Tests: http://jenkins-test-occt.nnov.opencascade.com/view/CR32979-master-ichesnok/view/COMPARE/
performance_new.JPG (85,301 bytes)

msv

2022-08-03 14:02

developer   ~0110122

This is not needed any more:
#include <OSD_ThreadPool.hxx>

kgv

2022-08-03 14:11

developer   ~0110123 

+  Standard_Boolean                              myToParallel;

New class field is left uninitialized

kgv

2022-08-03 14:15

developer   ~0110124 

+  Message_ProgressScope aScope(theProgress, "Write binary data", 2);

2 should be conditional based on draco on/off.

+    if (!aStatus.ok())
+    {
+      Message::SendFail (TCollection_AsciiString("Error: mesh cannot be encoded in draco buffer."));
+      return;

Failure should be propagated to the main interface.

git

2022-08-03 22:37

administrator   ~0110128

Branch CR32979_2 has been created by ichesnokova.

SHA-1: 21d070b115c03e98bc1b474211aaadfe60241b69


Detailed log of new commits:

Author: ichesnok
Date: Tue Jul 26 14:06:58 2022 +0300

    0032979: Data Exchange, RWGltf_CafWriter - support multi-threaded Draco compression
    
    'MultiThread' field was added to structure RWGltf_DracoParameters for using multithreading.
    Class CafWriter_DracoEncodingFunctor was added for multithreaded compression.

git

2022-08-04 08:13

administrator   ~0110129

Branch CR32979_2 has been updated forcibly by ichesnokova.

SHA-1: 403277678876c6d09ecff17ff5d2a8091e303577

ichesnokova

2022-08-04 17:54

developer   ~0110137

Branch CR32979_2 is ready for review.
Tests: http://jenkins-test-occt.nnov.opencascade.com/view/CR32979-master-ichesnok/view/ALL/

kgv

2022-08-05 09:50

developer   ~0110138

Last edited: 2022-08-05 09:51

 
+  : myProgress(theProgress, "Convert meshes to Draco buffer", Max(1, int(theMeshes.size()))),

Title looks unreasonably long to me. I would suggest "Draco compression" instead.

+      aBinFile->write ((*anEncoderBuffers.at(aBuffInd).get()).data(), std::streamsize((*anEncoderBuffers.at(aBuffInd).get()).size()));

const draco::EncoderBuffer& anEncBuff = *anEncoderBuffers.at(aBuffInd);
`aBinFile->write (anEncBuff.data(), std::streamsize(anEncBuff.size()));`

git

2022-08-05 10:54

administrator   ~0110139

Branch CR32979_2 has been updated forcibly by ichesnokova.

SHA-1: ab9b10435378b00e10bbd8356fd29f302a190444

ichesnokova

2022-08-05 16:40

developer   ~0110140

Branch CR32979_2 is ready for review.
Tests: http://jenkins-test-occt.nnov.opencascade.com/view/CR32979-master-ichesnok/view/ALL/

msv

2022-08-05 21:15

developer   ~0110141

For integration:
occt - CR32979_2
products - none

Issue History

Date Modified Username Field Change
2022-05-17 10:49 kgv New Issue
2022-05-17 10:49 kgv Assigned To => ichesnokova
2022-05-18 17:20 msv Note Added: 0108490
2022-05-18 17:21 msv Status new => assigned
2022-07-26 14:08 git Note Added: 0109999
2022-07-27 09:49 git Note Added: 0110013
2022-07-28 08:53 git Note Added: 0110028
2022-07-28 14:34 git Note Added: 0110055
2022-07-29 16:34 ichesnokova Note Added: 0110075
2022-07-29 16:34 ichesnokova Assigned To ichesnokova => msv
2022-07-29 16:34 ichesnokova Status assigned => resolved
2022-07-29 16:53 kgv Note Added: 0110078
2022-07-29 17:15 msv Note Added: 0110082
2022-07-29 17:21 kgv Note Added: 0110083
2022-08-01 11:53 msv Note Added: 0110094
2022-08-01 11:53 msv Assigned To msv => ichesnokova
2022-08-01 11:53 msv Status resolved => assigned
2022-08-02 10:44 git Note Added: 0110099
2022-08-02 14:07 ichesnokova Note Added: 0110105
2022-08-02 14:07 ichesnokova File Added: performance.JPG
2022-08-02 14:08 ichesnokova Assigned To ichesnokova => msv
2022-08-02 14:08 ichesnokova Status assigned => resolved
2022-08-02 14:17 kgv Note Added: 0110106
2022-08-02 18:38 msv Note Added: 0110116
2022-08-02 18:38 msv Assigned To msv => ichesnokova
2022-08-02 18:38 msv Status resolved => assigned
2022-08-03 10:01 git Note Added: 0110119
2022-08-03 13:40 ichesnokova Note Added: 0110121
2022-08-03 13:40 ichesnokova File Added: performance_new.JPG
2022-08-03 13:40 ichesnokova Assigned To ichesnokova => msv
2022-08-03 13:40 ichesnokova Status assigned => resolved
2022-08-03 14:02 msv Note Added: 0110122
2022-08-03 14:02 msv Assigned To msv => ichesnokova
2022-08-03 14:02 msv Status resolved => assigned
2022-08-03 14:11 kgv Note Added: 0110123
2022-08-03 14:15 kgv Note Added: 0110124
2022-08-03 22:37 git Note Added: 0110128
2022-08-04 08:13 git Note Added: 0110129
2022-08-04 17:54 ichesnokova Note Added: 0110137
2022-08-04 17:54 ichesnokova Assigned To ichesnokova => msv
2022-08-04 17:54 ichesnokova Status assigned => resolved
2022-08-05 09:50 kgv Note Added: 0110138
2022-08-05 09:51 kgv Note Edited: 0110138
2022-08-05 10:05 msv Assigned To msv => ichesnokova
2022-08-05 10:05 msv Status resolved => assigned
2022-08-05 10:54 git Note Added: 0110139
2022-08-05 16:40 ichesnokova Note Added: 0110140
2022-08-05 16:40 ichesnokova Assigned To ichesnokova => msv
2022-08-05 16:40 ichesnokova Status assigned => resolved
2022-08-05 21:15 msv Assigned To msv => bugmaster
2022-08-05 21:15 msv Status resolved => reviewed
2022-08-05 21:15 msv Note Added: 0110141
2022-08-08 08:48 ichesnokova Time allocated 8.8.2022: 166 h. => set