Data Exchange, RWGltf_CafWriter - support multi-threaded Draco compression

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

Branch [archived branch] has been created by Participant.

[revision removed]


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.

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

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

Commenter 2,
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`, Commenter 3 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').

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

Commenter 3, my bad.

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);

Branch [archived branch] has been created by Participant.

[revision removed]


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.

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

Commenter 2,

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

Please organize progressing like in the class BRepExtrema_DistShapeShape.cxx

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

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

This is not needed any more:

#include <OSD_ThreadPool.hxx>

+  Standard_Boolean                              myToParallel; 

New class field is left uninitialized

+  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.

Branch [archived branch] has been created by Participant.

[revision removed]


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.

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

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

+  : 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()));`

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

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

For integration:
occt - CR32979_2
products - none

Compilation problem with the USE_DRACO=OFF parameter

/dn61/builds/CR0-WEEK-32_CR0-WEEK-32/OCCT_SRC/src/RWGltf/RWGltf_CafWriter.cxx:180:25: error: use of undeclared identifier 'draco'
                        draco::Encoder& theDracoEncoder,
                        ^

/dn61/builds/CR0-WEEK-32_CR0-WEEK-32/OCCT_SRC/src/RWGltf/RWGltf_CafWriter.cxx:182:53: error: use of undeclared identifier 'draco'
                        std::vector<std::shared_ptr<draco::EncoderBuffer>>& theEncoderBuffers)
                                                    ^

/dn61/builds/CR0-WEEK-32_CR0-WEEK-32/OCCT_SRC/src/RWGltf/RWGltf_CafWriter.cxx:231:37: error: expected a type
  Message_ProgressScope myProgress;
                                    ^

/dn61/builds/CR0-WEEK-32_CR0-WEEK-32/OCCT_SRC/src/RWGltf/RWGltf_CafWriter.cxx:231:37: error: expected ')'
/dn61/builds/CR0-WEEK-32_CR0-WEEK-32/OCCT_SRC/src/RWGltf/RWGltf_CafWriter.cxx:179:24: note: to match this '('
  DracoEncodingFunctor (const Message_ProgressRange& theProgress,
                       ^

/dn61/builds/CR0-WEEK-32_CR0-WEEK-32/OCCT_SRC/src/RWGltf/RWGltf_CafWriter.cxx:232:3: error: use of undeclared identifier 'draco'
  draco::Encoder* myDracoEncoder;
  ^

/dn61/builds/CR0-WEEK-32_CR0-WEEK-32/OCCT_SRC/src/RWGltf/RWGltf_CafWriter.cxx:235:31: error: use of undeclared identifier 'draco'
  std::vector<std::shared_ptr<draco::EncoderBuffer>>* myEncoderBuffers;
                              ^

/dn61/builds/CR0-WEEK-32_CR0-WEEK-32/OCCT_SRC/src/RWGltf/RWGltf_CafWriter.cxx:235:79: error: expected a type
  std::vector<std::shared_ptr<draco::EncoderBuffer>>* myEncoderBuffers;
                                                                              ^

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

Combination -
OCCT branch : [archived branch]
master SHA - [revision removed]
[revision removed]
Products branch : [archived branch] SHA - changes and them, and you can discard any commits you make in this
[revision removed]
was compiled on Linux, MacOS and Windows platforms and tested in optimize mode.

Number of compiler warnings:
No new/fixed warnings

Regressions/Differences/Improvements:
No regressions/differences

CPU differences:
Debian80-64:
OCCT
Total CPU difference: 18719.310000000743 / 18726.600000000624 [-0.04%]
Products
Total CPU difference: 11976.710000000106 / 11968.290000000105 [+0.07%]
Windows-64-VC14:
OCCT
Total CPU difference: 20763.5 / 20741.5 [+0.11%]
Products
Total CPU difference: 13535.640625 / 13528.765625 [+0.05%]


Image differences :
No differences that require special attention

Memory differences :
No differences that require special attention

Branch [archived branch] has been deleted by Participant.

[revision removed]

Branch [archived branch] has been deleted by Participant.

[revision removed]

Branch [archived branch] has been deleted by Participant.

[revision removed]